Quadtree performance

[mzschwartz5]

Description

Background:

• Cesium terrain is loaded and organized as a Quadtree (each terrain tile can split into four children if higher resolution is needed).
• Entities in Cesium can register a callback with a Quadtree to be notified when the height at that entity's position changes.
• This callback may be fired, for example, when a higher resolution tile loads in, or when vertical exaggeration changes (these two examples are not exhaustive).
• These callbacks are called customData in Quadtree-speak. (Maybe the intention was to stay general, but that's all the custom data has been used for since its introduction ~10 years ago).

Current state:

The way callbacks are handled in the quadtree is very inefficient:

1. If a single callback is added or removed, every tile in the tree re-evaluates which callbacks belong to it. So the time-complexity to add or remove a callback scales with the number of callbacks currently in the tree.
2. During this evaluation process, every tile checks all of its parent's callbacks to see which belongs to that child. This means every callback gets considered 4x, at each level of the tree, instead of just once per level.
3. An array is used for the customData structure. Removing callbacks requires iterating through the array for each removed callback at each level of the tree.

Because Scene re-registers a height-changed callback whenever the camera moves, the tree currently gets reevaluated in this inefficient manner every single frame (that the camera moves).

State after this PR:

Fixes all the above issues by changing the direction of data flow to (parent --> child), rather than (child <-- parent). Adding / removing callbacks no longer re-evaluates the whole tree, and is now independent of the number of items in the tree. The customData member is now a Set, so removal of callbacks is constant-time.

Issue number and link

#12719 (this is really a sub-issue of that)

Testing plan

Sandcastle.

In current release, with 1k labels and 100 characters each, panning the camera around just halts the application entirely. Because each frame the quadtree re-evaluates the label's 100k callbacks (1k callbacks once #12905 is merged).

Performance comparison with 100 labels at 20 characters each, just panning the camera around:

Before changes: (see how the time is dominated by selectTilesForRendering, basically all in updateCustomData (at various stack levels))

After changes (selectTilesForRendering is now much smaller - updateCustomData is basically non-existent now)

__

Test with vertical exaggeration

There's definitely a bug here, but it seems to pre-date my changes to the Quadtree. The updates to vertical exaggeration do not consistently propagate to billboards / labels.

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

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

[javagl]

I had a short look at this.

The performance difference is huge. In the test with the labels, I did not see any functional difference (as in "regression"). So that looks fine for me.

---

Beyond that:

It can be difficult to separate "the old code" and "the change itself" during a review. For example, I'm easy to distract with things like QuadtreePrimitive.updateHeights, which seems to aim at an indentation depth highscore with " ", has that 11500 repeated three times (Mariana trench?), or defines that let i; that is used 150 lines later.

Trying to ignore all this, and looking at the code changes:

I won't claim to have fully understood what has been going on there to begin with. For example: The tile now stores the _addedCustomData and _removedCustomData which previously had been passed to updateCustomData. The _updateCustomData function is called in visitTile and selectTilesForRendering, and I haven't zoomed beyond that. I assume that these arrays are stored for reversing the direction of the dispatching.

A bit more abstractly: It looks like the whole "custom data" functionality is just dispatching these callbacks into a tree: "Starting at the root, here's a custom data - add it to the root, and to the child it falls in" - in pseudocode:

Tile.addCallback(callback, position) {
    this.callbacks.push(callback);
    const child = childFor(position);
    child.addCallback(callback, position);
}

recursively going down the tree.

There's probably a reason why it isn't done like that. It may be related to that _loadQueueTimeSlice, which seems to be a mechanism for saying: "We don't care whether the positions are wrong or flickering, as long as the computation doesn't take too long".

---

Bottom lines:

I don't see anything that could cause issues or regressions, and it looks like an overall improvement.

A detail is that there was that _lastTileIndex, which is now replaced by _customDataIterator, but the latter is not declared in the constructor.

[mzschwartz5]

Added some unit tests to make sure the update function is doing what it's supposed to, and that it works regardless of tiling scheme...

That said, I think it's bad practice to have a test per tiling scheme here. If someone adds a new one, how would they know we want to test it in QuadtreeTile of all places? Perhaps, instead, I should have defined the childTileAtPosition function in each tiling scheme instead of in QuadtreeTile, and that way each tiling scheme tests itself.

However, childTileAtPosition is currently implemented in such a way that a single implementation should be correct for all tiling schemes... So it doesn't need to be implemented per tiling scheme, and perhaps testing the two existing schemes is sufficient for now and the future? Input welcome.

[javagl]

After the initial review pass, I tried to keep track of the discussion about the childTileAtPosition function, and (although I'd probably have to dive deeper into that to fully 'understand' it) it looks like this was resolved.

I ran the test sandcastle again, with the latest state, and did (still see the performance improvement, of course, and) did not see any form of regression.

Apologies for the 'force-push' that I had to throw in here: Git did not recognize my merged-in main as a merge commit (I think it might be because I didn't set up the tracking branch properly? I never had seen this before...).