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 Animation Importing #43

Closed
wants to merge 33 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Sep 1, 2017

This adds importing GLTF animations to Unity. I tested it with the GLTF Samples and they all seemed to work with the exception of the WalkingLady. (Not sure why that one isn't working, but it seemed a clear exception.)

willswannackpluto and others added 30 commits August 4, 2017 14:20
@stevenvergenz
Copy link
Contributor

stevenvergenz commented Sep 13, 2017

Hi Will, sorry for the long delay in getting back to you. We're very interested in this PR, thanks for submitting it! We're still working on reviewing it as it's a pretty complicated piece of functionality. Plus there's a fairly large reorganization PR coming down the pipe, and we need to figure out if this should be merged before or after.

A question though: Unity animation clips require a "root" node, and all channels point to relative paths from there, but glTF animations have no such concept. How did you choose what GameObject should contain the Animation component? Does it work for glTFs with multiple scenes?

We're working to update our tests with animations to put this PR through its paces now, hopefully we'll have more questions for you once that's done.

Cheers,

EDIT: And a follow-on to the above question, how did you generate deterministic relative paths from the glTF scene? I'm guessing using the node index value?

@donmccurdy
Copy link

The walking lady model is broken (KhronosGroup/glTF-Sample-Models#83), no need to worry about that one.

@ghost
Copy link
Author

ghost commented Sep 14, 2017

@stevenvergenz Will left on vacation for three weeks this afternoon so it may be a little bit before we get you answers to your questions. I'll ask a couple other Pluton's who might have worked with him on this stuff to see if they know and update if I find out anything.

@bjornbytes
Copy link

The Animation component is added to the root node and we generate unique paths from there based on node/primitive index.

@sbtron sbtron added this to In Progress in Road to 1.0 Sep 14, 2017
@stevenvergenz
Copy link
Contributor

@bjornbytes What is the "root node" in your explanation? Is it the scene or the nearest common ancestor of channel targets?

@bjornbytes
Copy link

It's the scene root, we didn't get around to doing to nearest common ancestor approach.

@stevenvergenz
Copy link
Contributor

@willswannackpluto, you back from vacation? Thought you should know that there was a fairly major architecture change pushed a couple weeks back. It would be great if you could update this PR from master, and get things lined up again. That would make it easier to review too. We really appreciate this, animation is fairly high on the priority list, but manpower is always lacking.

@ghost
Copy link
Author

ghost commented Oct 18, 2017

@stevenvergenz Yes I am back! Thanks for the update. I saw the huge architecture change and it looks exciting. We're interested in updating the current PR but aren't yet sure when we'll have time to do it. I will post back when we either know we can't work on it or when we know when we'll start working on it.

@stevenvergenz
Copy link
Contributor

@willswannackpluto I got a notification that you added commits to this PR, but don't see them here. I'm guessing it was accidental, and was reverted?

@ghost
Copy link
Author

ghost commented Jan 25, 2018

@stevenvergenz Yes it was accidental so I reverted them.

@ghost
Copy link
Author

ghost commented Jan 25, 2018

We are at the moment not planning on merging this PR because we don't believe we'll have time to do so for quite a while (a couple months at minimum). I think we should probably close this PR. We can open a new one if we get time and animations still aren't in.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Road to 1.0
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

4 participants