Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GLTFLoader: Extensibility #11682

Closed
Jeremy-Gaillard opened this issue Jul 4, 2017 · 27 comments
Closed

GLTFLoader: Extensibility #11682

Jeremy-Gaillard opened this issue Jul 4, 2017 · 27 comments

Comments

@Jeremy-Gaillard
Copy link

Hi,

We are currently implement b3dm (a format that encapsulates glTF) support in our project and have encountered a few problems with the current GLTFLoader implementation.

We need to support addition attribute (batchid, used to identify the different objects packed in the geometry), extensions (CESIUM_RTC) and potentially additional uniforms. However, there does not seem to be any way to add these new features without modifying the GLTFLoader file itself.

So my question is: how to support the extensible nature of glTF? Do we contribute directly to the source and add the attributes/extensions/uniforms that we need? Or does the loader need to be modified so extensions can be "plugged in" without changing the source?

Related question: when loading glTFs in a scene rendered using a logarithmic depth buffer, the model's shader are not modified to account for it, resulting in depth issues: http://jsfiddle.net/x6ufnz3y/. I implemented a small hack to fix the problem. Should this process be added to the loader natively? Or could Three provide a helper function to patch the materials?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 5, 2017

So my question is: how to support the extensible nature of glTF?

Hi @Jeremy-Gaillard — I like the idea of modifying the glTF loader to support user-defined extensions, rather than just the extensions we've implemented ourselves. This would make it much easier to support draft extensions like #11577. Definite +1 from me on that. Hopefully we could do this by having a common interface for all extensions with methods like onAfterTexture, onAfterNode, ....

However, I think we would be implementing that extensibility on GLTF2Loader, and not GLTFLoader — are you able to upgrade to the newer loader?

when loading glTFs in a scene rendered using a logarithmic depth buffer, the model's shader are not modified to account for it, resulting in depth issues...

This seems specific to glTF1, as glTF2 does not have custom shaders currently. If it's not too much trouble, it may be best to keep your hack as-is, and if it's still an issue when GLTF2Loader gets custom shader support later, we'll figure it out then. Most development work is on GLTF2Loader at this point.

Some background on the shader thing: custom GLSL shaders were part of the core glTF1.0 specification, and the default export format in COLLADA2GLTF. Since glTF2.0, they've been moved to an optional extension, and that extension hasn't been fully defined yet. The default now is metal-rough PBR, with options for spec-gloss PBR or blinn-phong.

@Jeremy-Gaillard
Copy link
Author

Hopefully we could do this by having a common interface for all extensions with methods like onAfterTexture, onAfterNode, ....

Yes, that is the kind of feature that I need. But I wonder if it would be enough, especially for nested processes (like this switch). Maybe refactoring some of the code could help us. For example, we could replace the switch with something like this:

// Create a list of supported attributes
// The list can be modified by the user to add new attributes
this.supportedAttributes = {};
this.supportedAttributes['POSITION'] = {
   triangleAddAttribute: (geometry, bufferAttribute) => geometry.addAttribute( 'position', bufferAttribute ),
   lineAddAttribute: (geometry, bufferAttribute) => geometry.addAttribute( 'position', bufferAttribute ),
   replaceShader: (shaderText, regEx) => shaderText = shaderText.replace( regEx, 'position' )
};
...
// Replace switch l.1492 by this
this.supportedAttributes[semantic].replaceShader(shaderText, regEx);
// Replace switch l.2134 by this
this.supportedAttributes[attributeId].triangleAddAttribute(geometry, bufferAttribute);
// Replace switch l.2311 by this
this.supportedAttributes[attributeId].lineAddAttribute(geometry, bufferAttribute);

However, I think we would be implementing that extensibility on GLTF2Loader, and not GLTFLoader — are you able to upgrade to the newer loader?
Since glTF2.0, they've been moved to an optional extension

Good to know. Upgrading to GLTF2Loader should not be a problem.

@donmccurdy
Copy link
Collaborator

For custom attributes, a syntax like this might work?

class MyAttributeExtension extends GLTFExtension {
  constructor ( json ) {
    this.json = json;
  }
  onAfterGeometry ( primitive, dependencies, geometry ) {
    if ( primitive.attributes[ 'myAttribute' ] ) {
      geometry.addAttribute( 'myAttribute', bufferAttribute );
    }
  }
}

