Added tool to upgrade legacy 3DGS Tilesets to current spec

[weegeekps]

This adds a function to upgrade the legacy 3DGS Tilesets to the latest specification for the 3DGS + SPZ extension. I've added some unit tests, but I have some open questions that I've included in code comments within this PR.

I used the existing upgrade functionality as a guide for how to do this. I decided against using glTF Transform despite my impression that it's preferable these days, as using glTF Pipeline seemed more straightforward and wouldn't require me to finish my roughshod implementation of the KHR_gaussian_splatting and KHR_gaussian_splatting_compression_spz_2 extensions for glTF Transform.

cc: @ggetz & @jjspace since you asked me to complete this.

[weegeekps]

I use WebStorm for TypeScript & JavaScript so I added this ignore so my project files don't accidentally get checked in.

[weegeekps]

I wasn't sure where to create these. We don't appear to have anything existing today, and it helps with making sure we're keeping the correct data shape. Would this go into a new file or folder within src/structure? Open to thoughts here.

[javagl]

I think that things should generally be declared 'as local as possible' (and 'as global as necessary'). Right now, they are only used in the functions of this class.

As you may have seen, the glTF JSON is usually typed as any here. The reason is ... that (there aren't sooo many places where the raw JSON is used after all, and) this is what it is: JSON.parse can return any-thing, and that as GltfJson does not have any notable effect. Given that all properties are optional? (even the ones that are required according to the glTF spec, e.g. mesh.primitives), there isn't really any added type safety here. (Maybe convenience, like VSCode code completion...?).

In that manner, these types are similar to the types in src/structure. And it could be worthwhile to create such types for glTF. This wouldn't have to happen manually - the PR at CesiumGS/wetzel#87 might already be able to work on the glTF schema. (Maybe I'll try it out. It has been a while since I created that PR, but cannot imagine any major hurdles here - maybe just some minor manual tweaks on the output...).

That's only a "drive by comment" (it's late here) - I'll do a proper review soon.

Before that... maybe a rough estimate: On a scale between 0.0 (just merge it already) and 1.0 (strict!), where should that review be?

_{(You know... I think that this PR introduces the first ... "things" (types, functions, properties) that are not /** documented */... )}

[weegeekps]

On that scale, I'd say maybe a 0.5?

Oops on the documentation. In my haste to wrap it up last week, I forgot to go through and document everything, I will do that now.

[weegeekps]

Also, on the case of glTF structure in the structure directory, I did think about adding these there but opted against it until you had a look. Auto-generation has its benefits, but we should ensure that we add an indexer to each interface if we do that:

interface GltfJson {
    extensionsUsed?: string[];
    extensionsRequired?: string[];
    meshes?: string[];
    [x: string | number | symbol]: unknown;
}

That will allow you to use index access [] to access any fields not explicitly defined. This approach also uses an interface instead of a type.

[javagl]

One connection between these points is that wetzel would automatically insert the comments from the schema into the generated TypeScript structures.

[javagl]

Rather out of curiosity, I created a draft PR with the wetzel output at #195

I might need more details about the purpose of the "indexer" (assuming that it is the [...] : unknown part).

