Compute maximum depth collision points with Bullet dispatch

[amcastro-tri]

PR #2521 identified a "persistent" behavior in the method BulletModel::collisionPointsAllToAll and introduced Model::ClearCachedResults. This persistent behavior is a bug and this PR removes it by using Model::ClearCachedResults.

Method BulletModel::collisionPointsAllToAll does not implement an all-to-all dispatch but uses instead Bullet's dispatching. Therefore this PR refactors its name to Model::ComputeMaximumDepthCollisionPoints which more appropriately describes its functionality (I am open to suggestions here).

model_test.cc is then updated to reflect this corrected functionality by removing the last set of tests in
SmallBoxSittingOnLargeBox.ClearCachedResults.

This PR solves #2004.

---

This change is

[sherm1]

@amcastro-tri, this says do not review; is that still current?

[amcastro-tri]

It is. I pushed this on Friday and I din't have the time I thought to work on it during the weekend. This is one of my tasks for today.

[amcastro-tri]

For this PR I had implemented BulletModel::ComputeMaximumDepthCollisionPoints which uses Bullet dispatching to obtain the deepest point of penetration between collision elements if they collide. However, I realized now while cleaning this up that this method is equivalent to Model::collisionPointsAllToAll plus the newly introduced Model::ClearCachedResults in #2521.

Since the original Model::collisionPointsAllToAll does not perform an all-to-all query but a dispatch through Bullet, I decided to refactor this method to Model::ComputeMaximumDepthCollisionPoints (which now correctly describes its functionality). The only change to this method now is that I clear the cache before performing the query.

Therefore this PR actually became more like a bug fix + a refactor. Now hopefully the Matlab tests don't break...

[amcastro-tri]

+@sherm1 for feature review.
Since we are not in working hours right now and I already assigned @sherm1 for feature review, I will ask +@david-german-tri for platform review.

Previously, amcastro-tri (Alejandro Castro) wrote…

For this PR I had implemented BulletModel::ComputeMaximumDepthCollisionPoints which uses Bullet dispatching to obtain the deepest point of penetration between collision elements if they collide. However, I realized now while cleaning this up that this method is equivalent to Model::collisionPointsAllToAll plus the newly introduced Model::ClearCachedResults in #2521.

Since the original Model::collisionPointsAllToAll does not perform an all-to-all query but a dispatch through Bullet, I decided to refactor this method to Model::ComputeMaximumDepthCollisionPoints (which now correctly describes its functionality). The only change to this method now is that I clear the cache before performing the query.

Therefore this PR actually became more like a bug fix + a refactor. Now hopefully the Matlab tests don't break...

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[amcastro-tri]

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

drake/systems/plants/collision/bullet_model.cc, line 729 [r4] (raw file):

  // perform a clean, from scratch, collision dispatch.
  ClearCachedResults(use_margins);

This is the bug fix I am referring to. Bullet's persistent behavior is a bug. Good for games maybe but an undesired feature for physically accurate simulations.

---

drake/systems/plants/collision/test/model_test.cc, line 677 [r4] (raw file):

  // If Bullet cached those tests then there should be four results.
  ASSERT_EQ(4, points.size());

Here the single, maximum depth, collision point return is tested.

---

drake/systems/plants/collision/test/model_test.cc, line 691 [r4] (raw file):

  // Clearing cached results
  for (int i = 0; i < 4; ++i) {

This is the test removed. Now the correct behavior is "always" a single (deepest penetration) contact point.

---

Comments from Reviewable

[amcastro-tri]

This PR passed CI in 334ea0b (I just merged master again in 532cbb5 with no conflicts). Thus this PR is ready for review.

---

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[sherm1]

My understanding is that this eliminates altogether the multi-point contact that Drake was able to get out of Bullet and which then caused physical simulation problems due to the introduction of new points at arbitrary locations -- is that right? So now a block on the ground will be supported only by a single deepest point and will thus rattle around if simulated with Drake's current contact methods. I'm fine with that if that was your intent since this at least works in a predictable way. We will need to quickly develop a non-buggy multi-point contact capability in that case.

Other than the above caveat, the changes .

---

Reviewed 4 of 4 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[amcastro-tri]

@sherm1: This PR does not eliminate previously implemented functionality (even if buggy). The multi-point contact implementation (with random samples) is implemented by BulletModel::potentialCollisionPoints. This funciontality is accessed through the RBT in RigidBodySystem::dynamics. However this always caused problems or nonphysical behavior and we always set RigidBodySystem::use_multi_contact = false.

This PR does remove the apparent multi-point contact returns from (the now refactored) BulletModel::ComputeMaximumDepthCollisionPoints by removing the persistent behavior of Bullet's persistent manifolds (that certainly is a bug). That is enought (CI passes) for all the tests/examples we have right now.

I do agree we need a true multi-contact implementation. My plan is to explore polyhedral convex hulls in Bullet (I wonder if applicable to convex geometries only).

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[sherm1]

OK -- just need platform review now. Friday: +@ggould-tri

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[ggould-tri]

Took me a little while to get my head around this one, but I think I understand now.

---

Reviewed 4 of 4 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[sherm1]

Thanks, Grant! Merging.