[geometry] Add RenderEngine support for shared_ptr

[jwnimmer-tri]

Towards #5842 and #21968.

---

This change is 

[jwnimmer-tri]

+@SeanCurtis-TRI for feature review of the //geometry/... code, please.
+@rpoyner-tri for feature review of the //pydrake/... code, please.

Reviewed 10 of 11 files at r1, 10 of 10 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees SeanCurtis-TRI(platform),rpoyner-tri(platform)

---

a discussion (no related file):
Working

Needs (separate) platform reviewer assigned.

[SeanCurtis-TRI]

//geometry/... reviewed -- I have some open questions.

Reviewed 3 of 11 files at r1, 3 of 10 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)

---

a discussion (no related file):
Is it the case that if a user instantiates a RenderEngine in python and passes it to SceneGraph that the engine instance must be copied? I.e., the instance they had in hand was only the seed for what exists in SceneGraph, but is not a handle to it. That seems slightly unpythonic. Is it obvious to the python user that their instance got copied and operations on the in-hand instance will have no bearing on the in-SceneGraph instance? What if we allowed SceneGraph to take a shared_ptr<RenderEngine>? Would that lead to a more expected pythonic experience?

Admittedly, hanging onto the RenderEngine after creation and messing with it is a bit of an anti-pattern. So, maybe it's not worrying about.

---

geometry/geometry_state.h line 157 at r4 (raw file):

  }
  DeepCopySharedPtr(DeepCopySharedPtr&& other) noexcept {
    std::swap(value_, other.value_);

nit: I would've expected move semantics more akin to that of shared pointer. shared_ptr would leave other with a null value. Same for move assignment. Is there an explicit reason for this counter-intuitive implementation?

---

geometry/test/geometry_state_test.cc line 4958 at r4 (raw file):

TEST_F(GeometryStateNoRendererTest, PerceptionRoleWithoutRenderer) {
  const InternalGeometry& geometry = *gs_tester_.GetGeometry(geometries_[0]);
  ASSERT_EQ(gs_tester_.RendererCount(), 0u);

nit: the u is now quite the useless appendage.

---

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

  //@{

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

BTW The overload is documented as the "non-copying variant". Is it worth being explicit that the engine gets copied in this main documentation?

[jwnimmer-tri]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)

---

geometry/geometry_state.h line 157 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I would've expected move semantics more akin to that of shared pointer. shared_ptr would leave other with a null value. Same for move assignment. Is there an explicit reason for this counter-intuitive implementation?

Derp.

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)

---

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Is it the case that if a user instantiates a RenderEngine in python and passes it to SceneGraph that the engine instance must be copied? I.e., the instance they had in hand was only the seed for what exists in SceneGraph, but is not a handle to it. That seems slightly unpythonic. Is it obvious to the python user that their instance got copied and operations on the in-hand instance will have no bearing on the in-SceneGraph instance? What if we allowed SceneGraph to take a shared_ptr<RenderEngine>? Would that lead to a more expected pythonic experience?

Admittedly, hanging onto the RenderEngine after creation and messing with it is a bit of an anti-pattern. So, maybe it's not worrying about.

I guess I'd say that anyway their engine will be copied at context-creation time, so having it retain reference equality / identity while living in SceneGraph as a kind if "default value" but not while part of the Context didn't seem like an important win. The big picture I'm aiming for "if its destination is eventually the context, then you never get to keep a pointer". Things in the context have value semantics, not identity semantics. (This stands in contract with our recent pointer and memory tweaks for System subclasses, which do not have value semantics.)

If the user wants to keep tabs on all instances of their custom engines, their __init__ and/or __deepcopy__ methods have the power to register their engines with a global pool (e.g., for debugging / introspection).

I do think it's a bug for a user to try to mutate their engine using the retained pointer after adding it to a scene graph. There might be a case for granting them const access, but mutable access into internal geometry state smells wrong, and in Python we can't really enforce constness so it seems like a bad idea to allow them to keep a pointer to the engine they gave us.

I suppose the best reason to avoid the extra copy would be for performance, but I'd hope an empty engine would be relatively cheap to copy? The argument passed to AddRenderEngine should nominally always be empty (no geometry added yet).

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I guess I'd say that anyway their engine will be copied at context-creation time, so having it retain reference equality / identity while living in SceneGraph as a kind if "default value" but not while part of the Context didn't seem like an important win. The big picture I'm aiming for "if its destination is eventually the context, then you never get to keep a pointer". Things in the context have value semantics, not identity semantics. (This stands in contract with our recent pointer and memory tweaks for System subclasses, which do not have value semantics.)

If the user wants to keep tabs on all instances of their custom engines, their __init__ and/or __deepcopy__ methods have the power to register their engines with a global pool (e.g., for debugging / introspection).

I do think it's a bug for a user to try to mutate their engine using the retained pointer after adding it to a scene graph. There might be a case for granting them const access, but mutable access into internal geometry state smells wrong, and in Python we can't really enforce constness so it seems like a bad idea to allow them to keep a pointer to the engine they gave us.

I suppose the best reason to avoid the extra copy would be for performance, but I'd hope an empty engine would be relatively cheap to copy? The argument passed to AddRenderEngine should nominally always be empty (no geometry added yet).

