Custom extension support cookbook

[javagl]

This refers to the documentation and implementation of custom extensions, as described in the "Custom Extensions" section at https://gltf-transform.donmccurdy.com/extensions . This is not directly (or not only) a technical "feature request", so here is only a short summary of the "Feature Request" template points:

• Problem description: It's not really obvious how to add support for custom extensions.
• Desired solution: Make adding support for custom extensions easier on a technical level, or provide an easier-to-follow "cookbook"/tutorial".

---

The context is that I tried to implement basic support for the EXT_mesh_features extension. And even though this extension is (in its current form) structurally relatively simple, it was not sooo easy to implement support for that.

Referring to the "Custom Extensions" section on the website, created from this MD file: The "Emitter" example there is not really complete. The ExtensionProperty does not have a type parameter (so there should be an IEmitter interface definition). There are some issues with the properties and static properties of the ExtensionProperty class. (The details might be related to my cluelessness about TypeScript, technical details about declare and override and class initialization and whatnot, but ... for example, I wondered what that init method does, actually...). And beyond that, certain concepts may be obvious for someone who created them, and has implemented several extensions, and knows the underlying mechanisms of property-graph for the same reasons. But for others, "understanding" certain concepts just by looking at the examples may be difficult. A specific example is that of the role of the TextureInfo that is associated with a Texture. Yeah, it is used in many examples, but ... what is this about, exactly?

---

As a hopefully more constructive note: I started implementing that extension. And there are some preliminary notes that I dropped into my local files. These may end up in a similar form in the implementation, eventually. But they are written in a somewhat generic form, and thus, could count as some sort of "cookbook"/"tutorial":

For the ...Def interfaces that define the JSON structure:

//============================================================================
// Interfaces for the JSON structure
// 
// These interfaces reflect the structure of the JSON input, and can be
// derived directly from the JSON schema of the extension.
// 
// The naming convention for these interfaces (and variables that refer
// to them) is that they end with `...Def`.
//
// In the 'read' method of the Extension class, they will be obtained
// from the `context.jsonDoc.json` in raw form, and translated into
// the "model" classes that are defined as
//   export class MeshFeature extends ExtensionProperty<IMeshFeature> {...}
//
// Note that textures are represented as a `GLTF.ITextureInfo`, with
// the `index` and `texCoord` properites. The "model" classes offer 
// this as a `TextureInfo` object that is associated with the `Texture`
// object. This is used internally by glTF-Transform, to automatically 
// do some sort of deduplication magic.
//
// In the 'write' method of the Extension class, these objects will be 
// created from the "model" classes, and inserted into the JSON structure 
// from the `context.jsonDoc.json`.
// 
// The `GLTF.ITextureInfo` objects will be created with 
// `context.createTextureInfoDef`, based on the `Texture´ and
// `TextureInfo` object from the model class.
//  
interface MeshFeatureDef {
  ...
}
...
interface FeatureIdTextureDef extends GLTF.ITextureInfo {
  channels?: number[];
}
//============================================================================

For the I... interfaces that define the structure of the "model" classes (but that are not exposed!)

//============================================================================
// Interfaces for the model classes
//
// These correspond to the classes that are defined as the actual extension
// properties, as
//   export class MeshFeature extends ExtensionProperty<IMeshFeature> {...}
// 
// The naming convention for these interfaces is that they start with `I...`.
// They all have to extend the `IProperty` interface.
// 
// Note that the textures in these interfaces are using the actual 
// `Texture` type of the public glTF-Transform API, and each `texture`
// has an associated `TextureInfo`. This is used internally by glTF-Transform
// for some deduplication magic or whatnot.
//
// These interfaces are NOT publicly visible. They only serve as the type 
// pararameter for the `ExtensionProperty` class, which is the base
// for the actual "model" classes that are exposed to the user.
interface IMeshFeature extends IProperty {
  ...
}
...
interface IFeatureIdTexture extends IProperty {
  channels: number[];
  texture: Texture;
  textureInfo: TextureInfo;
}
//============================================================================

For the actual "model" classes that are visible to the user:

