Should use QueryObject / better geometry introspection for visualization

calderpg-tri commented 5 years ago

Available geometry introspection allows the Shape of the geometry to be retrieved, but does not allow the role, source, or properties of the geometry to be retrieved. In the context of an export-to-RViz visualizer, the following need to be available:

Source name for a given GeometryId or FrameId. This could be accomplished by making GeometryState::get_source_id(FrameId frame_id) public.
Role of geometry, to distinguish between visual and collision geometry.
IllustrationProperties of visual geometry.

The existing export-to-LCM pipeline relies on a number of internal methods to retrieve this information, so the simplest test that sufficient information is exposed is for GeometryVisualizationImpl::BuildLoadMessage to only use public methods and types.

@SeanCurtis-TRI I've assigned you for now since you did the other geometry introspection work.

EDIT(eric): My suggest checklist (2020/04/28)

[x] #10521 Ensure introspection has remaining API exposed in C++ and bound in Python
[x] Update Drake's visualization stuff to use it
- [x] meshcat_visualizer (#14292)
- [x] planar_scenegraph_visualizer (#15456)
- [x] (Issue: #13597; PRs #14247 #14282) ConnectDrakeVisualizer
[x] #15534 (for resolving ports: #12214)
[x] Celebrate

SeanCurtis-TRI commented 5 years ago

Several thoughts:

get_source_id() will never be public. SceneGraph cannot leak SourceIds otherwise one system can unilaterally hijack another systems "property". That's not to say that a SourceId can't be shared, but SceneGraph cannot be the agent of that. However, SceneGraphInspector::get_source_name() seems reasonable.
The SceneGraphInspector does have GetProximityProperties() and GetIllustrationProperties(). (The latter directly addresses your third point). It also implicitly reports on the role -- if either returns nullptr then the geometry does not have the corresponding role.

So, as I see it, what you're missing is access to the name of the source that owns an entity.

Something you should know, this quarter I'm going to add the generic visualization interface. It includes two ports: one for getting a description of the scene sufficient for populating a visualizer (which should be evaluated in an "initialization" sense) and the other port that reports poses for all of the frames indicated in the former. So, all of these other things are stop-gap efforts.

calderpg-tri commented 5 years ago

Thoughts:

Sure, a SceneGraphInspector::get_source_name(FrameId frame_id) or SceneGraphInspector::get_source_name(GeometryId geometry_id) would solve the source name issue. I suggested get_source_id since that's what the export-to-LCM pipeline uses. To the extent that a lot of the public API of GeometryState involves SourceIds, making the very SourceIds that you would need to call said API hidden is a curious choice.
The split between information available through GeometryState (which actually has all the information) and through SceneGraphInspector doesn't make much sense. A GeometryState is much more convenient, being directly accessible via a context, whereas the inspector requires a query object from eval'ing the geometry query port. If the information can be made accessible in SceneGraphInspector, keeping it private in the state makes no sense.

I would strongly suggest against developing a visualization interface until at least a public-facing RViz interface exists, so that multiple in-drake example consumers exist. I am skeptical that a generic visualization interface makes sense at all, versus several clear examples and enough sugar on geometry introspection to make it easy to work with. The majority of work in a visualizer interface is always going to be geometry conversion specific to the visualizer - as an estimate, of the ~420 lines in our RViz interface, at most about 40 or so involve interacting with SG or MBP - so the value-add of a generic interface is unclear.

I've renamed the issue to better capture the problem.

sherm1 commented 5 years ago

Those are some good arguments, @calderpg-tri !

SeanCurtis-TRI commented 5 years ago

SourceIds are only necessary to change things in SceneGraph. They are unnecessary for learning about it (i.e., I can look up the properties of an arbitrary GeometryId, whether I have the SourceId or not, but I can't remove it, or hang a geometry on someone else's frame, etc.)
I'm unclear on your second point. It seems to touch on two subjects:
- Division between GeometryState and SceneGraphInspector.
- If there appears a conflict in information, it's because you're looking into a non-public API. The GeometryState is not for public consumption (well, not anymore). It would be clearer if I moved it into an internal namespace (but that's one of the technical debt issues that I haven't tackled because I have so much new feature debt. The division is simple: SceneGraph is used to populate the world, QueryObject is used to perform queries on the scene graph (note lower case) that depend on geometry source-calculated poses, and SceneGraphInspector provides queries that don't depend on those input poses. The fact that the implementation of all of those things happen to be wrapped into a single class should be an under-the-hood implementation detail.
- Any code that is currently accessing the GeometryState directly is going to break in the "near" future. The current nature of GeometryState is a relic of a deprecated design goal and predates caching. To make SceneGraph cache compatible, GeometryState is going to change significantly. Subverting the SceneGraphInspector and QueryObject is loading up on technical debt that will burn you in the future. Although, I am sympathetic with the gripe about evaluating contexts when you're not entangled with the Systems framework. A
- The second issue isn't clear to me. I'm not sure what "keeping it private in the state makes no sense" means -- particular what the "it" in that sentence is. I assume it's related to the GeometryState::get_source_id(FrameId) method, but am otherwise unsure. Feel free to elaborate if you feel I'm missing something important.
The visualization interface was actually a request of @RussTedrake (almost a year ago in #8473) based on servicing a meshcat visualization. So, waiting on RViz isn't strictly necessary.
- However, your concern about being the right interface is dead on. However, It may turn out that simply using the SceneGraphInspector is enough. Although, I can conceive of something that walks the tree for you and filters out anything non-visualization would improve the experience. Right now, my primary concern about doing visualization is that you have to infer the structure of the tree encoded in the SceneGraph and that seems like an undue burden on the visualization code.

calderpg-tri commented 5 years ago

The second issue comes from the fact that most of the public API of SceneGraphInspector just calls methods on GeometryState (see GetProximityProperties() as an example), in this pattern:

val SceneGraphInspector::Fn(Arg arg) const {  // public
  return state_->Fn(arg);  // private
}

so having some of these methods in GeometryState public and others private doesn't make sense.

I specifically mentioned RViz on the basis that the drake visualizer-via-LCM and RViz interfaces are both C++ and essentially identical in operation, so any future sugar/interface should be used by both. (Likewise, neither should ever use private or internal bits).

I don't want to speak for @RussTedrake on this, but I don't think we need a visualization-specific interface. All visualizers (and other geometry-aware use cases) need "what are the geometries?" ex:

const auto& query_object = plant.EvalAbstractInput(plant_context, plant.get_geometry_query_input_port().get_index())->GetValue<drake::geometry::QueryObject<double>>();
const auto& inspector = query_object.inspector();
const auto& all_geometries = inspector.all_geometry_ids();
for (const auto geometry_id : all_geometries) {
  // do stuff
}

and "where are the frames?" ex:

const auto& pose_vector =  plant.get_geometry_poses_output_port().Eval<drake::geometry::FramePoseVector<double>>(plant_context);
for (auto id : pose_vector.frame_ids()) {
  const auto& X_WF = pose_vector.value(id);
}

which aren't hard now and would be even simpler with a little sugar.

Meshcat support means this needs to be accessible in Python, so somewhat more work may have to happen on that end, but I don't think the structure needs to differ.

SeanCurtis-TRI commented 5 years ago

I think you're biased by MBP's relationship with SG. MBP chose to register all its frames as children of the world frame: so X_WF frame is sufficient for describing the pose of every frame owned by MBP. But that's not a fundamental property of SceneGraph. SceneGraph can support arbitrary trees of frames (with geometries hanging on them). And to visualize that would require either a) rebuilding the tree in visualization, or b) re-calculating the X_WF from the various X_PF that are reported. Visualizers need to be expressed to be compatible with either.

I would offer a slight modification to the phrase "what are the geometries", to "what's in the scene and how is it organized?" But I agree, that's "all" a visualizer needs to know.

tri-ltyyu commented 5 years ago

Need to discuss the right code patterns that works for the different paths (based on visualization and non-visualizaiton uses). Next step is for Calder, Sean, and Eric should meet to talk about this.

tri-ltyyu commented 5 years ago

Sean has created some test code and will review with Calder and team.

SeanCurtis-TRI commented 5 years ago

Update: Meeting was held 1/29/2019. Notes can be found here: https://docs.google.com/document/d/1S2A0kqBRumaqfyaUP_Qow8TDpxvqFWyjf9VHVWttO4s/edit (access only available for TRI internal).

Next action: move the code snippet from a gist to a temp repo to facilitate discussion via Reviewable.