Drafts for polyline requestRender handling

[javagl]

Description

The Request Render Mode is "spotty" with Entities. This is a bit vague, and there are many different flavors of "spotty". As shown in an example of a recent comment there, the meaning of that can be very specific (and unspecific at the same time), as in "It does not work".

The reason for this specific case is, very roughly: Under the hood, the entity causes the creation of several things, collections, batches, geometries, updaters, and a Primitive. This primitive is created with the default of asynchronous=true. This essentially means that the first requestRender call from the sandcastle is issued before the geometry is actually created. (The creation happens with a worker, namely the createGroundPolylineGeometry worker in this case). The primitive then goes through the ""well-known"" PrimitiveState state machine, and eventually, the GroundPolylinePrimitive becomes ready=true.

This is where the "fix" is hooking in: Setting the ready flag is done in a frameState.afterRender function. And when such an afterRender function returns true, it will trigger a re-rendering in 'request render mode'.

EDIT: Originally, this change was done in the GroundPolylinePrimitive. Now it is done in Primitive.

Of course, this fix is likely "too narrow", and only a "patch" for this particular case. There are other types of geometry and primitives, and whenever something is created with a web worker, this "thing" may suffer from the same issue: A client-side requestRender call may be issued before the worker is done, and the desired re-rendering of the (then ready) object does not happen. It may be necessary to more thoroughly review the places where geometry/primitives are created asynchronously, and where some afterRender.push(... return true; ) may have to be inserted.

Issue number and link

Related to #12543

Testing plan

Run the sandcastle from this comment in the issue. Also try commenting out the line
clampToGround: true,
to see whether it works for the non-GroundPolylinePrimitive case. Further tests with other entities (e.g. polygons) may be added.

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, @javagl!

✅ We can confirm we have a CLA on file for you.

[ggetz]

@javagl Your fix makes sense to me, but I'm having problems reproducing the issue in order to test this. Any ideas?

[ggetz]

Also, I noticed this only fixes the issue for polylines that are clamped to ground. Should this change be applied "upstream" to either/or ClassificationPrimitive.js, or even Primitive.js, itself? I think the former may make sense, but the later is likely not needed.

[javagl]

I left a short comment in the issue. I mentioned that this (draft!) is probably too narrow, but I tried to "zoom in" to a place where I could try to articulate the reason and why the fix resolves it. Iff ... (there really was an issue that everyone could reproduce and) the fix should be applied, it would likely be necessary to fix (potentially many) other places (or try to "pull up" the change into some base class).

[ggetz]

OK, I was able to both reproduce the issue and confirm the updated behavior from this PR in Firefox. With the changes here, I do see a render (and the polyline) after the polyline is added and requestRender is called immediately after.

@javagl If the goal is to get this in, could you please update this PR and move it to "Ready for review"?

And while I understand this PR does not address all request render issues, do you plan on accounting for non-clamped polylines? I did confirmed that non-clamped polylines still are not rendered in this branch. It would also be helpful to understand if your intention is to make that specific update or not.

[javagl]

And while I understand this PR does not address all request render issues, do you plan on accounting for non-clamped polylines? I did confirmed that non-clamped polylines still are not rendered in this branch. It would also be helpful to understand if your intention is to make that specific update or not.

