Additive rotation fix

[blaztinn]

Quaternion multiplication rotates lhs by the rhs, but the current implementation of additive rotations on the blending job had the operands swapped.

Side note: When creating an additive animation the operands are in the right order https://github.com/guillaumeblanc/ozz-animation/blob/master/src/animation/offline/additive_animation_builder.cc#L66

[guillaumeblanc]

Hi,

thanks for submitting the PR!

I see the swap indeed. I tried the change and I can see on the additive sample that the resulting blend is not looking like what I expect visually.

How did you found that issue? How did you validate it gives the expected blend result?

Cheers,
Guillaume

[blaztinn]

Hi,

thx for taking a look! :)

I fixed the PR by swapping the arguments in the additive animation builder also. This now produces the desired results on the additive sample, but with a little different result than without this PR (it is the most visible when both splay and curl animations have weights of 1)

---

I found the issue when replacing the Unity animation system with Ozz animation. I was comparing blending+additive results from both systems and when using blending together with additive animations the systems produced different results. After swapping the operand order in Ozz, they produced the same results. (I used ozz in Unity by applying results of blending directly to Unity Transforms, that is using translation, rotation and scale directly, without 4x4 matrix).

---

I've modified the additive sample to easier see how swapping the operands affects the animation in this commit.
I overwrote (in code) the root joint of rest pose (Root (X) slider), splay and curl animations to do 90 degrees rotation in X, Y and Z directions, respectively.

Set the weights of all to 0 (I added a slider for rest pose above the splay/curl 2D slider) and then you can check for each animation how it affects the model. The model is not standing up when root joint rest pose is identity, so it is a little difficult to deduce rotation around which axis happened :(

But still you can see that with this PR the animations are applied (in local space) in order:

rest_pose -> splay (additive[0]) -> curl (additive[1])

while before it was in order:

curl (additive[1]) -> splay (additive[0]) -> rest_pose

resulting in way different results.

(Also, it seems like when only looking at the order rest_pose -> splay (additive[0]) -> curl (additive[1]) this PR applies them in local space while previously they were applied in global space. But I haven't thoroughly checked that.)

I think that additive rotations should be applied in local space to the previous steps (previous additive animation or blend step output) so this is why I believe this PR fixes the additive rotations order.

You can see this when having all weights at 0 and then first moving the slider for the rest pose to 1 (rotation around X) and then the splay animation weight to 1 (rotation around Y). With this PR the splay (rotation around Y) moves the model in the same local direction (head to feet) independently of the rest pose weight (rotation around X). While without the PR splay always rotates in global space, that is it rotates around the global Y independently of the rotation around X with the rest pose.

It could be that the Unity animation system does it wrong and Ozz animation system right. Or that the order doesn't even make sense and is only a convention used by the system. I did the change because I needed to have the same behaviour as Unity and pushed the PR here in case it fixes things. :)

[jankrassnigg]

I added support for additive blending in my engine a while ago, but was unable to get results that "made sense" to me. Though since I did not have any good animation data for additive things, anyway, I just ignored it, assuming my data is just not right. So I'm actually quite happy to see that it might be a bug in the code :D

[jankrassnigg]

Btw I am now 100% certain that this fix is necessary. All the data that I tried it with gives good results with the fix, and is completely broken without it.

I think there is one way to verify it visually: If you play ONLY an additive animation on a skeleton in rest pose, then there should be no visual difference to playing the same animation as a non-additive animation. Because it only adds the difference to the first frame and nothing else and thus ends up at the same final result.

With this fix, that is indeed the case. Without it, the skeleton mostly collapses in on itself.

If it helps, I can record a video of the different results, let me know.

[guillaumeblanc]

Hi,

Thanks @blaztinn and @jankrassnigg for the detailed explanations and experiments. I'm investigating this on my side too.

I'll get back to you soon.

Thank you very much.
Guillaume

[guillaumeblanc]

I merged the change, fixed a unit test and updated binary data.

By the way, I added your names to the authors list. Don't hesitate if you want to provide a name @blaztinn .

Thanks a lot for the PR!

Guillaume

[blaztinn]

By the way, I added your names to the authors list. Don't hesitate if you want to provide a name @blaztinn .

Thank you, it's Blaž Tomažič (blaz.tomazic@gmail.com)

Thanks a lot for the PR!

Thank you for reviewing and also for developing this software in the open.

Looking back at the comments I made on this issue, I see I wasn't particularly clear with my explanation :D. Luckily @jankrassnigg provided a way better one.

Blaz

[guillaumeblanc]

Can you understand what's happening with this build https://github.com/guillaumeblanc/ozz-animation/actions/runs/7963487480/job/21739641466 ? Or reproduce the issue ?

Cheers,
Guillaume

[blaztinn]

Since tests built with gcc don't fail and only clang 15 release build fails (clang 15 debug doesn't), I would guess that some compiler optimization affects the test.

I can also reproduce it locally when building with clang 17. And only for release builds, not debug.

If I'll find more time I'll try to find out which operation works differently on release vs debug builds.

[guillaumeblanc]

Hi,

I changed the implementation of the blending job tests to integrate this use case to the existing additive blending tests.
The cland15 issue doesn't show up anymore.
I'd be interested in understanding if there's a real issue behind it if you manage to isolate it.

Cheers,
Guillaume

[blaztinn]

Managed to find out why the runtime behavior differs. The clang compiler does floating point contraction (-ffp-contract) by default on non-debug builds, while the gcc compiler does not. This can result in compiler using FMA (fused multiply-add) for floating point operations which can produce different results, thus failing the test.

(It depends on the program if the floating point contraction has more or less precision. https://en.cppreference.com/w/cpp/preprocessor/impl -> search #pragma STDC FP_CONTRACT)

Because of contraction the test function produced different results for quaternion multiplication. I managed to extract a small example from it, showing the different behavior by only multiplying and subtracting a __m128: https://godbolt.org/z/7Pc34YTcE . The output of the program is in hex, because float formatting sometimes rounds the result.

In the example, if you change the clang flags from -O3 to -O3 -ffp-contract=off, you can see that the output then matches the one from gcc.

Also, using the -DCMAKE_CXX_FLAGS="-ffp-contract=off" for a local build made the test pass on clang release build.

I'm really not sure if the lib should change how it is compiled (-ffp-contract on or off), the optimizations should make it run faster. Most probably I just wrote a bad test that is not numerically stable.

---

If it helps, I've printed the output_transform[0] from the test with contraction on:

output_transforms[0].rotation.x -0.707107, 0.707107, 0.270598, 0.270598
output_transforms[0].rotation.y 0.000000, 0.000000, -0.653281, 0.653281
output_transforms[0].rotation.z 0.000000, 0.000000, -0.270598, 0.270598
output_transforms[0].rotation.w 0.707107, 0.707107, 0.653281, -0.653281

and without it:

output_transforms[0].rotation.x -0.707107, 0.707107, 0.270598, 0.270598
output_transforms[0].rotation.y 0.000000, 0.000000, 0.653281, 0.653282
output_transforms[0].rotation.z 0.000000, 0.000000, 0.270598, 0.270598
output_transforms[0].rotation.w 0.707107, 0.707107, 0.653281, -0.653282

Some fields got negated. I don't know enough about quaternions to know if this represents the same rotations though :D .
I would guess that the sign change came from contracted instructions producing a -0.0000000001 where a 0 would be otherwise. And later the sign of this value was used. Just guessing here though :)