Skip the sort rerun that follows a new mapping

[oscarlorentzon]

If a frame requests another sort while one runs and a new mapping then appears, driveSort sorts the old mapping again when the running sort ends, and the new mapping is shown one sort late. driveSort now starts a sort only while current.mappingVersion equals the mapping version of the most recent update.

A LoD result can also be written between frames, just before the running sort ends, and then no frame has computed the new mapping when that sort ends. driveSort now also does not start a sort while the new SplatAccumulator.mappingChanged() finds a mesh in current whose mappingVersion has changed since current was generated.

Verification

• New tests in test/unit/SparkRenderer.test.ts. On main, two tests fail with the old mapping sorted twice: one where a new mapping appears during a sort, and one where it appears between frames, just before the sort ends.
• Tracing main and this branch, with a rotating 10M splat scene in view and lodSplatCount 2.5M.

	New mappings that waited an extra sort	LoD result to the first frame drawing it, median
main	92%	151 ms
this branch	0%	105 ms

• npm run test:browser and npm run build pass.

Related to #347.

[mrxz]

With this condition removed, all unit tests still pass. So either this.current.mappingChanged() covers all relevant cases OR none of the unit tests cover the case where the check is needed.

Given that the mappingVersion can increment when generators are removed/added, I believe it is possible to end up in a situation where the this.current.mappingChanged indicates no change, as each generators' mappingVersion is unchanged, yet the accumulators mappingVersion would differ.

[oscarlorentzon]

Good catch, no test needed it. The check is still needed, as hiding, adding or removing a mesh changes the mapping without changing any mesh's mappingVersion, which mappingChanged() does not see. Added a commit with a test that hides a mesh during a sort, which fails without this condition.

[mrxz]

This mapping isn't necessarily tied to a frame, for example when autoUpdate is set to false. Not sure what a suitable name would be, the update method itself uses the next/current terminology, so nextMappingVersion could work, but IMHO also isn't the clearest.

Maybe something like latestMappingVersion?

[oscarlorentzon]

Agreed, it is set by every update, update() with autoUpdate off included. Added a commit that renames it to latestMappingVersion.

[mrxz]

Looks like a really nice improvement to the latency between the lod updates and the frame they are rendered.

One thing I'm wondering about is whether halting the driveSort is the right approach. Effectively it now stops because it knows there's a newer mapping that should be sorted. However that sort is delayed until the next driveSort, which is tied to updateInternal (generally the next frame/render). Ideally it could just start the "correct" sort.

Currently updateInternal skips accumulator generation when mapping changes while a sort is ongoing, to give the sort a chance to complete and update the display. While not exact opposites, we're now both delaying updates to give sorting a chance, while early stopping sorting to give updates priority. You probably have a clearer picture of how this interacts, but it feels like these two can be rolled into one, such that updateInternal can effectively "schedule" the right sort.

This probably won't cover the this.current.mappingChanged() condition, as that can literally happen at any point, but could remove the need of the this.frameMapping !== this.current.mappingVersion condition, as the sort defer logic in updateInternal is the main cause of such mismatch.

[oscarlorentzon]

Thanks, good points:

• Halting driveSort instead of starting the correct sort. Yes, the new mapping's sort waits for the next update. A sort can end at any time between two frames, so that wait lasts from zero to one frame. main has the same wait after the extra sort of the old mapping, since the new mapping is only sorted at the first update after that sort ends. This PR removes the extra sort and keeps that wait.

• Letting updateInternal schedule the right sort. Both waits enforce one rule: a sort is of the latest mapping, and that mapping is still current when the sort ends.

  • updateInternal waits so that the ordering the sort uploads matches the mapping display draws next.
  • driveSort waits so that the mapping is the latest.

  Agreed that updateInternal could generate the new mapping during the sort and have it sorted next, instead of skipping accumulator generation. latestMappingVersion would then have nothing left to stop. Compared with this PR, scheduling would remove the wait above, and this PR already removes the extra sort with a small change.

The content of display does not update for the whole sort of a new mapping, which is longer than that wait. Removing that freeze is the next step for now, and it builds on this PR as it is.

[mrxz]

Thanks for the clarification, the PR looks good in its current state.

• updateInternal waits so that the ordering the sort uploads matches the mapping display draws next.

Isn't the current condition used in updateInternal slightly off? Any in-flight sort will either have the same mappingVersion as the display or a newer one. In both cases that would end up becoming the next display after sorting. So mappingUpdated should ideally be based on comparing the mappingVersion to the one of the current in-flight sort, not the display.

[oscarlorentzon]

Isn't the current condition used in updateInternal slightly off?

Agreed. Changing to current would start the next sort at most a frame earlier. A freeze fix would change what the frames during the sort of a new mapping do, so leaving the condition to that change seems reasonable.