Prebuilt drake-visualizer fails to load obj/mtl files

[jadecastro]

#6259 has apparently broken AutomotiveSimulator's ability to render cars and RoadGeometries correctly within DrakeVisualizer. This has been verified on both Trusty and Xenial.

When running out-of-box simulations such as bazel run //drake/automotive:demo -- --num_dragway_lanes=3 --num_trajectory_car=12, the following is what is visualized:

For commits earlier than #6259, we have the following:

[jadecastro]

/cc @jwnimmer-tri, @jamiesnape

[jwnimmer-tri]

@jamiesnape are you the right person to shepherd this through a fix? If not, please reassign (back to me is OK).

[jwnimmer-tri]

(The work-around for now is to manually launch a drake-visualizer from some historical CMake build. It should render correctly, even when the demo launches its own visualizer that doesn't.)

[jamiesnape]

To clarify, this is the issue with vtkOBJImporter that we have discussed?

@jwnimmer-tri Yes, just trying to work out the best plan of action. We will need to talk to VTK, I think.

[jwnimmer-tri]

To clarify, this is the issue with vtkOBJImporter that we have discussed?

Yes.

[jamiesnape]

The background is that a fork of VTK's vtkOBJImporter was previously being used (RobotLocomotion/director@973d182) and now drake-visualizer uses upstream.

[jwnimmer-tri]

I guess I'm no longer as sure what's exactly wrong.

This is my current work-around, which indicates it might not be the MTL problem in fact?

--- a/drake/automotive/models/prius/prius_with_lidar.sdf
+++ b/drake/automotive/models/prius/prius_with_lidar.sdf
@@ -425,7 +425,7 @@
         <pose>-2.27 -0.91139 -0.21858 0 0 0</pose>
         <geometry>
           <mesh>
-            <uri>package://prius/prius.dae</uri>
+            <uri>prius.obj</uri>
           </mesh>
         </geometry>
         <material>

Maybe some automatic DAE->OBJ heuristic got lost?

[jwnimmer-tri]

Ah. From drake/systems/rendering/drake_visualizer_client.cc:

    case DrakeShapes::MESH: {
      ...
      if (mesh.uri_.find("package://") == 0) {
        geometry_data.string_data = mesh.uri_;
      } else {
        geometry_data.string_data = mesh.resolved_filename_;
      }

I suspect the problem is that because Director and Drake no longer share a superbuild, the PackageMap logic isn't working somehow.

[avalenzu]

@jwnimmer-tri, that seems like a separate issue from the OBJ/MTL issue, no?

[jwnimmer-tri]

Yes, though (from my quick testing) it is the only reason why Jon's example in the first post doesn't work; the automotive demo's problem does not appear to be related to the VTK5 OBJ importer stuff.

[patmarion]

This is the package map logic in the drakevisualizer.py side of things:

https://github.com/RobotLocomotion/director/blob/master/src/python/director/drakevisualizer.py#L184

I think you might find that the prius loading works because drake has a prius.vtp that is loaded in place of prius.obj, which was an earlier workaround for lack of .mtl support. But the road geometries require .obj and .mtl loading.

[clalancette]

@stonier asked me to take a quick look at this since it is holding us up at the moment. I ran the demo briefly, and I saw the following output:

[drake_visualizer]   File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/0e5a44dfdb0fa037a3af759ace4c4232/execroot/drake/bazel-out/clang-3.9-linux-opt/bin/drake/automotive/demo.runfiles/drake/external/director/lib/python2.7/dist-packages/director/drakevisualizer.py", line 264, in createGeometry
[drake_visualizer]     polyDataList, visInfo = Geometry.createPolyDataFromFiles(geom)
[drake_visualizer]   File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/0e5a44dfdb0fa037a3af759ace4c4232/execroot/drake/bazel-out/clang-3.9-linux-opt/bin/drake/automotive/demo.runfiles/drake/external/director/lib/python2.7/dist-packages/director/drakevisualizer.py", line 196, in createPolyDataFromFiles
[drake_visualizer]     filename = Geometry.resolvePackageFilename(geom.string_data)
[drake_visualizer]   File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/0e5a44dfdb0fa037a3af759ace4c4232/execroot/drake/bazel-out/clang-3.9-linux-opt/bin/drake/automotive/demo.runfiles/drake/external/director/lib/python2.7/dist-packages/director/drakevisualizer.py", line 185, in resolvePackageFilename
[drake_visualizer]     import pydrake
[drake_visualizer] ImportError: No module named pydrake

(I shortened the backtrace for brevity). From that, I surmised that the problem was that the visualizer frontend was failing in the very first resolvePackageFilename(), which happens to be the prius.dae. The parts that we can see (the chassis floor, wheels, hubs, etc) are instantiated before that. Thus, it seemed to me to be a PYTHONPATH problem. Indeed, if I make automotive_demo.py have a dependency on //drake/bindings:pydrake, it seems to fix the problem. I'll open a PR in a few minutes with that.

[jamiesnape]

Does the demo need the dependency or the drake_visualizer target?

[clalancette]

@jamiesnape Good point. Let me try out just the visualizer target.

[jamiesnape]

(You may need to add the location to the PYTHONPATH in the wrapper scripts.)

[jwnimmer-tri]

Indeed, if I make automotive_demo.py have a dependency on //drake/bindings:pydrake, it seems to fix the problem. I'll open a PR in a few minutes with that.

Why should the prebuilt version of drake_visualizer need to load any code (pydrake) from Drake? Whatever code it needs to run correctly should be part of the prebuilt release, yes?

[jamiesnape]

No, I don't think so.

[patmarion]

when drake-visualizer receives a load urdf request, and the request contains a package:// filename, then it imports pydrake in order to call pydrake.getDrakePath():

https://github.com/RobotLocomotion/director/blob/master/src/python/director/drakevisualizer.py#L185

This code could be refactored if we deem it unnecessary to call getDrakePath() here, for example, by relying on environment variables instead.

[clalancette]

To be honest, I'm not 100% sure what "prebuilt" means in this context. I just updated drake to the latest as of an hour ago, built, and then saw the problem. Does that use the prebuilt visualizer, or build it as part of the build process?

[jamiesnape]

It uses the prebuilt.

[jwnimmer-tri]

@patmarion Yeah, I was thinking the same thing. Possibly there should be a try block there so that if import pydrake doesn't work, we should skip it and only use the environment variables, rather than a hard fail? (Or maybe hard-fail if the environment variables are also both absent?) I guess it depends on the combination of how Drake master wants to use that logic (from a local build), contrasted with any other users that have their own builds of Drake that they want to have it search via pydrake (from an installed build).

[patmarion]

When that code was added, the expectation was that drake-visualizer was installed in the same location as the pydrake module, so failure to import pydrake should be a hard fail. If the bazel build lays out an installation such that there is no guarantee that pydrake can be imported, then i agree it makes sense to put a try/except around the import. I think without the call to pydrake.getDrakePath() it will fail to resolve the package path for the prius model, or do you have the environment variable(s) setup?

[jwnimmer-tri]

My intuition is that it would be both more reliable and less compile time (fewer dependencies) for the //tools:drake_visualizer wrapper to set DRAKE_PACKAGE_PATH directly to anchor the os.walk search for package.xml files, instead of going through pydrake.getDrakePath() to anchor the search.

[jwnimmer-tri]

... but if for some reason keeping pydrake.getDrakePath() is desirable, then I agree that having //tools:drake_visualizer depend on //drake/bindings:pydrake (or some sub-target within pydrake) would be a fine approach too.

[patmarion]

Sounds good to me.

In some scenarios drake-visualizer is launched without the wrapper (like in cmake builds such as spartan) but those builds can successfully import pydrake, and also spartan has an environment file that all users source so we can set paths there, too.

Would you like to assign someone the task of opening a director PR to add a try/except around the pydrake import? I can also do it when I have some free time/

[jwnimmer-tri]

My goal was to be sure I understood what was happening, which I now do; thanks! (I didn't realize that it was intentional for the drake-visualizer binary release to runtime load pydrake from elsewhere in order to bootstrap paths, but it makes sense now.) I am OK with @clalancette deciding whether to fix Drake's BUILD rules to make pydrake work, or else pushing forward on the change to Director.

[clalancette]

In order to get us unstuck for now, I'll probably open PR fixing the BUILD rules (or following @jamiesnape 's advice and fixing the PYTHONPATH in the drake_visualizer_*.sh). If you think we should also make the change to director, I'm happy to do that as well.

[jwnimmer-tri]

If the first PR (fixing the BUILD rules) ends up being robust, then I think it's enough.

If people later tire of waiting for all of Drake to rebuild anytime they want to launch the visualizer, then we can prune the pydrake dependency at that point.