-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Conversation
Sorry I know you seem busy but friendly ping @donmccurdy |
There was a problem hiding this 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!
examples/js/loaders/GLTFLoader.js
Outdated
} | ||
|
||
return node; | ||
return ext.createNodeMesh && ext.createNodeMesh( nodeIndex ); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
Probably better to move it to next cycle so we have time to test. |
It seems this PR had conflicts in |
We have no other option than trusting the script... 😅 |
Thanks! |
Background
We added invokeAll
createNodeAttachment
hook inGLTFLoader
#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 nonKHR_
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 toloadMaterial
or others. But coreloadNode()
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 usingcreateNodeAttachment
like light extension. I think it's good for most of the cases.