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: Add createNodeMesh hook #21458

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Conversation

takahirox
Copy link
Collaborator

Background

We added invokeAll createNodeAttachment hook in GLTFLoader #19892. It's a good hook for adding extra objects to a node object, like light.

But as I wrote in #19892 (comment) this hook can't be used for suppressing core spec meshes (and cameras). Some extensions want to suppress loading core spec objects (and instead want to load objects defined in the extensions). eg. materials unlit extension

Actually I needed this capability in node as I was writing MSFT_lod extension plugin. So I would like to add a new hook point for node. I know non KHR_ extensions are not really our glTF loader extensibility mechanism targets, but it would be good if we can add a new hook point without messing the core code.

InvokeOne loadNode() hook may be one of the options similar to loadMaterial or others. But core loadNode() does a lot common works for example make a group for multiple objects and setting transform. I think extensions need the same ones.

Suggestion

So I would like to suggest to extract the part creating node meshes and add an invokeOne hook for it, named createNodeMesh hook. The common works for node still be usable for the extensions using the new hook. And the extensions using the new hook doesn't have an effect to the extensions using createNodeAttachment like light extension. I think it's good for most of the cases.

@mrdoob mrdoob added this to the r127 milestone Mar 13, 2021
@mrdoob mrdoob requested a review from donmccurdy March 13, 2021 09:13
@takahirox
Copy link
Collaborator Author

Sorry I know you seem busy but friendly ping @donmccurdy

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

}

return node;
return ext.createNodeMesh && ext.createNodeMesh( nodeIndex );

Copy link
Collaborator Author

@takahirox takahirox Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I found a problem in this implementation after the review has been done. Plugins' createNodeMesh() should be called even if core nodeDef.mesh is undefined. e.g. GPU instancing extension mesh without fallback in core. I would update soon.

Nit: EXT_mesh_gpu_instancing doesn't define a behavior for a node with the extension that doesn't also have a mesh so the example above may not be good, anyways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@mrdoob
Copy link
Owner

mrdoob commented Mar 30, 2021

Probably better to move it to next cycle so we have time to test.

@mrdoob mrdoob modified the milestones: r127, r128 Mar 30, 2021
@takahirox
Copy link
Collaborator Author

It seems this PR had conflicts in examples/js/loaders/GLTFLoader.js made by #21588. But examples/jsm/loaders/GLTFLoader.js had no conflicts so I ran npm run build-examples and now no conflicts but there are lot of diffs in examples/js/loaders/GLTFLoader.js. I haven't looked into the examples builder script but do the diffs look ok? I think if I trust the script, it should be fine.

@mrdoob
Copy link
Owner

mrdoob commented Apr 8, 2021

We have no other option than trusting the script... 😅

@mrdoob mrdoob merged commit 5be89c0 into mrdoob:dev Apr 8, 2021
@mrdoob
Copy link
Owner

mrdoob commented Apr 8, 2021

Thanks!

@takahirox takahirox deleted the GLTFLoaderNodeHook branch April 8, 2021 16:56
@joshuaellis joshuaellis mentioned this pull request Apr 15, 2021
6 tasks
@joshuaellis joshuaellis mentioned this pull request May 8, 2021
11 tasks
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