EXT_mesh_features: Language and clarity updates#21
EXT_mesh_features: Language and clarity updates#21donmccurdy wants to merge 12 commits intoCesiumGS:3d-tiles-next-revfrom
Conversation
|
A number of helpful comments in #4 as well — I think many are covered here, but the common themes are worth keeping in mind when reviewing:
|
ptrgags
left a comment
ptrgags
left a comment
There was a problem hiding this comment.
@donmccurdy did a review pass. No major changes, mostly just minor wording tweaks.
extensions/2.0/Vendor/EXT_mesh_features/schema/schema.schema.json
Outdated
Show resolved
Hide resolved
Co-authored-by: Peter Gagliardi <ptrgags@gmail.com>
donmccurdy
left a comment
donmccurdy
left a comment
There was a problem hiding this comment.
Pushed an update addressing most feedback – I went ahead and listed allowed values for type and componentType exhaustively in the "Class Properties" section, but left the other properties in the schema file out, trying to find the right balance there...
|
That was weird, I just refreshed the page and GH acted like I submitted the review? 🤷♂️ Oh well, @donmccurdy I was done making comments anyway, there's a few things to update and discuss |
donmccurdy
left a comment
donmccurdy
left a comment
There was a problem hiding this comment.
I still need to implement the changes suggested in the last round, but wanted to go ahead and follow up on a couple of the comment threads that are still open.
/cc @ptrgags
- Expand raster format description. - Relax byte alignment requirements to match component size, not strictly 8-byte. - Clarify that GPU instancing does not allow property textures at the node level.
donmccurdy
left a comment
donmccurdy
left a comment
There was a problem hiding this comment.
Latest commits should address open comments — I've also added myself to the contributors section (happy to adjust that if needed though).
ptrgags
left a comment
ptrgags
left a comment
There was a problem hiding this comment.
@donmccurdy almost there, just a few last things. I think we can knock this out today
|
Whoops, looks like GH closed this automatically rather than updating the base branch... @donmccurdy after making updates could you open a new PR? |
donmccurdy
left a comment
donmccurdy
left a comment
There was a problem hiding this comment.
Remaining feedback addressed in a new PR, #25.
4 participants
See rendered markdown preview.
This PR does not include any (intentional) technical changes, but does specify some restrictions that were probably intended but not previously explicit. I've updated terminology in the illustrations, but haven't altered illustrations much otherwise. I've tried to introduce concepts "in order", but early references to later topics are necessary in a few places and I've tried to include links accordingly.
Written descriptions of properties are not exhaustive — hopefully this is a good balance of providing enough detail without duplicating content from the Core Metadata specification. Each section now links to a schema file, which does include exhaustive property references.