Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SceneGraph provides the convex hull for visualization #21061

Merged

Conversation

SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Feb 27, 2024

  • GeometryState computes the convex hull (and reports it).
  • InternalGeometry stores the convex hull.
  • MeshcatVisualizer uses the convex hull when appropriate.
  • ProximityEngine reports if it would use a convex hull.
  • SceneGraphInspector reports possible convex hull.
  • New code converts from polygonal surface mesh to triangle surface mesh.
  • update box.sdf to include collision geometry.

resolves #20618


This change is Reviewable

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@zachfang you up for feature review on this one? It touches a number of places, so there's no obvious feature reviewer, so I'm adopting a "spread-the-love" approach.

Reviewable status: LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)

@zachfang
Copy link
Contributor

Sounds good 👀.

@SeanCurtis-TRI
Copy link
Contributor Author

Thanks

@zachfang
Copy link
Contributor

FYI. CI is not happy with this error message.

[5:00:00 PM]  unknown file: Failure
[5:00:00 PM]  C++ exception with description "QH6154 Qhull precision error: Initial simplex is flat (facet 1 is coplanar with the interior point)

Also, do you already have a command I can run to see the effect of the PR changes in Meldis?

@jwnimmer-tri jwnimmer-tri added the release notes: feature This pull request contains a new feature label Feb 28, 2024
Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+(status: do not review)

When I've resolved the CI issues, I'll come back with a recipe for you to observe the results.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers

a discussion (no related file):
Blocking while I resolve an overlooked issue: when a mesh has been created that is intentionally an open planar mesh, qhull will fail. However, that's a perfectly valid rigid hydroelastic surface. I'll have to decide on a protocol for this and come back to it.


Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Blocking while I resolve an overlooked issue: when a mesh has been created that is intentionally an open planar mesh, qhull will fail. However, that's a perfectly valid rigid hydroelastic surface. I'll have to decide on a protocol for this and come back to it.

This now depends on #21066. Once that merges, I'll rebase here and we can continue.


Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-(status: do not review) Alrighty, we're ready for review again. This PR hasn't changed, but the baseline code has gotten more robust.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers

@SeanCurtis-TRI
Copy link
Contributor Author

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This now depends on #21066. Once that merges, I'll rebase here and we can continue.

All ready to go.

Copy link
Contributor

@zachfang zachfang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpoint before reviewing the tests.

Reviewed 9 of 19 files at r1, 4 of 5 files at r2.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee zachfang, needs platform reviewer assigned, needs at least two assigned reviewers


geometry/proximity/polygon_to_triangle_mesh.h line 11 at r2 (raw file):

