Add support for latest 3DGS extensions and deprecate original extension

[weegeekps]

Description

This PR deprecates the KHR_spz_gaussian_splats_compression extension and implements support for the KHR_gaussian_splatting and KHR_gaussian_splatting_compression_spz_2 extensions. These are the latest extensions and much closer to a stable point than the original extension. We only support 3D Tiles that are using both extensions and compressing the data in SPZ, and it's unknown if and when we will be adding support for uncompressed 3DGS data.

Complete removal of the KHR_spz_gaussian_splats_compression extension will be performed in the 1.135 release. This work can be tracked in issue #12842.

I've also updated the unit tests and sandcastles.

Issue number and link

Resolves #12837.

Testing plan

I tested against sandcastles using both the old and new extensions and updated the unit tests.

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @weegeekps!

✅ We can confirm we have a CLA on file for you.

[weegeekps]

Webstorm has been pestering me about this for a while, and it seems harmless to check in.

[weegeekps]

Not entirely pleased with this, but it seemed like the safest and most readable way to handle this was using an IIFE. The only alternative I found was a rather nasty conditional chain. I'm open to ideas.

[javagl]

Is the "nasty chain" something like
const spzExtension = extensions?.KHR_gaussian_splatting?.extensions?.KHR_gaussian_splatting_compression_spz_2;?
(The last '?' is part of my question 😆 )

I tried to follow the path that this object takes, into the depths of what would be the first point in my "This should be refactored NOW!!!"-list, namely the loaders. It's late here. I've lost track. But I'm reasonably sure (and will probably confirm tomorrow) that all this goes 💥 when there is an input file where the data is not stored in bufferView 0. At least, that's how I interpret this line.

(I actually wanted to check where the bufferView of the extension object is finally accessed, and whether it's really necessary to juggle around with that object (or its legacy object), and create specialized ...Spz... classes, when all that this object contains is an index, but ... maybe it has to be like that for some reason, even iff this bufferView is not accessed at all - not sure, shrugging it off for now...)

[weegeekps]

Yes, the loaders are overdue for a refactor.

The nasty chain is that but without the optional chaining operator. I didn't realize we can use the optional chaining operator in pure JS now. Looking at the browser compatibility charts, it looks like it's been supported since 2020 in all major browsers. I'm going to correct this using that operator.

I interpret that line you found the same way. That's definitely a mistake as we should be reading the buffer view value out of the extension. I opened issue #12847 to track this.

[javagl]

That optional chain is still not "pretty". There's always the option to turn this code block into

const spz = fetchSpzExtensionFrom(gltfPrimitive);
if (defined(spz)) {
  needsPostProcessing = true;
  primitivePlan.needsGaussianSplats = true;
}

with a function like (drafted)

/**
 * (5 lines of comment...)
 */
function fetchSpzExtensionFrom(meshPrimitive) {
  const extensions = meshPrimitive.extensions ?? {};
  const legacySpz = extensions.KHR_spz_gaussian_splats_compression;
  if (defined(legacySpz)) {
    console.log("Hey, that's gonna be deprecated soon, but... OK");
    return legacySpz;
  }
  const gaussianSplatting = extensions.KHR_gaussian_splatting;
  if (!defined(gaussianSplatting)) {
    return undefined;
  }
  const innerExtensions = gaussianSplatting.extensions ?? {}; 
  const spzSomewhatLegacy = innerExtensions.KHR_gaussian_splatting_compression_spz;
  if (defined(spzSomewhatLegacy)) {
    console.log("The _v2 is missing, but I'll allow it...");
    return spzSomewhatLegacy;
  }
  const spz = innerExtensions.KHR_gaussian_splatting_compression_spz_2;
  if (!defined(spz)) {
    console.log("Wo-ho... gaussian splatting without SPZ? No, not right now");
    return undefined;
  }
  return spz;
}

(I know, it would be a "style that is inconsistent to the other code", but ... ... ... ... well)

[weegeekps]

That's actually the kind of nasty stuff I was trying to avoid.

In retrospect though I'm wondering if it might be a bit safer approach. The downside is that it will potentially be an additional pain point when we implement support for uncompressed data, but I doubt we will support that anytime soon. Let me try it out.

[weegeekps]

Okay, I took what you suggested and derived something that's a bit cleaner. I like the use of a function because it means we'll always get a single SPZ extension or an undefined object. I removed all of the logging because we're broadcasting the deprecation warning elsewhere and we don't need to barrage users, and I didn't include support for the non _2 version of the SPZ extension name because we don't support that extension here, and we were never outputting files using that setup.

[javagl]

I'm lacking some context in how far uncompressed data is supposed to be handled. And while it does reflect that "(nasty) chain" in more elaborate form, it could probably be shortened a bit if it omitted the log outputs, which (on the other hand) I think can be helpful (at least compared to the "fail-silent" pattern of if (!something) return;).

Some of this is pretty temporary, so maybe there doesn't have to go toooo much thought about how to handle the legacy extension for the next 2 months.

(In my local experiments, I also created something that is pretty preliminary in that regard, just to have ~"a central place where all the differences are handled"...)

[weegeekps]

I'm lacking some context in how far uncompressed data is supposed to be handled.

Today, with our implementation in CesiumJS it is not handled. This is subject to change but it's probably best if we wait until after the base extension is ratified.

Some of this is pretty temporary, so maybe there doesn't have to go toooo much thought about how to handle the legacy extension for the next 2 months.

That's kind of my stance on further changes at the moment. I don't see harm in using the optional operator here as long as it's readable and safe, and we can likely further simplify this once the deprecated extension is removed.

[ggetz]

Thanks @weegeekps! Are there any dependencies between this PR and #12790? Should we review or make sure we merge them in any particular order?

[weegeekps]

@ggetz I'd actually recommend merging this one first and then the SH one to make sure that no updates have gotten missed in the latter.

[ggetz]

Code is looking good overall @weegeekps! Just a handful of small comments.

I'll test and confirm both the new and old datasets are still loading as expected.

[ggetz]

Tested and confirmed behavior in this branch with the new and deprecated extensions. All looking great 👍

[weegeekps]

@ggetz I've addressed all of your feedback. Thank you!

[ggetz]

Awesome looks great. Thanks @weegeekps!