Mujoco parser: improve error message for "size from mesh"

[RussTedrake]

Towards #20444. Related to #22372.

+@rpoyner-tri for feature review, please.

---

This change is 

[rpoyner-tri]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers

---

a discussion (no related file):
nit: consider adding (a) test case(s) to detail_mujoco_parser_test to to exercise the new error.

[RussTedrake]

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

---

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit: consider adding (a) test case(s) to detail_mujoco_parser_test to to exercise the new error.

Done.

+@jwnimmer-tri for platform review, please (per rotation)

[jwnimmer-tri]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)

---

multibody/parsing/detail_mujoco_parser.cc line 688 at r2 (raw file):
The listing of types here seems extraordinarily brittle, and I'm not even sure the logic is correct. The spec says that:

[mesh] can also be specified if the geom type corresponds to a geometric primitive, namely one of “sphere”, “capsule”, “cylinder”, “ellipsoid”, “box”.

The hard-coded list of types in the if-test here seems to be all of the known types from our if-else chain over the next 120 lines, excepting the types the spec calls out in the quote. That means if we add more (non-primitive) types to the if-else chain below, we would also need to remember to fix this spot, but that's not very likely to be noticed.

Wouldn't it be a much more robust phrasing go like this?

If has_mesh_attribute is True, then:

(1) if type is "mesh" then proceed without remark; else

(2) if type is “sphere”, “capsule”, “cylinder”, “ellipsoid”, or “box” then report Error() with message citing 22372; else

(3) report Error() that "mesh" is not allowed on this $type (e.g., a plane).

Phrasing the code an an allow-list that matches the spec exactly seems a lot better long-term.

If you agree with behavior (3) then we'll also need a (small) unit test case for trying to specify the mesh for a plane and/or hfield.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)

---

multibody/parsing/detail_mujoco_parser.cc line 688 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The listing of types here seems extraordinarily brittle, and I'm not even sure the logic is correct. The spec says that:

[mesh] can also be specified if the geom type corresponds to a geometric primitive, namely one of “sphere”, “capsule”, “cylinder”, “ellipsoid”, “box”.

The hard-coded list of types in the if-test here seems to be all of the known types from our if-else chain over the next 120 lines, excepting the types the spec calls out in the quote. That means if we add more (non-primitive) types to the if-else chain below, we would also need to remember to fix this spot, but that's not very likely to be noticed.

Wouldn't it be a much more robust phrasing go like this?

If has_mesh_attribute is True, then:

(1) if type is "mesh" then proceed without remark; else

(2) if type is “sphere”, “capsule”, “cylinder”, “ellipsoid”, or “box” then report Error() with message citing 22372; else

(3) report Error() that "mesh" is not allowed on this $type (e.g., a plane).

Phrasing the code an an allow-list that matches the spec exactly seems a lot better long-term.

If you agree with behavior (3) then we'll also need a (small) unit test case for trying to specify the mesh for a plane and/or hfield.

Or maybe (3) is a warning, not an error? I don't know the nominal MuJoCo parser policy when we encounter elements not specified in the schema.

[RussTedrake]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)

---

multibody/parsing/detail_mujoco_parser.cc line 688 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Or maybe (3) is a warning, not an error? I don't know the nominal MuJoCo parser policy when we encounter elements not specified in the schema.

Done. (the mujoco parser throws an error, so i have, too)
https://github.com/google-deepmind/mujoco/blob/69c9ac074a7f54c7f9b7619b8bcc3d0eb69bfbcd/src/user/user_mesh.cc#L744

[jwnimmer-tri]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform)