GLTFLoader .createParser() #15508

takahirox · 2019-01-03T07:30:58Z

This PR adds .getParser() method to GLTFLoader.

Currently GLTFLoader provides parser instance as an argument of onLoad() callback function of .load/parse() to users who want to do some extra works in user side, for example handling their custom extensions.

But it doesn't satisfy some use cases because onLoad() is called after parsing entire data. For example

Editing JSON before parsing. e.g. Replacing resource with smaller one for mobile to reduce download size
Partially loading/parsing data

Then #14492 is proposed, the option giving parser instance without parsing to user via onLoad() callback.

Here I'd like to suggest another API idea .getParser(), and removing parser argument from onLoad() callback. I think separating parser from load/parse()'s callback function is good because most of users don't need parser. parser is the core of GLTFLoader. Unnecessarily giving parser to user is exposing the inside too much.

And parser has some complexity and difficulty for users to use so it may be safer that only users who knows the complexity get the parser with explicit getter call . The examples of the complexity are,

The Parser's API isn't guaranteed to be remained the same.
It has cache so user needs to explicitly clear cache (or to release parser) to release resources.
It can implicitly clone Three.js objects in upper entity, for example material can be cloned in .loadMesh() for SkinnedMesh. Such cloned objects can't be gotten by parser.getDependency().

Examples:

var loader = new THREE.GLTFLoader();
loader.getParser(url, function(parser) {
   // json edit
   parser.json.materials[0]... = ...;

   // partial load
   parser.getDependency('material', 0).then(function(material) {
       // custom extension handling
       var foo = material.userData.gltfExtensions.something;
       parser.getDependency(..., foo).then(...);
   });
});

Other relating threads: #15477

takahirox · 2019-01-03T07:37:16Z

examples/js/loaders/GLTFLoader.js

+
+		},
+
+		_load: function ( url, onLoad, onProgress, onError, getParser ) {


'_' suffix method name indicates private here. If we want real private method we can rewrite

function loadGLTF( gltfLoader, url, onLoad, onProgress, onError, getParser ) { ... }

_load is fine with me. 👍

Let's name the getParser parameter parserOnly.

Renamed getParser to parserOnly, thanks.

donmccurdy

This looks like a good approach, thanks!

donmccurdy · 2019-01-03T23:43:26Z

docs/examples/loaders/GLTFLoader.html

+		</p>
+		<p>
+		Begin loading from url and call the callback function with the parser without parsing.
+		Note that we don't guarantee that the parser's API will remain the same over time.


Suggested wording:

Fetch the asset without its dependencies, and return a parser for incrementally constructing asset content. Resources (e.g. binary or texture files) will be loaded only when needed. For example, when using the asset as a material library the parser can load a particular material and its textures while ignoring the others.

The parser API is primarily designed for internal use. It may require some understanding of the glTF format to use effectively, and may receive breaking changes more frequently than the default .load() method.

Updated, thanks.

donmccurdy · 2019-01-03T23:44:09Z

examples/js/loaders/GLTFLoader.js

+
+		},
+
+		_load: function ( url, onLoad, onProgress, onError, getParser ) {


_load is fine with me. 👍

Let's name the getParser parameter parserOnly.

donmccurdy · 2019-01-03T23:48:23Z

examples/js/loaders/GLTFLoader.js

+
+		},
+
+		getParser: function ( url, onLoad, onProgress, onError ) {


I'm a little conflicted on the getParser name here. Other ideas:

createParser

initParser

loadPartial

loadIncremental

Since this method will make an initial request to get the JSON, I think a verb that implies more side effects than "get" might be better. 🤔

I can understand. In the list createParser sounds good to me.

This method itself initializes and creates parser then IMO loadPartial/Incremental doesn't really express the behavior of the method. For example, user may want to use this method just for editing JSON data before parsing, not for partial/incremental load.

Renamed getParser to createParser so far.

donmccurdy

Thanks! :)

mrdoob · 2019-01-09T22:20:01Z

I'm sure this has been discussed already (I'm having a de ja vu myself)...

Why not splitting it into THREE.GLTFParser and THREE.GLTFLoader? Users that need the parser will then be able to instantiate it themselves.

donmccurdy · 2019-01-09T22:28:10Z

As input, the parser needs access to JSON and any extension objects. So to instantiate one, users must download the file themselves and replecate the ~100 lines here:

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

