Skip to content

Conversation

@jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Apr 30, 2025

Description

This PR changes the way we handle the bounds of a voxel dataset. Previously, VoxelPrimitive.prototype.minBounds and VoxelPrimitive.prototype.maxBounds were defined in a "shape space", spanning nominal ranges such as [0..1], [-1.. 1], [-pi/2.. pi/2], or [-pi.. pi]. This was hard to document consistently, and inconsistent with the way bounds are defined in 3D Tiles.

After this PR, the minBounds and maxBounds are physical values, consistent with what is defined in the tileset.json.

Issue number and link

Resolves #12551.

Testing plan

Load the Voxels in 3D Tiles Sandcastle, and play with the clipping bounds. The extreme values of the sliders should represent the physical dimensions, and clipping should work normally.

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
Copy link

Thank you for the pull request, @jjhembd!

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

Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jjhembd! Besides the know TODO's, code is looking good so far. Let me know when you'd like a final review and merge.

@jjhembd jjhembd marked this pull request as ready for review May 1, 2025 13:12
@jjhembd
Copy link
Contributor Author

jjhembd commented May 1, 2025

Thanks for the review @ggetz! I think I addressed all your feedback and this is ready for another look.
I also tested it on several real-world datasets, of all 3 shape types: BOX, CYLINDER, and ELLIPSOID. Everything works as expected, including vertical exaggeration.

@ggetz
Copy link
Contributor

ggetz commented May 1, 2025

@jjhembd Testing this out, everything is working great in the "Voxels" example. It's much more intuitive controlling the bounds and clipping bounds with this change.

I tried the "Voxels in 3D Tiles" example and modifying the clipping bounds all works as expected. However, for the "Bounds", adjusting any of the components seems to visually set that component to 0. This applies to both Box and Cylinder shapes. Is that expected?

@jjhembd
Copy link
Contributor Author

jjhembd commented May 1, 2025

However, for the "Bounds", adjusting any of the components seems to visually set that component to 0.

@ggetz The slider ranges for the "Bounds" are currently not adjusted in VoxelInspectorViewModel. I just now tried making them update when a new VoxelPrimitive is tracked, but it's creating a funny feedback loop which might take a little while to figure out (we read the min/maxBounds to set the clippingBounds, etc).

Long term I think we should make the bounds fixed. Adjustable bounds was implemented for debugging (I assume), but for a finished app, I think adjusting bounds is really a task for a model editor rather than a renderer.

Would it make sense to remove the "Bounds" tracker from the VoxelInspector for now? Then we could have a followup later to make the bounds read-only in the VoxelPrimitive API itself, perhaps after we have had a chance to discuss further.

@ggetz
Copy link
Contributor

ggetz commented May 1, 2025

Long term I think we should make the bounds fixed. Adjustable bounds was implemented for debugging (I assume), but for a finished app, I think adjusting bounds is really a task for a model editor rather than a renderer.

Would it make sense to remove the "Bounds" tracker from the VoxelInspector for now? Then we could have a followup later to make the bounds read-only in the VoxelPrimitive API itself, perhaps after we have had a chance to discuss further.

This all tracks. Do you plan on removing those properties from the voxel inspector in this PR, or a follow-up?

@jjhembd
Copy link
Contributor Author

jjhembd commented May 1, 2025

@ggetz sounds good, I just pushed a commit to remove the "Bounds" editor from VoxelInspector.
I can open a follow-up issue/PR to make the bounds read-only in VoxelPrimitive.

@ggetz
Copy link
Contributor

ggetz commented May 1, 2025

Great, thanks @jjhembd!

@ggetz ggetz added this pull request to the merge queue May 1, 2025
Merged via the queue into main with commit 4d08517 May 1, 2025
9 checks passed
@ggetz ggetz deleted the voxel-bounds branch May 1, 2025 15:19
@lrjcx
Copy link

lrjcx commented Jun 4, 2025

new Cesium.PostProcessStage fragmentShader use czm_globeDepthTexture has a question TypeError: Cannot read properties of undefined (reading '_target'),how I can do it

@ggetz
Copy link
Contributor

ggetz commented Jun 4, 2025

@lrjcx Would you be able to include a Sandcastle example that replicates the issue? That would help us give us more context to determine the issue. Thanks!

@lrjcx
Copy link

lrjcx commented Jun 5, 2025

@ggetz
Sandcastle example When the camera moves to the sky, it will report an error.

@lrjcx
Copy link

lrjcx commented Jun 6, 2025

@ggetz Sandcastle That's the current address, thank you。 When the camera moves to the sky, it will report an error.

@ggetz
Copy link
Contributor

ggetz commented Jun 6, 2025

@lrjcx Thanks for the example! Could you please let us know how that is related to this PR? If it's not, could you please open a new issue, or ask a question on the Cesium Forum?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use consistent min/maxBounds handling in VoxelProvider and VoxelInspector

4 participants