GLTFLoader plugin system, first round #19144

takahirox · 2020-04-15T23:40:49Z

This PR adds hook points to GLTFLoader for the extensibility and rewrite clearcoat extension with the new system as the first extension handler. Let's make follow up PRs to move the other existing extension handlers to the new system one by one and add other necessary hook points for them.

API and example

// For overriding core `.loadMaterial`
class FooMaterialExtensionPlugin {
  constructor (parser) {
    this.parser = parser;
    // Plugin must have an unique name.
    // If plugin is an extension handler, the name should be extension name.
    this.name = 'EXT_foo';
  }

  loadMaterial(materialIndex) {
    const parser = this.parser;
    const json = parser.json;
    const materialDef = json.materials[materialIndex];
    if (!materialDef.extensions || !materialDef.extensions[this.name]) {
      return null;
    }
    const extensionDef = materialDef.extensions[this.name];
    const material = new THREE.FooMaterial();
    material.foo = extensionDef.foo;
    return Promise.resolve(material);
  }
}

// For adding some properties to core spec material
class BarMaterialExtensionPlugin {
  constructor (parser) {
    this.parser = parser;
    this.name = 'EXT_bar';
  }

  getMaterialType() {
    return THREE.BarMaterial;
  }

  extendMaterialParams(materialIndex, materialParams) {
    const parser = this.parser;
    const json = parser.json;
    const materialDef = json.materials[materialIndex];
    if (!materialDef.extensions || !materialDef.extensions[this.name]) {
      return Promise.resolve();
    }
    const extensionDef = materialDef.extensions[this.name];
    materialParams.bar = extensionDef.bar;
    return Promise.resolve();
  }
}

const loader = new GLTFLoader();
loader.register(parser => new FooMaterialExtensionPlugin(plugin));
loader.register(parser => new BarMaterialExtensionPlugin(plugin));
// loader.unregister(loader.pluginCallbacks[0]); // if you want to unregister a registered plugin
loader.load( ... );

Hook points this PR adds

loadMaterial
extendMaterialParams
getMaterialType
loadBufferView for @zeux's MESHOPT extension
loadMesh for @feiss's text mesh extension

WestLangley · 2020-04-16T00:26:32Z

Supporting chaining, also, would be nice for consistency.

new GLTFLoader()
    .register( FooMaterialExtensionPlugin )
    .register( BarMaterialExtensionPlugin )
    .load( ... );

takahirox · 2020-04-16T00:40:04Z

Good catch. I had forgotten to do that in this PR tho I did in the previous PR. Updated.

robertlong · 2020-04-16T02:58:34Z

This is great! I don't have anything to add, other than I'd love to have a loadNode API, but I can wait if that would hold this up.

munrocket · 2020-04-16T12:41:02Z

All is ok with CI here, freezing was fixed #19147
Changes not even covered with examples yet.

zeux · 2020-04-23T06:48:31Z

Thanks for working on this! A couple notes:

It would be nice to be able to somehow configure an extension object. For example, my extension requires a decoder module that in three.js is usually (I think?) specified externally. See setDRACOLoader for example. Right now register() creates the plugin instance so I'm not sure how to do this.
This code needs to be adjusted to recognize registered extensions and suppress the warning:

if ( extensionsRequired.indexOf( extensionName ) >= 0 ) {

    console.warn( 'THREE.GLTFLoader: Unknown extension "' + extensionName + '".' );

}

Other than these two issues I was able to get my extension running without changing GLTFLoader.js 🎉

zeux · 2020-04-23T06:50:15Z

For pt1, the way Babylon.JS works is it permits this:

loader.register((parser) => new MESHOPT_compression(parser, MeshoptDecoder));

