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

Splitting EXT_mesh_features into multiple extensions #56

Merged
merged 96 commits into from
Mar 8, 2022

Conversation

lilleyse
Copy link

@lilleyse lilleyse commented Feb 21, 2022

This PR splits EXT_mesh_features into three separate extensions:

This was done for multiple reasons

  • The extension was doing too much
  • Feature identification and metadata can be cleanly decoupled
  • Different metadata encodings may emerge in the future
  • Several users of the extension are interested in feature IDs but want to use their own database

We even considered splitting EXT_structural_metadata into four separate extensions - one for the schema definition and three others for the encodings (property tables, property textures, property attributes) - but decided it was better to keep them together.

Opening the PR here for early feedback, then we'll updated the Khronos/glTF PRs. This includes the work from #51 #52 #55

To do:

  • Dictionary vs. array form for featureIds
  • From @ptrgags: When the property table offset/scale overrides the parent, should those be checked in lockstep? E.g. what if the class defined offset but not scale and the property table defines scale but not offset? even though the property table's offset is undefined, a default (probably 0) would make more sense than inheriting the class offset (which could be different)
  • Allow propertyAttributes to refer to components of an accessor. Allows properties to be packed into the same attribute without excessive padding (glTF requires 4-byte padding for attribute elements)

…-as-ids

Mention Feature IDs by Indices in specification text
Copy link

@javagl javagl left a comment

Choose a reason for hiding this comment

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

The extensions that this PR consists of had originally been a single extension. The original, single extension has been reviewed. Each PR that carved out one of the new extensions has been created in a dedicated PR that went into the branch that this PR is based on. There have been changes in each individual part, but these should be consistent with the overall structure and description. So I think this could be merged.

Only one question: There are some links like https://github.com/CesiumGS/3d-tiles/blob/extension-revisions/next/REVISION_HISTORY.md that refer to a branch (and not main) - at which point are these supposed to be be updated?

@javagl javagl merged commit 9ccb0f9 into 3d-tiles-next Mar 8, 2022
@lilleyse lilleyse deleted the ext-mesh-features-revision branch March 8, 2022 20:18
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