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

Clarification for only using feature IDs #40

Merged

Conversation

javagl
Copy link

@javagl javagl commented Oct 27, 2021

Based on a PR comment, added an explanation and example of using feature IDs without property tables.

(See KhronosGroup#2082 (comment) )

Based on a PR comment, added an explanation and example
of using feature IDs without property tables.
@@ -247,7 +247,22 @@ Each feature ID definition may include only a single source, so the following ar
- `featureId.offset` and `featureId.repeat` (for a Feature ID Attribute)
- `featureId.index` (for a Feature ID Texture)

Every `propertyTables` index must have an associated `featureIds` definition, but feature IDs may be defined without a property table. The `propertyTables` entry at index `i` corresponds to the `featureIds` entry at the same index. As a result, the length of the `featureIds` array must be greater than or equal to the length of the `propertyTables` array. Each (`featureId`, `propertyTable`) pair must be unique, but individual feature IDs and property tables may be repeated within a primitive or node.
Feature IDs may be assigned to primitives or nodes, but do not have to be associated with a property table. This is useful when the feature IDs are supposed to be resolved externally by an application.
Copy link

Choose a reason for hiding this comment

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

I think this could be worded a little clearly, and I think we can add another use case (using feature IDs for styling.)

Here's one possible wording:

Suggested change
Feature IDs may be assigned to primitives or nodes, but do not have to be associated with a property table. This is useful when the feature IDs are supposed to be resolved externally by an application.
Feature IDs may be assigned to primitives or nodes without being associated with a property table. This is useful when an application retrieves properties from an external resource (e.g. a database or REST API). This can also be used for identifying features in a mesh for styling in an application, even if no properties are stored.

Copy link

Choose a reason for hiding this comment

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

oh another thing, sets of feature IDs may be stored in primitives or nodes in the schema, but conceptually feature IDs themselves are only ever assigned to vertices, GPU instances or texels. it would be good to read through this section and make sure we're consistent with our wording.

Copy link

Choose a reason for hiding this comment

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

hm... in thinking about this some more, "sets" of feature IDs could get a little ambiguous, as all the FEATURE_ID_n semantics together could also be referred to as a "set of feature IDs"...

When in doubt, it might be better to be explicit and say "featureIds entries" or something like that?

@lilleyse
Copy link

I think since this example is mainly about showing how to use feature ids without a property table the implicit feature ids might be a bit of a distraction and we should show a more common example using accessors (like "attribute": 0)

@javagl
Copy link
Author

javagl commented Oct 27, 2021

I'll have to re-read the respective sections to figure out how the feedback might relate to an "established vocabulary", and then think about a sensible solution. If anybody has a specific suggestion about how things should be, I'd integrate that, but for now, I assume that this has low priority, or might be sorted out via some internal discussion.

@wallabyway
Copy link

@ptrgags - I like this change.
Connecting to an external database, should be a 'section' in this spec. It's such an obvious use-case, and one that we will use.

Should I create a PR, with a section, an example and a diagram?

@javagl
Copy link
Author

javagl commented Nov 13, 2021

Coming back to this after a while, @ptrgags

oh another thing, sets of feature IDs may be stored in primitives or nodes in the schema, but conceptually feature IDs themselves are only ever assigned to vertices, GPU instances or texels.
...
hm... in thinking about this some more, "sets" of feature IDs could get a little ambiguous

If I understood that correctly, then the main point was to check whether we say "feature IDs are stored in a mesh primitive" at some point, and change that to something like "feature IDs for the vertices are stored in the mesh primitive".

(It may not even be necessary to talk about "sets" here).

But one could still argue whether they are really stored there, or only defined (or so). This also refers to the suggested change:

This can also be used for identifying features in a mesh for styling in an application, even if no properties are stored.

But "storing properties" may be a suitable short-hand form for (precise but clumsy) spec-parlance like ~"having non-zero-length propertyTables or propertyTextures arrays" or so.

An aside: If I had wrapped my head about the details of all documents, and all discussions that happened in issue threads, I'd probably have a clear mental model for the vocabularly, when it comes to "defining properties", "storing properties", "storing property values", "storing property definitions" and so on. I think there are differences, and I think they matter, but maybe they are less important than I thought.


@wallabyway

Connecting to an external database, should be a 'section' in this spec. It's such an obvious use-case, and one that we will use.

Should I create a PR, with a section, an example and a diagram?

Depending on whether this should be extended to a whole section, I can either extend this PR, or you can make a suggestion. For the diagram (or rather, an image) : We considered to have a somewhat more "generic" image in the overview that shows this case. Right now, the overview shows a table, but this could be replaced by some black box...

Cesium ID Lookup

But there may be ways of representing that in a more suitable form when it refers to a database in particular.

@wallabyway
Copy link

@javagl - Nice. That works.

How about a DB icon and something that looks like a DB table ?
(I also had to mention 'floor', because, digital twins 😉)

141661003-8faf4455-ff06-4ed5-9c10-0514a264dbf7

@javagl
Copy link
Author

javagl commented Nov 15, 2021

WIth that table, the diagram is again more similar to the original one (before I draw that 'black box' over the table). But I can create a "nice(r)" version of that diagram that can be used in that section, regardless of how far the text is extended or worded exactly.

@javagl
Copy link
Author

javagl commented Nov 16, 2021

The section "Specifying Feature IDs" now has two subsections, "Resolving Feature IDs with Property Tables" and "Resolving Feature IDs Externally".

Direct preview: https://github.com/javagl/glTF/tree/ext-mesh-features-only-feature-ids/extensions/2.0/Vendor/EXT_mesh_features#specifying-feature-ids

I think since this example is mainly about showing how to use feature ids without a property table the implicit feature ids might be a bit of a distraction and we should show a more common example using accessors (like "attribute": 0)

On the one hand, I agree that the details for the implicit ID in the "Example" may appear to be distracting. On the other hand, just having an example

"EXT_mesh_features": { "featureIds": [ { "attribute": 3 } ] }

does not convey much information, and might require a few words as well, and depending on the level of detail, may also be distracting. One could just say: "This defines the attribute that strores the IDs", or provide some context and say "The attribute is used for FEATURE_ID_<attribute> that points to an accessor that...".

Copy link

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

I think this section is a great addition, thanks! A few suggested wording changes attached.

extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
javagl and others added 5 commits November 23, 2021 02:50
Co-authored-by: Don McCurdy <dm@donmccurdy.com>
Co-authored-by: Don McCurdy <dm@donmccurdy.com>
Co-authored-by: Don McCurdy <dm@donmccurdy.com>
Co-authored-by: Don McCurdy <dm@donmccurdy.com>
Copy link

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Two minor comments left, then this looks good to me! The external resources section is not normative presumably, so we can always expand it later if needed.

extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
extensions/2.0/Vendor/EXT_mesh_features/README.md Outdated Show resolved Hide resolved
javagl and others added 2 commits November 23, 2021 14:21
Co-authored-by: Don McCurdy <dm@donmccurdy.com>
@ptrgags
Copy link

ptrgags commented Nov 30, 2021

This looks good to me, thanks @javagl!

@ptrgags ptrgags merged commit b66d787 into CesiumGS:3d-tiles-next Nov 30, 2021
nithinp7 pushed a commit that referenced this pull request Feb 15, 2022
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.

5 participants