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

Store feature tables and feature textures as arrays to support EXT_mesh_features #9872

Merged
merged 26 commits into from
Oct 21, 2021

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Oct 13, 2021

Recently we've been revising EXT_feature_metadata. This has resulted in a newer draft of the schema, EXT_mesh_features.

One of the biggest changes is that metadata textures and tables are now defined with an array instead of a dictionary. This PR updates GltfLoader to handle this array-based storage.

This adds most support for EXT_mesh_features, however, I have not yet handled the differences in data type schema, I'll do that as a separate PR since this was already a ~2000 line change.

I also updated 3 of the test datasets (weather, microcosm, and box instanced) to use the new extension.

@sanjeetsuhag could you review?

CC: @lilleyse

@ptrgags ptrgags requested a review from sanjeetsuhag October 13, 2021 18:39
@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

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.

@ptrgags I have done a first pass over the code. Have not looked at the tests yet.

Source/Scene/FeatureTable.js Outdated Show resolved Hide resolved
Source/Scene/ModelComponents.js Outdated Show resolved Hide resolved
Source/Scene/ModelComponents.js Show resolved Hide resolved
Source/Scene/FeatureMetadata.js Outdated Show resolved Hide resolved
Source/Scene/FeatureTable.js Outdated Show resolved Hide resolved
Source/Scene/GltfFeatureMetadataLoader.js Outdated Show resolved Hide resolved
Source/Scene/GltfFeatureMetadataLoader.js Outdated Show resolved Hide resolved
Source/Scene/ModelComponents.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 18, 2021

@sanjeetsuhag updated!

@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 19, 2021

@sanjeetsuhag just synced with main to hopefully avoid those CI failures

@sanjeetsuhag
Copy link
Contributor

Changes look good to me. Models are loading fine and picking works for both old and new extension.

@lilleyse Would you like to take a look before I merge?

Source/Scene/FeatureTable.js Outdated Show resolved Hide resolved
Source/Scene/GltfFeatureMetadataLoader.js Outdated Show resolved Hide resolved
Source/Scene/GltfFeatureMetadataLoader.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
Source/Scene/GltfLoader.js Show resolved Hide resolved
Source/Scene/ModelComponents.js Show resolved Hide resolved
Specs/Data/Models/GltfLoader/Weather/glTF/weather.gltf Outdated Show resolved Hide resolved
Specs/Scene/GltfFeatureMetadataLoaderSpec.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 21, 2021

@lilleyse at this point I've addressed your feedback, though I still have to go through and check test coverage to see what tests to add.

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.

Coverage is pretty good, just a few places that need improvement

@ptrgags
Copy link
Contributor Author

ptrgags commented Oct 21, 2021

@lilleyse updated! I also opened #9884 for the "feature IDs without property table" handling.

@lilleyse
Copy link
Contributor

Looks good!

@lilleyse lilleyse merged commit 48d7882 into main Oct 21, 2021
@lilleyse lilleyse deleted the mesh-features-transcoder branch October 21, 2021 21:59
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