search

RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.32k stars 1.26k forks source link

Improve documentation for setting center of mass of a body #17571

Closed allenzren closed 1 year ago

allenzren commented 2 years ago

In the Body/Rigidbody class, users can call SetCenterOfMassInBodyFrame to move the center of mass of a body in the body frame. However, this does not shift the spatial inertia of the body to the new center of mass. Namely, I_SP in spatial inertia is not updated.

Users may assume changing the center of mass automatically shifts the inertia, which is not true. The new spatial inertia becomes physically invalid.

We may add such to the documentation of SetCenterOfMassInBodyFrame: "Note that this does not shift the spatial inertia of the body to the new center of mass. Please see Shift and ShiftInPlace in SpatialInertia class."

EDIT: to shift the center of mass to a new point Q=[1,0,0] in the body frame, the user need to actually call spatial_inertia.Shift([-1,0,0]), which is not very intuitive. We should make this clear in the documentation.

allenzren commented 2 years ago

As a side note, I figured out the issue when the physically invalid spatial inertia caused SAP solver to fail: Failure at multibody/contact_solvers/sap/sap_model.cc:391 in CalcDelassusDiagonalApproximation(): condition 'A_ldlt[c].eigen_linear_solver().isPositive()' failed. @amcastro-tri TAMSI did not throw an error.

amcastro-tri commented 2 years ago

That's not just a documentation issue, it's a bug. Feel free to reassign @mitiguy

sherm1 commented 2 years ago

I don't think that's a bug, Alejandro, except maybe that we provided "Set" sugar methods when we should have stuck with the minimal SetSpatialInertia API. If someone sets mass properties by calling SetMass, SetInertia, SetCOM in that order I think they would not be pleased if the last call changed the the inertia. Should we deprecate these methods?

I could be convinced otherwise but that's my current thought. What do you propose?

amcastro-tri commented 2 years ago

SetCOM should also shift the COM leading to a valid model. Maybe the name is misleading. We should not have SetCOM but instead ShiftCOM? I don't think a user would ever like to call SetCOM and end up with an invalid model?

mitiguy commented 1 year ago

PR https://github.com/RobotLocomotion/drake/pull/18251 is part of resolution towards issue #17571.

mitiguy commented 1 year ago

PR https://github.com/RobotLocomotion/drake/pull/18582 resolves https://github.com/RobotLocomotion/drake/issues/17571.