/** Creates a triangle mesh from a polygon mesh. To whatever degree the polygon
 mesh has duplicate vertices, the resulting triangle mesh will have the same.

BTW. Does this essentially mean the number of vertices will be the same?

Code quote:

/** Creates a triangle mesh from a polygon mesh. To whatever degree the polygon
 mesh has duplicate vertices, the resulting triangle mesh will have the same.

geometry/proximity/test/polygon_to_triangle_mesh_test.cc line 31 at r2 (raw file):

GTEST_TEST(PolyToTriMeshTest, BasicSmokeTest) {
  // clang-format off
  std::vector<Vector3d> vertices{

nit.

Suggestion:

const std::vector<Vector3d> vertices

Copy link
Contributor

@zachfang zachfang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm_strong: with a question about showing hydro vs. convex hull.

Reviewed 5 of 19 files at r1, 1 of 5 files at r2.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers


geometry/meshcat_visualizer.cc line 231 at r2 (raw file):

      // Proximity role favors convex hulls if available.
      if (const PolygonSurfaceMesh<double>* hull = nullptr;

BTW. (This is the part I am a bit fuzzy). If show_hydroelastic flag is set, should we still display the convex hull rather than the VolumeMesh? I would assume this section will be guarded with if (!geometry_already_set) too.

Code quote:

if (const PolygonSurfaceMesh<double>* hull = nullptr;

geometry/test/meshcat_visualizer_test.cc line 437 at r2 (raw file):

  ASSERT_EQ(inspector.GetShape(box_id).type_name(), "Mesh");

  // Add a proximity visualizer, with or without hydro.

nit. If I understand correctly, we only test show_hydroelastic=falsehere?

Code quote:

// Add a proximity visualizer, with or without hydro.

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye, @zachfang. There's one last open issue, take a look and let me know what you'd prefer.

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers


geometry/meshcat_visualizer.cc line 231 at r2 (raw file):

Previously, zachfang wrote…

BTW. (This is the part I am a bit fuzzy). If show_hydroelastic flag is set, should we still display the convex hull rather than the VolumeMesh? I would assume this section will be guarded with if (!geometry_already_set) too.

You are 100% correct. This should also be conditioned on geometry_already_set.


geometry/test/meshcat_visualizer_test.cc line 437 at r2 (raw file):

Previously, zachfang wrote…

nit. If I understand correctly, we only test show_hydroelastic=falsehere?

Good catch. I've tweaked the test a bit. I've made the actual completeness of the test clearer.

Let me know if you want me to find a way to cover the missing case.

Copy link
Contributor

@zachfang zachfang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM feature. +@ggould-tri for platform review, thanks!

Reviewed 4 of 4 files at r3.
Reviewable status: LGTM missing from assignee ggould-tri(platform)

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggould-tri to test the feature you have a couple of options, your best involves a slight tweak:

  1. Open examples/hydroelastic/python_nonconvex_mesh/bowl.sdf
  2. Comment out line 39 (<drake:rigid_hydroelastic/>)
  3. Run bazel run //tools:model_visualizer_private -- package://drake/examples/hydroelastic/python_nonconvex_mesh/bowl.sdf
  4. Open the presented URL (e.g., http://localhost:7000)
  5. Open the controls.
  6. Open the drake branch and make the proximity group visible (if you're using meldis, the proximity group will be named something else.
  7. Possibly make the illustration group invisible.
  8. You should see the bowls' convex hull, made most apparent by the closing of the top of the bowl.

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

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 10 of 19 files at r1, 5 of 5 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: 3 unresolved discussions


geometry/meshcat_visualizer.cc line 207 at r3 (raw file):

      // The "object" will typically be the geometry's shape. But, for various
      // reasons, we may use a different representation.

minor: "Various reasons" is extremely unhelpful to a later reader. Can you be more clear?

Code quote:

      // The "object" will typically be the geometry's shape. But, for various
      // reasons, we may use a different representation.

geometry/proximity/polygon_to_triangle_mesh.cc line 24 at r3 (raw file):

  std::vector<SurfaceTriangle> tris;
  // A lower limit on the number of triangles required.
  tris.reserve(poly_mesh.num_faces());

BTW: Consider reserving (2 * num_vertices) - 4. This is an upper bound on the number of triangles in a trimesh via Euler's formula, and will be exact for closed meshes of genus zero, which is all of the meshes this is called on, so the excess space used will be small or zero and you will be sure never to reallocate.


geometry/proximity/test/polygon_to_triangle_mesh_test.cc line 83 at r3 (raw file):

    /* Note: This doesn't confirm that the two triangles properly cover the
     polygon, nor that the winding is in the right direction. */

BTW: You could trivially check the winding by checking that face_normal of the result faces matches that of the original face, I think?

 - GeometryState computes the convex hull (and reports it).
 - InternalGeometry stores the convex hull.
 - MeshcatVisualizer uses the convex hull when appropriate.
 - ProximityEngine reports if it would use a convex hull.
 - SceneGraphInspector reports possible convex hull.
 - New code converts from polygonal surface mesh to triangle surface mesh.
 - update box.sdf to include collision geometry.
@SeanCurtis-TRI SeanCurtis-TRI merged commit 67589d8 into RobotLocomotion:master Mar 6, 2024
10 checks passed
@SeanCurtis-TRI SeanCurtis-TRI deleted the PR_convex_hull_train branch March 6, 2024 13:44
jwnimmer-tri pushed a commit to jwnimmer-tri/drake that referenced this pull request Apr 1, 2024
…otion#21061)

 - GeometryState computes the convex hull (and reports it).
 - InternalGeometry stores the convex hull.
 - MeshcatVisualizer uses the convex hull when appropriate.
 - ProximityEngine reports if it would use a convex hull.
 - SceneGraphInspector reports possible convex hull.
 - New code converts from polygonal surface mesh to triangle surface mesh.
 - update box.sdf to include collision geometry.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature
Projects
None yet
4 participants