Bug: FBX Importer using wrong Angles in Animations

[Biohazard90]

This change broke my FBX animation importer: 9c37b94

Specifically the changes in FBXConverter.cpp line 3226 - 3279.

I had to remove all of the 4 AI_DEG_TO_RAD that were added in this PR to make animation importing work correctly again.

Maybe this PR has been made to fix the FBX Exporter without validating the functionality of the Importer.

[Biohazard90]

@JulianKnodt FYI

[JulianKnodt]

Are you importing from FBX to FBX? @Biohazard90 it seems like this is a case of "it works on my machine"
I was thinking that maybe it would be good to instead have a struct which is just a wrapper around float to make it easier to tell when to convert deg to rad.

Do all uses cause it or just some?

I'm doing the full import from FBX --> export to FBX (multiple times per week) so I have been verifying that it works

[Biohazard90]

I'm importing FBX files from the Unity asset store and some I exported from Blender myself after applying changes. I can't share the files directly for license reasons, but these ones are affected for example: https://assetstore.unity.com/packages/3d/characters/animals/dogs-cartoon-5-characters-189881

I'm not using the FBX Exporter from assimp at all so far.

Is it up to the client whether "Lcl Rotation" should be interpreted as radians or degrees or is it standardized? If it's variable, then I assume there should be some other property or flag in the file that describes how to interpret these euler angles?

[JulianKnodt]

I can dig more into it later this week, but are you re-importing these assets into unity or blender? I have been verifying correctness in Blender.

If I remember correctly Lcl Rotation is defined entirely from a matrix, and isn't just a raw number (but I need to double check). The problem with FBX is that there is no public spec, so understanding any feature basically means reading through Assimp, which itself contains some bugs. Other impl's such as Blenders are GPL, so I can't read them.

There's probably some combination of Deg->Rad & Rad->Deg that preserves correctness of both animation and export, but it may take some time to find.

[Biohazard90]

I'm not exporting them back, I'm using them in my own engine after import through assimp, so the assimp FBX Exporter is not involved in my case. I'm getting the matrices from aiNodeAnim::mRotationKeys[].mValue.GetMatrix() and those seem to be wrong for me with these new changes.

If they're stored as a matrix in the FBX format then at least this would mean that the confusion isn't happening due to interpretation of the FBX file I suppose.

[JulianKnodt]

👌 I'll dig into it later this week

Just to clarify, the only value you ultimately care about is mRotationKeys[].mValue.GetMatrix?

[Biohazard90]

FBXConverter::GetRotationMatrix is also converting deg 2 rad and this is being called when creating the quaternions that I'm using from InterpolateKeys(). So I think this may be where it's being converted twice then (for the default rotation value anyway)?

"Just to clarify, the only value you ultimately care about is mRotationKeys[].mValue.GetMatrix?"

Yep.

[JulianKnodt]

I feel like then a possible fix would just be to not call deg2rad in get rotation matrix?

[Biohazard90]

I believe it's still needed to convert actual key frames there, only default rotations may have the issue of being converted to radians twice.

I could reintroduce the issue to my build tomorrow and take screenshots for comparison. It's possible that only bones with default key frames were affected.

[JulianKnodt]

that would be great, I'll also double check tmrw when I get to my machine

[Biohazard90]

I compared the results here, you can see that the animation is mostly intact but it just seems orientated incorrectly. It looks like only the root bone is wrong, but it's at least two bones. In motion you can see ice skating on the limbs, so there must be more incorrect bones than just the root bone.

So it seems to be limited to certain bones, that would match my guess that it only affects the default value, since deg 2 rad has only been added there. All bones with keyframes seem to be fine and unaffected.

[JulianKnodt]

After taking a look today, I found this which contains an example of an FBX node:
https://www.daz3d.com/forums/discussion/511171/fbx-limb-rotation-values
That more convinces me that some of the AI_DEG_TO_RADs are needed

I'm not sure where you're getting that it only affects the default (which line exactly do you mean?)

Is it possible you can try just removing the AI_DEG_TO_RAD such that these match:

i.e. only the first branch has it but the other does not?

[Biohazard90]

Sure, I added the 4 instances of deg 2 rad back that I mentioned, then removed each one separately and checked for the issue again. The one you suggested me to look at did not change anything, the only one giving me the issue is the pre-rotation one: 9c37b94#diff-ec843a56028b1254b7b00fd43976f593892ee029095658a2fa2c612986350cbeR3268

However, I'm still thinking that all 4 of them are wrong. It seems in all 4 cases the values are being converted from degrees to radians twice.

All 4 cases end up calling GetRotationMatrix, which already does deg 2 rad on the same value. I haven't set a break point and debugged, I'm just doing a static analysis, but I'm pretty sure that's what's happening in all 4 instances. Do you see this as well, or do you disagree? (I'm using a compiler that gets called indirectly from my app so debugging is a bit annoying, but I can set it up and get a callstack if still needed.)

[JulianKnodt]

hmmmm alright, you right, I think they are all unnecessary
it's strange to me that having them all on my end or not doesn't affect the output, but I'm also not explicitly consuming the matrix, I'm taking the quaternion, I'll submit a PR later to remove all 4 (or if you'd like to feel free)

[JulianKnodt]

alright so I've actually had a case come up where this DEG_TO_RAD was required

I wonder if there's some versions where it's radians and some where it's degrees

In the model I'm using there's this attribute:

I'm not sure if it's actually related to whether or not it's in degrees, but I suspect it is

[Biohazard90]

Is it possible that your file has this value encoded as degrees twice? How does it behave in other tools like Blender when importing?

Yeah, I think it would make sense if some property defines how rotations are stored. I don't think any of my files use this particular property to debug it, but to me it looks like it would still do the conversion twice, the second time in GetRotationMatrix() (from EulerToQuaternion()), and I feel like that would more likely be an error with the file vs an intentional choice.

[JulianKnodt]

nope, the file is not exported from assimp, so unless some other random external tools encode degrees twice
The original looks fine in Blender when loaded.

I guess what's actually happening is that somehow the rotation shouldn't be loaded at all, and just converting it twice (multiplying by 1e-4 essentially) sets it to 0.

[Biohazard90]

Yeah, that makes sense. Unfortunately I have no idea what would decide that. Would it be possible to compare the behavior with Blender more to figure something out?

Never looked at the Blender source before and I only have an older version installed, but is the code parsing fbx files there this python script perhaps? https://github.com/sobotka/blender-addons/blob/master/io_scene_fbx/import_fbx.py#L483 I don't see a toggle for prerotation yet, seems to be enabled by default.

"RotationActive" seems to be missing from FBXConverter.cpp in assimp, is that maybe what's making it work in Blender?

[JulianKnodt]

I checked RotationActive on import, and they are active for my case. I'm not sure when that flag would ever be used though, seems kind of strange to have a flag for just toggling rotations.

I've also avoided the Blender FBX script because of its GPL license, I'm not sure the exact procedure around it, and I don't want to be breaking any rules on behalf of someone else's repo.

After more digging what I found to be the problem is this:

I just commented out this section and then it worked.

I was futzing around with the FBX exporter recently and found that Assimp doesn't handle quaternion exporting correctly due to ambiguity of conversion to Euler angles. Turns out there's also a few cases where it doesn't handle importing it correctly either, I think near 0 it flipped the quaternion and it was facing the wrong way.

[JulianKnodt]

For your PR, I think it was a spurious failure or caused by something else, so it might be good to force push after a rebase?

[Biohazard90]

The failure was unrelated yeah, I think @kimkulling needs to merge it now that the checks passed.