Reduces redundant clamping on labels

[mzschwartz5]

Description

Clamping things to ground is a slow operation (currently - #9961 will speed it up, but it's still relatively expensive). When a label is clamped to ground, it sets each of its glyphs (which are individual billboards) to clamp as well.

This is slow and unnecessary - we already have logic that copies the clamped position of the label to each of its glyphs.. It even claims to do this to reduce clamping redundancy, but unfortunately it's not sufficient.

To address the issue, this PR adds a new boolean flag to Billboard: positionFromParent - when true, this flag indicates that the billboard gets its position from some ancestor, and thus expensive operations like clamping can be skipped. (I'm generally not a fan of adding special flags, but I considered a few other options and nothing was particularly simpler or feasible without a much larger refactoring effort. Happy to discuss alternative ideas though).

Issue number and link

This is one piece of an overall labels performance improvement effort, stemming from #12719

Testing plan

Test with this sandcastle. Specifically, set labels to 1000, and toggle the number of characters between 5 and 100. Before this PR, there's a pretty big lag in creating / destroying the glyphs, and panning around is pretty intolerable as well. With this PR, it's noticeably faster (but still much room for improvement, which I plan to address).

Two notes:

• Depending on when you test this, there's another PR (Event performance #12896) which may or may not be released yet which significantly speeds up the destruction of glyphs. (Creation is unaffected).
• I believe there's also some caching going on which makes panning behave better over time. So it can be a little hard to discern the performance impact of this PR vs. caching and other performance improvement PRs. This PR's clearest, largest benefit is in the time it takes to create all the glyphs.

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.

[mzschwartz5]

I removed this line because:

1. Now that updateClamping is skipped for glyph billboards, this value is undefined. On changes to the label's clamped position, it will propagate to the glyph. But not on construction (not that it really matters - only for the sake of this test).
2. It wasn't a particularly robust test anyway, poking at the internal details of a label's glyph's billboard... we should be doing less of this.

[javagl]

👍, expect(some._nested._private._property).toBe(42) is an antipattern (not only in tests)

[javagl]

Maybe I could derive it by reading the surrounding code more thoroughly. But you likely have all that in the working memory right now, and maybe it's an easy question:

when true, this flag indicates that the billboard gets its position from some ancestor, and thus expensive operations like clamping can be skipped.

I see that this flag causes an early return in _updateClamping. But ... where does anything actually receive a "position from a parent" now?

---

Totally unrelated: I stumbled over that _updateClamping function a few months ago. The context where I stumbled over it: There is an abundant pattern of wrong ownership directions. If one had to describe what a LabelCollection is, on the level of Object-Oriented Design, then one could start rambling, for hours: How it is constructed. What it consists of. What it can do. How it is used. Nobody would ever say: "A LabelCollection contains a Scene". That's just wrong. (A Scene may contain a LabelCollection, but let's skip the question of what a Scene is for now). There's an implicit TODO on my list to suggest some refactorings there, but for now, here are some notes from a file on my Desktop that is called Cesium Scene Everywhere.txt:

• The scene is passed to the LabelCollection constructor

• It is stored as a property of the LabelCollection, but not used there - only forwarded to the BillboardCollection constructor

• In the BillboardCollection, it is stored, as this._scene

  • It is used in the constructor, to attach some event listener to the scene.terrainProviderChanged event
  • It is used in writeTextureCoordinateBoundsOrLabelTranslate (whatever that is...), but only to set some flag based on scene.globe.depthTestAgainstTerrain
  • But that private _scene property of BillboardCollection is also accessed by Billboard!

• In Billboard, it is used in different ways:

  • It is accessed via this._billboardCollection._scene.globe._surface.removeTileCustomData (No kidding...)
  • It is accessed in Billboard._updateClamping, which, let me take a guess, /** Updates the clamping */...
  • This _updateClamping throws an error if the scene is not defined...
  • In _updateClamping, it is mainly used to call scene.updateHeight and scene.getHeight.

Regardless of that: Avoiding redundant work is good. Further optimizations could be related to reducing the work implied by updateHeight and getHeight, but these might require further restructurings first.

[mzschwartz5]

@javagl

1. Here is where the glyphs positions get set from the parent (the label). The comment there claims to avoid redundant work, but it wasn't sufficient.
2. Yes, the patterns ownership and object-oriented flow of information are problematic in quite a few places, but are especially bad with billboards, labels, and their respective collections... but it would take a full refactor of all four (giant) classes to address.

Further optimizations could be related to reducing the work implied by updateHeight and getHeight, but these might require further restructurings first.

What do you mean here? There's the octree picking PR, which I intend to pull across the finish line.

In general, when I think "further optimizations" for labels, I would want to completely restructure it so glyphs aren't each their own billboard. But that would also require significant shader changes... ugh.

[javagl]

What do you mean here?

On a very high level, there are structural similarities between the clamping performance ( #12719 ) and picking performance: #11814. Both functionalities are elaborately wired in without proper abstractions, they use an inefficient implementation, and they are called faaar too often. (And ... maybe I'm misremembering something, but isn't clamping (under certain conditions) even using some pick function under the hood, with a ray pointing to the center of the earth...?)

However, similar to the "caching" that was drafted in https://github.com/CesiumGS/cesium/tree/cache-model-picks to address the model picking performance, one could have further options if there was an abstraction for the clamping. The current patterrn of

constructor(...scene...) {
   this.scene = scene;
   ..
}
...
setHeightReference(...) {
  this.heightReferece = value;
}
...
doTheClamping() {
  if (this.heightReference !== NONE) {
    if (this.scene === undefined) throw up; // Ouch...
    const height = scene.getHeight(...);
}

could benefit from some HeightProvider, i.e. something like

...
setHeightProvider(...) {
  this.heightProvider = value;
}
...
doTheClamping() {
  if (this.heightProvider !== undefined) {
    const height = this.heightProvider.getHeight(...);
  }
}

It would avoid the wrong ownership direction of a Billboard using a Scene. It would avoid inconsistencies between a HeightReference with a missing Scene. And it would allow things like wrapping whatever that HeightProvider is into a CachingHeightProvider, so that it is not necessary to perform the (highly complex and costly) computations that Scene.getHeight is doing thousands of times just because the camera moved by one pixel.

[ggetz]

This works much better. Thanks @mzschwartz5!