const loader = new GLTFLoader();
loader.registerExtension(MyExtension);
loader.load('foo.gltf', function () {
  // ...
});

As for patching the shaders, replaceShader is dead code in GLTF2Loader — there are no custom shaders in glTF2.0 yet, just PBR and blinn-phong, so no real way to test this. Until custom shaders are supported, you'd probably be better off writing an extension that creates a THREE.ShaderMaterial itself, like GLTFMaterialsPbrSpecularGlossinessExtension does now. Or just sit tight with what you have working already. 🙂

@donmccurdy
Copy link
Collaborator

@stevenvergenz I'd be interested in your thoughts on this sort of extension syntax, for making it easier to experiment on the AVR_* extensions.

@stevenvergenz
Copy link
Contributor

@donmccurdy I really like the idea of having free-standing extension support, without having to mess with the core loader. If you can define enough functionality hooks to cover the extension surface, I'd be happy to use the AVR extensions as a test.

@donmccurdy
Copy link
Collaborator

Copying over @mrdoob's comment from #12761, we could consider returning the entire JSON object as part of GLTFLoader's response.. doesn't solve all use cases but does allow some self-service custom behavior.

@vspdi
Copy link

vspdi commented Mar 8, 2019

So I was playing around lately with the idea of @donmccurdy in the first comment onAfterTexture, onAfterNode and ended basicly with adding an extension registry into the GLTFLoader.

var loader = new THREE.GLTFLoader();
loader.registerExtension('EXT_my_extension', function(json) {
  function MyExtension( json ) {
    this.name = 'EXT_my_extension'; // todo change register design to better enforce the name
  }

  MyExtension.prototype.onNode = function(def, node) {
     // modify or replace node here
  };

  return new MyExtension(json);
});

I noticed, that you have to add the draco decoder by an extra function (which seems a bit odd in my optinion btw). With the above API the user can add custom extensions.
The main Idea is, that all those Hooks will be called, if the provided extension class has the name in the form of on{{eventType}}

  1. The Implementation is pretty staight forward at this moment.
    I just added a registry and calls in the default cases, which call the provided factory function (so that each parser gets a fresh instance)
  2. In all methods which create the dependencies (node, material, etc. ) the return statement is wrapped in a return parser.acceptUserExtensions('onTexture', textureDef, texture) function, which calls all extensions for which the extensions property has a non null value

So far the raw idea of hooks. There are a coule of more thoughts that need to be done, like how to implement calls eg. onBeforeTexture or global Hooks or sth. like this.

If someones interested, look at this branch vspdi/three.js/gltf-custom-extension
There an example called webgl_loader_gltf_custom_extensions.html

If someone has any better ideas, tell me where to help, I would even like to implement that, because that is a use case i really would love to see 😄

@donmccurdy
Copy link
Collaborator

In all methods which create the dependencies (node, material, etc. ) the return statement is wrapped in a return parser.acceptUserExtensions('onTexture', textureDef, texture) function, which calls all extensions for which the extensions property has a non null value

An issue with wrapping the return statement and passing the already initialized texture like this would be that an extension might not need the pre-extension value. With an extension for compressed textures, the loader should not request the uncompressed textures:

