collision filter reporting from parser does not play nice with RenameModelInstance workflow

[rpoyner-tri]

What happened?

Found during work on RobotLocomotion/drake-ros#340 . A workflow like this (pseudo-code):

auto& parser = Parser(&plant);
for (/* some times */) {
  parser.SetAutoRenaming(true);
  auto model = AddModels("same_model")[0];
  plant.RenameModelInstance(model, /* other deconflicted name */);
}

crashes. It hits an assertion in the recently added collision filter reporting code. This clearly breaks code that used to work.

The workaround is fairly easy; just move construction of the Parser object inside the loop.

Version

1.28.0

What operating system are you using?

Ubuntu 22.04

What installation option are you using?

Unknown / Other

Relevant log output

abort: Failure at external/drake/multibody/parsing/collision_filter_groups.cc:17 in AddGroup(): condition '!groups_.contains(name_str)' failed.

[rpoyner-tri]

Proposal: just remove the assertion, and document that renaming workflows that reuse the Parser object may lose information, as later groups with conflicting names will overwrite earlier ones in the report. The advice to avoid loss is the same as the workaround above; reduce the scope of the Parser object.

[jwnimmer-tri]

Our forward-looking architecture is RobotDiagramBuilder which will always have a single long-lived Parser.

I suspect that filter group names need to be scoped by model instance name. We need each robot model instance to have distinct filtering, and the group name in the file we load will always be the same. Therefore, just like links and joints and everything else, filter group names must be instance-scoped.

[rpoyner-tri]

Of course filter group names are scoped; otherwise, none of it would work. The deeper gremlin is in the way auto-renaming and rename-model-instance workflows work: the auto-renaming name chooser uses the plant as state, and always finds the "lowest" unoccupied name. So, as you load and then rename (in the plant) models, you will keep finding the same auto-generated name over and over.

There are many options, as always. We can give the auto-renamer some state, so that it doesn't reuse names within a Parser lifetime. We could make the collision-filter reporting computation plant-aware in some way. Or...

[jwnimmer-tri]

Huh, maybe I don't understand the problem, then. Is it maybe that RenameModelInstance fails to update the CollisionFilterGroups dictionary? Ah right, because the rename is a plant feature and the groups are remembered in the Parser? Maybe they should graduate to be a real MbP abstraction?

[rpoyner-tri]

That's what I first proposed.

In the presence of renaming, scoped names are not durable, but model instance indexes are. That's a place to start.

[jwnimmer-tri]

I've moved this up in priority, with the most urgent question being, does the contract for the new function const CollisionFilterGroups& collision_filter_groups() const need to change or not?

[rpoyner-tri]

To decouple this effort from release schedules, #21562 does the API deprecation only, while the fix continues to wind its way through review, and mac compatibility issues.

[jwnimmer-tri]

Fixed in #21529 (review) -- the comment wasn't in the overview so didn't auto-close.