-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Conversation
Thanks for the pull request @ptrgags!
Reviewers, don't forget to make sure that:
|
There was a problem hiding this 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
@sanjeetsuhag this is ready for review, just don't merge until #9872 is merged. |
@sanjeetsuhag just synced with |
There was a problem hiding this 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.
@sanjeetsuhag updated! |
There was a problem hiding this 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!
Now that vector and matrix types are supported natively, would it make sense to revert #9495? |
@lilleyse no, we still need to unpack |
Ah thanks for pointing me to that, I skimmed too fast 😄 |
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) andEXT_mesh_features
(new).This PR handles the difference in types between the two extensions:
type: SINGLE, componentType: FLOAT32
VECn
andMATn
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.