[geometry] Add surface mesh, bvh, mapping and topology to SoftGeometry and SoftMesh

[joemasterjohn]

Towards #21744.

This adds a surface mesh, surface mesh bvh, surface element to volume element mapping, and mesh topology to SoftMesh and SoftGeometry. These data structures are necessary for implementing the improved Compliant-Compliant intersection algorithm described in #21744.

---

This change is 

[joemasterjohn]

+@SeanCurtis-TRI for feature review, please.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

[SeanCurtis-TRI]

First pass done. Some API rejiggering. But, on the whole, it'll make this PR much smaller.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: 14 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

a discussion (no related file):
On the subject of tet mesh toplogy:

1. The hydro contact algorithm doesn't depend on nice mesh topology. A tet soup is good enough.
2. We compute meshes with nice topology from primitives.
3. We compute pressure fields from input tet meshes. If the meshes don't have nice topology, the users get insane pressure fields. Insane pressure --> insane contact.
4. This approach that you're pursuing likewise requires nice topology.

I can imagine a day when the VTK we parse includes not only the volume mesh, but also an arbitrary pressure field. In that world, it can be a tet soup as long as the domain is conceptually continuous. With that kind of mesh, the original hydro contact algorithm would still work. But this algorithm won't.

I think we need to have some sense of this going forward, otherwise we're going to be laying landmines for ourselves (for example, if we parse pressure fields without validating meshes). I'm not sure what the "right" thing to do is right now, but we should consider this so that it doesn't simply fall through the cracks and burn us when we end up introducing an otherwise reasonable feature.

---

---

geometry/proximity/hydroelastic_internal.h line 44 at r1 (raw file):

        pressure_(std::move(pressure)),
        bvh_(std::make_unique<Bvh<Obb, VolumeMesh<double>>>(*mesh_)) {
    DRAKE_ASSERT(mesh_.get() == &pressure_->mesh());

nit: It's time to move the constructor to the .cc file.

---

geometry/proximity/hydroelastic_internal.h line 45 at r1 (raw file):

        bvh_(std::make_unique<Bvh<Obb, VolumeMesh<double>>>(*mesh_)) {
    DRAKE_ASSERT(mesh_.get() == &pressure_->mesh());
    surface_element_to_volume_element_ =

BTW Is there a strong reason to prefer surface_element_to_volume_element_ over tri_to_tet_? Particularly as we have differnet kinds of "surface elements", but these are only ever triangles.

---

geometry/proximity/hydroelastic_internal.h line 50 at r1 (raw file):

        ConvertVolumeToSurfaceMeshWithBoundaryVertices(
            *mesh_, nullptr, surface_element_to_volume_element_.get()));
    bvh_surface_mesh_ =

BTW This variable name makes it sound like that it's the surface mesh of the BVH (because of the implied nominal phrase). You might consier surface_mesh_bvh_ as being parsing in the more intuitive way.

Alternatively, if you like having bvh up front, then bvh_of_surface_mesh_ would likewise disambiguate it.

---

geometry/proximity/hydroelastic_internal.h line 219 at r1 (raw file):

  }

  /* Returns a reference to the surface mesh of the underlying volume mesh --

nit: This API suggests we're headed down the wrong path. A path that I admittedly started. It makes far more sense to simply have a getter for a SoftMesh or SoftHalfSpace and let those types' APIs handle everything else. What seemed maybe passable with a smaller API surface area now seems ludicrous as the surface area grows.

Here's what I propose for this PR:

1. Add the half_space() and mesh() getters.
2. Eliminate all of these new SoftMesh wrappers.
3. Put a TODO with my name to go through and eliminate all of the legacy API wrappers.

---

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

  ~VolumeMeshTopology();

  /*

BTW While this isn't introduced in this PR, your comment formatting is inconsistent in this file.

• Class documentation and this function's docs have different white space.
• The private members use // instead of /* ... */.

---

