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

[mujoco parser] Fix debug throw from RotationalInertia #22401

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

RussTedrake
Copy link
Contributor

@RussTedrake RussTedrake commented Jan 6, 2025

The InertiaCalculator is not supposed to throw, but could still throw in debug mode based on how it was constructing an intermediate RotationalInertia. Now we skip the intermediate check and properly catch the error to display a helpful message in the parser.

Towards #20444.

+@rpoyner-tri for feature review, please.


This change is Reviewable

@RussTedrake RussTedrake added the release notes: fix This pull request contains fixes (no new features) label Jan 6, 2025
Copy link
Contributor Author

@RussTedrake RussTedrake 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: 3 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers


multibody/parsing/test/detail_mujoco_parser_test.cc line 747 at r1 (raw file):

  // CalcSpatialInertia().
  const RlocationOrError rlocation = FindRunfile(
      "mujoco_menagerie_internal/hello_robot_stretch/assets/link_head_1.obj");

link_head_1 fails in the way that base_link_0 failed (invalid SpatialInertia), and also earlier (invalid RotationalInertia). So I felt that a complete replacement was best.


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 95 at r1 (raw file):

constexpr std::string_view kItWorks{""};
constexpr std::string_view kTooSlow{"skip me"};  // especially in debug mode.

btw -- yeah. now being too slow is the only reason we skip a test.
should we open an issue about the performance?


multibody/parsing/detail_mujoco_parser.cc line 585 at r1 (raw file):

                "Failed to compute spatial inertia even for the convex hull of "
                "{}.\n{}",
                mesh.source().path(), std::get<std::string>(result)));

btw -- i don't have any examples that can cause this failure, so don't have test coverage. But it seems like better form to catch and print a helpful error than to throw an obscure debug-time throw.

Copy link
Contributor

@rpoyner-tri rpoyner-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 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers


multibody/parsing/detail_mujoco_parser.cc line 585 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- i don't have any examples that can cause this failure, so don't have test coverage. But it seems like better form to catch and print a helpful error than to throw an obscure debug-time throw.

I can maybe find some input to provoke this. Could do that in a separate patch.


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 95 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- yeah. now being too slow is the only reason we skip a test.
should we open an issue about the performance?

Yes, let's have an issue, please.

Copy link
Collaborator

@jwnimmer-tri jwnimmer-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: 2 unresolved discussions, needs at least two assigned reviewers


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 95 at r1 (raw file):

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

Yes, let's have an issue, please.

BTW I believe this test program can inquire at runtime about the build flavor (e.g., check if asserts are armed), and use that to skip tests which are only too painful only in debug.

The InertiaCalculator is not supposed to throw, but could still throw
in debug mode based on how it was constructing an intermediate
RotationalInertia. Now we skip the intermediate check and properly
catch the error to display a helpful message in the parser.

Towards RobotLocomotion#20444.
Copy link
Contributor Author

@RussTedrake RussTedrake 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 for platform review, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform)


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 95 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I believe this test program can inquire at runtime about the build flavor (e.g., check if asserts are armed), and use that to skip tests which are only too painful only in debug.

=> #22412

re: conditional arming: in release mode on my (order) puget:

  • most models are <=350s
  • franka models are ~700ms
  • hello robot models are ~3600ms.
    wdyt?

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 2 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion

Copy link
Contributor

@rpoyner-tri rpoyner-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


multibody/parsing/detail_mujoco_parser.cc line 585 at r1 (raw file):

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

I can maybe find some input to provoke this. Could do that in a separate patch.

Consider cherry-picking #22414, or I can land it separately after this one -- your choice of workflow.

Copy link
Contributor

@rpoyner-tri rpoyner-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


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 95 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

=> #22412

re: conditional arming: in release mode on my (order) puget:

  • most models are <=350s
  • franka models are ~700ms
  • hello robot models are ~3600ms.
    wdyt?

See #22416 for either cherry-pick or later patching. It only skips slow cases in debug mode; in release mode it obeys some other result label for each case.

Copy link
Contributor

@rpoyner-tri rpoyner-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: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform)


multibody/parsing/test/detail_mujoco_parser_examples_test.cc line 95 at r1 (raw file):

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

See #22416 for either cherry-pick or later patching. It only skips slow cases in debug mode; in release mode it obeys some other result label for each case.

Meh. I'll claim having a patch in the funnel for this is good enough and recommend we just land this PR.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform)

@rpoyner-tri rpoyner-tri merged commit 6a7293d into RobotLocomotion:master Jan 7, 2025
10 checks passed
@RussTedrake RussTedrake deleted the mujoco_hello_robot branch January 7, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants