[multibody] Throw on mixed rigid/deformable body name collisions - #24933
[multibody] Throw on mixed rigid/deformable body name collisions#24933castor639 wants to merge 1 commit into
Conversation
Body names must be unique within a model instance across both rigid and deformable bodies. Previously only same-type duplicates were rejected, leaving collision-filter and name lookup ambiguous for mixed types. Fixes RobotLocomotion#23257.
|
+(release notes: fix) +a:@SeanCurtis-TRI for feature review |
|
@drake-jenkins-bot test this please |
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
@SeanCurtis-TRI reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on castor639).
multibody/plant/deformable_model.cc line 71 at r1 (raw file):
nit: This error message should mirror the deformable version above. We know the duplicate name is associated with a rigid body, so the error message should say so.
...already contains a rigid body named ...
multibody/plant/multibody_plant.h line 1453 at r1 (raw file):
if (deformable_model().HasBodyNamed(name, model_instance)) { throw std::logic_error(fmt::format( "Model instance '{}' already contains a body named '{}'. Body names "
nit: Reference what the name belongs to.
Suggestion:
already contains a deformable body named '{}'. multibody/plant/deformable_model.h line 94 at r1 (raw file):
MultibodyPlant. @throws std::exception if the model instance does not exist. @throws std::exception if a deformable body or rigid body with the same name
nit: This will require regenerating the documentation:
bazel run //bindings/generated_docstrings:regenerate
|
@drake-jenkins-bot retest this please |
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
@SeanCurtis-TRI made 2 comments.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers (waiting on castor639).
a discussion (no related file):
This change triggered a failure in //multibody/parsing:detail_collision_filter_group_resolver_test. The test CollisionFilterGroupResolverTest.AmbiguousRigidAndDeformableName tested the old behavior and needs to be updated. The test wasn't expecting a throw.
More particularly, the underlying code was expecting to resolve ambiguous body names after adding them to the plant while resolving the collision filter groups. With this PR, that's no longer necessary, because the plant will not allow such registration.
The simplest way to resolve this is to remove the logic from CollisionFilterGroupResolver::AddGroup() and the corresponding test. We could, instead, stash a DRAKE_DEMAND confirming that the name is unique in the model instance.
It would be good if during parsing, we dispatched an error message related to the input file and its duplicate names. SDFormat does it already. Neither Drake's mujoco nor urdf parsers have any preconditioning. Those simply throw when we trip over the plant, so this doesn't change that behavior, it simply makes sure that the unfortunate behavior of throwing, throws in more of the times it should be.
multibody/plant/test/deformable_model_test.cc at r1 (raw file):
nit: Change introduced lint. Fix via:
bazel-bin/tools/lint/clang-format -style=file -i multibody/plant/test/deformable_model_test.cc
Body names must be unique within a model instance across both rigid and deformable bodies. Previously only same-type duplicates were rejected, leaving collision-filter and name lookup ambiguous for mixed types.
Fixes #23257.
This change is