This seems to work as expected...

  const x = {
    componentType: 32
  } as Accessor;
  const c0 = x.componentType; 
  const c1 = x["componentType"];
  console.log(c0);
  console.log(c1);`

...and it's not clear for me what the indexer does beyond that.

[weegeekps]

In retrospect it's probably not useful for our cases, but I created a small repl to demonstrate the concept.

When working with JSON it can be beneficial to allow an escape hatch to access all of the potential data on the object, since the use of interfaces actively prevents that. The argument is that since JSON sources are typically not verified, then you should be able to access all the data, even that which is outside of the interface.

Realistically though, when I give deeper thought about it for glTF there's probably only a few specific areas where we would want this. Both extensions and extras come to mind.

[javagl]

The extensions and extras themself are already in the GltfProperty type, and inherited by all types. (In fact, the "manual cleanup" that I mentioned was to remove exactly these from the actual types: In the glTF schema, they are redeclared in every type. JSON schema is no real "inheritance" after all...).

This indexer might be justified, given that additionalProperties is using the default value of true in glTF. But I think that this is rarely used, and offering that indexer to access anything feels a bit like cheating the way around the typing. In standard glTF processing, there should never be a reason to access any non-standard properties. The code that is doing that must know that this property is there, in the JSON file that was just read. Whatever the use case for that could be: I'd rather "force" implementors to make that explicit with something like const p = (value as any).thatProperty; or so.

[weegeekps]

I opted for a "snapshot" style test here since Jasmine is very good about telling you exactly where some JSON isn't matching correctly.

[ggetz]

Thanks @weegeekps! I'm not too familiar with 3d-tiles-tools, so I'll defer to @javagl or @lilleyse for review.

[javagl]

My review on this is still pending, I'll try to do it during the weekend. (I'll consider to combine the review with the creation of an on-top PR that adds method comments. I'm a strickler, but hesitate to force others to do things like that)

[weegeekps]

method comments

Let me resolve this. It's easy to do.

[weegeekps]

@javagl I added additional class and method comments. Let me know if I missed any more.

That said, we probably should wait until after the discussion on Tuesday for us to do a final review and merge this.

[javagl]

On a scale between 0.0 (just merge it already) and 1.0 (strict!), where should that review be?

On that scale, I'd say maybe a 0.5?

I might have gone into the ">0.75 range" with some of the inlined comments.

Higher-level:

Most of the other ...Upgrader specs use the "golden" approach, actually upgrading a tileset that is stored as a file in the Specs/data directory and comparing the result to the expected ("golden") one. But given that the upgrades here only twiddle with the JSON (both tileset and glTF), this may not be necessary.

It would still be nice to have a "full, real-world" test. Some tileset with something like the "unit cube" that was actually generated by the tiler (based on a PLY file), and used verbatim here, verified to be 1. rendered properly by CesiumJS in its old version, and 2. rendered exactly the same by CesiumJS in its new version.

But ... it's difficult to add something like this now. It would have been neceessary to diligently track the outputs of actual unit (cube) tests from the tilers (with their exact versions and changes due to bugfixes), and align that with the CesiumJS changes and bugfixes.

I'll probably try to find/download some data from the tilers that was generated with the old extension (I might have some in my ion account), and apply the upgrade to this data to see how it goes. If anything doesn't work there, it will be impossible to say for sure whether it's a tilers issue, an upgrade issue, or a CesiumJS issue. But it could be worthwhile to at least have one "snapshot result", to validate this with some real data.

(Or... did you already try this out with real, old ion data?)

[javagl]

The combine command calls combine.
The merge command calls merge.
The upgrade command calls upgrade.
The upgradeLegacySplats command calls upgradeSplats.

Not "important", but ... maybe the command should be called upgradeSplats...?

More important: Documentation for this new command will have to be added into the main README.md, e.g. after the upgrade documentation, at

3d-tiles-tools/README.md

Line 132 in 24bf3b8

(This may happen at any point in time, and it may make sense to do it "last", after the review. If you had already done it, there would be two places where the command may have to be renamed)

[javagl]

There was some inlined discussion about these types.

Right now, they are only defined locally. As long as they don't surface to the API level (even private!), their names do not matter. We could consider just copy-and-pasting the relevant auto-generated types, e.g. the Mesh from https://github.com/CesiumGS/3d-tiles-tools/pull/195/files#diff-bc10fcfab289cf65e52b3d12c54162f35e9717543cd6674f4977489f5eb67735R9 . This has some degrees of freedom. For example, whether we should also copy the GlTFChildOfRootProperty (locally), or omit that part. In any case, it could make the transition easier if we later decide to make the auto-generated classes part of the 3d-tiles-tools. (No renaming and twiddling - just replacing these local classes with the corresponding import).

It doesn't have to hold up this PR, but ... something to consider.

[javagl]

I'll create a PR on top of this one, with comments for these.

I know: Some of the comments that I have inserted elsewhere (and the ones that I'll insert here) are "shallow". I won't deny that I'm sometimes ""documenting"" some function like doSomeComplexOperation with /** Does some complex operation */. These comments are "open-ended", in some way (listing preconditions and postconditions, and providing context). But ... I'll do it.

(In my local (Java) development, I've configured Eclipse to emit a warning when anything is undocumented, and still maintain these "0 warnings". If someone calls me pedantic and/or claims that this is useless, we can arrange a call with a screen-sharing session, to go through some code (e.g. CesiumJS))

[javagl]

For a tileset with extensionsUsed: [ "KHR_spz_gaussian_splats_compression", "EXAMPLE_extension" ], this will clear the array, and create a wrong result.

The deleteCount of splice has to be 1 here.

But... one could argue back and forth about a more "elaborate style"...

if (!gltf.extenisionsUsed) { complain(); return; }
if (!gltf.extensionsUsed.includes("...spz")) { complain(); return; }
gltf.extensionsUsed.splice(..., 1);
...

And (in view of the comment about comments), the doc of that function could say

/** 
 * ..
 * If the extension is not there, then it complains somehow, and nothing is done 
 */

The point is to make sure that the function won't crash or break anything, and to also say that. Callers should not have to fully read (aka reverse engineer) the function, and they should not have to be "afraid" of calling it. The function should be "defensive" and "safe".

[javagl]

There are about 31415926535 different ways of how this could be written, with room for endless discussion about what is "better" or "worse" or "more readable" or "less readable".

One example of how this could be written is

  private static updateGaussianSplattingPrimitiveAttributes(
    primitive: GltfPrimitive
  ) {
    if (!primitive.attributes) {
      return;
    }
    const attributes = primitive.attributes;
    for (const name of Object.keys(attributes)) {
      if (
        name === "_ROTATION" ||
        name === "_SCALE" ||
        name.startsWith("_SH_DEGREE_")
      ) {
        const nameRoot = name.substring(1);
        const newName = `KHR_gaussian_splatting:${nameRoot}`;
        attributes[newName] = attributes[name];
        delete attributes[name];
      }
    }
  }

This avoids the originalAttributesEntries, the performedUpdate variable(!), and disabling the eslint rule, and the comment about why the rule was disabled.

(I'm all for comments, even inlined ones, but ... this one could be seen as "unnecessary cognitive load" when there are so many (clear and clean) options where this is not necessary...)

[javagl]

Here we are in what might be a totally subjective territory:

There are currently 1409 appearances of import {. I always used one import per class.

Are there any strong, technical pros and cons for one approach or the other?

[javagl]

There is a CesiumJS-style defined in
import { defined } from "../../base";

In many cases, I've opted for if (!tileset.extensions) for checks like this.

(One could argue back and forth. Yes, if (!tileset.extensions) would not trigger when tileset.extensions = "Fooled ya!". But this would already violate the type definition of the extensions that was defined in RootProperty. I think when the type already dictates that it cannot be one of the "" or 0 cases that are so problematic for if-checks in JavaScript, the if-check should be OK...)

[javagl]

Similar to the other comment about splice, this will break when there was a
extensionsUsed: ["KHR_spz_gaussian_splats_compression", "EXAMPLE_extension"]
and result in the EXAMPLE_extension being omitted in the output.

This update is, once more, something that can be done in many different ways, with some subjective elements. One could consider using array.filter(...) to remove the old one, but that doesn't change the array in-place. One could use splice, but that requires checking the index (i.e. whether the element was actually contained). It's always more complicated than remove. One way could be this...

      const extensions = tileset.extensions;
      const contentGltfExtension = extensions["3DTILES_content_gltf"];
      if (!contentGltfExtension) {
        // Aaaaaahhh!!!
        return;
      }

      // Comment, maybe make this a `private static` function
      const replaceThem = (extensionsList: string[]) => {
        if (!extensionsList) {
          return;
        }
        const index = extensionsList.indexOf(OLD_SPLAT_EXTENSION);
        if (index >= 0) {
          extensionsList.splice(index, 1);
        }
        extensionsList.push(NEW_SPLAT_EXTENSION);
        extensionsList.push(NEW_SPZ_EXTENSION);
      };
      replaceThem(contentGltfExtension.extensionsUsed);
      replaceThem(contentGltfExtension.extensionsRequired);

But I don't have a strong preference about some details, beyond that it should (safely) do the right thing.

[javagl]

The processContentUri could be omitted with

  private async performContentUpgrades(
    tilesetProcessor: BasicTilesetProcessor
  ): Promise<void> {
    await tilesetProcessor.processTileContentEntries(
      (uri: string) => uri, // Identity function - the URI doesn't change
      this.processEntry
    );
  }

(This URI processing was needed for stuff like the .b3dm->.glb upgrades, even though this is always brittle and involves assumptions. From a 3D Tiles spec perspective, it's perfectly valid to have a content.b3dm that actually is a GLB file, so... 🤷‍♂️ )

[javagl]

See other comment about undefined check

[weegeekps]

It would still be nice to have a "full, real-world" test. Some tileset with something like the "unit cube" that was actually generated by the tiler (based on a PLY file), and used verbatim here, verified to be 1. rendered properly by CesiumJS in its old version, and 2. rendered exactly the same by CesiumJS in its new version. [...]

(Or... did you already try this out with real, old ion data?)

I did try this out with some real data from ion, but I didn't include it with unit tests because the files are large. I wholly welcome someone repeating the work though.

[javagl]

didn't include it with unit tests

Of course, that makes sense - I just wondered about some real-world examples. You know, the tilers might have written some "up-axis transform matrix" into the glTF as a node.matrix in one version, but applied some coordinate correction directly in the splats in another version or so. I'll try to run some tests with some (unspecified) examples that I may have in my ion account. (There's even a chance that I once tiled the "unit cube PLY" with the old tiler, but ... will see...)

[weegeekps]

You know, the tilers might have written some "up-axis transform matrix" into the glTF as a node.matrix in one version, but applied some coordinate correction directly in the splats in another version or so.

I hope not because @keyboardspecialist and I wrote all that code, and we tried to keep it consistent between versions. There were many very late caffeine-fueled nights there near the end of the first version, so I guess it's possible. 😬

Let me know if you want the SHAs for the tiler versions to produce any test data. Since this repo is public, I will send those to you directly if you want them.

[javagl]

I'll probably also try it with data that has been generated with that SpzToTileset experiment(!) that I created a while ago (in fact, it is currently not yet using the base extension by default).

But this is exactly one of the reasons why I brought up the axis conversion: There was a bit of back and forth, with the relevant point being that CesiumJS seems to expect a certain axis conversion matrix to be present (and the same data, with the matrix "baked" into the splats instead, was not displayed properly). Related: It was not (and admittedly: still is not) entirely clear for me whether (and where) the SPZ "RUB-to-LUF" conversion should be applied. But I'll probably have to run more systematic tests with the latest states here, to put my finger at "what is wrong".

The difficult thing for these axis conversions is that this is one of the few cases where two wrongs often do make a right
(or worse: a "wrong" that looks nearly "right"...)

[javagl]

The upgradeLegacySplats (or probably upgradeSplats in the future) is not defined in the parseToolArgs function as one of the yargs.... .command(...) definitions. So right now, it is not possible to execute that from the CLI.

[javagl]

Fix in #196

[javagl]

(I just wrote this comment, but it did not show up? Hopefully this is not causing a duplicate...)

This has to be an arrow function to capture the this

[javagl]

Fix in #196