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

Feature ID changes from EXT_mesh_features spec updates #10087

Merged
merged 21 commits into from
Feb 11, 2022

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Feb 9, 2022

This PR targets a staging branch, feature-id-changes

the EXT_mesh_features extension is under some revisions, being split into EXT_mesh_features and EXT_structural_metadata.

This PR doesn't cover everything, but it's a start. It includes:

  • the FEATURE_ID_n -> _FEATURE_ID_n attribute semantic
  • added nullFeatureId and featureCount to ModelComponents.FeatureIdXxx (though these are not yet used for anything)
  • feature ID textures now have the texture info moved into a texture property.
  • Parse feature IDs from a multi-channel texture

Sandcastle for multi-channel Feature IDs -- some of the feature ID textures are a bit contrived, I need to find a better way to check them for correctness. I also need to try picking/styling with the one texture that has null feature IDs.

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

Source/Scene/GltfLoader.js Outdated Show resolved Hide resolved
@ptrgags ptrgags changed the base branch from metadata-spec-changes-staging to extension-revisions February 10, 2022 13:51
@ptrgags ptrgags marked this pull request as ready for review February 10, 2022 21:06
@ptrgags
Copy link
Contributor Author

ptrgags commented Feb 10, 2022

@lilleyse the code is now ready for review. Note that I want to make two Sandcastles for testing:

  • Sandcastle link Update the sandcastle from the description to have better visualizations for the larger feature ID values -- the comments explain what should happen. Everything works fine except the RGBA one which fails due to precision issues (as expected).
  • Make a sandcastle for picking the 7th feature ID texture to check if the null feature ID code works correctly -- not actually possible since there's no property table, see Model EXT_mesh_features: handle feature IDs without a property table #9884

@ptrgags
Copy link
Contributor Author

ptrgags commented Feb 10, 2022

image
in that first sandcastle, it seems that fractal pattern I found yesterday happens only in the case where I use all 4 channels (RGBA for the first, AGBB for the second). Somehow the gradients in each channel + precision limitations combine to make a fractal pattern.

image

These precision issues were to be expected given that highp int is actually represented as a float32 in GLSL 1.00. All the other cases with 1-3 channels work fine.

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.

Looks good! Just a couple comments. I tested picking in the 3D Tiles Next sandcastles as well as pnts, i3dm, and b3dm with enableModelExperimental: true.

@@ -11,6 +11,7 @@
- Added support for shadows in `ModelExperimental`. [#10077](https://github.com/CesiumGS/cesium/pull/10077)
- Added the ability to load implicit tiling subtree files from JSON. [#10086](https://github.com/CesiumGS/cesium/pull/10086)
- Added support for subtree metadata in tilesets with implicit tiling. [#10093](https://github.com/CesiumGS/cesium/pull/10093)
- Updated Feature ID parsing to match changes to the EXT_mesh_features specification. [#10087](https://github.com/CesiumGS/cesium/pull/10087)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lilleyse just a note, I do plan to clean up this changelog before we merge the staging branch. it'll probably be best to make a list of sub-bullets (like we did when we first introduced 3D Tiles Next) but that'll be easier once everything is in place.

@ptrgags
Copy link
Contributor Author

ptrgags commented Feb 11, 2022

@lilleyse updated!

@lilleyse lilleyse merged commit 24f04d7 into extension-revisions Feb 11, 2022
@lilleyse lilleyse deleted the feature-id-changes branch February 11, 2022 21:52
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