From 2fd667315f5d295cde5fc1e723a3c96d45c52379 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Wed, 8 Jan 2025 13:43:37 -0800 Subject: [PATCH] [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 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. --- .../pydrake/geometry/geometry_py_render.cc | 43 +++++++- .../geometry/geometry_py_scene_graph.cc | 4 +- .../test/render_engine_subclass_test.py | 101 +++++++++++++++++- bindings/pydrake/geometry/test/render_test.py | 28 ++--- geometry/geometry_state.cc | 6 +- geometry/geometry_state.h | 43 +++++++- geometry/render/render_engine.cc | 22 +++- geometry/render/render_engine.h | 26 ++++- geometry/render/test/render_engine_test.cc | 39 +++++++ geometry/scene_graph.cc | 15 +++ geometry/scene_graph.h | 24 ++++- geometry/test/geometry_state_test.cc | 7 +- geometry/test/scene_graph_test.cc | 88 +++++++++------ 13 files changed, 367 insertions(+), 79 deletions(-) diff --git a/bindings/pydrake/geometry/geometry_py_render.cc b/bindings/pydrake/geometry/geometry_py_render.cc index 70aa77a5fc0f..5af7fc79d73d 100644 --- a/bindings/pydrake/geometry/geometry_py_render.cc +++ b/bindings/pydrake/geometry/geometry_py_render.cc @@ -63,7 +63,43 @@ class PyRenderEngine : public py::wrapper { } std::unique_ptr DoClone() const override { - PYBIND11_OVERLOAD_PURE(std::unique_ptr, Base, DoClone); + throw std::logic_error( + "Python subclasses of RenderEngine do not support calling " + "Clone; the C++ code which tried to call " + "it needs to be updated to call using shared_ptr instead."); + } + + std::shared_ptr 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 whose C++ lifetime + // keeps the python object alive. + RenderEngine* cpp_engine = py::cast(py_engine); + DRAKE_DEMAND(cpp_engine != nullptr); + return std::shared_ptr( + /* 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_ cls(m, "RenderEngine"); + py::class_> cls( + m, "RenderEngine"); cls // BR .def(py::init<>(), cls_doc.ctor.doc) .def("Clone", - static_cast<::std::unique_ptr (Class::*)() const>( + static_cast (Class::*)() const>( &Class::Clone), cls_doc.Clone.doc) .def("RegisterVisual", diff --git a/bindings/pydrake/geometry/geometry_py_scene_graph.cc b/bindings/pydrake/geometry/geometry_py_scene_graph.cc index c41e1e943ea5..36df0d9e2203 100644 --- a/bindings/pydrake/geometry/geometry_py_scene_graph.cc +++ b/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>(&Class::AddRenderer), + const render::RenderEngine&>(&Class::AddRenderer), py::arg("name"), py::arg("renderer"), cls_doc.AddRenderer.doc_2args) .def("AddRenderer", overload_cast_explicit*, std::string, - std::unique_ptr>(&Class::AddRenderer), + const render::RenderEngine&>(&Class::AddRenderer), py::arg("context"), py::arg("name"), py::arg("renderer"), cls_doc.AddRenderer.doc_3args) .def("RemoveRenderer", diff --git a/bindings/pydrake/geometry/test/render_engine_subclass_test.py b/bindings/pydrake/geometry/test/render_engine_subclass_test.py index 5a4a963da647..735199a009c9 100644 --- a/bindings/pydrake/geometry/test/render_engine_subclass_test.py +++ b/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,6 +73,7 @@ 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.+"): @@ -77,6 +81,7 @@ def DoRenderLabelImage(self, camera, image_out): 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.+"): @@ -84,3 +89,91 @@ def DoRenderLabelImage(self, camera, image_out): 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) diff --git a/bindings/pydrake/geometry/test/render_test.py b/bindings/pydrake/geometry/test/render_test.py index 7a6c1d4d9c5f..d74e180b1468 100644 --- a/bindings/pydrake/geometry/test/render_test.py +++ b/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(), diff --git a/geometry/geometry_state.cc b/geometry/geometry_state.cc index 6b91692a63bc..a534d555e386 100644 --- a/geometry/geometry_state.cc +++ b/geometry/geometry_state.cc @@ -1459,13 +1459,13 @@ SignedDistancePair GeometryState::ComputeSignedDistancePairClosestPoints( template void GeometryState::AddRenderer( - std::string name, std::unique_ptr renderer) { + std::string name, std::shared_ptr 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::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( diff --git a/geometry/geometry_state.h b/geometry/geometry_state.h index 5312a67922cb..1de1bf98c227 100644 --- a/geometry/geometry_state.h +++ b/geometry/geometry_state.h @@ -131,6 +131,41 @@ class DrivenMeshData { std::unordered_map> render_meshes_; }; +// A wrapper around a shared_ptr where copying calls T::Clone() instead of +// bumping the ref_count with a new alias. +template +class DeepCopySharedPtr { + public: + DeepCopySharedPtr() = default; + explicit DeepCopySharedPtr(std::shared_ptr 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 (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 value_; +}; + } // namespace internal #endif @@ -628,7 +663,7 @@ class GeometryState { /** Implementation of SceneGraph::AddRenderer(). */ void AddRenderer(std::string name, - std::unique_ptr renderer); + std::shared_ptr renderer); /** Implementation of SceneGraph::RemoveRenderer(). */ void RemoveRenderer(const std::string& name); @@ -1067,8 +1102,10 @@ class GeometryState { // and copy it. copyable_unique_ptr> geometry_engine_; - // The collection of all registered renderers. - std::unordered_map> + // The collection of all registered renderers. When copying a GeometryState, + // we must ensure that it's a deep copy via DeepCopySharedPtr. + std::unordered_map> render_engines_; // The version for this geometry data. diff --git a/geometry/render/render_engine.cc b/geometry/render/render_engine.cc index 0b6c4da9a548..e9b31db8fea2 100644 --- a/geometry/render/render_engine.cc +++ b/geometry/render/render_engine.cc @@ -22,8 +22,17 @@ using systems::sensors::ImageRgba8U; RenderEngine::~RenderEngine() = default; -std::unique_ptr RenderEngine::Clone() const { - std::unique_ptr clone(DoClone()); +template +Result RenderEngine::Clone() const + requires std::is_same_v> || + std::is_same_v> +{ // NOLINT(whitespace/braces) + Result clone; + if constexpr (std::is_same_v>) { + 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::Clone() const { return clone; } +// Explicit template instantiations. +template std::unique_ptr RenderEngine::Clone<>() const; +template std::shared_ptr RenderEngine::Clone<>() const; + +std::shared_ptr 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, diff --git a/geometry/render/render_engine.h b/geometry/render/render_engine.h index b8c24fefec8d..a812f4bfcb5b 100644 --- a/geometry/render/render_engine.h +++ b/geometry/render/render_engine.h @@ -103,9 +103,18 @@ class RenderEngine { virtual ~RenderEngine(); - /** Clones the render engine -- making the %RenderEngine compatible with - copyable_unique_ptr. */ - std::unique_ptr Clone() const; + /** Clones the render engine. + + @tparam Result must be either `std::unique_ptr` or + `std::shared_ptr`. In C++, it defaults to unique_ptr; + in Python, it's hard-coded to shared_ptr. + + @throws std::exception if Result is unique_ptr but this particular class only + supports cloning for shared_ptr. */ + template > + Result Clone() const + requires std::is_same_v> || + std::is_same_v>; /** @name Registering geometry with the engine @@ -345,7 +354,16 @@ class RenderEngine { removed, false if it wasn't registered in the first place. */ virtual bool DoRemoveGeometry(GeometryId id) = 0; - /** The NVI-function for cloning this render engine. */ + /** The NVI-function for cloning this render engine as a shared_ptr. When not + overridden, this base class implementation will call DoClone() to construct a + unique_ptr clone and then promote that to a shared_ptr upon return. Note that + in Python this is bound as simply "DoClone" not "DoCloneShared", because the + unique_ptr flavor is nonsense in Python. */ + virtual std::shared_ptr DoCloneShared() const; + + /** The NVI-function for cloning this render engine as a unique_ptr. It must + always be overridden, but in case a subclass does not support cloning into a + unique_ptr, it may throw an exception. */ virtual std::unique_ptr DoClone() const = 0; /** The NVI-function for rendering color with a fully-specified camera. diff --git a/geometry/render/test/render_engine_test.cc b/geometry/render/test/render_engine_test.cc index 8902b0e8a3b8..1bc32cd7a057 100644 --- a/geometry/render/test/render_engine_test.cc +++ b/geometry/render/test/render_engine_test.cc @@ -497,6 +497,8 @@ class NoDoCloneEngine : public CloneableEngine { GTEST_TEST(RenderEngine, DetectDoCloneFailure) { CloneableEngine cloneable; EXPECT_NO_THROW(cloneable.Clone()); + EXPECT_NO_THROW(cloneable.Clone>()); + EXPECT_NO_THROW(cloneable.Clone>()); NoDoCloneEngine not_cloneable; DRAKE_EXPECT_THROWS_MESSAGE( @@ -504,6 +506,43 @@ GTEST_TEST(RenderEngine, DetectDoCloneFailure) { "Error in cloning RenderEngine class of type .+NoDoCloneEngine; the " "clone returns type .+CloneableEngine. .+NoDoCloneEngine::DoClone.. was " "probably not implemented"); + DRAKE_EXPECT_THROWS_MESSAGE( + not_cloneable.Clone>(), + ".*not implemented"); + DRAKE_EXPECT_THROWS_MESSAGE( + not_cloneable.Clone>(), + ".*not implemented"); +} + +class CloneableAsSharedOnlyEngine final : public RenderEngine { + public: + DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(CloneableAsSharedOnlyEngine); + CloneableAsSharedOnlyEngine() = default; + + // Only implement shared_ptr cloning. + std::unique_ptr DoClone() const final { + throw std::logic_error("SHARED ONLY"); + } + std::shared_ptr DoCloneShared() const final { + return std::make_unique(*this); + } + + // No-op fluff for the pure virtuals. + void UpdateViewpoint(const math::RigidTransformd&) final {} + bool DoRegisterVisual(GeometryId, const Shape&, const PerceptionProperties&, + const RigidTransformd&) final { + return false; + } + void DoUpdateVisualPose(GeometryId, const math::RigidTransformd&) final {} + bool DoRemoveGeometry(GeometryId) final { return false; } +}; + +GTEST_TEST(RenderEngine, CloneOnlySupportsShared) { + CloneableAsSharedOnlyEngine dut; + DRAKE_EXPECT_THROWS_MESSAGE(dut.Clone(), "SHARED ONLY"); + DRAKE_EXPECT_THROWS_MESSAGE(dut.Clone>(), + "SHARED ONLY"); + EXPECT_NO_THROW(dut.Clone>()); } // Confirms that sub-classes that don't implement the DoRender*Image API get diff --git a/geometry/scene_graph.cc b/geometry/scene_graph.cc index bdf74ecdf14e..9f2f9596c4ca 100644 --- a/geometry/scene_graph.cc +++ b/geometry/scene_graph.cc @@ -379,6 +379,21 @@ void SceneGraph::RemoveGeometry(Context* context, SourceId source_id, g_state.RemoveGeometry(source_id, geometry_id); } +template +void SceneGraph::AddRenderer(std::string name, + const render::RenderEngine& renderer) { + return hub_.mutable_model().AddRenderer( + std::move(name), renderer.Clone>()); +} + +template +void SceneGraph::AddRenderer(Context* context, std::string name, + const render::RenderEngine& renderer) const { + auto& g_state = mutable_geometry_state(context); + g_state.AddRenderer(std::move(name), + renderer.Clone>()); +} + template void SceneGraph::AddRenderer( std::string name, std::unique_ptr renderer) { diff --git a/geometry/scene_graph.h b/geometry/scene_graph.h index 8c7fa67e96cb..818c6de5fea2 100644 --- a/geometry/scene_graph.h +++ b/geometry/scene_graph.h @@ -699,8 +699,8 @@ class SceneGraph final : public systems::LeafSystem { /** @name Managing RenderEngine instances */ //@{ - /** Adds a new render engine to this %SceneGraph. The %SceneGraph owns the - render engine. The render engine's name should be referenced in the + /** Adds a new render engine to this %SceneGraph. + The render engine's name should be referenced in the @ref render::ColorRenderCamera "ColorRenderCamera" or @ref render::DepthRenderCamera "DepthRenderCamera" provided in the render queries (see QueryObject::RenderColorImage() as an example). @@ -724,14 +724,28 @@ class SceneGraph final : public systems::LeafSystem { existing geometries (see @ref scene_graph_versioning). @param name The unique name of the renderer. - @param renderer The `renderer` to add. + @param renderer The `renderer` to add. (It will be copied / cloned, which + means the lifetime of the argument need not extend past this call.) @throws std::exception if the name is not unique. */ - void AddRenderer(std::string name, - std::unique_ptr renderer); + void AddRenderer(std::string name, const render::RenderEngine& renderer); /** systems::Context-modifying variant of AddRenderer(). Rather than modifying %SceneGraph's model, it modifies the copy of the model stored in the provided context. */ + void AddRenderer(systems::Context* context, std::string name, + const render::RenderEngine& renderer) const; + + /** Non-copying variant of AddRenderer(). + The %SceneGraph takes ownership the render engine instead of copying it. + The calling code must not retain a raw pointer to the renderer. + @exclude_from_pydrake_mkdoc{Not bound in pydrake.} */ + void AddRenderer(std::string name, + std::unique_ptr renderer); + + /** Non-copying, context-modifying variant of AddRenderer(). + The %SceneGraph takes ownership the render engine instead of copying it. + The calling code must not retain a raw pointer to the renderer. + @exclude_from_pydrake_mkdoc{Not bound in pydrake.} */ void AddRenderer(systems::Context* context, std::string name, std::unique_ptr renderer) const; diff --git a/geometry/test/geometry_state_test.cc b/geometry/test/geometry_state_test.cc index 5720819cf9e3..0b3a5c4e2150 100644 --- a/geometry/test/geometry_state_test.cc +++ b/geometry/test/geometry_state_test.cc @@ -193,9 +193,8 @@ class GeometryStateTester { return *state_->geometry_engine_; } - const unordered_map>& - render_engines() const { - return state_->render_engines_; + int RendererCount() const { + return state_->RendererCount(); } const GeometryVersion& geometry_version() const { @@ -4956,7 +4955,7 @@ class GeometryStateNoRendererTest : public GeometryStateTestBase, // is no renderer. TEST_F(GeometryStateNoRendererTest, PerceptionRoleWithoutRenderer) { const InternalGeometry& geometry = *gs_tester_.GetGeometry(geometries_[0]); - ASSERT_EQ(gs_tester_.render_engines().size(), 0u); + ASSERT_EQ(gs_tester_.RendererCount(), 0); EXPECT_TRUE(geometry.has_perception_role()); EXPECT_EQ( diff --git a/geometry/test/scene_graph_test.cc b/geometry/test/scene_graph_test.cc index 060e782a3d31..7ede66219f05 100644 --- a/geometry/test/scene_graph_test.cc +++ b/geometry/test/scene_graph_test.cc @@ -611,47 +611,65 @@ TEST_F(SceneGraphTest, ModelInspector) { // functions work. It relies on GeometryState to properly unit test the // full behavior. TEST_F(SceneGraphTest, RendererInSceneGraphSmokeTest) { - // Test the renderer added to the SceneGraph. - const std::string kRendererName = "bob"; - - EXPECT_EQ(scene_graph_.RendererCount(), 0); - EXPECT_EQ(scene_graph_.RegisteredRendererNames().size(), 0u); - EXPECT_FALSE(scene_graph_.HasRenderer(kRendererName)); - - DRAKE_EXPECT_NO_THROW(scene_graph_.AddRenderer( - kRendererName, make_unique())); + for (bool add_as_unique : {true, false}) { + SCOPED_TRACE(fmt::format("add_as_unique = {}", add_as_unique)); + + // Test the renderer added to the SceneGraph. + const std::string kRendererName = "bob"; + + EXPECT_EQ(scene_graph_.RendererCount(), 0); + EXPECT_EQ(scene_graph_.RegisteredRendererNames().size(), 0u); + EXPECT_FALSE(scene_graph_.HasRenderer(kRendererName)); + + if (add_as_unique) { + DRAKE_EXPECT_NO_THROW(scene_graph_.AddRenderer( + kRendererName, make_unique())); + } else { + DRAKE_EXPECT_NO_THROW( + scene_graph_.AddRenderer(kRendererName, DummyRenderEngine())); + } - EXPECT_EQ(scene_graph_.RendererCount(), 1); - EXPECT_EQ(scene_graph_.RegisteredRendererNames()[0], kRendererName); - EXPECT_TRUE(scene_graph_.HasRenderer(kRendererName)); + EXPECT_EQ(scene_graph_.RendererCount(), 1); + EXPECT_EQ(scene_graph_.RegisteredRendererNames()[0], kRendererName); + EXPECT_TRUE(scene_graph_.HasRenderer(kRendererName)); - DRAKE_EXPECT_NO_THROW(scene_graph_.RemoveRenderer(kRendererName)); - EXPECT_EQ(scene_graph_.RendererCount(), 0); - EXPECT_FALSE(scene_graph_.HasRenderer(kRendererName)); + DRAKE_EXPECT_NO_THROW(scene_graph_.RemoveRenderer(kRendererName)); + EXPECT_EQ(scene_graph_.RendererCount(), 0); + EXPECT_FALSE(scene_graph_.HasRenderer(kRendererName)); + } } TEST_F(SceneGraphTest, RendererInContextSmokeTest) { - // Test the renderer added to the context - CreateDefaultContext(); - const std::string kRendererName = "bob"; - - EXPECT_EQ(scene_graph_.RendererCount(*context_), 0); - EXPECT_EQ(scene_graph_.RegisteredRendererNames(*context_).size(), 0u); - EXPECT_FALSE(scene_graph_.HasRenderer(*context_, kRendererName)); - - DRAKE_EXPECT_NO_THROW(scene_graph_.AddRenderer( - context_.get(), kRendererName, make_unique())); - - EXPECT_EQ(scene_graph_.RendererCount(*context_), 1); - // No renderer inside SceneGraph since the renderer is added to the context. - EXPECT_EQ(scene_graph_.RendererCount(), 0); - EXPECT_EQ(scene_graph_.RegisteredRendererNames(*context_)[0], kRendererName); - EXPECT_TRUE(scene_graph_.HasRenderer(*context_, kRendererName)); + for (bool add_as_unique : {true, false}) { + SCOPED_TRACE(fmt::format("add_as_unique = {}", add_as_unique)); + // Test the renderer added to the context + CreateDefaultContext(); + const std::string kRendererName = "bob"; + + EXPECT_EQ(scene_graph_.RendererCount(*context_), 0); + EXPECT_EQ(scene_graph_.RegisteredRendererNames(*context_).size(), 0u); + EXPECT_FALSE(scene_graph_.HasRenderer(*context_, kRendererName)); + + if (add_as_unique) { + DRAKE_EXPECT_NO_THROW(scene_graph_.AddRenderer( + context_.get(), kRendererName, make_unique())); + } else { + DRAKE_EXPECT_NO_THROW(scene_graph_.AddRenderer( + context_.get(), kRendererName, DummyRenderEngine())); + } - DRAKE_EXPECT_NO_THROW( - scene_graph_.RemoveRenderer(context_.get(), kRendererName)); - EXPECT_EQ(scene_graph_.RendererCount(*context_), 0); - EXPECT_FALSE(scene_graph_.HasRenderer(*context_, kRendererName)); + EXPECT_EQ(scene_graph_.RendererCount(*context_), 1); + // No renderer inside SceneGraph since the renderer is added to the context. + EXPECT_EQ(scene_graph_.RendererCount(), 0); + EXPECT_EQ(scene_graph_.RegisteredRendererNames(*context_)[0], + kRendererName); + EXPECT_TRUE(scene_graph_.HasRenderer(*context_, kRendererName)); + + DRAKE_EXPECT_NO_THROW( + scene_graph_.RemoveRenderer(context_.get(), kRendererName)); + EXPECT_EQ(scene_graph_.RendererCount(*context_), 0); + EXPECT_FALSE(scene_graph_.HasRenderer(*context_, kRendererName)); + } } // Query the type name of a render engine. This logic is unique to SceneGraph