"textures": [
  { "source": 0, "extensions": { "FOO_texture_baz": { "source": 1 } }
]

Before/after hooks might solve that.

I think I'd also suggest that the extension explicitly declare the hooks where it needs to be called, rather than (a) relying on an extension being attached to that point, or (b) checking for existence of particular methods. An extension's data might be on a node, but it actually needs to do its processing after the scene is constructed (e.g. for skinning).

If the hook code can go exclusively in getDependency, that would be really nice. 👍

I noticed, that you have to add the draco decoder by an extra function (which seems a bit odd in my optinion btw).

Just a compromise – we don't want to depend on a global THREE.DRACOLoader variable, but also can't easily split extensions into separate files until the ES module conversion happens. I'm also not sure if Draco decompression could be implemented in a hook-based system like we're discussing here, unfortunately.

With the changes above, I think the API becomes something like:

class MyExtension {
  constructor ( json, parser ) {

  },
  beforeTexture ( textureDef ) { ... }
  afterTexture ( textureDef, texture ) { ... }
}

loader.registerExtension( 'EXT_texture_foo', MyExtension,  ['texture'] )

As a candidate to consider, I think we could move the MSFT_texture_dds extension out of GLTFLoader with something like this. It isn't an official extension, and doesn't necessarily need to be included in the loader.

@vspdi
Copy link

vspdi commented Mar 10, 2019

Before/after hooks might solve that.

Yeah, already thought about that idea. The concept i purposed just covers the "after" state. You mean, by before, you would provide that value (eg. for a texture) and what happens to that afterwards?
So for example:
You have a hook that has onBeforeNode
What is the hook able to do? It gets the value of the nodeDef and is responsible to create a corresponding element? Or does the hook replace the traditional logic that is run by the loader?

If the hook code can go exclusively in getDependency, that would be really nice. 👍

I guess you mean instead of return parser.acceptUserExtensions('onTexture', textureDef, texture)

Just wrap the execution here examples/js/loaders/GLTFLoader.js like this?

case 'node':
  // return or create dependency by extension if provided?
  dependency = this.loadNode( index );
  parser.acceptUserExtensions('onAfterNode', dependency)
  break;

  • But where do we get the definition (I think that the definition is necessary in most cases) from?
    One idea would be to modify the return type of loadXYZ to sth. like { def, value }
  • Whats the purposed
    or even dispatch the above wrapper code outside of the switch case, so there is no duplication of code
if ( ! dependency ) {
  dependency = // create dependency from extensions
  switch ( type ) {
     case 'node':
        dependency = this.loadNode(index, dependency);
        break;
    }
    parser.acceptUserExtensions(type, dependency);
}

Just a compromise – we don't want to depend on a global THREE.DRACOLoader variable, but also can't easily split extensions into separate files until the ES module conversion happens. I'm also not sure if Draco decompression could be implemented in a hook-based system like we're discussing here, unfortunately.

Ah ok, sounds right

I think I'd also suggest that the extension explicitly declare the hooks where it needs to be called, rather than (a) relying on an extension being attached to that point, or (b) checking for existence of particular methods. An extension's data might be on a node, but it actually needs to do its processing after the scene is constructed (e.g. for skinning).

With no checking of method existance and just giving the denpendency type which is used you will need to implement always both methods, dont you? Otherwise the type of dependency must be more specific than texture I think.
Method existance can be precached for efficient reuse when creating the extension, so in general that should be not the problem.

But, why not both?
If some node has extension data, the corresponding method extension will called on existance (either before or after).
If the the dependency type is (like you purposed in your API) added, ALL nodes of this type will go through this extension.
What will be the execution order of extensions that have hooks on the same node type?

@takahirox
Copy link
Collaborator

takahirox commented Apr 12, 2019

I'm interested in the approach discussed here and I'm locally experimenting, too, to see if the following plugins likely can be implemented and how their code look like with a new extensible system (I call plugin here).

  1. Extensions which glTF loader currently supports
  2. Extensions which glTF loader currently doesn't support and user defined custom extensions
  3. Not extensions but some hacks for example loading image as ImageBitmap for texture.

Code is below if anyone is interested in.


Demo
https://raw.githack.com/takahirox/three.js/GLTFLoaderPlugin/examples/webgl_loader_gltf_extensions.html

Repository
https://github.com/takahirox/three.js/tree/GLTFLoaderPlugin

GLTFLoader
https://github.com/takahirox/three.js/blob/GLTFLoaderPlugin/examples/js/loaders/GLTFLoader.js

Plugin API (WIP)
https://github.com/takahirox/three.js/blob/2829631fcae8f5b20f062c02bef17376489a69f6/examples/js/loaders/GLTFLoader.js#L2605-L2735

Plugins (1)
https://github.com/takahirox/three.js/blob/2829631fcae8f5b20f062c02bef17376489a69f6/examples/js/loaders/GLTFLoader.js#L2737-L3269

Plugins (2, 3)
https://github.com/takahirox/three.js/tree/GLTFLoaderPlugin/examples/js/loaders/plugins

example
https://github.com/takahirox/three.js/blob/GLTFLoaderPlugin/examples/webgl_loader_gltf_extensions.html

Plugins (2, 3) registration
https://github.com/takahirox/three.js/blob/2028148efa06647b3c053960f6556c7309a3108d/examples/webgl_loader_gltf_extensions.html#L305-L312

@takahirox
Copy link
Collaborator

The Plugin API I'm trying so far is onBeforeXXX, onAfterXXX, and onXXX where XXX is scene, node, mesh, and so on.

onBeforeXXX is called in getDependency() before loadXXX(). It's designed for overriding glTF definition, for example overriding textureDef.source with textureDef.fooExtension.source.

onAfterXXX is called in getDependency() after loadXXX(). It's designed for overriding generated Three.js object itself or properties for example replacing material.color with materialDef.fooExtension.color.

onXXX is called in loadXXX(). It generates XXX instance on behalf of entire or part of loadXXX(). Potential use case is plugin developer wants to make other type of object, for example not MeshStandardMaterial but MeshBasicMaterial, but one feels that converting from MeshStandardMaterial to MeshBasicMaterial in onAfterXXX is inefficient, one wants to directly create MeshBasicMaterial instead. I'm not really sure if we need onXXX. Only onBefore/AfterXXX might be simpler. But added as an experimental so far.

Return value from onBeforeXXX, onAfterXXX, onXXX accepts Promise.

So getDependency() would be like this.

case 'scene':
	dependency = this._onBefore( 'Scene', json.scenes[ index ] ).then( function ( sceneDef ) {

		return parser.loadScene( sceneDef ).then( function ( scene ) {

			return parser._onAfter( 'Scene', scene, sceneDef );

		} );

	} );

In the code above, this._onBefore/After() is the methods which runs onBefore/AfterXXX() of registered plugins.

Refer to the links in the previous comment for the detail code of plugin API, plugin implementation, plugin registration, and so on.

We need to discuss more for proper plugin API for sure. But from my experiment I think at least extensible system looks right direction because

  1. GLTFParser will look simpler because we can move extension handling out from the parser.
  2. Extension handling will look clearer because it'll be separated from GLTFParser core.
  3. Creating new extension handler(plugin) will be easier for both Three.js devs and Three.js users.

Extensions maintenance can be easier while we can keep GLTFLoader simple.

@garyo
Copy link
Contributor

garyo commented May 13, 2019

On a similar topic, I may need to postprocess some geometry before exporting to glTF. Would there be any interest if I proposed a similar extension mechanism for GLTFExporter?
For instance I need to not export certain subtrees, and change materials to glTF-compatible ones. (If there is any interest I'll follow up in a new issue of course. Don't want to hijack this discussion.)

@takahirox
Copy link
Collaborator

Yeah, I think good to open a new issue.

@takahirox
Copy link
Collaborator

I want to move GLTFLoader extensibility feature ahead, if no opposition to extensibility feature itself, because it looks good from my experiment.

But I realized PR will be big and it will be hard to review/discuss if I do everything, adding Plugin API and moving all existing extension handling to plugin system, in a single PR. So I want to do step by step. First PR is for discussing and adding the API. And in second or later PRs we move existing extensions handling to plugin system one by one. (First PR might include a few of them to help discussion.)

Please note, this won't block adding a new extension support even before we complete the all. For example we may want to add basis texture extension soon.

If it sounds good, I'll start to make a first PR.

@donmccurdy
Copy link
Collaborator

+1 for adding the API first. If existing extensions can safely use that API that would be good to know early on, but maybe we wait to merge that part of the change?

For example we may want to add basis texture extension soon.

It will be a while before that extension is ready, I think. The .basis files will be embedded in a KTX2 wrapper, and that schema is still being determined: KhronosGroup/glTF#1612.

@takahirox
Copy link
Collaborator

If existing extensions can safely use that API that would be good to know early on, but maybe we wait to merge that part of the change?

Yes, knowing early on helps determine the API. But everything in a single PR is very hard to review. This is my trial branch. LOC +3000, -2000.

dev...takahirox:GLTFLoaderPlugin

So I'm thinking to first make a branch including everything and discuss API. And after fixing API, splitting it up into PRs, one PR per one extension moving to plugin system.

@WestLangley
Copy link
Collaborator

But everything in a single PR is very hard to review. ... So I'm thinking to first make a branch including everything and discuss API. And after fixing API, splitting it up into PRs, one PR per one extension

I do not recall anyone trying that approach before... It sounds like a good idea to me. :-)

