Introduce GLTFLoader plugin system, first round #18484

takahirox · 2020-01-26T01:33:40Z

This PR adds extensiblity to GLTFLoader by adding hook points. I made #18421 but it was too huge so we decided to break up. This PR is first round, it adds extensibility API and move material unlit extension handler to the new system. If this PR looks good and is merged, we are going to make follow up PRs which move other extension handlers (maybe one by one) to the new system.

Benefits

Simplifying GLTFParser by separating extension handlers from core GLTFParser
Easiness to add new, non-concrete, or custom extension handlers without messing GLTFParser
Easiness to reuse even custom extension handlers which others made

Hook points

We add three type hook points.

onBeforeXXX which is called before creating an asset by calling .loadXXX() in the parser. User can override gltf definition. For example, it can work well for an extension handler which chooses an underline asset depending on platform (mobile/desktop) and wants to avoid the cost of creating core spec asset object. Also user can start to asynchronously load certain assets used in onAfterXXX or onXXX.
onAfterXXX which is called after creating an asset by calling .loadXXX() in the parser. User can override the properties of core spec asset object or the object it self. For example, it can work well for an extension handler which just adds some properties to a core spec asset object.
onXXX which is called on behalf of the parser's .loadXXX() if defined. User can create an alternative object. For example, it can work well for an extension handler which creates a different type object from core spec asset object and wants to avoid the cost of creating core spec asset object.

XXX is asset type like Node, Mesh, or Material. Plus, Root for .onBefore/AfterXXX to override entire json/gltf assets.

API and example

class MyMaterialExtensionPlugin {
  constructor () {
    // Plugin must have an unique name.
    // If plugin is an extension handler, the name should be extension name.
    this.name = 'EXT_foo';
  }

  // If onBeforeXXX method is defined it is called before parser.loadXXX()
  // @param {number} assetIndex
  // @param {GLTF.definition} def
  // @param {GLTFParser} parser
  onBeforeMaterial (assetIndex, def, parser) {
    var extensionDef = def.extensions || {};
    if (extensionDef[this.name] === undefined) return;
    // override def or start to asynchronously load certain assets used in onXXX or onAfterXXX
  }

  // If onXXX method is defined it is called to create an XXX instance
  // on behalf of entire parser.loadXXX().
  // @param {number} assetIndex
  // @param {GLTF.definition} def
  // @param {GLTFParser} parser
  // @return {Promise<Object>|null}
  onMaterial (assetIndex, def, parser) {
    var extensionDef = def.extensions || {};
    if (extensionDef[this.name] === undefined) return null;

    var object = new FooMaterial();
    // set up object
    return Promise.resolve(object);
    // return null if you don't want to create instance because of certain reasons.
    // In such a case, next handler is called, 
    // or `GLTFParser.loadXXX()` creates if no more handlers registered
  }

  // If onAfterXXX method is defined it is called after parser.loadXXX().
  // @param {Object} object
  // @param {number} assetIndex
  // @param {GLTF.definition} def
  // @param {GLTFParser} parser
  // @return {Promise<Object>|Object}
  onAfterMaterial (object, assetIndex, def, parser) {
    var extensionDef = def.extensions || {};
    if (extensionDef[this.name] === undefined) return object;

    // override object properties or 
    // creating a new object
    return Promise.resolve(object);
    // Also you can return an object synchronously
    // return object;
  }
}

const loader = new GLTFLoader();
const extension = new MyMaterialExtensionPlugin()
loader.register(extension); // registering plugin
// loader.unregister(loader.plugins[foo]); // if you want to unregister a registered plugin
loader.load( ... );

Main changes in the code

Add hookpoints into .getDependency()
Move assignExtrasToUserData() and addUnknownExtensionsToUserData() from .loadXXX() into .getDependency() because I think they should be called even handler's .onXXX() creates its special object
~~.loadXXX() takes gltf definition instead of gltf asset index~~

//.getDependency()
case 'material':
  var materialDef = json.materials[index];
  this._onBefore(type, index, materialDef);
  dependency = (parser._on(type, index materialDef) || parser.loadMaterial(index)).then(function (material) {
    return parser._onAfter(type, material, index, materialDef);
  }).then(function (material) {
    assignExtrasToUserData(material, materialDef);
    if (materialDef.extensions) addUnknownExtensionsToUserData(extensions, material, materialDef);
    return material;
  });
  break;

Note

