[render] Expose rendering API through QueryObejct/SceneGraph

[SeanCurtis-TRI]

1. Finishes off GeometryState API (image rendering API).
2. SceneGraph provides API for
   • Registering a renderer
   • Acquiriging SceneGraph-owned and managed render labels
   • Assigning/removing roles
3. Simplification of RenderLabel API to facilitate SceneGraph ownership
   of render labels.
4. QueryObject provided mechanism for creating render images.
5. Slight re-working of RigidBodyPlantBridge to support rendering.

---

This change is 

[SeanCurtis-TRI]

+@sherm1 for feature review, please.
-(status: do not review)

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[SeanCurtis-TRI]

@sherm1 with all this online activity, can this poor PR that's been waiting all week get some love?

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[sherm1]

Sorry for the delay, Sean. I'll attempt to grab some wifi on the plane on the way home.

Reviewable status: LGTM missing from assignee sherm1(platform), needs at least two assigned reviewers (waiting on @sherm1)

[SeanCurtis-TRI]

-@sherm1 +@EricCousineau-TRI for feature review, please.

Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

[EricCousineau-TRI]

First pass done, nitpicks on the need for (what I view as) excessive overloads...

Reviewed 10 of 11 files at r1.
Reviewable status: 13 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

---

geometry/geometry_state.h, line 488 at r1 (raw file):

  /** Implementation support for QueryObject::RenderDepthImage() overload.  */
  void RenderDepthImage(const render::DepthCameraProperties& camera,
                        FrameId parent_frame,

Can I ask why we have two overloads, one that has parent_frame, and one that doesn't?

Why can't parent_frame always be required, and it just be the world frame if a user doesn't need it?

I generally disliked this distinction in the old RgbdCamera, because it tended to motivate branching APIs, and sometimes made the moving-camera case less obvious (since there's two entries, and thus a usage could use the non-mobile case).

---

geometry/geometry_state.h, line 717 at r1 (raw file):

  // Utility function to facilitate getting a double-valued pose for a frame,
  // regardless of the value of T.

int "value of T" sounds a bit odd. Can this be rephrased to "type T"?

---

geometry/geometry_state.cc, line 946 at r1 (raw file):

  // TODO(SeanCurtis-TRI): Invoke UpdateViewpoint() as part of a calc cache
  //  entry. Challenge: how to do that with a parameter passed here?
  const_cast<render::RenderEngine&>(engine).UpdateViewpoint(X_WC);

This (understandably) violates the const declaration.

Is there a way this can be explicitly called out in an issue, with Sherm Cc'd?
(Then following references can just refer to the issue.)

---

geometry/geometry_state.cc, line 956 at r1 (raw file):

                                        ImageRgba8U* color_image_out,
                                        bool show_window) const {
  // This assumes that the poses in the engine have already been updated.

nit Should this comment be hoisted to the doxygen group in the header file?

---

geometry/query_object.h, line 387 at r1 (raw file):

   relative to the given parent frame P with the pose X_PC.  */
  void RenderColorImage(const render::CameraProperties& camera,
                        FrameId parent_frame,

FYI Yeah, per prior comment, I'm not enthused to see all this extra overload noise for supporting and implicit parent_frame.

---

geometry/query_object.h, line 392 at r1 (raw file):

                        bool show_window) const;

  /** Renders a depth image for the given `camera.

nit minor Missing closing backtick `camera`

---

geometry/query_object.cc, line 75 at r1 (raw file):

template <typename T>
std::vector<PenetrationAsPointPair<double>>

nit Can I ask why this was reordered?
(To match the *.h file?)

---

geometry/scene_graph.h, line 41 at r1 (raw file):

 @system{SceneGraph,
   @input_port{source_pose{0}} @input_port{...} @input_port{source_pose{N-1}},

nit N is not introduced before or immediately after this chart.
Can you add that in?

---

geometry/scene_graph.h, line 42 at r1 (raw file):

 @system{SceneGraph,
   @input_port{source_pose{0}} @input_port{...} @input_port{source_pose{N-1}},
   @output_port{lcm_visualization} @output_port{query}

Is there a TODO to not call this port lcm_visualization?

---

geometry/scene_graph.h, line 473 at r1 (raw file):

  //@}

  /** Adds a new render engine to this %SceneGraph. The %SceneGraph owns the

nit Can you add these to @name Rendering or something like that?

---

geometry/scene_graph.h, line 495 at r1 (raw file):

  std::vector<std::string> RegisteredRendererNames() const;

  /** @name     SceneGraph management of render labels

nit It's kinda weird to have this section introduce both concepts, and then say it only concerns itself with workflow 2.

Is there a way to make that flow a little bit more generally-scoped?
Can this label management be hoisted to a separate section, and then this section becomes purely the SceneGraph part?

Or can this cross reference existing documentation?
(e.g. in one of the RenderLabel* components?)

---

geometry/scene_graph.h, line 581 at r1 (raw file):

  /** systems::Context-modifying variant of
   @ref AssignRole(SourceId,GeometryId,ProximityProperties) "AssignRole()" for

BTW Tiny sad-face to see these API duplicates (internal state + context-modifying variants), when it could all channel through GeometryState orthogonaly, esp. given that it hasn't been exploded apart for caching purposes....

---

geometry/scene_graph.h, line 615 at r1 (raw file):

  /** Removes the indicated `role` from any geometry directly registered to the
   frame indicated by `frame_id` (if the geometry has the role).
   Note: if removing a _perception_ role, it will remove the geometry from _all_

nit Can this Note: be changed to @note?

---

geometry/scene_graph.h, line 620 at r1 (raw file):

   "RemoveFromRenderer()" variant for frames.
   @return The number of geometries affected by the removed role.
   @throws std::logic_error if 1) `source_id` does not map to a registered

BTW Super tangential: Albeit consistent, visual indents that get rid of a lot of usable whitespace and require 3+ extra line breaks also give me tiny sad faces :(

---

geometry/scene_graph.h, line 636 at r1 (raw file):

  /** Removes the indicated `role` from the geometry indicated by `geometry_id`.
   Note: if removing a _perception_ role, it will remove the geometry from _all_

nit Same as above for @note

---

geometry/scene_graph.h, line 666 at r1 (raw file):

                            id), or 4) the context has already been
                            allocated.  */
  int RemoveFromRenderer(const std::string& renderer_name, SourceId source_id,

Er... Sorry, I'm not sure if I caught / noticed this in the prior dev/SceneGraph versions.

I can see a hypothetical workflow for this, but can I ask if there was a concrete workflow for this?

Why can't this somehow be something that's a filter stashed in a property, e.g.
AssignRole(..., RenderEngineFilter::ExcludeFrom(bad_engines))?

[SeanCurtis-TRI]

Comments addressed, PTAL.

Reviewable status: 5 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

---

geometry/geometry_state.h, line 488 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Can I ask why we have two overloads, one that has parent_frame, and one that doesn't?

Why can't parent_frame always be required, and it just be the world frame if a user doesn't need it?

I generally disliked this distinction in the old RgbdCamera, because it tended to motivate branching APIs, and sometimes made the moving-camera case less obvious (since there's two entries, and thus a usage could use the non-mobile case).

Done I'm glad you said this. I'd pondered it myself. This was enough to tip me over the edge.

---

geometry/geometry_state.h, line 717 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

int "value of T" sounds a bit odd. Can this be rephrased to "type T"?

Done

---

geometry/geometry_state.cc, line 946 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

This (understandably) violates the const declaration.

Is there a way this can be explicitly called out in an issue, with Sherm Cc'd?
(Then following references can just refer to the issue.)

I don't believe that's necessary -- it's all related to making SceneGraph cache friendly. And that's part of my Q3 OKRs. Hardly going to fall in the cracks.

---

geometry/geometry_state.cc, line 956 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Should this comment be hoisted to the doxygen group in the header file?

Done

---

geometry/query_object.h, line 387 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

FYI Yeah, per prior comment, I'm not enthused to see all this extra overload noise for supporting and implicit parent_frame.

Done

---

geometry/query_object.h, line 392 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit minor Missing closing backtick `camera`

Done

---

geometry/query_object.cc, line 75 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can I ask why this was reordered?
(To match the *.h file?)

OK Yeah....someone else had modified these files but hadn't maintained the order. Sorry about muddying the PR.

---

geometry/scene_graph.h, line 41 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit N is not introduced before or immediately after this chart.
Can you add that in?

Done

---

geometry/scene_graph.h, line 42 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Is there a TODO to not call this port lcm_visualization?

Nope. But it would change if/when we have a generic visualization port.

---

geometry/scene_graph.h, line 473 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can you add these to @name Rendering or something like that?

Done

---

geometry/scene_graph.h, line 495 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit It's kinda weird to have this section introduce both concepts, and then say it only concerns itself with workflow 2.

Is there a way to make that flow a little bit more generally-scoped?
Can this label management be hoisted to a separate section, and then this section becomes purely the SceneGraph part?

Or can this cross reference existing documentation?
(e.g. in one of the RenderLabel* components?)

Done

---

geometry/scene_graph.h, line 581 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Tiny sad-face to see these API duplicates (internal state + context-modifying variants), when it could all channel through GeometryState orthogonaly, esp. given that it hasn't been exploded apart for caching purposes....

OK I know what you mean. But no matter what we do, we're kind of stuck. We have a number of bad options and this seems the least egregious. (At least, without take 100 steps back and doing something wholly different that isn't System dependent).

---

geometry/scene_graph.h, line 615 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Can this Note: be changed to @note?

Done

---

geometry/scene_graph.h, line 620 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Super tangential: Albeit consistent, visual indents that get rid of a lot of usable whitespace and require 3+ extra line breaks also give me tiny sad faces :(

BTW Whereas, I get really sad with poor use of whitespace in name of line count. :)

---

geometry/scene_graph.h, line 636 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Same as above for @note

Done

---

geometry/scene_graph.h, line 666 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Er... Sorry, I'm not sure if I caught / noticed this in the prior dev/SceneGraph versions.

I can see a hypothetical workflow for this, but can I ask if there was a concrete workflow for this?

Why can't this somehow be something that's a filter stashed in a property, e.g.
AssignRole(..., RenderEngineFilter::ExcludeFrom(bad_engines))?

Your question is a bit facile? Ultimately, you're asking "Why can't this API be a different API?" Of course it can be a different API. Is that one particularly better? I don't know.

All I know is that this was born of two things:

1. Anzu's request for post hoc role removal (at the time, specifically for collision), and
2. logical consistency.

Since the functionality was necessary anyways to remove geometry, it made sense to expose it as a public API (more or less).

[EricCousineau-TRI]

Reviewed 6 of 7 files at r2.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

---

geometry/geometry_state.h, line 488 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Done I'm glad you said this. I'd pondered it myself. This was enough to tip me over the edge.

OK Huzzah!

---

geometry/geometry_state.cc, line 946 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I don't believe that's necessary -- it's all related to making SceneGraph cache friendly. And that's part of my Q3 OKRs. Hardly going to fall in the cracks.

OK SGTM.

---

geometry/scene_graph.h, line 581 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

OK I know what you mean. But no matter what we do, we're kind of stuck. We have a number of bad options and this seems the least egregious. (At least, without take 100 steps back and doing something wholly different that isn't System dependent).

OK aye, understood.

---

geometry/scene_graph.h, line 620 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Whereas, I get really sad with poor use of whitespace in name of line count. :)

Hm... Each of us may need to submit requests for pet pandas for indicate our sadness :P

---

geometry/scene_graph.h, line 666 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Your question is a bit facile? Ultimately, you're asking "Why can't this API be a different API?" Of course it can be a different API. Is that one particularly better? I don't know.

All I know is that this was born of two things:

1. Anzu's request for post hoc role removal (at the time, specifically for collision), and
2. logical consistency.

Since the functionality was necessary anyways to remove geometry, it made sense to expose it as a public API (more or less).

I understand the goal to remove a geometry entirely.
I'm not sure if I understand the motivation for removing a geometry from a specific renderer, or at least exposing that publicly.

Rephrasing my question w/ mebbe a more useful suggestion:
Would would a user want to remove a geometry from a renderer, vs. just removing the geometry entirely?
If this is an answer in the hypothetical, can we minimize API surface area and keep this private for now, until it becomes a concrete request?

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

---

geometry/scene_graph.h, line 620 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Hm... Each of us may need to submit requests for pet pandas for indicate our sadness :P

I hadn't realized we had a panda service. I need to keep better abreast of our benefits.

---

geometry/scene_graph.h, line 666 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I understand the goal to remove a geometry entirely.
I'm not sure if I understand the motivation for removing a geometry from a specific renderer, or at least exposing that publicly.

Rephrasing my question w/ mebbe a more useful suggestion:
Would would a user want to remove a geometry from a renderer, vs. just removing the geometry entirely?
If this is an answer in the hypothetical, can we minimize API surface area and keep this private for now, until it becomes a concrete request?

A compelling point well made. I've pulled it back from the SceneGraph API. I'm slightly inclined to leave it in the GeometryState API just so I don't have to recreate it (with a note indicating why it is there). However, that may still be overly frivolous. Arguments in keeping it are tenuous but is better guaranteed to prevent code rot than if I move them off into my own branch. Your vote?

[SeanCurtis-TRI]

Another pass

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

[SeanCurtis-TRI]

Back to you.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

[EricCousineau-TRI]

Reviewed 1 of 3 files at r3.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

---

geometry/geometry_state.h, line 52 at r3 (raw file):

/**
 The context-dependent state of SceneGraph. This serves as an AbstractValue

Below, I wrote that it's OK to keep the removal in just because GeometryState is internal; however, reading the docs and looking at the namespaces, there's nothing in this file to indicate as much.

Is there a way to explicitly state that this is not intended for public consumption?
For now, I'm totes fine with: (Advanced only) Please use %QueryObject for public API., mebbe with a TODO to seal this into namespace internal.

---

geometry/geometry_state.h, line 365 at r3 (raw file):

  // determine definitively if these methods *should* exist (in which case I
  // put them in the SceneGraph API or not, in which case I can remove them
  // entirely.

BTW Missing closing parenthesis.

---

geometry/scene_graph.h, line 666 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

A compelling point well made. I've pulled it back from the SceneGraph API. I'm slightly inclined to leave it in the GeometryState API just so I don't have to recreate it (with a note indicating why it is there). However, that may still be overly frivolous. Arguments in keeping it are tenuous but is better guaranteed to prevent code rot than if I move them off into my own branch. Your vote?

Given that GeometryState is effectively internal, not strong opinions there. I think the doc comment sufficiently explains all dat business.

(I can demote this to non-blocking if there's something that explicitly states that GeometryState is not intended for public consumption...)

[SeanCurtis-TRI]

Last lob back to you.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

---

geometry/geometry_state.h, line 52 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Below, I wrote that it's OK to keep the removal in just because GeometryState is internal; however, reading the docs and looking at the namespaces, there's nothing in this file to indicate as much.

Is there a way to explicitly state that this is not intended for public consumption?
For now, I'm totes fine with: (Advanced only) Please use %QueryObject for public API., mebbe with a TODO to seal this into namespace internal.

Done

---

geometry/geometry_state.h, line 365 at r3 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Missing closing parenthesis.

Done

---

geometry/scene_graph.h, line 666 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Given that GeometryState is effectively internal, not strong opinions there. I think the doc comment sufficiently explains all dat business.

(I can demote this to non-blocking if there's something that explicitly states that GeometryState is not intended for public consumption...)

Done

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

---

geometry/scene_graph.h, line 539 at r4 (raw file):
I've modified the documentation to help make its purpose clearer. Some thoughts:

1. Exposing SourceId is a strict no-no.
2. The names provided are not inputs anywhere.

My issue is the asymmetry between inserting / retrieving individual values, and then retrieving all available values.

This seems easily resolved with a function that takes a single RenderLabel and returns the mangled name. Symmetry is achieved. I could even go further in allowing a parameter that says "don't mangle me" (although I would advocate against that.)

Ultimately, the mangling is 100% required (predicated on point 1 above).

---

geometry/scene_graph.h, line 505 at r5 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

The RenderLabel docs mention SceneGraphInspector::GetSemanticClassNameFromLabel(), but that doesn't exist (still)?

Is that to-be-added, or has it been superseded by other functionality here?

Good catch. Modified.

[EricCousineau-TRI]

Reviewed 1 of 2 files at r6.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

---

geometry/scene_graph.h, line 539 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I've modified the documentation to help make its purpose clearer. Some thoughts:

1. Exposing SourceId is a strict no-no.
2. The names provided are not inputs anywhere.

My issue is the asymmetry between inserting / retrieving individual values, and then retrieving all available values.

This seems easily resolved with a function that takes a single RenderLabel and returns the mangled name. Symmetry is achieved. I could even go further in allowing a parameter that says "don't mangle me" (although I would advocate against that.)

Ultimately, the mangling is 100% required (predicated on point 1 above).

Given that we still want to hide SourceId -- which I understand that rationale for, but may continue trying to poke holes in these assumptions -- yeah, that'd work.

I would suggest to further simplify it - always mangle the name with the source ID, even if it's the only occurrence. Otherwise, the mangling could change at separate points in time, and thus anything that tries to map this gets invalidated.
Additionally, you may want to ensure that there aren't collisions due to the manging - e.g. what if there's "source 1", and I register "plate", as well as the mangled name "plate (source 1)"?

Another possibility is to store pair<string, string>, where one string is the Source ID's name, rather than relying on string mangling.

(And all that being said, I'd like to start my hole-poking compaign: All of this would be greatly simplified if SourceId were not some asymmetric secret!)

[EricCousineau-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

---

geometry/scene_graph.h, line 539 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Given that we still want to hide SourceId -- which I understand that rationale for, but may continue trying to poke holes in these assumptions -- yeah, that'd work.

I would suggest to further simplify it - always mangle the name with the source ID, even if it's the only occurrence. Otherwise, the mangling could change at separate points in time, and thus anything that tries to map this gets invalidated.
Additionally, you may want to ensure that there aren't collisions due to the manging - e.g. what if there's "source 1", and I register "plate", as well as the mangled name "plate (source 1)"?

Another possibility is to store pair<string, string>, where one string is the Source ID's name, rather than relying on string mangling.

(And all that being said, I'd like to start my hole-poking compaign: All of this would be greatly simplified if SourceId were not some asymmetric secret!)

Actually, my quick and dirty proposal: Can we just keep this method as-is, but hide it entirely, and put a TODO to either hoist or delete it later?

I'm still having trouble thinking of a concrete workflow that benefits from this, and would prefer that we hide it for now?

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI)

---

geometry/scene_graph.h, line 539 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Actually, my quick and dirty proposal: Can we just keep this method as-is, but hide it entirely, and put a TODO to either hoist or delete it later?

I'm still having trouble thinking of a concrete workflow that benefits from this, and would prefer that we hide it for now?

Some nice suggestions/thoughts. I'll work on those.

The one thing you'll never persuade me of is exposing SourceId. You'd have a better chance to convince me that there should be no SourceId and that any source could modify any other source's registered data without any obstacle. (And, given how I phrased that, you can guess how likely that is).

The reason for this method comes from talking to the cloud group. I was told:

1. While there are typical "classes", there are no typical encodings of those classes.
2. Any good application that generates label image data should always include a table associating classes with numerical values. Period.

With those two thoughts, this method became a fundamental piece of the puzzle.

[EricCousineau-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

---

geometry/scene_graph.h, line 539 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Some nice suggestions/thoughts. I'll work on those.

The one thing you'll never persuade me of is exposing SourceId. You'd have a better chance to convince me that there should be no SourceId and that any source could modify any other source's registered data without any obstacle. (And, given how I phrased that, you can guess how likely that is).

The reason for this method comes from talking to the cloud group. I was told:

1. While there are typical "classes", there are no typical encodings of those classes.
2. Any good application that generates label image data should always include a table associating classes with numerical values. Period.

With those two thoughts, this method became a fundamental piece of the puzzle.

I initially typed out about 3 paragraph's response, but stashed it for now - can share it, but how's about after this PR merges? :D

Really, I just wanna see one of two things; either:
a) This method gets hidden for now, or
b) Pseudo-code of a concrete workflow that expresses how GetRenderLabel and GetRenderClasses are useful together (and don't inject artificial levels of pain through mangling)?

I believe I understand the reasons you mentioned, but they still seem very abstract w.r.t. the space of possible applications.

[sherm1]

Oh, I just got on the plane prepared to embark on this review and saw that @EricCousineau-TRI did it! Imagine my disappointment :( Now where are my headphones ... (Thanks, Eric!)

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)

[SeanCurtis-TRI]

I've killed SceneGraph's API for managing render labels. Now it is the solely the responsibility of the application to coordinate RenderLabels. That should resolve any concerns you have.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee EricCousineau-TRI(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)

---

geometry/scene_graph.h, line 539 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

I initially typed out about 3 paragraph's response, but stashed it for now - can share it, but how's about after this PR merges? :D

Really, I just wanna see one of two things; either:
a) This method gets hidden for now, or
b) Pseudo-code of a concrete workflow that expresses how GetRenderLabel and GetRenderClasses are useful together (and don't inject artificial levels of pain through mangling)?

I believe I understand the reasons you mentioned, but they still seem very abstract w.r.t. the space of possible applications.

See resolution.

[EricCousineau-TRI]

with some nits, and a question about testing for rendering, thanks!

Reviewed 10 of 10 files at r7.
Reviewable status: 5 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

---

attic/multibody/rigid_body_plant/rigid_body_plant_bridge.h, line 124 at r7 (raw file):

      const;

  /** Given a render label, reports the index of the RigidBody to which this

nit Even though this is on the way out, is it still possible to state that there should only be one source for this to work?

---

geometry/geometry_state.cc, line 947 at r7 (raw file):

      GetRenderEngineOrThrow(camera.renderer_name);
  // TODO(SeanCurtis-TRI): Invoke UpdateViewpoint() as part of a calc cache
  //  entry. Challenge: how to do that with a parameter passed here?

BTW uber minor, extra space before "entry"

---

geometry/query_object.h, line 361 at r7 (raw file):

   Eventually, there will be multiple renderers that can be invoked which vary
   in the fidelity of the images they produce. Currently, only the low fidelity

nit This comment seems stale, as it mentions "low" and "high" fidelity.
Should this be replace with something mentioning the renderer name?

---

geometry/query_object.h, line 366 at r7 (raw file):

   this same interface.

   <!-- TODO(SeanCurtis-TRI): Currently, pose is requested as a transform of

nit This <!-- doesn't seem to be closed?

---

geometry/scene_graph.h, line 484 at r7 (raw file):

   @param renderer  The `renderer` to add.
   @throws std::logic_error if the name is not unique, or geometry has already
                               been registered.  */

nit Misaligned visual indent?

---

geometry/scene_graph.cc, line 217 at r7 (raw file):

template <typename T>
bool SceneGraph<T>::HasRenderer(const std::string& name) const {

BTW Kinda weird to see const string& name + func(name) used here, but string name + func(move(name)) in the lines above.

---

geometry/test/query_object_test.cc, line 192 at r7 (raw file):

  RigidTransformd X_WC = RigidTransformd::Identity();
  ImageRgba8U color;
  EXPECT_DEFAULT_ERROR(default_object->RenderColorImage(

Dumb question: Is there anything to really catch that geometry_states rendering routines actually work?

All of these appear to be negative tests, and don't seem to refer to any other existing tests based on API forwarding...

[SeanCurtis-TRI]

+@sammy-tri for platform review, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sammy-tri)

---

attic/multibody/rigid_body_plant/rigid_body_plant_bridge.h, line 124 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Even though this is on the way out, is it still possible to state that there should only be one source for this to work?

OK I tried. I couldn't come up with a meaningful comment. It's largely because the definition of "work" is a very subtle nuance. So, unless you can propose some concrete text, I'm going to say "no" on this one.

---

geometry/geometry_state.cc, line 947 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW uber minor, extra space before "entry"

OK Nope I've taken to putting the extra space because CLion's logic for recognizing the TODO continuation is predicated on that extra space.

---

geometry/query_object.h, line 361 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This comment seems stale, as it mentions "low" and "high" fidelity.
Should this be replace with something mentioning the renderer name?

Done

---

geometry/query_object.h, line 366 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit This <!-- doesn't seem to be closed?

Done

---

geometry/scene_graph.h, line 484 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Misaligned visual indent?

Done

---

geometry/scene_graph.cc, line 217 at r7 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

BTW Kinda weird to see const string& name + func(name) used here, but string name + func(move(name)) in the lines above.

OK I don't understand your concern.

---

geometry/test/query_object_test.cc, line 192 at r7 (raw file):
Two thoughts:

1. The comment at the top of this test does say:

The correctness of those queries is handled in geometry_state_test.cc.

2. In this case, it's a lie. GeometryState does a very small amount to perform rendering, but it is clear to me that even that small amount is not tested. I've added a TODO.

[EricCousineau-TRI]

Reviewed 3 of 3 files at r8.
Reviewable status: LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @sammy-tri)

---

geometry/scene_graph.cc, line 217 at r7 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

OK I don't understand your concern.

OK My concern was an inconsistent application of using rvalue string temporaries; I had thought the usage was equivalent between the two methods, but now I realize one owns a string (copy), while the other is just for a query.

---

geometry/test/query_object_test.cc, line 192 at r7 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Two thoughts:

1. The comment at the top of this test does say:

The correctness of those queries is handled in geometry_state_test.cc.

2. In this case, it's a lie. GeometryState does a very small amount to perform rendering, but it is clear to me that even that small amount is not tested. I've added a TODO.

OK The TODO works for me!

[sammy-tri]

First pass complete. No major issues.

Reviewed 2 of 11 files at r1, 3 of 7 files at r2, 1 of 3 files at r5, 9 of 10 files at r7, 3 of 3 files at r8.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @SeanCurtis-TRI)

---

geometry/query_object.h, line 379 at r8 (raw file):

                        const math::RigidTransformd& X_PC,
                        systems::sensors::ImageRgba8U* color_image_out,
                        bool show_window) const;

