-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds drake package for model URI resolution #14363
Adds drake package for model URI resolution #14363
Conversation
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.
I've reviewed the non-test changes.
Reviewed 3 of 8 files at r1.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @IanTheEngineer and @jwnimmer-tri)
package.xml, line 3 at r1 (raw file):
<package format="2"> <name>drake</name> <version>0.24.0</version>
We've been avoiding hard-coding Drake version numbers anywhere on the master branch, so I'd like to punt this, assuming that we can do so?
multibody/parsing/package_map.cc, line 26 at r1 (raw file):
PackageMap::PackageMap() { const std::optional<string> maybe_drake_path = MaybeGetDrakePath();
A clearer and more durable way to locate the correct path to add is FindResourceOrThrow("drake/package.xml")
. That would not need any if-else branching. Then we could call AddPackageXml
instead of Add
, and avoid hard-coding the "drake" name here also.
multibody/parsing/test/package_map_test.cc, line 151 at r1 (raw file):
// Tests that PackageMap can be populated from an env var. GTEST_TEST(PackageMapTest, TestPopulateFromEnvironment) { PackageMap package_map;
nit It's probably clearer to reset the map to be empty here, so that the rest of the test is unchanged. Having the "drake" default intact during this test isn't really adding any value.
multibody/parsing/test/parser_test.cc, line 189 at r1 (raw file):
geometry::SceneGraph<double> scene_graph; Parser parser(&plant, &scene_graph); EXPECT_EQ(parser.package_map().size(), 1);
nit This parser unit test is asserting something about the package map's default state. Those assertions should live (only) in the package map unit test. Here, we should const int orig_package_size = parser.package_map().size();
and below check that it differs by +1.
40f4180
to
0d4f3c2
Compare
@IanTheEngineer Given your ROS project stuff happening, we should probably try to resurrect and land this one. We really do need to stop using relative paths in our sdformat data. |
package.xml, line 3 at r1 (raw file): Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The version tag is necessary for ROS tools to parse this file. REP 149 requires the |
@jwnimmer-tri Thank you for the ping on this, and your timing is impeccable. I was just having a discussion about resurrecting this with @sloretz in our weekly chat yesterday. Between the two of us we will clean this PR up and get some version of it landed to facilitate the other ROS2 API / examples work. |
Three weeks after the last poke, what's the status here? Should we simply close this PR and "resurrect" this effort in an independent PR? |
@IanTheEngineer or @sloretz might you still be interested in working on this soon? If we want to agree to use a merge workflow on this, we can all push incremental patches here to try to get it finished. |
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 6 of 8 files at r2.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @IanTheEngineer)
Should this be closed in lieu of #15468? |
Superseded by #15468. |
This is an initial proof of concept toward #10531, specifically the minimal subset of items listed in #10531 (comment) to prove that a drake package uri could be used in a model format to refer to a mesh.
The idea is to add a
drake/package.xml
, to allow resources inside SDFormat and URDF files to be able to resolve meshes inside of Drake. In the constructor ofPackageMap
, I prepopulated the map with the Drake package as we discussed in the issue linked above.I started down a few different paths to ensure
Scenegraph
andDrakeVisualizer
could resolve package URI's inside these model format files, but at least in my initial testing, it appears the functionality is already there once the Drake package is added to thePackgeMap
. I repurposed a basic parse & visualize test to show the cube mesh could be loaded intoDrakeVisualizer
.At this point, I'm interested in feedback on the design direction keeping in mind I intend to do cleanup on things like the unittests before turning this into a proper PR.
This change is