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

cubic_slerp() may take a rotation path that is not the shortest #57951

Closed
TokageItLab opened this issue Feb 11, 2022 · 13 comments · Fixed by #63380
Closed

cubic_slerp() may take a rotation path that is not the shortest #57951

TokageItLab opened this issue Feb 11, 2022 · 13 comments · Fixed by #63380

Comments

@TokageItLab
Copy link
Member

TokageItLab commented Feb 11, 2022

Godot version

4.0.dev, 3.4

System information

Any

Issue description

#40592 split and resend. There is a problem with the implementation of cubic_slerp(), it may mix non-shortest paths in the calculation.

Steps to reproduce

broken_cubic_slerp.mp4

Minimal reproduction project

cubic_slerp_test.zip

@owstetra
Copy link

owstetra commented Feb 11, 2022

Edit : Sorry wrong Issue, i was talking about another issue and got confused when i was opened multiple tabs in my browser
again, Sorry

@TokageItLab
Copy link
Member Author

@owstetra I don't know what you are meaning. Quaternions do not normally use lerp. Are you confusing it with #57952? The reason this issue was resent was to avoid confusion with that issue from #40592.

Can you send a sample project?

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 12, 2022

@GNSS-Stylist TokageItLab@5cd3792

There is a problem that the way to interpolate each element of the quaternion directly is spherical-interpolation, not spherical-linear-interpolation. So I tried to adapt it to exponential map, which is probably the most common solution. Can you test it?

I think it does make the behavior more consistent. However, cubic-slerp flips the current quaternion when picking the shortest path and it does not match the pre and post sign, which causes strange kickbacks sometimes. I am trying to seek a way to improve it.

Another problem is that the exponential map is an approximate expression, so it does not pass unit tests. For normal slerp() that is not cubic_slerp(), it may have to keep the old method.


If I cannot find a way to resolve the kickback, it may look smoother to interpolate each element directly. Then, maybe we should avoid combining slerp() and cubic_interpolation() and we should choose to interpolate each element directly as you said, and rename the method Quaternion::cubic_slerp() to Quaternion::cubic_interpolate().

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 13, 2022

I think it does make the behavior more consistent. However, cubic-slerp flips the current quaternion when picking the shortest path and it does not match the pre and post sign, which causes strange kickbacks sometimes. I am trying to seek a way to improve it.

Probably, flips will be done correctly by the reference literature sent by @fire. See #58043. Pending, need to more test.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 13, 2022

Repost from #58043:

For example, if we have [a, -b, c, -d, -e], and the first interpolationA is [a, Q, -b, c], and the next interpolationB is [b, Q, c, -d], then, the sign of the pre value of interpolationB will change (because there is b in interpolationB, but it was -b in interpolationA), and the interpolation-curve will be broken.

So, even if we use exp map, cubic_slerp() requires one of the following two things for sign consistency between pre and post.

  • Determine and store the sign of all keys at the start of the animation
  • We need to implement the values before_to = next_pre and before_post = next_to using a special case in the track cache to pass to the next section pre and post

Linear interpolation in Basis elements has major problems, and linear interpolation in Quaternion elements still has flip problems, so implementation in the track cache may be inevitable.

@fire
Copy link
Member

fire commented Feb 14, 2022

@GNSS-Stylist

We were able to get master...V-Sekai:fix-cubic_slerp to work. There's still some acceleration problems though.

2022-02-14_15.09.39-1.mov
2022-02-13_22-02-53.mp4
2022-02-13_18-11-11.mp4

@fire
Copy link
Member

fire commented Feb 14, 2022

@fire
Copy link
Member

fire commented Feb 20, 2022

Updated master...V-Sekai:fix-cubic_slerp if @GNSS-Stylist wants to test.

2022-02-19.21-38-55.mp4

@GNSS-Stylist
Copy link
Contributor

Sorry for the late response, I was away from my keyboard for a week.

Made a comparison video between the code from @fire 's link (on the left) and my previous component-wise interpolation (on the right, code from #40592 (comment) and previous comments there):

QuatInterpComparison.mp4

Paths seem to differ somewhat. The same flipping as seen on fire's video can be seen 00:25 onward. There are several glitches when the Z-axis (blue) of the the right-side thingy is close to the world's X-axis, about 1 min onward.

Repost from #58043:

For example, if we have [a, -b, c, -d, -e], and the first interpolationA is [a, Q, -b, c], and the next interpolationB is [b, Q, c, -d], then, the sign of the pre value of interpolationB will change (because there is b in interpolationB, but it was -b in interpolationA), and the interpolation-curve will be broken.

So, even if we use exp map, cubic_slerp() requires one of the following two things for sign consistency between pre and post.

* Determine and store the sign of all keys at the start of the animation

* We need to implement the values before_to = next_pre and before_post = next_to using a special case in the track cache to pass to the next section pre and post

Linear interpolation in Basis elements has major problems, and linear interpolation in Quaternion elements still has flip problems, so implementation in the track cache may be inevitable.

Pondered this for a while... As flipped quaternion represents the same orientation with the non-flipped one, shouldn't this work in any case without a cache or such? I mean does it actually matter which quaternions are flipped as long as they are adjusted to give the shortest path between subsequent ones? My brain refuses to understand the exp map right now, though (if the issue is related to it?).

Did some quick tests by flipping (negating all components) of a some quaternions in animation and this didn't change anything, apart from some minor rounding errors causing the center box to turn green/red instead of solid green by changing z-order just enough.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 21, 2022

Quaternions that are flipped in sign show the same orientation, but interpolation make difference the values of those quaternions.
For example, the following two interpolations with weight 0.5 should be different.

Quaternion(2, 1, 4, 3).blend(Quaternion(1, -2, 3, -5), 0.5) = Quaternion(1.5, -0.5, 3.5, -1)
Quaternion(2, 1, 4, 3).blend(Quaternion(-1, 2, -3, 5), 0.5) = Quaternion(0.5, 1.5, 0.5, 4)

The only successful way I have done in my experiments so far is to do an flip of all the key values in advance of the animation, but not during the interpolation. This ensures that the sign does not change during the interpolation process, so no odd paths are created.

I'm discussing with @fire how to implement this. For example, we could do an implicit flip every time we insert/change a Quaternion key.

@fire
Copy link
Member

fire commented Mar 30, 2022

@TokageItLab I noticed your animation scene has something wrong.

When I duplicated the track and set the path to be gizmo2 I had different results.

cubic_slerp_test_duplicate_track.zip

Also, scaling the track by 4 made the results easier to analyse.

@fire

This comment was marked as outdated.

@TokageItLab
Copy link
Member Author

Before:

broken_cubic_slerp.mp4

After #63380:

fixed_cubic_slerp.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment