Fix requestRender condition in DataSourceDisplay.update

[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?

[ggetz]

I think this "undos" the fix in #12429, as this function can now return
true one frame, then false when the billboard is loading, then true again when it's completed.

[Beilinson]

This should be fine, update returns whether or not any processing is finished, while ready is stored separately on the datasourcedisplay and stays enabled after the first time

The naming is conflated internally in the update function here, I can change that

[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?

[rropp5]

This code will effectively request a render each time a Billboard Entity is created because of the way the timing works with the new TextureAtlas implementation.

I believe an alternate solution is necessary since this is not consistent with the advertised behavior of requestRenderMode (read my 2nd comment on #11820 for more info).

I commented on the PR thread and suggested that it may be feasible to synchronously load the texture if the id already exists in the TextureAtlas. Then, for images that are not in the atlas, asynchronously load the image and then request a render once it has loaded. This will effectively request a render once per distinct image and then for all subsequent uses of the image, invokers may simply request a render after creating the Billboard since the image will load synchronously.

[rropp5]

Another alternative to think about is the possibility of introducing a callback mechanism from the Billboard Entity API to allow invokers to pass some code to execute once the image is loaded.

To address #11529, the Label Entity's default implementation could pass a callback that requests a render. Then, for Billboard Entities, invokers may choose to request a render (or not) when the Billboard finishes loading. This may be a good balance to preserve the advertised behavior of requestRenderMode while also addressing #11529 without introducing excessive render requests.

[Beilinson]

Great insight, I've been playing around with this idea locally and it seems to work well, will iron it out and commit soon

[rropp5]

Any luck with either of these paths?