GLTFLoader refactoring #12772
GLTFLoader refactoring #12772
Conversation
|
This looks much cleaner, thanks! ❤️ I’ll run some tests with more models later tonight. |
|
I ran all of the glTF-Sample-Models (well, one file from each set) as well as the rome-gltf samples and a few Sketchfab files. All looks good. Demo: https://gltf-viewer-experimental.donmccurdy.com/ The only weird thing I see is that loading seems slow in some cases: robin_hood_in_sherwood_forest.zip ^When I profile that, it looks like the Blob XHR request is just taking much longer, so maybe it's nothing to do with this PR, or just a fluke on my machine? Also note the experimental demo is on dev three.js, original demo is r88. Do you see the same thing? |
examples/js/loaders/GLTFLoader.js
Outdated
| @@ -1183,9 +1083,9 @@ THREE.GLTFLoader = ( function () { | |||
| * @param {THREE.Mesh} mesh | |||
| * @param {GLTF.Mesh} meshDef | |||
| * @param {GLTF.Primitive} primitiveDef | |||
| * @param {Object} dependencies | |||
| * @param {Array} accessors | |||
There was a problem hiding this comment.
nit: can you make this Array<THREE.BufferAttribute>?
examples/js/loaders/GLTFLoader.js
Outdated
| GLTFParser.prototype.getMultiDependencies = function ( types ) { | ||
|
|
||
| var results = {}; | ||
| var fns = []; |
There was a problem hiding this comment.
maybe name this deps or pending; fns sounds like Functions, but these are Promises.
examples/js/loaders/GLTFLoader.js
Outdated
|
|
||
| } ); | ||
|
|
||
| }; | ||
|
|
||
| /** | ||
| * Specification: https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#meshes | ||
| * @param {number} meshIndex | ||
| * @return {Promise<THREE.(Skinned)Material>} |
There was a problem hiding this comment.
{Promise<THREE.Mesh|THREE.SkinnedMesh>}
|
|
||
| }; | ||
| }(); |
There was a problem hiding this comment.
This is a pretty long with the IIFE here... maybe buildNodeHierarchy should just be a local helper function or a method of GLTFParser?
There was a problem hiding this comment.
For the performance, readability, or anything else?
There was a problem hiding this comment.
I didn't really wanna make it GLTFParser method because it's used only in .loadScene().
And the reason why I used IIFE is .loadScene() can be called many times with glTF including many scenes. I didn't wanna make function for each call.
There was a problem hiding this comment.
Yeah making a function for each call would be worse I agree. I think the alternative is making a helper function, like addMorphTargets that is a local function but not exposed as a method on GLTFParser. Just easier to read with less nesting I think.
There was a problem hiding this comment.
Ah, OK. I had misunderstood a bit. Yeah, helper function is one of the good ideas. Let's make PR for it if/when it's necessary.
|
I've confirmed loading performance issue here... |
|
Seems like something stalls request... 🤔
|
|
Probably I've figured out. This seems a bug. I'll try to fix. |
|
I've fixed the performance issue. I needed |
|
Sweet! |
|
Thanks! |
|
|
||
| } ); | ||
|
|
||
| }; | ||
|
|
||
| /** | ||
| * Specification: https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#meshes | ||
| * @param {number} meshIndex | ||
| * @return {Promise<THREE.Mesh|THREE.SkinnedMesh>} |
There was a problem hiding this comment.
Should the return type be Promise<THREE.Group>?
There was a problem hiding this comment.
Oh, it should actually be:
Promise<THREE.Mesh|THREE.SkinnedMesh|THREE.Group>
in the future we want to reduce the number of cases where a Group is returned (#11944), but probably can't eliminate that entirely.
There was a problem hiding this comment.
Oh, I see you spotted that already 😁
This PR refactors and cleans up
GLTFLoaderand it's much more readable now. It usesGroup.isGourpso requiresbuild/update.Major changes
_each()and._withDependencies()and introducing.getMultiDependencies()instead. They were a bit tricky especially for new developers. By removing them the code became more readable..loadXXXmethods..loadXXX( xxxIndex ), not.loadXXXs(), now (except for.loadGeometries()).SkinnedMeshmesh defs before parsing. We don't need to replaceMeshwithSkinnedMeshlater now.Skeletonbuild to.loadScene().onBeforeRender()set to.loadMesh/Node().*Defvar name for definitions in jsonI'd be very pleased if you folks check if this update won't break anything tho I already tested on
webgl_loader_gltf. And any feedbacks are welcome./cc @donmccurdy