-
Notifications
You must be signed in to change notification settings - Fork 48
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
Try to upgrade CMPT to GLB when target version is 1.1 #117
Conversation
I have added tests, in a similar style as the other migration tests: The tests perform an upgrade of the CesiumJS Specs There have been a few smaller hiccups here:
One thing is still open: The output of the |
A short update: For the case of the overwritten For the case of the invalid feature tables: That's more tricky. First of all: It's an issue in the extensions implementation or the merge process: The CMPT refers to two B3DMs, both define a batch table, this batch table is converted into a I started looking into that, and am checking whether anything "special" has to be done when calling |
Another short update: Merging multiple GLBs can be tricky. And a very specific problem here is merging multiple GLBs that each contain the I'll consider two paths:
|
I think the first path is preferable since multiple contents isn't widely supported. Have you been able to determine whether there's a bug in glTF-Transform or 3d-tiles-tools usage of it? |
@lilleyse That's hard to say for sure. I usually try to assume that the error is on "my side", but after I spent a certain amount of time with debug-stepping through the code, I think that this might be a limitation related to ~"extensions that define top-level elements". The question is: How are these top-level elements merged? And right now, it looks like there is no place in the API to "hook" into a proper merging of these top-level elements during the I first looked at how this is handled with Then I looked at |
I'm going down a similar path in CesiumGS/cesium-native#854 . Supporting i3dm implies that cmpt and therefore CesiumGltf::Model::merge() needs to support merging of the relevant extensions. Merging ExtMeshGpuInstancing is easy. I decided not to bother with RTC_CENTER in merge() and apply it to the glTF representation in the i3dm converter; I think that's the way to go for the other 1.0 format converters as well. I'm contemplating metadata in the next tranche of work and it seems to me that merging the feature tables and feature IDs is really the only way to go. It would be good to stay in sync on the general approach in both sets of tools. |
Yes, I think that should be the way to go: In the conversion from "old" tile formats to glTF in the tools, the approach here is usually to 1. ignore the RTC center for most part of the conversion (with subtle but important caveats that I'll omit here for now), and 2. eventually insert a new root node into the glTF, which has the RTC center as its translation, and attach all former root nodes as children to this node. The actual merging should then be independent of that, and should not require any special handling: The input consists of a bunch of glTFs that have been created from the inner tiles, and their root node translation does not matter any more. (Unless I'm overlooking some corner case...) Beyond that, there hasn't been much work for "merging" in the tools until now. It only came up in the context of CMPT, and there, the intention was to just delegate that to glTF-Transform. This will hit a wall in the case of
I agree. There are some really tricky aspects to that. If I had to give a high-level summary of the main problem here right now: Usually, "merging" means 'copying' everything from a source to a target. Every node that is contained in the source is added to the list of nodes in the target. And the same for the list of textures, meshes, images, accessors, and structural metada.... ❗ ... wait! The metadata is like "The Highlander": There can be only one. So this has to be a single object that was created by merging two objects. In the glTF-Transform based implementation of that extension, there is a pretty straightforward But that would only move the problem to another layer: Every "automatic" approach will break, inevietably, when it comes to the |
Ignoring implementation details for a moment, this is what I imagine the merge step would look like:
I don't think it's worth trying to merge feature IDs @javagl is this roughly inline with what you had planned? |
Just as a short 'ack': The first bullet points are already implemented as part of the "legacy format migration". This PR was supposed to ""only"" apply these existing steps on all inner tiles of CMPT, and merge the results. But "merging" is where the other points come into play:
These chould still be relatively easy. The structures of glTF-Transform don't require "updating references" in any way. There's a It still does involve a bit of effort and thought, though: A
This can be tricky. Deduplication and disambiguation based on structures and names or "deep structural equality", and ... there's that "schema ID". I'll try to create a draft and identify the critical points more clearly ASAP.
Not sure yet whether it might be necessary, but more generally: I hope that we won't have to touch any form of "binary data" for this merge operation... |
I have added a first draft for merging structural metadata. This is contained in a file that is pragmatically called This is a draft for several reasons:
For the tests: I already did implement some functionality that will go into the specs, and did run some local tests. The general idea for now is to create some documents that contain a schema and some property tables (or textures or attributes) and check the merged results. I created a function async function createExampleDocument(
schemaJson: any,
propertyTableJson: any
): Promise<Document> { ... } that can create "dummy" documents with the extension, from input data like this: const exampleSchemaX = {
id: "EXAMPLE_SCHEMA_ID",
enums: {
exampleEnum: {
valueType: "UINT8",
values: [
{
name: "EXAMPLE_ENUM_VALUE_A",
value: 12,
},
{
name: "EXAMPLE_ENUM_VALUE_B",
value: 34,
},
],
},
},
classes: {
exampleClass: {
name: "Example Class",
properties: {
example_ENUM: {
name: "Example ENUM property",
type: "ENUM",
enumType: "exampleEnum",
},
},
},
},
};
const exampleSchemaY = {
id: "EXAMPLE_SCHEMA_ID",
enums: {
exampleEnum: {
valueType: "UINT8",
values: [
{
name: "EXAMPLE_ENUM_VALUE_A",
value: 0,
},
{
name: "EXAMPLE_ENUM_VALUE_B",
value: 1,
},
],
},
},
classes: {
exampleClass: {
name: "Example Class",
properties: {
example_ENUM: {
name: "Example ENUM property",
type: "ENUM",
enumType: "exampleEnum",
},
},
},
},
};
const propertyTableX = {
class: "exampleClass",
properties: {
example_ENUM: [12, 34, 12, 34],
},
};
const propertyTableY = {
class: "exampleClass",
properties: {
example_ENUM: [0, 1, 0, 1],
},
}; These schemas/property tables should be merged. One caveat that is tested in this example: The schemas are structurally equal, except for the enum values. So the merge process should...
In contrast to that, when the enum values in both schemas are equal, then no renaming should take place, and it should only copy the source property table to the target. This works for now, but further configurations (property textures, attributes, other differences between the schemas, and further corner cases) still have to be sorted out and covered more systematically. There will be some degrees of freedom and judgement calls, but maybe these can be agreed upon as we move on.... |
Fixes #122 The last few commits tried to straighten out the handling of optional/required/default values in the metadata implementation. The interfaces that define the signatures of the "model" classes now define the types to be One slightly tricky aspect here is that of writing the structures - for example, imagine an
When this is written/serialized, then the result should be
and not
to avoid having the resulting JSON filled with unnecessary default value declarations. So the check whether a value has its default value has to be done in the writer. |
I have added a basic merge for property attributes and textures. (This turned out to be a bit trickier than expected - properly updating the references to the merged objects requires some knowledge of the glTF-Transform merge process, and special handling for updating these references). During the tests, I noticed a case that one could argue about. When there are two documents, and they contain the same class, and each contains a property attribute, then the result of merging them will currently be a document with one class and two property attributes: "propertyAttributes": [
{
"class": "exampleClass",
"properties": {
"example_INT8_SCALAR": {
"attribute": "_EXAMPLE_ATTRIBUTE"
}
}
},
{
"class": "exampleClass",
"properties": {
"example_INT8_SCALAR": {
"attribute": "_EXAMPLE_ATTRIBUTE"
}
}
}
] But one could make a case that there should only be one property attribute, considering that they are equal. (I'll locally try to create a draft of how this could be handled, but there are some details to be sorted out...) |
The last commits aim at wrapping this up. Duplicate property attributes and property textures are now de-duplicated. The question about "equality" still leaves some question marks here. For example, glTF-Transform is smart enough to determine that two Specs have been added to cover these cases, trying to cover the space of "(equal/different) classes and (equal/different) property (tables/attributes/textures)". Some specs for "tricky" cases have been sprinkled in, for example, for the handling of disambiguated enums mentioned in a previous comment, and the case that both merged metadatas refer to schemas with a (One minor quirk: When an existing schema is modified by 'merging in' classes from another schema, then the ID of the target schema has to be updated (because it changed). I originally did this with a UUID suffix. Now... for the comparison to the "golden" files in the specs, this one has to be a fixed string. I think it's OK to have a function for this that is only called from the specs...) The Sandcastle that can be used for easily comparing the migration input/output has been extended, to cover the It might be worthwhile to try and come up with more "nasty corner cases" that could be part of the specs. Right now, the specs are more in the category of "ensure what it works". I think that 'testing' generally should mean "try to break it 😈 ". But for some cases (like merged schemas, or the questions about 'equality' of things), it may not yet be clear enough what the result should actually be in some of these cases... |
I did a few cleanups, and what I thought could be a reasonable "stress test": I tried to merge (nearly) all of the glTF metadata samples from This might not have to hold up this PR, with the caveat that the functionality that is implemented here might create data that will cause CesiumJS to crash. So I'll mark it as "ready" for now, and wait for further feedback. |
When using the
upgrade
command with--targetVersion 1.1
, the tools currently try to upgrade PNTS, B3DM, and I3DM files to GLB.The goal of also upgrading CMPT (composite) tiles to GLB looks like a low-hanging fruit at the first glance. But there are caveats, and some of them are laid out at the bottom of this comment.
However, one can be "pragmatic", and just apply the obvious best-effort approach: Just call the existing upgrade functions, mush together the results into a single GLB, and call it a day. In most cases, it should work and make sense. The corner cases (non-combinable GLBs, implicit tiling with CMPT...) should be rare. Which of them should be detected or handled in which way is up for debate, but I think that in the current form, the extended functionality could already be useful, and ... that's something.
I still have to add tests here. I tried it out with the
CompositeOfComposite
tileset from the CesiumJS specs, and it seemed to work, and this will likely be the test case that will be added here as well.