Skip to content

Use consistent min/maxBounds handling in VoxelProvider and VoxelInspector #12551

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

Open
jjhembd opened this issue Apr 4, 2025 · 1 comment
Open

Comments

@jjhembd
Copy link
Contributor

jjhembd commented Apr 4, 2025

Cesium3DTilesVoxelProvider.prototype.minBounds and .maxBounds are currently not consistent for different VoxelShapeTypes:

  • For VoxelShapeType.BOX, the bounds are set to VoxelBoxShape.DefaultMinBounds and VoxelBoxShape.DefaultMaxBounds. The actual physical bounds as defined in the boundingVolume in the tileset.json are incorporated into the shapeTransform
  • For VoxelShapeType.CYLINDER, the bounds are set to the physical values from 3DTILES_bounding_volume_cylinder. The shapeTransform is constructed from the translation and rotation from the same extension.
  • For VoxelShapeType.ELLIPSOID, the bounds are set to the physical longitude, latitude, and height bounds. The shapeTransform captures the scaling of the ellipsoid axes.

For better consistency, we should use physically meaningful values in all shape types. This will require us to:

  1. Update the getBoxShape helper function in Cesium3DTilesVoxelProvider to store the physical bounds. We will then need to verify the shapeTransform throughout the rendering flow, to make sure we are not double-scaling anything.
  2. Update the VoxelInspectorViewModel.voxelPrimitive setter to apply the actual bounds to the min and max of the range slider (they are currently set to constant values based on the DefaultMinBounds and DefaultMaxBounds)
@jjhembd
Copy link
Contributor Author

jjhembd commented Apr 25, 2025

Another inconsistency between the shape types is the way we handle the transforms.

Current transform behavior

We have the following defined in VoxelProvider:

  /**
   * A transform from local space to global space.
   *
   * @default Matrix4.IDENTITY
   * @readonly
   */
  globalTransform: { /* ... */ },

  /**
   * A transform from shape space to local space.
   *
   * @default Matrix4.IDENTITY
   * @readonly
   */
  shapeTransform: { /* ... */  },

I think the descriptions are not very clear: what is shape space? What is local space?

These transforms date from a time when the shape and its bounds were defined in a "shape space" which covered [0, 1] in each dimension. (Except that sometimes it spanned [-1, 1], or [-pi, pi], or [-pi/2, -pi/2]... depending on which shape and which bound we were talking about.)

The globalTransform and shapeTransform were then either supplied by the user (in the case of custom VoxelProviders) or constructed from the tileset.json, using the root tile transform and some transform parameters derived from the shape bounds. The construction and meaning of each transform were different for each shape. But for 3D Tiles, the globalTransform mostly captured the effects of the root tile transform, while the shapeTransform captured something from the shape bounds or definition.

When the globalTransform and shapeTransform are actually used, they are never used separately: we always multiply them and use the product.

Proposed transform structure

We now want the shape bounds to be defined as physical dimensions, so the range of shape coordinates is now user-defined (except for angles, which are still [-pi/2, pi/2] or [-pi, pi]).

I propose that globalTransform and shapeTransform can be combined into one matrix:

/**
 * A transform from the space in which the bounds are defined to global ECEF coordinates.
 *
 * @default Matrix4.IDENTITY
 * @readonly
 */
 transform: { /*... */ },

For custom providers, the user would now provide only one transform. For 3D tilesets, the transform would combine values from both the shape definition and the root tile transform.

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

No branches or pull requests

2 participants