Extensions are not order independent

[javagl]

Describe the bug

The order of document.createExtension... calls affects whether the resulting document is correct or not.

To Reproduce

Execute this...

import { Document } from "@gltf-transform/core";
import { NodeIO } from "@gltf-transform/core";
import { KHRTextureTransform } from "@gltf-transform/extensions";
import { KHRMaterialsClearcoat } from '@gltf-transform/extensions';

function createDocument(wrongOrder: boolean) {
  const document = new Document();
  document.createBuffer();

  // Create the extensions. 
  // When `wrongOrder==true`, it will first create the KHR_texture_transform
  // extension, causing the texture transforms of the clearcoat texture to
  // be missing
  let khrTextureTransform : KHRTextureTransform;
  let khrMaterialsClearcoat : KHRMaterialsClearcoat;
  if (wrongOrder) {
    khrTextureTransform = document.createExtension(KHRTextureTransform);
    khrMaterialsClearcoat  = document.createExtension(KHRMaterialsClearcoat);
  } else {
    khrMaterialsClearcoat  = document.createExtension(KHRMaterialsClearcoat);
    khrTextureTransform = document.createExtension(KHRTextureTransform);
  }

  // Create a texture
  const texture = document.createTexture();
  texture.setMimeType("image/png");
  texture.setImage(new Uint8Array(10));

  // Create a KHR_texture_transform
  const transform = khrTextureTransform.createTransform();
  transform.setScale([100, 100]);

  // Create a material
  const material = document.createMaterial();
  material.setBaseColorTexture(texture);

  // Assign KHR_texture_transform to the base color texture
  const baseColorTextureInfo = material.getBaseColorTextureInfo();
  if (baseColorTextureInfo) {
    baseColorTextureInfo.setExtension("KHR_texture_transform", transform);
  }

  // Assign the clearcoat extension to the material
  const clearcoat = khrMaterialsClearcoat.createClearcoat();
  clearcoat.setClearcoatTexture(texture);

  // Assign KHR_texture_transform to the clearcoat texture
  const clearcoatTextureInfo = clearcoat.getClearcoatTextureInfo();
  if (clearcoatTextureInfo) {
    clearcoatTextureInfo.setExtension("KHR_texture_transform", transform);
  }
  material.setExtension('KHR_materials_clearcoat', clearcoat);
  return document;
}

async function runTest() {

  const EXAMPLE_EXTENSIONS = [
    KHRTextureTransform,
    KHRMaterialsClearcoat,
  ];
  const io = new NodeIO().registerExtensions(EXAMPLE_EXTENSIONS);

  const rightDocument = createDocument(false);
  const rightJsonDocument = await io.writeJSON(rightDocument);
  console.log(JSON.stringify(rightJsonDocument.json, null, 2));

  const wrongDocument = createDocument(true);
  const wrongJsonDocument = await io.writeJSON(wrongDocument);
  console.log(JSON.stringify(wrongJsonDocument.json, null, 2));
}

runTest();

It will create a document that applies a KHR_texture_transform to a KHR_materials_clearcoat texture, write this out as JSON, and print it to the console. More specifically, it will create this document twice, once with

• registering KHRMaterialsClearcoat then registering KHRTextureTransform (the right order)
• registering KHRTextureTransform then registering KHRMaterialsClearcoat (the wrong order)

With the right order, the texture transform of the clearcoat texture is maintained:

      "extensions": {
        "KHR_materials_clearcoat": {
          "clearcoatFactor": 0,
          "clearcoatRoughnessFactor": 0,
          "clearcoatTexture": {
            "index": 0,
            "extensions": {
              "KHR_texture_transform": {
                "scale": [
                  100,
                  100
                ]
              }
            }
          }
        }
      }

With the wrong order, it is omitted:

      "extensions": {
        "KHR_materials_clearcoat": {
          "clearcoatFactor": 0,
          "clearcoatRoughnessFactor": 0,
          "clearcoatTexture": {
            "index": 0
          }
        }
      }

Expected behavior

It should write the document with the full extension information.

Versions:

NodeJS, @gltf-transform/core 3.10.0

Additional context

I did investigate this a bit. You probably have a "mental model" of what's happening there, but my summary would be:

