[systems] Add DiagramBuilder overloads for shared_ptr

AddSystem and AddNamedSystem functions are overloaded to allow passing
the system via mutable shared_ptr (in addition to mutable unique_ptr).

Architecturally, the diagram still maintains exclusive ownership of
its children: each child still has a back-pointer to the Diagram (via
the SystemParentServiceInterface) so it's not possible to actually
"share" a subsystem across multiple diagrams. However, unique_ptr is
overly restrictive: it not only denotes exclusive ownership (which is
fine), but also demands that resource cleanup is `delete MyClass`
(which is insufficient for our Python bindings). We must use a smart
pointer type that allows customizing cleanup. We could imagine using
unique_ptr whose Deleter was a std::function, but it's simpler to just
use shared_ptr.

Relatedly, we also adjust the Diagram subclasses PidControlledSystem
and RgbdSensorDiscrete to accept the more general shared_ptr (instead
of unique_ptr) for their child systems and to use ref_cycle for their
pydrake bindings.

bindings/pydrake/pydrake_pybind.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <memory>
 #include <utility>
 
 // Here we include a lot of the pybind11 API, to ensure that all code in pydrake
@@ -145,6 +146,16 @@ inline void ExecuteExtraPythonCode(py::module m, bool use_subdir = false) {
     }                                                                     \
   }
 
+/// Given a raw pointer, returns a shared_ptr wrapper around it that doesn't own
+/// anything -- it's managed object is null, so there is no reference counting.
+/// Calling get() on the result will return `raw`.
+template <typename T>
+auto make_unowned_shared_ptr_from_raw(T* raw) {
+  return std::shared_ptr<T>(
+      /* managed object = */ std::shared_ptr<void>{},
+      /* stored pointer = */ raw);
+}
+
 }  // namespace pydrake
 }  // namespace drake
 

bindings/pydrake/systems/BUILD.bazel

@@ -121,6 +121,7 @@ drake_pybind_library(
         "//bindings/pydrake:documentation_pybind",
         "//bindings/pydrake:symbolic_types_pybind",
         "//bindings/pydrake/common:deprecation_pybind",
+        "//bindings/pydrake/common:ref_cycle_pybind",
         "//bindings/pydrake/common:wrap_pybind",
     ],
     cc_srcs = ["controllers_py.cc"],
