-
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
[tri_homecart] Move to drake_models package #21004
[tri_homecart] Move to drake_models package #21004
Conversation
9610c5f
to
82eb937
Compare
85804c4
to
2e20cef
Compare
+@zachfang for feature review, please. |
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 17 of 18 files at r1, 1 of 1 files at r2.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers
BUILD.bazel
line 69 at r2 (raw file):
"//manipulation/models/ycb:models", "//multibody/benchmarks/acrobot:models", "@drake_models",
BTW. Just to understand the strategy better, is our plan to lump all the model files into one @drake_models
even though a target only needs a subset of the models?
Code quote:
"@drake_models"
tools/workspace/drake_models/repository.bzl
line 9 at r2 (raw file):
name = name, repository = "RobotLocomotion/models", commit = "92ed9246562caf6a77a4376b74b41eb777b319c8",
I assume the commit needs updating after model#31 is merged. Put a comment to gate that.
Code quote:
92ed9246562caf6a77a4376b74b41eb777b319c8
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.
+@sherm1 for platform review per schedule, please.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee sherm1(platform)
BUILD.bazel
line 69 at r2 (raw file):
Previously, zachfang wrote…
BTW. Just to understand the strategy better, is our plan to lump all the model files into one
@drake_models
even though a target only needs a subset of the models?
Well, this is Drake's top-level :all_models
target, so it really does want everything.
For the benchmarking
program change down below, we could imagine writing down just the subset of of the models that it cares about, but I don't think it would help very much. Our developers do not frequently re-pin the models repository.
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.
Platform
Do these name changes require an Anzu update to match?
Reviewed 17 of 18 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion
2e20cef
to
926b4ae
Compare
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
tools/workspace/drake_models/repository.bzl
line 9 at r2 (raw file):
Previously, zachfang wrote…
I assume the commit needs updating after model#31 is merged. Put a comment to gate that.
Done.
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 1 of 1 files at r3.
Reviewable status: complete! all discussions resolved, LGTM from assignees sherm1(platform),zachfang
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.
Do these name changes require an Anzu update to match?
Nope, I checked and neither Anzu, nor Underactuated, nor Manipulation use this model. The only place it seems to be used is the planning benchmark.
Reviewable status: complete! all discussions resolved, LGTM from assignees sherm1(platform),zachfang
Update drake_models to latest commit. For example, users should now load package://drake_models/tri_homecart/homecart.dmd.yaml instead of package://drake/manipulation/models/tri_homecart/homecart.dmd.yaml
926b4ae
to
228191c
Compare
Update drake_models to latest commit. For example, users should now load package://drake_models/tri_homecart/homecart.dmd.yaml instead of package://drake/manipulation/models/tri_homecart/homecart.dmd.yaml
Update drake_models to latest commit. For example, users should now load package://drake_models/tri_homecart/homecart.dmd.yaml instead of package://drake/manipulation/models/tri_homecart/homecart.dmd.yaml
For example, users should now load
package://drake_models/tri_homecart/homecart.dmd.yaml
instead of
package://drake/manipulation/models/tri_homecart/homecart.dmd.yaml
Towards #20932 and #13942.
Models PR: RobotLocomotion/models#31
Per https://drake.mit.edu/stable.html#model-files our model data is not Stable API so this is regular release notes, not "breaking change".
This change is