drake_visualizer as provided by bazel is not able to find/visualize resources using package://

[RussTedrake]

example:

russt@ubuntu:~/drake-distro/drake/examples/valkyrie$ ~/drake-distro/bazel-bin/tools/drake_visualizer &
russt@ubuntu:~/drake-distro/drake/examples/valkyrie$ bazel run valkyrie_simulation

results in the visualizer spewing, e.g.

failed to resolve filename, unknown package: package://Valkyrie/urdf/model/meshes/pelvis/pelvis.dae
warning, cannot find file: package://Valkyrie/urdf/model/meshes/pelvis/pelvis.dae

users have known that this is an issue and have been replacing package:// statements in urdf files with relative paths (as demonstrated in the iiwa examples). that's not sufficient. we simply must support the package:// logic that used in essentially every urdf/sdf file.

@jwnimmer-tri pointed to #6685 as related. is that the proposed fix? this is a specific and high priority problem, so I think it warrants a separate issue to track it.

cc @siyuanfeng-tri @TristanThrush @patmarion

[RussTedrake]

N.B. - one way that this has worked in the past is that the drake-visualizer component could replace the package:// with the full path to the file, so that drake-visualizer did not need to understand package paths. The only reason that I consider this too frail is that it would not support a viewer running on a different computer. it feels like the right thing is to teach drake-visualizer to understand ROS_PACKAGE_PATH. @patmarion -- i know that we've discussed this before, and apologize for the repeated question -- but can you remind me your thoughts on that?

[patmarion]

drake-visualizer looks at ROS_PACKAGE_PATH and DRAKE_PACKAGE_PATH environment variables. It also imports the pydrake python module and calls pydrake.getDrakePath().

Most users don't have the ROS_PACKAGE_PATH environment variable set to a value that includes the drake urdf packages. But if you set this variable, it should work, please let me know if it does not.

In the cmake build, pydrake.getDrakePath() returns a working path, so that has always been a robust mechanism to find drake urdf resources. I believe the issue now is that the pydrake module does not return the correct information when executed from a bazel build.

[jwnimmer-tri]

See also #5584 and https://github.com/RobotLocomotion/drake/issues/6333#issuecomment-316782220.

[patmarion]

drake-visualizer has two methods for finding drake resources:

1. use ROS_PACKAGE_PATH
2. ask drake (by calling the drake API)

For 2., it works in cmake builds but does not currently work in bazel. I think @fbudin69500 has proposed a fix and opened a PR:

https://github.com/RobotLocomotion/drake/pull/6771

One thing I don't understand, this feature should be covered by drake's python tests, right here:

https://github.com/RobotLocomotion/drake/blob/master/drake/bindings/python/pydrake/test/testPR2IK.py#L39

[jwnimmer-tri]

The reason testPR2IK and Russ diverge is that their environments are different (in several ways).

First, that unit test declares a dependency data = ["//drake/examples/pr2:models"] in its BUILD rule, but Russ is using //tools:drake_visualizer which does not declare any dependencies on model data (or package files, or etc.). If users want to access packages by default, without having to set up extra environment variables, then the visualizer is going to have to declare data = on everything that should be find-able by default. In essence, the model data has to be compiled into it.

Second, pydrake.getDrakePath() is (on master) a hard-coded but relative path. Thus, it works when programs a run under certain conditions, but not others. We cannot make it an absolute path like it used to be in CMake, because we need it to work in installed form, binary releases, etc. The CMake spelling "works" for build-from-source users, but nobody else. The #6771 fix makes pydrake.getDrakePath() handle more environmental conditions, but it's not finished yet, so unclear whether it would solve the specific case Russ is asking about here.

