Add functionality to create Convex shapes from vertices

[Akshay5312]

This PR adds functionality to create Convex Shapes given sets of vertices. It addresses issue #22386.

---

This change is 

[jwnimmer-tri]

@drake-jenkins-bot ok to test

[Akshay5312]

I made a mistake when running tests. I'll make this a draft till some errors are fixed.

[jwnimmer-tri]

Thanks, no hurry. Our team probably won't look until Monday at the earliest.

[Akshay5312]

I'm a bit confused as to why this is failing? It builds and all '''bazel test ...''' pass on my computer.

I tried running bazel test --config=clang --compilation_mode=opt --config=everything ... as well, but I do not have a gurobi licence, so it's failing.

[jwnimmer-tri]

+@SeanCurtis-TRI for feature review / advice, please.

[SeanCurtis-TRI]

With a couple of small cosmetic items. Let me know if any of my suggestions aren't clear.

Reviewed 4 of 6 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/test/shape_specification_test.cc line 667 at r3 (raw file):

GTEST_TEST(ShapeTest, ConvexFromVertices) {
  // This will get normalized to ".obj".
  Eigen::MatrixXd vertices(3, 8);

btw: You don't require 8 vertices, the same four you use in vpolytope_test.cc would suffice.

---

geometry/shape_specification.h line 289 at r3 (raw file):

  explicit Convex(MeshSource source, double scale = 1.0);

  /** Constructs a convex shape specification from the given `vertices`.

BTW Would it be more generally correct to simply refer to this as a set of "points"? Or the whole as a "point cloud"? (Maybe not point cloud as there is an actual PointCloud type in Drake.) "Vertices" seems to suggest a mesh that isn't required and may not exist.

---

geometry/shape_specification.h line 289 at r3 (raw file):

  explicit Convex(MeshSource source, double scale = 1.0);

  /** Constructs a convex shape specification from the given `vertices`.

nit: I suspect it's worth being clear that this constructor will produce an obj-encoded in-memory Convex. As your test shows, introspecting the Convex after the fact requires that knowledge and it would be nice if we didn't surprise callers.

---

geometry/shape_specification.h line 296 at r3 (raw file):

   @param vertices      The vertices of the convex hull.
   @param filename_hint A label for the file. The label is used for warning and

BTW "filename_hint" is probably a misnomer. Perhaps "convex_name" or "name"? We have a filename_hint for MemoryFile because it is a kind of filename. This point cloud doesn't purport to be a file at all, so referencing "files" may give the wrong suggestion.

Either way, we should change the documentation so it's not a "label for the file", but, rather, a label for the point cloud.

---

geometry/shape_specification.cc line 22 at r3 (raw file):

namespace {
std::string VerticesToString(const Eigen::Matrix3X<double>& vertices) {

nit: Can we rename this VerticesToObjString or PointsToObjString, depending on whether we change the data from vertices to points. I'd like the function name to be clear about the formatting of the string.

---

geometry/shape_specification.cc line 25 at r3 (raw file):

  std::string result = "";

  result += fmt::format("# Vertices: {}\n", vertices.cols());

BTW This line isn't strictly necessary. It's a diagnostic comment for a string that lives in memory. I'd suggest we cut it.

That said, do you feel there's a good debug/diagnostic reason to keep it? If it's a good but not obvious reason, we might want to add a note to the code here justifying it.

---

geometry/shape_specification.cc line 152 at r3 (raw file):

Convex::Convex(const Eigen::Matrix3X<double>& vertices,
               const std::string& filename_hint, double scale)
    : Convex(InMemoryMesh(

nit: In clang the constructor required by this invocation doesn't exist. You should use the the designated initializer (we only have default, copy, and move constructors):

Convex::Convex(const Eigen::Matrix3X<double>& vertices,
               const std::string& filename_hint, double scale)
    : Convex(
          InMemoryMesh{
              .mesh_file =
                  MemoryFile(VerticesToString(vertices), ".obj", filename_hint),
          },
          scale) {}

(Note: I've also changed extension to lower case. Either works, but as we'll be changing it to lower case anyways -- as Drake's canonical format for extension -- we might as well use the preferred from right out of the gate.)

[Akshay5312]

Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

geometry/shape_specification.cc line 25 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This line isn't strictly necessary. It's a diagnostic comment for a string that lives in memory. I'd suggest we cut it.

That said, do you feel there's a good debug/diagnostic reason to keep it? If it's a good but not obvious reason, we might want to add a note to the code here justifying it.

I was debating whether to put it in or not. In he end I thought it might just be "better practice" in case anyone needed to debug. It probably wasn't really going to be useful at all; I've just removed it.

---

geometry/shape_specification.cc line 152 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: In clang the constructor required by this invocation doesn't exist. You should use the the designated initializer (we only have default, copy, and move constructors):

Convex::Convex(const Eigen::Matrix3X<double>& vertices,
               const std::string& filename_hint, double scale)
    : Convex(
          InMemoryMesh{
              .mesh_file =
                  MemoryFile(VerticesToString(vertices), ".obj", filename_hint),
          },
          scale) {}

(Note: I've also changed extension to lower case. Either works, but as we'll be changing it to lower case anyways -- as Drake's canonical format for extension -- we might as well use the preferred from right out of the gate.)

Is this what was failing in the CI test?

---

geometry/test/shape_specification_test.cc line 667 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

btw: You don't require 8 vertices, the same four you use in vpolytope_test.cc would suffice.

Understood. I also added a test to make sure the convex hull is taken.

[SeanCurtis-TRI]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/shape_specification.cc line 152 at r3 (raw file):

Previously, Akshay5312 wrote…

Is this what was failing in the CI test?

It certainly was the difference between success/failure when building locally against clang. So, I'm assuming it'll make CI happy. :)

---

geometry/shape_specification.h line 295 at r4 (raw file):

   scaled by the given scale factor.

   @param points      The points whose convex hull define the shape.

nit: spacing. Shortening the name now needs to be offset with a space to maintain consistent formatting.

---

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

  EXPECT_EQ(hull.num_elements(), 4);

  // make sure the convex hull is automatically taken.

nit: Capitalization: "Make"

---

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

  EXPECT_EQ(hull.num_elements(), 4);

  // make sure the convex hull is automatically taken.

nit: The two tests together are quite redundant. Only one is necessary. I'd recommend the following:

1. Eliminate the first test (only four points). There's nothing that this test tells us that the second test (five points) won't.
2. Move the "interior" point in the second test to lie in the middle of the array (e.g., index 2).
   • This will show that we're not just copying the first four vertices.

[Akshay5312]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: The two tests together are quite redundant. Only one is necessary. I'd recommend the following:

1. Eliminate the first test (only four points). There's nothing that this test tells us that the second test (five points) won't.
2. Move the "interior" point in the second test to lie in the middle of the array (e.g., index 2).
   • This will show that we're not just copying the first four vertices.

I made the change, but I realized now that this doesn't actually prove that we aren't just taking the first four points, as the first four points still make a shape with 4 vertices and 4 faces (which is all that is being tested).

[SeanCurtis-TRI]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):
You are, of course, correct.

Options:

1. Let's be more thorough:
   • Confirm that there are five lines in the mesh file's contents, each beginning with "v" (i.e., we know we're getting the requisite number of vertices written).
   • Create a bounding box on the vertices and confirm that the box reaches the extent we expect [0, 1] x [0, 1] x [0, 1].

or

2. Document what the test is doing and what it is not doing. Something like:

This test confirms that the vertices, name, and scale get translated into a memory file. It further confirms that the mesh file's contents are compatible with computing a convex hull successfully and that the convex hull has at least some positive indication of correctness (number of vertices and faces). However, it relies on inspection of the code to confirm that all vertices are included in the mesh file.

[Akshay5312]

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

You are, of course, correct.

Options:

1. Let's be more thorough:
   • Confirm that there are five lines in the mesh file's contents, each beginning with "v" (i.e., we know we're getting the requisite number of vertices written).
   • Create a bounding box on the vertices and confirm that the box reaches the extent we expect [0, 1] x [0, 1] x [0, 1].

or

2. Document what the test is doing and what it is not doing. Something like:

This test confirms that the vertices, name, and scale get translated into a memory file. It further confirms that the mesh file's contents are compatible with computing a convex hull successfully and that the convex hull has at least some positive indication of correctness (number of vertices and faces). However, it relies on inspection of the code to confirm that all vertices are included in the mesh file.

I added a test to check that the expected vertices are contained in the polygon surface mesh. This alongside the check that the number of vertices is correct should be sufficient, right?

[SeanCurtis-TRI]

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

Previously, Akshay5312 wrote…

I added a test to check that the expected vertices are contained in the polygon surface mesh. This alongside the check that the number of vertices is correct should be sufficient, right?

Oops. Somehow we're back to two tests again. It's not clear to me why you need to create an entirely different Convex to confirm that the vertices were included.

If you like, I can try pushing a commit to your PR and you can see what you think.

[SeanCurtis-TRI]

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Oops. Somehow we're back to two tests again. It's not clear to me why you need to create an entirely different Convex to confirm that the vertices were included.

If you like, I can try pushing a commit to your PR and you can see what you think.

Actually, new epiphany. It's such a simple test, perhaps it would be most direct to simply affirm the mesh file's contents:

EXPECT_EQ(source.in_memory().mesh_file.contents(), R"""(v 0 0 0
v 1 0 0
v 0.25 0.25 0.25
v 0 1 0
v 0 0 1

That is the most direct way to confirm the content is exactly what you expect.

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/shape_specification.cc line 29 at r6 (raw file):

        fmt::format("v {} {} {}\n", points(0, i), points(1, i), points(2, i));
  }
  result += "\n";

nit: You don't need this newline; you already have each vertex line ending with a newline.

[Akshay5312]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Actually, new epiphany. It's such a simple test, perhaps it would be most direct to simply affirm the mesh file's contents:

EXPECT_EQ(source.in_memory().mesh_file.contents(), R"""(v 0 0 0
v 1 0 0
v 0.25 0.25 0.25
v 0 1 0
v 0 0 1

That is the most direct way to confirm the content is exactly what you expect.

I didn't think I was creating a new Convex, just accessing the vertices of the PolytopeSurfaceMesh. I'll just test the file contents instead!

[SeanCurtis-TRI]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/test/shape_specification_test.cc line 686 at r4 (raw file):

Previously, Akshay5312 wrote…

I didn't think I was creating a new Convex, just accessing the vertices of the PolytopeSurfaceMesh. I'll just test the file contents instead!

You hadn't, I'd gotten lost in Reviewable. My mistake.

By testing the string, you don't have to compute the convex hull at all. Comparing the strings tells us everything we need to know about the new code.

BTW I realize the C++ code I suggested above was incomplete/defective. I think if you change the test to this it reads nicer:

  // Confirm that the contents of the in-memory mesh file are as expected. Note:
  // We prefix a "\n" to match leading "\n" in the nicely readable raw string.
  EXPECT_EQ("\n" + source.in_memory().mesh_file.contents(),
            R"""(
v 0 0 0
v 1 0 0
v 0.25 0.25 0.25
v 0 1 0
v 0 0 1
)""");

---

geometry/test/shape_specification_test.cc line 689 at r7 (raw file):

  EXPECT_EQ(hull.num_elements(), 4);

  // Confirm that the cntents of the in-memory mesh file are as expected.

nit: spelling error: "contents"

[SeanCurtis-TRI]

@drake-jenkins-bot retest linux-jammy-gcc-bazel-experimental-debug please.

[SeanCurtis-TRI]

@drake-jenkins-bot linux-jammy-gcc-bazel-experimental-debug please

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/shape_specification.cc line 29 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: You don't need this newline; you already have each vertex line ending with a newline.

Thanks

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/test/shape_specification_test.cc line 685 at r8 (raw file):

  EXPECT_EQ(source.in_memory().mesh_file.filename_hint(), mesh_name);

  const PolygonSurfaceMesh<double>& hull = convex.GetConvexHull();

nit: I may have buried other comment, I apologize.

But I do strongly recommend deleting the computation of the convex hull. In my mind, this was a proxy for making sure the file contents were, in some sense, "valid". Now that we're confirming the contents directly, it's redundant.

Is there some further value in your mind that I'm not currently seeing? If not, we should simplify the test and nix these three lines.

[Akshay5312]

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

geometry/test/shape_specification_test.cc line 685 at r8 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I may have buried other comment, I apologize.

But I do strongly recommend deleting the computation of the convex hull. In my mind, this was a proxy for making sure the file contents were, in some sense, "valid". Now that we're confirming the contents directly, it's redundant.

Is there some further value in your mind that I'm not currently seeing? If not, we should simplify the test and nix these three lines.

I misunderstood the point of checking the file contents I think; I thought it was just to replace the test that the convex hull is taken.

In my mind, this test was that the convex is actually constructed correctly from the data passed in (that there is data about the vertices at all).

I'll remove it!

[Akshay5312]

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

geometry/test/shape_specification_test.cc line 685 at r8 (raw file):

Previously, Akshay5312 wrote…

I misunderstood the point of checking the file contents I think; I thought it was just to replace the test that the convex hull is taken.

In my mind, this test was that the convex is actually constructed correctly from the data passed in (that there is data about the vertices at all).

I'll remove it!

I'll still keep the tests for scale and filename, as those can fail even if the file contents are correct.

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @Akshay5312)

---

geometry/test/shape_specification_test.cc line 685 at r8 (raw file):

Previously, Akshay5312 wrote…

I'll still keep the tests for scale and filename, as those can fail even if the file contents are correct.

Exactly. Because the Convex constructor is responsible for making sure scale and label get propagated correctly into resulting Convex. So, confirming that those values are propagated fits perfectly in this unit test.