-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[models] Move ycb models into drake_models package #21190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@SeanCurtis-TRI for both reviews, please (load balancing Rico).
Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)
manipulation/models/ycb/test/parse_test.cc
line 0 at r1 (raw file):
FYI The drake/tools/workspace/drake_models/test/parse_test.py
runs all of its model files through the parser, so we don't need bespoke unit tests like this anymore.
tools/workspace/drake_models/repository.bzl
line 9 at r1 (raw file):
name = name, repository = "RobotLocomotion/models", commit = "209d8aece4e3b5d36f0831f95156c2c89b01ea3a",
Working
Needs the final commit sha from models master.
There was a problem hiding this 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, LGTM missing from assignee SeanCurtis-TRI(platform)
a discussion (no related file):
Working
Blocking merge so I can coordinate the Anzu upgrade at the same time. There are couple hard-coded YCB paths there that will need fixing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions
bindings/pydrake/test/parse_models_test.py
line 47 at r1 (raw file):
model_files.remove(model_file) if model_relpath.startswith("external/drake_models/"): # These are checked elsewhere by drake_models-specific tests.
BTW It might be worth calling out parse_test.py
. It would be hard to know the correctness of this statement without some kind of cross reference. Hard to maintain.
[workspace] Upgrade drake_models to latest commit
There was a problem hiding this 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 r2, all commit messages.
Reviewable status: 3 unresolved discussions
There was a problem hiding this 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 r2, all commit messages.
Reviewable status: 2 unresolved discussions
tools/workspace/drake_models/repository.bzl
line 9 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Needs the final commit sha from models master.
Done
bindings/pydrake/test/parse_models_test.py
line 47 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW It might be worth calling out
parse_test.py
. It would be hard to know the correctness of this statement without some kind of cross reference. Hard to maintain.
I understand the point, but OTOH soon I aim to kill that test and replace it with more thorough CI on drake_models
itself (so that the parse test runs pre-merge there), in which case this comment would immediately bitrot.
There was a problem hiding this 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
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Blocking merge so I can coordinate the Anzu upgrade at the same time. There are couple hard-coded YCB paths there that will need fixing.
OK Anzu PR 12211 passed.
…1190) [workspace] Upgrade drake_models to latest commit
Towards #13942.
Twin PR: RobotLocomotion/models#42.
This change is