Improve exception message for bad geometry in .obj file.

[mitiguy]

This PR resolves issue #21924. It creates a more coherent error message when the volume calculated from geometry in an .obj file is not reasonably positive.

Resolves #21924

Note to release engineer:

This PR is a fix to a broken contract. This PR is not a breaking change.

Why? Before this PR, the documentation in the file in geometry_spatial_inertia.h for the function

SpatialInertia<double> CalcSpatialInertia(const geometry::TriangleSurfaceMesh<double>& mesh, double density);

included:

If these requirements are not met, a value *will* be returned, but its value is meaningless.

Before this PR, a values was not returned if the volume was negative or zero. Instead, a not-so-helpful exception was thrown (e.g., with a message about eigenvalues of a 3x3 matrix)..

---

This change is 

[mitiguy]

Feature review +@SeanCurtis-TRI

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

[SeanCurtis-TRI]

Can you elaborate what part of #21924 this doesn't address? The way that the issue is phrased, this seems to completely resolve it.

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

---

geometry/test/bad_geometryA.obj line 0 at r1 (raw file):
Given the error message is conditioned solely on positive volume, we could use a simpler mesh. What is the point of the more elaborate mesh? (And, yes, I know six vertices and eight faces hardly constitute "elaborate".) But this mesh is easier to understand and easier to correct (simply invert the order of vertex indices in the single face).

# When used to compute spatial inertia, the single triangle (with implied normal
# pointing towards the origin) will produce a negative volume.

v 0 0 1
v 1 0 1
v 0 1 1
f 3 2 1

---

