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]

@mzschwartz5 this is ready for re-review

[mzschwartz5]

I need to look at this more tomorrow, but my initial reaction is that this still seems like a bandaid. We already have state that tells us the subregion has been added already. We should find a way to use it.

Like (spitballing), maybe we can defer setting:
this._loadState = BillboardLoadState.LOADING;

until we've called some function on atlas to check if the subregion already exists. Basically splitting up TextureAtlas.prototype.addImageSubRegion into addImageSubRegion and checkSubRegionExists or something...?

[Beilinson]

I think that might not be a bad idea however that still wouldn't fix the problem, the checkSubRegionExists method would still return a promise to an index which could still only resolve on the next frame, and if this check is initiated every frame it would never render properly.

I've setup an alternative version of the code which now works how you suggested (split up syncronous check vs asyncronous addSubRegion), it seems to work as well, not sure how much I like this I think the previous implementation is clearer but its more or less the same thing just moving this check further up or down the code structure (actually this also changes some stuff with loadState like you suggested)

Image from newest state showing that it still works

[Beilinson]

@mzschwartz5 this is again ready for re-review (will be available tmrw for any additional convo on this). Please compare the latest commit (using your suggestions) against the older version (first 7 commits).

[Beilinson]

I personally think the older version is better - smaller footprint, slightly more understandable process, slightly better theoretical performance. The only real downside from my perspective is slightly worse theoretical memory usage, but its only reverting it to the same level that was before #12495

[plainheart]

Could this fix be included in the upcoming v1.135?

[mzschwartz5]

Could this fix be included in the upcoming v1.135?

@plainheart. We can try. It's a simple PR, but I'm still not satisfied with our understanding and just asked for a little follow up on my recent findings.

The release is in 2 days, so I'll keep an eye on this.

[Beilinson]

This is a race condition that is handled by loadImage but wasn't handled by addImageSubRegion before

[Beilinson]

Sandcastle with CallbackProperty for imageSubRegion showing that the new method works

[Beilinson]

Do note that unlike my previous implementation, if there are many (thousands+) of subregions then this could get pretty slow, since this is checked by all billboards with subRegions every frame. This could be solved by creating a new BidirectionalMap core class (which would store an additional inverse Map), and using it instead of the regular map for this _subRegions map. I don't think this is needed for this bug fix, but is a possible future improvement if this implementation is chosen over my previous one.