Improve error message when MbP meets unknown geometries

[xuchenhan-tri]

Closes #22164.

---

This change is 

[xuchenhan-tri]

+(release notes: fix) +(priority: low) +@jwnimmer-tri for feature review when you have time.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

[jwnimmer-tri]

Reviewed 5 of 9 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

multibody/plant/discrete_update_manager.h line 247 at r2 (raw file):

  /* Private MultibodyPlant method, made public here. */
  BodyIndex FindBodyByGeometryId(geometry::GeometryId geometry_id) const;

nit On a fresh reading, it seems to me like this declaration should live within the Doxygen group on lines 215-230? (I think maybe the EvalGeometryContactData ended up in the wrong place earlier, too.)

---

multibody/plant/test/discrete_update_manager_test.cc line 447 at r2 (raw file):

  Parser parser(&plant, &scene_graph);
  parser.AddModelsFromUrl("package://drake_models/skydio_2/quadrotor.urdf");

I am little bit hesitant to use a model file that's not super related to this test. If we ever change the model file for other reasons, this test could be unnecessarily affected.

WDYT about something like multibody/models/box.urdf instead?

---

multibody/plant/multibody_plant.cc line 1660 at r2 (raw file):

  if (!geometry_id.is_valid()) {
    throw std::logic_error(
        "MultibodyPlant::FindBodyByGeometryId received an invalid GeometryId");

I don't think using the name of a private method in exception text is particularly helpful to users (and usually we try to avoid doing that).

The existing message was not too bad so I would be OK leaving it alone. If we wanted to try to improve its clarity, the things we could try to highlight more would be that the geometries we're talking about here are for collision pairs reported by the scene graph that's connected to our query input port.

[xuchenhan-tri]

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

multibody/plant/discrete_update_manager.h line 247 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit On a fresh reading, it seems to me like this declaration should live within the Doxygen group on lines 215-230? (I think maybe the EvalGeometryContactData ended up in the wrong place earlier, too.)

Good call, Done.

---

multibody/plant/multibody_plant.cc line 1660 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't think using the name of a private method in exception text is particularly helpful to users (and usually we try to avoid doing that).

The existing message was not too bad so I would be OK leaving it alone. If we wanted to try to improve its clarity, the things we could try to highlight more would be that the geometries we're talking about here are for collision pairs reported by the scene graph that's connected to our query input port.

Done, PTAL.

---

multibody/plant/test/discrete_update_manager_test.cc line 447 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I am little bit hesitant to use a model file that's not super related to this test. If we ever change the model file for other reasons, this test could be unnecessarily affected.

WDYT about something like multibody/models/box.urdf instead?

My thought process was that quadrotor.urdf is a model of an actual hardware so it's less prone to significant hange (e.g. like iiwa and pandas). But after looking more closely, it seems like it's a crude model properly put together for demo purposes?

[jwnimmer-tri]

feature.

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: needs at least two assigned reviewers

---

multibody/plant/test/discrete_update_manager_test.cc line 447 at r2 (raw file):

Previously, xuchenhan-tri wrote…

My thought process was that quadrotor.urdf is a model of an actual hardware so it's less prone to significant hange (e.g. like iiwa and pandas). But after looking more closely, it seems like it's a crude model properly put together for demo purposes?

I don't know that I can speak to the quadrotor's provenance, but in any case I think using a multibody-test-local model is a nice improvement anyway.

[xuchenhan-tri]

+@sammy-tri for platform review on Monday per rotation please.

Reviewable status: LGTM missing from assignee sammy-tri(platform)