RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.33k stars 1.26k forks source link

models: Fix resource URIs + file layout #10531

Closed EricCousineau-TRI closed 2 years ago

EricCousineau-TRI commented 5 years ago

At present, URDFs / SDFs in Drake specify things using relative paths, rather than proper URIs. For compatibility with tools like ROS, we should make them specify package paths.

In addition, we shouldn't provide packages with generic descriptions (like iiwa_description) in Drake, as it can shadow other non-Drake packages. Possible alternatives are:

Relates #9500 - see design doc.

\cc @calderpg-tri

jwnimmer-tri commented 4 years ago

... we shouldn't provide packages with generic descriptions (like iiwa_description) in Drake, as it can shadow other non-Drake packages.

The counter-argument is that Drake is providing an implementation of those model packages (by vendoring them + fixing bugs), so should use the stock naming for interoperability with consumers that expect the standard name.

I don't know which direction ends up best, but I think it's important to discuss and confirm the above assertion, instead of taking it as an axiom.

calderpg-tri commented 4 years ago

Drake is providing an implementation of those model packages

The counter to this is that Drake is providing a deliberately ROS-incompatible variant of these packages, so interoperability with customers used to the "standard" version of the package is quite poor.

EricCousineau-TRI commented 4 years ago

Aye. Perhaps the best way to confirm the assertion is to verify:

I can get around to this in (hopefully) 2 weeks.

ggould-tri commented 4 years ago

FYI: This is in effect a prerequisite to remove the much-disliked AddPackage model directive from the model directive language. The AddPackage directive is used exclusively to handle the large number of miscellaneous drake packages.

jwnimmer-tri commented 4 years ago

I've come around to prefer package://drake/ and have just a single package.xml within Drake (+ the Drake install), to keep things simple and best reflect the monorepo / monoreleate nature of our current stack.

If in the future we start to split descriptions into their own small packages, then at that point we can give their own package.xml files.

I gave @IanTheEngineer a longer f2f debrief of my thoughts on this.

jwnimmer-tri commented 4 years ago

The next question here is exactly how to make the transition. I'll work on writing up some longer thoughts on that topic, but @IanTheEngineer in the meantime as a first step, I know that we're going to need a PackageMap::Remove(const std::string& package_name) method as part of the new story. If you're game, you could PR that to get us started.

IanTheEngineer commented 4 years ago

That sounds like a good plan. I'll look into adding a PackageMap::Remove function with that signature.

jwnimmer-tri commented 3 years ago

I know that we're going to need a PackageMap::Remove(const std::string& package_name) method as part of the new story.

To elaborate...

My proposal is that a default-constructed PackageMap begins life with package://drake pre-populated. If a user did not want this, we should offer a way to remove it.

EricCousineau-TRI commented 3 years ago

Sweet, that's what I was missing!

IanTheEngineer commented 3 years ago

Per f2f chat with @jwnimmer-tri , we discussed how to tackle the rest of this issue. Add a Drake package.xml so that mesh resources can be referenced using the uri syntax of package://drake/. We enumerated the following steps:

1) Create a top-level drake package.xml

2) Deprecation of old package.xml's, coordinating with the community on this process

3) Ensure Scenegraph can load a mesh referred via package uri

I'll prototype this in a quick ~& hacky~ way as a proof of concept, then if all goes well, start on the list above. I'd welcome comments and feedback on the plan in the mean time.

jwnimmer-tri commented 2 years ago

@EricCousineau-TRI / @IanTheEngineer / @cottsay what work still remains here?

Maybe the only task remaining is to convert all bare filenames in sdf & urdf files in Drake to use package URIs instead?

IanTheEngineer commented 2 years ago

That is correct, Jeremy. We still need to convert each sdf and urdf that references an external file in Drake to use the Drake package URI (replacing relative package:// paths). That should be the final task until the deprecations expire and we need to remove the corresponding package.xml's.

jwnimmer-tri commented 2 years ago

When the package.xml removal lands on 2022-03-01, we should also land the https://github.com/jwnimmer-tri/drake/commits/no-populate-upstream branch, which removes the now-pointless "populate upstream" hunting.

RussTedrake commented 2 years ago

The README in manipulation/models currently says "After the resolution of #10531, this should be simplified and more compatible."

Since this issue is now closed, presumably that README needs an update?

jwnimmer-tri commented 2 years ago

Seems like yes. I think we could actually just delete the README, since the incorrect statements are its entirety.