Support collision filtering in SDF parsing

[avalenzu]

9220 made sure that the MultibodyPlant parser for SDF-files doesn't fall over when it encounters a collision_filter_group tag, but those tags still aren't parsed. We should have some mechanism for indicating filtered collisions in SDF files. We're currently adding these in bespoke C++ code in downstream projects.

[avalenzu]

My opening bid is that we should raise ParseCollisionFilterGroup() into parsing/detail_common and call it in the SDF parser. Any counter-offers @amcastro-tri or @SeanCurtis-TRI ?

[SeanCurtis-TRI]

In principle, I'm in favor. However, the name collision_filter_group is anachronistic. We'd need to create the drake: prefixed set of tags. It should definitely be that way in SDF, and it would be better to update URDF to be the same. But other than that minor detail, 100%, yes.

[avalenzu]

The URDF version is already using drake::collision_filter_group, so that should be ok: https://github.com/RobotLocomotion/drake/blob/044b908dbde42c28bf04582f01e61ce3bb5ddce8/multibody/parsing/detail_urdf_parser.cc#L224-L227

[SeanCurtis-TRI]

Oh, yeah! Wasn't paying enough attention. Huzzah!

[amcastro-tri]

In favor, great capability to have.

[calderpg-tri]

@EricCousineau-TRI and I recently encountered a problematic corner case of the existing collision_filter_group parsing - collision filtering via collision_filter_group happens separately from the other collision filtering, so if you add additional collision geometry post-parsing to a body involved in a collision filter group, it's difficult to ensure the new geometry inherits the same collision filtering as the existing collision geometry. So far the only case where this bites us is on the Panda, but that's mostly because we don't use collision_filter_group much. Let's address this issue before we enable widespread use of collision_filter_group.

@EricCousineau-TRI can you post a reproduction of the issue with adding to the Panda collision model?

[SeanCurtis-TRI]

I can definitively confirm this behavior. It is expected, even if regrettable. The "fix", such as it is, won't be a small one. I don't think it should hold up the basic access to collision filter groups in the sdf file. An interim band-aid would be to clearly document this behavior in a discoverable context.

That goes on my TODO list -- as part of my Q4 OKRs (as in, I am already addressing geometry documentation topics of this ilk, and this is a detail that needs to be captured as I draw that net closed).

[EricCousineau-TRI]

If it helps, I've added a quick breadcrumb in Anzu PR 5833.

Basically, the filters are ephemeral in our current code: https://github.com/RobotLocomotion/drake/blob/96e0cc0b94926f0725ba69f544fda6e9200b216c/multibody/plant/multibody_plant.cc#L880-L895

But yes, agreed - this is a usability defect in existing collision filtering, and doesn't necessarily hurt SDFormat support of that feature.

[EricCousineau-TRI]

If it's OK, I can uber-briefly split that aspect out in a separate issue.

[sherm1]

Assigning to @SeanCurtis-TRI for disposition.

[EricCousineau-TRI]

@anubhav-tri just ran into this (see Anzu PR 7244). @SeanCurtis-TRI Would you be heartbroken if I stole this from you over the next few weeks?

[SeanCurtis-TRI]

@EricCousineau-TRI, if you want it, it's yours.

[marcoag]

My opening bid is that we should raise ParseCollisionFilterGroup() into parsing/detail_common and call it in the SDF parser.

I've been having a look at this and this doesn't seem possible right now due to the fact that ParseCollisionFilterGroup() uses XMLElement which is not currently available on the sdf parser side. I thought there's two approaches that can be taken here:

• Get the XMLDocument out of the actual file, find the models and look for the drake:ignored_collision_filter_group tag so the popper info can be passed to the common ParseCollisionFilterGroup() function.
• Instead of moving it to parsing/detail_common, duplicate ParseCollisionFilterGroup() and related functions in parsing/detail_sdf_parser.cc and make it work using the Model and other classes/functions from libsdf.

I thought the second option was cleaner, and probably safer (mostly in terms of missing error checks) so I started working on that direction. However, the first option leaves the XMLDocument on the sdf parser side which also leaves the door open to move/create more common functions and share them between de urdf and the sdf parser. I'm not too sure if we expect a good number of functions to be shared to justify this option, any thoughts?

[SeanCurtis-TRI]

@marcoag, I may have misunderstood the gist of your comment. We can't literally share ParseCollisionFilterGroup between URDF and SDF, but we do have plenty of examples where we do have common code for parsing custom tags in both URDF and SDF (e.g., parsing <drake:proximity_properties> which can be seen here). More generally, take a look at detail_common.h.

So, this effort should be relatively straightforward. All of the required pieces are in place, it's just a case of impementing it.

[RussTedrake]

@SeanCurtis-TRI -- i believe this should be closed now (thanks to #15477). Please re-open if you disagree.