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

Handle changes in metadata types from EXT_mesh_features #9880

Merged
merged 16 commits into from
Oct 22, 2021

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Oct 19, 2021

Wait to merge this until #9872 is merged.

This PR is the second in a series to update our implementation to support both EXT_feature_metadata (old) and EXT_mesh_features (new).

This PR handles the difference in types between the two extensions:

  • we simplified the types so individual values look similar to: type: SINGLE, componentType: FLOAT32
  • we added dedicated VECn and MATn types for vectors/matrices.

This does NOT handle changing optional/default -> noData/required, that will be a follow-up PR since this one was already quite large.

@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.

Copy link
Contributor Author

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

Just checked the code coverage, it's pretty close, just a few places need additional tests

Source/Scene/MetadataClassProperty.js Outdated Show resolved Hide resolved
Source/Scene/MetadataClassProperty.js Show resolved Hide resolved
Source/Scene/MetadataClassProperty.js Show resolved Hide resolved
Source/Scene/MetadataComponentType.js Show resolved Hide resolved
Source/Scene/MetadataType.js Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 21, 2021

@sanjeetsuhag this is ready for review, just don't merge until #9872 is merged.

Base automatically changed from mesh-features-transcoder to main October 21, 2021 21:59
@ptrgags ptrgags marked this pull request as ready for review October 22, 2021 12:13
@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 22, 2021

@sanjeetsuhag just synced with main to pull in the two PRs from yesterday.

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

The changes here look good. Tested loading of glTFs and tilesets with metadata and they are working fine. Coverage is fine too. The documentation needs a little work.

Source/Scene/MetadataClassProperty.js Show resolved Hide resolved
Source/Scene/MetadataClassProperty.js Outdated Show resolved Hide resolved
Source/Scene/MetadataClassProperty.js Show resolved Hide resolved
Source/Scene/MetadataClassProperty.js Outdated Show resolved Hide resolved
Source/Scene/MetadataComponentType.js Outdated Show resolved Hide resolved
Source/Scene/parseBatchTable.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 22, 2021

@sanjeetsuhag updated!

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

Latest commits look good. Thanks @ptrgags!

@sanjeetsuhag sanjeetsuhag merged commit 3ee0c96 into main Oct 22, 2021
@sanjeetsuhag sanjeetsuhag deleted the mesh-features-types branch October 22, 2021 18:13
@lilleyse
Copy link
Contributor

Now that vector and matrix types are supported natively, would it make sense to revert #9495?

@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 22, 2021

@lilleyse no, we still need to unpack VECN and MATN, just not things like ARRAY[FLOAT32, N], this PR already accounts for that. See these lines: https://github.com/CesiumGS/cesium/pull/9880/files#diff-a559a016280a5baf661f1c19a79de91a848b919fe0f0b5ed3f44efd261009036L327-R366

@lilleyse
Copy link
Contributor

Ah thanks for pointing me to that, I skimmed too fast 😄

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.

4 participants