Example showing usage of resource from an external ROS package in Drake #194
Conversation
Signed-off-by: Addisu Z. Taddese <[email protected]>
| # -*- mode: cmake -*- | ||
| # vi: set ft=cmake : | ||
|
|
||
| # Copyright (c) 2020, Massachusetts Institute of Technology. |
| @@ -0,0 +1,27 @@ | |||
| Copyright (c) 2020, Massachusetts Institute of Technology. | |||
| <package format="3"> | ||
| <name>drake_example_pendulum</name> | ||
| <version>0.1.0</version> | ||
| <description>Ament CMake project with an installed Drake</description> |
There was a problem hiding this comment.
Can this be more specific?
| @@ -0,0 +1,9 @@ | |||
| cmake_minimum_required(VERSION 3.5) | |||
There was a problem hiding this comment.
Can we stick with CMake 3.10 for consistency. And can you add the license block.
| cmake_minimum_required(VERSION 3.5) | ||
| project(drake_example_pendulum_description) | ||
|
|
||
| find_package(ament_cmake REQUIRED) |
There was a problem hiding this comment.
find_package(ament_cmake CONFIG REQUIRED)
|
My question on this one is at a higher level. The "how to access a URDF" is properly within the scope of the build system, so is appropriate for this repository. (That's usually the first hurdle where PRs fail in the concept stage, but in this case there is no problem.) However, we still want to keep this repository minimal. As of this PR we now have two examples in the ament folder. Do we need both particles and pendulum? Can we show resource-loading as an add-on to the particle demo, for example, by using URDF help to render the scene? Or can we remove the particle demo and only keep the pendulum? Should we plan to add to the other (non-ament) folders a similar URDF demo, and should it be a pendulum in that case too, removing the particle everywhere? I'd like to hear a statement and to have you all share thoughts with respect to the overall design intent and how this fits alongside everything else here. The implementation details are boring and in pretty good shape already. |
|
From a build system demonstration point of view, we could lose the particle example, but it is our hello world that is implemented for all the other projects. If a Drake ROS project is always likely to need this setup with resources, maybe it could replace the pendulum, and consistency is less of an issue. EDIT: So retrofitting the particle demo and adding the same code to the other projects would be my preference. |
|
Ideally what whatever happens would be back ported to Drake somehow. I would like to be able to just generated the non-build system parts of this repro (offline) eventually. The code review should really happen in Drake itself. |
There was a problem hiding this comment.
From the perspective of a ROS2 user who wants to integrate Drake into their workflow, I think this example provides an easy entrypoint. Two packages, one a simple robot description with some (minimal) ROS dependencies, and the other being a Ament package that leverages drake but utilizes the external robot description directly. All this is to say, I think Addisu's packages function well as a standalone example.
With that said, I haven't looked into the particle example in much depth. It is entirely possible that we can reduce this folder to just two packages in the future. In that case, we would keep the description package (pendulum model) and then combine the particle and pendulum resource finding examples into one generic drake_ament_examples package, or something similarly named. That generic package would contain multiple executable examples.
As to the content of this PR, I think it looks great, keeping things minimal while covering how to find package resources from an Ament path. I left a few nits below to discuss.
Reviewed 10 of 10 files at r1.
Reviewable status: 11 unresolved discussions, platform LGTM missing (waiting on @azeey)
drake_ament_cmake_installed/README.md, line 30 at r1 (raw file):
To run the
drake_example_pendulumexample, source the workspace and run the executable
nit: line length for comments / docs in source is generally limited to 80 char.
Also, end the line with a semicolon to present the code.
drake_ament_cmake_installed/src/drake_example_pendulum/LICENSE, line 1 at r1 (raw file):
Copyright (c) 2020, Massachusetts Institute of Technology.
The top-level repo license has a copyright for both MIT and TRI. I think it makes sense to follow suit in this PR everywhere you invoke the license:
Copyright (c) 2021, Massachusetts Institute of Technology.
Copyright (c) 2021, Toyota Research Institute.
drake_ament_cmake_installed/src/drake_example_pendulum/package.xml, line 7 at r1 (raw file):
<version>0.1.0</version> <description>Ament CMake project with an installed Drake</description> <maintainer email="[email protected]">Addisu Z. Taddese</maintainer>
nit: Unless you plan on maintaining this code, think the <author> tag makes more sense for you than the <maintainer> tag here :)
However, as a maintainer tag is required for package.xml format, please feel free to add me as the maintainer:
<maintainer email="[email protected]"> Ian McMahon </maintainer>
drake_ament_cmake_installed/src/drake_example_pendulum/src/drake_example_pendulum.cc, line 48 at r1 (raw file):
// their resources, such as meshes
nit: . punctuation
drake_ament_cmake_installed/src/drake_example_pendulum/src/drake_example_pendulum.cc, line 56 at r1 (raw file):
could not be found. Have you sourced your work space?"
nit: workspace. I would also be more explicit about what you're referring to with workspace (as the definition of workspace is different in Drake/bazel lingo).
drake_ament_cmake_installed/src/drake_example_pendulum_description/package.xml, line 7 at r1 (raw file):
<version>0.1.0</version> <description>Example package to demonstrate using ROS resources in Drake</description> <maintainer email="[email protected]">Addisu Z. Taddese</maintainer>
nit: ditto for the maintainer comments from the other package.xml
|
drake_ament_cmake_installed/src/drake_example_pendulum/package.xml, line 7 at r1 (raw file): Previously, IanTheEngineer (Ian McMahon) wrote…
Actually Drake convention is to use lists, not people, for these: |
Signed-off-by: Addisu Z. Taddese <[email protected]>
There was a problem hiding this comment.
I believe I've addressed all the code comments. Let me know if I should proceed with combining the particle and pendulum demos.
Reviewed 10 of 10 files at r1.
Reviewable status: 7 unresolved discussions, platform LGTM missing (waiting on @azeey, @IanTheEngineer, and @jamiesnape)
drake_ament_cmake_installed/README.md, line 30 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
nit: line length for comments / docs in source is generally limited to 80 char.
Also, end the line with a semicolon to present the code.
Limited to 80 char and added colon in 42fdbbc.
drake_ament_cmake_installed/src/drake_example_pendulum/CMakeLists.txt, line 4 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
2021
Done. 42fdbbc
drake_ament_cmake_installed/src/drake_example_pendulum/LICENSE, line 1 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
2021
Done. 42fdbbc
drake_ament_cmake_installed/src/drake_example_pendulum/LICENSE, line 1 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
The top-level repo license has a copyright for both MIT and TRI. I think it makes sense to follow suit in this PR everywhere you invoke the license:
Copyright (c) 2021, Massachusetts Institute of Technology. Copyright (c) 2021, Toyota Research Institute.
Done. I added TRI to all the files that invoke the license. 42fdbbc
drake_ament_cmake_installed/src/drake_example_pendulum/package.xml, line 6 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
Can this be more specific?
Done. 42fdbbc.
drake_ament_cmake_installed/src/drake_example_pendulum/package.xml, line 7 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Actually Drake convention is to use lists, not people, for these:
Done. I used the lists @jwnimmer-tri suggested. 42fdbbc
drake_ament_cmake_installed/src/drake_example_pendulum/src/drake_example_pendulum.cc, line 48 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
// their resources, such as meshesnit:
.punctuation
Done. 42fdbbc
drake_ament_cmake_installed/src/drake_example_pendulum/src/drake_example_pendulum.cc, line 56 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
could not be found. Have you sourced your work space?"nit:
workspace. I would also be more explicit about what you're referring to with workspace (as the definition of workspace is different in Drake/bazel lingo).
I made it "ROS workspace" in 42fdbbc. How does that sound?
drake_ament_cmake_installed/src/drake_example_pendulum_description/CMakeLists.txt, line 1 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
Can we stick with CMake 3.10 for consistency. And can you add the license block.
Done. 42fdbbc
drake_ament_cmake_installed/src/drake_example_pendulum_description/CMakeLists.txt, line 4 at r1 (raw file):
Previously, jamiesnape (Jamie Snape) wrote…
find_package(ament_cmake CONFIG REQUIRED)
Done. 42fdbbc
drake_ament_cmake_installed/src/drake_example_pendulum_description/package.xml, line 7 at r1 (raw file):
Previously, IanTheEngineer (Ian McMahon) wrote…
nit: ditto for the maintainer comments from the other package.xml
Done. 42fdbbc
I want to resolve the repository-design question now (i.e., have a plan), before we merge this. If we think that amending the particle example to load resources is the correct plan, then we should not merge a pendulum in the meantime. If we think having the pendulum be the sole example is the right plan, then this PR should remove the particle concurrently. Given the relatively low attention and velocity we give to this repository, iterating and stumbling our way to a resolution will leave an awkward situation intact for too long. |
|
After discussing this with Addisu and Eric, we decided this PR will be re-targeted toward the new |
This adds an example of loading a URDF from an external ROS package. The URDF contains a mesh with a
package://type URI. For this to be found, I populate the package map fromAMENT_PREFIX_PATHas described in #170.Closes #170
cc @IanTheEngineer , @EricCousineau-TRI
This change is