Mujoco bad inertia test case #22414
Mujoco bad inertia test case #22414
Conversation
There was a problem hiding this comment.
+(status: do not merge) +(status: do not review)
This is a follow-up (or maybe cherry-pick in-) to #22401.
FYI @RussTedrake @ggould-tri
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
There was a problem hiding this comment.
This contains a copy of #22401, so I've added the blocking labels until we figure out the workflow.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
rpoyner-tri
force-pushed
the
mujoco-bad-inertia-test-case
branch
from
c881384 to
c8b0d0a
Compare
rpoyner-tri
force-pushed
the
mujoco-bad-inertia-test-case
branch
from
c8b0d0a to
2c50572
Compare
rpoyner-tri
added
release notes: none
There was a problem hiding this comment.
Rebased onto newer master. -(status: do not merge) -(status: do not review) +(release notes: none)
+@RussTedrake for feature review.
Reviewable status: LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge" (waiting on @rpoyner-tri)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers
multibody/parsing/test/detail_mujoco_parser_test.cc line 805 at r1 (raw file):
DRAKE_EXPECT_THROWS_MESSAGE( AddModelFromString(xml, "test"), ".*\n.*\n.*\n.*\n.*CouldBePhysicallyValid.*");
Wait. I thought that I fixed this in #22401... no?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers
multibody/parsing/test/detail_mujoco_parser_test.cc line 805 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
Wait. I thought that I fixed this in #22401... no?
You'd think, but this is a rather more evil mesh. I'll try to do some debug tracing to see why some uses miss this and other hit. Ultimately, the whole debug-only-throw regime is going to be an endless headache. Hence, issue #22415.
This tests the error path in the parser when even falling back to convex hull can't save us. Fix one control flow path bug in the parser found by this new test.
rpoyner-tri
force-pushed
the
mujoco-bad-inertia-test-case
branch
from
2c50572 to
c95d4e9
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers
multibody/parsing/test/detail_mujoco_parser_test.cc line 805 at r1 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
You'd think, but this is a rather more evil mesh. I'll try to do some debug tracing to see why some uses miss this and other hit. Ultimately, the whole debug-only-throw regime is going to be an endless headache. Hence, issue #22415.
You'll see an added return statement in the parser in r2. This is the sort of thing that doesn't matter (yet) in production, because the public API doesn't allow any action for Error() other than throw. It does matter in tests. I've maintained things this way on purpose, to allow future us to do something different on Error().
There was a problem hiding this comment.
I'll still claim this is "release notes: none", since the one fix to actual parser code is (currently) only exercised in tests.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers
multibody/parsing/test/detail_mujoco_parser_test.cc line 805 at r1 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
You'll see an added
returnstatement in the parser inr2. This is the sort of thing that doesn't matter (yet) in production, because the public API doesn't allow any action for Error() other thanthrow. It does matter in tests. I've maintained things this way on purpose, to allow future us to do something different on Error().
Awesome. Thanks for catching and fixing that.
There was a problem hiding this comment.
+xuchenhan-tri for platform review, please.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers
There was a problem hiding this comment.
With r2, this is no longer a case a test bifurcation. Is that still a real issue? Do we want an example in this PR to help frame https://reviewable.io/reviews/RobotLocomotion/drake/22414?
Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees RussTedrake(platform),xuchenhan-tri(platform)
There was a problem hiding this comment.
Having seen the resolution here, I may rephrase #22415 somehow. We are so far able to work around it, but it still lurks, a monster in the deep.
Reviewable status:
This change is