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: Clean up node hierarchy build #25058

Merged
merged 1 commit into from
Dec 3, 2022

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Dec 2, 2022

This is the third PR of the GLTFLoader clean up PR series following #25033 #25007

Description

Add child nodes in loadNode(). So when loadNode() is resolved, the descendant nodes are ready and already added. I think it's more intuitive. And we can remove buildNodeHierarchy() with this change.

Additional context

When I tried the similar clean up last time, @donmccurdy has raised a concern #15587 (comment)

If the scene graph looks like this...

scene
  └──mesh1
      └── mesh2
           └── mesh3

... and each mesh has its own textures, are we going to request those textures serially? It looks like the child meshes will not begin loading their textures until the parent mesh has finished. Similar issue if each mesh referenced a different .bin file, but that's uncommon.

If I'm right, this problem has existed even since before the change in this PR.

Perhaps the problem can be resolved by creating child nodes in parallel to creating node objects

const pending = [];
const meshPromise = parser._invokeOne( function ( ext ) {
    return ext.createNodeMesh && ext.createNodeMesh( nodeIndex );
} );
if ( meshPromise ) {
    pending.push( meshPromise );
}

if ( nodeDef.camera !== undefined ) {
    pending.push( parser.getDependency( 'camera', nodeDef.camera ).then( function ( camera ) {
        return parser._getNodeRef( parser.cameraCache, nodeDef.camera, camera );
    } ) );
}

parser._invokeAll( function ( ext ) {
    return ext.createNodeAttachment && ext.createNodeAttachment( nodeIndex );
} ).forEach( function ( promise ) {
    pending.push( promise );
} );

// Add this
const childPending = [];
for (let i = 0; i < nodeDef.children.length; i++) {
    childPending.push(parser.getDependency('node', nodeDef.children[i]));
}
//

return Promise.all(
    Promise.all(pending),
    Promise.all(childPending)
);

I want to make another PR for it if needed.

@takahirox takahirox force-pushed the GLTFLoaderCleanupBuildHierarchy branch from 680c12a to 512a218 Compare December 2, 2022 03:01
@donmccurdy
Copy link
Collaborator

If I'm right, this problem has existed even since before the change in this PR.

Hm, I see what you mean. Yes, this PR doesn't appear to change anything there.

Perhaps the problem can be resolved by creating child nodes in parallel to creating node objects...

We do need to be careful not to change the order in which meshes/cameras/lights/children get appended to the node (see: gltfjsx). As long as we can do that I'm good with the additional PR, or also OK to skip since this hasn't been a complaint from users.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 2, 2022

Just for clarification: Does this PR fix #18774?

@Mugen87 Mugen87 added this to the r148 milestone Dec 2, 2022
@takahirox
Copy link
Collaborator Author

takahirox commented Dec 2, 2022

Regarding the node order, yes this PR fixes #18774. The change in this PR makes the Three.js node.children order match the order in glTF node.children. Currently Three.js node.children order is unconsistent. Depending on the runtime.

And the additional change I mentioned in the PR comment doesn't have an effect to the Three.js node.children order.

@takahirox
Copy link
Collaborator Author

Made a PR #25079 for the additional change.

takahirox added a commit to Hubs-Foundation/three.js that referenced this pull request Dec 7, 2022
[HUBS]
This change is needed for glTF LOD support.
This commit will be in r148. We can remove this commit when
upgrading our Three.js fork to r148 or newer.
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

Successfully merging this pull request may close these issues.

3 participants