Fix exaggeration of ellipsoid voxel clipping bounds

[jjhembd]

Description

This PR fixes exaggeration of ellipsoid voxels, by exaggerating the clipping bounds along with the shape bounds.

A VoxelPrimitive implements vertical exaggeration differently depending on the shape type:

• VoxelShapeType.BOX and VoxelShapeType.CYLINDER apply the exaggeration as a transform, which is multiplied with the modelMatrix. This approach assumes the exaggeration is along the same axis for every point in the voxel (e.g., Z for cylinders). This is a reasonable approximation for datasets only covering a small area of the Earth, with some assumptions (!) about the shape orientation.
• VoxelShapeType.ELLIPSOID is often used for datasets covering a large portion of the globe, so the exaggeration must be along the local ellipsoid normal, which varies across the dataset. This is implemented via scaling/shifting of the shape's height bounds.

The missing element for ellipsoids, highlighted in #12811, was that clipping bounds were not exaggerated. Data near the clipping bound could therefore be clipped off when the shape was exaggerated. This PR corrects that by exaggerating the clipping bounds along with the shape bounds.

Issue number and link

Fixes #12811

Testing plan

Load the test local Sandcastle and verify that the entire dataset is exaggerated without being truncated.

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

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

[mzschwartz5]

Is it really necessary to have 8 different pieces of state related to clipping bounds? ( (old, new) x (exaggerated, regular) )

I don't totally understand how these are consumed, but I have this feeling that they're not all necessary.

Are the old members just used for tracking dirtiness? Do we even need to store the exaggerated clipping bounds explicitly, or can we just update the regular clip bounds directly?

[jjhembd]

The expensive call is VoxelShape.prototype.update. This call is only necessary if modelMatrix, shape bounds, clipping bounds, or exaggeration have changed. (Unfortunately we expose setters for both the shape and clipping bounds, so we do have to check those.)

The checks happen in checkTransformAndBounds. If we want to reduce the amount of state, we could perhaps store the state of the exaggeration itself rather than exaggerated bounds. We would then have verticalExaggerationOld and verticalExaggerationRelativeHeightOld, rather than exaggerated???BoundsOld * 4. Would this make sense to you? Or is there a better way to reduce state?

I realized the current setup is state-heavy. I was sort of kicking the cleanup down the road until we resolve the API question: Do we really need to expose all these bounds setters?.

[mzschwartz5]

(Unfortunately we expose setters for both the shape and clipping bounds, so we do have to check those.)

Not sure I understand- isn't this a good thing? In the setter, we could set some dirty flag to true.

[javagl]

In the setter, we could set some dirty flag to true.

Even if this was done in some setter like this one, there is no sensible encapsulation^* for all this. The 'getter' still returns the Cartesian3, and users will do primitive.maxClippingBounds.y += 42 and expect that to have the... expected ... effect.

Nevertheless, there are indeed quite a few "bounds" here. It looks like there are things that could be "grouped". The comment at #12813 (comment) mentioned dedicated classes for the "clipping" information. Without that, it looks like there could be different groupings:

• Roughly something like ExaggeratablePoint, to store a point and exaggeratedPoint, used for minRender, maxRender: ExaggeratablePoint and minClip, maxClip: ExaggeratablePoint
• Roughly something like an ExaggeratableRange with min/max/exaggereatedMin/exaggeratedMax (encapsulating the exaggeration functionality), used for renderRange: ExaggeratableRange and clippingRange: ExaggeratableRange
• (In both cases, these Exaggeratable...Things... could store (and hide!) the old... properties for the dirty checks where necessary)

This is just from browsing over the code in the GitHub view - I'll have to check this more closely in a dev environment to see which of these (or similar "groupings") could make sense....

---

^* This encapsulation would require a ReadonlyCartesian3. Maybe, in the future... (TODO: Insert "I would be so happy" meme here.

[mzschwartz5]

@jjhembd Yes I do think we should be storing the state of the exaggeration rather than the state of everything that depends on it. Then, in updateVerticalExaggeration, just compare to the old state and set a dirty flag if needed.

(To be honest, there should really be an event that one could listen to for vertical exaggeration changes... instead, every class that cares is storing the old exaggeration value... but that's a topic for another day)

[mzschwartz5]

Looking at the VoxelShape interface, I don't see any update method that has 5 arguments. Looks like it's a special case for ellipsoid shaped voxels... I don't love that :D It degrades the benefits of using polymorphism.

[mzschwartz5]

Is VoxelPrimitive.update called every frame? If not, could we just calculate the exaggerated clipping bounds on the fly here rather than storing it as a member? (Honestly, even if it is called every frame, it's extremely cheap to calculate the exaggeration)

[jjhembd]

VoxelPrimitive.update is called every frame (we should probably shortcut out of it earlier). The reason for storing the bounds was to avoid calls to VoxelShape.update -- see my previous comment above.

[jjhembd]

@mzschwartz5 thanks for the feedback! I think I addressed it all except for the question about how much (or which) state we should store. See my comment above for one possible resolution--let me know if there's a cleaner way to do it.

[mzschwartz5]

Per comment, only change I'd still like to see is storing the exaggeration and dirtying based on that, instead of storing old fields for everything that depends on exaggeration