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

Restricting EXT_mesh_features to the definition of feature IDs #51

Conversation

javagl
Copy link

@javagl javagl commented Feb 9, 2022

The EXT_mesh_features extension is revised, and split into a EXT_mesh_features extension that only defines the concept of feature IDs, and a dedicated EXT_structured_metadata extension that defines how to associate features with property tables.

This PR adds parts of the original EXT_mesh_features README (as of https://github.com/CesiumGS/glTF/tree/e8b8a1065ac9e3db571fa8af836fac5480020228/extensions/2.0/Vendor/EXT_mesh_features ) back into the revised state. This is the part of the README that refers to feature IDs only.

The adjustments until now:

  • Removed specification text that defined property tables, schema, etc.
  • Minor updates of the remaining text (e.g. at places where the "Feature IDs" section talked about property tables)
  • Updated the FEATURE_ID_n attribute name to use the underscore: _FEATURE_ID_n
  • Removed the concepts of "Implicit Feature IDs"
  • Updated the inlined examples to use the new JSON structure for the definition of the feature IDs
    • Added some details about how to compute the feature ID from multiple channels
  • Omitted most of the examples

Preview: https://github.com/javagl/glTF/tree/ext-mesh-features-revision-spec/extensions/2.0/Vendor/EXT_mesh_features

Edited:

Things that still have to be done:

  • The concept of the nullFeatureId is not yet mentioned. The featureCount is already used, but not really explained in the text
  • The change log could/should contain more information about the actual JSON changes
  • The images have partially been updated, but until now, this just meant removing the property tables that they showed. We will consider to create dedicated examples with proper images that show exactly what is now covered with this part of the specification.

2022-02-09

  • The inlined examples still use feature ID arrays. They should be dictionaries

@javagl javagl marked this pull request as ready for review February 10, 2022 18:58
@lilleyse
Copy link

Looks great.

Do you plan on addressing these two points before the PR is merged?

  • The concept of the nullFeatureId is not yet mentioned. The featureCount is already used, but not really explained in the text
  • The inlined examples still use feature ID arrays. They should be dictionaries

To go along with the second bullet, there could be a deeper explanation about set IDs and how they can be used to identify the same set of feature IDs across primitives.

The revision history is now included in https://github.com/CesiumGS/3d-tiles/blob/extension-revisions/next/REVISION_HISTORY.md (with some light restructuring) so that section can be removed.

@lilleyse
Copy link

Nitpick - the "feature ID by vertex example" is great but I wish it stood out better and wasn't preceded by two Implementation note blocks. Maybe a preceding sentence like "Feature IDs can be used to differentiate batched objects like in the example below." Use your own wording, but something to that effect.

image

@javagl
Copy link
Author

javagl commented Feb 11, 2022

I did a slight refactoring, and separated the part about "Features" and "Feature IDs". The latter explains feature IDs in general, on the level of what their role is in mesh primitives, and also mentions nullFeatureId and featureCount. I think it makes sense to first explain the general concept, and then "incrementally" explain its specializations. A detail of that is the links to the schema definitions: This general section links to the mesh.primitive....features and featureId schema, with the subsections linking to the "specializations" of featureId.

This refactoring also avoids some overlap with the "Specifying Feature IDs" section: Part of this section has been moved in the the "Feature IDs" section now. The original "Specifying Feature IDs" is now called "Using Feature IDs", and can focus on how the IDs can be used. (I think that usually the "WHY?" is supposed to be explained before the "HOW?", but I think that the "WHY?" is sufficiently explained in the introduction, and the "Using Feature IDs" is rather on the level of "Examples/Use-Cases", and thus, better fits there).

Overall, I think that this structure is now "nicer" in terms of "reading-flow", and for building an understanding, but am open to suggestions for alternatives (including "Undo that commit!" ;-) ).

@javagl
Copy link
Author

javagl commented Feb 11, 2022

Do you plan on addressing these two points before the PR is merged?

The nullFeatureId and featureCount might be subject to changes as of CesiumGS/3d-tiles#624 , but I included their current meaning (in the refactored state).

The inlined examples have been changed to dictionaries (sorry, I had missed that).

The change log has been replaced by a link to the "common" change log.

@lilleyse
Copy link

The latest version looks really solid. I have some minor copy-edit type suggestions but I'll just apply those in a separate commit after this is merged.

@lilleyse lilleyse merged commit 0ed57a0 into CesiumGS:ext-mesh-features-revision Feb 11, 2022
@lilleyse
Copy link

c76b8ce

Main change to note there is the removal of the table of contents, which I don't think is needed anymore now that spec is a reasonable length.

@wallabyway
Copy link

I noticed that when there is only one feature set, that this text is repeated in every primitive unnecessarily.

image (6)

it adds a lot of bloat to the glTF.

Could we cater for a 'single feature set' and remove this data ?

@javagl
Copy link
Author

javagl commented Aug 22, 2024

The extension is closely associated with the mesh primitives. Each mesh primitive may nor may not have the feature ID attribute, and there may be multiple ones, and they may have different names...

In theory, one could add some extension object at the top-level of the glTF, saying "All primitives in this glTF will have the _FEATURE_ID_x attribute".

(Even deeper in theory, clients could just assume that an attribute that has this name is a feature ID attribute of EXT_mesh_features, but ... let's not go there 🙂 We'll rather have to think about that naming in general...).

But I think that there would have to be a strong(er) reasoning for trying to do this.

  • I can see the point that there is duplication (and duplication is not good, because it is duplication, and duplication is not good).
  • I can also see the point that it implies a certain "parsing overhard" and some processing logic of associating the IDs with the primitive. That could be simplified if there was only(!) some sort of top-level definition. But that's a somewhat weak point: This logic will be required even if there was an option to make such a top-level definition - so handling the top-level definition might in fact be what increases the implementation effort here.
  • One convincing reasoning could be the amount of data. I'm not sure how strong of a point that is. The marked code snippet contains ~100 bytes. Even when there are 100 primitives in one glTF, that's roughly 10KB, and would usually be negligible compared to the size of a glTF with so many mesh primitives.

(Maybe lilleyse also has some thoughts here)

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