There are several ways to put all the pieces together to make this work, but first we need to decide the full list of use cases we want to support, and second we need to add regression tests under CI that show what is (or isn't) working.

[RussTedrake]

Great. I agree. Let's isolate the requested use cases and put CI coverage on them.

I think fundamental to this is the question of how we instruct users to launch the drake-visualizer. My example above was launching the app directly from bazel-bin, following the instructions in https://github.com/RobotLocomotion/drake/blob/master/drake/automotive/README.md I know that https://github.com/RobotLocomotion/drake/blob/master/drake/automotive/automotive_demo.py launches drake-visualizer automatically with the rest of the apps, but I don't think that's sufficient as a workflow -- many users/developers will leave a single instance of drake-visualizer open while starting/stopping multiple apps during development. Other users may open drake-visualizer as a stand-alone executable on a completely different machine (outside the bazel workspace/workflow).

So I think supporting drake-visualizer being launched from bazel-bin (as opposed to launching only in a sandbox with the model files copied in) is a requirement. Can we agree on that?

[jwnimmer-tri]

The first part of your problem is that Director assumes that folder name is the same as package name -- it doesn't open the package.xml to read out the <name> element: https://github.com/RobotLocomotion/director/blob/97338da55042132aaf673f243ab9644ec8fd7d4b/src/python/director/packagepath.py#L8-L9. So when you renamed drake/examples/Valkyrie to drake/examples/valkyrie, visualization was going to be broken no matter what else was happening with setting the search defaults or the build system. The reason that "old CMake" builds of drake-visualizer were still working was that they still had the Valkyrie folder with the old name. I don't know if the folder name match is something ROS requires all the time anyway, or if its a Director bug that it doesn't open package.xml and read out the element.

Edit: Fixed in PR #6981.

(I have more direct reply to the prior post in draft form; stay tuned.)

[jwnimmer-tri]

Assuming the Valkyrie vs valkyrie package name has been worked around (in my case, I changed the URDF to cite the lowercase package name), the second part of your problem is that the vtm and vtp files were not being declared as dependencies, which is fixed by:

--- a/tools/install_data.bzl
+++ b/tools/install_data.bzl
@@ -33,6 +33,8 @@ def install_data(
         "urdf",
         "xml",
         "dae",
+        "vtp",
+        "vtm",
     ]
     exclude_patterns = ["**/test/*", "**/test*"]
     prod_models_include = ["**/*.{}".format(x) for x in models_extensions]

Because those files were missing, Director was falling back to using obj instead, and if you look at createPolyDataFromFiles at https://github.com/RobotLocomotion/director/blob/97338da55042132aaf673f243ab9644ec8fd7d4b/src/python/director/drakevisualizer.py#L193-L230 you will see that while the vtp and vtm handling uses the package map, the obj handling ignores it (by citing geom.string_data instead of filename). This is seems like a defect that was introduced in RobotLocomotion/director#495.

(I have more direct reply to the grand-prior post in draft form; stay tuned.)

Edit: Fixed in PR #6919.

[jwnimmer-tri]

And finally, getting back to the questions ...

[M]any users/developers will leave a single instance of drake-visualizer open while starting/stopping multiple apps during development. Other users may open drake-visualizer as a stand-alone executable on a completely different machine (outside the bazel workspace/workflow). So I think supporting drake-visualizer being launched from bazel-bin (as opposed to launching only in a sandbox with the model files copied in) is a requirement. Can we agree on that?

I certainly agree that the fully-self-launching automotive demo is not the only way, and that we should also support the use case of a long-lived visualizer process. Indeed, https://github.com/RobotLocomotion/drake/blob/master/drake/automotive/README.md#running-the-dynamics says to run the visualizer exactly as the top post in the issue -- from bazel-bin. In short: it's fine with me for bazel-bin/tools/drake_visualizer to be a way that's supported (it is already). What we lack is CI coverage of that mode while loading package:// resources from Valkyrie data, as evidenced by the two separate defect noted above that crept in.

In an terms of making the Valkyrie visualization work easily, out of the box -- don't confuse the self-contained automotive demo approach with whether or not model files are compiled in. Assuming the prior two defects are fixed, then the following patch compiles the Valkyrie data into the visualizer, such that the commands in your top post work again:

--- a/tools/BUILD
+++ b/tools/BUILD
@@ -25,18 +25,19 @@
 sh_binary(
     name = "drake_visualizer",
     srcs = select({
         "//tools:apple": ["drake_visualizer_apple.sh"],
         "//conditions:default": ["drake_visualizer_linux.sh"],
     }),
     data = [
         "//drake/bindings:pydrake",
+        "//drake/examples/valkyrie:prod_models",
         "@director",
     ],
 )

In short, if you want the visualizer to have a-priori knowledge of certain packages, just list them!

[jwnimmer-tri]

Relatedly, instead of asking users to run bazel-bin/tools/drake_visualizer in our READMEs, my own suggestion would be to do #6680 -- install the pre-compiled visualizer (and its model resources, and pydrake, and etc.) and suggest that users to run it from the installed location. We would still support bazel-bin being used directly, but it would not be the highlighted recommendation in most README's.

This mode of interaction would be more similar to what downstream users would be doing (when Drake is a library), and would transfer more smoothly to users consuming a binary release where they would necessarily be running from the install tree.

[patmarion]

while the vtp and vtm handling uses the package map, the obj handling ignores it (by citing geom.string_data instead of filename). This is seems like a defect that was introduced in RobotLocomotion/director#495.

Thanks for tracking down this bug! Fixed by https://github.com/RobotLocomotion/director/pull/546

[RussTedrake]

woohoo! Thanks for all the bug fixes @jwnimmer-tri ! (and apologies for the ones that I introduced with the name changes; my search was for examples/Valkyrie, etc which missed package://Valkyrie).
I will PR fixes to all of the effected urdfs throughout the examples directory tomorrow.

[RussTedrake]

Re: how to launch / install drake-visualizer. I don't think it's tenable to have users list all of their resources at installation time for the drake-visualizer. downstream users will have their own robot directory, with it's own resources, but will be using the precompiled visualizer provided by drake. or they may even have a runtime load of a new urdf, with its new resources. Is the plan for those cases to fall back to ROS_PACKAGE_PATH?

[jwnimmer-tri]

... my search was for examples/Valkyrie, etc which missed package://Valkyrie.

I noticed this during the review and assumed it was on purpose, to keep compatibility with other pieces of the ecosystem. I thought package names were a bit of a "commons", where everyone agrees to use the same package name for the same thing? In any case, that belief was only from anecdotal evidence, so I'm happy to be overridden.

[jwnimmer-tri]

I don't think it's tenable to have users list all of their resources at installation time for the drake-visualizer. downstream users will have their own robot directory, with it's own resources, ...

Sure. I only meant that we should install the Drake-hosted packages by default (we already do) in a way our installed version of drake-visualizer would find (which needs something like #6771 or #6772 or #6333).

Is the plan for those cases to fall back to ROS_PACKAGE_PATH?

I think that can happen however Director wants it to happen; I have no strong opinion. I think ROS_PACKAGE_PATH sounds like a great answer.

[RussTedrake]

I thought package names were a bit of a "commons"

I agree that is how they are supposed to be used. But since director (and drake before it) only support ROS_PACKAGE_PATH by directory structure, and since I don't think anybody is relying on downstream use of the packages in drake master right now, i think I should probably change them (and package.xml) to match the directory structure.

@avalenzu , @patmarion , @manuelli , @rdeits , @tkoolen -- any thoughts?

[patmarion]

I'm fine with using ROS_PACKAGE_PATH (or DRAKE_PACKAGE_PATH) to find package based on the directory structure and presence of package.xml. Some other options:

1. read each package.xml found and parse out the <name>...</name> tag.
2. call rospack find <package name>

It would be easy enough to add those mechanisms to the drake-visualizer application, but they need parallel implementations in drake c++, right? If drake c++ has implementation, can't drake-visualizer rely on drake API to locate packages? Currently drake-visualizer assumes it can import pydrake to access the drake api.

I think if you decide to use on ROS_PACKAGE_PATH or other environment variables then you might want a consistent environment setup across users. Similar to how your source a setup.bash script in ROS, or how we used to call addpath_drake back in the pods build system. Openhumanoids and spartan also have an environment file. Just something to consider, might not be required/desired in the bazel setup.

[jwnimmer-tri]

... read each package.xml found and parse out the <name>...</name> tag ... It would be easy enough to add those mechanisms to the drake-visualizer application, but they need parallel implementations in drake c++, right? ...

Ah good question; I hadn't considered that. However, looking at it now, Drake already opens the package.xml and extracts the <name> element, so I think that case is already handled.

[rdeits]

I don't feel like I understand this well enough to have an opinion, sorry.

But I do think that it should be possible for a user to, e.g., create a new urdf inside the examples folder and then load and visualize that urdf immediately without any compilation (e.g. by loading it in pydrake). As far as I can tell, that implies that urdf and package resolution cannot depend on having direct data dependencies declared in the build system.

[manuelli]

I am in a similar situation to Robin, I don't understand all the nuances here but the use case he outlined should definitely be supported. If I put my own urdf somewhere then director should be able to load it at run time in some consistent manner.

Also I usually use director via a custom app, i.e. directorPython myApp.py. This is all on the python side and requires no compilation. Many times this app needs to load some drake urdf, e.g. valkyrie, so there should be some way of locating that urdf. For me at least this is a common use case, and thus I would prefer solutions that work more generally for director apps and not only for the drake-visualizer executable.

[TristanThrush]

@jwnimmer-tri changing BUILD and install_data.bzl don't seem to fix the problem for me, but perhaps I should wait until a pull request that fixes this is submitted?

[jwnimmer-tri]

To the best of my knowledge, the software is working now. What remains is to add appropriate regression testing. I have self-assigned in order to take care of that follow-up, likely just by opening new issue(s).

[jwnimmer-tri]

Regression coverage filed under #7050.