I read through some of the Primitive code, and think that the basic idea behind this fix can be generalized by moving it from the GroundPolylinePrimitive to the Primitive. Whenever a Primitive becomes _ready, a new rendering pass is triggered. I tested it for the case that you suggested, with the given example and clampToGround: true, being commented out. If the approach in general does not look ~"totally wrong", I can do some other tests with other entity types (that involve some form of asynchronous creation - I'll have to did through some code to find sensible test cases here).

[Beilinson]

Hey @javagl @ggetz, just wanted to point out that my PR #12630 fixes the sandcastle mentioned as well, and is a better solution with many polylines/other entities added in batches, as the render is only requested after all processing is completed at the DataSourceDisplay level.

This was the original behavior prior to #12429, and as end users of Cesium on a complex BI+GIS system we think adding a render after every primitive is "ready" might render a significant amount more in comparison, resulting in more CPU time which we would like to keep to a minimum.

[Beilinson]

@javagl Your fix makes sense to me, but I'm having problems reproducing the issue in order to test this. Any ideas?

To reproduce this I increased the first timeout from 1 to 5 seconds, and made sure to test with the maximized viewer

[javagl]

@Beilinson Yes, I saw the PR #12630 , and originally intended to cross-link (and make sure that there's no interference, both in terms of the effects of the changes, as well as in terms of merge conflicts). I eventually ... "forgot" ... to make this cross-link, because the other one looked like it was focussed on billboards.

From skimming over it, it looks like it addresses the issue on an even higher level - not GroundPolylinePrimitive, and not Primitive, but DataSourceDisplay. (The other changes obviously are specific for billboards). So the change at https://github.com/CesiumGS/cesium/pull/12630/files#diff-ae5c3742ec2d5461204c239bdfc025bd240d473e79c1c41804b0fad87b082203R327 is, on a very high level, similar to what is done here: There's a bunch of "update" functions. They return some value. And when things are "done", a re-render should be triggered.

The reason why I cannot say more is: I don't know what the "update" functions are doing. (I have a mantra: "Whenever you create a function that has a name that starts with update..., you're probably doing something wrong"). I don't know what the value means that they are returning. (The function is called on data sources and visualizers, and there are many implementations of those - do they all agree on what the return value means?). I don't know whether this change might cause renders to be triggered under conditions under which they should not be triggered (or vice versa).

tl;dr: @ggetz Maybe this is obsolete due to the linked PR. Someone with a deeper "understanding" will have to decide this.

[Beilinson]

I agree regarding the naming of the update functions, the DataSource and Visualizer "base classes" state the following:

Visualizer.update:

/**
 * @returns {boolean} True if the display was updated to the provided time,
 * false if the visualizer is waiting for an asynchronous operation to
 * complete before data can be updated.
 */

DataSource.update:

/**
 * @returns {boolean} True if this data source is ready to be displayed at the provided time, false otherwise.
 */

So #12630 does the following:

When all datasources and visualizers are ready (i.e, finished updating), and provided they were not ready (i.e, working on some background processing) on a previous frame, then trigger a new render.

Note that datasources and visualizers don't begin processing unless a render is requested by the user first, so the above method is always a "re-render on update complete"

From my testing I couldn't find a scenario which isn't solved by this (and which might require additional renders at the primitive render as per this PR). The Scene adds requestRenders on the TaskProcessor and RequestScheduler, both of which however missed the sync image resolving asyncronously issue which was what I fixed with the TextureAtlas. There may be more missing, but I can't be certain.

[javagl]

I cannot say much about the DataSource/Visualizer classes. I'm reeeeally careful with certain terms here. For example, you mentioned

When all datasources and visualizers are ready (i.e, finished updating), and provided they were not ready (i.e, working on some background processing) on a previous frame, then trigger a new render.

And that sounds reasonable, and makes perfect sense, on the level of that statement itself. (I mean, it nearly sounds self-evident, as in "Why wasn't it done like that 10 years ago?").

But I started investigating the case of the GroundPolylinePrimitive not appearing, and did this in a "bottom-up" fashion, and while going "up", I looked at the Primitive, and ... long story short, added some comments in e508c36, with one core point being: When a Primitive is in the READY state, then this is the opposite of the _ready flag being true. I also tried to figure out and document what update means, but when such a function starts with an if-statement like this, then this is not even worth documenting.

So iff all data sources and visualizers agree that update returns true when something is in fact "ready" ( as in "renderable"), and false when some asynchronous work is still pending, then your fix will likely be preferable to this one, in that it addresses the issue more broadly.

[ggetz]

This was the original behavior prior to #12429, and as end users of Cesium on a complex BI+GIS system we think adding a render after every primitive is "ready" might render a significant amount more in comparison, resulting in more CPU time which we would like to keep to a minimum.

@Beilinson Can you elaborate on this a bit? #12429 did change the "ready" behavior, but was not targeted at request render mode specifically.

As I understand it, the main difference between this PR and #12630 is the following: That this PR handles things "bottom-up" and should render a new frame as required for most async primitive updates. #12630 aggregates all entities "ready" states and issues one "batch" call to requestRender. Is this all correct?

My concern is that all entities types may not be properly handling the update cycle and ready value consistently. I'm more confident that the change here will ensure the underlying primitives get updated such that they can eventually become "ready" consistantly.

I do believe this change is in the spirit of what is described in Improving Performance with Explicit Rendering, as it handles "an asynchronous process [that] returns from a web worker" and due to the asynchronous nature of the update is typically not something the user could account for using the existing API.

But if you could provide a use case or test case where this behavior triggers too many updates and uses too many CPU resources, we can use it to verify this update versus #12630.

[Beilinson]

Hi @ggetz,

#12429 not being targeted at requestRenderMode just from my search through cesiums versions the primary cause to the issues we were seeing in our organization related to rendering of polylines/polygons.

As I understand it, the main difference between this PR and #12630 is the following: That this PR handles things "bottom-up" and should render a new frame as required for most async primitive updates. #12630 aggregates all entities "ready" states and issues one "batch" call to requestRender. Is this all correct?

Yes that was exactly my intention.

My concern is that all entities types may not be properly handling the update cycle and ready value consistently. I'm more confident that the change here will ensure the underlying primitives get updated such that they can eventually become "ready" consistantly.

I completely understand, this repository is obviously not my area of expertise so I can only judge from my experience as a cesium user.

Regarding Improving Performance with Explicit Rendering, my understanding that I expect a batched render when all work is done comes from the following statement:

If the app makes changes to the scene or its contents through the Cesium API in a way that is not covered by one of the above cases, for example, using the Entity or Primitive API, then explicitly request a new render frame.

From how I see it, if I request one render, I would expect it to render only once. Now obviously this is impossible due to the asyncronous nature of the entity-to-primitive pipeline, but seeing as the main goal of requestRenderMode is:

Cesium apps can benefit from rendering less frequently. Rendering a new frame uses CPU resources and is often not necessary if your app is idle. Improved performance with explicit rendering means you can run Cesium apps without worries of kicking your laptop fan into high gear or draining battery life on mobile devices.

Then I would prefer any additional requested frames (especially those that process hundreds/thousands of new primitives) to be batched and minimized.

Now in reality all of the above is highly "theoretical", and the real question is:

But if you could provide a use case or test case where this behavior triggers too many updates and uses too many CPU resources, we can use it to verify this update versus #12630.

I don't currently have such a test-case, but our use case for example is: thousands of polylines + polygons + points + labels are added in batches as a certain dataset loads (not necessarily on startup, this is a continuous process throughout our applications lifespan). We use request render mode as our application users mostly low-end laptop users, so this is quite important for us to try to avoid as many renders as possible (and difficult for me to personally check due to my lack of access to one of these). We obviously limit targetFrameRate, and anything else possible. The performance and behavior is completely suitable on Cesium version 1.86 (which we currently use), where only one additional render is queued after all primitives are ready.

In the worst case scenario, I fear that all primitives in a given batch may finish on different frames, resulting in possible hundreds/thousands of renders while the entire batch is still not 100% ready. All the while on the main thread we are doing completely separate processing, and these additional renders would make that even slower and more jagged.

However, it is almost impossible for me to say whether this PR would actually cause such a detriment to the performance.

[Beilinson]

However, it is almost impossible for me to say whether this PR would actually cause such a detriment to the performance.

I will try running cesium with this patch locally tomorrow to see if there is any substance to my claims and will report back.

[Beilinson]

I will try running cesium with this patch locally tomorrow to see if there is any substance to my claims and will report back.

I've ran a performance sample (using 4x cpu slowdown and hardware concurrency: 4, to simulate a low end machine) directly in our system with and without the patch, here are the results:

Without this patch:

With this patch:

Due to external factors like differing request speeds, etc, I'm mainly focusing on rendering time, which is the only thing that is definitely caused by a difference in the patch - about ~1s increase in total render time.

The idle time went down ~0.5s as well, but that can be due to external factors so I won't focus on that.

From my perspective this is a decent slowdown, not something I feel must be avoided at all costs but also not trivial. We are already running #12630 using a monkeypatch, and we aren't seeing any issues in entity rendering using requestRenderMode, but there may always be use cases unknown to us.

I hope this is enough to help you make an informed decision!

[ggetz]

Thank you @Beilinson! That's helpful data and I think it represents a fair use case.

For the interest of getting an initial, simple fix to the entity rendering update issue, I'm leaning towards moving forward with @javagl's updates here for now. However, I don't think this the end-all solution to meeting everyone's needs around request render mode.

Let's still continue discussion in related PRs and issues, and I'd be open to eventually removing this patch if it becomes superseded by that work.