Conversation
pjcozzi
left a comment
There was a problem hiding this comment.
@ptrgags @lilleyse this overall looks pretty solid to me, most of my comments are on the writing and the naming. However, I only skimmed the JSON reference so let me know if there is a specific part of the schema that it would be helpful to deep dive on.
I don't think that this needs to be split into more than one extension but let's discuss if/when Feature Textures should be used.
Other discussion topics:
- Name this extension from the context of 3D Tiles or glTF? E.g. "batched"
- Mix and match: features in different buffers; vertices with multiple feature ids; using both feature tables and feature textures. Help the reader build the mental model. Probably need more diagrams
- Writing
- Mixing the why and the what/how: batching in the intro, multiple feature IDs
- With core concepts, start broad then go narrow/specific. Try not to implicitly introduce overall concepts and structure implicitly while introducing the details that implement them.
- JSON examples needs prose introducing them
- Needs an appendix / sample models to show complete connections between schema, feature IDs, feature table, etc.
- Feature Texture -> “Feature Metadata Texture” ?
- Use case for Feature Textures - high frequency?
@abwood @emackey this glTF extension is basically the next generation Batch Table for 3D Tiles so that now 3D Tiles will contain .gltf/.glb + this extension instead of .b3dm and .i3dm, and that it will have more flexible and semantically-rich ways to store metadata. Any chance you have a few cycles for a shallow or deep review??? 🙏
| @@ -0,0 +1,1665 @@ | |||
| <!-- omit in toc --> | |||
| # EXT_feature_metadata | |||
There was a problem hiding this comment.
Let's discuss if "feature metadata" is the right name - and in what context should the name be "right", e.g.,
- This is a glTF context so should the name be more easily understand to that community, or
- This was built for 3D Tiles so should the name be more easily understand to that community?
If the former, then this extension might be something like "batched_metadata", "batched_objects", "batched_meshes", etc.; however we do want the names within the extension, e.g., "feature" to line up with the rest of the 3D Tiles ecosystem I think.
There was a problem hiding this comment.
Thoughts, in no particular order:
- glTF extension naming convention is
<PREFIX>_<scope>_<feature>where "scope" is preferably a noun. For a name likebatched_objectsorbatched_meshesI would expect this extension to define the batching mechanism, but arguably the batching mechanism is already there, and this extension provides the metadata/properties that make batches interactive. - In my mind "feature" is a geospatial term, but the concept (i.e. a part within a primitive) does not exist in glTF today so I'm not aware of any other precedent.
- The term "metadata" is a little overloaded, e.g
KHR_xmp_json_ldwrites, "metadata has no normative effect on the glTF asset appearance and rendering". Arguably "properties" (e.g. GeoJSON properties) would be another option here.
Still thinking this one over, but leaning along lines of...
EXT_[feature | batch]_[metadata | properties]
There was a problem hiding this comment.
Renamed to EXT_mesh_features in #20 – hopefully the right balance here, with "features" from 3D Tiles ecosystem and similar "mesh" prefix to EXT_mesh_gpu_instancing. Comments still welcome though!
| @@ -0,0 +1,1665 @@ | |||
| <!-- omit in toc --> | |||
There was a problem hiding this comment.
Just a heads up that I was a bit confused as to why this is in master, CC #3. Even though this is a fork of the main glTF repo, I would suggest to keep master in sync with the main repo or - if there is a good reason - to have another branch in this repo that mirrors master from the main repo.
For this pull request, I just picked a branch that was out of date so the diff for EXT_feature_metadata would be available.
There was a problem hiding this comment.
Created a new branch for this extension and future Cesium extensions and retargeted this PR: https://github.com/CesiumGS/glTF/tree/3d-tiles-next
|
|
||
| Written against the glTF 2.0 specification. | ||
|
|
||
| Adds new functionality to the [`EXT_mesh_gpu_instancing` extension](../../EXT_mesh_gpu_instancing). |
There was a problem hiding this comment.
Does this add new functionality to the GPU instancing extension? Or is the feature metadata extension dependent on GPU instancing?
I believe it is the later; this extension gets the new functionality; it does not modify the GPU instancing extension.
This is just wording, but it is important to get right so it doesn't confuse the reader or the ecosystem.
There was a problem hiding this comment.
Updated in #21:
Optionally, this extension may be used in conjunction with EXT_mesh_gpu_instancing. When used together, certain GPU instance attributes defined by EXT_mesh_gpu_instancing are used as instance feature IDs.
|
|
||
| ## Overview | ||
|
|
||
| A **feature** is an entity that has both geometry and metadata. In Geographic Information Systems (GIS) a feature is an entity such as a point, polyline, or polygon that represents some element on a map. In another domain like CAD/BIM a feature might be a component of a design model. A feature could also be a 3D building in a city, a tree in a forest, a sample point in a weather model, or a patch of texels on a 3D model. |
There was a problem hiding this comment.
Start with why
Before describing what this extension is, e.g., what a feature is.
Start with the motivation for the extension in the first place and a few use cases to make that concrete.
| } | ||
| ``` | ||
|
|
||
| ## Examples |
There was a problem hiding this comment.
A good supplement to this table that will help the reader build their mental model would be a diagram that shows the breath of granularity from per-texel to per-vertex to per-triangle to per-node, etc.
That would be helpful early in the spec.
| Per-triangle metadata|An implicit feature ID is assigned to each set of three vertices. The feature table stores `FLOAT64` area values.| | ||
| Per-point metadata|An implicit feature ID is assigned to each point. The feature table stores `FLOAT64` , `STRING`, and `ENUM` properties, which are not possible through glTF vertex attribute accessors alone.| | ||
| Per-node metadata|Vertices in node 0 and node 1 are assigned different `constant` feature IDs. Because the door has articulations these two nodes can't be batched together.| | ||
| Multi-point features|A point cloud with two feature tables, one storing metadata for groups of points and the other storing metadata for individual points.| |
There was a problem hiding this comment.
This is the first time that multiple feature tables are introduced. That concept should be intrduced separately and motivated by the potential need to have multiple classes in the same glTF.
The text here should also be expanded to note the use of both explicit and implicit feature IDs if I understand it correctly.
If the format of a table proves difficult to give each of these examples enough context, just switch to making each example / row in the table its own subsection.
| Per-point metadata|An implicit feature ID is assigned to each point. The feature table stores `FLOAT64` , `STRING`, and `ENUM` properties, which are not possible through glTF vertex attribute accessors alone.| | ||
| Per-node metadata|Vertices in node 0 and node 1 are assigned different `constant` feature IDs. Because the door has articulations these two nodes can't be batched together.| | ||
| Multi-point features|A point cloud with two feature tables, one storing metadata for groups of points and the other storing metadata for individual points.| | ||
| Multi-instance features|Instanced tree models where trees are assigned to groups with a per-instance feature ID attribute. One feature table stores per-group metadata and the other stores per-tree metadata.| |
There was a problem hiding this comment.
Vocab. Is "group" here aligned with group in the other 3D Tiles Next specs?
| Multi-point features|A point cloud with two feature tables, one storing metadata for groups of points and the other storing metadata for individual points.| | ||
| Multi-instance features|Instanced tree models where trees are assigned to groups with a per-instance feature ID attribute. One feature table stores per-group metadata and the other stores per-tree metadata.| | ||
| Material classification|A textured mesh using a feature texture to store both material enums and normalized `UINT8` thermal temperatures.| | ||
| Composite|A glTF containing a 3D mesh (house), a point cloud (tree), and instanced models (fencing) with three feature tables.| |
There was a problem hiding this comment.
Will calling this composite confuse readers familiar with composite tiles in 3D Tiles?
| <!-- omit in toc --> | ||
| ### glTF extension | ||
|
|
||
| glTF extension that assigns metadata to features in a model. |
There was a problem hiding this comment.
in a model
Throughout, please avoid "glTF = model"
From https://www.khronos.org/gltf/:
glTF™ (GL Transmission Format) is a royalty-free specification for the efficient transmission and loading of 3D scenes and models by engines and applications.
March 8, 2021Peter, Patrick, Sean General NotesFeature Textures
Big Picture
|
|
I'm having trouble figuring out how to leave comments at arbitrary locations in this PR, since the "Files changed" list doesn't include the extension itself... is this a good place to leave comments/questions, and if so does it need a rebase to allow that? Would it be more, or less, confusing if I opened a new PR for comments/questions? |
b3f48c2 to
3bf5af9
Compare
|
Unfortunately syncing |
@ptrgags @lilleyse this pull request is not meant to be merged; it is just for feedback from the spec deep dive.