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.

Sign up for GitHub

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

Jump to bottom

Fixed inertia tensor computation and center of mass #7426

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Fixed inertia tensor computation and center of mass #7426

merged 1 commit into from
Jan 11, 2017

Conversation

m4nu3lf
Copy link
Contributor

@m4nu3lf m4nu3lf commented Jan 3, 2017

Issues fixed:

  • To compute the distribution of mass of a rigid body the AABB was used for collision shapes for which the area (volume) is trivial to compute. Now the exact area is computed for those shapes as the AABB can have a pretty different volume if the shape is in an arbitrary orientation.

  • The inertia tensor was not computed correctly because the orientation of the object w.r.t. the rigid body origin was not taken into account

  • The vector of inertia only makes sense in the "Principal Axes of Inertia" frame of reference that isn't always equal to the rigid body frame of reference and which has to be computed through diagonalization of the tensor of inertia ( The Jacobi method has been implemented to diagonalize a symmetric matrix, it is the same used by Bullet ).
    https://en.wikipedia.org/wiki/Moment_of_inertia#Principal_axes

  • The vector of inertia/inertia tensor only makes sense if computed w.r.t the rotation center i.e. the center of mass for a free rigid body, which has to be computed. The center of mass returned by the new get_center_of_mass() method is in global orientation with position relative to the rigid body position.

NOTE: the slider joint dosn't work but it seems not to be related to these changes as it wasn't working even without

Here is a test project with some test scenes that can be run before and after my changes to see the difference https://www.dropbox.com/sh/y44l0surabi56rb/AADpJjVHtJsv4IKwCaZnUISga?dl=0

@reduz
Copy link
Member

reduz commented Jan 3, 2017 via email

@akien-mga akien-mga added this to the 3.0 milestone Jan 3, 2017
@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Jan 3, 2017

@reduz I'm not really sure how the custom center of mass would interact with the rest of the simulation, I have no real knowledge on why this is needed for ragdolls. Adding the method is trivial though

@reduz
Copy link
Member

reduz commented Jan 4, 2017

I think @SaracenOne made a PR that was not merged for this functionality. Do you remember why it was needed to set custom center of mass?

@akien-mga
Copy link
Member

@reduz It's #4912. It's still mergeable btw, there's just one conflict to solve.

@ghost
Copy link

ghost commented Jan 6, 2017

I don't see any corrections to the inertia tensor in #4912

@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Jan 6, 2017

@reduz I'm giving a look at #4912. There are a few things that I don't get. I'm going to review and comment the PR

@bojidar-bg bojidar-bg mentioned this pull request Jan 8, 2017
Closed
@SaracenOne
Copy link
Member

SaracenOne commented Jan 8, 2017 •
edited
Loading

@reduz I spent a fair amount of time trying to get ragdolls to work, and as far as I could tell, overriding the center of mass was the only way to make them anything resembling accurate. Unity has similar technique of allowing you to override centor of mass which is likewise used in ragdoll creation. The only other issue I encountered was discovering that the existing physics system wasn't truly deterministic, in that objects would often fall differently at random, but my meager understanding of physics is nowhere near good enough to understand why it works that way. That said, initial PR I did should be fine, though I'll likely have to update it to account for the recent changes.

@SaracenOne
Copy link
Member

Hold on, let me see if I can upload a ragdoll example I have...

@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Jan 8, 2017

@SaracenOne Yep, It would be much appreciated :)
Also you mentioned Unity, so I had a look at their API. They have a centerOfMass, inertiaTensor and inertiaRotation properties that are automatically computed from the colliders (equivalent to the center_of_mass, inv_inertia and principal_inertia_axes) but that can be also changed manually. Now that I think about it it makes sense to be able to override them to simulate objects with non uniform density (sorry still don't get why they are needed for ragdolls)

I guess we can have the same. Plus a flag like "use_custom_center_of_mass" and "use_custom_inertia" and "use_custom_principal_axes" to avoid the engine to recompute them automatically if the collider are changed change

@SaracenOne
Copy link
Member

SaracenOne commented Jan 8, 2017 •
edited
Loading

@m4nu3lf I've got an example ready along with an updated PR (though not quite synced to the latest due to compatibility breakage). It shows how a ragdoll can be constructed using this method, along with what I think are the optimal settings for sleep thresholds. For comparison, observe how this simulation breaks without overriding the centre of mass. The initial PR contains methods for auto-calculating the centre of mass, but I've included a GDScript addon which provides a simple tool to actually do it, though I've been meaning to provide this functionality natively too. Control over the centre of mass should also make it possible to construct single rigid bodies out of multiple collision primitives.

I'll see if there's more that can be done with this tomorrow.
ragdoll_test.zip

@SaracenOne
Copy link
Member

Better example with an actual mesh. Shows how to map bones to ragdoll rigids.
ragdoll_test_02.zip

m4nu3lf
m4nu3lf commented Jan 8, 2017
View reviewed changes
servers/physics/body_sw.cpp
_inv_inertia=_inertia.inverse();
else
_inv_inertia=Vector3();
if (Math::abs(inertia_tensor.determinant()) > CMP_EPSILON) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This make no sense, the determinant should be allowed to be smaller and we can allow it to be zero as we are computing the inverse through a simple transpose. Will fix asap

@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Jan 8, 2017

@SaracenOne @reduz Fixed a bug, now the ragdoll example provided works

m4nu3lf
m4nu3lf commented Jan 9, 2017
View reviewed changes
doc/base/classes.xml
@@ -22,7 +22,7 @@
<description>
Make a color from red, green, blue and alpha. Arguments can range from 0 to 255.
</description>
</method>
</method>/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whops

@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Jan 9, 2017

No idea why the build fails on Windows and Mac, I didn't change anything in platform/windows/joypad.cpp what is going on?

@reduz
Copy link
Member

reduz commented Jan 9, 2017 via email

@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Jan 9, 2017

But it worked before I updated the PR with the bugfix, and I didn't merge from master again. Does Travis merge before running the build?

@akien-mga
Copy link
Member

Does Travis merge before running the build?

Yes.

@reduz reduz merged commit 7106922 into godotengine:master Jan 11, 2017
@akien-mga akien-mga mentioned this pull request Jan 11, 2017
Closed
@bojidar-bg bojidar-bg mentioned this pull request May 4, 2017
Closed
@m4nu3lf m4nu3lf mentioned this pull request Sep 9, 2017
Closed
@HugoSlt HugoSlt mentioned this pull request Aug 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug topic:physics
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
5 participants
@m4nu3lf @reduz @akien-mga @SaracenOne @bojidar-bg