• It iterates over the extensionsUsed in the writer.ts
• For extensions that involve textures, it will generate the internal mappings for the TextureInfo objects with context.createTextureInfoDef...
• These 'defs' will be picked up by the texture-transform.ts write function, and the extension information will be added to the 'defs'
• When the texture transform extension appears first in the extensionsUsed, then this will not see the 'defs' that are later generated by other extensions (like the clearcoat extension in this example)

Maybe that's not an "issue". Maybe it is anticipated. "Garbage in -> garbage out". But there are some caveats when it comes to finding the right order. First of all: Nobody knows the order (even though it could be pointed out in specific cases). But my gut feeling is that there could be obscure cases where the order for reading things has to be exactly the opposite order that is used for writing things. (That's speculation, and I hope that is not the case (for the task that I want to solve), but should probably be examined, iff the goal of complete order-independence cannot be achieved).

If this is anticipated and/or can/will not be fixed: I wonder in how far implementors could alleviate this problem...

[donmccurdy]

Oof — That sounds painful to have tracked down, thank you and sorry! 😩

100% agreed that the order-dependency here is a bug. With both extensions relying on the write function, the order matters when it shouldn't. In this specific case, the solution is probably to have one or both extensions implement prewrite instead, which can be configured to run at a specific step in the export process, e.g. before writing materials or before writing textures.

Embedding data from extension A under extension B is, otherwise, a rare situation in the glTF spec... so hopefully that simple fix will be enough. But I'll need to think a little more about this.

[javagl]

It was a bit painful, yes, but ... isn't that "Aha! 💡 " moment great? 😁

I did consider prewrite and started to play with that, but haven't thought this through (in view of the related functionality around the internal management of TextureInfo and some of the internal mappings).

In particular:

• I wasn't sure whether it would be enough to basically ~"do all the createTextureInfoDef calls (of my implementation) in prewrite". My gut feeling is that this might solve the issue for that one extension, but I haven't tried it out yet
• If this was the intended solution, then all other extensions that use textures would have to do the same thing

In both cases, I wasn't sure whether it would really work without changes to KHRTextureTransform, so I thought that it could make sense to bring it up here.

---

A late edit: I have been looking for something like postwrite, wondering whether this could be a way to tackle this within KHRTextureTransform, but ... there may be a solution that does not increase the complexity with something like that.

[donmccurdy]

Here's a failing test for starters:

• https://github.com/donmccurdy/glTF-Transform/pull/1257

Regardless of any other fixes, it might make sense for I/O to sort the extensions by name before exporting. Then at least we'll be sure execution order is guaranteed and that unit tests are covering what happens in production.

[donmccurdy]

Pushed to #1257, sorting extensions alphabetically before writing fixes the issue here. I'll probably make that change, but I don't feel comfortable relying only on that solution. Vendor extensions are supposed to work the same way as Khronos extensions in glTF Transform, and here two hypothetical extensions ALPHA_materials_toon and ZETA_materials_toon would behave diferently under sorting.

So, I think we'll also want some TBD change to the implementations of either KHRTextureTransform or KHRMaterials*, possibly requiring new API hooks.

This will have to go into v4 rather than v3.x, as the sorting (at least) is potentially a breaking change.

[javagl]

I'll have to more carefully read the changes, and maybe try it out with some cases, and as I said: There are some internal aspects that I only started to grasp by some debugstepping and trial-and-error.

From a first, quick glance: Sorting the extensions alphabetically will make sure that the are order-independent - so one could say that "the issue is resolved".

But... from my current understanding, it would mean that it will consistently (and order-independently) break in certain cases. This does not refer to the potentially changed (and therefore 'breaking') behavior, but to whether it works correctly or not. Imagine the KHR_material_clearcoat and KHR_texture_transform were called KHR_zealously_renamed_material_clearcoat and KHR_another_name_for_texture_transform Then the KHRTextureTransform would always be executed before the KHRMaterialsClearcoat one, and thus, always omit the transform....

(I have to assume that I misunderstood something here...)

[donmccurdy]

Exactly correct. I plan to sort extensions so that execution order does not depend on registration order. That's preferable regardless. But KHRTextureTransform remains order-dependent, whether the order comes from registration order or sort order, and that's a problem I still plan to find a solution for.

[javagl]