//============================================================================
// The actual model classes
//
// These are exported, and visible to users.
//
// They offer accessor methods for the properties that are defined in
// the model class interfaces. Depending on the type of the properties,
// these accesor methods come in different flavors:
// 
// - For "primitive" property types (like `number`, `boolean` and `string`),
//   the implementations use `this.get(...)`/`this.set(...)`
// - For property types that correspond to other "model" classes, 
//   the implementations use `this.getRef(...)`/`this.setRef(...)`.
// - For property types that correspond to ARRAYS of "model" classes,
//   the implementations don't offer "getters/setters", but instead,
//   they offer `add/remove/list` methods, implemented based on
//   `this.addRef(...)`/`this.removeRef(...)`/`this.listRefs(...)`.
// 
// A special case is that of textures: 
// 
// Each texture in these classes is modeled as a property with the
// type `Texture`, and an associated `TextureInfo`. The `TextureInfo`
// can only be accessed with a `get` method, but not explicitly 
// set: It is managed internally by glTF-Transform. So the for
// an `exampleTextureInfo: TextureInfo` property, there will only 
// be a getter, implemented as 
// ```
// getExampleTextureInfo(): TextureInfo | null {
//   return this.getRef("exampleTexture") ? 
//     this.getRef("exampleTextureInfo") : null;
// }
// ```

export class MeshFeature extends ExtensionProperty<IMeshFeature> {
  static override EXTENSION_NAME = NAME;
  override extensionName = NAME;
  override propertyType = "MeshFeature";
  override parentTypes = [PropertyType.PRIMITIVE];

  protected init(): void {
    // Nothing to do here...?
  }

...

Further comments or hints could refer to the actual Extension class. This could be things that are "simple" in hindight - for example, point out that it has create... methods for the "model" classes, and how they are used in the read method. But it may also refer to information about where and why to call context.setTextureInfo, or details, like what exactly is happening in a line like

const texture = context.textures[textureDefs[textureInfoDef.index].source!];

from one example implementation.

---

On a more technical side:

It would be nice if the redundancy between the Something/ISomething/SomethingDef definitions could be reduced. Or if something like the line about the texture above could be hidden behind some const texture = context.getTextureFor(textureInfoDef) or so. But I due to a lack of background knowledge, I sometimes have to assume that there is no easier solution for certain things, so this is just a vague "Feature Wish" and not a "Feature Request" for now.

[donmccurdy]

Agreed, the documentation covers the API broadly but is lacking in tutorials or cookbooks at the moment. Related topics: #808, #864.

... "Emitter" example there is not really complete. The ExtensionProperty does not have a type parameter (so there should be an IEmitter interface definition)

This example is written in JavaScript, not TypeScript. You're free to write extensions in either language. Like all TypeScript syntax, the IEmitter interface is purely a compile-time concept — it does not exist at runtime, and does not need to be provided if implementing the extension in JavaScript. I generally use JavaScript in examples to keep things simpler for readers, but perhaps it has the opposite effect here. Editors like VSCode offer excellent type hints with TypeScript, which I've found helpful in this project.

... a specific example is that of the role of the TextureInfo that is associated with a Texture...

This is probably beyond what an extension cookbook can cover, though perhaps Concepts should cover it. The TextureInfo class represents data from the glTF specification's own "TextureInfo" and "Sampler" property types. A Material (or Material extension) holds references to Texture and TextureInfo pairs, instead of the Material -> TextureInfo -> Texture nesting of the glTF specification.

[javagl]

A somewhat "higher-level" question about custom extensions (that could also be mentioned briefly in an extended documentation section about extensions in general):

Iff someone implements support for a custom extension, what is the preferred way of dealing with that? Should/could such an implementation eventually become a PR into the extensions package? Or should/will there be something like a third-party-extensions package? Or is the preferred way to ~"just publish it as an own package in npm"?

[donmccurdy]

Discussed briefly under "roadmap" in the contributing guide, and agree it would make sense in a cookbook too —

Most official Khronos Group extensions (those prefixed with KHR) will be implemented on a rolling basis — pull requests are very welcome. Multi-vendor extensions (prefixed with EXT) may be included on a case-by-case basis. Single-vendor extensions (any other prefix) are unlikely to be included directly in the project, although glTF-Transform does provide APIs to build and maintain implementations for those extensions externally.

Some implementations for NEEDLE, OMI, WIRED, and unratified KHR extensions exist on NPM. I've been meaning to keep a list of those, but for now I usually search npm. It's very easy to pull these extensions from NPM into a script-based pipeline with glTF Transform. Using them in the glTF Transform CLI is more experimental.

[javagl]

It's probably difficult to establish a guideline that covers the criteria that are relevant for the "case-by-case" decision. Maybe also because there's not even a clear process about how extensions end up in the Khronos repo to begin with.

A first, very drafty state of the implementation of EXT_mesh_features is currently at https://github.com/CesiumGS/3d-tiles-tools/blob/8c64997786546b224819cbfc7599fe17ad1746ba/src/contentProcessing/gltftransform , including the comments that I quoted above. If you have time to quickly skim over that, to see whether something stands out as being completely and utterly wrong, that would be great. (What's with these init methods, for example?). Otherwise, I would (test and clean that up as part of the broader task, and) consider to pull out the comments into a PR for some sort of "cookbook" section in /docs/.../extensions.md.

[donmccurdy]

If you have time to quickly skim over that, ...

No red flags that I see, a few quick thoughts:

• I've moved toward naming the "Extension" class with its prefix, like EXTMeshFeatures. I think this just makes it easier to read and differentiate from the properties like MeshFeature. No effect on runtime, just readability.
• It might be good to include explicit defaults in getDefaults(), like an empty array for channels.

What's with these init methods, for example?

The reason for init() has to do with JavaScript's inheritance model, as demonstrated here. If we just declare extension properties as class fields or in the subclass constructor, they won't be available when the parent constructor runs, and we need some of them to correctly initialize the class. It's a bit ugly, but declaring them in the class body, and defining them in init() gets around that issue, because the parent constructor invokes init() when it needs them:

https://github.com/donmccurdy/glTF-Transform/blob/b85c53418bb78887751e5abf70ea7a2cee831b4a/packages/extensions/src/ext-mesh-gpu-instancing/instanced-mesh.ts#L15-L24

[donmccurdy]

... criteria that are relevant for the "case-by-case" decision ...

Definitely subjective, but mostly comes down to (1) does this feel like something I can easily maintain, and (2) do I have a positive impression of the extension itself. I tend to view each implementation of an EXT extension as an "upvote" for that thing eventually becoming a KHR extension, so I would not necessarily implement something I view as niche, harmful, or not ready for use.

[javagl]

Carefully avoiding to further talk about your impression about an extension that 'you' proposed ;-), and focussing on the init question:

