Add python wrapper for volume mesh refinement #23083

nepfaff · 2025-06-09T00:21:53Z

This change is

nepfaff · 2025-06-09T00:22:35Z

geometry/proximity/volume_mesh_refiner.h

@@ -125,5 +125,13 @@ class VolumeMeshRefiner {
 };

 }  // namespace internal


This seems slightly weird, but maybe the right way to do this?

In terms of having a file with one public function and many private functions? Yes, this is the typical way -- put the private functions in the internal namespace, and the public function(s) outside of it.

jwnimmer-tri

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

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

namespace drake {
namespace geometry {
namespace internal {

Most (probably all) of these "write to file" functions are unsuitable for making public. (For example, they use string for paths instead of std::filesystem::path.)

We could probably work on all of the fixes to make them suitable, but I don't think we actually need them?

The mesh refiner code can operate on bytes->bytes, using the contents of the files, and the downstream code can just Path(filename).write_bytes(content), etc. to get the bytes onto disk.

Ditto for the "read from file" half the functions in the other file.

geometry/proximity/volume_mesh_refiner.h line 133 at r1 (raw file):

//  @param mesh  The mesh to refine.
//  @return      The refined mesh, or the input mesh if no refinement was
//               needed.

nit For public functions, use /** ... */ doxygen style, not // non-doxygen style.

Suggestion:

/** Refines a tetrahedral mesh to eliminate problematic simplices.
@param mesh  The mesh to refine.
@return      The refined mesh, or the input mesh if no refinement was
             needed. */

jwnimmer-tri · 2025-06-09T22:15:10Z

geometry/proximity/volume_mesh_refiner.h

@@ -125,5 +125,13 @@ class VolumeMeshRefiner {
 };

 }  // namespace internal


In terms of having a file with one public function and many private functions? Yes, this is the typical way -- put the private functions in the internal namespace, and the public function(s) outside of it.

nepfaff

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

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

The mesh refiner code can operate on bytes->bytes, using the contents of the files

I can't see how this would work from looking at the code. All the refinement code operates on VolumeMesh, and converting .vtk to VolumeMesh is a multi-step process. Would you mind expanding a bit on how you would approach this?

jwnimmer-tri

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

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

Previously, nepfaff (Nicholas Pfaff) wrote…

The mesh refiner code can operate on bytes->bytes, using the contents of the files

I can't see how this would work from looking at the code. All the refinement code operates on VolumeMesh, and converting .vtk to VolumeMesh is a multi-step process. Would you mind expanding a bit on how you would approach this?

To be clear, I meant that RefineVolumeMesh could (should) operate on bytes -> bytes. But after a little bit more thought, I think that we should actually keep the VolumeMesh -> VolumeMesh function you have already, but then overload it for bytes -> bytes. And in fact, the input should be MeshSource not bytes so that the user can pass either a filename or a buffer.

Concretely, we should revert all of the "make load and save public" changes, and instead add a new overload to your new function, like this:

std::string RefineVolumeMesh(const MeshSource& mesh) {
  const VolumeMesh<double> original = ReadVtkToVolumeMesh(mesh);
  const VolumeMesh<double> refined = RefineVolumeMesh(original);
  return WriteVolumeMeshToVtkString(refined);
}

The only other new bit is that you need to create the new (internal) function WriteVolumeMeshToVtkString which is like WriteVolumeMeshToVtk but returns the string instead of outputting to a file. That should be a simple matter of changing std::ofstream to std::ostream in the implementation so that the same code can be reused for either files or buffers (with std::stringstream).

nepfaff

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

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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

To be clear, I meant that RefineVolumeMesh could (should) operate on bytes -> bytes. But after a little bit more thought, I think that we should actually keep the VolumeMesh -> VolumeMesh function you have already, but then overload it for bytes -> bytes. And in fact, the input should be MeshSource not bytes so that the user can pass either a filename or a buffer.

Concretely, we should revert all of the "make load and save public" changes, and instead add a new overload to your new function, like this:
std::string RefineVolumeMesh(const MeshSource& mesh) {
  const VolumeMesh<double> original = ReadVtkToVolumeMesh(mesh);
  const VolumeMesh<double> refined = RefineVolumeMesh(original);
  return WriteVolumeMeshToVtkString(refined);
}
The only other new bit is that you need to create the new (internal) function WriteVolumeMeshToVtkString which is like WriteVolumeMeshToVtk but returns the string instead of outputting to a file. That should be a simple matter of changing std::ofstream to std::ostream in the implementation so that the same code can be reused for either files or buffers (with std::stringstream).

Thank you for the detailed suggestion! I now implemented this approach, keeping the original RefineVolumeMesh in both C++ and python.

bindings/pydrake/geometry/geometry_py_common.cc line 390 at r2 (raw file):

