fix billboard subregion not displayed

[Beilinson]

Description

As per #12585, image subregions currently do not work since 1.127. The cause was a conditionless addImageSubregion call on the BillboardVisualizer every frame prior to rendering, which resulted in a large promise queue but also made sure that image.state was LOADING for every render.

This PR adds a comparison against the previous loading subRegion, which skips adding a new subregion if not needed, letting the first call resolve properly and set the state to LOADED.

Issue number and link

#12585

Testing plan

Sandboxes from #12585

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @Beilinson!

✅ We can confirm we have a CLA on file for you.

[Beilinson]

this basically doubles all BoundingRectangle objects added to the scene. I tried making this test against the internal width/height, but those are only set after the subRegion promise resolves so that wont work.

[mzschwartz5]

I notice that a billboard's imageSubRegion is created as a constant property (based on the lack of the createPropertyCallback arg passed in to createPropertyDescriptor).

Since the property never changes, maybe the right move here is to remove this code from update entirely and find another home for it? (Like a constructor or some setter - something that's only called once, not every frame).

[Beilinson]

Interesting, I'll try to put something together tmrw

[Beilinson]

createProperty creates a setter that uses the value directly if it is a Property, so users can for example do:

billboard.imageSubRegion = new CallbackProperty(...);

And then createProperty will set the internal value to actually be a CallbackProperty, so I dont think what you wrote can work here, correct me if I'm wrong.

[mzschwartz5]

Ooh I see what you're saying. So the the value of the property is constant, but the reference to a property can change, essentially.

I agree, that invalidates my idea. Follow up question, though: how does this "double all BoundingRectangle objects added to the scene`? Every time you clone, GC (eventually) deletes the old one.

[Beilinson]

Because I'm using BoundingRectangle.clone all subRegions exist twice: once on the BillboardGraphics, and once as a deep clone on the item in the BillboardVisualizer, to be used for equality comparison. The one on the BillboardGraphics isn't GC'ed since the reference to the graphics is never lost (until the graphics is removed of course)

I don't think this is a significant issue, just wanted to point that out for review in case there was a better solution

[mzschwartz5]

Going back to the original idea for a second, I do think something like this could work. E.g. subscribe to the definition change in the constructor.

But... it also means there's now an event listener listening to every property change on a billboard and filtering down to just imageSubRegion based on name. That would be in exchange for checking BoundingRectangle equality on every Update though, which maybe is worth it.

[Beilinson]

Deleted because these are unused anywhere else in the code