But anyway, you're right that this is one of the key questions to decide in this pull request, so I'm open to more discussion with you and always want to hear what Rico things. I'd rather take our time and get this right.

[SeanCurtis-TRI]

on the //geometry/... stuff.

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

But anyway, you're right that this is one of the key questions to decide in this pull request, so I'm open to more discussion with you and always want to hear what Rico things. I'd rather take our time and get this right.

In some ways, I wonder if life wouldn't have been better if the SceneGraph API had simply taken the RenderEngine factory function instead of the instance. :)

I believe your hypothesis is correct that copying an empty engine should be cheap. The only cost is the infrastructure. RenderEngineVtk recreates all of the VTK pipeline artifacts. RenderEngineGl....shares the context? Something like that. So, performance at engine registration is probably not the end of the world.

That said, changing the SceneGraph api from const RenderEngine& renderer to std::shared_ptr<RenderEngine> renderer seems like an unfettered win. It completely eliminates the copy, regardless and, generally, I think we've taught our users that all configuration (geometry, etc.) should pass through SceneGraph. So, the idea that they have a handle on an engine stored in the model would only be used in the most self-flagellating circumstances. I like the idea of not having to ask the question, "is the copy expensive", at all.

tl;dr

Assuming there are no hidden traps in upstream library resource allocation, copying empty RenderEngine should be an acceptable mechanism to ensure user's can't even try to mutate SceneGraph internals directly.

At the same time, I'm ok with simply telling them not to do that in favor of the benefits.

I'm good either way.

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)

---

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

In some ways, I wonder if life wouldn't have been better if the SceneGraph API had simply taken the RenderEngine factory function instead of the instance. :)

I believe your hypothesis is correct that copying an empty engine should be cheap. The only cost is the infrastructure. RenderEngineVtk recreates all of the VTK pipeline artifacts. RenderEngineGl....shares the context? Something like that. So, performance at engine registration is probably not the end of the world.

That said, changing the SceneGraph api from const RenderEngine& renderer to std::shared_ptr<RenderEngine> renderer seems like an unfettered win. It completely eliminates the copy, regardless and, generally, I think we've taught our users that all configuration (geometry, etc.) should pass through SceneGraph. So, the idea that they have a handle on an engine stored in the model would only be used in the most self-flagellating circumstances. I like the idea of not having to ask the question, "is the copy expensive", at all.

tl;dr

Assuming there are no hidden traps in upstream library resource allocation, copying empty RenderEngine should be an acceptable mechanism to ensure user's can't even try to mutate SceneGraph internals directly.

At the same time, I'm ok with simply telling them not to do that in favor of the benefits.

I'm good either way.

My main worry with taking a shared_ptr<RenderEngine> in the C++ API was that we'd need to write the API documentation "no really, you must stop using this engine now, please pinky swear" which is somewhat at odds with what shared_ptr<> usually denotes. Having the "stop using it" be absolutely enforced by cloning seemed safest and more natural.

I'll wait for Rico to weigh in before changing anything.

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My main worry with taking a shared_ptr<RenderEngine> in the C++ API was that we'd need to write the API documentation "no really, you must stop using this engine now, please pinky swear" which is somewhat at odds with what shared_ptr<> usually denotes. Having the "stop using it" be absolutely enforced by cloning seemed safest and more natural.

I'll wait for Rico to weigh in before changing anything.

I buy that argument. It would put both c++ and python in the same pinky-swear space. In the absence of unique pointers (or simply making sure the pointer is never accessible in the first place), copying is the only protection we have left. I'm sold.

[rpoyner-tri]

pydrake stuff.

Reviewed 3 of 10 files at r2.
Reviewable status: 3 unresolved discussions

---

bindings/pydrake/geometry/geometry_py_render.cc line 82 at r5 (raw file):

    // can post-process the return value.
    auto make_python_deepcopy = [&]() -> py::object {
      PYBIND11_OVERLOAD_INT(py::object, Base, "DoClone");

nit It's probably worth noting to the casual reader that control will fall through this macro if there is no suitable DoClone method. That wasn't obvious to me until I went and read the macro.

[rpoyner-tri]

Reviewed 1 of 10 files at r2.
Reviewable status: 3 unresolved discussions

[rpoyner-tri]

Reviewed all commit messages.
Reviewable status: 3 unresolved discussions

[jwnimmer-tri]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform)

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Needs (separate) platform reviewer assigned.

+@xuchenhan-tri for platform review per schedule, please.

---

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I buy that argument. It would put both c++ and python in the same pinky-swear space. In the absence of unique pointers (or simply making sure the pointer is never accessible in the first place), copying is the only protection we have left. I'm sold.

Done (no changes).

---

bindings/pydrake/geometry/geometry_py_render.cc line 82 at r5 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit It's probably worth noting to the casual reader that control will fall through this macro if there is no suitable DoClone method. That wasn't obvious to me until I went and read the macro.

Indeed, my comment paragraph above primed you to expect an unconditional return.

[SeanCurtis-TRI]

Reviewed all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)

[rpoyner-tri]

Reviewed 2 of 11 files at r1, 2 of 10 files at r2, 2 of 2 files at r3, 3 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)

[xuchenhan-tri]

Platform

Reviewed 2 of 11 files at r1, 5 of 10 files at r2, 2 of 2 files at r3, 3 of 3 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform),xuchenhan-tri(platform)