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

ColladaLoader: Use Object3D.animations. #20767

Merged
merged 1 commit into from Nov 26, 2020
Merged

ColladaLoader: Use Object3D.animations. #20767

merged 1 commit into from Nov 26, 2020

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 26, 2020

Related issue: #20738

Description

Since Object3D.animations is now available, I suggest to migrate all loaders to use the new property (FBXLoader already uses this approach). In this way, no separate property in the result object is necessary anymore. Besides, the editor (and potentially other software) can lookup animation clips at a common place.

The change in ColladaLoader is backwards compatible by introducing a getter for the old animations property. This could be removed after a few releases.

If the PR gets merged, I'll change the remaining loaders in the same fashion.

@mrdoob mrdoob added this to the r124 milestone Nov 26, 2020
@mrdoob mrdoob merged commit ba91312 into mrdoob:dev Nov 26, 2020
@mrdoob
Copy link
Owner

mrdoob commented Nov 26, 2020

Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 26, 2020

@donmccurdy glTF supports multiple scenes but after some research I've found KhronosGroup/glTF#1542.

Do you think it's safe to assign the animation clips to gltf.scene.animation? Or should we assign an animation to each scene where it animates a node?

@donmccurdy
Copy link
Collaborator

KhronosGroup/glTF#1542 is really a proposal for glTF 3.0, which is probably still a long way off. I wouldn't read into that at this point. I'd suggest either:

a. return only the current gltf.animations list
b. attach scene.animations containing only animations that affect the given scene, for all scenes in the file

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 28, 2020

Okay, then I think I will give b) a try.

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