Skip to content
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

3D Tiles Next Metadata: Automatically unpack vector types #9495

Merged
merged 5 commits into from
Apr 20, 2021

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Apr 19, 2021

This is another PR that targets the 3d-tiles-next branch.

This PR is to maintain backwards compatibility with the old batch table, where vector types are automatically parsed as Cartesian(2|3|4) types.

The following types are automatically packed: ARRAYs with componentCount of 2, 3, or 4, and one of the following componentTypes:

  • UINT8
  • UINT16
  • UINT32
  • INT8
  • INT16
  • INT32
  • FLOAT32
  • FLOAT64

64-bit integers are NOT supported as vectors, as they are not compatible with the Cartesian classes.

Local Sandcastle

@lilleyse could you review?

@ptrgags ptrgags requested a review from lilleyse April 19, 2021 20:30
@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@ptrgags
Copy link
Contributor Author

ptrgags commented Apr 19, 2021

@lilleyse merge conflicts are fixed!

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

I'm glad the code changes were minimal here. (Ignoring the large amount of unit test changes which look good as well 😄)

@@ -227,6 +227,7 @@ MetadataEntity.getProperty = function (

if (defined(classProperty)) {
value = classProperty.normalize(value);
value = classProperty.unpackVectorTypes(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

In the future we may want to consider adding a result parameter to getProperty so it doesn't need to allocate the cartesian object. Looking at Cesium3DTileBatchTable it doesn't take a result parameter for getProperty either, maybe because the type could be anything without the new stricter type system?

@lilleyse lilleyse merged commit 70d0d31 into 3d-tiles-next Apr 20, 2021
@lilleyse lilleyse deleted the unpack-vectors branch April 20, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants