-
Notifications
You must be signed in to change notification settings - Fork 1
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
Use slerp for Quat interpolation #8
Use slerp for Quat interpolation #8
Conversation
@@ -165,7 +121,7 @@ impl Animatable for Transform { | |||
if input.additive { | |||
translation += input.weight * Vec3A::from(input.value.translation); | |||
scale += input.weight * Vec3A::from(input.value.scale); | |||
rotation = (input.value.rotation * input.weight) * rotation; | |||
rotation = rotation.slerp(input.value.rotation, input.weight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% convinced on the math here. Additive blending requires that the addition operation be commutative to function correctly with this one-at-a-time implementation.
That definitely isn't the case for linear interpolation of quaternions, and I'm not 100% convinced it is for spherical interpolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rotations are never commutative, so neither are animations of rotations. There's nothing special about slerp(). In general, animation blending is an ordered operation which yields different results if you reorder it, even for non-rotations.
Additive blending requires that the addition operation be commutative
That sounds very wrong. Blending should be performed in a predefined order, and not be reordered. Did you mean associative rather?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This article claims we need to interpolate between the identity and the additive pose, and then multiply the result by the base pose. I didn't check it's correct, but it's probably not what happens here, so likely we need someone to validate that code before merging.
https://animcoding.com/post/animation-tech-intro-part-3-blending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you very much for the link. That makes much more sense. I was not aware that order was preserved during animation blending.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking on GitHub web on mobile (which is bad and confusing), I can still see we're doing a single slerp()
between source and destination instead of identity. Is this really fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- base anim rotates 90° right (+)
- additive anim rotates 90° left (-)
- w=1/3
- base.slerp(additive, w) gives: 90 * 2/3 - 90 * 1/3 = 30° right
- base + additive.slerp(identity, w) gives: 90 - 90 * 1/3 = 60° right
I believe the link tells us the second one is correct, which feels right. But we still do the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(note that "+" above is the rotation composition, so quaternion multiplication)
As discussed by @djeedai, Quat interpolation should use a spherical interpolation by default.