  // Add RefineVolumeMesh overload for MeshSource
  m.def("RefineVolumeMesh",

It would be nice if this could be in geometry_py_meshes.cc to be with its overload, but I think it must be here? If it is correct to split these into separate files, then we should also split the test cases (currently in the same file).

jwnimmer-tri

Okay, I've gotten this off the ground, but to help me to stay focused on other work in my limited time, I'm going to pass the baton to +@SeanCurtis-TRI for feature review and advice for the remainder -@jwnimmer-tri.

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

bindings/pydrake/geometry/geometry_py_common.cc line 390 at r2 (raw file):

Previously, nepfaff (Nicholas Pfaff) wrote…

It would be nice if this could be in geometry_py_meshes.cc to be with its overload, but I think it must be here? If it is correct to split these into separate files, then we should also split the test cases (currently in the same file).

Yes, this shouldn't be in "common" for sure, and probably "meshes" is a good home. If there is some problem where there's not good home for it yet, it would probably be okay to make a new file "geometry_py_refine" if need be.

SeanCurtis-TRI

First pass done. Generally fine with some small details. I believe most of the things I don't like predate this PR.

Reviewed 1 of 12 files at r1, 14 of 14 files at r2, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @nepfaff)

geometry/proximity/volume_mesh_refiner.h line 140 at r2 (raw file):
nit: Copypasta.

Furthermore, this function desperately needs documentation explaining the string that is returned.

Returns a string containing the contents of an ascii VTK file which describes the refined tetrahedral mesh.

Code quote:

@return      The refined mesh, or the input mesh if no refinement was
             needed. */

bindings/pydrake/geometry/geometry_py_common.cc line 390 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Yes, this shouldn't be in "common" for sure, and probably "meshes" is a good home. If there is some problem where there's not good home for it yet, it would probably be okay to make a new file "geometry_py_refine" if need be.

Agreed. geometry_py_meshes.cc by preference, geometry_py_refine.cc if all else fails.

geometry/proximity/mesh_to_vtk.h line 35 at r2 (raw file):

/*
 Writes VolumeMesh to VTK string.

nit: I found "VTK string" to be a bit light; it's not doing much work. I come away wondering "what is a VTK string" (after all, there is such a thing as vtkString).

Suggestions:

Let's change the name to WriteVolumeMeshToVtkFileContents (I'm also ok if we want to throw out "file" and go with WriteVolumeMeshToVtkContents.
Document the return value as contents of a valid, ascii vtk file defining the equivalent tetrahedral mesh.

geometry/proximity/mesh_to_vtk.h line 37 at r2 (raw file):

 Writes VolumeMesh to VTK string.
 @param mesh       A tetrahedral volume mesh.
 @param title      Name of the data set will be written on the second line of

BTW This is not your fault, but this is a horrible piece of documentation. :) The fact that you've made this match the other two functions that describe title in the exact same way is proper. But, boy, that needs to be revisited if this ever comes out of internal name space. :)

geometry/proximity/refine_mesh.cc line 62 at r2 (raw file):

      internal::DetectInteriorEdgeWithAllBoundaryVertices(mesh);

  drake::log()->info(

BTW The standalone application has lost the ability to report on diagnostic information (x bad tets, y bad faces, etc.) I think this is regrettable.

I poked at it briefly, the typical Drake pattern for resolving it would be to have an internal function that does the work and generates messages and have the new public API use that method and discard the report and have this application use the report.

That said, the exact semantics of what message gets printed with what result of considering refinement is awkward.

If you feel ambitious and want to maintain this message, go for it. But if you don't want to be bothered, I can accept that outcome.

geometry/proximity/mesh_to_vtk.cc line 142 at r2 (raw file):

  std::stringstream ss;
  WriteMeshToVtk(ss, mesh, title);
  return ss.str();

nit: I don't believe we get copy elision for free. We'll need to help it.

Suggestion:

return std::move(ss).str();

geometry/proximity/volume_mesh_refiner.h line 131 at r2 (raw file):

}  // namespace internal

/** Refines a tetrahedral mesh to eliminate problematic simplices.

nit: Although these functions don't do any explicit effort to validate or throw, I feel the documentation of the public methods should document the conditions that would cause an exception to be thrown. Especially as the code that does do all the complaining is internal and would not otherwise be discoverable.

geometry/proximity/volume_mesh_refiner.h line 133 at r2 (raw file):

/** Refines a tetrahedral mesh to eliminate problematic simplices.
@param mesh  The mesh to refine.
@return      The refined mesh, or the input mesh if no refinement was

nit: More precision

Suggestion:

or a copy of the input mesh

geometry/proximity/volume_mesh_refiner.h line 141 at r2 (raw file):

@return      The refined mesh, or the input mesh if no refinement was
             needed. */
std::string RefineVolumeMesh(const MeshSource& mesh);

I don't like the "overload" flavor of these two functions. This function

Has the probability of being passed the wrong kind of mesh as a MeshSource. That's impossible for the first overload. So, that needs to be spelled out in documentation.
The return type is so different. Although they are both, ultimately, representations of volume meshes, they are such radically different representations of the tet mesh, that they really aren't as close as the overloaded names suggest. It'd be like the two functions where one returns a double and the other returns a string representation of the double. Both numerical values, but you can't remotely do the same operations.

I think this is most easily resolved by changing the name of this function (and providing the supporting documentation).

How about RefineVolumeMeshIntoVktString() or something like that?

(At the very least, it simplifies the bindings. :D)

bindings/pydrake/geometry/geometry_py_common.cc line 392 at r2 (raw file):

  m.def("RefineVolumeMesh",
      py::overload_cast<const MeshSource&>(&RefineVolumeMesh), py::arg("mesh"),
      doc.RefineVolumeMesh.doc);

nit: IN the bindings you'll have to change the reference to documentation as the two overloads will have very different documentation.

jwnimmer-tri

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

geometry/proximity/volume_mesh_refiner.h line 141 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I don't like the "overload" flavor of these two functions. This function

Has the probability of being passed the wrong kind of mesh as a MeshSource. That's impossible for the first overload. So, that needs to be spelled out in documentation.

The return type is so different. Although they are both, ultimately, representations of volume meshes, they are such radically different representations of the tet mesh, that they really aren't as close as the overloaded names suggest. It'd be like the two functions where one returns a double and the other returns a string representation of the double. Both numerical values, but you can't remotely do the same operations.

I think this is most easily resolved by changing the name of this function (and providing the supporting documentation).

How about RefineVolumeMeshIntoVktString() or something like that?

(At the very least, it simplifies the bindings. :D)

BTW Sean asked me about this f2f, and I support renaming it. Overloading was probably a bridge too far.

nepfaff

Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/proximity/mesh_to_vtk.h line 35 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I found "VTK string" to be a bit light; it's not doing much work. I come away wondering "what is a VTK string" (after all, there is such a thing as vtkString).

Suggestions:

Let's change the name to WriteVolumeMeshToVtkFileContents (I'm also ok if we want to throw out "file" and go with WriteVolumeMeshToVtkContents.

Document the return value as contents of a valid, ascii vtk file defining the equivalent tetrahedral mesh.

done

geometry/proximity/mesh_to_vtk.h line 37 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This is not your fault, but this is a horrible piece of documentation. :) The fact that you've made this match the other two functions that describe title in the exact same way is proper. But, boy, that needs to be revisited if this ever comes out of internal name space. :)

Should I change this to anything else? Maybe "title" is the wrong name.

geometry/proximity/mesh_to_vtk.cc line 142 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I don't believe we get copy elision for free. We'll need to help it.

done

geometry/proximity/refine_mesh.cc line 62 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW The standalone application has lost the ability to report on diagnostic information (x bad tets, y bad faces, etc.) I think this is regrettable.

I poked at it briefly, the typical Drake pattern for resolving it would be to have an internal function that does the work and generates messages and have the new public API use that method and discard the report and have this application use the report.

That said, the exact semantics of what message gets printed with what result of considering refinement is awkward.

If you feel ambitious and want to maintain this message, go for it. But if you don't want to be bothered, I can accept that outcome.

Do you mean that the internal function should return a tuple of (result, print string)?

Considering that this is a helper script that probably isn't speed-critical, I added the logging back in. This will now result in duplicated computations, but maybe this doesn't matter for this interface. Happy to remove it again if you think this is bad practice.

geometry/proximity/volume_mesh_refiner.h line 131 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Although these functions don't do any explicit effort to validate or throw, I feel the documentation of the public methods should document the conditions that would cause an exception to be thrown. Especially as the code that does do all the complaining is internal and would not otherwise be discoverable.

done . Let me know if you think I should change the wording of these conditions.

geometry/proximity/volume_mesh_refiner.h line 133 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: More precision

done

geometry/proximity/volume_mesh_refiner.h line 140 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Copypasta.

Furthermore, this function desperately needs documentation explaining the string that is returned.

Returns a string containing the contents of an ascii VTK file which describes the refined tetrahedral mesh.

done, using the same terminology for the other function

geometry/proximity/volume_mesh_refiner.h line 141 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Sean asked me about this f2f, and I support renaming it. Overloading was probably a bridge too far.

done. I used RefineVolumeMeshIntoVtkFileContents to match the suggested WriteVolumeMeshToVtkFileContents

bindings/pydrake/geometry/geometry_py_common.cc line 390 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Agreed. geometry_py_meshes.cc by preference, geometry_py_refine.cc if all else fails.

meshes doesn't work due to the circular dependence with common. I created the refine file.

bindings/pydrake/geometry/geometry_py_common.cc line 392 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: IN the bindings you'll have to change the reference to documentation as the two overloads will have very different documentation.

done (no longer overloads)

nepfaff · 2025-06-11T22:38:44Z

geometry/proximity/volume_mesh_refiner.h

@@ -125,5 +125,13 @@ class VolumeMeshRefiner {
 };

 }  // namespace internal


SeanCurtis-TRI

Reviewed 13 of 14 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @nepfaff)

geometry/proximity/mesh_to_vtk.h line 37 at r2 (raw file):

Previously, nepfaff (Nicholas Pfaff) wrote…

Should I change this to anything else? Maybe "title" is the wrong name.

Oh, sorry. No. You don't have to do anything. That was just me complaining. :)

geometry/proximity/refine_mesh.cc line 62 at r2 (raw file):

Previously, nepfaff (Nicholas Pfaff) wrote…

Do you mean that the internal function should return a tuple of (result, print string)?

Considering that this is a helper script that probably isn't speed-critical, I added the logging back in. This will now result in duplicated computations, but maybe this doesn't matter for this interface. Happy to remove it again if you think this is bad practice.

I'm content with you adding it back in. That's certainly simple.

You might go ahead and add a TODO that says we know this work is redundant (in place simply to be able to print the message), and the TODO would be to eliminate the redundant computation.

geometry/proximity/volume_mesh_refiner.h line 131 at r2 (raw file):

Previously, nepfaff (Nicholas Pfaff) wrote…

done . Let me know if you think I should change the wording of these conditions.

See note at the comments.

bindings/pydrake/geometry/geometry_py_refine.cc line 21 at r4 (raw file):

  m.def("RefineVolumeMeshIntoVtkFileContents",
      &RefineVolumeMeshIntoVtkFileContents, py::arg("mesh"),

nit: Let's change mesh to mesh_source.

geometry/proximity/volume_mesh_refiner.h line 135 at r4 (raw file):

@return      The refined mesh, or a copy of the input mesh if no refinement
             was needed.
@throws std::exception if:

General question: can you tell me which code is throwing these exceptions? I don't find any throw statements in volume_mesh_refiner.cc or detect_zero_simplex.cc.

We certainly don't have any tests that show the exceptions throw.

I've got issues about some of the line items as well, but first I wanted to confirm the provenance of these exceptions.

nepfaff

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

geometry/proximity/refine_mesh.cc line 62 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I'm content with you adding it back in. That's certainly simple.

You might go ahead and add a TODO that says we know this work is redundant (in place simply to be able to print the message), and the TODO would be to eliminate the redundant computation.

done

geometry/proximity/volume_mesh_refiner.h line 131 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

See note at the comments.

done

geometry/proximity/volume_mesh_refiner.h line 135 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

General question: can you tell me which code is throwing these exceptions? I don't find any throw statements in volume_mesh_refiner.cc or detect_zero_simplex.cc.

We certainly don't have any tests that show the exceptions throw.

I've got issues about some of the line items as well, but first I wanted to confirm the provenance of these exceptions.

I'm referring to the DRAKE_THROW_UNLESS statements in volume_mesh_refiner.cc. Were these the ones that you meant with your original comment?

bindings/pydrake/geometry/geometry_py_refine.cc line 21 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Let's change mesh to mesh_source.

done

SeanCurtis-TRI · 2025-06-12T20:16:03Z

geometry/proximity/refine_mesh.cc line 62 at r2 (raw file):
BTW TODOs should generally include an action. What you have is the preamble.

E.g.,

Computing the statistics is redundant and only here for printing the diagnostics message. Change the call to RefineVolumeMesh() so that it returns this diagnostic information.

nepfaff

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

geometry/proximity/refine_mesh.cc line 62 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW TODOs should generally include an action. What you have is the preamble.

E.g.,

Computing the statistics is redundant and only here for printing the diagnostics message. Change the call to RefineVolumeMesh() so that it returns this diagnostic information.

done

SeanCurtis-TRI · 2025-06-12T21:19:22Z

geometry/proximity/volume_mesh_refiner.h line 135 at r4 (raw file):

Previously, nepfaff (Nicholas Pfaff) wrote…

I'm referring to the DRAKE_THROW_UNLESS statements in volume_mesh_refiner.cc. Were these the ones that you meant with your original comment?

Ha....I'd gone looking for throw but not DRAKE_THROW_UNLESS. Silly me.

Nevertheless, we need to revisit the error messages. They should be couched in terms of the parameter mesh. If an exception gets thrown strictly to bookeeping problems, that isn't part of the documentation.

Having looked through the code, most of the DRAKE_THROW_UNLESS would really be better as DRAKE_DEMAND because they are simply invariants within the bookkeeping of the algorithm.

That said, there is one throw condition I can see that depends on the input:

If a tet isn't defined based on four unique vertices (i.e., if one vertex is replicated), then this algorithm might throw. (To throw, it must also have all of its vertices on the boundary.)

For this, I'd simply state the prereq that the mesh is a "valid mesh".

SeanCurtis-TRI · 2025-06-12T21:19:27Z

geometry/proximity/volume_mesh_refiner.h line 143 at r5 (raw file):

/** Refines a tetrahedral mesh to eliminate problematic simplices.
@param mesh_source  The mesh to refine.

nit: This function has a different set of error conditions:

If mesh_source specifies an invalid tet mesh.
If mesh_source specifies an inaccessible tet mesh.
If mesh_source specifies a file that isn't a tet mesh.

In other words:

@throws std::exception if `mesh_source` does not reference a valid VTK-formatted
                       tetrahedral mesh.

I think that would be sufficient.

SeanCurtis-TRI

Reviewed 5 of 5 files at r5.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @nepfaff)

SeanCurtis-TRI

Sorry for the piecemeal review -- Reviewable has been rejecting comments until just recently.

We're definitely on the last step and I'm tagging this in anticipation of its completion.

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

SeanCurtis-TRI

+@sammy-tri for platform review, come Monday.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @nepfaff)

nepfaff

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/proximity/volume_mesh_refiner.h line 135 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ha....I'd gone looking for throw but not DRAKE_THROW_UNLESS. Silly me.

Nevertheless, we need to revisit the error messages. They should be couched in terms of the parameter mesh. If an exception gets thrown strictly to bookeeping problems, that isn't part of the documentation.

Having looked through the code, most of the DRAKE_THROW_UNLESS would really be better as DRAKE_DEMAND because they are simply invariants within the bookkeeping of the algorithm.

That said, there is one throw condition I can see that depends on the input:

If a tet isn't defined based on four unique vertices (i.e., if one vertex is replicated), then this algorithm might throw. (To throw, it must also have all of its vertices on the boundary.)

For this, I'd simply state the prereq that the mesh is a "valid mesh".

I see. That makes sense. Thank you for looking into this!

geometry/proximity/volume_mesh_refiner.h line 143 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This function has a different set of error conditions:

If mesh_source specifies an invalid tet mesh.

If mesh_source specifies an inaccessible tet mesh.

If mesh_source specifies a file that isn't a tet mesh.

In other words:
@throws std::exception if `mesh_source` does not reference a valid VTK-formatted
                       tetrahedral mesh.
I think that would be sufficient.

done Thanks!

SeanCurtis-TRI

Reviewed 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @nepfaff)

Add python wrapper for volume mesh refinement

5955c38

nepfaff commented Jun 9, 2025

View reviewed changes

jwnimmer-tri self-assigned this Jun 9, 2025

jwnimmer-tri added the release notes: feature This pull request contains a new feature label Jun 9, 2025

jwnimmer-tri reviewed Jun 9, 2025

View reviewed changes

nepfaff commented Jun 10, 2025

View reviewed changes

jwnimmer-tri reviewed Jun 10, 2025

View reviewed changes

nepfaff added 2 commits June 10, 2025 16:05

Revert make load and save public changes

3ff2d58

Add RefineVolumeMesh string overload

2c7ca8e

nepfaff commented Jun 10, 2025

View reviewed changes

jwnimmer-tri assigned SeanCurtis-TRI and unassigned jwnimmer-tri Jun 10, 2025

jwnimmer-tri reviewed Jun 10, 2025

View reviewed changes

SeanCurtis-TRI reviewed Jun 11, 2025

View reviewed changes

jwnimmer-tri reviewed Jun 11, 2025

View reviewed changes

Address comments

598af1f

nepfaff commented Jun 11, 2025

View reviewed changes

geometry/proximity/volume_mesh_refiner.h

@@ -125,5 +125,13 @@ class VolumeMeshRefiner {

};

} // namespace internal

Copy link

Member Author

nepfaff Jun 11, 2025

Choose a reason for hiding this comment

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

done

Remove unused import

073710f

SeanCurtis-TRI reviewed Jun 11, 2025

View reviewed changes

Address comments

8c5295d

nepfaff commented Jun 12, 2025

View reviewed changes

Improve TODO

dbbfdf8

nepfaff commented Jun 12, 2025

View reviewed changes

SeanCurtis-TRI reviewed Jun 12, 2025

View reviewed changes

SeanCurtis-TRI assigned sammy-tri Jun 12, 2025

SeanCurtis-TRI reviewed Jun 12, 2025

View reviewed changes

Improve throw comments

cdf0efc

nepfaff commented Jun 12, 2025

View reviewed changes

SeanCurtis-TRI reviewed Jun 12, 2025

View reviewed changes

		@@ -125,5 +125,13 @@ class VolumeMeshRefiner {
		};

		} // namespace internal

Add python wrapper for volume mesh refinement #23083

Are you sure you want to change the base?

Add python wrapper for volume mesh refinement #23083

Uh oh!

Conversation

nepfaff commented Jun 9, 2025 • edited by sherm1 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nepfaff Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

nepfaff Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri left a comment

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

nepfaff left a comment

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri left a comment

Choose a reason for hiding this comment

Uh oh!

nepfaff left a comment

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri left a comment

Choose a reason for hiding this comment

Uh oh!

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

Uh oh!

jwnimmer-tri left a comment

Choose a reason for hiding this comment

Uh oh!

nepfaff left a comment

Choose a reason for hiding this comment

Uh oh!

nepfaff Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

Uh oh!

nepfaff left a comment

Choose a reason for hiding this comment

Uh oh!

SeanCurtis-TRI commented Jun 12, 2025

Uh oh!

nepfaff left a comment

Choose a reason for hiding this comment

Uh oh!

SeanCurtis-TRI commented Jun 12, 2025

Uh oh!

SeanCurtis-TRI commented Jun 12, 2025

Uh oh!

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

Uh oh!

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

Uh oh!

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

Uh oh!

nepfaff left a comment

Choose a reason for hiding this comment

Uh oh!

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nepfaff commented Jun 9, 2025 •

edited by sherm1

Loading