Support a parser option: like URDF_USE_INERTIA_FROM_FILE

RussTedrake commented 1 year ago

Failing to parse assets due to poorly specified inertial parameters is a real problem (see, for instance: https://github.com/RobotLocomotion/drake/issues/14436#issuecomment-1427468439)

Other simulators support an option like URDF_USE_INERTIA_FROM_FILE. In bullet, the default behavior is actually to ignore the inertial parameters in the file. I think the default behavior from MuJoCo is more appropriate for us.

pybullet documentation: "URDF_USE_INERTIA_FROM_FILE: by default, Bullet recomputed the inertia tensor based on mass and volume of the collision shape. If you can provide more accurate inertia tensor, use this flag."

mujoco documentation inertiafromgeom: [false, true, auto], “auto” This attribute controls the automatic inference of body masses and inertias from geoms attached to the body. If this setting is “false”, no automatic inference is performed. In that case each body must have explicitly defined mass and inertia with the inertial element, or else a compile error will be generated. If this setting is “true”, the mass and inertia of each body will be inferred from the geoms attached to it, overriding any values specified with the inertial element. The default setting “auto” means that masses and inertias are inferred automatically only when the inertial element is missing in the body definition. One reason to set this attribute to “true” instead of “auto” is to override inertial data imported from a poorly designed model. In particular, a number of publicly available URDF models have seemingly arbitrary inertias which are too large compared to the mass. This results in equivalent inertia boxes which extend far beyond the geometric boundaries of the model. Note that the built-in OpenGL visualizer can render equivalent inertia boxes.

Note that MuJoCo offers a few other inertial-parsing options that we would do well to support, also.

jwnimmer-tri commented 1 year ago

Another path to the same goal would be to provide a tool that rewrites the XML file on disk to fix the inertia data (either editing in place, or creating a new copy of the file).

That might be good enough for the common case where users commit their models into source control next to their code. It avoids them needing to plumb the "ignore inertia" flag down into whatever parser instance they are using, which isn't always easy. Broadly, my opinion is that if the model is broken, the user should fix the model, not teach every downstream tool of the model to ignore its crazy bits.

That doesn't work as well when the model isn't on disk, e.g. fetching remote ROS packages. In that case, users would need to fork the model before they fixed it. To me, that's not necessarily a blocker here. Our experience is that users will eventually always want to customize the model to work better in Drake, so eventually they are going to need to fork it anyway.

The other thing we could do to help here would be to change the "non-physical inertia" into a parser warning instead of an exception. Then at least they could still load the broken model, though the dynamics might be weird. (And for motion planning, it wouldn't matter.)

jwnimmer-tri commented 1 year ago

The other thing we could do to help here would be to change the "non-physical inertia" into a parser warning instead of an exception.

That's probably worth doing, but upon further thought it's not really relevant here. A lot of people who choose Drake intend to use its dynamics engine. There are plenty of physically valid mass/inertia values that still yield absurd simulation results. Best to focus on the broader fix here -- rewriting the file to reset the mass/inertia stanzas based on the geometries.