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

Fixed add_angle #421

Merged
merged 2 commits into from
Jul 13, 2024
Merged

Fixed add_angle #421

merged 2 commits into from
Jul 13, 2024

Conversation

hocop
Copy link
Contributor

@hocop hocop commented Jul 13, 2024

Objective

Fixes 415 and 416

Solution

Old implementation was only for small angles. When impact on RevoluteJoint is big, XPBD tries to apply a big update on angles and fails

Implemented add_angle function using precise computation of sin and cos

@hocop
Copy link
Contributor Author

hocop commented Jul 13, 2024

Although I get that the linearized implementation of add_angle makes more sense for XBPD updates, as they are linear.

Maybe then add another function add_angle_linearized and remove assert from it?

@Khanoto
Copy link
Contributor

Khanoto commented Jul 13, 2024

Shouldn't we use these to add angle ?
image

I think something like that may do it
image

@Jondolf
Copy link
Owner

Jondolf commented Jul 13, 2024

The existing implementation was based on Solver2D's s2IntegrateRot.

We should not compute it by computing sin and cos like that, as it is more expensive, and sin and cos also produce varying results on different architectures, so this would need to use libm for cross-platform determinism when enhanced-determinism is enabled, which is also more expensive.

We should just do the normalization in an optimized way before using from_sin_cos, similarly to what Box2D V3 does.

/// Adds the given counterclockiwise angle in radians to the [`Rotation`].
#[inline]
#[must_use]
pub fn add_angle(&self, radians: Scalar) -> Self {
    let (sin, cos) = (self.sin + radians * self.cos, self.cos - radians * self.sin);
    let magnitude_squared = sin * sin + cos * cos;
    let magnitude_recip = if magnitude_squared > 0.0 {
        magnitude_squared.sqrt().recip()
    } else {
        0.0
    };
    Rotation::from_sin_cos(sin * magnitude_recip, cos * magnitude_recip)
}

@hocop
Copy link
Contributor Author

hocop commented Jul 13, 2024

Thanks! I changed to your implementation and checked that it works

@hocop
Copy link
Contributor Author

hocop commented Jul 13, 2024

@Khanoto I think it is identical to what I initially proposed, just more verbose

Copy link
Owner

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Jondolf Jondolf added bugfix A-Dynamics Relates to rigid body dynamics: motion, mass, constraint solving, joints, CCD, and so on labels Jul 13, 2024
@Jondolf Jondolf merged commit e71de1a into Jondolf:main Jul 13, 2024
4 checks passed
@Jondolf Jondolf added C-Bug Something isn't working and removed bugfix labels Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Dynamics Relates to rigid body dynamics: motion, mass, constraint solving, joints, CCD, and so on C-Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: "the given sine and cosine produce an invalid rotation"
3 participants