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

GLTF2Loader should load dependencies top-down. #11924

Closed
donmccurdy opened this issue Aug 11, 2017 · 3 comments
Closed

GLTF2Loader should load dependencies top-down. #11924

donmccurdy opened this issue Aug 11, 2017 · 3 comments

Comments

@donmccurdy
Copy link
Collaborator

donmccurdy commented Aug 11, 2017

The current pattern in GLTF2Loader looks something like this:

class GLTF2Parser {
  loadMeshes () {

    return this._withDependencies( [

      'materials',
      'accessors'

    ] ).then( ( dependencies ) => {

      _each( json.meshes, ( meshDef ) => {

        var geometry = /* ... */;
        var material = dependencies.materials[ meshDef.material ];
        var mesh = new THREE.Mesh( geometry, material );
        // ...
        return mesh;

      } );
      // ...
      return meshes;

    } );

  }

  // ...
}

As a result, all accessors (which in turn depend on all of the .bin buffer files) will be loaded before any meshes can be created. This introduces a problem, if not all buffers are actually needed. For example:

  • The (draft) KHR_draco_mesh_compression extension allows an asset to include a Draco .bin file, and an uncompressed .bin, so that that clients not supporting Draco can still fall back on the unoptimized version. In either case, we want to load only one of the buffers.
  • The (draft) MSFT_lod extension has essentially the same problem.
  • It is already valid, although I haven't seen any examples, to declare both metal-rough and spec-gloss materials on the same mesh.The loader can choose which to use, according to performance and feature support, but you certainly don't want to request the textures for both. Currently, this is exactly what GLTF2Loader would do.

So, I think it might be necessary to restructure GLTF2Loader to request dependencies top-down. Proposed syntax (unless there are good existing examples to imitate) would be something like this:

class GLTF2Parser {
  loadMesh ( meshDef ) {

     return Promise.all( [

       this.loadGeometry( meshDef.primitives ),
       this.loadMaterial( meshDef.material )

     ] ).then( ( dependencies ) => {
     
       var [geometry, material] = dependencies;
       var mesh new THREE.Mesh( geometry, material );
       // ...
       return mesh;

     } );

  }

  // ...
}

^Using ES6 for conciseness, although presumably that can't actually be checked into examples/js.

Before I embark on a journey of refactoring, any strong preferences on how this should work? I'm hoping, but will need to verify, that this change could happen incrementally.

/cc @takahirox

@takahirox
Copy link
Collaborator

I agree with refactoring.
IMO the combination of _withDependencies and _each is a bit complex.

BTW, @mrdoob do you allow us to use Promise and ES6?

@mrdoob
Copy link
Owner

mrdoob commented Aug 11, 2017

I guess it's ok to ditch Internet Explorer in order to get manageable code for loading gltf 👌

So Promises are cool. ES6... depends of the feature and browser compatibility.

@donmccurdy
Copy link
Collaborator Author

Thanks @takahirox for the refactoring!

I think this can be closed. Some parts are not strictly top-down — for a model with multiple scenes, we might someday want to not have nodes or accessors retrieved via getMultiDependencies(). But for all example models I've seen so far, the current structure looks much better.

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

No branches or pull requests

3 participants