multibody/tree/geometry_spatial_inertia.cc line 134 at r1 (raw file):

  const double volume = vol_times_six / 6.0;
  constexpr double kEpsilon = std::numeric_limits<double>::epsilon();
  if (volume <= kEpsilon) {

This test here leaves a space of masses that this function chokes on but SpatialInertia accepts -- SpatialInertia only requires strictly greater than zero. Is there a reason to make this function pickier than SpatialInertia?

Plus, this only addresses mass. There are other ways for this to be invalid.

I'd suggest the following alternative formulation:

1. Go ahead and compute the SpatialInertia below, but disable validation upon construction.
2. Explicitly call IsPhysicallyValid() on the constructed inertia.
3. If it returns true, return the inertia, otherwise throw an exception.

There are several benefits to this approach:

1. it gives you the opportunity to provide a "computing inertia from obj"-specific error message,
2. it doesn't redefine mesh validity (deferring to SpatialInertia to define that).
3. Testing becomes easier; we don't have to come up with different .obj files that could create invalid inertias in various ways. We only need one that shows that if SpatialInertia considers the result invalid, we throw appropriately.

---

multibody/tree/geometry_spatial_inertia.cc line 136 at r1 (raw file):

  if (volume <= kEpsilon) {
    const std::string error_message = fmt::format(
        "{}(): The calculated volume of a triangle surface mesh is {} whereas "

BTW I suspect that what we could say and what we might want to say in order to be helpful in this case would not fit nicely in a single exception message.

For example, while this touches on winding, it doesn't address the question of open meshes. It also really can't accomplish anything pedagogical on how to go about correcting, etc.

This seems like it's a great candidate for https://drake.mit.edu/troubleshooting.html. In that case, you put a terse message about the mesh isn't suitable for computing inertia, and then give a proper treatment of the issue in trouble shooting.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 165 at r1 (raw file):

// Check exception messages when CalcSpatialInertia(Shape) is called on a file
// with bad geometry (e.g. not watertight or bad outward normals, or ...).

nit: This "e.g." is imprecise and misleading. All you're really testing for is "reasonably positive mass". But this becomes simpler when we change the failure condition. Now, this can simply say it's testing for dispatching an appropriate error message for an obj's physically invalid spatial inertia.

Code quote:

e.g. not watertight or bad outward normals, or ...

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 168 at r1 (raw file):

GTEST_TEST(GeometrySpatialInertiaTest, ExceptionOnBadGeometry) {
  std::string geometry_file_path = FindResourceOrThrow(
      "drake/geometry/test/bad_geometryA.obj");

nit bad_geometry_A.obj and bad_geometryB.obj seem to have exactly the same comment. Therefore, I have no basis for assessing why the second one is necessary. I need to you to better articulate the point of the two meshes or remove one of them.

What does bad_geometry_B.obj reveal about our code that bad_geometry_A.obj didn't?

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 181 at r1 (raw file):

      "positive value was expected. The mesh may have bad geometry.*");

  geometry_file_path = FindResourceOrThrow(

Is there true value in having bad_geometry_corrected.obj? Surely we already have tests that show that a proper mesh produces proper mass properties? If there is value here, it is in showing that a bad mesh can be changed into a good mesh and that doesn't really seem worth Drake's CI time.

[mitiguy]

I updated the top-level discussion so it now signals that this PR resolves the issue.

Reviewable status: 6 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 @mitiguy)

---

geometry/test/bad_geometryA.obj line at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Given the error message is conditioned solely on positive volume, we could use a simpler mesh. What is the point of the more elaborate mesh? (And, yes, I know six vertices and eight faces hardly constitute "elaborate".) But this mesh is easier to understand and easier to correct (simply invert the order of vertex indices in the single face).

# When used to compute spatial inertia, the single triangle (with implied normal
# pointing towards the origin) will produce a negative volume.

v 0 0 1
v 1 0 1
v 0 1 1
f 3 2 1

I started with the mesh that was submitted and modified it to get different results (negative volume, zero volume, positive volume).

A possible way to incorporate your suggestion: I'll modify the tests to start with your proposed super-simple test and then do the slightly more complicated ones too. The run times on these must be short. And a mesh with a single face is so degenerate, it may be too obvious.

---

multibody/tree/geometry_spatial_inertia.h line 54 at r2 (raw file):

 @pydrake_mkdoc_identifier{mesh} */
SpatialInertia<double> CalcSpatialInertia(
    const geometry::TriangleSurfaceMesh<double>& mesh, double density);

Self blocking: Add optional argument for mesh name (filename of mesh).

---

multibody/tree/geometry_spatial_inertia.cc line 134 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This test here leaves a space of masses that this function chokes on but SpatialInertia accepts -- SpatialInertia only requires strictly greater than zero. Is there a reason to make this function pickier than SpatialInertia?

Plus, this only addresses mass. There are other ways for this to be invalid.

I'd suggest the following alternative formulation:

1. Go ahead and compute the SpatialInertia below, but disable validation upon construction.
2. Explicitly call IsPhysicallyValid() on the constructed inertia.
3. If it returns true, return the inertia, otherwise throw an exception.

There are several benefits to this approach:

1. it gives you the opportunity to provide a "computing inertia from obj"-specific error message,
2. it doesn't redefine mesh validity (deferring to SpatialInertia to define that).
3. Testing becomes easier; we don't have to come up with different .obj files that could create invalid inertias in various ways. We only need one that shows that if SpatialInertia considers the result invalid, we throw appropriately.

The 3 files correspond to a mesh that has zero volume, a mesh with negative volume, and a mesh that has the same vertices as the other two, but with appropriate volume and spatial inertia. The zero volume mesh is a good error detection as a zero mass can pass the test in SpatialInertia::IsPhysicallyValid() whereas a zero volume should fail.

I like your general approach (particularly with the enhanced "computing inertia from obj" specific error message, but I am unsure how it spatial inertia would know anything about the volume (and handle the zero volume case). If this expanded task is what we want to do, let's consider this in a subsequent PR so others can weigh in on whether it is helpful.

---

multibody/tree/geometry_spatial_inertia.cc line 136 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I suspect that what we could say and what we might want to say in order to be helpful in this case would not fit nicely in a single exception message.

For example, while this touches on winding, it doesn't address the question of open meshes. It also really can't accomplish anything pedagogical on how to go about correcting, etc.

This seems like it's a great candidate for https://drake.mit.edu/troubleshooting.html. In that case, you put a terse message about the mesh isn't suitable for computing inertia, and then give a proper treatment of the issue in trouble shooting.

In terms of giving a "proper treatment" of the issue, you are more the world expert on how many different ways a mesh can be mismanaged, whereas I know my fair share of mass properties. For now, I prefer Kaizen with an in-situ error message on volume that is as instructive as possible. It would be helpful to get your best wording for the message as is. If we want to take on the troubleshooting.hmtl, we can modify later and point to there.

FYI: Result of my Google search (AI) on "what is an invalid 3d mesh".

An invalid 3D mesh can have a variety of issues, including: 

• Holes or triangles in the wrong direction

  Check the direction of the blue lines in the normal vector display to see if the triangles are oriented correctly. If the lines point inside the part, the mesh has an invalid orientation. 

• Surface defects

  These can include free edges, non-manifolding edges, or overlaps and intersections. 

• Interference between meshes

  If the mesh of a cooling line interferes with the mesh of a mold insert, the 3D mesh generation will fail. 

• Problems with boundary edges

  These can be indicated by node labels in Moldflow. 

Other ways to identify issues with a 3D mesh include:

• Slicing the part and checking the 3D preview for missing or filled regions 

A 3D mesh is a collection of polygons and vertices that defines the shape of a 3D object. 3D meshes are used in many ways, including CGI in movies, computer games, and 3D printing.

---

multibody/tree/geometry_spatial_inertia.cc line 145 at r2 (raw file):

        "a reasonable positive value was expected. The mesh may have bad "
        "geometry, e.g., it is an open mesh or the winding (order of vertices) "
        "of one or more faces do not produce outward normals.",

Self blocking. do not product -> does not produce.
Also, request Sean for an improved message.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 165 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This "e.g." is imprecise and misleading. All you're really testing for is "reasonably positive mass". But this becomes simpler when we change the failure condition. Now, this can simply say it's testing for dispatching an appropriate error message for an obj's physically invalid spatial inertia.

Done -- language improved.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 168 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit bad_geometry_A.obj and bad_geometryB.obj seem to have exactly the same comment. Therefore, I have no basis for assessing why the second one is necessary. I need to you to better articulate the point of the two meshes or remove one of them.

What does bad_geometry_B.obj reveal about our code that bad_geometry_A.obj didn't?

Done. Comments modified.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 181 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Is there true value in having bad_geometry_corrected.obj? Surely we already have tests that show that a proper mesh produces proper mass properties? If there is value here, it is in showing that a bad mesh can be changed into a good mesh and that doesn't really seem worth Drake's CI time.

I find it helpful to have the corrected geometry to how to fix the bad geometry. And are we talking about milliseconds or something significantly more?

Perhaps I could cut/paste the good-geometry in a comment below the bad-geometry in each file and then delete the corrected geometry file. Thoughts?

[jwnimmer-tri]

Reviewable status: 6 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 @mitiguy)

---

geometry/test/bad_geometryA.obj line at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

I started with the mesh that was submitted and modified it to get different results (negative volume, zero volume, positive volume).

A possible way to incorporate your suggestion: I'll modify the tests to start with your proposed super-simple test and then do the slightly more complicated ones too. The run times on these must be short. And a mesh with a single face is so degenerate, it may be too obvious.

Every single line of test code and sample input comes with an ongoing cost of upkeep. We should aim to write the minimum amount of lines that gets the job done, while maintaining readability, clarity, etc. More is not always better, often the opposite.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 181 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

I find it helpful to have the corrected geometry to how to fix the bad geometry. And are we talking about milliseconds or something significantly more?

Perhaps I could cut/paste the good-geometry in a comment below the bad-geometry in each file and then delete the corrected geometry file. Thoughts?

It's not about the time it takes to run. It's about ongoing maintenance cost of copying the same file over and over again. Just like copy-pasting code is bad for maintenance, copy-pasting test data and making tiny inscrutable edits manually to each copy is likewise a problem for ongoing upkeep. There is absolutely no good reason to copy the file over an over again. We could trivially generate variations of good meshes on the fly, programatically in this test case, where we would just tweak a detail here and there without all of the copy-paste.

[sherm1]

-@SeanCurtis-TRI +@sherm1 for feature review

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

[sherm1]

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

---

geometry/test/bad_geometryA.obj line at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Every single line of test code and sample input comes with an ongoing cost of upkeep. We should aim to write the minimum amount of lines that gets the job done, while maintaining readability, clarity, etc. More is not always better, often the opposite.

BTW I prefer that the tests use the (small) bad mesh that was supplied by the OP in the original bugreport, with minor tweaks to demonstrate proper behavior. No need to simplify further IMO, this is good.

---

multibody/tree/geometry_spatial_inertia.h line 54 at r2 (raw file):

Previously, mitiguy (Mitiguy) wrote…

Self blocking: Add optional argument for mesh name (filename of mesh).

This proposed change seems out of scope for this PR which fixes the user-reported bad error message. Is there some reason it has to be here? If not, please handle separately.

---

multibody/tree/geometry_spatial_inertia.cc line 134 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

The 3 files correspond to a mesh that has zero volume, a mesh with negative volume, and a mesh that has the same vertices as the other two, but with appropriate volume and spatial inertia. The zero volume mesh is a good error detection as a zero mass can pass the test in SpatialInertia::IsPhysicallyValid() whereas a zero volume should fail.

I like your general approach (particularly with the enhanced "computing inertia from obj" specific error message, but I am unsure how it spatial inertia would know anything about the volume (and handle the zero volume case). If this expanded task is what we want to do, let's consider this in a subsequent PR so others can weigh in on whether it is helpful.

BTW I think it makes sense for the volume method to be more strict than the raw SpatialInertia constructor.

---

multibody/tree/geometry_spatial_inertia.cc line 136 at r1 (raw file):

Previously, mitiguy (Mitiguy) wrote…

In terms of giving a "proper treatment" of the issue, you are more the world expert on how many different ways a mesh can be mismanaged, whereas I know my fair share of mass properties. For now, I prefer Kaizen with an in-situ error message on volume that is as instructive as possible. It would be helpful to get your best wording for the message as is. If we want to take on the troubleshooting.hmtl, we can modify later and point to there.

FYI: Result of my Google search (AI) on "what is an invalid 3d mesh".

An invalid 3D mesh can have a variety of issues, including: 

• Holes or triangles in the wrong direction

  Check the direction of the blue lines in the normal vector display to see if the triangles are oriented correctly. If the lines point inside the part, the mesh has an invalid orientation. 

• Surface defects

  These can include free edges, non-manifolding edges, or overlaps and intersections. 

• Interference between meshes

  If the mesh of a cooling line interferes with the mesh of a mold insert, the 3D mesh generation will fail. 

• Problems with boundary edges

  These can be indicated by node labels in Moldflow. 

Other ways to identify issues with a 3D mesh include:

• Slicing the part and checking the 3D preview for missing or filled regions 

A 3D mesh is a collection of polygons and vertices that defines the shape of a 3D object. 3D meshes are used in many ways, including CGI in movies, computer games, and 3D printing.

BTW I agree an improved error message here is sufficient to address the presenting issue. The proposed message or one with minor additions as above seems likely to solve the OP's problem -- consider merging that, with possible upgrades & troubleshooting guide later if warranted.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 181 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It's not about the time it takes to run. It's about ongoing maintenance cost of copying the same file over and over again. Just like copy-pasting code is bad for maintenance, copy-pasting test data and making tiny inscrutable edits manually to each copy is likewise a problem for ongoing upkeep. There is absolutely no good reason to copy the file over an over again. We could trivially generate variations of good meshes on the fly, programatically in this test case, where we would just tweak a detail here and there without all of the copy-paste.

Agreed. This is fine as-is with no cut/paste duplication. IMO it is helpful to show the OP's bad geometry with beautiful new error message with the minimal change to it showing success.

[sherm1]

Feature pending open comments

Reviewed 3 of 6 files at r2.
Reviewable status: 5 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy)

[mitiguy]

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

---

geometry/test/bad_geometryA.obj line at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW I prefer that the tests use the (small) bad mesh that was supplied by the OP in the original bugreport, with minor tweaks to demonstrate proper behavior. No need to simplify further IMO, this is good.

Done. Using mesh from original bug report.

---

multibody/tree/geometry_spatial_inertia.h line 54 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

This proposed change seems out of scope for this PR which fixes the user-reported bad error message. Is there some reason it has to be here? If not, please handle separately.

Done. This can be done in a subsequent PR (if desirable -- Sean and I thought yes).
I'll add a TODO.

---

multibody/tree/geometry_spatial_inertia.cc line 134 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW I think it makes sense for the volume method to be more strict than the raw SpatialInertia constructor.

Done. It is important to identify volume = zero and throw an exception here. Note: We divide by volume in two subsequent lines of code in this function. Also, spatial inertia does not throw if mass = zero.

---

multibody/tree/geometry_spatial_inertia.cc line 136 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW I agree an improved error message here is sufficient to address the presenting issue. The proposed message or one with minor additions as above seems likely to solve the OP's problem -- consider merging that, with possible upgrades & troubleshooting guide later if warranted.

Done. TODO(Mitiguy) added for troubleshooting guide.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 181 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Agreed. This is fine as-is with no cut/paste duplication. IMO it is helpful to show the OP's bad geometry with beautiful new error message with the minimal change to it showing success.

Done.

[sherm1]

+@rpoyner-tri for platform review, please

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

[jwnimmer-tri]

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

---

geometry/BUILD.bazel line 807 at r3 (raw file):

    testonly = 1,
    srcs = [
        "test/bad_geometryA.obj",

nit The filename tails "A" and "B" are nonsense. We should always use readable and meaningful names, not only for classes and functions, but also for filenames.

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 181 at r1 (raw file):

IMO it is helpful to show the OP's bad geometry with beautiful new error message with the minimal change to it showing success.

The premise of contrasting between failure and success is a fair idea. The defect is about the realization of that idea in this test program ...

no cut/paste duplication

The contents of the three data files are copy-pasted, for the most part.

[jwnimmer-tri]

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

---

multibody/tree/geometry_spatial_inertia.h line 51 at r3 (raw file):

 If these requirements are not met, a value *will* be returned, but its value
 is meaningless.
 @throws std::exception if the volume of `mesh` is negative or nearly zero.

(1) Adding a new exception to Stable API is a breaking change.

(2) These two sentences are now in disagreement.

Code quote:

 If these requirements are not met, a value *will* be returned, but its value
 is meaningless.
 @throws std::exception if the volume of `mesh` is negative or nearly zero.

[sherm1]

-@rpoyner-tri (we're going to make a few more changes before this is ready)

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

[sherm1]

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

a discussion (no related file):
Per f2f w/Jeremy: PR description will explain rationale for "fix" rather than "breaking change" here (despite contract change)

---

[sherm1]

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

---

multibody/tree/test/geometry_spatial_inertia_test.cc line 164 at r3 (raw file):

// Throw an exception message when CalcSpatialInertia(Shape) calculates an
// invalid volume for an associated geometry file.
GTEST_TEST(GeometrySpatialInertiaTest, ExceptionOnBadGeometry) {

Per f2f: add a test running the original mesh through the parser to verify that the new error message bubbles up all the way so that the OP's complaint is actually resolved.

[sherm1]

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

[rpoyner-tri]

Reviewed 9 of 10 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @mitiguy)

a discussion (no related file):

Previously, mitiguy (Mitiguy) wrote…

Done. See PR description.

FYI: Odd problems were encountered after commit #5 to CI (10/3/2024 at 2:48 PM).
This PR failed in CI on only one computer with the following error:
//bindings/pydrake:memory_leak_test_test TIMEOUT in 60.0s

To circumvent this odd problem, there were two subsequent (failed) attempts of:
@drake-jenkins-bot linux-jammy-gcc-bazel-experimental-debug please.

Thanks to Jeremy for seeing this and creating 2 PRs, namely:
PR #21986 [pydrake] Add more examples of memory leaks.
PR #21986 affected the file bindings/pydrake/BUILD.bazel and
bindings/pydrake/test/memory_leak_test.py

PR #21990 [pydrake] Hotfix debug timeout for memory_leak_test with
"priority emergency" changes to bindings/pydrake/BUILD.bazel:
test_rule_args = [
"--count=2",
],

Ironically, another odd failure just arose now.
//bindings/pydrake/visualization:py/multicam_scenario_test FAILED in 3.7s

In a saner world, it would be easier to construct arguments of the form "that can't be my failure, because the failing test doesn't depend on my changes". Sadly, pydrake depends on approximately all of c++ drake, so it's harder, but not impossible to determine when failures are related.

---

---

multibody/parsing/test/parser_test.cc line 454 at r8 (raw file):

  std::string filename = "drake/geometry/test/bad_geometry_volume_zero.obj";
  std::string geometry_file_path = FindResourceOrThrow(filename);
  DRAKE_EXPECT_THROWS_MESSAGE(

The parser should never throw just because input files are bad. It is permitted to throw for more dire problems (internal errors, resource exhaustion, etc.).

---

multibody/tree/geometry_spatial_inertia.cc line 92 at r8 (raw file):

}

SpatialInertia<double> CalcSpatialInertia(

minor It shouldn't be too hard to come up with an alternative design here, where the public api function throws, but relies on another function (maybe internal::) that either returns an inertia or a (list of?) message(s). The public function would then call the other function, and throw if it got a message instead of an inertia.

The parser(s) should use the proposed other function.

[rpoyner-tri]

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

---

multibody/parsing/test/parser_test.cc line 446 at r7 (raw file):

// Ensure an exception is thrown if parsing a mesh with zero volume.
// Related: issue #21924 [github.com/RobotLocomotion/drake/issues/21924].
GTEST_TEST(FileParserTest, ZeroVolumeShouldThrowException) {

This test should really be in multibody/parsing/test/detail_mesh_parser_test.cc. It's a bit more fussy to write, but not too bad. Ping me if you need help with it.

[mitiguy]

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

---

multibody/parsing/test/parser_test.cc line 446 at r7 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

This test should really be in multibody/parsing/test/detail_mesh_parser_test.cc. It's a bit more fussy to write, but not too bad. Ping me if you need help with it.

OK -- I am happy with moving it (regardless of my request in my next comment below).

---

multibody/parsing/test/parser_test.cc line 454 at r8 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

The parser should never throw just because input files are bad. It is permitted to throw for more dire problems (internal errors, resource exhaustion, etc.).

It would be helpful to discuss this with you. I am hoping that we could push this PR through as it corrects a broken contract (i.e., corrects broken documentation) that is invalid for various reasons. Immediately thereafter, I would like to do a separate PR that incorporates your best practices. It would help me learn better if this nugget was separated from other things that need to be fixed anyways. Please let me know if you are OK with that.

[rpoyner-tri]

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

---

multibody/parsing/test/parser_test.cc line 454 at r8 (raw file):

Previously, mitiguy (Mitiguy) wrote…

It would be helpful to discuss this with you. I am hoping that we could push this PR through as it corrects a broken contract (i.e., corrects broken documentation) that is invalid for various reasons. Immediately thereafter, I would like to do a separate PR that incorporates your best practices. It would help me learn better if this nugget was separated from other things that need to be fixed anyways. Please let me know if you are OK with that.

I don't think it is particularly hard to get this right the first time.

Recall that:

• currently, there is no public api to convert diagnostic.Error() from throw to anything else. So (for now), Error() will throw.
• in future, there might be. I have striven mightily to make sure the tests cover continuation of execution after Error() -- the tests do replace Error() with an implementation that just remembers the messages emitted.
• I've already had to work around hard-coded throw behavior in spatial algebra code, including violating the style guide by catching exceptions. I'd rather not have to make that a habit. See [parsing] Improve URDF inertia invalidity handling #19238, [parsing] Improve SDFormat inertia handling #19245.

I get that someone's contract is violated by broken meshes. However, it certainly is not the parser. The whole point of a parser is to evaluate untrusted input, and (hopefully) tell the user what is wrong with it. While we don't want to sleep on broken meshes, we have a variety of choices of what to do with them. For example, in the broken inertia patches listed above, the parser actually inserts some "good enough" inertia in place of the broken one. I can't yet say that's the right idea here yet, but I know that just throwing from some low level, regardless of other context, is not a great user experience.

[mitiguy]

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

---

multibody/parsing/test/parser_test.cc line 454 at r8 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

I don't think it is particularly hard to get this right the first time.

Recall that:

• currently, there is no public api to convert diagnostic.Error() from throw to anything else. So (for now), Error() will throw.
• in future, there might be. I have striven mightily to make sure the tests cover continuation of execution after Error() -- the tests do replace Error() with an implementation that just remembers the messages emitted.
• I've already had to work around hard-coded throw behavior in spatial algebra code, including violating the style guide by catching exceptions. I'd rather not have to make that a habit. See [parsing] Improve URDF inertia invalidity handling #19238, [parsing] Improve SDFormat inertia handling #19245.

I get that someone's contract is violated by broken meshes. However, it certainly is not the parser. The whole point of a parser is to evaluate untrusted input, and (hopefully) tell the user what is wrong with it. While we don't want to sleep on broken meshes, we have a variety of choices of what to do with them. For example, in the broken inertia patches listed above, the parser actually inserts some "good enough" inertia in place of the broken one. I can't yet say that's the right idea here yet, but I know that just throwing from some low level, regardless of other context, is not a great user experience.

For our next conversation: The mesh in bad_geometry_volume_negative.obj only has a single face, so it may be difficult to determine "good enough" inertia. I improved the comment in bad_geometry_volume_negative.obj to help further describe the defects in that mesh. I agree with you: throwing from some low level is not a great user experience. It seems true in the parser and perhaps also true in other parts of our code.

[rpoyner-tri]

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

[rpoyner-tri]

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

a discussion (no related file):
I have some mods to this PR in the works. It's a bit more involved than I thought, but I think it will be worth the trouble. We will bet to address at least part of #21666 in the bargain. Stay tuned.

---

[rpoyner-tri]

Closes: #21666 .
+(release notes: feature) adds SpatialInertia::CriticizeNotPhysicallyValid() method.

FWIW, this will probably need a re-read and a different platform reviewer, as I am now a co-author.
I'll stay assigned until we get things ironed out.

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

a discussion (no related file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

I have some mods to this PR in the works. It's a bit more involved than I thought, but I think it will be worth the trouble. We will bet to address at least part of #21666 in the bargain. Stay tuned.

r10 contains my in-progress work on keeping throw logic out of parser implementation. It's pretty complete in terms of my main goal, but I had to add a public API to SpatialInertia which currently has no unit tests. There are probably a few other tests missing, and doc probably needs work. PTAL

---

---

multibody/tree/geometry_spatial_inertia.cc line 92 at r8 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

minor It shouldn't be too hard to come up with an alternative design here, where the public api function throws, but relies on another function (maybe internal::) that either returns an inertia or a (list of?) message(s). The public function would then call the other function, and throw if it got a message instead of an inertia.

The parser(s) should use the proposed other function.

Done.