-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix shape bounds and transforms for VoxelCylinderShape #12522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thank you for the pull request, @jjhembd! ✅ We can confirm we have a CLA on file for you. |
|
|
||
| /** | ||
| * Defines the minimum bounds of the shape. Corresponds to minimum radius, height, angle. | ||
| * Defines the minimum bounds of the shape. Corresponds to minimum radius, angle. height. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Defines the minimum bounds of the shape. Corresponds to minimum radius, angle. height. | |
| * Defines the minimum bounds of the shape. Corresponds to minimum radius, angle, and height. |
I assume?
| }; | ||
| } | ||
|
|
||
| function getCylinderShape(cylinder, tileTransform) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there are no spec updates since everything now "just works" with the new sample data correct?
Should we add a unit test for the exaggeration as well to prevent any future regressions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, since we updated the sample data at the same time, the existing specs capture the behavior of the new data format.
I added a unit test for vertical exaggeration of cylinder voxels, which didn't work before this PR.
|
Thanks @jjhembd! Some small comments from me on the code. The testing sandcastle now works as expected for me. 👍 However, I noticed some strange behavior when adjusting the max angle in the inspector (this is again the provided testing Sandcastle, I just didn't hit the "Make bigger" button first). There appears to be some clipping on the right side of the frame:
I'm also seeing some unexpected behavior when adjusting the transforms in the inspector in both the "Voxels" and "Voxels in 3D Tiles" sandcastles. The translation doesn't seem to have any effect, and any scale adjustments end up flattening the shape completely:
|
|
Thanks @ggetz! I addressed your code comments. For your last two observations:
|
|
|
All looks fantastic. Thanks @jjhembd! |


Description
Recent updates to the 3D Tiles extension 3DTiles_bounding_volume_cylinder included a change in the way the shape bounds were defined. Instead of an oriented bounding box, the extension now defines the radius, angle, and height bounds, along with an optional transform specified as a rotation and translation.
This PR updates the test datasets to follow the new spec, and reworks the
getCylinderShapehelper inCesium3DTilesVoxelProviderto read the new values.Other changes include:
(radius, angle, height)coordinate order everywhere. This fixes the naming and functionality of theBoundsandClippingeditors inVoxelInspector.updateVerticalExaggerationhelper inVoxelPrimitive. This enables vertical exaggeration of cylinders, and also passes dynamic updates to themodelMatrixthrough toprimitive._exaggeratedModelMatrixso that they are accounted for in the rendering.Issue number and link
Fixes #12275.
Fixes #12276.
Fixes #12521.
Testing plan
VoxelPrimitiveSpecandCesium3DTilesVoxelProviderSpeclocally.Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change