Geometries registered with SceneGraph and not MbP w/ role==kProximity don't get picked up for simulation collision in MbP?

[EricCousineau-TRI]

Reproduction

Ran into this into some subgraph protoype code here: https://github.com/EricCousineau-TRI/repro/blob/b2d7f668ed1b5a757b984c54845b78a56e991d5b/drake_stuff/multibody_plant_prototypes/multibody_plant_subgraph.py#L473-L491

Basically, if I register a collision with MultibodyPlant.RegisterCollisionGeometry, I'm golden. However, if I instead register the collision with SceneGraph.RegisterGeometry, some necessary bookkeeping gets lost (the coulomb friction? some collision index?) such that the collision doesn't produce any reaction forces in simulation; however, it can be used in proximity queries, like SceneGraph.HasCollisions.

Problem

Show in this TODO, I think? https://github.com/RobotLocomotion/drake/blob/a1cb3343c86b03b2f02152ed81eb768607d1597b/multibody/plant/multibody_plant.cc#L454-L456

Workaround

Just use RegisterCollisionGeometry, as shown in example code above.

Solution

Dunno?

[EricCousineau-TRI]

Tentative assignment for now! \cc @SeanCurtis-TRI @joemasterjohn

[amcastro-tri]

I believe #13371 could actually solve your problem. Could you verify @EricCousineau-TRI ? essentially that PR gets rid of MBP's internal bookkeeping collision_index_. Towards #13064, which'd allow specifying all contact properties through ProximityProperties, which I believe is exactly what you want.

[EricCousineau-TRI]

Just to follow-up - I would love to try out #13371, but if possible, would wanna try it out after it's been merged to master. That cool? Or will my trying it out somehow ~inform~ affect the PR itself?

[SeanCurtis-TRI]

FTR #13371 does not address this issue. There was discussion on possibly pushing it that way, but it proved to be a big enough issue that it was deserving of its own issue. See the "discussion (no related file)" in #13371 (search for geometry_id_to_collisionindex, reviewable apparently doesn't like providing links to those discussions without related files.)

[joemasterjohn]

Hi @EricCousineau-TRI, actually I don't think #13371 will solve your problem. MBP is doing some bookkeeping of collision geometries as @amcastro-tri said, and keeping a mapping of collision geometry IDs around. It uses the size of that map (geometry_id_to_collision_index_) in num_collision_geometries() to back out of contact computations when MBP has 0 registered geometries. Working on removing that in a separate PR. Will update with details.

[EricCousineau-TRI]

Thank y'all for the near simultaneous response! And sounds good!

FTR, @SeanCurtis-TRI, I see the discussion you're talking about - basically the one upstream of here: https://github.com/RobotLocomotion/drake/pull/13371#pullrequestreview-421225818 (but yeah, the link itself doesn't go anywhere useful).

[joemasterjohn]

I believe #13545 will solve the problem you're experience here. Trying to get it merged ASAP.

[EricCousineau-TRI]

Thanks!

And Is there a way this can be guaranteed in an unittest, either in that PR or in a follow-up? I'm happy to write out a reduced MWE/repro in C++ to provide a basis for the unittest.

[EricCousineau-TRI]

Basically, add a half-plane via MBP API, then an MBP body w/ no geometry, then add the geometry (a ball / sphere) through SG. Forward simulate, and show that the ball did not fall through the ground.

[EricCousineau-TRI]

:thinking: :stuck_out_tongue_winking_eye:

[EricCousineau-TRI]

This came up in Anzu 5510. I'll try to circle back to the test which failed due to the doubly-tracked geometry.

In that Anzu PR, it came up with what happens to MultibodyPlant::collision_geometries_ when those geometries are changed (e.g. a geometry removed) by SceneGraph, not MBP: https://github.com/RobotLocomotion/drake/blob/868c758afb89c0e25802a99335688d9cf617dd72/multibody/plant/multibody_plant.cc#L481

\cc @calderpg-tri

[EricCousineau-TRI]

Er, re-reading the convos and re-inspecting things, I'm not yet convinced... Also, I'm a bit uncomfortable with the fact that #13545 introduces even more SG method-forwarding on top of what MBP already does...

AFAICT It's because "we don't store / expose the plant.scene_graph() b/c transmogrification / the Systems framework", but egad. I wish there some way around that at some point, but I don't have a good non-hack solution...

[SeanCurtis-TRI]

So, the "forwarding" is only happening to provide support to the API that by rights should not exist. The work flow should be: ask MBP for body -> ask MBP for frame id from body -> ask SG for geometries for frame id. However, we've got these long-lived APIs that are used.

We've started whittling away at MBP's dependence on its collision_geometriers_, but as long as it is there, there are land mines waiting for unsuspecting perambulators.

We still need to extract MBP's knowledge of its collision geometries entirely. @joemasterjohn had started a foray in that direction, but I think it got larger than anticipated. Once MBP no longer tracks its own geometries, then changes to scene graph will automatically percolate through. Until that glorious day, we're screwed.

[jwnimmer-tri]

We've started whittling away at MBP's dependence on its collisiongeometriers, but as long as it is there, there are land mines waiting for unsuspecting perambulators.

We still need to extract MBP's knowledge of its collision geometries entirely ...

@SeanCurtis-TRI I thought we had a ticket for that already, but I'm not seeing it. Do you know of one? I was hoping to close this ticket as a duplicate of that one.

[SeanCurtis-TRI]

I'm not aware of any ticket. I know @joemasterjohn had taken a stab in the past. I've likewise taken a stab at it. The biggest thing is that APIs that used to call directly into the plant (accessing plant members) now require a context. I don't remember all the details, but I remember at the time it started spiraling larger than I'd expected.

At one point, I'd tried hacking something as well.