`default_dissipation >= 0.0` is not a helpful error message for urdf typos

[ggould-tri]

TL;DR: A trivial urdf typo leads to an insane yak shave that no naive user will ever win. We need to reject syntactically bogus elements or physically bogus objects or at least DRAKE_DEMAND close to places where we divide by user-created values.

Many thanks to @xuchenhan-tri for helping me chase this. FYI @rpoyner-tri the parsermeister.

Consider the following URDF snippet, one of three identical spheres:

  <link name="sphere1">
    <collision name="sphere1_collision">
      <geometry>
        <sphere radius="0.50"/>
        <dissipation>1</dissipation>
      </geometry>
    </collision>
    <mass value="1"/>
    <inertia ixx="1"  ixy="0"  ixz="0" iyy="1" iyz="0" izz="1"/>
  </link>

Put the spheres into collision like so:

   diagram_->CreateDefaultContext();
  Context& plant_context =
    diagram_->GetMutableSubsystemContext(*plant_, diagram_context.get());
  const Body& sphere1 = plant_->GetBodyByName("sphere1");
  const Body& sphere2 = plant_->GetBodyByName("sphere2");
  plant_->SetFreeBodyPose(&plant_context, sphere1,
                          drake::math::RigidTransform(
                            Eigen::Vector3d{0, 0, 1}));
  plant_->SetFreeBodyPose(&plant_context, sphere2,
                          drake::math::RigidTransform(
                            Eigen::Vector3d{0, 0, 1.9}));

Evaluate the contact results Eval<ContactResults<double>>(context); and you get:

abort: Failure at external/drake/multibody/plant/contact_properties.cc:74 in GetCombinedHuntCrossleyDissipation(): condition 'default_dissipation >= 0.0' failed.

If you run this in gdb you'll get a stack trace sixty frames deep leading to a NaN. Chase the NaN back in the source code (it's ages earlier in actual execution) and you'll get to https://github.com/RobotLocomotion/drake/blob/c1cbed668360f8da2b96c7d42803a5204098e588/multibody/plant/multibody_plant.cc#L1653 Break on that and you'll find that the dissipation is depends indirectly on a parameter omega that is sqrt(0/0).

... all of which tells you that your mass was zero.

...because mass was meant to go inside an inertial tag. Did you spot that error? I sure didn't!

There are three ways we could have avoided or shortened the yak-shave here; I suggest doing all three:

• multibody_plant.cc:1653 divides by a value of user origin. It should assert that the value is positive finite.
• The mass tag under link was invalid. It should have caused an error. There is no value in allowing it. If we want to allow unknown tags we should at least forbid unknown tags with the same names as real tags.
• Having a floating body of zero mass is never going to be good. We should considering overriding the urdf "default zero mass" to default to epsilon mass, because (I assert) there is no correct calculation that could ever rely on that zero.

[rpoyner-tri]

I buy the first and second bullets above. See (and upvote?) especially https://github.com/RobotLocomotion/drake/issues/16997.

The third bullet I don't buy. I don't think we win much for deviating from the URDF "spec", even when it is dumb.

[xuchenhan-tri]

Thank you for the detailed write up!

Without doing any systematic investigation, I'm not sure if I agree with the last point. It seems like it would cover up the issue of unintended uninitialized mass and prevent failing fast.

[rpoyner-tri]

I could see a revision of the third bullet where some plant code notices a free body with zero mass and gives errors; I'd rather not make that a (specific format sub-) parser responsibility.

[ggould-tri]

Here is a somewhat more ambitious but I think more "spec"-correct proposal.

Presumably the use case for unspecified mass in the plant is kinematics-only work -- IK, collision-checking, visualization, etc. -- that doesn't advance the plant. I think we can agree that nobody should have to write out inertials to do a collision check, and that it doesn't matter for that case what the default is.

Rather, the case where the insane default matters is in simulation, so perhaps MBP should politely decline to time-step if there's a zero-mass rigid subunit. That would save having to add error checks to every single place we do math on mass to figure out which one will fail on any given zero-mass condition.