Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Beilinson wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/engine/Source/Renderer/TextureAtlas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ async function resolveImage(image, id) {
* or a URL to an Image, or a Promise for an image, or a function that creates an image.
* @param {number} width A number specifying the width of the texture. If undefined, the image width will be used.
* @param {number} height A number specifying the height of the texture. If undefined, the image height will be used.
* @returns {Promise<number>} A Promise that resolves to the image region index, or -1 if resources are in the process of being destroyed.
* @returns {Promise<number> | number} The image region index or a promise that resolves to it. -1 is returned if resources are in the process of being destroyed.
*/
TextureAtlas.prototype.addImage = function (id, image, width, height) {
//>>includeStart('debug', pragmas.debug);
Expand All @@ -666,7 +666,7 @@ TextureAtlas.prototype.addImage = function (id, image, width, height) {
}
if (defined(index)) {
// This image has already been added and resolved
return Promise.resolve(index);
return index;
}

index = this._nextIndex++;
Expand Down
5 changes: 4 additions & 1 deletion packages/engine/Source/Scene/BillboardTexture.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,10 @@ BillboardTexture.prototype.loadImage = async function (
let index;
const atlas = this._billboardCollection.textureAtlas;
try {
index = await atlas.addImage(id, image, width, height);
index = atlas.addImage(id, image, width, height);
if (index instanceof Promise) {
index = await index;
}
Comment on lines +202 to +205
Copy link
Contributor

@mzschwartz5 mzschwartz5 Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mzschwartz5 mzschwartz5 Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@Beilinson Beilinson Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's exactly right

Copy link
Contributor Author

@Beilinson Beilinson Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do typeof instead like I did with addImageSubRegion

Copy link
Contributor Author

@Beilinson Beilinson Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mzschwartz5 mzschwartz5 Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Beilinson Beilinson Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@javagl javagl Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mzschwartz5 mzschwartz5 Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@javagl javagl Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (error) {
// There was an error loading the image
billboardTexture._loadState = BillboardLoadState.ERROR;
Expand Down
Loading