I expected ~"something like that". There are technical subtleties of property/field initializations, particularly when inheritance comes into play. I mainly know that from other languages (e.g. ones where adding the final keyword can dramatically change certain results). But I'm not familiar enought with the TypeScript/JavaScript class compilation/initialization behavior to know the exact effects of certain combinations of the readonly, abstract, declare, or override keywords...

From a naive, high-level, perspective, and looking at the code part that you quoted and the redundancy in

 public declare propertyType:  'InstancedMesh'; 
 this.          propertyType = 'IntsancedMesh'; 

 public declare parentTypes:  [PropertyType.NODE]; 
 this.          parentTypes = [PropertyType.NODE]; 

I had to wonder whether it wasn't possible to avoid one of them. ^{That typo is not part of the quote, but intentionally inserted to emphasize that point.}

Specifically, I wondered why it was necessary to store these things as properties in the first place. Declaring (abstract!) methods for accessing these values in the base class could solve that: Extending classes have to implement that, and can return a fixed value (so I'd not expect any noteworthy performance implications).

---

FWIW, I played with three versions of such classes:

• One without init (that shows the wrong behavior of the property being undefined in the constructor)
• One with init (similar to what is currently done in glTF-Transform)
• One with an "abstract getter", that might also work...

There's that special case of the static property that adds another dimension of complexity, but I don't know whether the fact that this is not initialized (yet) in the constructor is relevant here (that is also the case for the one with the init method, so it might not be).

The output of the following test is

Without init:
Static name in base without init is undefined
Static name in derived without init is EXAMPLE_PROPERTY_WITHOUT_INIT
Instance name without init is undefined (that's wrong)
With init
Static name in base with init is undefined
Static name in derived with init is EXAMPLE_PROPERTY_WITH_INIT
Instance name with init is ExamplePropertyWithInit (that's right)
With getter
Static name in base with getter is undefined
Static name in derived with getter is EXAMPLE_PROPERTY_WITH_GETTER
Instance name with getter is ExamplePropertyWithGetter (that's also right)

from the code

//============================================================================
// Without init
abstract class ExtensionPropertyWithoutInit {
  public static EXTENSION_NAME: string;
  public abstract readonly extensionName: string;
  constructor() {
    console.log(
      "Static name in base without init is " +
        ExtensionPropertyWithoutInit.EXTENSION_NAME
    );
  }
}

class ExamplePropertyWithoutInit extends ExtensionPropertyWithoutInit {
  static override EXTENSION_NAME = "EXAMPLE_PROPERTY_WITHOUT_INIT";
  public declare extensionName: string;
  constructor() {
    super();
    console.log(
      "Static name in derived without init is " +
        ExamplePropertyWithoutInit.EXTENSION_NAME
    );
    console.log(
      "Instance name without init is " + this.extensionName + " (that's wrong)"
    );
  }
}

//============================================================================
// With init
abstract class ExtensionPropertyWithInit {
  public static EXTENSION_NAME: string;
  public abstract readonly extensionName: string;
  constructor() {
    this.init();
    console.log(
      "Static name in base with init is " +
        ExtensionPropertyWithInit.EXTENSION_NAME
    );
  }
  abstract init(): void;
}

class ExamplePropertyWithInit extends ExtensionPropertyWithInit {
  static override EXTENSION_NAME = "EXAMPLE_PROPERTY_WITH_INIT";
  public declare extensionName: string;
  constructor() {
    super();
    console.log(
      "Static name in derived with init is " +
        ExamplePropertyWithInit.EXTENSION_NAME
    );
    console.log(
      "Instance name with init is " + this.extensionName + " (that's right)"
    );
  }
  init() {
    this.extensionName = "ExamplePropertyWithInit";
  }
}

//============================================================================
// With getter
abstract class ExtensionPropertyWithGetter {
  public static readonly EXTENSION_NAME: string;
  constructor() {
    console.log(
      "Static name in base with getter is " +
        ExtensionPropertyWithGetter.EXTENSION_NAME
    );
  }
  abstract getExtensionName(): string;
}

class ExamplePropertyWithGetter extends ExtensionPropertyWithGetter {
  static override readonly EXTENSION_NAME = "EXAMPLE_PROPERTY_WITH_GETTER";
  constructor() {
    super();
    console.log(
      "Static name in derived with getter is " +
        ExamplePropertyWithGetter.EXTENSION_NAME
    );
    console.log(
      "Instance name with getter is " +
        this.getExtensionName() +
        " (that's also right)"
    );
  }
  override getExtensionName(): string {
    return "ExamplePropertyWithGetter";
  }
}

function run() {
  console.log("Without init:");
  new ExamplePropertyWithoutInit();
  console.log("With init");
  new ExamplePropertyWithInit();
  console.log("With getter");
  new ExamplePropertyWithGetter();
}

run();

But...

1. that simple test might not cover all aspects that are relevant here
2. changing that would be a "breaking change" (with a reasonably simple deprecation/migration path, but still....)

[donmccurdy]

... the exact effects of certain combinations of the readonly, abstract, declare, or override keywords

The important thing really is that none of them affect the compiled code – only whether your compiler shows you warnings, or does not.

I had to wonder whether it wasn't possible to avoid one of them.

I have certainly spent time trying. :) I'd hoped that public class fields might work, but they have the same behavior. I'm still hoping something like that might exist someday, or that typed JavaScript type annotations might have cleaner semantics.

I agree that methods would also solve the problem, but it doesn't necessarily feel better to me. Semantically I would prefer that these be properties, anything else could raise the same questions you're asking of init(). The extra few lines have not affected maintenance so far.

Getters (the language feature, not a method) would also be an option:

class B extends A {
  get type () { return 'B' }
}

const b = new B();
console.log( b.type );

Possibly an extension could do this now, without defining init, and it would just work? Just unsure if this would satisfy TypeScript without more changes upstream.

[donmccurdy]

Carefully avoiding to further talk about your impression about an extension that 'you' propsed

Well I don't think it is harmful, at least. 😅 On complexity and broader appeal I am not sure yet, probably a question of "when" not "if".

[javagl]

Getters (the language feature, not a method) would also be an option:

From a quick test, ...

  abstract get extensionName () : string;
...
  override get extensionName(): string { ... }
...
    console.log("Instance name with getter is " + this.extensionName + " (that's also right)");
    //==============================================================^ no () here!

seems to work as well, but all that with a few uncertainties about the exact behavior.

(Completely subjective: I don't like these get getters. You never know what they do. At the 'call site', x = foo.example might be a plain assignment of a plain property value, or a call to a get example() function that could do anything, and there is no rasonable way to distinguish that by just reading the code - but that's a somewhat unrelated rant about the quirks of JS...)

[donmccurdy]

Haha yes. I would consider it bad practice for a getter to do anything other than returning a value, or perhaps lazy initialization of that value. Most common use case I've found is that the getter simply returns a value, the setter sets a value and also marks a dirty flag on the instance or something like that. Anything more is a bit too magical for me. :)