From c8d03fbd34ed9fb35d55b1a6016f7f3c34ab9112 Mon Sep 17 00:00:00 2001 From: Jeremy Nimmer Date: Sun, 22 Dec 2024 17:36:54 -0800 Subject: [PATCH] [systems] Add DiagramBuilder overloads for shared_ptr --- .../pydrake/systems/framework_py_semantics.cc | 18 +++- .../pydrake/systems/test/lifetime_test.py | 15 +-- systems/framework/diagram.h | 4 +- systems/framework/diagram_builder.cc | 21 +++- systems/framework/diagram_builder.h | 97 +++++++++++-------- systems/framework/system_base.cc | 15 +++ systems/framework/system_base.h | 11 +-- .../framework/test/diagram_builder_test.cc | 16 +++ 8 files changed, 135 insertions(+), 62 deletions(-) diff --git a/bindings/pydrake/systems/framework_py_semantics.cc b/bindings/pydrake/systems/framework_py_semantics.cc index 5f7f85f843f9..62237fa7713c 100644 --- a/bindings/pydrake/systems/framework_py_semantics.cc +++ b/bindings/pydrake/systems/framework_py_semantics.cc @@ -598,26 +598,34 @@ void DoDefineFrameworkDiagramBuilder(py::module m) { .def(py::init<>(), doc.DiagramBuilder.ctor.doc) .def( "AddSystem", - [](DiagramBuilder* self, unique_ptr> system) { + [](DiagramBuilder* self, System& system) { // Using BuilderLifeSupport::stash makes the builder // temporarily immortal (uncollectible self cycle). This will be // resolved by the Build() step. See BuilderLifeSupport for // rationale. BuilderLifeSupport::stash(self); - return self->AddSystem(std::move(system)); + // The ref_cycle is responsible for object lifetime, so we'll give + // the DiagramBuilder an unowned pointer. + return self->AddSystem(std::shared_ptr>( + /* managed object = */ std::shared_ptr{}, + /* stored pointer = */ &system)); }, py::arg("system"), internal::ref_cycle<1, 2>(), doc.DiagramBuilder.AddSystem.doc) .def( "AddNamedSystem", - [](DiagramBuilder* self, std::string& name, - unique_ptr> system) { + [](DiagramBuilder* self, std::string& name, System& system) { // Using BuilderLifeSupport::stash makes the builder // temporarily immortal (uncollectible self cycle). This will be // resolved by the Build() step. See BuilderLifeSupport for // rationale. BuilderLifeSupport::stash(self); - return self->AddNamedSystem(name, std::move(system)); + // The ref_cycle is responsible for object lifetime, so we'll give + // the DiagramBuilder an unowned pointer. + return self->AddNamedSystem( + name, std::shared_ptr>( + /* managed object = */ std::shared_ptr{}, + /* stored pointer = */ &system)); }, py::arg("name"), py::arg("system"), internal::ref_cycle<1, 3>(), doc.DiagramBuilder.AddNamedSystem.doc) diff --git a/bindings/pydrake/systems/test/lifetime_test.py b/bindings/pydrake/systems/test/lifetime_test.py index a574153bdf1b..3581b94e3925 100644 --- a/bindings/pydrake/systems/test/lifetime_test.py +++ b/bindings/pydrake/systems/test/lifetime_test.py @@ -71,15 +71,18 @@ def test_ownership_diagram(self): def test_ownership_multiple_containers(self): info = Info() system = DeleteListenerSystem(info.record_deletion) + + # Add the system to a built diagram. builder_1 = DiagramBuilder() - builder_2 = DiagramBuilder() builder_1.AddSystem(system) - # This is tested in our fork of `pybind11`, but echoed here for when - # we decide to switch to use `shared_ptr`. - with self.assertRaises(RuntimeError): - # This should throw an error from `pybind11`, since two containers - # are trying to own a unique_ptr-held object. + diagram_1 = builder_1.Build() + + # Add it again to another diagram. We don't care if the Add fails or + # the Build fails, so long as one of them does. + builder_2 = DiagramBuilder() + with self.assertRaisesRegex(Exception, "already.*different Diagram"): builder_2.AddSystem(system) + builder_2.Build() def test_ownership_simulator(self): info = Info() diff --git a/systems/framework/diagram.h b/systems/framework/diagram.h index c802805d3358..def546dd262e 100644 --- a/systems/framework/diagram.h +++ b/systems/framework/diagram.h @@ -51,7 +51,7 @@ class OwnedSystems { decltype(auto) end() const { return vec_.end(); } decltype(auto) operator[](size_t i) const { return vec_[i]; } decltype(auto) operator[](size_t i) { return vec_[i]; } - void push_back(std::unique_ptr>&& sys) { + void push_back(std::shared_ptr>&& sys) { vec_.push_back(std::move(sys)); } void pop_back() { @@ -59,7 +59,7 @@ class OwnedSystems { } private: - std::vector>> vec_; + std::vector>> vec_; }; // External life support data for the diagram. The data will be moved to the diff --git a/systems/framework/diagram_builder.cc b/systems/framework/diagram_builder.cc index 52c5172a974b..754fb84d71ab 100644 --- a/systems/framework/diagram_builder.cc +++ b/systems/framework/diagram_builder.cc @@ -45,7 +45,7 @@ void DiagramBuilder::RemoveSystem(const System& system) { const size_t system_index = std::distance( registered_systems_.begin(), std::find_if(registered_systems_.begin(), registered_systems_.end(), - [&system](const std::unique_ptr>& item) { + [&system](const std::shared_ptr>& item) { return item.get() == &system; })); DRAKE_DEMAND(system_index < registered_systems_.size()); @@ -443,6 +443,25 @@ void DiagramBuilder::ThrowIfAlreadyBuilt() const { } } +template +void DiagramBuilder::AddSystemImpl(std::shared_ptr>&& system) { + DRAKE_THROW_UNLESS(system != nullptr); + ThrowIfAlreadyBuilt(); + if (system->get_name().empty()) { + system->set_name(system->GetMemoryObjectName()); + } + systems_.insert(system.get()); + registered_systems_.push_back(std::move(system)); +} + +template +void DiagramBuilder::AddNamedSystemImpl( + const std::string& name, std::shared_ptr>&& system) { + DRAKE_THROW_UNLESS(system != nullptr); + system->set_name(name); + this->AddSystemImpl(std::move(system)); +} + template void DiagramBuilder::ThrowIfInputAlreadyWired( const InputPortLocator& id) const { diff --git a/systems/framework/diagram_builder.h b/systems/framework/diagram_builder.h index ba9cb0390614..132348ff6043 100644 --- a/systems/framework/diagram_builder.h +++ b/systems/framework/diagram_builder.h @@ -77,29 +77,36 @@ class DiagramBuilder { DiagramBuilder(); virtual ~DiagramBuilder(); - /// Takes ownership of @p system and adds it to the builder. Returns a bare + /// Takes ownership of `system` and adds it to the builder. Returns a bare /// pointer to the System, which will remain valid for the lifetime of the /// Diagram built by this builder. /// /// If the system's name is unset, sets it to System::GetMemoryObjectName() /// as a default in order to have unique names within the diagram. /// - /// @code - /// DiagramBuilder builder; - /// auto foo = builder.AddSystem(std::make_unique>()); - /// @endcode + /// @warning a System may only be added to at most one DiagramBuilder. + /// Multiple Diagram instances cannot share the same System. + template + S* AddSystem(std::shared_ptr system) { + S* result = system.get(); + this->AddSystemImpl(std::move(system)); + return result; + } + + /// Takes ownership of `system` and adds it to the builder. Returns a bare + /// pointer to the System, which will remain valid for the lifetime of the + /// Diagram built by this builder. /// - /// @tparam S The type of system to add. + /// If the system's name is unset, sets it to System::GetMemoryObjectName() + /// as a default in order to have unique names within the diagram. + /// + /// @exclude_from_pydrake_mkdoc{Not bound in pydrake -- pydrake only uses the + /// shared_ptr overload.} template S* AddSystem(std::unique_ptr system) { - ThrowIfAlreadyBuilt(); - if (system->get_name().empty()) { - system->set_name(system->GetMemoryObjectName()); - } - S* raw_sys_ptr = system.get(); - systems_.insert(raw_sys_ptr); - registered_systems_.push_back(std::move(system)); - return raw_sys_ptr; + S* result = system.get(); + this->AddSystemImpl(std::move(system)); + return result; } /// Constructs a new system with the given @p args, and adds it to the @@ -119,17 +126,16 @@ class DiagramBuilder { /// auto foo = builder.template AddSystem>("name", 3.14); /// @endcode /// - /// You may prefer the `unique_ptr` variant instead. - /// - /// /// @tparam S The type of System to construct. Must subclass System. /// /// @exclude_from_pydrake_mkdoc{Not bound in pydrake -- emplacement while /// specifying doesn't make sense for that language.} template S* AddSystem(Args&&... args) { - ThrowIfAlreadyBuilt(); - return AddSystem(std::make_unique(std::forward(args)...)); + auto system = std::make_shared(std::forward(args)...); + S* result = system.get(); + this->AddSystemImpl(std::move(system)); + return result; } /// Constructs a new system with the given @p args, and adds it to the @@ -150,8 +156,6 @@ class DiagramBuilder { /// auto foo = builder.template AddSystem("name", 3.14); /// @endcode /// - /// You may prefer the `unique_ptr` variant instead. - /// /// @tparam S A template for the type of System to construct. The template /// will be specialized on the scalar type T of this builder. /// @@ -159,26 +163,33 @@ class DiagramBuilder { /// specifying doesn't make sense for that language.} template class S, typename... Args> S* AddSystem(Args&&... args) { - ThrowIfAlreadyBuilt(); - return AddSystem(std::make_unique>(std::forward(args)...)); + auto system = std::make_shared>(std::forward(args)...); + S* result = system.get(); + this->AddSystemImpl(std::move(system)); + return result; } - /// Takes ownership of @p system, applies @p name to it, and adds it to the + /// Takes ownership of `system`, set its name to `name`, and adds it to the /// builder. Returns a bare pointer to the System, which will remain valid /// for the lifetime of the Diagram built by this builder. /// - /// @code - /// DiagramBuilder builder; - /// auto bar = builder.AddNamedSystem("bar", std::make_unique>()); - /// @endcode - /// - /// @tparam S The type of system to add. - /// @post The system's name is @p name. + /// @warning a System may only be added to at most one DiagramBuilder. + /// Multiple Diagram instances cannot share the same System. + template + S* AddNamedSystem(const std::string& name, std::shared_ptr system) { + S* result = system.get(); + this->AddNamedSystemImpl(name, std::move(system)); + return result; + } + + /// Takes ownership of `system`, set its name to `name`, and adds it to the + /// builder. Returns a bare pointer to the System, which will remain valid + /// for the lifetime of the Diagram built by this builder. template S* AddNamedSystem(const std::string& name, std::unique_ptr system) { - ThrowIfAlreadyBuilt(); - system->set_name(name); - return AddSystem(std::move(system)); + S* result = system.get(); + this->AddNamedSystemImpl(name, std::move(system)); + return result; } /// Constructs a new system with the given @p args, applies @p name to it, @@ -207,9 +218,10 @@ class DiagramBuilder { /// specifying doesn't make sense for that language.} template S* AddNamedSystem(const std::string& name, Args&&... args) { - ThrowIfAlreadyBuilt(); - return AddNamedSystem( - name, std::make_unique(std::forward(args)...)); + auto system = std::make_shared(std::forward(args)...); + S* result = system.get(); + this->AddNamedSystemImpl(name, std::move(system)); + return result; } /// Constructs a new system with the given @p args, applies @p name to it, @@ -240,9 +252,10 @@ class DiagramBuilder { /// specifying doesn't make sense for that language.} template class S, typename... Args> S* AddNamedSystem(const std::string& name, Args&&... args) { - ThrowIfAlreadyBuilt(); - return AddNamedSystem( - name, std::make_unique>(std::forward(args)...)); + auto system = std::make_shared>(std::forward(args)...); + S* result = system.get(); + this->AddNamedSystemImpl(name, std::move(system)); + return result; } /// Removes the given system from this builder and disconnects any connections @@ -490,6 +503,10 @@ class DiagramBuilder { void ThrowIfAlreadyBuilt() const; + void AddSystemImpl(std::shared_ptr>&& system); + void AddNamedSystemImpl(const std::string& name, + std::shared_ptr>&& system); + // Throws if the given input port (belonging to a child subsystem) has // already been connected to an output port, or exported to be an input // port of the whole diagram. diff --git a/systems/framework/system_base.cc b/systems/framework/system_base.cc index e4f19d76df78..0b359f01d159 100644 --- a/systems/framework/system_base.cc +++ b/systems/framework/system_base.cc @@ -425,6 +425,21 @@ void SystemBase::CreateSourceTrackers(ContextBase* context_ptr) const { } } +void SystemBase::set_parent_service( + SystemBase* child, + const internal::SystemParentServiceInterface* parent_service) { + DRAKE_DEMAND(child != nullptr); + DRAKE_DEMAND(parent_service != nullptr); + if (child->parent_service_ != nullptr) { + throw std::logic_error(fmt::format( + "Cannot build subsystem '{}' into Diagram '{}' because it has already " + "been built into a different Diagram '{}'", + child->GetSystemName(), parent_service->GetParentPathname(), + child->parent_service_->GetParentPathname())); + } + child->parent_service_ = parent_service; +} + // The only way for a system to evaluate its own input port is if that // port is fixed. In that case the port's value is in the corresponding // subcontext and we can just return it. Otherwise, the port obtains its value diff --git a/systems/framework/system_base.h b/systems/framework/system_base.h index daeae9569864..e154a75a0458 100644 --- a/systems/framework/system_base.h +++ b/systems/framework/system_base.h @@ -1098,19 +1098,14 @@ class SystemBase : public internal::SystemMessageInterface { bool IsObviouslyNotInputDependent(DependencyTicket dependency_ticket) const; /** (Internal use only) Declares that `parent_service` is the service - interface of the Diagram that owns this subsystem. Aborts if the parent - service has already been set to something else. */ + interface of the Diagram that owns this subsystem. Throws if the parent + service has already been set. */ // Use static method so Diagram can invoke this on behalf of a child. // Output argument is listed first because it is serving as the 'this' // pointer here. static void set_parent_service( SystemBase* child, - const internal::SystemParentServiceInterface* parent_service) { - DRAKE_DEMAND(child != nullptr && parent_service != nullptr); - DRAKE_DEMAND(child->parent_service_ == nullptr || - child->parent_service_ == parent_service); - child->parent_service_ = parent_service; - } + const internal::SystemParentServiceInterface* parent_service); /** (Internal use only) Given a `port_index`, returns a function to be called when validating Context::FixInputPort requests. The function should attempt diff --git a/systems/framework/test/diagram_builder_test.cc b/systems/framework/test/diagram_builder_test.cc index be286a166c35..02d2b44a6047 100644 --- a/systems/framework/test/diagram_builder_test.cc +++ b/systems/framework/test/diagram_builder_test.cc @@ -132,6 +132,22 @@ GTEST_TEST(DiagramBuilderTest, AlreadyBuilt) { ".*DiagramBuilder may no longer be used.*"); } +// Integration test of the Digram <=> System <=> SystemBase interaction when the +// same System is added to multiple diagrams. +GTEST_TEST(DiagramBuilderTest, AddSystemToMultipleDiagrams) { + auto adder = std::make_shared>(1 /* inputs */, 1 /* size */); + + DiagramBuilder builder_1; + builder_1.AddSystem(adder); + std::unique_ptr> diagram_1; + EXPECT_NO_THROW(diagram_1 = builder_1.Build()); + + DiagramBuilder builder_2; + builder_2.AddSystem(adder); + DRAKE_EXPECT_THROWS_MESSAGE(builder_2.Build(), + ".*already.*different Diagram.*"); +} + // A special class to distinguish between cycles and algebraic loops. The system // has one input and two outputs. One output simply "echoes" the input (direct // feedthrough). The other output merely outputs a const value. That means, the