-
Notifications
You must be signed in to change notification settings - Fork 16
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 conversion and related tests for smooth loading into drake #73
Models conversion and related tests for smooth loading into drake #73
Conversation
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
…g/repro into focal_robot_cameras_issue
To elaborate on my tests about the second issue (robots with different sizes on gazebo and drake cameras): The HFOV used in ModelPhotoShoot gazebo plugin is 60 degrees, which according to this can be used to calculate the focal following: Also using the gazebo API to obtain the VFOV I obtained Here is the result on the NAO with Ignition position controller model front view (smaller robot, looks small on drake): And this is a result of the CERBERUS_ANYMAL_C_SENSOR_CONFIG_2 model front view (bigger robot, looks bigger on drake): |
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)
a discussion (no related file):
Working: Will run locally, see if I can repro rendering diff. for NAO / ANYMal, and see if we can distill to simpler example as in this notebook:
https://github.com/EricCousineau-TRI/repro/blob/master/drake_stuff/notebooks/sphere_rendering.ipynb
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Will run locally, see if I can repro rendering diff. for NAO / ANYMal, and see if we can distill to simpler example as in this notebook:
https://github.com/EricCousineau-TRI/repro/blob/master/drake_stuff/notebooks/sphere_rendering.ipynb
Which Gazebo should I use? I'm assuming Ignition Gazebo, Fortress?
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @marcoag)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Which Gazebo should I use? I'm assuming Ignition Gazebo, Fortress?
Ohh, I don't see any env vars for Ignition Gazebo; so perhaps it should be Gazebo classic?
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Ohh, I don't see any env vars for Ignition Gazebo; so perhaps it should be Gazebo classic?
Correct, Gazebo classic.
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @marcoag)
a discussion (no related file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
Correct, Gazebo classic.
Can I ask what the motivation was for Gazebo classic (older versions of libsdformat
) vs. Ignition Gazebo?
Is it difficult to remap this onto Ignition Gazebo? (perhaps not immediately, but say in a few weeks?)
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: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 165 at r1 (raw file):
fi temp_directory=$(mktemp -d)
Working: This should source Gazebo setup file too.
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: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Can I ask what the motivation was for Gazebo classic (older versions of
libsdformat
) vs. Ignition Gazebo?Is it difficult to remap this onto Ignition Gazebo? (perhaps not immediately, but say in a few weeks?)
The reason to go for gazebo classic was the existence of the ModelProShop plugin: https://github.com/osrf/gazebo/blob/gazebo11/plugins/ModelPropShop.cc, which was used to take different images for fuel. I am not aware of the existence of a similar plugin for Ignition Gazebo but I guess worst case scenario we can port it...
It took me a bit to figure out exact correct invocation for stuff like Cerberus. I'm trying to patch this PR w/ exact instructions, but I'm having trouble with using
Is there something I'm missing here? |
Pushed manual steps for now in 8090178 |
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.
It seems the ignition website is using blobs to generate the download link for the model, so I used the browser developer tools to figure out the actual location of the file, this one works for me:
$ wget https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2/6/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2.zip
Using ignition fuel I use the url from the bibtex at the bottom of the page:
$ ign fuel download --url https://app.ignitionrobotics.org/OpenRobotics/fuel/models/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2
this leaves the downloaded model at ~/.ignition/fuel/fuel.ignitionrobotics.org/openrobotics/models/
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)
Ah, gotcha! I can run the Regarding
Version:
|
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.
It's not hard to figure out but you have to use the developer tools. Usually the usage of blobs for file downloads is intentional to hide the actual location of the file, so I am not sure how much the ignition team will be willing to expose this URL further.
Sorry about that, I posted the wrong URL, this one should work (it's the URL from the BibTeX at the bottom of the page of the model):
ign fuel download --url https://fuel.ignitionrobotics.org/1.0/OpenRobotics/models/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2
Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)
Per VC, perhaps use Ignition Gazebo + this GSoC plugin? |
Yup, that seems to be exactly what i was missing. Thanks! I added it to the model loading process and removed the part that was adding the fixed joint. |
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: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI)
a discussion (no related file):
Working: Should determine best place for this to land, e..g drake-ignitino
, drake-model-interop
, drake-compatibility
, drake-comparison
, drake-benchmarks
.
Once we have some candidates, should post to Drake Slack to solicit other Drake Dev opinions.
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: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
a discussion (no related file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
The reason to go for gazebo classic was the existence of the ModelProShop plugin: https://github.com/osrf/gazebo/blob/gazebo11/plugins/ModelPropShop.cc, which was used to take different images for fuel. I am not aware of the existence of a similar plugin for Ignition Gazebo but I guess worst case scenario we can port it...
OK Marking as resolved given ignition efforts - thanks!
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 5 at r5 (raw file):
## Prereqs Tested on Ubuntu 20.04 (Focal).
How much do we need ROS 1? Is it possible to make this all work with just xacro
?
(or does roslaunch
basically tie our hands?)
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 7 at r5 (raw file):
Tested on Ubuntu 20.04 (Focal). - Install Gazebo Classic:
Is this still true? (should this be ignition?)
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 27 at r5 (raw file):
unzip /tmp/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2.zip -d ./CERBERUS_ANYMAL_C_SENSOR_CONFIG_2/ # Run setup.
Is it possible to split up general setup (virtualenv, drake, etc) and per-model testing into separate scripts? e.g.
./setup.sh
mkdir -p repos && cd repos
# untar stuff
./format_model_and_generate_manifest.py ... model.sdf
./compare_model_via_drake_and_ingitiom_images.py ...
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 14 at r5 (raw file):
from os.path import abspath, dirname, isfile from subprocess import PIPE, run import sys
For Python styling, could you use black
?
https://github.com/psf/black
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 117 at r5 (raw file):
# Workaround for issue https: // github.com/assimp/assimp/issues/849. if(suffix == ".dae"):
nit Can you carve this into a separate function?
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 29 at r5 (raw file):
) } _generate_sdf() { (
Yeah, can you carve out all complex instructions / tests into a Python file?
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 213 at r5 (raw file):
if __name__ == "__main__": try: main(sys.argv[1], sys.argv[2], sys.argv[3])
nit Relying on raw argv
for simple cases may be confusing.
Can you use argparse
here?
Signed-off-by: Marco A. Gutierrez <[email protected]>
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: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @EricCousineau-TRI)
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 5 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
How much do we need ROS 1? Is it possible to make this all work with just
xacro
?(or does
roslaunch
basically tie our hands?)
I believe roslaunch
does tie our hands here.
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 7 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Is this still true? (should this be ignition?)
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 27 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Is it possible to split up general setup (virtualenv, drake, etc) and per-model testing into separate scripts? e.g.
./setup.sh mkdir -p repos && cd repos # untar stuff ./format_model_and_generate_manifest.py ... model.sdf ./compare_model_via_drake_and_ingitiom_images.py ...
Broke it down into:
source setup.sh
bash format_model_and_generate_manifest.sh <model_directory> <model_sdf>
bash compare_model_via_drake_and_ingition_images.sh <model_directory> <model_sdf>
I also added a script setup_transform_test.sh
that calls the three of them to offer a one step command.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 14 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
For Python styling, could you use
black
?
https://github.com/psf/black
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 117 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you carve this into a separate function?
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 165 at r1 (raw file):
g: This should source Gazebo setup file too.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 29 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Yeah, can you carve out all complex instructions / tests into a Python file?
Anything not related to dealing with files and calling scripts is now in Python. With regards to this function, the file generated here is needed to call ignition later, seems like it will be a bit tangled to have it in python, do you think it's still worth to move?
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: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 29 at r5 (raw file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
Anything not related to dealing with files and calling scripts is now in Python. With regards to this function, the file generated here is needed to call ignition later, seems like it will be a bit tangled to have it in python, do you think it's still worth to move?
Yes, we should still move it. Our bash scripts should be as small as possible, and have as little content as possible.
I know Bash and Python pretty well, and I hate programming anything with a modicum of complexity in Bash way more 😅
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: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Working: Should determine best place for this to land, e..g
drake-ignitino
,drake-model-interop
,drake-compatibility
,drake-comparison
,drake-benchmarks
.Once we have some candidates, should post to Drake Slack to solicit other Drake Dev opinions.
Thinking on this some more, I think we can place this inside of drake-ros
, under drake_ignition
or something to that effect.
Will confirm w/ Ian.
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: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @EricCousineau-TRI)
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 165 at r1 (raw file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
g: This should source Gazebo setup file too.
\
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 29 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Yes, we should still move it. Our bash scripts should be as small as possible, and have as little content as possible.
I know Bash and Python pretty well, and I hate programming anything with a modicum of complexity in Bash way more 😅
Shall I try to remove bash completely then? Anything you see we need/want to keep under Bash?
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 6 files at r1, 1 of 3 files at r3, 11 of 11 files at r6, all commit messages.
Reviewable status: 13 of 15 files reviewed, 6 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 5 at r5 (raw file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
I believe
roslaunch
does tie our hands here.
OK SGTM
drake_stuff/mbp_robot_arm_joint_limit_stuff/README.md, line 27 at r5 (raw file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
Broke it down into:
source setup.sh bash format_model_and_generate_manifest.sh <model_directory> <model_sdf> bash compare_model_via_drake_and_ingition_images.sh <model_directory> <model_sdf>
I also added a script
setup_transform_test.sh
that calls the three of them to offer a one step command.
OK Thanks!
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 29 at r5 (raw file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
Shall I try to remove bash completely then? Anything you see we need/want to keep under Bash?
This looks better!
If you can minimize it's usage to bare minimum, i.e. only setup.sh
, which is hopefully max 20-40 lines, then that'd be good.
However, I will mark myself as non-blocking for getting initial revisions in and refining in follow-up passes.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 34 at r6 (raw file):
mkdir -p ${_venv_dir} tar -xzf $(_download_drake) -C ${_venv_dir} --strip-components=1
nit Consider changing this to a pip
requirement line. For example:
drake == 1.1.0 |
repro/drake_stuff/multibody_plant_prototypes/setup.sh
Lines 31 to 35 in 34e2c79
python3 -m venv ${_venv_dir} | |
cd ${_venv_dir} | |
./bin/pip install -U pip wheel | |
./bin/pip install -r ${_cur_dir}/requirements.txt | |
./bin/pip freeze > ${_cur_dir}/requirements.freeze.txt |
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh, line 3 at r6 (raw file):
#!/bin/bash # Either source this, or use it as a prefix:
nit Consider removing this docstring. This should ideally only be a script to run, not source.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh, line 12 at r6 (raw file):
# ./setup.sh ./my_program if [[ $# -lt "2" ]]; then
nit Should make more strict, $# -eq 2
; otherwise, unused arguments can be passed.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 63 at r6 (raw file):
def generate_images_and_IoT(simulator, sensor, temp_directory, poses_dir, num_image):
nit IoT
=. IoU
(intersection over union)
Also, consider keeping acronyms as nominal lower case, i.e., iou
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: 13 of 15 files reviewed, 6 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Thinking on this some more, I think we can place this inside of
drake-ros
, underdrake_ignition
or something to that effect.
Will confirm w/ Ian.
Talked w/ Ian; we could put under drake-ros
, but ultimately we'd want it to move out.
Instead, I think it'd be good to place in separate repo.
Posted question: https://drakedevelopers.slack.com/archives/C2NN588UA/p1649632043418799
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: 13 of 15 files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 34 at r6 (raw file):
== 1.1.0
Nicer this way, yes. Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh, line 3 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Consider removing this docstring. This should ideally only be a script to run, not source.
Removed.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh, line 12 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Should make more strict,
$# -eq 2
; otherwise, unused arguments can be passed.
Current behavior is with 2 parameters (a directory and a ansdf
filename) it would trigger the sdf conversion, if only one directory is passed then the entire unviersal_robot
checkout and conversion happens. I was only trying to preserve the old behavior but it's not
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 63 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit
IoT
=.IoU
(intersection over union)Also, consider keeping acronyms as nominal lower case, i.e.,
iou
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 8 of 9 files at r7, all commit messages.
Reviewable status: 14 of 16 files reviewed, 22 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Talked w/ Ian; we could put under
drake-ros
, but ultimately we'd want it to move out.
Instead, I think it'd be good to place in separate repo.
Posted question: https://drakedevelopers.slack.com/archives/C2NN588UA/p1649632043418799
Per convo on Drake Slack, we will place this in drake-ros
for now.
Can you create a PR that only has a README placeholder, setup/install_prereqs
to install necessary deps, along with a new setup for CI?
Then we can start to fold in the work here into that repo.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 115 at r7 (raw file):
# https://github.com/assimp/assimp/issues/849. def rotate_ninety_model(scene): rotation = np.matrix([[1, 0, 0, 0], [0, 0, -1, 0], [0, 1, 0, 0], [0, 0, 0, 1]])
nit For simplicit, consider renaming this to rotate_root_node(scene, rotation)
, and pass rotation in externally.
Consider denoting rotation as a constant:
ROT_X_90 = np.matrix(...)
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 206 at r7 (raw file):
data = etree.tostring(root, pretty_print=True).decode("utf-8") text_file = open(description_file, "w")
nit Use of open()
should happen within a with
block.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 215 at r7 (raw file):
# resolved def create_pacakge_xml(description_file): if os.path.isfile("package.xml") == False:
nit Please avoid if expr == False
. Instead, use if not expr
.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 221 at r7 (raw file):
with open("package.xml", "w") as f: f.write( '<package format="2">\n <name>' + package_name + "</name>\n</package>"
nit String types mixed between ''
and ""
; can use f-strings.
Consider using """ """
and f-strings to avoid switching string types.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 231 at r7 (raw file):
] def preprocess_sdf_and_materials(model_directory, description_file):
nit Can you be sure to re-apply black
here?
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 247 at r7 (raw file):
if filename.endswith(".jpg") or filename.endswith(".jpeg"): im = Image.open(os.path.join(root, filename)) filename = re.sub('\.jpg|\.jpeg', '.png', filename)
This (and other strings) needs r''
to ensure \.
is escaped correctly.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py, line 329 at r7 (raw file):
if __name__ == "__main__": try: parser = argparse.ArgumentParser()
nit try / catch
block encompasses too much code.
Please narrow it down to only surround main()
.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh, line 34 at r7 (raw file):
mkdir -p ${_venv_dir} # tar -xzf $(_download_drake) -C ${_venv_dir} --strip-components=1
nit Can you remove stale comment?
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh, line 12 at r6 (raw file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
Current behavior is with 2 parameters (a directory and a an
sdf
filename) it would trigger the sdf conversion, if only one directory is passed then the entireunviersal_robot
checkout and conversion happens. I was only trying to preserve the old behavior but it's not
This will fail if $# -eq 0
. Can you be explicit about this constraint? i.e. [[ $# -lt 1 || $# -gt 2 ]]
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh, line 1 at r7 (raw file):
#!/bin/bash
nit Scripts should ideally have set -eu -o pipefail
.
You may briefly need set +e; ...; set -e
for calling source setup.sh
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 72 at r7 (raw file):
color = sensor.color_image_output_port().Eval(sensor_context).data image = Image.frombytes("RGBA", (960, 540), color)
(960, 540)
should not be hard-coded here, esp. if it can be determined from data.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 72 at r7 (raw file):
color = sensor.color_image_output_port().Eval(sensor_context).data image = Image.frombytes("RGBA", (960, 540), color)
Image.fromarray
should be better, b/c it respects dimensions.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 83 at r7 (raw file):
px1 = im.load() with image as im:
nit Unclear why you save+load. Not sure if we should bake this in here.
Can you keep px2
as image
?
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 86 at r7 (raw file):
px2 = im.load() union_count = 0
For clarity, this should be separated into a separate function.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 88 at r7 (raw file):
union_count = 0 intersection = 0 for i in range(960):
Please avoid repeating constants. Can you replace with variables from above?
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 90 at r7 (raw file):
for i in range(960): for j in range(540): if px1[i, j] != (0, 255, 0) or px2[i, j] != (204, 229, 255, 255):
I don't understand why we're checking specific pixel values here.
Can you state in a discussion comment? I believe there should be a better way than this.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 126 at r7 (raw file):
</plugin> <include> <uri>''' + model + '''</uri>
You should use f"""..."""
so you can leverage f-strings, and then template can be clearer.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 135 at r7 (raw file):
</include> <model name="photo_shoot"> <pose>2.2 0 0 0 0 -3.14</pose>
We should be more precise than -3.14
.
Can you either use degrees="true"
, or use {math.pi}
?
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 140 at r7 (raw file):
<sensor name="camera" type="camera"> <camera> <horizontal_fov>1.047</horizontal_fov>
nit Same as above - unclear what exactly this value is.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 142 at r7 (raw file):
<horizontal_fov>1.047</horizontal_fov> <image> <width>960</width>
Many of these values seem highly coupled to drake rendering.
Can you make those definitions available, and use them here? (width, height, fov, depth, etc.)
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py, line 323 at r7 (raw file):
# Create ignore namespace so lxml don't complain #os.system("sed -i 's/visual/ignore:collision/g' " + model_file_path) MY_NAMESPACES={'ignore': 'http://ignore'}
nit ALL_CAPS
should only be for global-scope.
Can you either hoist this to global, or make it lower case?
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: 14 of 16 files reviewed, 18 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Per convo on Drake Slack, we will place this in
drake-ros
for now.Can you create a PR that only has a README placeholder,
setup/install_prereqs
to install necessary deps, along with a new setup for CI?Then we can start to fold in the work here into that repo.
Opened a PR with the initial stuff: RobotLocomotion/drake-ros#76.
If you agree, I will follow up with a PR there with the relevant files from here so we can continue iterating there.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py
line 115 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit For simplicit, consider renaming this to
rotate_root_node(scene, rotation)
, and pass rotation in externally.Consider denoting rotation as a constant:
ROT_X_90 = np.matrix(...)
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py
line 206 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Use of
open()
should happen within awith
block.
True, added.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py
line 215 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Please avoid
if expr == False
. Instead, useif not expr
.
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py
line 221 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit String types mixed between
''
and""
; can use f-strings.Consider using
""" """
and f-strings to avoid switching string types.
Good idea, added also in: https://github.com/marcoag/repro/blob/e2f0b8508490f3febfc63c95d1fd91d6acea696d/drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py#L114.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py
line 231 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you be sure to re-apply
black
here?
Yes, I'll make sure I re-apply black
for each update.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py
line 247 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
This (and other strings) needs
r''
to ensure\.
is escaped correctly.
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/render_ur_urdfs.py
line 329 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit
try / catch
block encompasses too much code.Please narrow it down to only surround
main()
.
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh
line 29 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
This looks better!
If you can minimize it's usage to bare minimum, i.e. onlysetup.sh
, which is hopefully max 20-40 lines, then that'd be good.
However, I will mark myself as non-blocking for getting initial revisions in and refining in follow-up passes.
Great. I guess we could make the entire thing just python?
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh
line 34 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Can you remove stale comment?
Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh
line 12 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
This will fail if
$# -eq 0
. Can you be explicit about this constraint? i.e.[[ $# -lt 1 || $# -gt 2 ]]
True, will change it. Also in https://github.com/marcoag/repro/blob/e2f0b8508490f3febfc63c95d1fd91d6acea696d/drake_stuff/mbp_robot_arm_joint_limit_stuff/format_model_and_generate_manifest.sh#L35 and https://github.com/marcoag/repro/blob/e2f0b8508490f3febfc63c95d1fd91d6acea696d/drake_stuff/mbp_robot_arm_joint_limit_stuff/compare_model_via_drake_and_ingition_images.sh#L3.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 72 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
(960, 540)
should not be hard-coded here, esp. if it can be determined fromdata.
True, changed in https://github.com/marcoag/repro/blob/e2f0b8508490f3febfc63c95d1fd91d6acea696d/drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py#L71.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 72 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Image.fromarray
should be better, b/c it respects dimensions.
Yes, better, using that instead now.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 83 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Unclear why you save+load. Not sure if we should bake this in here.
Can you keep
px2
asimage
?
I was saving for user visualization purposes, the loading it's clearly unnecessary, so I removed it. I also renamed image variables to image_drake
and image_ignition
for some clarity.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 86 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
For clarity, this should be separated into a separate function.
Moved it to a new function intersection_over_union
.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 88 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Please avoid repeating constants. Can you replace with variables from above?
Changed to use image1.size[0]
and image1.size[1]
respectively.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 126 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
You should use
f"""..."""
so you can leverage f-strings, and then template can be clearer.
Yes, I agree. Done.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 323 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit
ALL_CAPS
should only be for global-scope.Can you either hoist this to global, or make it lower case?
Changing to lower case.
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 5 of 6 files at r8, all commit messages.
Reviewable status: 14 of 16 files reviewed, 7 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup.sh
line 29 at r5 (raw file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
Great. I guess we could make the entire thing just python?
Yup, as much as possible!
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 71 at r8 (raw file):
union_count = 0 intersection = 0 for i in range(image1.size[0]):
Per VC, this should be reduced to intersection_over_union(mask_a, mask_b)
, and infer_mask(image, bg_pixel=...)
Additionally, we should avoid large for-loops in pure Python, and instead leverage NumPy for (CPU) acceleration.
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: 13 of 16 files reviewed, 7 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 90 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I don't understand why we're checking specific pixel values here.
Can you state in a discussion comment? I believe there should be a better way than this.
Not checking them anymore in b131ace.
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 71 at r8 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Per VC, this should be reduced to
intersection_over_union(mask_a, mask_b)
, andinfer_mask(image, bg_pixel=...)
Additionally, we should avoid large for-loops in pure Python, and instead leverage NumPy for (CPU) acceleration.
Done in b131ace.
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 r9.
Reviewable status: 14 of 16 files reviewed, 6 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 75 at r9 (raw file):
intersection = np.logical_and(mask_a, mask_b).sum() union = np.logical_or(mask_a, mask_b).sum() print(intersection / union)
nit Soz, one more - consider keeping that as math, and return the value itself.
Then testing can check this at a threshold.
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: 14 of 16 files reviewed, 3 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 140 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Same as above - unclear what exactly this value is.
OK Deferring to incorporation into drake-ros
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 142 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Many of these values seem highly coupled to drake rendering.
Can you make those definitions available, and use them here? (width, height, fov, depth, etc.)
OK Deferring to incorporation into drake-ros
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 75 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Soz, one more - consider keeping that as math, and return the value itself.
Then testing can check this at a threshold.
OK Deferring to incorporation into drake-ros
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: 14 of 16 files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @marcoag)
drake_stuff/mbp_robot_arm_joint_limit_stuff/test_models.py
line 135 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
We should be more precise than
-3.14
.Can you either use
degrees="true"
, or use{math.pi}
?
OK Deferring to incorporation into drake-ros
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: 14 of 16 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)
drake_stuff/mbp_robot_arm_joint_limit_stuff/setup_transform_test.sh
line 1 at r7 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Scripts should ideally have
set -eu -o pipefail
.You may briefly need
set +e; ...; set -e
for callingsource setup.sh
OK Deferring to incorporation into drake-ros
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: 14 of 16 files reviewed, all discussions resolved (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, marcoag (Marco A. Gutiérrez) wrote…
Opened a PR with the initial stuff: RobotLocomotion/drake-ros#76.
If you agree, I will follow up with a PR there with the relevant files from here so we can continue iterating there.
OK Thanks! going to defer more review to the PR train to this repo.
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 6 files at r1, 1 of 6 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @marcoag)
This PR is to provide a point of discussion and follow up on the issue RobotLocomotion/drake#15024.
It adds IoU testing to the model transformation process. It does this for default joint poses and random joint poses:
test_models.py
script which will load the model in drake and take the same pictures. This script will also produce the IoU results and other tests using drake.To run the entire process (all the above plus the model conversion):
./setup.sh <model_directory> <file_modelname.sdf>
For an easier debug process I created a jupyter notebook (Model_photo_testing.ipynb), note that this code does not perform the model conversion nor the plugin download and compilation so those steps would have to be done previously. Also the variables at the top related to certain files locations might have to be adjusted.
This has been tested with the following models:
Pending issues:
MPL right arm
model. (fixed in 8130ffb)This change is