Try to upgrade CMPT to GLB when target version is 1.1

[javagl]

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.

[javagl]

I have added tests, in a similar style as the other migration tests: The tests perform an upgrade of the CesiumJS Specs Composite tilesets, and compare the JSON part of the resulting GLB files to the "golden" reference files.

---

There have been a few smaller hiccups here:

• Some CI glitches (some 409 (Conflict) errors during npm... operations - maybe npm had a bad day 🤷 )
• I thought that the line endings (LF vs. CRLF) were wrong, but...
• The main hiccup: glTF-Transform inserts its name and version number into the asset: { generator: ... } field. For comparing to "golden" files, the version number has to be omitted. This is already done in other places. But apparently, glTF-Transform does re-insert this during every transform operation. This is ... not ideal, and I'd even consider calling this a "bug". But this is now solved by adding the (version-less) generator name as the last operation on the Document.

---

One thing is still open: The output of the CompositeOfInstanced conversion is a valid glTF file that can be rendered in common glTF viewers. But CesiumJS refuses to render it, saying
Error: this.featureTables[this.featureTableId] is undefined
I think that this is a bug in CesiumJS, but this may have to be investigated.

[javagl]

A short update:

For the case of the overwritten generator, I opened donmccurdy/glTF-Transform#1363 (although one could argue about the desired behavior - right now, this is resolved here).

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 EXT_structural_metadata property table, but ... the resulting GLB contains only one property table after the merge (so one of the property table indices in one of the attributes turns out to be -1).

I started looking into that, and am checking whether anything "special" has to be done when calling merge on documents with root-level extensions. For things like KHR_lights_punctual, merging does work (so when merging two documents that each contain one light, then the result does contain two lights). But for some reason, one of the PropertyTable objects is lost. I'll keep digging...

[javagl]

Another short update:

Merging multiple GLBs can be tricky. And a very specific problem here is merging multiple GLBs that each contain the EXT_structural_metadata extension.

I'll consider two paths:

1. Continue looking for a workaround. There might be a hacky workaround, like brutally extracting the metadata from both inputs and manually re-inserting it into the merged document. But until now, I mainly looked for a solution (and not a workaround)
2. Don't merge. Just write out all GLB files individually, with multiple contents. This would not be "nice", but somewhat "clean" (except for the case where one CMPT combines 100 sub-tiles....), with the drawback of more requests going out. In any case, it's a path that is more likely to work, and in parts of the code that we have more control over.

[lilleyse]

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?

[javagl]

@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 merge process.

I first looked at how this is handled with KHR_lights_punctual, but even though the lights do end up in a "top-level array", that's not where they are defined (from the perspective of glTF-Transform): They are just attached to nodes, and the writer code assembles that top-level array just-in-time, before writing it.

Then I looked at KHR_xmp_json_ld. This does have definitions (of the packets) in the top-level object. But ... the implementation does not perform a proper "merge" of these definitions. I opened donmccurdy/glTF-Transform#1367, and hope that there might be hints about how this could be done for EXT_structural_metadata based on ideas how how it could be done for KHR_xmp_json_ld. If there is any form or place of hooking in some form of "special handling" for extensions during the merge operation, I'd likely be able to figure out a solution based on that.

[timoore]

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.

[javagl]

I decided not to bother with RTC_CENTER in merge()

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 EXT_structural_metadata.

It would be good to stay in sync on the general approach in both sets of tools.

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 StructuralMetadata that just has all the properties that are defined in the schema - mainly, a Schema, and arrays of PropertyTable/PropertyTexture/PropertyAttribute objects. It might be possible to change the class structure here in a tricky way, so that at least merging the PropertyTable/PropertyTexture/PropertyAttribute arrays could be handled automatically, by the built-in mechanisms of glTF-Transform (which, by the way, are implemented in a pretty 'abstract' but powerful and generic way).

But that would only move the problem to another layer: Every "automatic" approach will break, inevietably, when it comes to the Schema. And one could quickly dive into the really tricky caveats here. (E.g. when you talk about "merging feature IDs", then I think that this will imply an actual "postprocessing" to update vertex attribute values....). But I'll first have to sort my thoughts here to have more of these things on the radar...

[lilleyse]

Ignoring implementation details for a moment, this is what I imagine the merge step would look like:

• Upgrade b3dm to glTF + EXT_mesh_features + EXT_structural_metadata (property table)
• Upgrade i3dm to glTF + EXT_mesh_gpu_instancing + EXT_instance_features + EXT_structural_metadata (property table)
• Upgrade pnts to glTF + EXT_structural_metadata (property attribute)
• Create a new schema that combines all the classes (deduplicate classes if applicable)
• Merge PropertyTables into a single array and update references in EXT_mesh_features / EXT_instance_features
• Merge PropertyAttributes into a single array and update references in EXT_mesh_features / EXT_instance_features (deduplicate property attributes if applicable)

I don't think it's worth trying to merge feature IDs

@javagl is this roughly inline with what you had planned?

[javagl]

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:

• Merge PropertyTables into a single array and update references in EXT_mesh_features / EXT_instance_features
• Merge PropertyAttributes into a single array and update references in EXT_mesh_features / EXT_instance_features (deduplicate property attributes if applicable)

These chould still be relatively easy. The structures of glTF-Transform don't require "updating references" in any way. There's a PropertyTable object, and this can be tossed around, and low-level things like "its index in some array" are only determined in the serialization layer, when all this is written out as some JSON.

It still does involve a bit of effort and thought, though: A PropertyTable in one glTF-Transform Document cannot be "moved" to another one. So it will be necessary to create some sort of "deep copy" functionality for all the classes and structures that are defined as part of the metadata implementation. This will likely be 90% boring, tedious boilerplate code. It would probably also be possible to do that generically with a few lines of code and some TypeScript magic, but... that would raise the question abot how to 'intercept' this magic process at the right place, to handle these remaining 10%. I'll still have to identify more clearly what these "10%" are, but roughly: It will be everything that is related to the Schema, which leads to the next point:

• Create a new schema that combines all the classes (deduplicate classes if applicable)

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.

I don't think it's worth trying to merge feature IDs

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...

[javagl]

I have added a first draft for merging structural metadata. This is contained in a file that is pragmatically called StructuralMetadataMerger.

This is a draft for several reasons:

• It uses a function copyToDocument that has been introduced in Merging documents with extensions donmccurdy/glTF-Transform#1367 , but that is not yet part of a release. This function is currently copied here, to run the first tests and see whether the merging strategy can be implemented on top of this function. Depending on the timeline, this would be replaced by the built-in function from glTF-Transform
• There is a "small" issue related to the metadata extension implementations, now tracked in Consistently handle optional properties in metadata implementations #122 - I applied a "minimal" fix here, but I think that it could make sense to address this issue holistically in the context of this PR (a bit of tedious work, but should be worth it...)
• There are not many tests yet

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...

• copy the source enum to the target, renaming it to exampleEnum_0
• copy the source class to the target, renaming it to exampleClass_0
• copy the source property table to the target, updating its class from exampleClass to exampleClass_0

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....

[javagl]

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 ... | null when they are optional. The getDefaults() implementations define the default values that are defined in the JSON schemas.

One slightly tricky aspect here is that of writing the structures - for example, imagine an Example class with an exampleValue that is optional, but has a default value of 0.0.

const example = new Example();

// The return value here should never be `null`: When the value has not been
// set explicitly, then the default value of `0.0` should be returned
const exampleValue = example.getExampleValue(); 

// One could argue that this should be possible, because the value is optional.
// But this does not seem to be what is done in other parts of glTF-Transform...
//example.setExampleValue(null);

// One can only set the default value explicitly:
example.setExampleValue(0.0);

When this is written/serialized, then the result should be

{
}

and not

{
    exampleValue: 0.0
}

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.

[javagl]

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...)

[javagl]

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 Texture objects are equal when the pixels of the images that they refer to are equal (even when they are "different images"). This is good, in the context of glTF-Transform abstracting away the concept of a glTF image. But it might raise questions when a similar behavior should be implemented on top of other glTF libraries/infrastructures.

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 schemaUri. (In this case, the merge operation will always create an inlined schema. One could think about further configurability here...)

(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 Composite/... examples from the CesiumJS specs, and it looks like they are converted properly.

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...

[javagl]

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 3d-tiles-samples. And this did work smoothly (with minor fixes at CesiumGS/3d-tiles-samples#67 and CesiumGS/3d-tiles-validator#309 that are unrelated to this PR). And the resulting files did pass validation. But unfortunately, when trying to inspect the result in CesiumJS, I encountered some ... errors. While it can be interesting to take a Hex editor off the shelf to mess around with the JSON inside a GLB, I did sink a considerable amount of time into that, until I was sure that the problem is CesiumGS/cesium#11977

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.