geometry/proximity/volume_mesh_topology.h line 41 at r1 (raw file):

  }

  bool Equal(const VolumeMeshTopology& topology) const {

nit: Functions this large should not be defined inline; the definition needs to go in the .cc file. As the styleguide says, only make the function inline if "there is an acute reason to put it in a header".

---

geometry/proximity/volume_mesh_topology.h line 41 at r1 (raw file):

  }

  bool Equal(const VolumeMeshTopology& topology) const {

BTW Probably worth being clear that this Equals should in no way be considered "equivalence". I.e., The topology of one volume mesh is strictly different from the topology of a mesh that differs from the original by simply "rotating" the order of the vertices in a mesh.

---

geometry/proximity/volume_mesh_topology.h line 41 at r1 (raw file):

  }

  bool Equal(const VolumeMeshTopology& topology) const {

nit: this whole function can be implemented far more simply:

  bool Equal(const VolumeMeshTopology& topology) const {
    return tetrahedra_neighbors_ == topology.tetrahedra_neighbors_;
  }

It should still go in the .cc file.

---

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

  int face_index{};

  bool operator==(const TetFace& other) const {

nit Probably better to implement <=>.

---

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

    EXPECT_TRUE(copy.surface_mesh().Equal(original.surface_mesh()));
    EXPECT_TRUE(copy.bvh_surface_mesh().Equal(original.bvh_surface_mesh()));
    EXPECT_THAT(copy.surface_element_to_volume_element(),

nit: for the same reasons the Equal() implementation can change for Topology, this can be:

    EXPECT_EQ(copy.surface_element_to_volume_element(),
              original.surface_element_to_volume_element());

(In fact, I note that you've done this below for not equals.)

---

geometry/proximity/test/hydroelastic_internal_test.cc line 106 at r1 (raw file):

    EXPECT_TRUE(copy.surface_mesh().Equal(original.surface_mesh()));
    EXPECT_TRUE(copy.bvh_surface_mesh().Equal(original.bvh_surface_mesh()));
    EXPECT_THAT(copy.surface_element_to_volume_element(),

nit: Another candidate for EXPECT_EQ.

---

geometry/proximity/test/hydroelastic_internal_test.cc line 187 at r1 (raw file):

    EXPECT_NE(&original.pressure_field(), &dut.pressure_field());
    EXPECT_NE(&original.bvh(), &dut.bvh());
    EXPECT_NE(&original.surface_mesh(), &dut.surface_mesh());

BTW The SoftGeometry tests are overwrought. Given that copying a SoftGeometry relies on the SoftGeometry's own ability to copy, we don't have to exhaustively confirm that it copied correctly. We just need evidence that it copied.

Again, you've taken what was there (which wasn't entirely well reasoned) and by extending it you've revealed it to be a bad idea.

I'd say:

1. Remove the extra test on SoftMesh fields.
2. Document we're looking for evidence of copying.
3. TODO with my name to clean it up.

This applies to all of the copy/move semantics of SoftGeometry, here and below.

---

geometry/proximity/test/hydroelastic_internal_test.cc line 1053 at r1 (raw file):

  DRAKE_EXPECT_THROWS_MESSAGE(
      half_space->bvh(), "SoftGeometry::bvh.* cannot be invoked .* half space");
  DRAKE_EXPECT_THROWS_MESSAGE(

nit: These extra test will likewise go away (and I'll clean up the rest later).

[joemasterjohn]

Reviewable status: 14 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

On the subject of tet mesh toplogy:

1. The hydro contact algorithm doesn't depend on nice mesh topology. A tet soup is good enough.
2. We compute meshes with nice topology from primitives.
3. We compute pressure fields from input tet meshes. If the meshes don't have nice topology, the users get insane pressure fields. Insane pressure --> insane contact.
4. This approach that you're pursuing likewise requires nice topology.

I can imagine a day when the VTK we parse includes not only the volume mesh, but also an arbitrary pressure field. In that world, it can be a tet soup as long as the domain is conceptually continuous. With that kind of mesh, the original hydro contact algorithm would still work. But this algorithm won't.

I think we need to have some sense of this going forward, otherwise we're going to be laying landmines for ourselves (for example, if we parse pressure fields without validating meshes). I'm not sure what the "right" thing to do is right now, but we should consider this so that it doesn't simply fall through the cracks and burn us when we end up introducing an otherwise reasonable feature.

Maybe we should save this conversation for #22069 since this PR doesn't contain any of the details of this algorithm (the new one) to reference in discussions.

But, one subtle thing I'd want to point out first is that the new algorithm does work if intersecting two geometries that are tet soups. Conceptually, the boundary of an actual tet soup is well defined; it is simply every face of every tet with num_tets connected components. Of course, the bvh of this degenerate "surface" wouldn't provide any speedup (most likely a slowdown) over the original algorithm, but it would produce a correct surface. The new algorithm fails when asked to intersect a tet-soup with a mesh with a well-defined topology that stores the bvh of its actual surface.

I'm just going to dump my thoughts about everything you brought up:

The way I see the state of things right now is we have two features in production that rely on compliant vtk tet meshes having "good" topology: hydro margin and computing pressure fields for non-convex meshes. We don't currently check that the input meshes are well-behaved. We have a new algorithm on deck that provides a good (2x) speed-up in shallow penetration and a significant speedup (>10x) in deep penetration. This algorithm also depends on good topology.

Parsing pressure fields from files is a proposed feature, and I'm not sure it is tied to any existing feature request or user need. I'm also not convinced that it would be a good feature to expose to users. Most users still have a hard time making the right choices from our limited set of hydro parameters for the good fields that we generate for them. I don't think they would do better with the freedom to specify the fields themselves.

And, as you point out, the pressure field provided on disk would have to be conceptually continuous to work. That seems like an immensely complicated thing to do. Effectively, we would need to find adjacencies to check continuity across the interface of two tet faces. To be non-self-intersecting, the tets of the soup would have to intersect exactly at faces (non-empty, 0 volume overlap), which is almost impossible to verify using only double precision.

In my experience, it is pretty rare even to see a tet soup in the wild. Most tet meshes that I've seen used in Drake come from tetwild, tetgen, or some other generating program that guarantees good topology. Tet soups also, in general, require ~6x more memory to represent a volume as compared to a connected mesh with good topology (the average valence of a vertex in a tet mesh is ~6).

On the other hand, checking that an input mesh has good topology is relatively easy. I believe the correct formal requirement for input tet meshes would be: For a tet mesh M, its boundary ∂(M) should be a closed 2-manifold that is not self-intersecting. We may also want to say that we require ∂(M) to have a single connected component (because of the way we define pressure fields from maximal interior signed distance), but that isn't strictly required by any of our features. I consider all of those things relatively trivial to check.

Further, having the requirement that user input meshes have good topology doesn't stop us from adding a feature for parsing a pressure field from file. Suppose the user provides a tet mesh with good topology, finite pressure values at each vertex, and 0 pressure values at each boundary vertex. In that case, it meets all the criteria of a continuous piece-wise linear pressure field that works for hydro.

tl;dr

• I think tet soups are bad, and we should not allow them.
• We should add functionality to test that all user-input meshes have good topology to ensure our existing features always produce expected results.
• Topology requirements on meshes do not stand in the way of parsing arbitrary pressure fields from file but in fact, make it easier to verify the arbitrary field is a valid hydroelastic field.

---

[SeanCurtis-TRI]

Previously, joemasterjohn (Joe Masterjohn) wrote…

Maybe we should save this conversation for #22069 since this PR doesn't contain any of the details of this algorithm (the new one) to reference in discussions.

But, one subtle thing I'd want to point out first is that the new algorithm does work if intersecting two geometries that are tet soups. Conceptually, the boundary of an actual tet soup is well defined; it is simply every face of every tet with num_tets connected components. Of course, the bvh of this degenerate "surface" wouldn't provide any speedup (most likely a slowdown) over the original algorithm, but it would produce a correct surface. The new algorithm fails when asked to intersect a tet-soup with a mesh with a well-defined topology that stores the bvh of its actual surface.

I'm just going to dump my thoughts about everything you brought up:

The way I see the state of things right now is we have two features in production that rely on compliant vtk tet meshes having "good" topology: hydro margin and computing pressure fields for non-convex meshes. We don't currently check that the input meshes are well-behaved. We have a new algorithm on deck that provides a good (2x) speed-up in shallow penetration and a significant speedup (>10x) in deep penetration. This algorithm also depends on good topology.

Parsing pressure fields from files is a proposed feature, and I'm not sure it is tied to any existing feature request or user need. I'm also not convinced that it would be a good feature to expose to users. Most users still have a hard time making the right choices from our limited set of hydro parameters for the good fields that we generate for them. I don't think they would do better with the freedom to specify the fields themselves.

And, as you point out, the pressure field provided on disk would have to be conceptually continuous to work. That seems like an immensely complicated thing to do. Effectively, we would need to find adjacencies to check continuity across the interface of two tet faces. To be non-self-intersecting, the tets of the soup would have to intersect exactly at faces (non-empty, 0 volume overlap), which is almost impossible to verify using only double precision.

In my experience, it is pretty rare even to see a tet soup in the wild. Most tet meshes that I've seen used in Drake come from tetwild, tetgen, or some other generating program that guarantees good topology. Tet soups also, in general, require ~6x more memory to represent a volume as compared to a connected mesh with good topology (the average valence of a vertex in a tet mesh is ~6).

On the other hand, checking that an input mesh has good topology is relatively easy. I believe the correct formal requirement for input tet meshes would be: For a tet mesh M, its boundary ∂(M) should be a closed 2-manifold that is not self-intersecting. We may also want to say that we require ∂(M) to have a single connected component (because of the way we define pressure fields from maximal interior signed distance), but that isn't strictly required by any of our features. I consider all of those things relatively trivial to check.

Further, having the requirement that user input meshes have good topology doesn't stop us from adding a feature for parsing a pressure field from file. Suppose the user provides a tet mesh with good topology, finite pressure values at each vertex, and 0 pressure values at each boundary vertex. In that case, it meets all the criteria of a continuous piece-wise linear pressure field that works for hydro.

tl;dr

• I think tet soups are bad, and we should not allow them.
• We should add functionality to test that all user-input meshes have good topology to ensure our existing features always produce expected results.
• Topology requirements on meshes do not stand in the way of parsing arbitrary pressure fields from file but in fact, make it easier to verify the arbitrary field is a valid hydroelastic field.

I'm happy with all that you've written. I just wanted to make sure thought was given to it. We are accumulating algorithms that require "nice" geometry with no validation of the niceness. We've already tripped over one such case (computing mass properties for a mesh that isn't so nice). It's easy to imagine that we'll have more and more landmines in the future. I just want to make sure we're clear about documenting the needs for a piece of code to work as it is intended and equally open and honest about the fact that we're not validating those requirements.

[joemasterjohn]

Ok @SeanCurtis-TRI, sorry for the delay. All comments addressed, PTAL.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'm happy with all that you've written. I just wanted to make sure thought was given to it. We are accumulating algorithms that require "nice" geometry with no validation of the niceness. We've already tripped over one such case (computing mass properties for a mesh that isn't so nice). It's easy to imagine that we'll have more and more landmines in the future. I just want to make sure we're clear about documenting the needs for a piece of code to work as it is intended and equally open and honest about the fact that we're not validating those requirements.

Got it. I'll make sure that those prereqs are adequately documented when the algorithm goes in. And then make a plan for what it would take to have the correct topology checking in general.

---

---

geometry/proximity/hydroelastic_internal.h line 44 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: It's time to move the constructor to the .cc file.

Done.

---

geometry/proximity/hydroelastic_internal.h line 45 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Is there a strong reason to prefer surface_element_to_volume_element_ over tri_to_tet_? Particularly as we have differnet kinds of "surface elements", but these are only ever triangles.

No preference. I just wrote this before we merged the margin stuff that named this tri_to_tet. Changed.

---

geometry/proximity/hydroelastic_internal.h line 50 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This variable name makes it sound like that it's the surface mesh of the BVH (because of the implied nominal phrase). You might consier surface_mesh_bvh_ as being parsing in the more intuitive way.

Alternatively, if you like having bvh up front, then bvh_of_surface_mesh_ would likewise disambiguate it.

Done. I went with surface_mesh_bvh.

---

geometry/proximity/hydroelastic_internal.h line 219 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This API suggests we're headed down the wrong path. A path that I admittedly started. It makes far more sense to simply have a getter for a SoftMesh or SoftHalfSpace and let those types' APIs handle everything else. What seemed maybe passable with a smaller API surface area now seems ludicrous as the surface area grows.

Here's what I propose for this PR:

1. Add the half_space() and mesh() getters.
2. Eliminate all of these new SoftMesh wrappers.
3. Put a TODO with my name to go through and eliminate all of the legacy API wrappers.

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW While this isn't introduced in this PR, your comment formatting is inconsistent in this file.

• Class documentation and this function's docs have different white space.
• The private members use // instead of /* ... */.

Done.

---

geometry/proximity/volume_mesh_topology.h line 41 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Functions this large should not be defined inline; the definition needs to go in the .cc file. As the styleguide says, only make the function inline if "there is an acute reason to put it in a header".

Done.

---

geometry/proximity/volume_mesh_topology.h line 41 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Probably worth being clear that this Equals should in no way be considered "equivalence". I.e., The topology of one volume mesh is strictly different from the topology of a mesh that differs from the original by simply "rotating" the order of the vertices in a mesh.

Done. I added a clarifcation to the docs.

---

geometry/proximity/volume_mesh_topology.h line 41 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: this whole function can be implemented far more simply:

  bool Equal(const VolumeMeshTopology& topology) const {
    return tetrahedra_neighbors_ == topology.tetrahedra_neighbors_;
  }

It should still go in the .cc file.

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit Probably better to implement <=>.

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: for the same reasons the Equal() implementation can change for Topology, this can be:

    EXPECT_EQ(copy.surface_element_to_volume_element(),
              original.surface_element_to_volume_element());

(In fact, I note that you've done this below for not equals.)

Done.

---

geometry/proximity/test/hydroelastic_internal_test.cc line 187 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW The SoftGeometry tests are overwrought. Given that copying a SoftGeometry relies on the SoftGeometry's own ability to copy, we don't have to exhaustively confirm that it copied correctly. We just need evidence that it copied.

Again, you've taken what was there (which wasn't entirely well reasoned) and by extending it you've revealed it to be a bad idea.

I'd say:

1. Remove the extra test on SoftMesh fields.
2. Document we're looking for evidence of copying.
3. TODO with my name to clean it up.

This applies to all of the copy/move semantics of SoftGeometry, here and below.

Done.

[SeanCurtis-TRI]

With a couple of editorial quibbles.

+@xuchenhan-tri for platform review on Wednesday.

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform)

---

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

Previously, joemasterjohn (Joe Masterjohn) wrote…

Done.

Two things:

1. Declare the return type as std::strong_ordering
2. Let's move the = default into the .cc file.

---

geometry/proximity/hydroelastic_internal.h line 154 at r2 (raw file):

  /* Returns a reference to the SoftMesh -- calling this will throw if
   is_half_space() returns `true`.  */
  const SoftMesh& soft_mesh() const {

BTW Calling this soft_mesh (and the next soft_half_space) is fine. But consider that these are members of a class called SoftGeometry. An aptly named variable of type SoftGeometry will probably already indicate softness. So, we'd end up with something like soft_geometry.soft_mesh(). A bit redundant?

Given the scope of this method loudly declares Soft as a property of the member class, we don't necessarily need to repeat it in its members.

---

geometry/proximity/volume_mesh_topology.h line 44 at r2 (raw file):

   to distinguish whether two `VolumeMeshTopology` are *topologically*
   equivalent to each other (i.e. whether they describe the same adjacency graph
   up to isomorphism.)

nit typo

Suggestion:

isomorphism).

[xuchenhan-tri]

with a few minor points.

Reviewed 1 of 6 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: 5 unresolved discussions

---

geometry/proximity/hydroelastic_internal.cc line 100 at r2 (raw file):

      bvh_(std::make_unique<Bvh<Obb, VolumeMesh<double>>>(*mesh_)) {
  DRAKE_ASSERT(mesh_.get() == &pressure_->mesh());
  tri_to_tet_ = std::make_unique<std::vector<TetFace>>();

BTW, why do we need the indirection here?

---

geometry/proximity/hydroelastic_internal.h line 67 at r2 (raw file):

    return *surface_mesh_bvh_;
  }
  const std::vector<TetFace>& tri_to_tet() const { return *tri_to_tet_; }

BTW, it's not immediately clear to me what the semantics of tri_to_tet is. Consider adding a comment.

---

geometry/proximity/hydroelastic_internal.h line 145 at r2 (raw file):

   data members of the *wrong* type will throw an exception.  */
  // TODO(SeanCurtis-TRI): remove all  of these legacy API wrappers for SoftMesh
  // and SoftHalfSpace.

nit

Suggestion:

  // TODO(SeanCurtis-TRI): remove all of these legacy API wrappers for SoftMesh
  // and SoftHalfSpace.

[joemasterjohn]

Reviewable status: 1 unresolved discussion

---

geometry/proximity/hydroelastic_internal.h line 67 at r2 (raw file):

Previously, xuchenhan-tri wrote…

BTW, it's not immediately clear to me what the semantics of tri_to_tet is. Consider adding a comment.

Done. I fleshed out the documentation of the whole class a bit more.

---

geometry/proximity/hydroelastic_internal.h line 154 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Calling this soft_mesh (and the next soft_half_space) is fine. But consider that these are members of a class called SoftGeometry. An aptly named variable of type SoftGeometry will probably already indicate softness. So, we'd end up with something like soft_geometry.soft_mesh(). A bit redundant?

Given the scope of this method loudly declares Soft as a property of the member class, we don't necessarily need to repeat it in its members.

I agree that it is redundant. But, unfortunately, there is a legacy API SoftGeometry::mesh() that returns the VolumeMesh, so I went with soft_mesh() (and soft_half_space() for congruence.) I suppose I could go with get_mesh() or as_mesh(), but those still feel like they conflict with the already existing mesh().

We can rename things with impunity after the legacy wrapper APIs go away.

---

geometry/proximity/hydroelastic_internal.cc line 100 at r2 (raw file):

Previously, xuchenhan-tri wrote…

BTW, why do we need the indirection here?

True, strictly not neccessary. Removed.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Two things:

1. Declare the return type as std::strong_ordering
2. Let's move the = default into the .cc file.

There's something strange that I don't understand. I'm testing by running:

bazel run //geometry/proximity:hydroelastic_internal_test

If I define the explicitly-defaulted <=> in the header, everything works:

  std::strong_ordering operator<=>(const TetFace&) const = default;

If I define it in the .cc file as you suggested I get the error:

no match for 'operator==' (operand types are 'const drake::geometry::internal::TetFace' and 'const drake::geometry::internal::TetFace')

If I define both explicitly-defaulted <=> and == then everything works fine. I guess there's no harm in having both operators defined, but why is <=> not providing a default == when defined in the implementation?

Any idea?

[joemasterjohn]

Reviewable status: 1 unresolved discussion

---

geometry/proximity/volume_to_surface_mesh.h line 31 at r1 (raw file):
I guess that is just the expected behavior. The C++20 standard Sec. 11.1.1 states:

If the class definition does not explicitly declare an == operator function, but declares a defaulted three-way comparison operator function, an == operator function is declared implicitly with the same access as the three-way comparison operator function.

And the 2023 standard 11.10.1 clarifies:

If the member-specification does not explicitly declare any member or friend named operator==, an == operator function is declared implicitly for each three-way comparison operator function defined as defaulted in the member-specification, with the same access and function-definition and in the same class scope as the respective three-way comparison operator function, except that the return type is replaced with bool and the declarator-id is replaced with operator==.

So I guess to get the implicit ==, the <=> must be defined as default inline in the member-specification.

[SeanCurtis-TRI]

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

---

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

Previously, joemasterjohn (Joe Masterjohn) wrote…

I guess that is just the expected behavior. The C++20 standard Sec. 11.1.1 states:

If the class definition does not explicitly declare an == operator function, but declares a defaulted three-way comparison operator function, an == operator function is declared implicitly with the same access as the three-way comparison operator function.

And the 2023 standard 11.10.1 clarifies:

If the member-specification does not explicitly declare any member or friend named operator==, an == operator function is declared implicitly for each three-way comparison operator function defined as defaulted in the member-specification, with the same access and function-definition and in the same class scope as the respective three-way comparison operator function, except that the return type is replaced with bool and the declarator-id is replaced with operator==.

So I guess to get the implicit ==, the <=> must be defined as default inline in the member-specification.

So, we've come full circle.

So, let's just have operator== with the default value in the .cc file. (YOu don't require sorting semantics, right?)

[joemasterjohn]

Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

So, we've come full circle.

So, let's just have operator== with the default value in the .cc file. (YOu don't require sorting semantics, right?)

Nope, just equality semanitcs. Done.

[SeanCurtis-TRI]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion

---

geometry/proximity/volume_to_surface_mesh.h line 31 at r4 (raw file):

  int face_index{};

  bool operator==(const TetFace&) const;  // = default;

btw: We don't need this trailing comment. And we don't typically do so when it's defaulted in the .cc file (at least, I don't think we do.) Is there a reason to include this comment? I'd assumed it was a transitory artifact of tinkering.