@donmccurdy
Copy link
Collaborator

@takahirox thank you!

@takahirox
Copy link
Collaborator

Made a new branch including existing extensions moved to plugin system.

dev...takahirox:GLTFLoaderPlugin2

LOC +750 -700 now.

Plugin API I'm trying is.

class MyExtension {
  constructor () {
    this.name = 'EXT_texture_foo';
    this.targets = ['Texture']; // Thinking if this property is really needed

    // This means,
    // if json.textures[textureIndex].extensions['EXT_texture_foo'] exists
    // the following methods will be called while parsing json.textures[textureIndex]
  },

  // If onBeforeXXX method is defined,
  // it is called before parser.loadXXX()
  // @return {GLTF.Texture||Promise<GLTF.Texture>}
  onBeforeTexture (textureDef, parser) {

    // override textureDef

    return textureDef;
  },

  // If onXXX method is defined
  // it is called in parser.loadXXX() to create an XXX instance
  // on behalf of entire or part of parser.loadXXX().
  // Currently Texture, Material, Geometry and Node suppots onXXX
  // @return {THREE.Texture||Promise<THREE.Texture>}
  onTexture (textureDef, parser) {

    var texture = new THREE.Texture();

    // set up texture

    return texture;
  },

  // If onAfterXXX method is defined
  // it is called after parser.loadXXX()
  // @return {THREE.Texture||Promise<THREE.Texture>}
  onAfterTexture (texture, textureDef, parser) {

    // override texture properties or 
    // creating a new texture

    return texture;
  }
}

