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

Added option to assign metadata to nodes #63

Merged
merged 3 commits into from
Apr 5, 2022

Conversation

javagl
Copy link

@javagl javagl commented Apr 4, 2022

When metadata is stored in a property table, it is possible to associate glTF nodes with metadata instances/entities, by adding information about the property table and the feature (row) index inside the node:

"EXT_structural_metadata": {
  "propertyTable": 1,
  "index": 4
}

This is described in a short paragraph at https://github.com/javagl/glTF/tree/3d-tiles-next-node-metadata/extensions/2.0/Vendor/EXT_structural_metadata#node-metadata and the associated schema file.


Note: This is a draft PR for now, because there are two technical questions that might have to be discussed in more detail:

1. Should it be possible to associate multiple metadata entities with a node?

As a first shot, it could just be an array that contains the propertyTable/index objects that describe the actual entities:

"EXT_structural_metadata": {
  "entities": [   
    {
      "propertyTable": 1,
      "index": 4
    },
    {
      "propertyTable": 2,
      "index": 5
    }
}

But this is not yet thought through, and if we decide to support that conceptually, then details of the structure can still be sorted out.

2. Should it be possible to assign metadata to other glTF elements (beyond nodes?)

The same structure that is used for associating metadata entities with nodes could be used for each glTFChildOfRootProperty. These are the elements of the top-level glTF object arrays, like animations, nodes, textures etc. One could argue that it's hard to find a use-case for metadata in a bufferView, but even if it is not going to be allowed generically, one could think about how this concept could later be extended to other objects in a sensible way (i.e without creating <type>.EXT_structural_metadata.schema.json files for each of them, and in a preferably backward-compatible way...)

@lilleyse
Copy link

lilleyse commented Apr 4, 2022

1. Should it be possible to associate multiple metadata entities with a node?

As a first shot, it could just be an array that contains the propertyTable/index objects that describe the actual entities:

This could potentially be solved by hierarchical metadata: CesiumGS/3d-tiles#558. Maybe that's a good enough resource to stick with the simpler approach for now.

2. Should it be possible to assign metadata to other glTF elements (beyond nodes?)

This would make it similar to KHR_xmp_json_ld, which could be a good thing, but could also lead to the question "why not XMP?". Personally I think there's room for multiple metadata encodings as long as they're sufficiently different. (In this case the binary type system would be a differentiator). So hesitantly, yeah, it should be possible to assign metadata to other glTF elements.

@javagl
Copy link
Author

javagl commented Apr 4, 2022

So hesitantly, yeah, it should be possible to assign metadata to other glTF elements.

I'm also hesitating a bit. At this point, one could argue that it could/should also be possible to assign it to all glTFProperty objects (and not only glTFChildOfRootProperty). But I changed it to the latter right now.

@javagl javagl marked this pull request as ready for review April 4, 2022 20:48

*Defined in [glTFChildOfRootProperty.EXT_structural_metadata.schema.json](./schema/glTFChildOfRootProperty.EXT_structural_metadata.schema.json).*

When property values are stored in a [Property Table](#property-tables), then the entries of this table may be associated with elements of the glTF asset. This applies to all elements that are a `glTFChildOfRootProperty`. Each of these elements can contain an `EXT_structural_metadata` object that defines the source of the metadata values for this element. It contains the `propertyTable`, which is the index of the property table in the array of property tables of the root-level `EXT_structural_metadata` extension object, and the `index`, which is the index of the row in this table that contains the metadata values for the respective element.
Copy link

Choose a reason for hiding this comment

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

This applies to all elements that are a glTFChildOfRootProperty.

glTFChildOfRootProperty feels a bit too jargony unless someone is familiar with the glTF schema. Should the types be listed out explicitly like in KHR_xmp_json_ld? Note that some types like accessor and bufferView are omitted. As a side note, which approach is easier for code generators?

If we list them explicitly maybe something like this:

Property table values can be referenced from glTF objects of type: (list out types explicitly). Each of these elements can contain an EXT_structural_metadata object that defines the source of the metadata values for this element. It contains the propertyTable, which is the index of the property table in the array of property tables of the root-level EXT_structural_metadata extension object, and the index, which is the index of the row in this table that contains the metadata values for the respective element.

@@ -0,0 +1,26 @@
{
Copy link

Choose a reason for hiding this comment

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

This document mentions "node" a few times

@@ -39,6 +39,7 @@ Written against the glTF 2.0 specification.
- [Property Attributes](#property-attributes)
- [Property Textures](#property-textures)
- [Binary Data Storage](#binary-data-storage)
- [Assigning Metadata](#assigning-metadata)
Copy link

Choose a reason for hiding this comment

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

It feels more appropriate for the Assigning Metadata section to go before Binary Data Storage

@javagl
Copy link
Author

javagl commented Apr 5, 2022

it could/should also be possible to assign it to all glTFProperty objects (and not only glTFChildOfRootProperty).

glTFChildOfRootProperty feels a bit too jargony unless someone is familiar with the glTF schema. [...] Note that some types like accessor and bufferView are omitted.

As a side note, which approach is easier for code generators?

These points are somewhat interdependent. One thing that I wondered about: The schema file is currently called glTFChildOfRootProperty.EXT_structural_metadata.schema.json. And I think that this naming pattern of

<whatItIsAttachedTo>.<extensionName>.schema.json

is somehow related to some detail of the cesium-native code generator (although I'd have to verify this). In contrast to that: KHR_xmp_json_ld calls that object just KHR_xmp_json_ld.schema.json, without any prefix.

The point is: We could now restrict it to nodes, and call it node.EXT.... But if we intended to generalize it, we'd have to rename it to glTFChildOfRootProperty.EXT_.... And later maybe even to glTFProperty.EXT_.... And these are changes that would likely be incompatible (or at least problematic for code generators).

I think that it could make sense to solve this in a similar way as KHR_xmp_json_ld: The reference is just called EXT_structural_metadata.schema.json. And technically (on the pure JSON level), it could be assigned to any glTF element. But in the specification text, we define that it can only be assigned to node (for now). That could be a sensible middle-ground.

@javagl
Copy link
Author

javagl commented Apr 5, 2022

I tried this ^ in https://github.com/javagl/glTF/tree/3d-tiles-next-node-metadata/extensions/2.0/Vendor/EXT_structural_metadata#assigning-metadata

An aside: You mentioned

It feels more appropriate for the Assigning Metadata section to go before Binary Data Storage

but the 'Binary Data Storage' is a short paragraph that is directly related to the preceding 'Metadata Storage' section - describing the 'Metadata Storage' itself, then switching to something unrelated, and then coming back to an "encoding detail" for the metadata storage looks a bit odd for me. I just made 'Binary Data Storage' a subsection of 'Metadata Storage' - could that make sense for you as well?

@lilleyse
Copy link

lilleyse commented Apr 5, 2022

@javagl all that sounds reasonable. 👍

@lilleyse lilleyse merged commit e3d043e into CesiumGS:3d-tiles-next Apr 5, 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.

2 participants