[xuchenhan-tri]

Reviewed 4 of 6 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion

---

geometry/proximity/hydroelastic_internal.cc line 100 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

True, strictly not neccessary. Removed.

What about the other indirections added in this PR? Are they necessary?

[joemasterjohn]

Reviewable status: 1 unresolved discussion

---

geometry/proximity/hydroelastic_internal.cc line 100 at r2 (raw file):

Previously, xuchenhan-tri wrote…

What about the other indirections added in this PR? Are they necessary?

@SeanCurtis-TRI I might need some insight from the original design of the class to answer this.

[joemasterjohn]

Reviewable status: 1 unresolved discussion

---

geometry/proximity/volume_to_surface_mesh.h line 31 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

btw: We don't need this trailing comment. And we don't typically do so when it's defaulted in the .cc file (at least, I don't think we do.) Is there a reason to include this comment? I'd assumed it was a transitory artifact of tinkering.

I've seen it a few places before. To me it's a nice way to know that the function is defaulted without having to jump over and find it in the .cc file. But I can take it out if it is bothersome.

[SeanCurtis-TRI]

Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

---

geometry/proximity/hydroelastic_internal.cc line 100 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

@SeanCurtis-TRI I might need some insight from the original design of the class to answer this.

Ha, ha. I was going to ask about the unique pointers as well. Then I realized that you were simply following the existing pattern and I couldn't remember why the existing pattern was the way it was. So, instead of opening up that can of worms, I remained mum.