This issue raises some deeper questions about the dependency handling between extensions in general. I hope that it's OK to drop in some (brainstorming) thoughts here.

One question is how specific for KHRTextureTransform the fix will be. Overly suggestive: One could just slam in some postProcessTextureInfoDefs method into the extension and call it a day, but ... no. Some further considerations might be worthwhile here, to have something that is generically applicable to "all" extensions and their interdependencies.

I could imagine that one could, on a very abstract level, build some "dependency graph" of the extensions, apply a topological ordering, and call the readers/writers in this order. There are three levels to this:

• one level where it is trivial: Just build a graph and sort it
• one level where it is nearly impossible: Which elements of the glTF do even have extensions?
• one level where it might be nearly trivial again (although that's just a guess): You have that property-graph library there, and the name suggests that some of the "apparent impossibilities" might not be sooo impossible after all

One question that might even affect the specification: Can there be a topological ordering? I think that the specification currently does not say anything about interdependencies between extensions. And in the worst case, there could be something like

"extensions": {
  "KHR_ping": {
    "level": 0,
    "extensions": {
      "KHR_pong": {
        "level": 1,
        "extensions": {
          "KHR_ping": {
            "level": 2
          }
        }
      }
    }
  }
}

This could mean that implementing readers/writers would require a generic way of storing and maintaining "pending things" (probably within the respective context). And with "pending things", I mean things like the "TextureInfo-defs", but generic. Figuring out a way to say which "pending things" are created by one reader/writer and used (or updated/finalized/committed) by another reader/writer could be tricky.

Maybe I'm overthinking this. Maybe I'm underthingking this...

[javagl]

Sorry... there's another dimension to this issue.

There is the issue of the extensionsUsed within the document. This affects whether a correct document is written.

But when reading the document, then the order of registering the extensions within the io.registerExtensions call affects the result.

The following is an extended version of the test from the initial post. It creates the "right" and "wrong" document, and then tries to read it

• once with registering [ KHRTextureTransform, KHRMaterialsClearcoat ] (the wrong order)
• once with registering [ KHRMaterialsClearcoat, KHRTextureTransform ] (the right order)

It prints whether the transform is found in the clearcoat texture, and the output is

================================================================================
With WRONG registering order:
After reading right document:
transform is there?:  false
After reading wrong document:
transform is there?:  false
================================================================================
With RIGHT registering order:
After reading right document:
transform is there?:  true
After reading wrong document:
transform is there?:  false

So only when the extensions are registered in the right order, and appear in the right order in extensionsUsed, the transform is created.

(EDIT: Of course it does not find it in the "wrong" document. It is not there. The point is that there is another order-dependency here...)

import { Document, Extension } from "@gltf-transform/core";
import { NodeIO } from "@gltf-transform/core";
import { KHRTextureTransform } from "@gltf-transform/extensions";
import { Transform } from "@gltf-transform/extensions";
import { KHRMaterialsClearcoat } from '@gltf-transform/extensions';
import { Clearcoat } from '@gltf-transform/extensions';

function createDocument(wrongOrder: boolean) {
  const document = new Document();
  document.createBuffer();

  // Create the extensions. 
  // When `wrongOrder==true`, it will first create the KHR_texture_transform
  // extension, causing the texture transforms of the clearcoat texture to
  // be missing
  let khrTextureTransform : KHRTextureTransform;
  let khrMaterialsClearcoat : KHRMaterialsClearcoat;
  if (wrongOrder) {
    khrTextureTransform = document.createExtension(KHRTextureTransform);
    khrMaterialsClearcoat  = document.createExtension(KHRMaterialsClearcoat);
  } else {
    khrMaterialsClearcoat  = document.createExtension(KHRMaterialsClearcoat);
    khrTextureTransform = document.createExtension(KHRTextureTransform);
  }

  // Create a texture
  const texture = document.createTexture();
  texture.setMimeType("image/png");
  texture.setImage(new Uint8Array(10));

  // Create a KHR_texture_transform
  const transform = khrTextureTransform.createTransform();
  transform.setScale([100, 100]);

  // Create a material
  const material = document.createMaterial();
  material.setBaseColorTexture(texture);

  // Assign KHR_texture_transform to the base color texture
  const baseColorTextureInfo = material.getBaseColorTextureInfo();
  if (baseColorTextureInfo) {
    baseColorTextureInfo.setExtension("KHR_texture_transform", transform);
  }

  // Assign the clearcoat extension to the material
  const clearcoat = khrMaterialsClearcoat.createClearcoat();
  clearcoat.setClearcoatTexture(texture);

  // Assign KHR_texture_transform to the clearcoat texture
  const clearcoatTextureInfo = clearcoat.getClearcoatTextureInfo();
  if (clearcoatTextureInfo) {
    clearcoatTextureInfo.setExtension("KHR_texture_transform", transform);
  }
  material.setExtension('KHR_materials_clearcoat', clearcoat);
  return document;
}

function printExpectedTextureTransform(document: Document) {
  const root = document.getRoot();
  const material0 = root.listMaterials()[0];
  const clearcoat = material0.getExtension<Clearcoat>("KHR_materials_clearcoat");
  const clearcoatTextureInfo = clearcoat?.getClearcoatTextureInfo();
  const transform = clearcoatTextureInfo?.getExtension<Transform>("KHR_texture_transform");
  console.log("transform is there?: ", transform !== null);
}

async function runTest(wrongRegisteringOrder: boolean) {

  let EXAMPLE_EXTENSIONS : (typeof Extension)[];
  if (wrongRegisteringOrder) {
    EXAMPLE_EXTENSIONS = [
      KHRTextureTransform,
      KHRMaterialsClearcoat,
    ];
  } else {
    EXAMPLE_EXTENSIONS = [
      KHRMaterialsClearcoat,
      KHRTextureTransform,
    ];
  }
  const io = new NodeIO().registerExtensions(EXAMPLE_EXTENSIONS);

  console.log("=".repeat(80));
  console.log("With " + (wrongRegisteringOrder ? "WRONG" : "RIGHT") + " registering order:");

  const rightDocument = createDocument(false);
  const rightJsonDocument = await io.writeJSON(rightDocument);
  //console.log("-".repeat(80));
  //console.log("Right order:");
  //console.log(JSON.stringify(rightJsonDocument.json, null, 2));

  const wrongDocument = createDocument(true);
  const wrongJsonDocument = await io.writeJSON(wrongDocument);
  //console.log("-".repeat(80));
  //console.log("Wrong order:");
  //console.log(JSON.stringify(wrongJsonDocument.json, null, 2));

  const resultRightDocument = await io.readJSON(rightJsonDocument);
  console.log("After reading right document:");
  printExpectedTextureTransform(resultRightDocument);

  const resultWrongDocument = await io.readJSON(wrongJsonDocument);
  console.log("After reading wrong document:");
  printExpectedTextureTransform(resultWrongDocument);
}

async function runTests() {
  await runTest(true);
  await runTest(false);
}

runTests();

---

In the initial post, I wrote

But my gut feeling is that there could be obscure cases where the order for reading things has to be exactly the opposite order that is used for writing things.

It's close. I hate my gut.

[donmccurdy]

Right — order is important when reading a document as well. I believe the unit test in #1257 should cover both reading and writing. I'm planning to make changes to the implementations (beyond sorting), but I'm not yet ready to comment on what the changes will be.

[donmccurdy]

I think I've landed on a two-part fix here:

1. Extension read() and write() methods will execute in order (alphabetically by extension name).
2. Material extensions now implement preread() and prewrite() hooks, rather than read() and write() hooks, to ensure they execute as early as safely possible in the I/O process. The pre- hooks also run in alphabetical order, immediately before reading/writing the resource types specified by extension.prereadTypes and extension.prewriteTypes.

Either change alone would technically fix the current issue. But (1) alone wouldn't prevent similar issues for third-party extensions like a hypothetical ZETA_materials_toon. And (2) alone would leave the project at greater risk of similar issues in the future, because the unit tests can't realistically cover all possible execution orders of all possible extensions. So we'll avoid the dependency on extension registration order and use alphabetical sort.

I'm not entirely satisfied with the solution... I had intended for preread() and prewrite() to be rarely-used escape hatches in the API, but they're now necessary for every material-based extension. Any other solution I can think of would require breaking changes to the API affecting all extension implementors, and involving a dependency graph is something I'd prefer to avoid unless the issue is persistent. For the purposes of the v4 release, I hope that #1257 will do!