Use package instead of relative path for allegro model files #14252
Conversation
There was a problem hiding this comment.
+@jwnimmer-tri for feature review. Thanks!
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)
|
+@IanTheEngineer for feature review as a step along fixing #10531. One or both of you (Ian and/or Huihua) should confirm locally that viewing the model in drake-visualizer still works. See manipulation/util for geometry_inspector and show_model. I'll handle platform review. It seems fine on first glance. |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer (waiting on @IanTheEngineer)
There was a problem hiding this comment.
I have run locally and the model looks fine.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer (waiting on @IanTheEngineer)
There was a problem hiding this comment.
Could you please describe how you tested the visualization locally? I might be building or invoking things incorrectly, but bazel-bin/manipulation/util/show_model.runfiles appears to only have access to iiwa_description, wsg_50_description, and ycb packages, which leads the following commands to fail (with a meshcat server in the background):
./bazel-bin/manipulation/util/show_model \
--meshcat default --find_resource \
drake/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf
and
./bazel-bin/manipulation/util/show_model --meshcat default \
${PWD}/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf
The allegro_hand_description tests themselves pass with the package URI change. Once I can replicate a visualization locally, I'll give the green light for feature review.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),IanTheEngineer (waiting on @IanTheEngineer)
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer)
There was a problem hiding this comment.
You have to provide the package_path for it to find the file. Here is what I did ./bazel-bin/manipulation/util/show_model ./manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf --package_path manipulation/models/allegro_hand_description
Reviewable status: LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer)
There was a problem hiding this comment.
I have added the explanation in the show_model file. PTAL.
Reviewable status: LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer)
huihuaTRI
force-pushed
the
102620_update_allegro_model_files
branch
from
b052e27 to
91efb8f
Compare
There was a problem hiding this comment.
Thanks for the updates. I was able to visualize using the package path using the command you used, specifying the package_path and a full or relative path to the sdf file. However, the command you added to the documentation is not able to find the allegro_hand_description with find_resource in my testing. See the details below.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer (waiting on @huihuaTRI, @IanTheEngineer, and @jwnimmer-tri)
manipulation/util/show_model.py, line 41 at r2 (raw file):
manipulation/models/iiwa_description \ --find_resource \ drake/manipulation/models/iiwa_description/iiwa7/iiwa7_no_collision.sdf
Thank you for the explanation. In my testing, show_model --find_resource seems to require the allegro_hand_description files to be available under show_model.runfiles:
./bazel-bin/manipulation/util/show_model --package_path manipulation/models/allegro_hand_description --find_resource drake/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf
Using FindResourceOrThrow('drake/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf')
Traceback (most recent call last):
File "/path/to/drake/./bazel-bin/manipulation/util/show_model.runfiles/drake/manipulation/util/show_model.py", line 210, in <module>
main()
File "/path/to/drake/./bazel-bin/manipulation/util/show_model.runfiles/drake/manipulation/util/show_model.py", line 186, in main
filename, make_parser = parse_filename_and_parser(args_parser, args)
File "/path/to/drake/./bazel-bin/manipulation/util/show_model.runfiles/drake/manipulation/util/show_model.py", line 110, in parse_filename_and_parser
filename = FindResourceOrThrow(res_path)
RuntimeError: Sought 'drake/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf' in runfiles directory '/path/to/drake/bazel-bin/manipulation/util/show_model.runfiles' but the file does not exist at that location nor is it on the manifest; perhaps a 'data = []' dependency is missing.
However, fully specifying the location of the sdf file works, as long as the package_path is supplied, works:
./bazel-bin/manipulation/util/show_model --package_path manipulation/models/allegro_hand_description ${PWD}/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf
[2020-10-28 08:27:04.588] [console] [warning] Currently MultibodyPlant does not handle joint limits for continuous models. However some joints do specify limits. Consider setting a non-zero time step in the MultibodyPlant constructor; this will put MultibodyPlant in discrete-time mode, which does support joint limits.
[2020-10-28 08:27:04.588] [console] [warning] Joints that specify limits are: `joint_8`, `joint_9`, `joint_10`, `joint_11`, `joint_12`, `joint_13`, `joint_14`, `joint_15`, `joint_4`, `joint_5`, `joint_6`, `joint_7`, `joint_0`, `joint_1`, `joint_2`, `joint_3`.
[2020-10-28 08:27:04.594] [console] [warning] Currently MultibodyPlant does not handle joint limits for continuous models. However some joints do specify limits. Consider setting a non-zero time step in the MultibodyPlant constructor; this will put MultibodyPlant in discrete-time mode, which does support joint limits.
[2020-10-28 08:27:04.594] [console] [warning] Joints that specify limits are: `joint_8`, `joint_9`, `joint_10`, `joint_11`, `joint_12`, `joint_13`, `joint_14`, `joint_15`, `joint_4`, `joint_5`, `joint_6`, `joint_7`, `joint_0`, `joint_1`, `joint_2`, `joint_3`.
I don't think you need to necessarily handle all of show_model's use cases with this PR, but since you're updating the documentation, I think it should reflect the functional case using a fully specified path.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer (waiting on @huihuaTRI)
huihuaTRI
force-pushed
the
102620_update_allegro_model_files
branch
from
91efb8f to
9e1be26
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer and @jwnimmer-tri)
manipulation/util/show_model.py, line 41 at r2 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
Thank you for the explanation. In my testing,
show_model --find_resourceseems to require theallegro_hand_descriptionfiles to be available undershow_model.runfiles:./bazel-bin/manipulation/util/show_model --package_path manipulation/models/allegro_hand_description --find_resource drake/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf Using FindResourceOrThrow('drake/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf') Traceback (most recent call last): File "/path/to/drake/./bazel-bin/manipulation/util/show_model.runfiles/drake/manipulation/util/show_model.py", line 210, in <module> main() File "/path/to/drake/./bazel-bin/manipulation/util/show_model.runfiles/drake/manipulation/util/show_model.py", line 186, in main filename, make_parser = parse_filename_and_parser(args_parser, args) File "/path/to/drake/./bazel-bin/manipulation/util/show_model.runfiles/drake/manipulation/util/show_model.py", line 110, in parse_filename_and_parser filename = FindResourceOrThrow(res_path) RuntimeError: Sought 'drake/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf' in runfiles directory '/path/to/drake/bazel-bin/manipulation/util/show_model.runfiles' but the file does not exist at that location nor is it on the manifest; perhaps a 'data = []' dependency is missing.However, fully specifying the location of the sdf file works, as long as the
package_pathis supplied, works:./bazel-bin/manipulation/util/show_model --package_path manipulation/models/allegro_hand_description ${PWD}/manipulation/models/allegro_hand_description/sdf/allegro_hand_description_left.sdf [2020-10-28 08:27:04.588] [console] [warning] Currently MultibodyPlant does not handle joint limits for continuous models. However some joints do specify limits. Consider setting a non-zero time step in the MultibodyPlant constructor; this will put MultibodyPlant in discrete-time mode, which does support joint limits. [2020-10-28 08:27:04.588] [console] [warning] Joints that specify limits are: `joint_8`, `joint_9`, `joint_10`, `joint_11`, `joint_12`, `joint_13`, `joint_14`, `joint_15`, `joint_4`, `joint_5`, `joint_6`, `joint_7`, `joint_0`, `joint_1`, `joint_2`, `joint_3`. [2020-10-28 08:27:04.594] [console] [warning] Currently MultibodyPlant does not handle joint limits for continuous models. However some joints do specify limits. Consider setting a non-zero time step in the MultibodyPlant constructor; this will put MultibodyPlant in discrete-time mode, which does support joint limits. [2020-10-28 08:27:04.594] [console] [warning] Joints that specify limits are: `joint_8`, `joint_9`, `joint_10`, `joint_11`, `joint_12`, `joint_13`, `joint_14`, `joint_15`, `joint_4`, `joint_5`, `joint_6`, `joint_7`, `joint_0`, `joint_1`, `joint_2`, `joint_3`.I don't think you need to necessarily handle all of
show_model's use cases with this PR, but since you're updating the documentation, I think it should reflect the functional case using a fully specified path.
Done. Thanks for checking it and sorry for my carelessness. Now updated and tested locally. It should work this time.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer)
There was a problem hiding this comment.
Your new instructions work well. One final comment on line length, and we're good to go (assuming the rest of the tests then pass).
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer (waiting on @huihuaTRI)
manipulation/util/show_model.py, line 35 at r3 (raw file):
${PWD}/manipulation/models/iiwa_description/sdf/iiwa14_no_collision.sdf If the model is using package path (e.g. "package://package_name/model_sdf.obj")
This line is one character beyond the 79 char limit. It's is quite pedantic, but it is causing the CI tests to fail.
huihuaTRI
force-pushed
the
102620_update_allegro_model_files
branch
from
9e1be26 to
e8a8f96
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer and @jwnimmer-tri)
manipulation/util/show_model.py, line 35 at r3 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
This line is one character beyond the 79 char limit. It's is quite pedantic, but it is causing the CI tests to fail.
Done.
There was a problem hiding this comment.
The CI is happy now. PTAL. Thank you!
Reviewable status: 1 unresolved discussion, LGTM missing from assignee IanTheEngineer (waiting on @IanTheEngineer and @jwnimmer-tri)
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),IanTheEngineer (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Yep. Thanks for reviewing!
Reviewable status:
This change is