And now all of my hard work has been undone...

We have several choices:

1. unique ptrs for everyone for consistency.
2. non-indirection for new stuff because it is apparently not necessary.
3. Digging into the history and figuring out if any indirection is necessary (regardless of whether it ever was in the past).

I'd say that the indirection is harmless. I'd vote, vaguely, for (1) biased by uniformity (this is implied by my previous quiescence). We can certainly put a TODO on it asking if that's what we want. You can even put my name on it.

---

geometry/proximity/volume_to_surface_mesh.h line 31 at r4 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

I've seen it a few places before. To me it's a nice way to know that the function is defaulted without having to jump over and find it in the .cc file. But I can take it out if it is bothersome.

SGTM

[joemasterjohn]

Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

---

geometry/proximity/hydroelastic_internal.cc line 100 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ha, ha. I was going to ask about the unique pointers as well. Then I realized that you were simply following the existing pattern and I couldn't remember why the existing pattern was the way it was. So, instead of opening up that can of worms, I remained mum.

And now all of my hard work has been undone...

We have several choices:

1. unique ptrs for everyone for consistency.
2. non-indirection for new stuff because it is apparently not necessary.
3. Digging into the history and figuring out if any indirection is necessary (regardless of whether it ever was in the past).

I'd say that the indirection is harmless. I'd vote, vaguely, for (1) biased by uniformity (this is implied by my previous quiescence). We can certainly put a TODO on it asking if that's what we want. You can even put my name on it.

Done. I'm going with (1) and a TODO.

[xuchenhan-tri]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)

[SeanCurtis-TRI]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)