Fix Entity Rendering with requestRenderMode=true

[Beilinson]

Description

Fixes the requestRenderMode issue which stems from #12429.

Issue number and link

Fixes #12543

Testing plan

Sandbox Example

The setTimeouts are so that the scene is definitely NOT requesting any render when updating the polyline material (all the tiles should already be loaded in).

With the above test the bug is easily reproducible, and is fixed with my branch.

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]

I don't want to revert the change from #12429 as to me that does seem like a logical change (if the DataSourceDisplay is ready it should stay ready regardless of entity updates).

I originally didn't think it correct to add an additional flag, however upon further investigation I understood that the former logic prior to #12429 scheduled a render whenever result changed from false to true on consecutive frames (which is logical, on the current frame the result is false whenever any of the visualizers on any of the datasources hasn't finished async processing, and on the first frame when all async work is done result is true and that is the only frame where we want to request a render).

TL;DR - returns the previous logic by adding a new flag instead of undoing the changes on the ready flag

[Beilinson]

I'll point out that with this fix, requestRenderMode reverts to the previous behavior where renders are only requested after all datasources under the DataSourceDisplay finish asynchronous work. This can actually cause delays in rendering, if for exampling 1 datasource finishes after 2 frames, while another takes significantly more, a render will be requested only after the longer one finishes.

I could alter this behavior if desired

[jasonmprop]

Thanks for working on this patch @Beilinson, much appreciated.

We also noticed this regression breaking our app (that uses request render) when updating from 1.121 to 1.128, I think it snuck in with 1.126 as noted in the CHANGES.md

We have confirmed that your first commit 9d2fe69 fixes our issue (we patched it on top of 1.128), we have not yet tested your commit bd38955

In terms of the second commit, is that what causes the delays in rendering you mentioned above ?

Let us know if there is something we can do to help out.

[Beilinson]

Glad I could help @jasonmprop,

The second commit is a more technically correct fix, from what I can see more or less the requestRender() that we call manually whenever updating any property/adding/removing entities primes the next frame, but the actual update almost always happens 1 frame directly after that, which is the frame that the update method I patched calls.

[Beilinson]

Theres actually an interesting alternative fix which can change how requestRenderMode works, by setting:

if (this._prevResult !== result) {
  this._scene.requestRender();
}

Then a frame render will be requested twice when the async work begins and when it finishes, which will allow for things like updating an entity property, or adding an entity to a scene to be rendered automatically (without manually calling requestRender()), while still keeping renders to a minimum when the scene is unchanging.

While this is not consistent with the https://cesium.com/blog/2018/01/24/cesium-scene-rendering-performance/ blog post by @ggetz, I think this is worth considering:

1. These renders will only happen whenever any asynchronous work begins and whenever all async work ends. This still means if updating thousands of entities simultaneously, there should only be 2 frames rendered, which is even better than what some developers may be doing where they requestRender as part of a loop over thousands of entities
2. This could be some alternative mode in addition to requestRenderMode, or another flag used in tandem

[ggetz]

Thanks for your patience here @Beilinson.

Then a frame render will be requested twice when the async work begins and when it finishes, which will allow for things like updating an entity property, or adding an entity to a scene to be rendered automatically (without manually calling requestRender()), while still keeping renders to a minimum when the scene is unchanging.

I like this idea! However, as I mentioned in my review comment, I'm concerned that allowing the billboard visualizer to go back to returning false after return true breaks some existing behavior.

Maybe we're conflating ready and the return value of update with too many things. Maybe visualizers should expose a separate property for tracking updates requiring a re-render?

[rropp5]

@ggetz I have some concerns that the changes here (to fix #12543) will introduce excessive rendering when the system creates new Billboard entities very often.

#11820 - This was the original bug report that was caused by some of the logic in the Billboard's _loadImage function. Essentially, it always requested a render when the Billboard's image loaded which would cause a render every time a new Billboard entity was created.

I believe the best tradeoff here would be to have the Billboard load its image from the TextureAtlas synchronously if the image id is already in the TextureAtlas. Otherwise, the image should be loaded asynchronously and then a render should be requested immediately after the image is resolved. This allows users to reuse images in request render mode without introducing unnecessary render cycles while still fixing the problem with Label backgrounds failing to render after "missing" their target frame.

I had identified a workaround to #11820 that essentially implemented this behavior by passing a "redraw" parameter to the completeImageLoad function that was defined in Billboard.js:

Billboard.prototype._loadImage = function () {

  ...

  function completeImageLoad(index, redraw) {

    ...

    // Request a new render in request render mode
    if( redraw )
      scene.frameState.afterRender.push(() => true);
  }

  if (defined(image)) {
    imageIndexPromise = atlas.addImage(imageId, image);
  }
  if (defined(imageSubRegion)) {
    imageIndexPromise = atlas.addSubRegion(imageId, imageSubRegion);
  }

  this._imageIndexPromise = imageIndexPromise;

  if (!defined(imageIndexPromise)) {
    return;
  }

  // If the promise has already successfully resolved, we can return immediately without waiting a frame
  const index = atlas.getImageIndex(imageId);
  if (defined(index) && !defined(imageSubRegion)) {
    // Do not request a redraw because the image exists in the atlas
    completeImageLoad(index, false);
    return;
  }

  // Request a redraw the first time an image is loaded into the atlas
  imageIndexPromise.then((index) => completeImageLoad(index, true)).catch(function (error) {
    console.error(`Error loading image for billboard: ${error}`);
    that._imageIndexPromise = undefined;
  });
};

Would we be able to implement something similar with the new code structure?

[Beilinson]

Hey @ggetz, I've updated the PR with a fix for #12138 and the issues described in #12543 related to billboard and label rendering with requestRenderMode=true.

The issue stemmed from the difference between image urls/resources and client-side images (i.e, canvas).

• urls/Resources were fetched as part of the resolveImage function in TextureAtlas.js. At the end of the fetch, a callback added in Scene.js (line 709) to the RequestScheduler would request a render, which would later trigger the textureAtlas.update method call in BillboardCollection.js (line 1813)
• Client-side images (HTMLImageElement|HTMLCanvasElement) do not trigger this additional render, however are forced into the async pipeline as described in Refactor TextureAtlas to avoid race conditions leading to billboard rendering artifacts #12495

My solution was to simply treat them as "auto-completing resources", and trigger RequestScheduler.requestCompleteEvent inside of resolveImage given that the image is not a resource. This solves the issues described above.

The solution I think is actually optimal is to allow client-side images to be added synchronously to the image processing queue, allowing them to be processed at the end of the rendered frame without any additional renders. The entire process is still fully async as per the TextureAtlas promise refactor, as the internal _addImage still returns its promise which is resolved when TextureAtlas.update is ran.

Regarding @rropp5's comments, I added a flag Scene.renderOnUpdateComplete (enabled by default to keep current behavior), which is used in DataSourceDisplay.update and BillboardCollection.update to optionally prevent the renders that happen when update processing completes.

Sandboxes tested (make sure to run the test twice to prevent slow tiles from causing the additional renders)

1. Polyline Example fix-requestrendermode vs main - My branch fixes the fact that the blue polyline doesn't render properly after the last requestRender. Removing that line in my branch sandbox also behaves properly by not rendering the polyline. In main it doesn't render regardless (this was the issue fixed by the DataSourceDisplay.update change)
2. Billboard with canvas image fix-requestrendermode vs main - My branch fixes the fact that the blue square billboard doesn't render properly after the last requestRender (this was the issue fixed by the TextureAtlas - resolveImage change)
3. Label example fix-requestrendermode vs main - My branch fixes the fact that the label with red background doesn't render properly after the last requestRender (only resolved glyphs render, the letter a), (this was the issue fixed by the TextureAtlas - resolveImage change)

[Beilinson]

@ggetz An alternative solution I considered was moving the TextureAtlas.update method to scene._preUpdate.addEventListener(() => textureAtlas.update(frameState.context)), rather than as part of the BillboardCollection.update method.

My reasoning stems from the fact that processing the texture atlas according to your explanations in #12495:

• Added an update function which is designed to be called once per frame which processes all resource updates at once
  • This means we can perform all of the needed texture copies before or after the frame is being rendered

However in requestRenderMode=true, the textureAtlas is only called after rendering a frame, rather than on every actual frame, unlike DataSourceDisplay.update which runs every frame regardless of rendering.

Is this a valid solution or have I misunderstood the PR above?

[ggetz]

Thanks for the update here @Beilinson! I think this is getting much closer.

I have a few comments. Let me know what you think.