[geometry] Add RenderEngine support for shared_ptr (#22353)

Because we allow implementations of RenderEngine as Python subclasses,
we cannot assume that the deleter associated with a call to Clone is
`delete get()`, i.e., the return type of unique_ptr<RenderEngine> is
incompatible with Python implementations.

Instead, we add a template argument to Clone() to select between
unique_ptr and shared_ptr (retaining the default of unique_ptr), add
another NVI hook for the shared_ptr flavor, switch our bindings to use
shared_ptr, and change all of our internal C++ call sites (just one --
in GeometryState) to opt-in to shared_ptr so that Python subclasses
can be safely manipulated from C++.

We also now allow Python implementations to implement the canonical
pythonic spelling of the "__deepcopy__" method, instead of the weird
DoClone spelling.

Engines subclasses implemented in C++ do not need to change; their
override of NVI DoClone for unique_ptr will keep working indefinitely.

This commit contains one breaking change: If downstream C++ code was
calling RenderEngine::Clone and the runtime type of the object was a
downstream Python subclass of RenderEngine, then the call will now
throw an exception. The C++ call must be updated to opt-in to the
shared_ptr template argument.

bindings/pydrake/geometry/geometry_py_render.cc

@@ -63,7 +63,43 @@ class PyRenderEngine : public py::wrapper<RenderEngine> {
   }
 
   std::unique_ptr<RenderEngine> DoClone() const override {
-    PYBIND11_OVERLOAD_PURE(std::unique_ptr<RenderEngine>, Base, DoClone);
+    throw std::logic_error(
+        "Python subclasses of RenderEngine do not support calling "
+        "Clone<std::unique_ptr>; the C++ code which tried to call "
+        "it needs to be updated to call using shared_ptr instead.");
+  }
+
+  std::shared_ptr<RenderEngine> DoCloneShared() const override {
+    py::gil_scoped_acquire guard;
+    // RenderEngine subclasses in Python must implement cloning by defining
+    // either a __deepcopy__ (preferred) or DoClone (legacy) method. We'll try
+    // DoClone first so it has priority, but if it doesn't exist we'll fall back
+    // to __deepcopy__ and just let the "no such method deepcopy" error message
+    // propagate if both were missing. Because the PYBIND11_OVERLOAD_INT macro
+    // embeds a conditional `return ...;` statement, we must wrap it in lambda
+    // so that we can post-process the return value in case it does return.
+    auto make_python_deepcopy = [&]() -> py::object {
+      PYBIND11_OVERLOAD_INT(py::object, Base, "DoClone");
+      auto deepcopy = py::module_::import("copy").attr("deepcopy");
+      py::object copied = deepcopy(this);
+      if (copied.is_none()) {
+        throw pybind11::type_error(fmt::format(
+            "{}.__deepcopy__ returned None", NiceTypeName::Get(*this)));
+      }
+      return copied;
+    };
+    py::object py_engine = make_python_deepcopy();
+    // Convert the py_engine to a shared_ptr<RenderEngine> whose C++ lifetime
+    // keeps the python object alive.
+    RenderEngine* cpp_engine = py::cast<RenderEngine*>(py_engine);
+    DRAKE_DEMAND(cpp_engine != nullptr);
+    return std::shared_ptr<RenderEngine>(
+        /* stored pointer = */ cpp_engine,
+        /* deleter = */ [captured_py_engine = std::move(py_engine)](
+                            void*) mutable {
+          py::gil_scoped_acquire deleter_guard;
+          captured_py_engine = py::none();
+        });
   }
 
   void DoRenderColorImage(ColorRenderCamera const& camera,
@@ -246,11 +282,12 @@ void DoScalarIndependentDefinitions(py::module m) {
   {
     using Class = RenderEngine;
     const auto& cls_doc = doc.RenderEngine;
-    py::class_<Class, PyRenderEngine> cls(m, "RenderEngine");
+    py::class_<Class, PyRenderEngine, std::shared_ptr<Class>> cls(
+        m, "RenderEngine");
     cls  // BR
         .def(py::init<>(), cls_doc.ctor.doc)
         .def("Clone",
-            static_cast<::std::unique_ptr<Class> (Class::*)() const>(
+            static_cast<std::shared_ptr<Class> (Class::*)() const>(
                 &Class::Clone),
             cls_doc.Clone.doc)
         .def("RegisterVisual",

bindings/pydrake/geometry/geometry_py_scene_graph.cc

@@ -298,11 +298,11 @@ void DefineSceneGraph(py::module m, T) {
             cls_doc.collision_filter_manager.doc_0args)
         .def("AddRenderer",
             overload_cast_explicit<void, std::string,
-                std::unique_ptr<render::RenderEngine>>(&Class::AddRenderer),
+                const render::RenderEngine&>(&Class::AddRenderer),
             py::arg("name"), py::arg("renderer"), cls_doc.AddRenderer.doc_2args)
         .def("AddRenderer",
             overload_cast_explicit<void, systems::Context<T>*, std::string,
-                std::unique_ptr<render::RenderEngine>>(&Class::AddRenderer),
+                const render::RenderEngine&>(&Class::AddRenderer),
             py::arg("context"), py::arg("name"), py::arg("renderer"),
             cls_doc.AddRenderer.doc_3args)
         .def("RemoveRenderer",

bindings/pydrake/geometry/test/render_engine_subclass_test.py

@@ -4,10 +4,13 @@
 
 import pydrake.geometry as mut
 
+import gc
 from math import pi
 import unittest
+import weakref
 
-from pydrake.math import RigidTransform, RigidTransform_
+from pydrake.math import RigidTransform
+from pydrake.systems.framework import DiagramBuilder
 from pydrake.systems.sensors import (
     CameraInfo,
     ImageRgba8U,
@@ -36,8 +39,8 @@ def DoUpdateVisualPose(self, id, X_WG):
             def DoRemoveGeometry(self, id):
                 pass
 
-            def DoClone(self):
-                pass
+            def __deepcopy__(self, memo):
+                return type(self)()
 
         class ColorOnlyEngine(MinimalEngine):
             """Rendering Depth and Label images should throw"""
@@ -54,7 +57,7 @@ class LabelOnlyEngine(MinimalEngine):
             def DoRenderLabelImage(self, camera, image_out):
                 pass
 
-        identity = RigidTransform_[float]()
+        identity = RigidTransform()
         intrinsics = CameraInfo(10, 10, pi / 4)
         core = mut.RenderCameraCore("n/a", intrinsics,
                                     mut.ClippingRange(0.1, 10), identity)
@@ -70,17 +73,107 @@ def DoRenderLabelImage(self, camera, image_out):
             color_only.RenderDepthImage(depth_cam, depth_image)
         with self.assertRaisesRegex(RuntimeError, ".+pure virtual function.+"):
             color_only.RenderLabelImage(color_cam, label_image)
+        self.assertIsInstance(color_only.Clone(), ColorOnlyEngine)
 
         depth_only = DepthOnlyEngine()
         with self.assertRaisesRegex(RuntimeError, ".+pure virtual function.+"):
             depth_only.RenderColorImage(color_cam, color_image)
         depth_only.RenderDepthImage(depth_cam, depth_image)
         with self.assertRaisesRegex(RuntimeError, ".+pure virtual function.+"):
             depth_only.RenderLabelImage(color_cam, label_image)
+        self.assertIsInstance(depth_only.Clone(), DepthOnlyEngine)
 
         label_only = LabelOnlyEngine()
         with self.assertRaisesRegex(RuntimeError, ".+pure virtual function.+"):
             label_only.RenderColorImage(color_cam, color_image)
         with self.assertRaisesRegex(RuntimeError, ".+pure virtual function.+"):
             label_only.RenderDepthImage(depth_cam, depth_image)
         label_only.RenderLabelImage(color_cam, label_image)
+        self.assertIsInstance(label_only.Clone(), LabelOnlyEngine)
+
+    def test_legacy_DoClone(self):
+        """Sanity checks that DoClone (without __deepcopy__) is sufficient."""
+
+        class CloneableEngine(mut.RenderEngine):
+            def DoClone(self):
+                return CloneableEngine()
+
+        dut = CloneableEngine()
+        clone = dut.Clone()
+        self.assertIsInstance(clone, CloneableEngine)
+        self.assertIsNot(clone, dut)
+
+    def test_lifecycle(self):
+        """Tests lifecycle, keep_alive, ownership, etc."""
+
+        num_engines = 0
+        num_renders = 0
+
+        class MyEngine(mut.RenderEngine):
+            def __init__(self):
+                super().__init__()
+                nonlocal num_engines
+                num_engines = num_engines + 1
+
+            def __del__(self):
+                nonlocal num_engines
+                num_engines = num_engines - 1
+
+            def UpdateViewpoint(self, X_WC):
+                pass
+
+            def DoRenderColorImage(self, camera, image_out):
+                nonlocal num_renders
+                num_renders = num_renders + 1
+
+            def __deepcopy__(self, memo):
+                return type(self)()
+
+        # Wrap a SceneGraph in a Diagram.
+        builder = DiagramBuilder()
+        scene_graph = builder.AddSystem(mut.SceneGraph())
+        builder.ExportOutput(scene_graph.get_query_output_port(), name="query")
+        diagram = builder.Build()
+        del builder
+        gc.collect()
+        world_frame = scene_graph.world_frame_id()
+
+        # Add a render engine. It will be *deep copied* into scene_graph, so
+        # the original engine will be GC'd.
+        self.assertEqual(num_engines, 0)
+        engine = MyEngine()
+        spy = weakref.finalize(engine, lambda: None)
+        scene_graph.AddRenderer("name", engine)
+        del engine
+        gc.collect()
+        self.assertFalse(spy.alive)
+        self.assertEqual(num_engines, 1)
+        del scene_graph
+        del spy
+        gc.collect()
+
+        # Check that the cloned MyEngine instance can still be called.
+        diagram_context = diagram.CreateDefaultContext()
+        self.assertGreater(num_engines, 1)
+        query = diagram.GetOutputPort("query").Eval(diagram_context)
+        self.assertEqual(num_renders, 0)
+        query.RenderColorImage(
+            camera=mut.ColorRenderCamera(
+                mut.RenderCameraCore(
+                    renderer_name="name",
+                    intrinsics=CameraInfo(640, 480, 0.5),
+                    clipping=mut.ClippingRange(0.1, 10.0),
+                    X_BS=RigidTransform(),
+                ),
+            ),
+            parent_frame=world_frame,
+            X_PC=RigidTransform(),
+        )
+        self.assertEqual(num_renders, 1)
+
+        # Release everything and ensure we get back to zero engines.
+        del diagram
+        del diagram_context
+        del query
+        gc.collect()
+        self.assertEqual(num_engines, 0)

bindings/pydrake/geometry/test/render_test.py

@@ -265,21 +265,22 @@ def DoRemoveGeometry(self, id):
                 DummyRenderEngine.latest_instance = self
                 self.registered_geometries.remove(id)
 
-            def DoClone(self):
+            def __deepcopy__(self, memo):
                 DummyRenderEngine.latest_instance = self
                 new = DummyRenderEngine()
-                new.force_accept = copy.copy(self.force_accept)
-                new.registered_geometries = copy.copy(
-                    self.registered_geometries)
-                new.updated_ids = copy.copy(self.updated_ids)
-                new.include_group_name = copy.copy(self.include_group_name)
-                new.X_WC = copy.copy(self.X_WC)
-                new.color_count = copy.copy(self.color_count)
-                new.depth_count = copy.copy(self.depth_count)
-                new.label_count = copy.copy(self.label_count)
-                new.color_camera = copy.copy(self.color_camera)
-                new.depth_camera = copy.copy(self.depth_camera)
-                new.label_camera = copy.copy(self.label_camera)
+                new.force_accept = copy.deepcopy(self.force_accept, memo=memo)
+                new.registered_geometries = copy.deepcopy(
+                    self.registered_geometries, memo=memo)
+                new.updated_ids = copy.deepcopy(self.updated_ids, memo=memo)
+                new.include_group_name = copy.deepcopy(
+                    self.include_group_name, memo=memo)
+                new.X_WC = copy.deepcopy(self.X_WC, memo=memo)
+                new.color_count = copy.deepcopy(self.color_count, memo=memo)
+                new.depth_count = copy.deepcopy(self.depth_count, memo=memo)
+                new.label_count = copy.deepcopy(self.label_count, memo=memo)
+                new.color_camera = copy.deepcopy(self.color_camera, memo=memo)
+                new.depth_camera = copy.deepcopy(self.depth_camera, memo=memo)
+                new.label_camera = copy.deepcopy(self.label_camera, memo=memo)
                 return new
 
             def DoRenderColorImage(self, camera, color_image_out):
@@ -305,7 +306,6 @@ def DoRenderLabelImage(self, camera, label_image_out):
         renderer_name = "renderer"
         builder = DiagramBuilder()
         scene_graph = builder.AddSystem(mut.SceneGraph())
-        # N.B. This passes ownership.
         scene_graph.AddRenderer(renderer_name, engine)
         sensor = builder.AddSystem(RgbdSensor(
             parent_id=scene_graph.world_frame_id(),

geometry/geometry_state.cc

@@ -1459,13 +1459,13 @@ SignedDistancePair<T> GeometryState<T>::ComputeSignedDistancePairClosestPoints(
 
 template <typename T>
 void GeometryState<T>::AddRenderer(
-    std::string name, std::unique_ptr<render::RenderEngine> renderer) {
+    std::string name, std::shared_ptr<render::RenderEngine> renderer) {
   if (render_engines_.contains(name)) {
     throw std::logic_error(fmt::format(
         "AddRenderer(): A renderer with the name '{}' already exists", name));
   }
   render::RenderEngine* render_engine = renderer.get();
-  render_engines_[name] = std::move(renderer);
+  render_engines_.emplace(name, std::move(renderer));
   bool accepted = false;
   for (auto& id_geo_pair : geometries_) {
     InternalGeometry& geometry = id_geo_pair.second;
@@ -2165,7 +2165,7 @@ const render::RenderEngine& GeometryState<T>::GetRenderEngineOrThrow(
     const std::string& renderer_name) const {
   auto iter = render_engines_.find(renderer_name);
   if (iter != render_engines_.end()) {
-    return *iter->second;
+    return *iter->second.get();
   }
 
   throw std::logic_error(

geometry/geometry_state.h

@@ -131,6 +131,41 @@ class DrivenMeshData {
   std::unordered_map<GeometryId, std::vector<RenderMesh>> render_meshes_;
 };
 
+// A wrapper around a shared_ptr<T> where copying calls T::Clone() instead of
+// bumping the ref_count with a new alias.
+template <typename T>
+class DeepCopySharedPtr {
+ public:
+  DeepCopySharedPtr() = default;
+  explicit DeepCopySharedPtr(std::shared_ptr<T> value)
+      : value_(std::move(value)) {}
+  DeepCopySharedPtr(const DeepCopySharedPtr& other) {
+    if (other.value_ != nullptr) {
+      const T& other_value = *other.value_;
+      // Use a static_cast<> to obtain a function pointer for Clone, in case it
+      // is templated on the return type.
+      auto Clone = static_cast<std::shared_ptr<T> (T::*)() const>(&T::Clone);
+      value_ = (other_value.*Clone)();
+    }
+  }
+  DeepCopySharedPtr& operator=(const DeepCopySharedPtr& other) {
+    DeepCopySharedPtr other_copy(other);
+    *this = std::move(other_copy);
+    return *this;
+  }
+  DeepCopySharedPtr(DeepCopySharedPtr&& other) noexcept
+      : value_(std::move(other.value_)) {}
+  DeepCopySharedPtr& operator=(DeepCopySharedPtr&& other) noexcept {
+    value_ = std::move(other.value_);
+    return *this;
+  }
+  const T* get() const { return value_.get(); }
+  T* get_mutable() const { return value_.get(); }
+
+ private:
+  std::shared_ptr<T> value_;
+};
+
 }  // namespace internal
 #endif
 
@@ -628,7 +663,7 @@ class GeometryState {
 
   /** Implementation of SceneGraph::AddRenderer().  */
   void AddRenderer(std::string name,
-                   std::unique_ptr<render::RenderEngine> renderer);
+                   std::shared_ptr<render::RenderEngine> renderer);
 
   /** Implementation of SceneGraph::RemoveRenderer(). */
   void RemoveRenderer(const std::string& name);
@@ -1067,8 +1102,10 @@ class GeometryState {
   // and copy it.
   copyable_unique_ptr<internal::ProximityEngine<T>> geometry_engine_;
 
-  // The collection of all registered renderers.
-  std::unordered_map<std::string, copyable_unique_ptr<render::RenderEngine>>
+  // The collection of all registered renderers. When copying a GeometryState,
+  // we must ensure that it's a deep copy via DeepCopySharedPtr.
+  std::unordered_map<std::string,
+                     internal::DeepCopySharedPtr<render::RenderEngine>>
       render_engines_;
 
   // The version for this geometry data.

geometry/render/render_engine.cc

@@ -22,8 +22,17 @@ using systems::sensors::ImageRgba8U;
 
 RenderEngine::~RenderEngine() = default;
 
-std::unique_ptr<RenderEngine> RenderEngine::Clone() const {
-  std::unique_ptr<RenderEngine> clone(DoClone());
+template <class Result>
+Result RenderEngine::Clone() const
+  requires std::is_same_v<Result, std::unique_ptr<RenderEngine>> ||
+           std::is_same_v<Result, std::shared_ptr<RenderEngine>>
+{  // NOLINT(whitespace/braces)
+  Result clone;
+  if constexpr (std::is_same_v<Result, std::unique_ptr<RenderEngine>>) {
+    clone = DoClone();
+  } else {
+    clone = DoCloneShared();
+  }
   // Make sure that derived classes have actually overridden DoClone().
   // Particularly important for derivations of derivations.
   // Note: clang considers typeid(*clone) to be an expression with side effects.
@@ -40,6 +49,15 @@ std::unique_ptr<RenderEngine> RenderEngine::Clone() const {
   return clone;
 }
 
+// Explicit template instantiations.
+template std::unique_ptr<RenderEngine> RenderEngine::Clone<>() const;
+template std::shared_ptr<RenderEngine> RenderEngine::Clone<>() const;
+
+std::shared_ptr<RenderEngine> RenderEngine::DoCloneShared() const {
+  // When not overriden, we simply delegate to the unique_ptr flavor.
+  return this->DoClone();
+}
+
 bool RenderEngine::RegisterVisual(GeometryId id,
                                   const drake::geometry::Shape& shape,
                                   const PerceptionProperties& properties,