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

EXT_feature_metadata: featureTables/featureTextures → propertyTables/propertyTextures #17

Merged

Conversation

donmccurdy
Copy link

@donmccurdy donmccurdy commented Sep 20, 2021

Renames:

  • featureTablepropertyTable
  • featureTexturepropertyTexture

I've also attempted to use the term "properties" instead of "metadata" in a few cases, but may have a few more to clean up in the language/clarity pass.

We've discussed perhaps renaming the extension from EXT_feature_metadata. Also see #4. The nearest extension in the glTF specification is EXT_mesh_gpu_instancing, for reference. Ideas, in no particular order:

  • EXT_mesh_batching
  • EXT_mesh_feature_batching
  • EXT_mesh_properties
  • EXT_feature_properties
  • EXT_mesh_features
  • ... ?

If we do want to rename I'll do that update in a separate PR, but it does relate a bit to the wording used in this change. /cc @ptrgags @lilleyse

@lilleyse
Copy link

I added EXT_mesh_features to the list. I like it because (1) the word "batching" might be confusing / overloaded (2) it tells the reader enough information about the extension by its name and hopefully the the word "feature" can be inferred

From there the extension should be able to quickly answer

  • What is a feature?
  • How are features identified in a mesh?
  • What properties do features have?

@ptrgags
Copy link

ptrgags commented Sep 21, 2021

Hm, yeah EXT_mesh_features doesn't sound bad to me.

Though personally I like the sound of EXT_mesh_properties, the name dodges the confusion of "the geospatial community thinks a feature means one thing but the modeling feature thinks feature means something else..."

Thinking through it, if you zoom out that's what we're describing. we have a mesh, and whether at the vertices, or texels along each face there's some sort of properties associated with the geometry.

@donmccurdy
Copy link
Author

As a sanity check (since most of my experience with the word "feature" is in geospatial and product development contexts) here is a dictionary definition of the term:

"Feature". A distinctive attribute or aspect of something.

I think that's appropriate, although I do also like EXT_mesh_properties...

If we are using the word "mesh" then I don't think the term "batch" or "batching" is needed; a mesh does not have to represent a batch, the artist might be thinking of their product asset (e.g. a lamp) as a single mesh and they want to display contextual information when parts of the product are hovered or interacted with.

@lilleyse
Copy link

My main problem with the name EXT_mesh_properties is that it doesn't quite get to core of what the extension does. I think of other types of properties a mesh could have, like material properties or physics properties, and don't really think about the central mechanic of this extension which is to identify features within a mesh. Also properties are arguably secondary in this extension because you could have a feature table without any properties.

The definition for "features" fits really well. I don't think we have to shy away from the word feature too much.

@donmccurdy
Copy link
Author

I'm happy with EXT_mesh_features then — Do we want to go ahead and make that change?

@ptrgags
Copy link

ptrgags commented Sep 22, 2021

@donmccurdy yes EXT_mesh_features sounds good, though let's do that as a follow-up PR so the other changes don't get lost in the diff

@donmccurdy
Copy link
Author

@ptrgags sounds good, let me know if there are any other issues with the PR then!

Copy link

@ptrgags ptrgags left a comment

Choose a reason for hiding this comment

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

@donmccurdy noticed a couple things to be updated after the rename

@ptrgags
Copy link

ptrgags commented Sep 23, 2021

@donmccurdy looks good now, thanks!

@ptrgags ptrgags merged commit 36ceb70 into CesiumGS:3d-tiles-next-rev Sep 23, 2021
@donmccurdy donmccurdy deleted the ext-feature-metadata-v2.4 branch September 23, 2021 23:48
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