loader.registerPlugin(new MyExtension());

Feedback is welcome.

@donmccurdy
Copy link
Collaborator

@takahirox the API looks good to me, but are both onBeforeTexture and onTexture needed? It doesn't seem like a callback that can only modify the definition object would be that useful, or at least I can't think of when to use it.

Replacing this...

dependency = this.loadCamera( index );

... with this ...

dependency = this._onBefore( 'Camera', json.cameras[ index ] ).then( ... );

... feels a bit strange to me. Mainly just that _onBefore doesn't seem like the right name for what that method is doing. What's the distinction between that and getDependency here?

@takahirox
Copy link
Collaborator

Thanks for the comment. Sorry I'm a bit busy. I'll comment later, hopefully this weekend.

@takahirox
Copy link
Collaborator

Sorry for the very late response.

@donmccurdy Yeah, that's what I'm struggled with. I thought "modifying definition" somehow can be used but yes I realized any existing extension we support doesn't use. So removing onBefore now and thinking that when needed? But onBeforeGLTF, modifying json before starting to parse, seems necessary. I know some people requested.

@takahirox
Copy link
Collaborator

@garyo Any updates on the exporter extensibility mechanism? A coworker of mine is interested in.

@zeux
Copy link
Contributor

zeux commented Nov 22, 2019

FWIW Babylon.JS implements an extensibility mechanism by allowing extensions to replace a set of existing loading functions with custom versions. Here’s meshoptimizer support implemented with it:
https://github.com/zeux/meshoptimizer/blob/master/demo/babylon.MESHOPT_compression.js

Note that if the extension implementation decides it can’t/doesn’t want to perform the custom processing, it returns null and falls back to the next extension or, ultimately, to baseline implementation. It could also decide to call baseline implementation manually. Overall I like the explicit override mechanism a bit better than before/after callbacks due to more explicit and obvious control flow but maybe after/before fit existing code style better?

Definitely +1 on extensibility in general. I currently maintain a fork of upstream loader and synchronizing changes gets tiring and error prone. Even setting up a simple framework with just a few extendable entrypoints coild be super valuable. We can iterate from this point on...

@takahirox
Copy link
Collaborator

Sorry for delaying so long but I'll try to work on again. I think I have time in Dec.

@takahirox
Copy link
Collaborator

Opened #18421

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 17, 2020

Should be solved via #19144 (which was merged in r118).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants