Skip to content

Fix glTF voxel ordering for ellipsoid shapes #12544

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

Merged
merged 6 commits into from
Apr 1, 2025
Merged

Conversation

jjhembd
Copy link
Contributor

@jjhembd jjhembd commented Mar 31, 2025

Description

#12432 updated Cesium3DTilesVoxelProvider to read from tilesets containing glTF tiles, conforming to the proposed update to the EXT_primitive_voxels extension. This included a change to the ordering of the input metadata, based on glTF conventions for the up-axis.

However, the updated extension did not change the ordering of the metadata for ellipsoid-shaped voxels, since the (longitude, latitude, altitude) ordering defines its own "up-axis" along altitude. #12432 did not consider the ellipsoid case, and therefore broke ellipsoid-shaped voxel tilesets.

This PR updates the logic to consider the metadata order for all VoxelShapeTypes. Along the way, the keys of the VoxelMetadataOrder enum were updated to the more explicit Z_UP and Y_UP, since GLTF does not really describe what is happening in each case.

I also updated the VoxelEllipsoid3DTiles sample dataset to make the orientation of the data apparent in the Sandcastle, so we can catch problems like this more quickly.

Issue number and link

Testing plan

Load the "Voxels in 3D Tiles" Sandcastle and verify that the colors render with the following correspondence:

  • Red = longitude
  • Green = latitude
  • Blue = altitude

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

Copy link

Thank you for the pull request, @jjhembd!

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

@jjhembd
Copy link
Contributor Author

jjhembd commented Mar 31, 2025

@ggetz for the note in CHANGES.md, I listed this as a fix. However, it technically includes a breaking change, since we changed the keys of the public enum VoxelMetadataOrder.
Perhaps VoxelProvider.metadataOrder could be private? It defaults to Z_UP for non-3D Tiles voxels.

@ggetz
Copy link
Contributor

ggetz commented Mar 31, 2025

Perhaps VoxelProvider.metadataOrder could be private? It defaults to Z_UP for non-3D Tiles voxels.

If it is part of a public interface, I don't believe we can make it private. However, we can remove it from the public interface and only implement it for Cesium3DTilesVoxelProvider.

Technically speaking, we should probably list breaking changes as such in CHANGES.md, even if (hopefully) not many users are depending on it yet.

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.

@jjhembd Code here is looking good! Let's just make sure to address the above comment, and this should then be good to merge.

Thanks for the quick fix!

@jjhembd
Copy link
Contributor Author

jjhembd commented Mar 31, 2025

we can remove it from the public interface and only implement it for Cesium3DTilesVoxelProvider

@ggetz I think this makes sense. I removed metadataOrder from the VoxelProvider interface and marked the enum as @private. This is called out in a "breaking change" notice in CHANGES.md.

@ggetz
Copy link
Contributor

ggetz commented Apr 1, 2025

Sounds good to me! Thanks @jjhembd!

@ggetz ggetz added this pull request to the merge queue Apr 1, 2025
Merged via the queue into main with commit d503ccc Apr 1, 2025
9 checks passed
@ggetz ggetz deleted the ellipsoid-voxel-order branch April 1, 2025 14:20
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.

2 participants