Edit pass over the 3D Tiles Next extensions #62

lilleyse · 2022-03-13T21:05:55Z

Comments inlined...

Preview links:

EXT_instance_features
EXT_mesh_features
EXT_structural_metadata

lilleyse · 2022-03-13T21:06:07Z

extensions/2.0/Vendor/EXT_instance_features/README.md

+## Optional vs. Required
+
+This extension is optional, meaning it should be placed in the `extensionsUsed` list, but not in the `extensionsRequired` list.


Moved the Optional vs. Required section sooner in the document

lilleyse · 2022-03-13T21:07:18Z

extensions/2.0/Vendor/EXT_instance_features/README.md

+A feature ID set may also include the following properties:

-This extension is optional, meaning it should be placed in the `extensionsUsed` list, but not in the `extensionsRequired` list.
+* `nullFeatureId`: a value that indicates that a certain instance is not considered to be an identifiable object
+* `label`: an alphanumeric string used to identify feature ID sets across different glTF nodes
+* `propertyTable`: the index of a property table in the [`EXT_structural_metadata`](../EXT_structural_metadata/) extension
+
+> **Example:** A more complex example with two feature ID sets. The first feature ID set groups instanced trees into forests. The tree with feature ID `2` matches the `nullFeatureId` and therefore does not belong to a forest feature. The second feature ID set (feature ID values not shown) is defined implicitly based on instance index, and identifies individual trees. Each feature ID set refers to a property table that contains metadata about the features.
+>
+> ![Feature ID by instance](figures/trees.png)
+>
+> ```jsonc
+> {
+>   "nodes": [
+>     {
+>       "mesh": 0,
+>       "extensions": {
+>         "EXT_mesh_gpu_instancing": {
+>           "attributes": {
+>             "TRANSLATION": 0,
+>             "ROTATION": 1,
+>             "SCALE": 2,
+>             "_FEATURE_ID_0": 3
+>           },
+>         },
+>         "EXT_instance_features": {
+>           "featureIds": [
+>             {
+>               "nullFeatureId": 2,
+>               "featureCount": 2,
+>               "attribute": 0,
+>               "propertyTable": 0,
+>               "label": "Forests"
+>             },
+>             {
+>               "featureCount": 9,
+>               "propertyTable": 1,
+>               "label": "Trees"
+>             }
+>           ]
+>         }
+>       }
+>     }
+>   ]
+> }
+> ```


Added a second example to show other concepts like grouping instances into features and defining implicit feature IDs

lilleyse · 2022-03-13T21:08:24Z

extensions/2.0/Vendor/EXT_mesh_features/README.md

