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: Fix node parse #25377

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Jan 30, 2023

Fixed #25332.

Description

This commit fixes a GLTFLoader node parse bug that can cause an infinite loop in node parse - skin parse - node parse.

The solution is adding ._loadNode() that parses a single node without child nodes or skin to the parser. .loadNode() and .loadSkin() call ._loadNode() to get dependent nodes. ._loadNode() doesn't have dependency with other nodes so infinite loop can be avoided.

._loadNode() caches created nodes to avoid duplication because it is called from two different methods, .loadNode() and .loadSkin().

/cc @donmccurdy

This commit fixes a GLTFLoader node parse bug that
can cause an infinite loop in node parse - skin
parse - node parse.

The solution is adding ._loadNode() that parses a
single node without child nodes or skin to the parser.
.loadNode() and .loadSkin() call ._loadNode() to
get dependent nodes. ._loadNode() doesn't have
dependency with other nodes so infinite loop can
be avoided.

._loadNode() caches created nodes to avoid duplication
because it is called from two different methods,
.loadNode() and .loadSkin().
@takahirox takahirox force-pushed the FixGLTFLoaderNodeParse branch from 5545dba to 2ba9aac Compare January 30, 2023 21:03
@takahirox takahirox requested a review from donmccurdy January 31, 2023 04:45
@mrdoob mrdoob added this to the r150 milestone Jan 31, 2023
examples/jsm/loaders/GLTFLoader.js Show resolved Hide resolved
examples/jsm/loaders/GLTFLoader.js Outdated Show resolved Hide resolved
examples/jsm/loaders/GLTFLoader.js Show resolved Hide resolved
This commit renames _loadNode to _loadNodeShallow.

The behavior of the method is more clarified by
the new name so remove a commend that sounds like
duplicated with the method name (and also sounds
like a bit misleading).
@takahirox takahirox force-pushed the FixGLTFLoaderNodeParse branch from 5fc253a to 1cf338e Compare February 1, 2023 05:30
@mrdoob mrdoob merged commit 68d4bd9 into mrdoob:dev Feb 2, 2023
@takahirox takahirox deleted the FixGLTFLoaderNodeParse branch February 2, 2023 03:14
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.

GLTFLoader: Unable to load some SkinnedMesh objects in r148
3 participants