Lines 141 to 243 in edada31

    
           var content; 
        
           var extensions = {}; 
        
           if ( typeof data === 'string' ) { 
        
           	content = data; 
        
           } else { 
        
           	var magic = THREE.LoaderUtils.decodeText( new Uint8Array( data, 0, 4 ) ); 
        
           	if ( magic === BINARY_EXTENSION_HEADER_MAGIC ) { 
        
           		try { 
        
           			extensions[ EXTENSIONS.KHR_BINARY_GLTF ] = new GLTFBinaryExtension( data ); 
        
           		} catch ( error ) { 
        
           			if ( onError ) onError( error ); 
        
           			return; 
        
           		} 
        
           		content = extensions[ EXTENSIONS.KHR_BINARY_GLTF ].content; 
        
           	} else { 
        
           		content = THREE.LoaderUtils.decodeText( new Uint8Array( data ) ); 
        
           	} 
        
           } 
        
           var json = JSON.parse( content ); 
        
           if ( json.asset === undefined || json.asset.version[ 0 ] < 2 ) { 
        
           	if ( onError ) onError( new Error( 'THREE.GLTFLoader: Unsupported asset. glTF versions >=2.0 are supported. Use LegacyGLTFLoader instead.' ) ); 
        
           	return; 
        
           } 
        
           if ( json.extensionsUsed ) { 
        
           	for ( var i = 0; i < json.extensionsUsed.length; ++ i ) { 
        
           		var extensionName = json.extensionsUsed[ i ]; 
        
           		var extensionsRequired = json.extensionsRequired || []; 
        
           		switch ( extensionName ) { 
        
           			case EXTENSIONS.KHR_LIGHTS_PUNCTUAL: 
        
           				extensions[ extensionName ] = new GLTFLightsExtension( json ); 
        
           				break; 
        
           			case EXTENSIONS.KHR_MATERIALS_UNLIT: 
        
           				extensions[ extensionName ] = new GLTFMaterialsUnlitExtension( json ); 
        
           				break; 
        
           			case EXTENSIONS.KHR_MATERIALS_PBR_SPECULAR_GLOSSINESS: 
        
           				extensions[ extensionName ] = new GLTFMaterialsPbrSpecularGlossinessExtension( json ); 
        
           				break; 
        
           			case EXTENSIONS.KHR_DRACO_MESH_COMPRESSION: 
        
           				extensions[ extensionName ] = new GLTFDracoMeshCompressionExtension( json, this.dracoLoader ); 
        
           				break; 
        
           			case EXTENSIONS.MSFT_TEXTURE_DDS: 
        
           				extensions[ EXTENSIONS.MSFT_TEXTURE_DDS ] = new GLTFTextureDDSExtension( json ); 
        
           				break; 
        
           			case EXTENSIONS.KHR_TEXTURE_TRANSFORM: 
        
           				extensions[ EXTENSIONS.KHR_TEXTURE_TRANSFORM ] = new GLTFTextureTransformExtension( json ); 
        
           				break; 
        
           			default: 
        
           				if ( extensionsRequired.indexOf( extensionName ) >= 0 ) { 
        
           					console.warn( 'THREE.GLTFLoader: Unknown extension "' + extensionName + '".' ); 
        
           				} 
        
           		} 
        
           	} 
        
           } 
        
           var parser = new GLTFParser( json, extensions, { 
        
           	path: path || this.resourcePath || '', 
        
           	crossOrigin: this.crossOrigin, 
        
           	manager: this.manager 
        
           } ); 
        
           if ( parserOnly ) { 
        
           	parser.markDefs(); 
        
           	onLoad( parser ); 
        
           	return;

I guess we could move all of that, except the initial request, into the GLTFParser constructor, like:

parser = new GLTFLoader.Parser( stringOrArrayBuffer, { ... } );

mrdoob · 2019-01-09T22:31:45Z

Yeah. That seems like a nicer API already.

takahirox · 2019-01-09T23:35:47Z

GLTFParser is designed for internal use. I don't think exposing everything is good. For example, if we do some users may want to extend the parser but I don't recommend doing that because we don't guarantee its API will remain the same. Adding parser instance creation method a bit safer. I'd like to suggest we first go with parser instance creation method, and will think the exposing option when actual users request that (and it seems safe and reasonable).

donmccurdy · 2019-01-09T23:45:00Z

@takahirox I don't know if I see the difference — it seems like the same amount is exposed either way, except that the API for creating the parser is different?

takahirox · 2019-01-10T01:54:50Z

Here exposing instance vs exposing class. I think exposing instance is a bit better encapsulation than exposing class, for example users can't (easily) extend class if we don't expose class.

0b5vr · 2019-01-22T08:05:28Z

parser = new GLTFLoader.Parser( sounds cool to me!

takahirox · 2019-01-22T09:06:09Z

Any reasons why exposing class sounds better than exposing instance to you?

0b5vr · 2019-01-22T09:32:33Z

I'm not good at use English so let me talk in actual coding language, here's what I want:

MyGLTFParser = class extends THREE.GLTFParser {
  loadMaterial(materialIndex) {
    return super.loadMaterial(materialIndex).then((material) => {
      // do `material.userData.gltfIndex = {}`-like stuff
      material.userData.gltfIndex.materials = materialIndex;
      // or I can just refer VRM's metadata using `materialIndex` at this point, it's just an example!
    });
  }
}

// then replace GLTFLoader's parser using some sort of magic

Doesn't the implementation sound straightforward?

some users may want to extend the parser but I don't recommend doing that because we don't guarantee its API will remain the same

Yes it's also the point, but I don't think exposing instances instead solves the issue.
Agreeing that GLTFParser 's interface is too complex to be exposed (a bit) though.

yomotsu · 2019-01-22T09:57:34Z

I think ppl who want the Parser may not need loading feature. Therefore, loader.createParser is slightly complicated for them (like me)

How about like this for ppl who want both loader and custom parser?
Rather than making a parser from the loader instance.

const myParser = new MyGLTFParser();
loader.setParser( myParser );

takahirox · 2019-01-22T11:25:56Z

My main concern is the parser is primarily designed for internal use so we don't guarantee the parser's API remain the same. I don't want users to extend the parser because it should be fragile, and I don't want such extended parsers to oppose the gltf parser's refactoring.

So if we expose the parser class, I'd like to make clear which methods should be public and which ones should be hidden. I'm thinking of exposing only .parse() and .getDependency(), and try to keep the behavior of these methods as much as possible. If we do, I think it's ok to expose the parser class. (And I think we should've done that even if we expose only instance.)

/cc @donmccurdy

yomotsu · 2019-01-22T14:41:32Z

exposing only .parse() and .getDependency()

That would be great as a first step!
Also, I personally don't care about breaking changes. three.js has done some already.

donmccurdy · 2019-01-22T21:45:03Z

I don't think createParser() vs new GLTFLoader.Parser() has much effect on how much of the API is exposed. Given an instance, users could still do instance.prototype.foo = bar. But considering parse() and getDependency() the only public/stable methods sounds good. It's not necessary to technically enforce that, e.g. by hiding or freezing other methods, but we could only document those methods (if we document the parser at all).

takahirox · 2019-01-29T18:43:54Z

I meant, either way exposing instance or class (or even without the change because we're already exposing instance via .load()), we should've first clarified which methods of the parser user should be able to directly call.

Even if we don't technically make the other methods private, I think better to add _ prefix like other modules do. With accessibility clarification, I have no reason opposing to exposing the parser class. It should be simpler.

I'd like to divide the PR into two PRs.

Private methods (Add prefix, technically hide, documenting, or something)
Exposing class (or instance)

I'll make soon.

yomotsu · 2019-01-29T19:00:56Z

Sounds good!

jiawenquan · 2019-07-24T12:01:58Z

@donmccurdy @takahirox
I'm new to this, I can't handle conflict resolution manually for gltfloader.js. This PR has been around for a long time, so do you have time to resolve conflicts

mrdoob · 2019-07-25T02:59:52Z

@donmccurdy how do you feel about this PR?

takahirox · 2019-07-25T03:50:22Z

Ah, sorry for pending. As I posted above, I was thinking of dividing this PR into two. (And exposing class rather than instance.) So, please don't merge this PR. Closing so far.

takahirox · 2019-07-25T03:58:00Z

One thing I've been thinking about exposing.

I've been working on GLTFLoader plugin system in #11682 (Sorry for delaying. I took a vacation. I'm back and going to start working on that again.) And coming to think that, do we still really need to expose the parser even if we introduce plugin system?

IIRC, the purposes of exposing are 1. custom operation 2. enable to partial/lazy load on demand.

I'm wondering if plugin system would cover 1. I'm glad If anyone shares any "custom operation" use cases where plugin system can't handle.

But yeah plugin system doesn't solve 2, we need another discussion of course tho.

takahirox added 3 commits December 29, 2018 14:25

Add getParser() to GLTFLoader

6062492

Remove parser from onLoad() arguments of .parse/load() in GLTFLoader

dad6cf3

Update GLTFLoader doc for .getParser()

1389a7f

takahirox commented Jan 3, 2019

View reviewed changes

donmccurdy reviewed Jan 4, 2019

View reviewed changes

takahirox added 4 commits January 4, 2019 15:46

Rename variable name getParser to parserOnly

47c9c54

Update GLTFLoader doc

07e43c6

Rename getParser to createParser in GLTFLoader

de45c2b

Rename getParser to createParser in GLTFLoader doc

edada31

takahirox changed the title ~~GLTFLoader .getParser()~~ GLTFLoader .createParser() Jan 8, 2019

donmccurdy approved these changes Jan 8, 2019

View reviewed changes

donmccurdy mentioned this pull request Jan 11, 2019

Multiple Animations Not Working KhronosGroup/glTF-Blender-IO#219

Closed

0b5vr mentioned this pull request Jan 22, 2019

GLTFLoader: Add accessibility to original JSON index from THREE objects #15418

Closed

jiawenquan approved these changes Jul 24, 2019

View reviewed changes

takahirox closed this Jul 25, 2019

Zielon mentioned this pull request Jan 17, 2020

GLTFLoader: Lazy dependencies loading #18414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GLTFLoader .createParser() #15508

GLTFLoader .createParser() #15508

takahirox commented Jan 3, 2019 •

edited

takahirox Jan 3, 2019

donmccurdy Jan 3, 2019

takahirox Jan 4, 2019

donmccurdy left a comment

donmccurdy Jan 3, 2019

takahirox Jan 4, 2019

donmccurdy Jan 3, 2019

donmccurdy Jan 3, 2019

takahirox Jan 4, 2019 •

edited

takahirox Jan 8, 2019

donmccurdy left a comment

mrdoob commented Jan 9, 2019

donmccurdy commented Jan 9, 2019 •

edited

mrdoob commented Jan 9, 2019

takahirox commented Jan 9, 2019 •

edited

donmccurdy commented Jan 9, 2019

takahirox commented Jan 10, 2019 •

edited

0b5vr commented Jan 22, 2019

takahirox commented Jan 22, 2019

0b5vr commented Jan 22, 2019 •

edited

yomotsu commented Jan 22, 2019 •

edited

takahirox commented Jan 22, 2019 •

edited

yomotsu commented Jan 22, 2019

donmccurdy commented Jan 22, 2019

takahirox commented Jan 29, 2019 •

edited

yomotsu commented Jan 29, 2019

jiawenquan commented Jul 24, 2019

mrdoob commented Jul 25, 2019

takahirox commented Jul 25, 2019 •

edited

takahirox commented Jul 25, 2019 •

edited


		},

		_load: function ( url, onLoad, onProgress, onError, getParser ) {


		},

		getParser: function ( url, onLoad, onProgress, onError ) {

GLTFLoader .createParser() #15508

GLTFLoader .createParser() #15508

Conversation

takahirox commented Jan 3, 2019 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takahirox Jan 4, 2019 • edited

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

mrdoob commented Jan 9, 2019

donmccurdy commented Jan 9, 2019 • edited

mrdoob commented Jan 9, 2019

takahirox commented Jan 9, 2019 • edited

donmccurdy commented Jan 9, 2019

takahirox commented Jan 10, 2019 • edited

0b5vr commented Jan 22, 2019

takahirox commented Jan 22, 2019

0b5vr commented Jan 22, 2019 • edited

yomotsu commented Jan 22, 2019 • edited

takahirox commented Jan 22, 2019 • edited

yomotsu commented Jan 22, 2019

donmccurdy commented Jan 22, 2019

takahirox commented Jan 29, 2019 • edited

yomotsu commented Jan 29, 2019

jiawenquan commented Jul 24, 2019

mrdoob commented Jul 25, 2019

takahirox commented Jul 25, 2019 • edited

takahirox commented Jul 25, 2019 • edited

takahirox commented Jan 3, 2019 •

edited

takahirox Jan 4, 2019 •

edited

donmccurdy commented Jan 9, 2019 •

edited

takahirox commented Jan 9, 2019 •

edited

takahirox commented Jan 10, 2019 •

edited

0b5vr commented Jan 22, 2019 •

edited

yomotsu commented Jan 22, 2019 •

edited

takahirox commented Jan 22, 2019 •

edited

takahirox commented Jan 29, 2019 •

edited

takahirox commented Jul 25, 2019 •

edited

takahirox commented Jul 25, 2019 •

edited