FBX: Regression importing animations

jankrassnigg commented 3 years ago

Describe the bug

I just switched from release 5.0.1 to master (9ce0671134912fd49010d81b89ae56dc41029c39). An FBX file with skeletal animations that I use for testing was working perfectly fine before but now doesn't anymore.

I don't think I can share the model here, but if you need it to reproduce the issue, just ping me directly.

With both versions I can import the mesh just fine:

good_mesh

In the old version, the node hierarchy has considerably more nodes, though. I assume that the new code merges them together, and I wouldn't be surprised if this change broke the animations.

Old skeleton (working):

good_skeleton

New skeleton (broken):

bad_skeleton

Finally, here is a GIF showing an animation. The green lines visualize the bind pose, which looks correct. The pink lines are the animated pose:

broken fbx anim

Most of it looks good, it's not complete garbage, but every couple of joints appear to be messed up. That's what leads me to the assumption, that the optimization that was done to reduce the overall node count is missing a step to also adjust the animation matrices.

PS: I upgraded to latest assimp because I encountered several issues with GLB files, which have been fixed. So all THOSE files now work perfectly fine. Of course I can't rule out an issue in my own code, but it feels unlikely, considering that it was working with the release 5.0.1.

Frooxius commented 3 years ago

I can confirm that this happens as well. Was previously using an older version (roughly a few dozen commits after 5.0.1 if I remember correctly), updated to latest master: https://github.com/assimp/assimp/commit/294d4230b04636947060c4b611742fad01e9bd47

I've got videos showing the breakage on two models (first it imports with the old version of Assimp and then I restart my app to import with the new): https://youtu.be/BZG3txkwVq0 https://youtu.be/4GzXt6tA6Vw

I can't share the models publicly either, but I can send them to you if you need them for testing.

However I fortunately managed to isolate the breaking change to this commit: https://github.com/assimp/assimp/commit/14b8d1242b0f32fef9dd8ace0a1d4e8cbd20d88c

Particularly changing FBXConverter.cpp line 658 from:

if (comp == TransformationComp_Rotation || comp == TransformationComp_Scaling || comp == TransformationComp_Translation) {
            continue;
        }

To:

if (comp == TransformationComp_Rotation || comp == TransformationComp_Scaling || comp == TransformationComp_Translation ||
            comp == TransformationComp_PreRotation || comp == TransformationComp_PostRotation) {
            continue;
        }

I have reverted this particular change on the latest master on my fork here: https://github.com/Frooxius/assimp/commit/b6b307025a7ee19c056130f8d4c0fb61e8efb1ab

I have confirmed that this fixes the issue on my end, but I'm not sure if it breaks anything else or just generates unnecessary transformation nodes. My guess is that when those nodes are collapsed, the animation transforms aren't recomputed properly, but I don't know enough about FBX internals to do a proper fix. I hope that this info helps for anyone who does though and at least provides a workaround for now.

Here's video of the fix if necessary: https://www.youtube.com/watch?v=W4gt15RBzn4

cpope9141 commented 2 years ago

I believe I am still seeing this issue in v5.1.5.

cpope9141 commented 2 years ago

I believe this issue is being reported again in #4487.

lucasjinreal commented 2 years ago

I believe FBX is nightmare.

mwestphal commented 5 months ago

Any news on this one ? It is still present here.

assimp / assimp

FBX: Regression importing animations #3390