(which doesn't work with this PR but probably could be made to work?)

zeux · 2020-04-23T07:17:53Z

... oh, the magic of JavaScript. The arrow function doesn't work but this does work:

loader.register(function (parser) { return new MESHOPT_compression(parser, MeshoptDecoder); });

So the only issue is the warning; suppressing this might be slightly awkward though with the current interface because the plugins must be created after the parser, but to create the parser we need the extension list. Maybe this can work with a second pass like this (after setPlugins call):

			parser.setPlugins( this.plugins );

			if ( json.extensionsRequired ) {

				for ( var i = 0; i < json.extensionsRequired.length; ++ i ) {

					var extensionName = json.extensionsRequired[ i ];

					if ( !parser.extensions[ extensionName ] ) {

						console.warn( 'THREE.GLTFLoader: Unknown extension "' + extensionName + '".' );

					}
				}
			}

			parser.parse( onLoad, onError );

This is a GLTFLoader plugin for THREE.js that works with mrdoob/three.js#19144. Once that PR goes through we will be able to stop maintaining a custom GLTFLoader build (modulo Basis/KTX2 changes that are still a bit in flux...).

takahirox · 2020-04-23T16:53:05Z

@zeux Good catch! Yes, I think we should be able to pass parameter to plugin. I'll update.

takahirox · 2020-05-06T01:48:00Z

@zeux

It would be nice to be able to somehow configure an extension object. For example, my extension requires a decoder module that in three.js is usually (I think?) specified externally. See setDRACOLoader for example. Right now register() creates the plugin instance so I'm not sure how to do this.

I updated to resolve 1. so far.

loader.register(parser => new FooPlugin(parser));

the magic of JavaScript. The arrow function doesn't work

Arrow function seems to work here. What type of error do you see?

zeux · 2020-05-06T02:04:47Z

@takahirox That comment was referring to old version of the code with ‘new’; with callbacks I believe arrow functions should work just fine.

takahirox · 2020-05-06T02:29:40Z

OK, thanks. And also I updated to resolve 2.

zeux · 2020-05-08T02:14:42Z

Thanks! Confirmed that the new code fixes all issues for my use case. Looks like all that's left is for @donmccurdy to review this?

robertlong · 2020-05-09T17:47:56Z

examples/js/loaders/GLTFLoader.js

@@ -2160,6 +2270,18 @@ THREE.GLTFLoader = ( function () {

 			}

+			materialType = this._invokeOne( function ( ext ) {
+
+				return ext.getMaterialType && ext.getMaterialType();


getMaterialType() should take an argument materialDef or materialIndex so that you can optionally use it to determine if a material applies to just this material definition.

Agreed – I'd vote for materialIndex for consistency with load methods.

Sounds good to me. Added materialIndex, thanks.

donmccurdy

A couple small comments but I think this is close! Thank you @takahirox!

donmccurdy · 2020-05-09T21:43:44Z

examples/js/loaders/GLTFLoader.js

@@ -110,10 +113,35 @@ THREE.GLTFLoader = ( function () {

 		},

+		register: function ( callback ) {
+
+			if ( ! this.pluginCallbacks.includes( callback ) ) {


Let's avoid array methods missing from IE11 if we can, see #19293. It is simpler if the only polyfill GLTFLoader requires is the Promise API.

I was missing IE11 doesn't support includes(). Replaced with indexOf(), thanks.

donmccurdy · 2020-05-09T21:46:10Z

examples/js/loaders/GLTFLoader.js

+
+		this.plugins = plugins;
+
+	};


We don't intend for users to call setPlugins or setExtensions, right?

Right, I don't expect users to call them.

donmccurdy · 2020-05-09T21:49:23Z

examples/js/loaders/GLTFLoader.js

@@ -2160,6 +2270,18 @@ THREE.GLTFLoader = ( function () {

 			}

+			materialType = this._invokeOne( function ( ext ) {
+
+				return ext.getMaterialType && ext.getMaterialType();


Agreed – I'd vote for materialIndex for consistency with load methods.

donmccurdy · 2020-05-16T02:58:13Z

Would it be OK to just have an extensions object, without the plugins object? And then as we convert the existing extensions to use the new system, they would be added to an extensionCallbacks list?

I realize that not all plugins will be related to glTF extensions, but I don't think it's necessary to try to separate the two concepts inside the loader, when they behave the same way.

takahirox · 2020-05-16T04:12:37Z

My plan is, plugins has the extension handlers using the new system while extensions has the extension handlers using the existing system, we will convert the existing extension handlers to the ones using the new system, and when we finish converting we will have only plugins (or extensions, either name is fine to me) object. plugins holds the new system handlers so it will have both glTF extension handlers and non-glTF-related handlers.

Let me confirm my understanding. Are you suggesting we would be better to have only one object even before we finish converting?

donmccurdy · 2020-05-16T05:02:03Z

Ok, I'm fine with having both during the transition. Once that is finished I think I would prefer to have only extensions.

robertlong · 2020-06-04T22:13:05Z

Can we get a status update on this PR? I'm assuming we're waiting on tests, examples, and fixing merge conflicts? Is there more that needs to be figured out for the API?

takahirox · 2020-06-10T01:36:58Z

I think I have reflected all the comments to the code. Let me know if I have missed anything.

donmccurdy

Extensions are go! 🚀

feiss · 2020-06-10T09:49:20Z

Weeeeeeee!!

examples/jsm/loaders/GLTFLoader.js

mrdoob · 2020-06-11T04:44:00Z

Thanks!

takahirox · 2020-06-23T18:44:07Z

Thanks for merging.

By the way, I should have noted about loadNode() hookpoint.

I think we definitely should have loadNode() hookpoint but I want to discuss a bit about it separatedly from the first plugin PR so I didn't include it in this PR.

In this PR we use invokeOne() for loadXxx(). But it doesn't fit to Light extension. A node can have mesh, camera, and light extension at a time and all the objects should be created under the node. invokeOne() doesn't fit to it because invokeOne() stops to call the core loadXxx() if extension's loadXxx() creates an object. Then mesh and camera objects will not be created.

Probably we need workaround or something. Let's discuss more closely when we move Light extension to the new system.

donmccurdy · 2020-06-23T18:55:11Z

That makes sense, sounds like nodes should use invokeAll instead?

takahirox · 2020-06-23T19:32:48Z

invokeAll would work for light extension but not really sure for other possible node extensions yet. I'd like to hear more comments, for example I heard Hubs team wants node hookpoint and @feiss may need it for his text extension so I'd like to know if it satisfies their demands. (I know we should prioritize to support Khronos glTF extension but also it would be worth if we can support possible custom extensions without complicating.)

This thread is already closed, I'd like to continue the discussion in a new open thread. Probably it will be a good opportunity when moving light extension to the new system.

donmccurdy · 2020-06-23T20:01:47Z

We can use invokeAll for some hooks and invokeOne for others, that's the reason for having both. It seems pretty clear that loadNode must use invokeAll.

takahirox · 2020-06-23T20:41:11Z

I thought there could be a case where an extension in a node points to a mesh/camera and the core node points to another mesh/camera as fallback, meaning either one should be created depending on whether the extension is supported. But it seems to unlikely happen in possible custom extensions or future Khronos glTF extensions? If so, invokeAll should work fine.

"nodes": [
  "extensions": {
    "Foo_extension": {
      "mesh": 1
    }
  },
  "mesh": 0 // as fallback
]

robertlong · 2020-06-23T21:50:00Z

Here's what I did for an internal Hubs hackathon: Hubs-Foundation/three.js@hubs/master...MozillaReality:hubs/feature/plugin-api

I needed to swap out all the Object3D types for one with an ECS mixin. We don't have this requirement anymore, but it was a useful thought exercise.

These were the hooks I implemented:

loadNode(index)
createPrimitive(meshIndex, primitiveIndex, geometry, material)
finalizeMesh(meshIndex, primitives)
createCamera(cameraIndex)

So even though I don't have concrete uses for these now, here are my thoughts on how you might use these methods:

loadNode lets you totally override the behavior of the loadNode method. I think most of the time you don't want to do this. loadNode does a lot. We'd need to break up the contents of that method into reusable parts.

createPrimitive lets you pick what Object3D class to use for a specific primitive, maybe I'd use this for creating an InstancedMesh or LOD?

finalizeMesh is where you take all of the primitives and add them to a Group, you could do anything that needed access to all of the primitive Object3Ds here.

createCamera is where the camera Object3D (PerspectiveCamera and OrthographicCamera) are created. The camera properties are still assigned in the body of loadCamera

I don't think any of these are final APIs, but maybe it's useful to see how I broke it apart. I think the text extension would be a good one to focus on for designing an API. The text extension isn't going to touch the mesh loading part of the GLTFLoader. It really just needs to alter what Object3D is returned from loadNode and I think that'd be a good starting point.

Introduce GLTFLoader plugin system

dc390fc

This was referenced Apr 15, 2020

Introduce GLTFLoader plugin system, first round #18484

Closed

GLTFLoader: Introduce plugin system #18421

Closed

GLTFLoader method chainable register/unregister

c2fe64e

feiss mentioned this pull request Apr 16, 2020

aframe? MozillaReality/MOZ_text#1

Closed

mrdoob requested a review from donmccurdy April 20, 2020 11:38

mrdoob added this to the rXXX milestone Apr 20, 2020

donmccurdy mentioned this pull request Apr 26, 2020

Support for "MESHOPT_compression" GLTF Extension google/model-viewer#1164

Closed

GLTFLoader: take callback in .register()

74fedc2

GLTFLoader: Suppress unknown extension warning

f963837

donmccurdy mentioned this pull request May 7, 2020

GLTFLoader: record association between glTF elements and Three.js objects #19257

Closed

13 tasks

robertlong reviewed May 9, 2020

View reviewed changes

donmccurdy reviewed May 9, 2020

View reviewed changes

takahirox added 2 commits May 11, 2020 11:16

GLTFLoader: Pass materialIndex argument to getMaterialType()

879f28f

GLTFLoader: Replace Array.includes() with Array.indexOf()

91d296e

donmccurdy mentioned this pull request May 18, 2020

Draft: MESHOPT_compression KhronosGroup/glTF#1702

Closed

Repository owner deleted a comment May 18, 2020

Merge branch 'dev' into GLTFLoaderPluginSystemNew

f33b606

donmccurdy approved these changes Jun 10, 2020

View reviewed changes

mrdoob reviewed Jun 11, 2020

View reviewed changes

examples/jsm/loaders/GLTFLoader.js Outdated Show resolved Hide resolved

Replace soft tags with hard tabs

7208eb8

mrdoob merged commit 7d976f3 into mrdoob:dev Jun 11, 2020

takahirox deleted the GLTFLoaderPluginSystemNew branch June 11, 2020 05:20

Mugen87 modified the milestones: rXXX, r118 Jun 22, 2020

0b5vr mentioned this pull request Jul 17, 2020

Use GLTFLoader plugin system to load MToonMaterial pixiv/three-vrm#329

Closed

Mugen87 mentioned this pull request Jul 17, 2020

GLTFLoader: Extensibility #11682

Closed

takahirox mentioned this pull request Jul 21, 2020

Add node hookpoint to GLTFLoader and move the lights extension handler to the new system #19892

Merged

This was referenced Nov 19, 2020

types (GLTFLoader): add definitions about plugin system #20713

Merged

GLTFLoader: Make KHR_materials_unlit able to be overridden #20722

Closed

GLTFLoader plugin system, first round #19144

GLTFLoader plugin system, first round #19144

Conversation

takahirox commented Apr 15, 2020 • edited Loading

WestLangley commented Apr 16, 2020

takahirox commented Apr 16, 2020

robertlong commented Apr 16, 2020

munrocket commented Apr 16, 2020

zeux commented Apr 23, 2020

zeux commented Apr 23, 2020

zeux commented Apr 23, 2020

takahirox commented Apr 23, 2020

takahirox commented May 6, 2020

zeux commented May 6, 2020

takahirox commented May 6, 2020

zeux commented May 8, 2020

robertlong May 9, 2020 • edited Loading

Choose a reason for hiding this comment

donmccurdy May 9, 2020

Choose a reason for hiding this comment

takahirox May 11, 2020

Choose a reason for hiding this comment

donmccurdy left a comment

Choose a reason for hiding this comment

donmccurdy May 9, 2020

Choose a reason for hiding this comment

takahirox May 11, 2020

Choose a reason for hiding this comment

donmccurdy May 9, 2020

Choose a reason for hiding this comment

takahirox May 11, 2020

Choose a reason for hiding this comment

donmccurdy May 9, 2020

Choose a reason for hiding this comment

donmccurdy commented May 16, 2020

takahirox commented May 16, 2020 • edited Loading

donmccurdy commented May 16, 2020

robertlong commented Jun 4, 2020

takahirox commented Jun 10, 2020

donmccurdy left a comment

Choose a reason for hiding this comment

feiss commented Jun 10, 2020

mrdoob commented Jun 11, 2020

takahirox commented Jun 23, 2020 • edited Loading

donmccurdy commented Jun 23, 2020

takahirox commented Jun 23, 2020 • edited Loading

donmccurdy commented Jun 23, 2020

takahirox commented Jun 23, 2020 • edited Loading

robertlong commented Jun 23, 2020 • edited Loading

takahirox commented Apr 15, 2020 •

edited

Loading

robertlong May 9, 2020 •

edited

Loading

takahirox commented May 16, 2020 •

edited

Loading

takahirox commented Jun 23, 2020 •

edited

Loading

takahirox commented Jun 23, 2020 •

edited

Loading

takahirox commented Jun 23, 2020 •

edited

Loading

robertlong commented Jun 23, 2020 •

edited

Loading