-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature Metadata extension (EXT_3dtiles_feature_metadata) #1
Conversation
@lilleyse I am happy to be the "last" review on this and on all upcoming 3D Tiles / glTF updates. Is this ready for me to review? Or should others look at it first? |
@pjcozzi I'll bump you when it's ready for the "last" review, which isn't now |
|
e49a654
to
d2715f8
Compare
Not sure if it helps, but I wrote a draft of an external batch table extension for the OGC pilot: Basic principle is that external batch table(s) are always referenced from the JSON portion. The JSON describes not only where to find the external batch table (the URI) but also which properties are available there, so that clients can choose to load only the external batch tables they care about. The external batch data itself can be either JSON or binary. |
Feel free to ignore all of this if you don't think it's useful, but I often find that when I'm explaining batch tables, the name "batch table" throws people a bit. This is pure semantics and probably not a big deal, but I think it's generally better to name something after what it is rather than what it does-- although the distinction between these two things is often small. I assume the term "batch" comes from their use in batching models to optimise rendering, and I guess it is a table of batches, but I generally think of them in terms of storing feature information. I would say they should be called feature tables, but I know feature table has a different specific meaning in the context of 3d tiles. Entity tables, maybe? Some other synonym of feature? On the other hand, keeping the name "Batch Table" might make it easier for people already familiar with 3d tiles to understand the new extension... |
Updated to support the composite tile use case where heterogeneous data sources are combined into the same glTF. Required a mapping from the batch id accessor to the batch table index. See composite example. |
@KeyboardSounds agreed on all points. The new working title for this extension is |
We also need to find a home for |
… width/height into implicit object
Option 3 Here's a new approach that lets you pack properties. Note that glTF
external_1.json
external_2.json
|
Side note: I wonder if HTTP/2 multiplexing makes it less burdensome to do multiple requests in option 2. Though there's still some overhead and more complexity in the server. |
Yeah, it certainly should! I don't think that eliminates the need for this capability, but it should probably at least make us unwilling to make too many compromises to enable it. I don't think it's too bad, though.
Not a huge fan of this, because it makes featureTables different from pretty much everything else in glTF. Also I think it's not great to overload So maybe we can solve both problems by doing something like this: {
"extensions": {
"EXT_3dtiles_feature_metadata": {
"featureTables": [
{
"name": "Land Cover",
"featureCount": 256,
"featureProperties": {
"Names": {
"external": {
"uri": "external_1.json",
"key": "landCoverNames",
"type": "string"
}
},
"Colors": {
"external": {
"uri": "external_1.json",
"key": "landCoverColors",
"type": "any"
}
}
}
},
{
"name": "Trees",
"featureCount": 2492,
"featureProperties": {
"Heights": {
"accessor": 0
},
"Colors": {
"external": {
"uri": "external_1.json",
"key": "treeColors",
"type": "any"
}
}
}
},
{
"name": "Buildings",
"featureCount": 10,
"featureProperties": {
"Heights": [9, 9, 8, 3, 5, 4, 3, 9, 8, 3],
"Colors": {
"external": {
"uri": "external_2.json",
"key": "buildingColors",
"type": "any"
}
}
}
}
]
}
}
} The external files would have to be flattened, too: external_1.json {
"landCoverNames": [...],
"landCoverColors": [...],
"treeColors": [...]
} If there's no Originally I thought it might be useful to have an "externalBinary" with a URI, byte offset, and component type. But that use-case should already be covered pretty well by pointing the feature table at an accessor with an external buffer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! The design choices all seem very sensible to me.
I started to suggest some edits to the writing, but I think I was getting carried away and Sarah is probably much more qualified there anyway. But if anyone thinks it'd be useful I'm happy to do a more thorough edit for clarity before or after Sarah.
Bikeshedding a bit, but I think a table of contents at the top would add a lot.
|
||
| Property | Description | Caveats | | ||
|---------------------|-------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| `featureTable` | Index of the feature table that this feature layer is using. | Multiple feature layers in a single primitive cannot use the same feature table. Feature tables used by a single primitive cannot have any of the same property names. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple feature layers in a single primitive cannot use the same feature table.
Probably rare that this would be useful, but does it need to be forbidden?
Feature tables used by a single primitive cannot have any of the same property names.
This seems like a really big limitation. Why is it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably rare that this would be useful, but does it need to be forbidden?
I can't remember why anymore? But I could see a use case where a per-vertex ID layer and a per-texel ID layer want to use the same feature table. So maybe we should lift this restriction.
This seems like a really big limitation. Why is it necessary?
It was added for styling reasons to not overload variable names. Like ${Height}
could mean two different things if multiple feature tables have a Height property. Maybe this could be solved by prefixing variables with the feature table name like ${Buidings:Height}
, or something else. Styling is still a mostly unknown topic here. Personally I'd like to move closer to something like GLSL where conflicts like this can be worked around easier, and just gives more control in general. Styling really deserves its own issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite the "3dtiles" in the name (maybe it shouldn't even be there?), I think of this extension as being pretty general. glTF currently lacks a way to attach metadata to a parts of a model, and that's useful in a whole lot of scenarios. This extension should work well for most of them. So we shouldn't let (current) limitations in the styling language impose limitations on metadata expression in this extension. Why not just define in the styling spec what happens when there's a conflict? Last one wins, or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed both restrictions for now.
Despite the "3dtiles" in the name (maybe it shouldn't even be there?)
I've seen some confusion about the naming recently. I added it to the TODO list. I'm curious to see what @pjcozzi thinks when he reviews.
@kring I tweaked your example so that Before "featureProperties": {
"Names": {
"external": {
"uri": "external_1.json",
"key": "landCoverNames",
"type": "string"
}
}
} After "featureProperties": {
"Names": {
"array": {
"type": "string",
"external": {
"uri": "external_1.json",
"key": "landCoverNames"
}
}
}
} Full example:
external_1.json
external_2.json
|
Offline @pjcozzi brought up the idea of storing all properties in binary, including strings, to simplify the schema. One drawback is less visibility of these properties, but that can be solved by better tooling, like support for this extension in the glTF Tools VSCode extension. |
Woah woah. Lemme pause right there. No strings in binary please, all strings in JSON for glTF. Binary can do the normal accessor Among other things, the JSON schema helps validate the document in VSCode even without help from the official glTF Validator. So having the schema be more detailed, and more tightly controlling of the contents of the document, is an easy win for validation. |
Also, if you wish, you may use VSCode to essentially "debug" the schema, by testing it against documents that are known to be valid and others that are invalid. Basically you follow the instructions in the gltf-vscode repo to get a debug environment going, and then these instructions to add your schema into the set of schemas that are validated. Probably we would get a branch of gltf-vscode going where the schema had been imported, so it could be tested against documents. |
Visual Studio Code also tends to crash when you try to load large files in it, particularly large glTFs. Relying on VSCode tools to be able to read feature metadata easily would be bad in those (albeit less common) cases. As a side note, it's much, much easier to deal with JSON than binary in JavaScript code. A lot of things that support glTFs are written in JS. This is maybe not super important but I thought it was worth mentioning. |
#3 is the latest spec, #2 is the old feature hierarchy spec @lilleyse I believe most everything is covered, except for the following:
|
Possibly relevant for vector data in glTF since
Yup, let's add that requirement there |
Opened https://github.com/CesiumGS/3d-tiles-next/issues/110 so we don't forget about it, and added the requirement to the EXT_feature_metadata PR |
…ancing Rename KHR_mesh_instancing -> EXT_mesh_gpu_instancing
See #3 for the more up-to-date schema
For CesiumGS/3d-tiles#269
For CesiumGS/3d-tiles#385
EXT_3dtiles_feature_metadata
EXT_feature_metadata
b3dm, pnts, i3dm, and cmpt are moving to glTF! This gives us the opportunity to rethink how feature metadata works and lean on glTF wherever possible.
Here's the gist of it:
EXT_3dtiles_feature_metadata
:feature-metadata
branchTo do:
number
?EXT_mesh_gpu_instancing
andEXT_feature_metadata
interop be simplified?RGB565
compressed colors. Draco and Meshopt offer better solutions so we should consider scrappingRGB565
.componentType
and padding for glTF chunks is 4 bytes which makes it more difficult to align double properties to 8-byte boundaries for usage with JavaScript'sFloat64Array
. If this would be a breaking change to the glTF spec what can we do to get around it? Enforce that double properties are aligned to an 8-byte boundary relative to the start of the glb / glTF buffer? (OBE: now using Apache Arrow)semantic
field like "NAME" or "ID", and the client knows that it can get the name from that property.NEAREST
sampling makes sense most of the time but maybe it's best left up to the discretion of the tileset author to chose their own samplers.normalized
is applicable to signed and unsigned typesEXT_3dtiles_feature_metadata
the right name? Should the3dtiles
part be dropped?Feature Metadata extension (EXT_3dtiles_feature_metadata) #1 (comment)CONSTANT_RGBA
. (Topic moved to 3DTILES_content_gltf)