The Wayback Machine - https://webcf.waybackmachine.org/web/20220402130107/https://github.com/mrdoob/three.js/pull/19892
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

Add node hookpoint to GLTFLoader and move the lights extension handler to the new system #19892

Merged
merged 2 commits into from on Sep 15, 2020

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented on Jul 21, 2020

This PR adds hookpoint for node in GLTFLoader and moves the lights extension handler to the new system.

Background of this PR is #19144 (comment)

In #19144, we use invokeOne() for loadXxx(). But it doesn't fit to Light extension. A node can have mesh, camera, and light extension at a time and all the objects should be created under the node. invokeOne() doesn't fit to it because invokeOne() stops to call the core loadXxx() if extension's loadXxx() creates an object. Then mesh and camera objects will not be created.

A solution idea is adding invokeAll() hookpoint here

GLTFParser.prototype.loadNode = function ( nodeIndex ) {
    ...
    var nodeDef = json.nodes[ nodeIndex ];
    return ( function () {
        var pending = [];
        if ( nodeDef.mesh !== undefined ) {
            pending.push( parser.getDependency( 'mesh', nodeDef.mesh ).then( function ( mesh ) {
                ...
            } ) );
        }
        if ( nodeDef.camera !== undefined ) {
            pending.push( parser.getDependency( 'camera', nodeDef.camera ).then( function ( camera ) {
                ...
            } ) );
        }

        // Add hookpoint here
        parser._invokeAll(ext => ext.createNode && ext.createNode(nodeIndex))
            .forEach(promise => pending.push(promise));

        ...

It is a hookpoint where extension handlers create their special objects in addition to core spec mesh and camera. A good thing is the latter transform and node group initializations can be reused. It very fits to the light extension and @feiss 's text extension.

What do you think of?

Some notes.

  • This new hook point can't be used for extensions which want to suppress creating core spec mesh or camera in a node. If users request a hookpoint which can suppress them, we may need to make another hookpoint (maybe loadNode() hookpoint with invokeOne() as loadMaterial, loadTexture, or others does).
  • I named createNode for the new hookpoint. Better name idea is welcome. (I know I'm bad at naming in English...)
  • I realized that extension handlers which use the new hookpoint for creating their special objects need to manage their NodeRef then markDef() also needs to be extended and the handlers need to call get/addNodeRef().

…r to the new system
@mrdoob mrdoob requested a review from donmccurdy 5 years ago
@donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented on Aug 25, 2020

This looks like the right way to structure it to me, yes! The invokeOne and invokeAll hooks handle two different cases, and this one fits invokeAll nicely.

I named createNode for the new hookpoint. Better name idea is welcome. (I know I'm bad at naming in English...)

Your English is good, naming things is hard. 😅 Maybe createNodeAttachment, to clarify that it's creating something attached to the glTF's definition of a Node?

@mrdoob mrdoob added this to the r121 milestone on Aug 25, 2020
@takahirox
Copy link
Collaborator Author

@takahirox takahirox commented on Aug 29, 2020

Thanks for the review! I have renamed to createNodeAttachment.

@mrdoob mrdoob merged commit 4c75646 into mrdoob:dev on Sep 15, 2020
8 checks passed
@mrdoob
Copy link
Owner

@mrdoob mrdoob commented on Sep 15, 2020

Thanks!

@takahirox takahirox deleted the GLTFLoaderNodeHook branch 5 years ago
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.

None yet

3 participants