https://drake.mit.edu/styleguide/cppguide.html#Output_Parameters says "If output-only parameters are used they should appear after input parameters." (and it looks like the implementation in GeometryState even swaps the order of color_image_out and show_window when calling into RenderEngine.)

---

geometry/scene_graph.cc, line 28 at r8 (raw file):

using systems::SystemTypeTag;
using std::make_unique;
using std::unordered_map;

I don't see any new uses of this in this diff? (actually I think this applies to RenderLabel above too)

Actually, I don't see any uses in the existing code for for InputPort, LeafContext, SystemOutput, SystemSymbolicInspector, or vector. (some of them are used but only with the full namespace).

---

geometry/test/scene_graph_test.cc, line 40 at r8 (raw file):

using std::make_unique;
using std::unique_ptr;
using std::unordered_map;

This doesn't seem to get used in this diff. (also RigidTransformd and RenderLabel)

[SeanCurtis-TRI]

Comments addressed (good catches, all).

Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sammy-tri)

---

geometry/query_object.h, line 379 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

https://drake.mit.edu/styleguide/cppguide.html#Output_Parameters says "If output-only parameters are used they should appear after input parameters." (and it looks like the implementation in GeometryState even swaps the order of color_image_out and show_window when calling into RenderEngine.)

Done

---

geometry/scene_graph.cc, line 28 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I don't see any new uses of this in this diff? (actually I think this applies to RenderLabel above too)

Actually, I don't see any uses in the existing code for for InputPort, LeafContext, SystemOutput, SystemSymbolicInspector, or vector. (some of them are used but only with the full namespace).

Done

---

geometry/test/scene_graph_test.cc, line 40 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

This doesn't seem to get used in this diff. (also RigidTransformd and RenderLabel)

Done

[SeanCurtis-TRI]

ping @EricCousineau-TRI and @sammy-tri for follow up. Thanks.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee sammy-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @sammy-tri)

[sammy-tri]

Reviewed 7 of 7 files at r9.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

[EricCousineau-TRI]

Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

[SeanCurtis-TRI]

+(status: squashing now)

Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),EricCousineau-TRI(platform)