[proximity] Adds VolumeMeshTopology

[joemasterjohn]

Adds a class VolumeMeshTopology to compute associated topology data for a given VolumeMesh. As of now, this class only computes tetrahedra adjacencies, but can be further improved to compute and store other variouis topological data.

Towards #21744.

---

This change is 

[joemasterjohn]

+@DamrongGuoy for feature review, if you have the time. Otherwise, I might ask @SeanCurtis-TRI.

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers

[DamrongGuoy]

with minor requests.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers

---

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

namespace drake {
namespace geometry {

BTW, we might want to keep it internal for now.

Suggestion:

namespace geometry {
namespace internal {

---

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

  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(VolumeMeshTopology);

  explicit VolumeMeshTopology(const VolumeMesh<T>* mesh) : mesh_(mesh) {

BTW, consider moving the implementation into .cc file, please.

---

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

  /**
   Returns the index of `i`-th neighbor of tet `e` (i.e. the tet across from
   vertex `i`).

nit: document the -1 case, please.

Suggestion:

   Returns the index of `i`-th neighbor of tet `e` (i.e. the tet across from
   vertex `i`) or -1 for the absence of the `i`-th neighbor.

---

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

  // We use `reset_on_copy` so that the default copy constructor resets
  // the pointer to null when a VolumeMeshTopology is copied.
  reset_on_copy<const VolumeMesh<T>*> mesh_;

BTW, perhaps we don't need to store the pointer?

Code quote:

  // We use `reset_on_copy` so that the default copy constructor resets
  // the pointer to null when a VolumeMeshTopology is copied.
  reset_on_copy<const VolumeMesh<T>*> mesh_;

[joemasterjohn]

+@jwnimmer-tri for platform review, please.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform)

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, we might want to keep it internal for now.

Done.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, consider moving the implementation into .cc file, please.

Done.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

nit: document the -1 case, please.

Done.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, perhaps we don't need to store the pointer?

Done. Good idea. Per f2f, this backpointer is the one thing that required the whole class to be templated (overkill). So we are removing the backpointer and classwide templating.

[jwnimmer-tri]

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

---

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

namespace internal {

/** %VolumeMeshTopology represents the topology of a tetrahedral volume mesh.

nit Throughout the whole header, use /* not /** since the code is in namespace internal and therefore not part of the website / public API.

---

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

  template <typename T>
  explicit VolumeMeshTopology(const VolumeMesh<T>& mesh) {

nit This does not meet the styleguide rules for inline functions. The definition should be in the cc file. (That probably also means the helper function can just fold back into the constructor body, for clarity.)

Likewise, the destructor should be declared in the header and defined in the cc file.

---

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

  template <typename T>
  explicit VolumeMeshTopology(const VolumeMesh<T>& mesh) {

BTW Does this class assume/expect/impose any invariants about the VolumeMesh connectedness / corner cases / other details, beyond those requirements that VolumeMesh already documents and enforces?

---

geometry/proximity/test/volume_mesh_topology_test.cc line 3 at r2 (raw file):

#include "drake/geometry/proximity/volume_mesh_topology.h"

#include <memory>

nit Unused <memory>?

---

geometry/proximity/test/volume_mesh_topology_test.cc line 13 at r2 (raw file):

namespace {

using internal::VolumeMeshTopology;

BTW One nice way to avoid this boilerplate is to put the test inside the internal namespace. That's typically what I do for internal code. It doesn't make much difference here since there is only 1 imported type, but in cases where there is >1 it can help de-boilerplate.

---

geometry/proximity/test/volume_mesh_topology_test.cc line 18 at r2 (raw file):

// components.
GTEST_TEST(VolumeMeshTopologyTest, TestVolumeMeshTopology) {
  // A  trivial volume mesh comprises of two tetrahedral elements with

typo Too many spaces A trivial.

[jwnimmer-tri]

-(release notes: feature) +(release notes: none) since this is internal-only code.

[joemasterjohn]

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),DamrongGuoy

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Throughout the whole header, use /* not /** since the code is in namespace internal and therefore not part of the website / public API.

Done.

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit This does not meet the styleguide rules for inline functions. The definition should be in the cc file. (That probably also means the helper function can just fold back into the constructor body, for clarity.)

Likewise, the destructor should be declared in the header and defined in the cc file.

Done. I had to instantiate the templates by hand in the cc file because I can't take the address of the constructor for the macro DRAKE_DEFINE_FUNCTION_TEMPLATE_INSTANTIATIONS_ON_DEFAULT_NONSYMBOLIC_SCALARS. Is there a known pattern for this?

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Does this class assume/expect/impose any invariants about the VolumeMesh connectedness / corner cases / other details, beyond those requirements that VolumeMesh already documents and enforces?

Nope, no additional assumptions or expectations on the VolumeMesh passed in. Element adjacency is well defined for any input mesh.

---

geometry/proximity/test/volume_mesh_topology_test.cc line 3 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Unused <memory>?

Done. Yes, unused <memory> and <utility> for that matter. Just leftover from testing.

---

geometry/proximity/test/volume_mesh_topology_test.cc line 13 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW One nice way to avoid this boilerplate is to put the test inside the internal namespace. That's typically what I do for internal code. It doesn't make much difference here since there is only 1 imported type, but in cases where there is >1 it can help de-boilerplate.

Done. Just an oversight to not move the test to internal when Damrong asked me to move the class itself to internal.

---

geometry/proximity/test/volume_mesh_topology_test.cc line 18 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

typo Too many spaces A trivial.

Done.

[jwnimmer-tri]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion

---

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

Previously, joemasterjohn (Joe Masterjohn) wrote…

Done. I had to instantiate the templates by hand in the cc file because I can't take the address of the constructor for the macro DRAKE_DEFINE_FUNCTION_TEMPLATE_INSTANTIATIONS_ON_DEFAULT_NONSYMBOLIC_SCALARS. Is there a known pattern for this?

Ah, interesting, I didn't foresee that detail.

In that case, another option would be to leave the constructor inline, but spell it something more like this:

// This constructor is inline so that it can be templated without templating
// the entire class.
template <typename T>
explicit VolumeMeshTopology(const VolumeMesh<T>& mesh) {
  Initialize<T>(mesh);
}

or this:

// This constructor is inline so that it can be templated without templating
// the entire class.
template <typename T>
explicit VolumeMeshTopology(const VolumeMesh<T>& mesh) {
  Initialize<T>(mesh.num_elements(), mesh.tetrahedra());
}

This is slightly beyond what GSG directly allows for, but still meets the main goal of removing expensive code (the resize) from the header. I think it's in the spirit of prong (4) from the GSG -- there is no easy macro we can use to avoid inlining this.

With this spelling, the Initialize can be done as DEFINE_FUNCTION_TEMPLATE... in the cc file for the first spelling, and doesn't need anything special for option for the second spelling.

---

geometry/proximity/volume_mesh_topology.h line 19 at r3 (raw file):

  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(VolumeMeshTopology);

  template <typename T>

nit Needs @tparam_nonsymbolic_scalar in the constructor docstring, to match the cc file listing of implementations.

---

geometry/proximity/test/volume_mesh_topology_test.cc line 3 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Done. Yes, unused <memory> and <utility> for that matter. Just leftover from testing.

(The <utility> was merited by the uses of std::move.)

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion

---

geometry/proximity/volume_mesh_topology.h line 19 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Needs @tparam_nonsymbolic_scalar in the constructor docstring, to match the cc file listing of implementations.

(Or if the cc file loses its explicit instantiations, then we don't need any comment here.)

[DamrongGuoy]

Reviewed 1 of 4 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion

[joemasterjohn]

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),DamrongGuoy

---

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Ah, interesting, I didn't foresee that detail.

In that case, another option would be to leave the constructor inline, but spell it something more like this:

// This constructor is inline so that it can be templated without templating
// the entire class.
template <typename T>
explicit VolumeMeshTopology(const VolumeMesh<T>& mesh) {
  Initialize<T>(mesh);
}

or this:

// This constructor is inline so that it can be templated without templating
// the entire class.
template <typename T>
explicit VolumeMeshTopology(const VolumeMesh<T>& mesh) {
  Initialize<T>(mesh.num_elements(), mesh.tetrahedra());
}

This is slightly beyond what GSG directly allows for, but still meets the main goal of removing expensive code (the resize) from the header. I think it's in the spirit of prong (4) from the GSG -- there is no easy macro we can use to avoid inlining this.

With this spelling, the Initialize can be done as DEFINE_FUNCTION_TEMPLATE... in the cc file for the first spelling, and doesn't need anything special for option for the second spelling.

Done, thanks!. I prefer the second spelling so I went with that.

[jwnimmer-tri]

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

---

geometry/proximity/volume_mesh_topology.h line 42 at r5 (raw file):

 private:
  // Calculate the adjacency information for all tetrahedra in `elements`.

nit GSG verb mood Calculates.

---

geometry/proximity/volume_mesh_topology.cc line 14 at r5 (raw file):

    const std::vector<VolumeElement>& elements) {
  tetrahedra_neighbors_.resize(ssize(elements),
                               std::array<int, 4>{-1, -1, -1, -2});

nit Where did the -2 come from?

[joemasterjohn]

Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),DamrongGuoy

---

geometry/proximity/volume_mesh_topology.cc line 14 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit Where did the -2 come from?

From my caveman way of making bazel recompile this file. Removed.

[jwnimmer-tri]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),DamrongGuoy