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

Models conversion and related tests for smooth loading into drake #73

Merged

Conversation

marcoag
Copy link
Contributor

@marcoag marcoag commented Oct 29, 2021

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:

  • It creates a temp directory to store all the temporal stuff that will be created. It's name is printed at the begging of the script output.
  • It downloads and compiles the ModelPhotoShoot gazebo plugin.
  • It loads the model in gazebo and makes the required pictures using the previous plugin.
  • It runs the 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:

  1. The gazebo plugin seems to have problems loading the MPL right arm model. (fixed in 8130ffb)
  2. The robot on the drake pictures look bigger when the robot is big and smaller when the robot is small, even when the intrinsic are set to the same values.

This change is Reviewable

@marcoag
Copy link
Contributor Author

marcoag commented Oct 29, 2021

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: focal_lengtth = image_width / (2*tan(hfov_radian / 2)) Which resulted in 831.382036787 which I tested for both focal_x and focal_y values.

Also using the gazebo API to obtain the VFOV I obtained 0.62803 which I tested as the fov_y camera value with similar results.

Here is the result on the NAO with Ignition position controller model front view (smaller robot, looks small on drake):

  • Gazebo:
    3
  • Drake:
    3_drake

And this is a result of the CERBERUS_ANYMAL_C_SENSOR_CONFIG_2 model front view (bigger robot, looks bigger on drake):

  • Gazebo:
    3 (1)
  • Drake:
    3_drake (1)

@EricCousineau-TRI EricCousineau-TRI self-requested a review November 1, 2021 14:25
Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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


Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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?


Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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?


Copy link
Contributor Author

@marcoag marcoag 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: 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.


Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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?)


Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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.

Copy link
Contributor Author

@marcoag marcoag 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: 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...


@EricCousineau-TRI
Copy link
Owner

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 wget, curl, or ign fuel to download the archive.
Things I've tried for https://app.ignitionrobotics.org/OpenRobotics/fuel/models/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2/6

# Download the archive from browser, extract download link, and test with wget or curl.
$ wget <url> -O model.zip
# Only has HTML information
$ curl <url> -o model.zip
# Only has HTML information

# Try using `ign fuel`
$ ign fuel download --url https://app.ignitionrobotics.org/OpenRobotics/models/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2/6
[Err] [FuelClient.cc:571] Can't download model, server configuration incomplete:
...

Is there something I'm missing here?

@EricCousineau-TRI
Copy link
Owner

Pushed manual steps for now in 8090178

Copy link
Contributor Author

@marcoag marcoag left a 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)

@EricCousineau-TRI
Copy link
Owner

Ah, gotcha! I can run the wget command. Is it easy to expose this in the Ignition Fuel website? (or should we simply file an issue?)

Regarding ign fuel, I am unable to download it w/ above command:

$ ign fuel download --url https://app.ignitionrobotics.org/OpenRobotics/fuel/models/CERBERUS_ANYMAL_C_SENSOR_CONFIG_2
Invalid URL: only models and worlds or collections can be downloaded so far.

Version:

$ dpkg -s ignition-tools | grep Version
Version: 1.4.1-1~focal

Copy link
Contributor Author

@marcoag marcoag left a 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)

@EricCousineau-TRI
Copy link
Owner

Per VC, perhaps use Ignition Gazebo + this GSoC plugin?
https://community.gazebosim.org/t/gsoc-2021-machine-learning-extension-to-ignition-gazebo/1070

@marcoag
Copy link
Contributor Author

marcoag commented Mar 3, 2022

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.

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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.


Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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]>
Copy link
Contributor Author

@marcoag marcoag 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: 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?

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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 😅

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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.


Copy link
Contributor Author

@marcoag marcoag 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: 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?

Copy link
Owner

@EricCousineau-TRI EricCousineau-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 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:


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

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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, under drake_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


Copy link
Contributor Author

@marcoag marcoag 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: 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.

Copy link
Owner

@EricCousineau-TRI EricCousineau-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 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 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

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?

Copy link
Contributor Author

@marcoag marcoag 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: 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 a with 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, use if 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. 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.

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 from data.

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 as image?

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.

Copy link
Owner

@EricCousineau-TRI EricCousineau-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 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.

Copy link
Contributor Author

@marcoag marcoag 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: 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), and infer_mask(image, bg_pixel=...)

Additionally, we should avoid large for-loops in pure Python, and instead leverage NumPy for (CPU) acceleration.

Done in b131ace.

Copy link
Owner

@EricCousineau-TRI EricCousineau-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 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.

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI 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: 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 calling source setup.sh

OK Deferring to incorporation into drake-ros

Copy link
Owner

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

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

:lgtm:, merging prototype now!

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.


Copy link
Owner

@EricCousineau-TRI EricCousineau-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 1 of 6 files at r1, 1 of 6 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @marcoag)

@EricCousineau-TRI EricCousineau-TRI merged commit d13518f into EricCousineau-TRI:master May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants