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

[gltf] Avoid duplicate keyframe track names #11789

Closed
wants to merge 1 commit into from
Closed

Conversation

jamesgk
Copy link
Contributor

@jamesgk jamesgk commented Jul 18, 2017

I found that when node.name is used in a keyframe track name, some track names can be shared between models. This ultimately means that an animation mixer can only animate one of the models at a time.

When node.name is used in a keyframe track name, some track names can
be shared between models. This ultimately means that an animation
mixer can only animate one of the models at a time.
@mrdoob
Copy link
Owner

mrdoob commented Jul 18, 2017

/ping @donmccurdy

@jamesgk
Copy link
Contributor Author

jamesgk commented Jul 18, 2017

Hmm, I just found some relevant discussion at #10536 (comment).
I guess the current behavior is not sufficient for meshes sharing node names, but this patch is not sufficient for cases with cloned meshes.

@donmccurdy
Copy link
Collaborator

Could you share a model illustrating the problem? Are there multiple nodes with the same name? If so, that is a larger issue...

Relying on UUID introduces other issues with cloning and serialization, so I would like to use node.name when it's available.

@donmccurdy
Copy link
Collaborator

This might have been fixed already by #11785, actually.

@jamesgk
Copy link
Contributor Author

jamesgk commented Jul 18, 2017

It doesn't seem to be fixed in the latest GLTF2Loader. To test this you can try loading two of the example monster models from https://threejs.org/examples/#webgl_loader_gltf2 into a scene, I don't think it's possible to animate both at once.

@jamesgk
Copy link
Contributor Author

jamesgk commented Jul 18, 2017

Never mind, I'm mistaken, that model's nodes don't have names so this doesn't apply. I'll try to come up with a model for which this occurs.

@jamesgk
Copy link
Contributor Author

jamesgk commented Jul 18, 2017

Here's a monster model for which multiple loaded instances can't be animated at once:
Monster.gltf.zip

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 18, 2017

So, to make sure I understand, what you are hoping to do is to animate two identical models separately (not synced to a single animation necessarily). And, this doesn't work because the nodes in each model have the same names, so the animations interfere?

If so, you should be able to avoid the whole issue by scoping your animation mixer to the proper object (new THREE.AnimationMixer(mesh) ) and having a second mixer for the second model.

Or have I misunderstood? I will try to set up a demo doing what I assume you are trying to do, but example code would help.

@jamesgk
Copy link
Contributor Author

jamesgk commented Jul 18, 2017

You're exactly right. It seems obvious now to use multiple mixers!

Though, IMO it seems more reasonable that a cloned model won't be animated by its source model's AnimationClips, than that two separately-loaded identical models can't be animated under the same scene-scoped mixer. But I may be misunderstanding the issue and anyways this is subjective so I'm happy to close this.

@donmccurdy
Copy link
Collaborator

Though, IMO it seems more reasonable that a cloned model won't be animated by its source model's mixer...

Certainly reasonable, but because UUIDs change in cloning, the cloned model couldn't be animated at all without also creating new animation clips with the new UUIDs. That is harder than it sounds, because the scene – not the model – owns the animation clips, clips might point to multiple objects and not all objects were cloned, etc etc.

But anyway I think multiple mixers, each with different root objects, should do the trick.

@jamesgk
Copy link
Contributor Author

jamesgk commented Jul 18, 2017

I think multiple mixers, each with different root objects, should do the trick.

It did indeed, thanks.

@jamesgk jamesgk closed this Jul 18, 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.

3 participants