RobotLocomotion / drake

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

Mesh used to pass uri around for the drake visualizer. #2783

Closed amcastro-tri closed 7 years ago

amcastro-tri commented 8 years ago

A Mesh is instantiated when parsing a URDF/SDF using the Mesh constructor. For the SDF parser this happens here. The constructor is passed not only the resolved filename (which is probably the only one of interest to Mesh) but also the uri (which could contain ROS-like information about packages). The reason for this is that Mesh is used as a channel of communication with the Drake visualizer here.

Should we keep using Mesh as this communication channel between Drake and its visualizer? is that the right abstraction?

liangfok commented 8 years ago

Would a better alternative be to load the mesh data and transfer the data? Perhaps in an ideal world there will be globally accessible "parameter server" that holds the mesh data. Clients like the URDF / SDF parsers and Drake Visualizer can then exchange references to the parameter server. Lacking such infrastructure, I believe stuffing the actual mesh inside the communication channel may be the only truly portable route in terms of allowing the sender and receiver to reside on different PCs with different mesh files in the local file system.

amcastro-tri commented 8 years ago

Drake visualizer could also read the urdf/sdf file and get that information from there. Does it right now?

liangfok commented 8 years ago

Yes, another solution would be to have the Drake Visualizer and Drake Simulator both individually load the model separately. One issue with that is how to ensure the simulator and visualizer both have the same model once we support the dynamic loading and unloading of parts of the model (e.g., tiling the world to support larger and/or more random worlds).

amcastro-tri commented 8 years ago

Good point. @sherm1, given that System's will be const objects (and presumably RBT as well?), are we planning to support the dynamic addition/deletion of objects? that would change the world and the system in addition to the context.

david-german-tri commented 8 years ago

Here's how I've been thinking about addition/deletion of objects. Whenever some event occurs that requires an object to come or go, simulation is paused, the RigidBodyTree/System are modified, a lot of stuff in cache is invalidated, and then simulation resumes. From a C++ standpoint, this means a certain type of event handler has non-const access to the System.

So, for the purposes of this thread, I agree with @liangfok: it would be a nice step forwards to remove the requirement that sim and vis read the same mesh files from disk, and a significant step backwards to require sim and vis to read the same model files from disk.

jwnimmer-tri commented 8 years ago

I'm unclear why this deserves any brain cycles right now. We have a working system, let's leave it alone until we have a requirement to do something better. Is there some acute problem with the way things are now?

amcastro-tri commented 8 years ago

Let me recap. The constantness conversation actually wasn't the intention when creating this issue. This issue highlights a problem with our "Geometry" support (actually none). A representation for a geometric primitive is not being used a channel between Drake and the visualizer. I personally think that is not a right abstraction and should at some point be dealt with in another way. I am not sure what that other way would be and I am asking for people's options here. If you were a user of Mesh you would just like to instantiate it something like Mesh(file_name) but not like Mesh(uri, file_name). I don't think the mesh should know at all about any uri, that seems like a hack to me to the lack of a better communication channel between Drake and the visualizer.

liangfok commented 8 years ago

Here are Mesh's current constructors:

  explicit Mesh(const std::string &filename);
  Mesh(const std::string &filename, const std::string &resolved_filename);

I recommend removing the second one that contains both a filename and a resolved_filename. The resolution of the file name should be hidden within the parser. Luckily, the resolution of the file name is already being done in the SDF and URDF parsers here and here, respectively.

I would then load the mesh itself into the Mesh object and then send the entire mesh to the visualizer over the wire interface, or directly use the loaded mesh in collision modeler or simulated sensor.

jwnimmer-tri commented 8 years ago

Do you actually want Director to use the same mesh as the rigid body? Drake's collision Geometry specifically looks for an *.obj extension, even if the SDF or URDF file names a different format as its preference -- do you want to cripple Director to rely on the obj data, or use the preferred format for rendering?

Again -- why is this a problem, what is the actual problem we are trying to solve, and why are we spending time and energy on it?

liangfok commented 8 years ago

The fact that Drake's collision geometry only supports .obj files is technical debt. It should be able to consume a wide variety of mesh file formats.

Also, just because Drake's collision geometry uses .obj does not mean Director must use a .obj-based mesh. Drake can load multiple meshes, e.g., one for collision modeling and one for visualization / sensing. Drake can then decide which to send to the visualizer.

@amcastro-tri, are the actual problems we're trying to solve the following?

1 .Drake's Mesh abstraction is insufficiently portable / expressive.

  1. Drake's Mesh abstraction too tightly couples Drake with the visualizer.
  2. Drake currently has no support for visual meshes, which should ideally be used by the simulated sensors.
amcastro-tri commented 8 years ago

I would say number 2. Why should a simple geometric primitive like Mesh be the channel to pass Drake's visualizer something like a uri? Sending the whole mesh through LCM seems like an overkill, especially with large files. I would just pass the visualizer the relative path (relative to drake-distro/) to the obj file.

To give good examples of what a mesh primitive should be designed like take a look at VTK's polydata or CGAL's polyhedron.

jwnimmer-tri commented 8 years ago

Sure, item 2 is slightly annoying -- to have an ornament on the collision ADT that helps us visualize it. But what is the actual problem? Is it difficult to write or refactor code with this one extra member field hanging out there? It is difficult to create new call sites to meshes due to this? I can understand that it's bothersome, but fixing it will take effort, and I'm not sure why we should invest in that right now.

rdeits commented 8 years ago

It looks like you probably don't want to send the entire mesh contents over LCM, which seems reasonable. But, for what it's worth, that functionality does exist. The decoding happens here: https://github.com/RobotLocomotion/director/blob/master/src/python/director/drakevisualizer.py#L88 and an example of encoding is here: https://github.com/rdeits/DrakeVisualizer.jl/blob/master/src/DrakeVisualizer.jl#L79

I've found that it works just fine, and performance for even something as complex as the entire Valkyrie mesh was acceptable.

amcastro-tri commented 7 years ago

I think this will be very well handled by GeometryWorld. @SeanCurtis-TRI, keep it in your radar though.