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

Update gltf loader #10536

Merged
merged 19 commits into from
Jan 10, 2017
Merged

Update gltf loader #10536

merged 19 commits into from
Jan 10, 2017

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Jan 9, 2017

This PR updates GLTFLoader.
With this change, it can load the simple glTF 1.1 models listed here

https://github.com/javagl/gltfTutorialModels/blob/master/README.md

Please refer to the commit logs for the change.
And I added some notes where perhaps I need to explain a bit.

I was focusing on loading public sample models.
There're lot of works left to perfectly follow the glTF 1.0/1.1 specifications.

@@ -1408,20 +1589,18 @@ THREE.GLTFLoader = ( function () {
if ( node.jointName ) {

_node = new THREE.Bone();
_node.name = node.name !== undefined ? node.name : node.jointName;
Copy link
Collaborator Author

@takahirox takahirox Jan 9, 2017

Choose a reason for hiding this comment

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

For skeletal animation, generally bone should have non-empty name (and unique name in a skeleton).
So I set jointName to bone.name if name in json is undefined.

if ( node.extras ) _node.userData = node.extras;

_node.matrixAutoUpdate = false;

if ( node.matrix !== undefined ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If matrixAutoUpdate is false,
translation/rotation/scale of this node won't be affected to world
when scene.updateMatrixWorld(true) is called.
So I think here should be true(default).

@cx20
Copy link
Contributor

cx20 commented Jan 9, 2017

I confirmed that the glTF 1.1 tutorial model is displayed in the local environment.
I think that there is no problem, but there is one point to worry a little.
The white cube seems to be displayed in VC.gltf of glTF 1.0.
image
Unfortunately, I do not know whether it is a model problem or a converter problem or an implementation problem.
I do not understand, but the following may be helpful.
KhronosGroup/glTF#576

@takahirox
Copy link
Collaborator Author

takahirox commented Jan 9, 2017

I've confirmed the white boxes by applying this change.

I agreed with this comment

KhronosGroup/glTF#576 (comment)

that blend functions (or fragment shader) of input file is wrong.

Other references
KhronosGroup/glTF-Sample-Models#8
KhronosGroup/COLLADA2GLTF#7

Existing GLTFLoader doesn't support blend functions input and always uses NormallBlending.
So that transparency(blending) looked working correctly even with wrong
blend functions input.

@@ -1653,7 +1857,7 @@ THREE.GLTFLoader = ( function () {
lightNode = new THREE.PointLight( color );
break;

case "spot ":
case "spot":
lightNode = new THREE.SpotLight( color );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like the cause of #8983. 🙃

@@ -1370,11 +1550,13 @@ THREE.GLTFLoader = ( function () {
? THREE.QuaternionKeyframeTrack
: THREE.VectorKeyframeTrack;

var targetName = node.name ? node.name : node.uuid;
Copy link
Collaborator

@donmccurdy donmccurdy Jan 10, 2017

Choose a reason for hiding this comment

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

This will, I hope, be unnecessary when #10366 is fixed.

NVM, I'm too optimistic there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As probably you've noticed, here is not for #10366.

The aim is to give the information to AnimationMixer to find the target node from a scene even if node.name is empty (node.name is optional in glTF).

To guarantee to pass the unique specification , always passing uuid could be safer because glTF allows two or more nodes have the same name.

Alternative is to return the mappings of animation root node(=target node) and animation from GLTFLoader, like this

{
    targetNode1: animation1,
    targetNode2: animation2
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I misread. My concern with UUID (mentioned in #10366) is that cloned meshes will be incompatible with the original AnimationClip objects, and there isn't (and really shouldn't be) a mechanism for cloning an AnimationClip. The same concern applies to a direct mapping, because you still can't clone the object easily.

But let's keep what you have here, and deal with #10366 later as you suggest. I think we can find a good solution later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, that makes sense. Cloning makes the situation complex...
Yeah, let's discuss later.

@mrdoob mrdoob merged commit 6db8a5d into mrdoob:dev Jan 10, 2017
@mrdoob
Copy link
Owner

mrdoob commented Jan 10, 2017

Thanks thanks!

cx20 added a commit to cx20/gltf-test that referenced this pull request Jan 10, 2017
cx20 added a commit to cx20/gltf-test that referenced this pull request Jan 10, 2017
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.

4 participants