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

Add mujoco_menagerie as a test-only dependency #21649

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

RussTedrake
Copy link
Contributor

@RussTedrake RussTedrake commented Jun 30, 2024

And use it for testing the mujoco parser.
Towards #20444 and #21648.


This change is Reviewable

@RussTedrake
Copy link
Contributor Author

+@jwnimmer-tri for feature review of build-system, please.

@jwnimmer-tri
Copy link
Collaborator

See RussTedrake#56 for changes.

@jwnimmer-tri jwnimmer-tri added release notes: fix This pull request contains fixes (no new features) and removed release notes: none This pull request should not be mentioned in the release notes labels Jun 30, 2024
@jwnimmer-tri
Copy link
Collaborator

(I also changed the release notes tag -- adding new dependencies to the workspace is notable.)

@RussTedrake
Copy link
Contributor Author

thanks @jwnimmer-tri -- are you satisfied enough to give one or both LGTMs?

@jwnimmer-tri
Copy link
Collaborator

Build system stuff looks good now.

Both for reasons of time, and somewhat because I co-authored, we should probably find someone else to review -@jwnimmer-tri.

Probably @joemasterjohn or @rpoyner-tri would be good for feature review, or maybe @sherm1 feels confident enough to do both reviews.

@jwnimmer-tri jwnimmer-tri removed their assignment Jul 1, 2024
@RussTedrake
Copy link
Contributor Author

+@rpoyner-tri for feature review, please (unless @sherm1 feels good to take both)
+@sherm1 for platform review, please.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Platform :lgtm:
My grasp of build system details is tenuous, so I think it is worth getting a feature review from Rico (he's back tomorrow).

Reviewed 4 of 8 files at r1, 4 of 4 files at r2, 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 @RussTedrake)

@jwnimmer-tri
Copy link
Collaborator

jwnimmer-tri commented Jul 1, 2024

If your only doubt is about the build system (rather than parser semantics), then I've already LGTMd the build system, in a sense. I mean, I wrote the code for it, so I believe it's using the correct habits. The only question is if it makes sense to you with fresh eyes.

Update: If Russ fixes my BTW below, then this PR becomes boring and we can do the following:

  • Count me as author for the build system, and Russ as feature reviewer.
  • Count Russ as author for the new test cases, and me as feature reviewer.
  • Sherm as platform for everything.

The only thing we lack feature review on is the weird docs change.

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.

Ugh, I'm trying to keep my hands out of this one and focus work that more directly requires my skillset, but I can't help myself but comment again

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 @RussTedrake)


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

 @throws std::exception if `shape` is an instance of geometry::HalfSpace or
                        geometry::MeshcatCone.
 @throws std::exception if the resulting spatial inertia computation does not

BTW Probably better to revert this header file change until a future PR that deals in semantics instead of build system.

(1) This is a "release notes: breaking change" (by changing the contract of a public API in a backwards-incompatible way).

(2) The rationale for editing this comment now is not clear. There are no tests that relate to this documentation change. Why aren't we testing the new promise?

(3) Why is only this overload changed? It's the overload below the actual source of the throwing? Why isn't that one also changed?

(4) Why change this public API in the first place, instead of fixing the bug so that we don't actually throw? This seems like it's pointing us in the wrong direction.

@sherm1 sherm1 added the status: single reviewer ok https://drake.mit.edu/reviewable.html label Jul 1, 2024
Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

With the new comment reverted, +(status: single reviewer ok)

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 @RussTedrake)

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: LGTM missing from assignee rpoyner-tri(platform)


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

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Probably better to revert this header file change until a future PR that deals in semantics instead of build system.

(1) This is a "release notes: breaking change" (by changing the contract of a public API in a backwards-incompatible way).

(2) The rationale for editing this comment now is not clear. There are no tests that relate to this documentation change. Why aren't we testing the new promise?

(3) Why is only this overload changed? It's the overload below the actual source of the throwing? Why isn't that one also changed?

(4) Why change this public API in the first place, instead of fixing the bug so that we don't actually throw? This seems like it's pointing us in the wrong direction.

Done. (i included it because the method threw an exception that it was not documented as throwing , and it happened while testing this PR; but I agree it should be done more carefully in the future).

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.

Reviewed 3 of 8 files at r1, 3 of 4 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)


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

Previously, RussTedrake (Russ Tedrake) wrote…

Done. (i included it because the method threw an exception that it was not documented as throwing , and it happened while testing this PR; but I agree it should be done more carefully in the future).

For sure, the code had (has) a bug.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignee sherm1(platform)

@jwnimmer-tri jwnimmer-tri merged commit 9037767 into RobotLocomotion:master Jul 1, 2024
9 checks passed
@RussTedrake RussTedrake deleted the mujoco_menagerie branch July 1, 2024 18:17
RussTedrake added a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
And use it for testing the mujoco parser.
Towards 20444 and 21648.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: fix This pull request contains fixes (no new features) status: single reviewer ok https://drake.mit.edu/reviewable.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants