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

Fix rotations when center of mass is offset #250

Merged
merged 8 commits into from
Nov 18, 2023

Conversation

mbrea-c
Copy link
Contributor

@mbrea-c mbrea-c commented Nov 15, 2023

Objective

Fixes #246 and #244.

Solution

If we consider AccumulatedTranslation to represent the translation of the center of mass (instead of the position),
it's only necessary to account for the center of mass offset when applying the translation. Since we already have access to the original position and translation, computing the change in Position based on the change in center of mass position and rotation is straightforward.

Usages of AccumulatedTranslation that assumed it refers to a translation applied directly to Position had to be updated. Fortunately, AccumulatedTranslation is often accessed through RigidBodyQuery::current_position, so not many changes were needed.

Performance

There's no measurable performance impact when testing in my game with hundreds of child colliders and profiling with Tracy (external trace includes this PR, this trace is main branch):

performance

Alternative solutions considered

An alternative approach is to apply the additional translation due a rotation every time we apply a rotation (during integration, constraint solving) and take into account only the change in position of the center of mass in update_lin_vel. This was what I tried first, but I was unable to get it to work due to numerical inaccuracies that caused unexpected accelerations, or some other insidious bug I couldn't find :). Either way, this alternative would have required more changes, and for what it's worth I personally found it less clean as there was no longer a clean separation between updates to position and rotation (every rotation had to add a small change to the position).

Changelog

  • AccumulatedTranslation now refers to the translation of the center of mass, which may not equal the translation applied to Position when the center of mass is offset.
  • Rotational motion with offset centers of mass now works as expected.
    • As a result, child colliders finally feel stable in my testing.

@Jondolf Jondolf added the A-Dynamics Relates to rigid body dynamics: motion, mass, constraint solving, joints, CCD, and so on label Nov 16, 2023
@mbrea-c mbrea-c marked this pull request as ready for review November 16, 2023 13:51
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.

Huge thanks for looking into this! The behavior looks a lot more realistic and stable with your fixes and resolves a lot of issues with child colliders. I'm a bit surprised that it's as easy as applying AccumulatedTranslation a bit differently. Thanks again!

@Jondolf Jondolf merged commit fdb570d into Jondolf:main Nov 18, 2023
4 checks passed
@Jondolf Jondolf linked an issue Nov 18, 2023 that may be closed by this pull request
@mbrea-c
Copy link
Contributor Author

mbrea-c commented Nov 19, 2023

No problem, thank you for your work on this project!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rotational motion ignores center of mass Unstable when multiple colliders spawned as rigid body children
2 participants