[geometry] Remove the non-BVH interface for rigid-soft mesh intersection #14110

SeanCurtis-TRI · 2020-09-22T20:48:44Z

For historical reasons, computing intersection between a soft volume mesh and a rigid surface mesh had two separate APIs. One used broadphase acceleration with a bounding volume hierarchy (BVH) one did not. Both APIs were propagated as a basis for measuring improvement. The need for both APIs is long gone. So, we'll simplify the API and eliminate the slow version. Going forward, improvements should be against strictly the best possible results. This change had several implications:

mesh_intersection_benchmark now only considers one case -- it reports the performance of queries with the current BVH implementation.
Unit tests in mesh_intersection_test would exploit the BVH-free API to shorten the tests. They needed to be expanded to use the BVH API.
- One test in particular would produce a smoke test confirming that the test could be used with AutoDiffXd-valued meshes. That used the old BVH-free API. In removing that API, we need to be able to build a BVH for an AutoDiffXd-valued mesh.
Bvh and obb (and tests) have been updated to allow constructing a BVH for an AutoDiffXd-valued mesh.

Resolves #13861

This change is

SeanCurtis-TRI

+@DamrongGuoy for feature review, please.

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

DamrongGuoy

I finished feature review. PTAL.

Reviewed 10 of 10 files at r1.
Reviewable status: 15 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

a discussion (no related file):
This PR introduces convert_to_double() in obb.cc to temporarily allow AutoDiffXd. Could you please update this line of documentation in obb.h to reflect that?
obb.h

151  @tparam MeshType is either SurfaceMesh<double> or VolumeMesh<double>.  */
152 template <class MeshType>
153 class ObbMaker {

Right now it says only SurfaceMesh<double> or VolumeMesh<double>. Now it can takes SurfaceMesh<AutoDiffXd> and VolumeMesh<AutoDiffXd>; however, internally it converts AutoDiffXd to double.

I don't have a good way to describe these details, so I won't block this. Set to Discussing.

geometry/benchmarking/mesh_intersection_benchmark.cc, line 92 at r1 (raw file):

 - RigidSoftMesh/2/4/3: 107.44 m^2, 3808 triangles
 - RigidSoftMesh/2/3/1: 48.26 m^2, 2190 triangles
 - RigidSoftMesh/2/2/2: 1.52 m^2, 62 triangles

I think this table is out of date. Could you please update it?

I ran the benchmark and surprisingly got very different number of triangles:

Resulting contact surface sizes:
 - RigidSoftMesh/0/4/0: 0.00 m^2, 0 triangles
 - RigidSoftMesh/1/4/0: 0.00 m^2, 0 triangles
 - RigidSoftMesh/2/4/0: 0.00 m^2, 0 triangles
 - RigidSoftMesh/3/4/0: 0.00 m^2, 0 triangles
 - RigidSoftMesh/2/0/0: 0.00 m^2, 0 triangles
 - RigidSoftMesh/2/1/0: 0.00 m^2, 0 triangles
 - RigidSoftMesh/2/2/0: 1.50 m^2, 62 triangles
 - RigidSoftMesh/2/3/0: 1.14 m^2, 159 triangles
 - RigidSoftMesh/2/4/1: 0.00 m^2, 0 triangles
 - RigidSoftMesh/2/4/2: 0.00 m^2, 0 triangles
 - RigidSoftMesh/2/4/3: 0.00 m^2, 0 triangles
 - RigidSoftMesh/2/3/1: 1.17 m^2, 133 triangles
 - RigidSoftMesh/2/2/2: 1.52 m^2, 70 triangles

I also went back to master branch, so I can run WithoutBVH too.

Both WithBVH and WithoutBVH agree, but they are very different from the supposedly outdated table.

geometry/proximity/bvh.h, line 26 at r1 (raw file):

struct MeshTraits;

template <typename U>

Could it be T instead of U ?

geometry/proximity/bvh.h, line 31 at r1 (raw file):

};

template <typename U>

Could it be T instead of U ?

geometry/proximity/bvh.cc, line 156 at r1 (raw file):

    drake::geometry::VolumeMesh<double>>;

template class drake::geometry::internal::Bvh<

Could you please add a comment that these AutoDiffXd versions are here temporarily?

geometry/proximity/mesh_intersection.cc, line 381 at r1 (raw file):

/* A variant of ComputeContactSurfaceFromSoftVolumeRigidSurface but with
 broad-phase culling to reduce the number of element-pairs evaluated.  */

Remove the comment "A variant of ..." please. Now we have only one version not two variants.

geometry/proximity/mesh_intersection.cc, line 447 at r1 (raw file):