@@ -171,6 +172,7 @@ drake_pybind_library(
         "//bindings/pydrake/common:cpp_template_pybind",
         "//bindings/pydrake/common:default_scalars_pybind",
         "//bindings/pydrake/common:eigen_pybind",
+        "//bindings/pydrake/common:ref_cycle_pybind",
         "//bindings/pydrake/common:serialize_pybind",
         "//bindings/pydrake/common:type_pack",
         "//bindings/pydrake/common:value_pybind",

bindings/pydrake/systems/controllers_py.cc

@@ -1,4 +1,5 @@
 #include "drake/bindings/pydrake/common/deprecation_pybind.h"
+#include "drake/bindings/pydrake/common/ref_cycle_pybind.h"
 #include "drake/bindings/pydrake/common/wrap_pybind.h"
 #include "drake/bindings/pydrake/documentation_pybind.h"
 #include "drake/bindings/pydrake/pydrake_pybind.h"
@@ -180,40 +181,73 @@ PYBIND11_MODULE(controllers, m) {
   }
 
   {
-    using Class = PidControlledSystem<double>;
+    using T = double;
+    using Class = PidControlledSystem<T>;
     constexpr auto& cls_doc = doc.PidControlledSystem;
-    py::class_<Class, Diagram<double>>(m, "PidControlledSystem", cls_doc.doc)
-        .def(py::init<std::unique_ptr<System<double>>, double, double, double,
-                 int, int>(),
+    py::class_<Class, Diagram<T>>(m, "PidControlledSystem", cls_doc.doc)
+        .def(py::init(
+                 [](System<T>& plant, double Kp, double Ki, double Kd,
+                     int state_output_port_index, int plant_input_port_index) {
+                   // The C++ constructor doesn't offer a bare-pointer overload,
+                   // only shared_ptr. Because object lifetime is already
+                   // handled by the ref_cycle annotation below (as required for
+                   // all subclasses of Diagram), we can pass the `plant` as an
+                   // unowned shared_ptr.
+                   return std::make_unique<Class>(
+                       make_unowned_shared_ptr_from_raw(&plant), Kp, Ki, Kd,
+                       state_output_port_index, plant_input_port_index);
+                 }),
             py::arg("plant"), py::arg("kp"), py::arg("ki"), py::arg("kd"),
             py::arg("state_output_port_index") = 0,
             py::arg("plant_input_port_index") = 0,
-            // Keep alive, ownership: `plant` keeps `self` alive.
-            py::keep_alive<2, 1>(), cls_doc.ctor.doc_6args_double_gains)
-        .def(py::init<std::unique_ptr<System<double>>, const VectorX<double>&,
-                 const VectorX<double>&, const VectorX<double>&, int, int>(),
+            // `self` and `plant` form a cycle as part of the Diagram.
+            internal::ref_cycle<1, 2>(), cls_doc.ctor.doc_6args_double_gains)
+        .def(py::init(
+                 [](System<T>& plant, const Eigen::VectorXd& Kp,
+                     const Eigen::VectorXd& Ki, const Eigen::VectorXd& Kd,
+                     int state_output_port_index, int plant_input_port_index) {
+                   // See comment in py::init() above for how &plant is handled.
+                   return std::make_unique<Class>(
+                       make_unowned_shared_ptr_from_raw(&plant), Kp, Ki, Kd,
+                       state_output_port_index, plant_input_port_index);
+                 }),
             py::arg("plant"), py::arg("kp"), py::arg("ki"), py::arg("kd"),
             py::arg("state_output_port_index") = 0,
             py::arg("plant_input_port_index") = 0,
-            // Keep alive, ownership: `plant` keeps `self` alive.
-            py::keep_alive<2, 1>(), cls_doc.ctor.doc_6args_vector_gains)
-        .def(py::init<std::unique_ptr<System<double>>, const MatrixX<double>&,
-                 double, double, double, int, int>(),
+            // `self` and `plant` form a cycle as part of the Diagram.
+            internal::ref_cycle<1, 2>(), cls_doc.ctor.doc_6args_vector_gains)
+        .def(py::init([](System<T>& plant,
+                          const MatrixX<double>& feedback_selector, double Kp,
+                          double Ki, double Kd, int state_output_port_index,
+                          int plant_input_port_index) {
+          // See comment in py::init() above for how &plant is handled.
+          return std::make_unique<Class>(
+              make_unowned_shared_ptr_from_raw(&plant), feedback_selector, Kp,
+              Ki, Kd, state_output_port_index, plant_input_port_index);
+        }),
             py::arg("plant"), py::arg("feedback_selector"), py::arg("kp"),
             py::arg("ki"), py::arg("kd"),
             py::arg("state_output_port_index") = 0,
             py::arg("plant_input_port_index") = 0,
-            // Keep alive, ownership: `plant` keeps `self` alive.
-            py::keep_alive<2, 1>(), cls_doc.ctor.doc_7args_double_gains)
-        .def(py::init<std::unique_ptr<System<double>>, const MatrixX<double>&,
-                 const VectorX<double>&, const VectorX<double>&,
-                 const VectorX<double>&, int, int>(),
+            // `self` and `plant` form a cycle as part of the Diagram.
+            internal::ref_cycle<1, 2>(), cls_doc.ctor.doc_7args_double_gains)
+        .def(py::init(
+                 [](System<T>& plant, const MatrixX<double>& feedback_selector,
+                     const Eigen::VectorXd& Kp, const Eigen::VectorXd& Ki,
+                     const Eigen::VectorXd& Kd, int state_output_port_index,
+                     int plant_input_port_index) {
+                   // See comment in py::init() above for how &plant is handled.
+                   return std::make_unique<Class>(
+                       make_unowned_shared_ptr_from_raw(&plant),
+                       feedback_selector, Kp, Ki, Kd, state_output_port_index,
+                       plant_input_port_index);
+                 }),
             py::arg("plant"), py::arg("feedback_selector"), py::arg("kp"),
             py::arg("ki"), py::arg("kd"),
             py::arg("state_output_port_index") = 0,
             py::arg("plant_input_port_index") = 0,
-            // Keep alive, ownership: `plant` keeps `self` alive.
-            py::keep_alive<2, 1>(), cls_doc.ctor.doc_7args_vector_gains)
+            // `self` and `plant` form a cycle as part of the Diagram.
+            internal::ref_cycle<1, 2>(), cls_doc.ctor.doc_7args_vector_gains)
         .def("get_control_input_port", &Class::get_control_input_port,
             py_rvp::reference_internal, cls_doc.get_control_input_port.doc)
         .def("get_state_input_port", &Class::get_state_input_port,

bindings/pydrake/systems/estimators_py.cc

@@ -63,10 +63,7 @@ PYBIND11_MODULE(estimators, m) {
           return SteadyStateKalmanFilter(
               // The lifetime of `system` is managed by the keep_alive below,
               // not the C++ shared_ptr.
-              std::shared_ptr<const LinearSystem<double>>(
-                  /* managed object = */ std::shared_ptr<void>{},
-                  /* stored pointer = */ &system),
-              W, V);
+              make_unowned_shared_ptr_from_raw(&system), W, V);
         },
         py::arg("system"), py::arg("W"), py::arg("V"),
         // Keep alive, reference: `result` keeps `system` alive.
@@ -80,10 +77,7 @@ PYBIND11_MODULE(estimators, m) {
           return SteadyStateKalmanFilter(
               // The lifetime of `system` is managed by the keep_alive below,
               // not the C++ shared_ptr.
-              std::shared_ptr<const System<double>>(
-                  /* managed object = */ std::shared_ptr<void>{},
-                  /* stored pointer = */ &system),
-              context, W, V);
+              make_unowned_shared_ptr_from_raw(&system), context, W, V);
         },
         py::arg("system"), py::arg("context"), py::arg("W"), py::arg("V"),
         // Keep alive, reference: `result` keeps `system` alive.

bindings/pydrake/systems/framework_py_semantics.cc

@@ -598,26 +598,31 @@ void DoDefineFrameworkDiagramBuilder(py::module m) {
       .def(py::init<>(), doc.DiagramBuilder.ctor.doc)
       .def(
           "AddSystem",
-          [](DiagramBuilder<T>* self, unique_ptr<System<T>> system) {
+          [](DiagramBuilder<T>* self, System<T>& 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<T>::stash(self);
-            return self->AddSystem(std::move(system));
+            // The C++ method doesn't offer a bare-pointer overload, only
+            // shared_ptr. Because object lifetime is already handled by the
+            // ref_cycle annotation below, we can pass the `system` as an
+            // unowned shared_ptr.
+            return self->AddSystem(make_unowned_shared_ptr_from_raw(&system));
           },
           py::arg("system"), internal::ref_cycle<1, 2>(),
           doc.DiagramBuilder.AddSystem.doc)
       .def(
           "AddNamedSystem",
-          [](DiagramBuilder<T>* self, std::string& name,
-              unique_ptr<System<T>> system) {
+          [](DiagramBuilder<T>* self, std::string& name, System<T>& 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<T>::stash(self);
-            return self->AddNamedSystem(name, std::move(system));
+            // Ditto with "AddSystem" above for how we handle the `&system`.
+            return self->AddNamedSystem(
+                name, make_unowned_shared_ptr_from_raw(&system));
           },
           py::arg("name"), py::arg("system"), internal::ref_cycle<1, 3>(),
           doc.DiagramBuilder.AddNamedSystem.doc)

bindings/pydrake/systems/sensors_py_rgbd.cc

@@ -1,3 +1,4 @@
+#include "drake/bindings/pydrake/common/ref_cycle_pybind.h"
 #include "drake/bindings/pydrake/documentation_pybind.h"
 #include "drake/bindings/pydrake/systems/sensors_py.h"
 #include "drake/systems/sensors/camera_info.h"
@@ -140,12 +141,22 @@ void DefineSensorsRgbd(py::module m) {
   py::class_<RgbdSensorDiscrete, Diagram<double>> rgbd_camera_discrete(
       m, "RgbdSensorDiscrete", doc.RgbdSensorDiscrete.doc);
   rgbd_camera_discrete
-      .def(py::init<std::unique_ptr<RgbdSensor>, double, bool>(),
+      .def(py::init(
+               [](RgbdSensor& sensor, double period, bool render_label_image) {
+                 // The C++ constructor doesn't offer a bare-pointer overload,
+                 // only shared_ptr. Because object lifetime is already handled
+                 // by the ref_cycle annotation below (as required for all
+                 // subclasses of Diagram), we can pass the `sensor` as an
+                 // unowned shared_ptr.
+                 return std::make_unique<RgbdSensorDiscrete>(
+                     make_unowned_shared_ptr_from_raw(&sensor), period,
+                     render_label_image);
+               }),
           py::arg("sensor"),
           py::arg("period") = double{RgbdSensorDiscrete::kDefaultPeriod},
           py::arg("render_label_image") = true,
-          // Keep alive, ownership: `sensor` keeps `self` alive.
-          py::keep_alive<2, 1>(), doc.RgbdSensorDiscrete.ctor.doc)
+          // `self` and `sensor` form a cycle as part of the Diagram.
+          internal::ref_cycle<1, 2>(), doc.RgbdSensorDiscrete.ctor.doc)
       // N.B. Since `camera` is already connected, we do not need additional
       // `keep_alive`s.
       .def("sensor", &RgbdSensorDiscrete::sensor, py_rvp::reference_internal,

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()

systems/controllers/pid_controlled_system.cc

@@ -9,7 +9,7 @@ namespace systems {
 namespace controllers {
 
 template <typename T>
-PidControlledSystem<T>::PidControlledSystem(std::unique_ptr<System<T>> plant,
+PidControlledSystem<T>::PidControlledSystem(std::shared_ptr<System<T>> plant,
                                             double Kp, double Ki, double Kd,
                                             int state_output_port_index,
                                             int plant_input_port_index)
@@ -26,7 +26,7 @@ PidControlledSystem<T>::PidControlledSystem(std::unique_ptr<System<T>> plant,
 }
 
 template <typename T>
-PidControlledSystem<T>::PidControlledSystem(std::unique_ptr<System<T>> plant,
+PidControlledSystem<T>::PidControlledSystem(std::shared_ptr<System<T>> plant,
                                             const Eigen::VectorXd& Kp,
                                             const Eigen::VectorXd& Ki,
                                             const Eigen::VectorXd& Kd,
@@ -39,7 +39,7 @@ PidControlledSystem<T>::PidControlledSystem(std::unique_ptr<System<T>> plant,
 
 template <typename T>
 PidControlledSystem<T>::PidControlledSystem(
-    std::unique_ptr<System<T>> plant, const MatrixX<double>& feedback_selector,
+    std::shared_ptr<System<T>> plant, const MatrixX<double>& feedback_selector,
     double Kp, double Ki, double Kd, int state_output_port_index,
     int plant_input_port_index)
     : state_output_port_index_(state_output_port_index),
@@ -53,7 +53,7 @@ PidControlledSystem<T>::PidControlledSystem(
 
 template <typename T>
 PidControlledSystem<T>::PidControlledSystem(
-    std::unique_ptr<System<T>> plant, const MatrixX<double>& feedback_selector,
+    std::shared_ptr<System<T>> plant, const MatrixX<double>& feedback_selector,
     const Eigen::VectorXd& Kp, const Eigen::VectorXd& Ki,
     const Eigen::VectorXd& Kd, int state_output_port_index,
     int plant_input_port_index)
@@ -64,7 +64,7 @@ PidControlledSystem<T>::PidControlledSystem(
 
 template <typename T>
 void PidControlledSystem<T>::Initialize(
-    std::unique_ptr<System<T>> plant, const MatrixX<double>& feedback_selector,
+    std::shared_ptr<System<T>> plant, const MatrixX<double>& feedback_selector,
     const Eigen::VectorXd& Kp, const Eigen::VectorXd& Ki,
     const Eigen::VectorXd& Kd) {
   DRAKE_DEMAND(plant != nullptr);

systems/controllers/pid_controlled_system.h

@@ -83,8 +83,12 @@ class PidControlledSystem : public Diagram<T> {
   /// @param[in] plant_input_port_index identifies the input port on the plant
   /// that takes in the input (computed as the output of the PID controller).
   ///
+  /// @warning a System (i.e., `plant`) may only be added to at most one Diagram
+  /// (i.e., this `PidControlledSystem`) so should not be re-used outside of the
+  /// `PidControlledSystem`.
+  ///
   /// @pydrake_mkdoc_identifier{6args_double_gains}
-  PidControlledSystem(std::unique_ptr<System<T>> plant, double Kp, double Ki,
+  PidControlledSystem(std::shared_ptr<System<T>> plant, double Kp, double Ki,
                       double Kd, int state_output_port_index = 0,
                       int plant_input_port_index = 0);
 
@@ -100,12 +104,16 @@ class PidControlledSystem : public Diagram<T> {
   /// @param[in] plant_input_port_index identifies the input port on the plant
   /// that takes in the input (computed as the output of the PID controller).
   ///
+  /// @warning a System (i.e., `plant`) may only be added to at most one Diagram
+  /// (i.e., this `PidControlledSystem`) so should not be re-used outside of the
+  /// `PidControlledSystem`.
+  ///
   /// @pydrake_mkdoc_identifier{6args_vector_gains}
-  PidControlledSystem(std::unique_ptr<System<T>> plant,
+  PidControlledSystem(std::shared_ptr<System<T>> plant,
                       const Eigen::VectorXd& Kp, const Eigen::VectorXd& Ki,
                       const Eigen::VectorXd& Kd,
                       int state_output_port_index = 0,
-                      int plant_output_port_index = 0);
+                      int plant_input_port_index = 0);
 
   /// A constructor where the gains are scalar values and some of the plant's
   /// output is part of the feedback signal as specified by
@@ -123,8 +131,12 @@ class PidControlledSystem : public Diagram<T> {
   /// @param[in] plant_input_port_index identifies the input port on the plant
   /// that takes in the input (computed as the output of the PID controller).
   ///
+  /// @warning a System (i.e., `plant`) may only be added to at most one Diagram
+  /// (i.e., this `PidControlledSystem`) so should not be re-used outside of the
+  /// `PidControlledSystem`.
+  ///
   /// @pydrake_mkdoc_identifier{7args_double_gains}
-  PidControlledSystem(std::unique_ptr<System<T>> plant,
+  PidControlledSystem(std::shared_ptr<System<T>> plant,
                       const MatrixX<double>& feedback_selector, double Kp,
                       double Ki, double Kd, int state_output_port_index = 0,
                       int plant_input_port_index = 0);
@@ -145,8 +157,12 @@ class PidControlledSystem : public Diagram<T> {
   /// @param[in] plant_input_port_index identifies the input port on the plant
   /// that takes in the input (computed as the output of the PID controller).
   ///
+  /// @warning a System (i.e., `plant`) may only be added to at most one Diagram
+  /// (i.e., this `PidControlledSystem`) so should not be re-used outside of the
+  /// `PidControlledSystem`.
+  ///
   /// @pydrake_mkdoc_identifier{7args_vector_gains}
-  PidControlledSystem(std::unique_ptr<System<T>> plant,
+  PidControlledSystem(std::shared_ptr<System<T>> plant,
                       const MatrixX<double>& feedback_selector,
                       const Eigen::VectorXd& Kp, const Eigen::VectorXd& Ki,
                       const Eigen::VectorXd& Kd,
@@ -229,7 +245,7 @@ class PidControlledSystem : public Diagram<T> {
   // A helper function for the constructors. This is necessary to avoid seg
   // faults caused by simultaneously moving the plant and calling methods on
   // the plant when one constructor delegates to another constructor.
-  void Initialize(std::unique_ptr<System<T>> plant,
+  void Initialize(std::shared_ptr<System<T>> plant,
                   const MatrixX<double>& feedback_selector,
                   const Eigen::VectorXd& Kp, const Eigen::VectorXd& Ki,
                   const Eigen::VectorXd& Kd);