-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
Fix real part of quaternion derivatives #488
Conversation
In the Taylor expansion of a quaternion rotation, the first-order term should be purely imaginary. Seems like something got translated incorrectly in the change from xpbd to the current integrator
Thanks, good catch! This changes behavior slightly, so I just updated the Insta snapshots (currently used for cross-platform determinism tests). There's still one more failing test however:
The error from the expected value seems somewhat large 🤔 The fix in this PR looks correct to me though |
Ah, that result actually makes sense! This ended up long. TL;DR changing It's a result of the expression The expression being used is equivalent to pre-multiplying with which in components is For an intended rotation angle The accuracy of the rotation can be improved, taking advantage of the fact that the quaternion exponential itself isn't super hard to compute. Using let scaled_axis = ang_vel * delta_seconds;
if scaled_axis != AngularVelocity::ZERO.0 && scaled_axis.is_finite() {
let delta_rot = Quaternion::from_vec4((scaled_axis / 2.0).extend(0.0)) * rot.0;
rot.0 = (rot.0 + delta_rot).normalize();
} changes to let scaled_axis = ang_vel * delta_seconds;
if scaled_axis != AngularVelocity::ZERO.0 && scaled_axis.is_finite() {
let delta_rot = Quaternion::from_scaled_axis(scaled_axis);
rot.0 = delta_rot * rot.0;
} And now things should rotate more accurately. In my testing the test that was failing at epsilon=0.01 is now good even down to epsilon=1e-12 I initially didn't suggest this change here because I thought there might be a significant performance hit (was intending to ask if you were interested later on), but I've been playing with tracy this past week and it didn't look like there was much difference, if any. Both versions involve a single square root (the current version needs one in the normalise, my proposed change in separating the rotation axis from the rotation angle); the new method additionally computes a sin_cos of the angle for the quaternion exponentiation. If that all sounds good to you, I'll commit and push some changes to that effect! |
I think that sounds good! The more accurate version is probably better if there is no meaningful performance hit. It also seems to be what Rapier uses. This does remove the normalization though, which might lead to error accumulating over time. It might be worth considering a fast renormalization afterwards, based on a first-order Taylor approximation, like Nalgebra's |
This should improve the accuracy of rotations, in particular for faster rotation rates (> approx 0.5 radians per substep for a previous error of 5% in rotation rate)
Wasn't sure of the best way to handle this: * `(self) -> Self` vs `(&mut self) -> ()` * impl on Rotation vs Quaternion (would be harder because it doesn't belong to Avian)
Replace the weird thing I was doing with something that should also work in f64
Well that was a bit of a mess. Helped once I remembered I can run tests locally to check if they'll work in CI 🤡 I've gone through and changed all the rotation updates I could find to use the accurate multiplication method, and do an approximate renormalise afterwards (I think several of the renormalisations might not be necessary, where there's no accumulation of changes across multiple time-steps, but figured I'd use it everywhere just to be safe) Wasn't quite sure where'd be the best place to implement the approximate renormalise function - a |
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.
Thanks, looks good now!
I'm not entirely sure if the accurate rotation update is really necessary, since the approximate linearized formulas should be pretty accurate for smaller individual rotations resulting from substepping, but it's probably still useful if there is no significant performance impact.
In the future, it might be worth trying out if we could use the linearized formulas by default and only fall back to the accurate version if the angle change is large enough that the approximation would exceed some error threshold. Not sure if it would be worth the branching though, so this would need benchmarking.
Objective
The first-order approximations to quaternion rotations were wrong in a couple of places - rather than the
angular velocity * dt/2
quaternions being purely imaginary they were being given a real part related to the real part of the existing quaternion. I think this got mis-translated in commit c1bcff1 fromto
(Note in the "before" case, the
.extend(delta_secs * 0.5 * q.w)
is taking thew
component from the rotation defined on the previous line (i.e. the 0.0 it's beenextend
ed by), not fromrot
!)There are a few other places that do this quaternion maths: in the collider backend and the angular and positional constraints, and those all still extend by
0.0
Solution