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: Fix resource URIs + file layout #10531

Closed
EricCousineau-TRI opened this issue Jan 29, 2019 · 15 comments · Fixed by #16674
Closed

models: Fix resource URIs + file layout #10531

EricCousineau-TRI opened this issue Jan 29, 2019 · 15 comments · Fixed by #16674

Comments

@EricCousineau-TRI
Copy link
Contributor

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:

  • Ensure that all resource URIs use package://drake/ (I prefer this one, as it's less awkward, and requires fewer awkward package paths to be added)
  • Prefix all folders with drake_

Relates #9500 - see design doc.

\cc @calderpg-tri

@jwnimmer-tri
Copy link
Collaborator

... 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
Copy link
Contributor

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
Copy link
Contributor Author

EricCousineau-TRI commented Jul 2, 2020

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

  • Is there a practical update policy in place for the packages?
    • Do we ever update these packages from upstream?
    • Do we ever upstream any changes?
  • Are these vendored packages a strict superset of functionality for ROS?
    • Do they work at all in ROS - e.g. relative paths as Calder pointed out
    • Do we remove/rename any frames or geometries?

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

@ggould-tri
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member

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

@jwnimmer-tri
Copy link
Collaborator

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
Copy link
Contributor Author

Sweet, that's what I was missing!

@IanTheEngineer
Copy link
Member

IanTheEngineer commented Nov 9, 2020

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
    • Add a new drake/package.xml
    • Pre-populate the PackageMap with package://drake uri
    • Create a new test URDF or SDFormat file that uses the package://drake/ syntax for its meshes
    • Add a unit test that verifies the new URDF/SDF file finds its meshes
    • Add an acceptance install test that verifies the new URDF/SDF file finds its meshes after they've been installed

  1. Deprecation of old package.xml's, coordinating with the community on this process
    • Mark the other package.xml's inside drake as deprecated
    • Rewrite all URDF/SDF's inside Drake to use the new package://drake/ uri syntax

  1. Ensure Scenegraph can load a mesh referred via package uri
    • Change Scenegraph's load message to utilize package uri's in additional to absolute path arguments
    • Ensure Drake Visualizer can process file uri's
    • Fix package.xml parser to use the package.xml's name tag rather than the directory name

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
Copy link
Collaborator

@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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator

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

RussTedrake added a commit to RussTedrake/drake that referenced this issue Jun 12, 2022
As discussed in RobotLocomotion#10531, the issue that this README was referring to
has been closed; the README is no longer relevant.
sammy-tri pushed a commit that referenced this issue Jun 13, 2022
As discussed in #10531, the issue that this README was referring to
has been closed; the README is no longer relevant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment