Fix image subRegion... again... #13072

[Beilinson]

Description

In #12958, I fixed subRegions for entity billboards. I however ended up missing that my implementation didn't fix (or even caused a new bug I'm not sure) when adding billboards directly using a BillboardCollection.

The issue stems from a change that was fundamental to fixing subRegions for entities: because the subRegion is set every frame, and this used to always await the image index promise, the subRegion ended up being in the LOADING state every frame. The fix was to allow the subRegion to resolve synchronously , and even better return synchronously if the image had also already resolved.

However this causes an issue when using regular billboards, since those are instantiated once and the properties are set only once. If the image has already loaded, then the subRegion will synchronously run, and the image will resolve a frame after due to the Promise.resolve + await combo that is removed by this PR.

An alternative fix exists:

When creating a new Billboard, chain the addImageSubRegion call to the promise returned by loadImage:

// Billboard.js line 203
if (defined(image)) {
    this._computeImageTextureProperties(options.imageId, image);
    const imagePromise = this._imageTexture.loadImage(
      this._imageId,
      image,
      this._imageWidth,
      this._imageHeight,
    );
    if (defined(options.imageSubRegion)) {
      imagePromise.then(() => this._imageTexture.addImageSubRegion(this._imageId, options.imageSubRegion));
    }
  }

This solution is less safe though in my opinion, if we are treating the image as ready synchronously within the TextureAtlas, as well as treating the subRegion in that way, we should continue this trend throughout the BillboardTexture code as well, rather than having Billboard assume that in a certain edge case addImageSubRegion will run before loadImage because of promise shenanigans.

Issue number and link

#13072

Testing plan

Test both the entity sandcastle and the provided sandcastle in the linked issue (it works locally).

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]

Visual confirmation of both working:

Main sandcastle (entities):

BillboardCollection.add (plainhearts sandcastle):

[mzschwartz5]

Help me understand how this solves the issue? The issue occurs when returning a resolved promise, and you're saying that awaiting that causes a one-frame delay (even though it's already resolved, microtask queue shenanigans?) that causes this bug?

[mzschwartz5]

It's just kinda annoying - I don't like relying on instanceof. It generally indicates something has been designed incorrectly (which is already definitely the case in terms of overall billboard code architecture / texture loading, but I was hoping we improved on that last time).

[Beilinson]

Yes that's exactly right

[Beilinson]

I'll do typeof instead like I did with addImageSubRegion

[Beilinson]

But i definitely agree with you regarding the billboard texture architecture. IMO the best option long term is to do what I suggested in #12958 (comment), but as I mentioned there this will also fail for CallbackProperties so we still need some way of synchronously accessing already resolved textures + subregions.

[mzschwartz5]

On the bright side, this isn't much of a hot code path. It's not something that really runs every frame.

[Beilinson]

I'm going to sleep on this. I’ve been reading up on the "Releasing Zalgo" anti-pattern, which warns against APIs that return either a sync value or a promise depending something.

Since the recommendation is to be consistent (always sync or always async), and considering this function is called every frame for the entity api, returning a Promise here might just be the root cause.

Idea:

• Always return synchronously.
• Return undefined if the internal promise hasn't resolved yet.
• For non-entity billboards, we can simply trigger a check on subsequent ticks until a value is returned.

then basically all the areas of the code that currently run after await will just be refactored to run given the value has returned, otherwise retrigger a subsequent check on frameState.afterRender or something along those lines. Thoughts?

[javagl]

No really constructive comments (except for reiterating how much I appreciate the thoroughness and thoughtfulness that you're putting into that).

But I cannot pass the chance to chime in when there's a JavaScript/async rant 🙂 What bugs me: async is viral. You can design a beautiful, clean, efficient, slim, versatile API. And at some point, 20 steps down the call chain, you suddenly have to call some async function, and this async will bubble up, until the main function, so to speak, and pollute your whole API with a breaking change. (And when Zalgo is already mentioned, the step to this meme is not so large...)

For me, the ugly consequence is: When you define an interface, you have to declare all functions as async, period. Not doing so will result in a breaking change sooner or later.
(And the following is not as sarcastic as it may look at the first glance: I once considered to ""design"" a new language, called 'AsyncScript', which is just JavaScript with each function declaration being async by default, and each function call being awaited by default. Let the compiler and runtime sort out optimizations here - this is a syntactical level that a software engineer should not be concerned with).

All this is somewhat unrelated to fixing an existing bug, or handle an existing shortcoming or inconsistency in a large codebase. The combination of "promises/callbacks" and the state management (something being "LOADED" or "READY" or whatnot) requires great care and thorough documentation. I hope that PRs like this lead to a gradual improvement here.

[mzschwartz5]

I have to say, a (large) part of me agrees viscerally with this sentiment ("async is viral"). But there's a smaller part of me that always thinks "I must just not know the proper design principles for async programming." I feel that I have a good grasp on clean code design in general, but this must require an entirely separate regime of thought.

Could be wrong, that's just the nagging feeling I get! I will say, when I last did some async stuff in the Material class, my strategy was - rather than letting async bubble up and pollute the whole stack - to simply not await the promise once it got to a level where it didn't matter whether it was synchronous or not. Then, that function (and its ancestors) do not have to be marked async.

[javagl]

to simply not await the promise once it got to a level where it didn't matter whether it was synchronous or not.

That may be applicable in some cases. Particularly in CesiumJS, where many structures are revolving around the assumption that the application is running indefinitely, and ~"some update" is called every frame, and the promise just sets some ready-flag so that this update function can do some if (ready) render(); else dont();.

I recently had/currently have to deal with that. A "(web) request" involves a promise, basically for ~"the tile content". This promise is never awaited. When it's done, it's done. And then, it just changes some state somewhere, so that something else happens in the next update.

But this has two drawbacks:

1. it is highly prone for race conditions. One recent example here. And eventually, most forms of ~"something flickering" are boiling down to that. Making sure that the promise is not re-generated and executed multiple times is tricky. And making sure that the state change that is caused by the promise is consistent with other state changes that happen elsewere (maybe even by other promises!) can be really tricky. (Another reason why the state space should be as small as possible)
2. It's hard or impossible to test. In order to test the effect of the promise, it has to be awaited, eventually. Things like pollToPromise are just crutches for converting an un-awaited promise (that is hidden deeply in opaque structures, and that does some state change) back into an awaitable promise - namely, by polling for the desired state change.

I occasionally advocated for enabling something like https://typescript-eslint.io/rules/no-floating-promises/ - I'm pretty sure that there are some dangling promises in CesiumJS that could turn out the reasons for bugs. Even if it's sometimes intended, I think that making that explicit (and rasing awareness) by having to insert some ESLint-'ignore' could make sense.

And after all, I'm in the camp of "There are no asynchronous operations,". Whether or not something should be executed asynchronously has to be decided by the caller. Yes, you'd usually not block/wait for some web request. But that should rather be handled with something like promise = makeAsync(thatWebRequest()); than the other way around.