guillaumeblanc / ozz-animation

Open source c++ skeletal animation library and toolset
http://guillaumeblanc.github.io/ozz-animation/
Other
2.46k stars 302 forks source link

Mostly working glTF importer. Help needed. #67

Closed AlexanderDzhoganov closed 5 years ago

AlexanderDzhoganov commented 5 years ago

This is a mostly spec-compliant importer from glTF 2.0 to Ozz. It subclasses OzzImporter and can be used in the same way as the FBX one. It depends on tinygltf for parsing.

I have successfully implemented a glTF->Ozz pipeline in my custom renderer. There are however some issues that need to be solved before it is fully usable. I'll explain below.

I have done all my tests by exporting from Blender 2.80 (commit 157cc54). There may be issues with other exporters that I have not encountered yet. Also this was my first time implementing skeletal animation in a renderer so I might have some fundamental misunderstanding of either Ozz or the glTF spec which led to these issues.

  1. It looks like Ozz disregards the default transformation passed to every joint (or I've made a mistake when creating the transformations) because if every empty JointTrack doesn't get padded with the default joint transformation then after animation all joints without keys remain at 0, 0, 0. The conversion from a glTF node's transform to an Ozz joint transform is done via the CreateTransform method. The current workaround is to pad every empty track with the default transform of the node by using the GetDefaultKeyValue methods. This seems to work as the meshes animate correctly in my renderer but is suboptimal and hints there's an issue somewhere.

  2. The produced Ozz skeleton does not render correctly in the playback sample. For example this Blender armature renders like this in the playback sample after export. I haven't looked into the sample framework renderer so this could just be a difference in bone rendering behavior between it and Blender or it could be a result of (1). This issue does not seem to affect in-engine skinning but also hints to a problem somewhere.

I hope somebody could shed some light on these two issues as I've been unable to figure out what's wrong after hours of testing.

Other limitations:

Things to improve:

guillaumeblanc commented 5 years ago

Hi Alexander,

Thank you very much for your work and sending a PR. gltf support is going to be a great addition to ozz project! I could only have quick look to the code. I can see that you have a good understanding off ozz, there's no big issue or misunderstanding with the API or skeleton animation fundamentals. I can't really say gltf that I don't know.

I'll try to answer your questions and give some feedback about what I noticed:

  1. "It looks like Ozz disregards the default transformation passed to every joint" Ozz runtime animation builder will push identity keys for empty tracks. So there should be no need to do that on your side. Could you try to remove that code and see what happens in animation builder? On the other hand, it's possible that a node exist in the skeleton, but isn't animated (like a fixed default transformation). In the case, fbx importer will push the skeleton bind pose for the joint. I think that's something missing from your implementation, as it's asserted.

  2. "The produced Ozz skeleton does not render correctly in the playback sample" Ozz sample framework skeleton renderer displays a leaf joint as a "gizmo", not a joint, because the length of this bone is unknow. I think DCC renders them as bones, allowing animators to set leaf joint length. I'm not sure gltf would carry that information, as it's an editor feature only, it has no use at runtime. Just to be sure, you could double check that the number of joints is the correct one.

  3. Morphing is not supported This isn't supported by ozz either, as it's not skeletal based.

  4. Cubic spline interpolation is not supported The solution would be to sample the spline in the gltf importer. That's ozz fbx importer strategy. Sample rate is provided to the function. By the way "Step" is not supported either by your implementation I think.

  5. "Currently the importer can handle only one (fully-connected) skeleton per glTF" That's a limitation fbx importer has as well, importing all nodes that match with OzzImporter::NodeType to a single skeleton. I think it's fine. By the way, does gltf have different node types?

  6. "Animation duration is guessed from the key with the highest time." It's probably ok. Can you point me to gtlf spec about that? I couldn't find it.

  7. "Animation starts (and ends?) might need to be padded with copies of the first existing key" You don't need to pad key at the begining or end of the animation. Ozz runtime animation builder is handling that.

  8. I'm wondering what's happening if buffers component types are not floats, like when reading translation, rotation or scales from buffers? Is tinygltf handling the conversion, or is this needed from the importer?

  9. Please use log::Err only for errors that can't be recovered, usualy when, function return false. "Warnings" should rather be Log or LogV I think.

  10. You choosed to us tinygltf. It needs c++11 so something needs to be done to activate c++11 or disable gltf importer when c++11 isn't supported. Please look at fbx importer on develop branch for an example.

  11. CI dashboards are failing on windows and linux. I think c++11 support is the first issue, but there seems to be issues with cmake script also.

  12. The PR is for master branch, it should be for develop branch. I can probably change it when merging, but it's easier if it's targeting the right branch from the beginning, especially something changed on developed and requires more merge.

  13. You use a lot of auto, even for known and simple types. That's something clang-format won't be able to change alone ;).

  14. You choose tinygltf, which seems easy to use. What's the motivation for choosing it? Have you considered other options?

  15. Do you know if gltf supports user channels? It's like an animation channel on a node, that isn't specifically a transformation.

16.That'd be great add some tests, import a few different gltf files (from different sources), and load them in a sample, like this for fbx.

Thanks again for your work and thoroughness in implementing and testing this importer. I'm looking forward to continuing this discussion and see the progress. I can jump in at some point to help, or maybe add more tests.

Cheers, Guillaume

AlexanderDzhoganov commented 5 years ago

Thank you for the response @guillaumeblanc and for making this wonderful library. I will go through all the things you noted and push some new commits probably over the weekend and into next week. Apologies for the failing tests and lack of comments in the code, I'll get to that after the core issues are dealt with.

I have managed to get some more testing done, this time with more complex armatures. Here's the same animation in Blender and the Ozz player. Blender - https://i.imgur.com/FUnLTHz.gifv Ozz - https://i.imgur.com/yXpm31S.gifv

guillaumeblanc commented 5 years ago

No worries, no hurries, you can take time to iterate and I'm happy to help. You showed very good progress already!

AlexanderDzhoganov commented 5 years ago

Closing this PR. Will make a new one against the develop branch.