// triggers compile error because:
//   Bvh<VolumeMesh<T>>& bvh_M
// does not support VolumeMesh<AutoDiffXd>.

I think this comment is now outdated. Could you please update this comment in one of these ways?

Remove it, because the template class SurfaceVolumeIntersector<AutoDiffXd> has no compile errors anymore, or
Replace it with this line to confirm that it compiles:

template class SurfaceVolumeIntersector<AutoDiffXd>;

Comment that it's not recommended to do it, or
Other ways as you see fit?

geometry/proximity/test/mesh_intersection_test.cc, line 1161 at r1 (raw file):

  auto s_id = GeometryId::get_new_id();
  unique_ptr<VolumeMesh<double>> s_mesh_S = OctahedronVolume<double>();
  const Bvh<VolumeMesh<double>> bvh_S(*s_mesh_S);

s_bvh_S please. s_bvh_S conforms with s_mesh_S better. Otherwise, we have bvh_S and bvh_R that sound like the same bvh expressed in two different frames.

geometry/proximity/test/mesh_intersection_test.cc, line 1167 at r1 (raw file):

  auto r_id = GeometryId::get_new_id();
  unique_ptr<SurfaceMesh<double>> r_mesh_R = PyramidSurface<double>();
  const Bvh<SurfaceMesh<double>> bvh_R(*r_mesh_R);

r_bvh_R please. r_bvh_R conforms with r_mesh_R better. Otherwise, we have bvh_S and bvh_R that sound like the same bvh expressed in two different frames.

geometry/proximity/test/mesh_intersection_test.cc, line 1268 at r1 (raw file):

GTEST_TEST(MeshIntersectionTest, DoubleAutoDiffMixed) {
  unique_ptr<VolumeMesh<double>> soft_mesh = OctahedronVolume<double>();
  const Bvh<VolumeMesh<double>> bvh_S(*soft_mesh);

bvh_soft_S or soft_bvh_S please. Otherwise, bvh_S and bvh_R sound like the same bvh expressed in two different frames.

geometry/proximity/test/mesh_intersection_test.cc, line 1272 at r1 (raw file):

      OctahedronPressureField<double>(soft_mesh.get());
  unique_ptr<SurfaceMesh<double>> rigid_mesh = PyramidSurface<double>();
  const Bvh<SurfaceMesh<double>> bvh_R(*rigid_mesh);

bvh_rigid_R or rigid_bvh_R please. Otherwise, bvh_S and bvh_R sound like the same bvh expressed in two different frames.

geometry/proximity/test/mesh_to_vtk_test.cc, line 63 at r1 (raw file):

  const Box soft_box(4., 4., 2.);
  // resolution_hint 0.5 is enough to have vertices on the medial axis.
  const VolumeMesh<double> mesh_S = MakeBoxVolumeMesh<double>(soft_box, 0.5);

soft_mesh_S please. Otherwise, we have mesh_S and mesh_R, which sound like the same mesh expressed in two different frames. They are two different meshes of two different resolutions.

geometry/proximity/test/mesh_to_vtk_test.cc, line 67 at r1 (raw file):

  const VolumeMeshFieldLinear<double, double> field_S =
      MakeBoxPressureField<double>(soft_box, &mesh_S, kElasticModulus);
  const Bvh<VolumeMesh<double>> bvh_S(mesh_S);

soft_bvh_S please.

geometry/proximity/test/mesh_to_vtk_test.cc, line 73 at r1 (raw file):

  const Box rigid_box(4, 4, 2);
  // Very coarse resolution_hint 4.0 should give the coarsest mesh.
  const SurfaceMesh<double> mesh_R = MakeBoxSurfaceMesh<double>(rigid_box, 4.0);

rigid_mesh_R please. Otherwise, we have mesh_S and mesh_R, which sound like the same mesh expressed in two different frames. They are two different meshes.

geometry/proximity/test/mesh_to_vtk_test.cc, line 74 at r1 (raw file):

  // Very coarse resolution_hint 4.0 should give the coarsest mesh.
  const SurfaceMesh<double> mesh_R = MakeBoxSurfaceMesh<double>(rigid_box, 4.0);
  const Bvh<SurfaceMesh<double>> bvh_rigid(mesh_R);

rigid_bvh_R please.

SeanCurtis-TRI

I homogenized the names of things: rather than s_mesh_S and soft_mesh_S (and the same for rigid), I opted to take the pre-existing pattern: volume_S, surface_R and propagate it around. Finally, for thing_X, I've named the Bvh bvh_thing_X. PTAL.

Reviewable status: 9 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

a discussion (no related file):