->         "propertyTable": 1
+>         "propertyTable": 1,
+>         "label": "classification"
 >       },
 >       {
 >         "featureCount": 2,
 >         "attribute": 0,
->         "propertyTable": 0
+>         "propertyTable": 0,
+>         "label": "components"


The is a better example to add label to since it has two feature ID sets

lilleyse · 2022-03-13T21:17:04Z

extensions/2.0/Vendor/EXT_mesh_features/README.md

@@ -89,8 +89,7 @@ Per-vertex feature IDs can be used to identify individual objects that have been
 >         "EXT_mesh_features": {
 >           "featureIds": [{
 >             "featureCount": 2,
->             "attribute": 0,
->             "label": "buildings"
+>             "attribute": 0


I removed the text from the feature-id-by-attribute diagram above, partly for aesthetic reasons, but also because its redundant with the text above.

Before After

lilleyse · 2022-03-13T21:18:06Z

extensions/2.0/Vendor/EXT_structural_metadata/README.md

@@ -75,7 +74,7 @@ A schema may be embedded in the extension directly or referenced externally with
 >   "extensions": {
 >     "EXT_structural_metadata": {
 >       "schema": {
->         "id": "schema-001",
+>         "id": "schema_001",


The id schema-001 is invalid according to https://github.com/CesiumGS/3d-tiles/tree/draft-1.1/specification/Metadata#identifiers

Now that you mention this: The current form does not allow the "usual representation" of a https://de.wikipedia.org/wiki/Globally_Unique_Identifier - should - be allowed?

Hm good point. Nor would it support guids with a number as the first character. This could be problematic.

The reason for the limited character set is to allow IDs to be valid identifiers in GLSL, C, C++. If we support hyphens we might as well support full unicode. Engines that generate code based on the IDs - like custom shaders in CesiumJS - would need a translation step. We have something like that already for unicode batch table properties in CesiumJS: CesiumGS/cesium#8785.

Should we drop the alphanumeric requirement for IDs?

@javagl maybe we should move this discussion to a new issue so that it doesn't hold up this PR?

Moved this to CesiumGS/3d-tiles#672

lilleyse · 2022-03-13T21:23:20Z

extensions/2.0/Vendor/EXT_structural_metadata/README.md

@@ -218,12 +217,12 @@ Enum values are defined as entries in the `enum.values` array. Duplicate names o
 The classes defined in the schema are templates describing the data types and meanings of properties. An instance of such a metadata class is referred to as a _metadata entity_, and can be created from a set of values that conform to the structure of the class. This extension defines different ways of storing large amounts of property values inside a glTF asset, in compact binary forms: 

 - **Property Tables** store property values as parallel arrays in a column-based binary layout, using standard glTF buffer views. These tables can be accessed with a row index, and allow associating complex, structured metadata with arbitrary types with entities of a glTF asset on different levels of granularity.
- **Property Attributes** are a way of storing metadata as vertex attributes, using standard glTF accessors. They can be used to associate certain forms of metadata with vertices of a mesh primitive. 
+- **Property Attributes** associate vertex attributes of a mesh primitive with a particular metadata class. 


I think this is a slightly clearer definition of what property attributes do

lilleyse · 2022-03-13T21:23:28Z

extensions/2.0/Vendor/EXT_structural_metadata/README.md

-The following sections describe these storage formats in more detail.
+Each storage type refers to a metadata class, and contains a dictionary of `properties`. Each of these properties corresponds to one property of the metadata class and defines how the actual property data is stored. These property storage definitions may override the [`minimum` and `maximum` values](https://github.com/CesiumGS/3d-tiles/tree/main/specification/Metadata#minimum-and-maximum-values) and the [`offset` and `scale`](https://github.com/CesiumGS/3d-tiles/tree/main/specification/Metadata#offset-and-scale) from the property definition in the class, to account for the actual range of values that is stored for each property.

-Each storage type refers to a metadata class, and contains a dictionary of `properties`. Each of these properties corresponds to one property of the metadata class. Each of these properties define the way how the actual property data is stored. These property storage definitions allow to override the [`minimum` and `maximum` values](https://github.com/CesiumGS/3d-tiles/tree/main/specification/Metadata#minimum-and-maximum-values) and the [`offset` and `scale`](https://github.com/CesiumGS/3d-tiles/tree/main/specification/Metadata#offset-and-scale) from the property definition in the class, to account for the actual range of values that is stored for each property.
+The following sections describe these storage formats in more detail.


Swapped these two paragraphs

lilleyse · 2022-03-13T21:24:00Z

extensions/2.0/Vendor/EXT_structural_metadata/README.md

+> uint32 rawBits = byte0 | (byte1 << 8) | (byte2 << 16) | (byte3 << 24);
+> float32 value = uintBitsToFloat(rawBits);


Conceptually the same, but maybe clearer as a uint

lilleyse · 2022-03-13T21:25:54Z

extensions/2.0/Vendor/EXT_structural_metadata/README.md


-Each storage type refers to a metadata class, and contains a dictionary of `properties`. Each of these properties corresponds to one property of the metadata class. Each of these properties define the way how the actual property data is stored. These property storage definitions allow to override the [`minimum` and `maximum` values](https://github.com/CesiumGS/3d-tiles/tree/main/specification/Metadata#minimum-and-maximum-values) and the [`offset` and `scale`](https://github.com/CesiumGS/3d-tiles/tree/main/specification/Metadata#offset-and-scale) from the property definition in the class, to account for the actual range of values that is stored for each property.
+The following sections describe these storage formats in more detail.

 ### Property Tables


Removed the text from the property table diagram since it is redundant with the section above.

javagl

The changes look fine for me, just one inlined comment.

lilleyse added 2 commits March 13, 2022 14:25

Revised diagram for vertex feature ids

2144c3d

Update glTF extensions

0412a6e

lilleyse force-pushed the 3d-tiles-next-edits branch from d2c0b36 to 0412a6e Compare March 13, 2022 21:18

lilleyse commented Mar 13, 2022

View reviewed changes

lilleyse requested a review from javagl March 13, 2022 21:28

javagl reviewed Mar 15, 2022

View reviewed changes

javagl merged commit 094af35 into 3d-tiles-next Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit pass over the 3D Tiles Next extensions #62

Edit pass over the 3D Tiles Next extensions #62

lilleyse commented Mar 13, 2022 •

edited

Loading

lilleyse Mar 13, 2022

lilleyse Mar 13, 2022

lilleyse Mar 13, 2022

lilleyse Mar 13, 2022

lilleyse Mar 13, 2022

javagl Mar 15, 2022

lilleyse Mar 15, 2022

lilleyse Mar 15, 2022

javagl Mar 15, 2022

lilleyse Mar 13, 2022

lilleyse Mar 13, 2022

lilleyse Mar 13, 2022

lilleyse Mar 13, 2022

javagl left a comment

		## Optional vs. Required

		This extension is optional, meaning it should be placed in the `extensionsUsed` list, but not in the `extensionsRequired` list.

		> uint32 rawBits = byte0 \| (byte1 << 8) \| (byte2 << 16) \| (byte3 << 24);
		> float32 value = uintBitsToFloat(rawBits);

Edit pass over the 3D Tiles Next extensions #62

Edit pass over the 3D Tiles Next extensions #62

Conversation

lilleyse commented Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javagl left a comment

Choose a reason for hiding this comment

lilleyse commented Mar 13, 2022 •

edited

Loading