I think better to include existing extension handlers in GLTFLoader.js and register them as default for compatibility and avoiding to let user opt-in, and also better to keep adding and registering concrete KHR_ prefix extensions as default. (We may want to remove DDS extension from the default at some point because it isn't KHR_ extension?)
The new system forms promise chain so may have an impact to response time. I roughly measured loading some models and response time became 10-50% slower. I didn't look into deeply yet but I guess no or small impact to total CPU main thread blocking time? If so this impact can be acceptable?

takahirox · 2020-01-26T01:38:14Z

examples/js/loaders/GLTFLoader.js

+
+				if ( materialDef.extensions ) addUnknownExtensionsToUserData( extensions, material, materialDef );
+
+				return material;


Refer to here for this duplicated code with the one in .loadMaterial() which sets up some common properties. I'm thinking what the best way is...

Some options in my mind so far are

Let extension handlers have the duplicated code in the one in the parser like this current PR. The parser code can be simpler but extension handlers may be a bit harder to maintain.

Expose such common properties set up methods to extension handler devs via the parser and let them explicitly call the methods if needed.

Passing custom object ._onXXX() of extension handler creates to .loadXXX() and .loadXXX() sets up some common properties even for custom object like the code in GLTFLoader: Introduce plugin system #18421. Cons are code can be a bit messy and some extension handlers may not want such properties to be overridden because they set up something special.

While our current material extensions are independent material types (metal/rough, spec/gloss, and unlit), in the future there will be material extensions that simply add a specific feature to the metal/rough material. For example, KHR_materials_clearcoat and KHR_materials_sheen would together add clearcoat and sheen to a metal/rough PBR material. See https://github.com/KhronosGroup/glTF/milestone/2 for samples.

I'm not sure what that implies for the options above. 🙃

I guess such simple addition extensions can be handled with .onAfterXXX. (In those material simple addition cases, also with .onBeforeCompile or node-based material if three.js core doesn't support those properties.)

The duplicated code problem I described above can be happened if (a.) extension handler makes an object different type from core spec object and (b.) the handler wants to avoid the cost of creation/download core spec dependent assets. So I chose material unlit extension as the first extension moving to the new system to discuss.

I'm leaning toward option 2. And also moving assignExtrasToUserData() and addUnknownExtensionsToUserData() out from .loadXXX(), and calling in getDependency() instead because these two functions may be necessary to be called regardless of handlers.

We can rewrite to

//.getDependency() case 'material': var materialDef = json.materials[index]; dependency = this._onBefore(type, materialDef).then(function (def) { materialDef = def; return parser._on(type, materialDef); }).then(function (material) { return material || parser.loadMaterial(materialDef); }).then(function (material) { return parser._onAfter(type, material, materialDef); }).then(function (material) { assignExtrasToUserData(material, materialDef); if (materialDef.extensions) addUnknownExtensionsToUserData(extensions, material, materialDef); return material; }); break; // better method name? GLTFParser.prototype.finalizeMaterial = function (material, materialDef) { if (materialDef.name !== undefined) material.name = materialDef.name; if (materialDef.doubleSided === true) material.side = THREE.DoubleSide; var alphaMode = materialDef.alphaMode || ALPHA_MODES.OPAQUE; if (alphaMode === ALPHA_MODES.BLEND) { material.transparent = true; } else { material.transparent = false; if (alphaMode === ALPHA_MODES.MASK) { material.alphaTest = materialDef.alphaCutoff !== undefined ? materialDef.alphaCutoff : 0.5; } } // baseColorTexture, emissiveTexture, and specularGlossinessTexture use sRGB encoding. if (material.map) material.map.encoding = THREE.sRGBEncoding; if (material.emissiveMap) material.emissiveMap.encoding = THREE.sRGBEncoding; return material; }; GLTFParser.prototype.loadMaterial = function (materialDef) { var material = new THREE.MeshStandardMaterial(); .... return this.finalizeMaterial(material, materialDef); }; GLTFMaterialsUnlitExtension.prototype.onMaterial = function (materialDef, parser) { var material = new THREE.MeshBasicMaterial(); .... return this.finalizeMaterial(material, materialDef); };

Moved assignExtrasToUserData() and addUnknownExtensionsToUserData() calls from .loadXXX() into .getDependency() so far.

Coming to wonder if exposing .finalizeXXX() is the best idea because the parser can be complex by having bunch of relatively small methods... I'd like to think of the solution through more extension handlers. I added an inline note that material unlit extension handler currently has common property setup code duplicated with the one in .loadMaterial() so far and want to optimize later if needed. What do you think?

I like this finalizeMaterial function to reuse that behavior.

I think we should remove setting texture encoding from finalizeMaterial, it seems too material specific. For example MeshBasicMaterial doesn't have emissiveMap. Perhaps we can add texture encoding as an additional argument to assignTexture?

I actually think we should have assignTexture take both texture encoding and format. Then we can get rid of this code:

if ( ! texture.isCompressedTexture ) { switch ( mapName ) { case 'aoMap': case 'emissiveMap': case 'metalnessMap': case 'normalMap': case 'roughnessMap': texture.format = THREE.RGBFormat; break; } }

Then assignTexture will no longer have any material specific code.

Making assignTexture non material specific probably sounds like a good idea. One concern is taking encoding and format parameters enables to refer the same texture but with different parameters, like

pending.push(parser.assignTexture(materialParams, 'fooMap', mapDef, THREE.RGBFormat)); pending.push(parser.assignTexture(materialParams, 'barMap', mapDef, THREE.RGBAFormat));

in such a case parser needs to clone texture. So we need to add code maintaining it in assignTexture.

I want to think of this adding encoding and format parameters in another PR (if needed) because I don't really want to make this PR bigger.

Let's not change assignTexture right now, agreed.

donmccurdy

Thanks – this seems manageable to review. :)

examples/js/loaders/GLTFLoader.js

donmccurdy · 2020-01-27T17:59:56Z

examples/js/loaders/GLTFLoader.js

+
+				if ( materialDef.extensions ) addUnknownExtensionsToUserData( extensions, material, materialDef );
+
+				return material;


While our current material extensions are independent material types (metal/rough, spec/gloss, and unlit), in the future there will be material extensions that simply add a specific feature to the metal/rough material. For example, KHR_materials_clearcoat and KHR_materials_sheen would together add clearcoat and sheen to a metal/rough PBR material. See https://github.com/KhronosGroup/glTF/milestone/2 for samples.

I'm not sure what that implies for the options above. 🙃

…lls in getDependency()

…emFirst

takahirox · 2020-02-10T20:35:16Z

Updated to reflect the review comments. Still feedback is very welcome, especially about whether the API is or isn't fitting to the existing, upcoming, or your custom extensions.

…emFirst

takahirox · 2020-02-14T02:28:07Z

This PR is relatively big so easily gets conflicted if other PRs for GLTFLoader are merged. It's hard for me to keep this PR clean. Actually I've resolved the conflicts some times.

I know completing the review and making a decision is hard because of big PR, but I'd be happy if somehow we'd move forward (or terminate this idea) quicker.

Please let me know any concerns to the API, implementation, or anything else if you have. If there is nothing, I hope we can go ahead.

donmccurdy · 2020-02-20T05:36:10Z

I'll try to keep feedback coming more regularly here, although you may still want to skip rebasing the PR for now, I don't want to make more work for you!

Clearcoat is a useful example to consider: it needs to (1) change the material type from MeshStandardMaterial to MeshPhysicalMaterial, and (2) optionally add several textures. Preferably, those textures would be loaded in parallel with any other textures referenced by the material — not sequentially. I'm struggling to understand how we create that API here, does this seem right?

ClearcoatExtension:onBeforeMaterial: initiate requests for clearcoat textures asynchronously
ClearcoatExtension:onMaterial: do not override onMaterial, because we don't want to duplicate the default MeshStandardMaterial creation code?
Core:loadMaterial: initiate requests for default textures, await completion, construct a MeshStandardMaterial
ClearcoatExtension:onAfterMaterial: Copy properties from MeshStandardMaterial to MeshPhysicalMaterial, wait for clearcoat texure requests to finish, then apply clearcoat properties.

Am I correct in understanding that overriding onMaterial would prevent core loadMaterial from running? So that's likely something we'd do for KHR_materials_pbrSpecularGlossiness, but not for KHR_materials_clearcoat...

Since onBeforeMaterial can't block — we want its textures loading at the same time as the default ones — this also implies that onAfterMaterial can recover some state that was created in onBeforeMaterial. If we're sure the same "def" object won't be loaded twice (seems reasonable, we cache for that) then clearcoat might do something like this?

class GLTFMaterialsClearcoatExtension {
  constructor () {
    this.cache = new Map();
  }

  // requests dependencies asynchronously.
  onBeforeMaterial ( materialDef, parser ) {
    var clearcoatMap = textureLoader.load( ... );
    this.cache.set( materialDef, {
      pending: Promise.all( [ clearcoatMap ] ),
      clearcoatMap: clearcoatMap,
      // ...
    } );
    return Promise.resolve(); // resolve immediately
  }

  // waits for onBeforeMaterial results then installs them, blocking until then.
  onAfterMaterial ( material, materialDef, parser ) {
    material = convertStandardToPhysical( material );
    var deps = this.cache.get( materialDef );

    // resolve when all deps are loaded.
    return deps.pending.then(() => {
      material.clearcoatMap = deps.clearcoatMap;
    });
  }
}

robertlong · 2020-02-20T18:32:25Z

examples/js/loaders/GLTFLoader.js

@@ -96,6 +102,24 @@ THREE.GLTFLoader = ( function () {

 		},

+		register: function ( plugin ) {


You might want to make this more explicit by renaming register to registerExtension or registerPlugin.

I renamed from registerPlugin to register in #18484 (comment) I don't have strong preference tho.

robertlong · 2020-02-20T18:50:46Z

examples/js/loaders/GLTFLoader.js

+
+				if ( materialDef.extensions ) addUnknownExtensionsToUserData( extensions, material, materialDef );
+
+				return material;


I like this finalizeMaterial function to reuse that behavior.

I think we should remove setting texture encoding from finalizeMaterial, it seems too material specific. For example MeshBasicMaterial doesn't have emissiveMap. Perhaps we can add texture encoding as an additional argument to assignTexture?

I actually think we should have assignTexture take both texture encoding and format. Then we can get rid of this code:

if ( ! texture.isCompressedTexture ) { switch ( mapName ) { case 'aoMap': case 'emissiveMap': case 'metalnessMap': case 'normalMap': case 'roughnessMap': texture.format = THREE.RGBFormat; break; } }

Then assignTexture will no longer have any material specific code.

examples/js/loaders/GLTFLoader.js

robertlong · 2020-02-20T19:07:23Z

examples/js/loaders/GLTFLoader.js

@@ -1860,43 +2226,62 @@ THREE.GLTFLoader = ( function () {

 		var parser = this;

-		return this.getDependency( 'texture', mapDef.index ).then( function ( texture ) {
+		return this._onBefore( 'map', mapDef ).then( function ( def ) {


Do we need this hook if we move texture format out to assignTexture? It seems like we could move the KHR_texture_transform functionality out to a onAfterTexture hook. I think the existing texture hooks should handle all of the cases you are using the map hook for.

I think we still need. assignTexture handles mapDef (textureInfo in glTF) which has a reference to texture and can have KHR_texture_transform extension, while load/on/onBefore/onAfterTexture handles textureDef (texture in glTF) which defines texture itself.

texture itself can't know whether KHR_texture_transform is applied to it.

Because KHR_texture_transform is nested underneath a material object, I think I would expect it to use onAfterMaterial to apply transforms to textures. We might be able to do without the textureInfo or map callbacks here?

Let me confirm one thing.

In the current proposed API, extension handler plugin should have name and its on/onBefore/onAfterXXX (XXX is Material, Texture, Mesh, or so on) is fired if a XXX asset definition has the extension whose name is same as the extension handler plugin's.

KHR_texture_transform extension is placed in textureInfo, so on/onBefore/onAfterTextureInfo can be fired but on/onBefore/onAfterMaterial won't be. Do you want to change it to that on/onBefore/onAfterMaterial can be fired if underneath textureInfo has a certain extension?

Oh I see... I think plugins should override whatever on/onBefore/onAfter hooks they need to use. If a plugin comes along that needs to use both onBeforeCamera and onAfterMaterial for whatever reason, that's fine.

In other words, the plugin should decide when it gets called, not the location of the extension data in the glTF schema. Two ways to implement this would be:

Loop over all registered plugins to see if they have onBeforeFoo method, calling those that do.

Each plugin has a .hooks = ['texture', ...] array defining the resources it wants to affect. So unlit might have this:

class UnlitExtension { constructor { this.name = 'KHR_materials_unlit'; this.hooks = [ 'material' ]; } onAfterMaterial () { ... } }

The KHR_texture_transform material could then use the material hook, or a new textureInfo hook as you've done here, and either would would work. But ideally, extensions in new places (like material.pbrMetallicRoughness.*) would not always require us to add a new set of three callbacks.

Hm, the reason why I added that limitation is likely we can implement extension handlers of the existing extensions we currently support even with the limitation and I thought it can make the API and implementation simpler and we have less overhead, but yeah it may be important for us to have more flexible API for the future.

Loop over all registered plugins to see if they have onBeforeFoo method, calling those that do.

Each plugin has a .hooks = ['texture', ...] array defining the resources it wants to affect.

I guess 1 can have performance impact if a lot of plugins are registered so am inclined for 2.

Or if we think defining on(Before|After)XXX method and hooks = [ XXX ] property sounds duplicated, we may automatically generate hooks (or similar stuffs), for example when a plugin is registered, by checking what method is defined. I guess checking up to tens of methods one time per a plugin may not hurt.

Let me confirm my understanding. With this example

class UnlitExtension { constructor { this.name = 'KHR_materials_unlit'; this.hooks = [ 'material' ]; } onAfterMaterial () { ... } }

onAfterMaterial is called regardless of whether material definition has KHR_materials_unlit extension so onAfterMaterial checks if the definition has the extension and does nothing if it doesn't have the extension, correct?

onAfterMaterial(material, materialIndex, materialDef, parser) { if (materialDef.extensions.KHR_material_unlit === undefined) { return material; } ... }

BTW

Because KHR_texture_transform is nested underneath a material object, I think I would expect it to use onAfterMaterial to apply transforms to textures.

If I understand correctly, isn't it difficult to handle KHR_texture_transform in onAfterMaterial? It may be impossible for onAfterMaterial to find a mapping from a certain textureInfo to a certain Three.js material property because the mapping is core/extension spec or plugin specific.

For example imagine that there is the following material definition and Foo_material_extension handler plugin creates something special material and assigns fooTexture to Three.js material barMap. I don't think KHR_texture_transform plugin's onAfterMaterial can know the mapping from fooTexture textureInfo to Three.js material barMap.

"material": [{ "extensions": { "Foo_material_extension": { "fooTexture": { // -> Three.js material.barMap "index": 0, "extensions": { "KHR_texture_transform": { ... } } } } } }]

Updated. I have removed the limitation and now on/onBefore/onAfterXXX is always fired regardless of extension definition. So extension handler plugin needs to check extension definition in plugin side to know whether the plugin should process.

Loop over all registered plugins to see if they have onBeforeFoo method, calling those that do.

Each plugin has a .hooks = ['texture', ...] array defining the resources it wants to affect.

I guess 1 can have performance impact if a lot of plugins are registered so am inclined for 2.

I said I'm inclined for 2. but I implemented 1. so far because 1. is easier to implement and I don't see significant performance (response time) degradation for now. Let's revisit if we realize it can have a big performance impact (in some cases).

And regarding where we should handle KHR_texture_transform, I'd like to discuss in more detail in another PR moving KHR_texture_transform handler into the new API, and I'd like to keep textureInfo hook point as is for now.

examples/js/loaders/GLTFLoader.js

robertlong · 2020-02-20T19:48:55Z

@donmccurdy instead of adding another object to the cache, couldn't you just use parser.loadTexture in onBeforeMaterial, resolve immediately, and then parser.loadTexture again in onAfterMaterial and wait for those to resolve?

donmccurdy · 2020-02-20T20:01:47Z

@robertlong that would work, too, yes. There could be extensions that require async work the parser can't provide, like decoding a Basis texture, but the extension could work around that without needing a different API I think.

robertlong · 2020-02-20T20:06:09Z

I think better to include existing extension handlers in GLTFLoader.js and register them as default for compatibility and avoiding to let user opt-in, and also better to keep adding and registering concrete KHR_ prefix extensions as default. (We may want to remove DDS extension from the default at some point because it isn't KHR_ extension?)

I think this is important. For example in Hubs, we want to disable KHR_lights_punctual, we use our own extension instead.

donmccurdy · 2020-02-20T20:07:55Z

Maybe an unregister( extension ) method? or register( name, null )?

takahirox · 2020-02-22T00:32:50Z

ClearcoatExtension:onBeforeMaterial: initiate requests for clearcoat textures asynchronously

ClearcoatExtension:onMaterial: do not override onMaterial, because we don't want to duplicate the default MeshStandardMaterial creation code?

Core:loadMaterial: initiate requests for default textures, await completion, construct a MeshStandardMaterial

ClearcoatExtension:onAfterMaterial: Copy properties from MeshStandardMaterial to MeshPhysicalMaterial, wait for clearcoat texure requests to finish, then apply clearcoat properties.

Yes, this looks the correct approach on the proposed API to me.

Am I correct in understanding that overriding onMaterial would prevent core loadMaterial from running?

Yes, if you define onMaterial it prevents core loadMaterial from running on the proposed API.

those textures would be loaded in parallel with any other textures referenced by the material — not sequentially.

we want its textures loading at the same time as the default ones

To be honest, I had forgotten that we want to load the resources in parallel and I had assumed that we start to load in onAfterMaterial() after onMaterial() completion. But you are right. I'm lucky that there seems being a way to do that on the proposed API. Currently onBeforeXXX() is designed to return def, but maybe it's useless. I change to return nothing.

For example in Hubs, we want to disable KHR_lights_punctual, we use our own extension instead.

Maybe an unregister( extension ) method? or register( name, null )?

Agreed, we should provide a way to disable certain extension handler. Voting for unregister(extension).

donmccurdy · 2020-02-22T19:42:05Z

Sounds good!

EDIT: Perhaps onBefore* does not need to return a Promise, even? It can just be synchronous, as far as I can see.

takahirox · 2020-02-24T18:07:05Z

Perhaps onBefore* does not need to return a Promise, even? It can just be synchronous, as far as I can see.

I'm thinking the same thing. Return nothing, or return promise just in case and use Promise.all.

// parser._onBefore()
const functionName = 'onBefore' + key;
for (const plugin of plugins) {
  if (plugin[functionName]) {
    pending.push(plugin[functionName](args));
  }
}
return Promise.all(pending);

We first go with return nothing and then think of the promise option if we actually get the request from plugin devs?

…emFirst

zeux · 2020-03-08T19:22:12Z

@donmccurdy Is there anything that can be done to move forward with a plugin system? I understand it's a big change, but what I don't understand yet if there's a specific path you have in mind to get there, or specific concerns with this PR that need to be addressed. In other words I'm not sure if we're waiting for something specific, or if there's just a hesitation that won't go away.

Asking because I'm merging my fork with the latest r114 GLTFLoader.js, and lamenting the lack of this - I haven't had to maintain my Babylon.JS extension for months now since it just plugs in, and based on my tests of a earlier version of this change from #18421 my extension would work with that without having to change the loader internals further.

donmccurdy · 2020-03-09T07:23:47Z

My concerns are:

Amount of new code (650 lines here).
Amount of new API surface (3 hooks per dependency type)
Amount of new async call nesting

Let's start with (2), as I think that is a direct cause of the other two. By comparison, the BabylonJS extension API defines roughly one hook for each overideable type. I think we could just let extensions override certain functions that GLTFParser already uses:

loadNode, loadMaterial, loadTexture, ...
extendParams
assignTexture

... maybe, let's not allow extending any API we don't need yet, like loadAnimation.

This prototype is only half-tested, but I think the rough idea works?

dev...donmccurdy:feat-gltfloader-extension-api

donmccurdy · 2020-03-09T05:15:57Z

examples/js/loaders/GLTFLoader.js

+
+			return this;
+
+		},


Could we reduce this to just:

delete this.plugins[ plugin.name ]; return this;

The extra checking and warning is not necessary, I think.

donmccurdy · 2020-03-09T05:16:24Z

examples/js/loaders/GLTFLoader.js

+
+			}
+
+			return this;


Could we reduce this to:

this.plugins[ pluginName ] = plugin; return this;

donmccurdy · 2020-03-09T05:36:02Z

examples/js/loaders/GLTFLoader.js

+
+						return camera;
+
+					} );


I think I'd suggest omitting anything we don't currently need to extend: cameras, animations, scenes, skins, buffers, accessors.

takahirox · 2020-03-09T18:03:01Z

Amount of new API surface (3 hooks per dependency type)

I was thinking of the same stuff. Perhaps I can reduce the three hooks to the one per asset type like

class FooExtensionPlugin {
  constructor() {
    this.name = 'EXT_FOO';
  }

  loadMaterial(index, def, parser) {
    if (def.extensions[this.name] === undefined) return null;
    const extensions = def.extensions[this.name];
    const pending = [];

    // Plugin can make core spec object with parser.loadXXX(index), or
    // it can make its something else special object.
    // Alternate of .onMaterial().
    pending.push(parser.loadMaterial(index));

    // Plugin can request some assets load in parallel of core spec object load.
    // Alternate of .onBeforeMaterial().
    pengind.push(parser.getDependency('texture', extensions.fooTexture.index));

    return Promise.all(pending).then(results => {
      const material = results[0];
      const fooTexture = results[1];

      // Plugin can override core spec object's property.
      // Alternate of .onAfterMaterial().
      material.fooMap = fooTexture;

      return material;
    });
  }
}

GLTFParser.prototype._loadAssetFromPlugin = function (functionName, index, def) {
  const plugins = this.plugins || {};
  for (const pluginName in plugins) {
    var plugin = plugins[pluginName];
    if (plugin[functionName] === undefined) continue;
    const result = plugin[functionName](index, def, this);
    // @TODO: What if a definition has two or more extensions which can be handled by two or more plugins?
    if (result) return result;
  }
  return null;
};

//.getDependency() in parser
case 'material':
  const materialDef = this.json.materials[index];
  dependency = (this._loadAssetFromPlugin('loadMaterial', index, materialDef) || this.loadMaterial(index)).then(material => {
    assignExtrasToUserData(material, materialDef);
    if (materialDef.extensions) addUnknownExtensionsToUserData(extensions, material, materialDef);
    return material;
  });
  break;

takahirox · 2020-03-09T18:26:56Z

I think we could just let extensions override certain functions that GLTFParser already uses:

loadNode, loadMaterial, loadTexture, ...
extendParams
assignTexture

... maybe, let's not allow extending any API we don't need yet, like loadAnimation.

I think I'd suggest omitting anything we don't currently need to extend: cameras, animations, scenes, skins, buffers, accessors.

To clarify,

Does "what we need to extend now" mean the methods which the existing (and likely upcoming) extension handlers need?
If 1 is yes, which do you think (a) we won't expose the new API to devs and it's for internal use or (b) extend more API after we confirm it looks good by trying only the API we currently need?

If (b) of 2 is correct, I think it's good. Small start should be good way to go ahead.

donmccurdy · 2020-03-09T19:11:40Z

I would just give each plugin the parser in its constructor. Then we don't have to pass a parser and a def in each method, they can take index like the parser's loadFoo( index ) methods do.

do you think (a) we won't expose the new API to devs and it's for internal use or (b) extend more API after we confirm it looks good by trying only the API we currently need?

I don't think we should expose extension APIs until a user has a clear need for them. Some of these — in particular loadAccessor — might be called many times in a single asset, which means checking all extensions for the method many times, and we don't have any real use cases for extending e.g. loadAccessor. So until a method needs to be public, it isn't public.

takahirox · 2020-03-09T19:27:57Z

I would just give each plugin the parser in its constructor. Then we don't have to pass a parser and a def in each method, they can take index like the parser's loadFoo( index ) methods do.

Custom extension or non-KHR prefix extension plugins can be implemented out of GLTFLoader. How can we pass parser instance to their constructor?

import FooExtensionPlugin from './plugins/FooExtensionPlugin.js';

const loader = new GLTFLoader();
loader.register(new FooExtensionPlugin());
loader.load(...);

I don't think we should expose extension APIs until a user has a clear need for them. Some of these — in particular loadAccessor — might be called many times in a single asset, which means checking all extensions for the method many times, and we don't have any real use cases for extending e.g. loadAccessor. So until a method needs to be public, it isn't public.

I was thinking of a different view. I guess sooner or later we end up with adding hooks to (almost) all types because of user requests. So it'd be good to see the performance impact at first with hooks to all types. Because I want to avoid the case where we realize the new system isn't scalable later by adding new hooks and end up changing the API.

Update: But either way, I agree with having only hooks we need now in this PR because easier to review less changes.

donmccurdy · 2020-03-09T19:35:45Z

How can we pass parser instance to their constructor?

For example:

// before
loader.register( new FooExtension() );

// after
loader.register( FooExtension );

I guess sooner or later we end up with adding hooks to (almost) all types because of user requests. So it'd be good to see the performance impact at first with hooks to all types.

That is an outcome we can also avoid by saying no to some user requests. Some assets have thousands of accessors, so I don't think there's any good method of polling N extensions for each of 1000+ accessors. I'd rather avoid having that problem than maintain a complex solution for a problem we don't have yet. If you'd like to measure the performance impact with all hooks, that's much appreciated, but is it OK to not expose the hooks yet?

takahirox · 2020-03-09T19:46:03Z

is it OK to not expose the hooks yet?

As I wrote it's ok for me because I want to quickly move it forward.

Update: But either way, I agree with having only hooks we need now in this PR because easier to review less changes.

But I also want to hear @zeux and @robertlong opinions because they may write their plugins once the new API lands.

zeux · 2020-03-09T20:02:39Z

The only hook my extension needed is for the buffer view loading.

takahirox · 2020-03-09T20:04:49Z

@zeux Thanks for the comment. Do you think you can rewrite your extension plugin with the new API suggested in #18484 (comment) ?

@robertlong Do you think your extension can be written with the new API, too?

donmccurdy · 2020-03-09T20:54:59Z

Here are the methods I think would be needed for each extension. Comments welcome:

THREE.GLTFLoader Extension API

There are probably some exceptions we'll discover, like KHR_materials_pbrSpecularGlossiness might have to duplicate some parser.loadMaterial code, or use a different hook.

takahirox · 2020-03-09T21:05:17Z

Thanks for the listing up. Regarding getMaterialType() and extendMaterialParams() extension, do we still need them? Actually I don't use them in unlit extension handler plugin in this PR.

zeux · 2020-03-09T21:09:52Z

@takahirox Yeah I believe so. I have the original code here #18421 (comment) - I think if I can override the loadBufferView and call into the parser.loadBufferView to have the original version, this can work just as well.

donmccurdy · 2020-03-09T21:11:01Z

Perhaps not, but if using them would let us avoid duplicating the alphaMode code it might be worth it. See dev...donmccurdy:feat-gltfloader-extension-api. Or perhaps the extension could call parser.loadMaterial(), copy the properties it wants to a MeshBasicMaterial, and return that.

Also note that clearcoat/sheen/ior extensions cannot all override loadMaterial in this pattern. In my prototype above the load* methods execute only the first implementation that returns non-null, and other methods (extendMaterialParams, ...) allow all extensions to execute.

robertlong · 2020-03-09T21:21:07Z

For Hubs and Spoke we need the following hooks:

loadScene // We store quite a bit of info on the scene node
loadNode // We want to be able to override what Object3D is created
loadAnimation // In Spoke we override animation generation to use uuids instead of the animation name for clip names
loadMaterial // We have a legacy MOZ_alt_materials extensions which needs this hook
loadTexture // We patch the texture loader here

extendParams and assignTexture would also be nice.

The on/onBefore/onAfter hooks are probably unnecessary so long as we can override the behavior of these methods completely. The code reuse in the initial API was better IMO, but if this is simpler then I'm all for it.

zeux · 2020-03-09T21:23:15Z

I think it's key that you can call into the original (non-extended) implementation as part of the extension. Once you have this I feel like we don't need after/before.

takahirox · 2020-03-09T21:29:51Z

Also note that clearcoat/sheen/ior extensions cannot all override loadMaterial in this pattern.

I was thinking the same ones, the suggested API in #18484 (comment) can't handle such simple addition extensions with others.

extendMaterialParams may be ok but sounds similar to onAfterMaterial so it may be kinda we have two hooks for Material. If we have a request from users that they want similar mechanism for other types we may end up with having two hooks per asset type? I know I may be overthinking... perhaps only some of types may need second hook point, other won't need.

donmccurdy · 2020-03-09T21:40:55Z

If we have a request from users that they want similar mechanism for other types we may end up with having two hooks per asset type?

I'd like to start as simply as we possibly can. If we later need to add more hooks, we can do so. The goal is to have 1 hook per 1 place that we absolutely need a hook, and nothing more. If implementing material extensions really requires two hooks, that's OK. If accessors require no hooks, we'll expose no hooks. It's the total number I'd like to reduce, and the amount of async code required to call them.

Having an extension API doesn't mean we have to solve all users' problems – some advanced cases may just have to write their own fork of the loader, and that's fine. We'll try to cover the KHR_ extensions, and provide enough flexibility for users to implement custom extensions that are reasonably similar to those.

robertlong · 2020-03-09T21:54:06Z

extendMaterialParams may be ok but sounds similar to onAfterMaterial so it may be kinda we have two hooks for Material.

Yes, I would like something like extendMaterialParams to add support for multiple extensions that modify the material or simple extensions that just set some property on the material.

The goal is to have 1 hook per 1 place that we absolutely need a hook, and nothing more.

I agree with this, but I do hope that we will not be too conservative in adding additional hooks in the future. If there are multiple users asking for a hook because they can't implement something any other way except by forking the GLTFLoader, then we should probably add that hook. I'm not as concerned about the total number of hooks, but I also don't think we should implement any that aren't absolutely necessary.

takahirox · 2020-03-09T22:06:00Z

Thanks for the comments. I see, it may sound reasonable. Thinking too much about flexibility at first will never get the new API merged.

So which one do you want in this PR?

Unlit extension handler plugin + the new loader API (register and unregister) + the hookpoint unlit extension needs, loadMaterial() (and may be extendMaterialParam() and getMaterialType()?). And we keep adding other hooks when necessary, for example adding assignTexture hook point when we move texture format extension handler to the new system.
Unlit extension handler plugin + the new loader API + all the hook points likely we need soon as listed up above and in the document?

I think 1 can be easier to review because of less changes?

robertlong · 2020-03-09T22:16:00Z

I don't need the hooks for our loader right away. If this is blocking the implementation of the KHR_ extensions then I would focus on those. I'd also like to make sure that @zeux's MESHOPT extension is covered.

donmccurdy · 2020-03-10T05:08:21Z

I'm fine with either (1) or (2), your choice. In dev...donmccurdy:feat-gltfloader-extension-api I think I was maybe halfway between the two. But currently the PR includes more hooks than necessary for either, and that complicates things.

Current:

three.js/examples/js/loaders/GLTFLoader.js

Lines 1672 to 1707 in 28a297a

    
           case 'scene': 
        
           	var sceneDef = json.scenes[ index ]; 
        
           	this._onBefore( type, index, sceneDef ); 
        
           	dependency = ( this._on( type, index, sceneDef ) || this.loadScene( index ) ).then( function ( scene ) { 
        
           		return parser._onAfter( type, scene, index, sceneDef ); 
        
           	} ).then( function ( scene ) { 
        
           		assignExtrasToUserData( scene, sceneDef ); 
        
           		if ( sceneDef.extensions ) addUnknownExtensionsToUserData( extensions, plugins, scene, sceneDef ); 
        
           		return scene; 
        
           	} ); 
        
           	break; 
        
           case 'node': 
        
           	var nodeDef = json.nodes[ index ]; 
        
           	this._onBefore( type, index, nodeDef ); 
        
           	dependency = ( this._on( type, index, nodeDef ) || this.loadNode( index ) ).then( function ( node ) { 
        
           		return parser._onAfter( type, node, index, nodeDef ); 
        
           	} ).then( function ( node ) { 
        
           		assignExtrasToUserData( node, nodeDef ); 
        
           		if ( nodeDef.extensions ) addUnknownExtensionsToUserData( extensions, plugins, node, nodeDef ); 
        
           		return node; 
        
           	} ); 
        
           	break;

Proposed:

https://github.com/donmccurdy/three.js/blob/8f6a6d8c0b472ad2326ba1c9cbf4181a1c131f13/examples/js/loaders/GLTFLoader.js#L1615-L1621

takahirox · 2020-03-10T20:25:15Z

I learned from your prototype that by letting plugin have the same interface as the loader, for example .loadMaterial(index), we can write much simpler. Interesting.

I may go with (1), and update (or create a new PR) hopefully in this week.

takahirox · 2020-03-13T23:37:21Z

Sorry, let me postpone until next week. Let me share the status. Hopefully I want to separate the second hookpoint (like extends extendMaterialParam() and getMaterialType()) from the first plugin system PR including main hookpoint per asset like .loadMaterial() because I guess there might be a chance to take a bit longer to review, discuss and decide about the second hookpoint.

So I was thinking what extension I should first implement. KHR_texture_transform or KHR_light_punctual may be good candidates but no asset in our examples now. KHR_material_unlits can be another good candidate but we need side and alphaMode duplicate code unless we have the second hookpoint, or we need to expose a method setting up side and alphaMode.

Then time is up this week. Let me continue next week. Maybe I go with KHR_material_unlits with (a) we temporally accept the duplicate code and clean up with the second hookpoint later once they're in or (b) I include the second hookpoint in the first PR.

zeux · 2020-03-14T00:48:14Z

fwiw https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/TextureTransformTest/glTF

takahirox · 2020-04-07T21:30:39Z

Sorry for delaying. I had a trouble at home and I couldn't work on it. I would make a new PR hopefully this week.

takahirox · 2020-04-15T23:41:14Z

Closing in favor of #19144

Introduce GLTFLoader plugin system

825cce9

takahirox commented Jan 26, 2020

View reviewed changes

takahirox changed the title ~~Introduce GLTFLoader plugin system~~ Introduce GLTFLoader plugin system, first round Jan 26, 2020

takahirox mentioned this pull request Jan 26, 2020

GLTFLoader: Introduce plugin system #18421

Closed

donmccurdy reviewed Jan 27, 2020

View reviewed changes

takahirox added 4 commits January 29, 2020 06:07

GLTFLoader: Reflect the comments about plugin system

02f3659

GLTFLoader: Clean up

acaa427

Move assignExtrasToUserData() and addUnknownExtensionsToUserData() ca…

af66aa9

…lls in getDependency()

GLTFLoader: Clean up

36e46f5

donmccurdy mentioned this pull request Feb 7, 2020

TypeError: Cannot read property 'array' of undefined pmndrs/gltfjsx#8

Closed

Merge remote-tracking branch 'upstream/dev' into GLTFLoaderPluginSyst…

a3a6f6f

…emFirst

takahirox added 2 commits February 11, 2020 11:18

Merge remote-tracking branch 'upstream/dev' into GLTFLoaderPluginSyst…

beccbad

…emFirst

Merge remote-tracking branch 'upstream/dev' into GLTFLoaderPluginSyst…

7e336db

…emFirst

robertlong reviewed Feb 20, 2020

View reviewed changes

takahirox mentioned this pull request Feb 20, 2020

GLTFLoader: Lazy dependencies loading #18414

Closed

Rename GLTF hookpoint to root

3d98102

takahirox added 4 commits February 24, 2020 11:38

Merge remote-tracking branch 'upstream/dev' into GLTFLoaderPluginSyst…

3fb990c

…emFirst

Rename hookpoint key from Geometry to geometry

78a9458

Return nothing from onBefore

a831ab9

Add unregister method to GLTFLoader

2a6ea61

donmccurdy reviewed Mar 9, 2020

View reviewed changes

takahirox mentioned this pull request Apr 15, 2020

GLTFLoader plugin system, first round #19144

Merged

takahirox closed this Apr 15, 2020


		if ( materialDef.extensions ) addUnknownExtensionsToUserData( extensions, material, materialDef );

		return material;

		@@ -96,6 +102,24 @@ THREE.GLTFLoader = ( function () {

		},

		register: function ( plugin ) {

Introduce GLTFLoader plugin system, first round #18484

Introduce GLTFLoader plugin system, first round #18484

Conversation

takahirox commented Jan 26, 2020 • edited Loading

takahirox Jan 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

takahirox Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox commented Feb 10, 2020

takahirox commented Feb 14, 2020 • edited Loading

donmccurdy commented Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

takahirox Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

takahirox Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

robertlong commented Feb 20, 2020

donmccurdy commented Feb 20, 2020

robertlong commented Feb 20, 2020

donmccurdy commented Feb 20, 2020

takahirox commented Feb 22, 2020 • edited Loading

donmccurdy commented Feb 22, 2020 • edited Loading

takahirox commented Feb 24, 2020 • edited Loading

zeux commented Mar 8, 2020

donmccurdy commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox commented Mar 9, 2020 • edited Loading

takahirox commented Mar 9, 2020 • edited Loading

donmccurdy commented Mar 9, 2020

takahirox commented Mar 9, 2020 • edited Loading

donmccurdy commented Mar 9, 2020

takahirox commented Mar 9, 2020

zeux commented Mar 9, 2020

takahirox commented Mar 9, 2020 • edited Loading

donmccurdy commented Mar 9, 2020

takahirox commented Mar 9, 2020

zeux commented Mar 9, 2020

donmccurdy commented Mar 9, 2020 • edited Loading

robertlong commented Mar 9, 2020 • edited Loading

zeux commented Mar 9, 2020

takahirox commented Mar 9, 2020 • edited Loading

donmccurdy commented Mar 9, 2020

robertlong commented Mar 9, 2020

takahirox commented Mar 9, 2020 • edited Loading

robertlong commented Mar 9, 2020

donmccurdy commented Mar 10, 2020 • edited Loading

takahirox commented Mar 10, 2020 • edited Loading

takahirox commented Mar 13, 2020 • edited Loading

zeux commented Mar 14, 2020

takahirox commented Apr 7, 2020

takahirox commented Apr 15, 2020

takahirox commented Jan 26, 2020 •

edited

Loading

takahirox Jan 26, 2020 •

edited

Loading

takahirox Jan 28, 2020 •

edited

Loading

takahirox Jan 28, 2020 •

edited

Loading

takahirox commented Feb 14, 2020 •

edited

Loading

donmccurdy commented Feb 20, 2020 •

edited

Loading

takahirox Feb 20, 2020 •

edited

Loading

takahirox Feb 25, 2020 •

edited

Loading

takahirox Feb 25, 2020 •

edited

Loading

takahirox Feb 26, 2020 •

edited

Loading

takahirox Feb 27, 2020 •

edited

Loading

takahirox commented Feb 22, 2020 •

edited

Loading

donmccurdy commented Feb 22, 2020 •

edited

Loading

takahirox commented Feb 24, 2020 •

edited

Loading

takahirox commented Mar 9, 2020 •

edited

Loading

takahirox commented Mar 9, 2020 •

edited

Loading

takahirox commented Mar 9, 2020 •

edited

Loading

takahirox commented Mar 9, 2020 •

edited

Loading

donmccurdy commented Mar 9, 2020 •

edited

Loading

robertlong commented Mar 9, 2020 •

edited

Loading

takahirox commented Mar 9, 2020 •

edited

Loading

takahirox commented Mar 9, 2020 •

edited

Loading

donmccurdy commented Mar 10, 2020 •

edited

Loading

takahirox commented Mar 10, 2020 •

edited

Loading

takahirox commented Mar 13, 2020 •

edited

Loading