From 4c04f4866faee0d96502f3eb1641cf35dec10969 Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Sat, 9 May 2020 21:44:02 -0700 Subject: [PATCH 1/3] Update signal to remove a connection when disconnected --- dart/common/Signal.cpp | 17 +----- dart/common/Signal.hpp | 18 +++++- dart/common/detail/ConnectionBody.cpp | 18 ------ dart/common/detail/ConnectionBody.hpp | 61 +++++++++++-------- dart/common/detail/Signal.hpp | 88 +++++++++------------------ unittests/unit/test_Signal.cpp | 1 + 6 files changed, 87 insertions(+), 116 deletions(-) diff --git a/dart/common/Signal.cpp b/dart/common/Signal.cpp index a78791733d0f4..8e7d7319d6ecf 100644 --- a/dart/common/Signal.cpp +++ b/dart/common/Signal.cpp @@ -94,25 +94,14 @@ Connection::~Connection() //============================================================================== bool Connection::isConnected() const { - std::shared_ptr connectionBody( - mWeakConnectionBody.lock()); - - if (nullptr == connectionBody) - return false; - - return connectionBody->isConnected(); + return !mWeakConnectionBody.expired(); } //============================================================================== void Connection::disconnect() const { - std::shared_ptr connectionBody( - mWeakConnectionBody.lock()); - - if (nullptr == connectionBody) - return; - - connectionBody->disconnect(); + if (auto connectionBody = mWeakConnectionBody.lock()) + connectionBody->disconnect(); } //============================================================================== diff --git a/dart/common/Signal.hpp b/dart/common/Signal.hpp index b150111e419df..0a27922fe5598 100644 --- a/dart/common/Signal.hpp +++ b/dart/common/Signal.hpp @@ -37,6 +37,7 @@ #include #include +#include "dart/common/Deprecated.hpp" #include "dart/common/detail/ConnectionBody.hpp" namespace dart { @@ -69,6 +70,7 @@ class Connection /// Disconnect the connection void disconnect() const; + // TODO(JS): Make this non-const in the next major release template class Combiner> friend class Signal; @@ -115,7 +117,7 @@ class Signal<_Res(_ArgTypes...), Combiner> using SlotType = std::function; using SignalType = Signal<_Res(_ArgTypes...), Combiner>; - using ConnectionBodyType = signal::detail::ConnectionBody; + using ConnectionBodyType = signal::detail::ConnectionBody; using ConnectionSetType = std::set< std::shared_ptr, std::owner_less>>; @@ -135,11 +137,17 @@ class Signal<_Res(_ArgTypes...), Combiner> /// Disconnect given connection void disconnect(const Connection& _connection) const; + /// Disconnect a connection + void disconnect(const std::shared_ptr& connectionBody); + /// Disconnect all the connections void disconnectAll(); /// Cleanup all the disconnected connections + DART_DEPRECATED(6.10) void cleanupConnections(); + // This explicit connection cleaning is no longer necessary because now a + // connection gets removed when it's disconnected. /// Get the number of connections std::size_t getNumConnections() const; @@ -165,7 +173,7 @@ class Signal using SlotType = std::function; using SignalType = Signal; - using ConnectionBodyType = signal::detail::ConnectionBody; + using ConnectionBodyType = signal::detail::ConnectionBody; using ConnectionSetType = std::set< std::shared_ptr, std::owner_less>>; @@ -185,11 +193,17 @@ class Signal /// Disconnect given connection void disconnect(const Connection& _connection) const; + /// Disconnect given connection + void disconnect(const std::shared_ptr& connectionBody); + /// Disconnect all the connections void disconnectAll(); /// Cleanup all the disconnected connections + DART_DEPRECATED(6.10) void cleanupConnections(); + // This explicit connection cleaning is no longer necessary because now a + // connection gets removed when it's disconnected. /// Get the number of connections std::size_t getNumConnections() const; diff --git a/dart/common/detail/ConnectionBody.cpp b/dart/common/detail/ConnectionBody.cpp index e60b20b662472..f888c3533a024 100644 --- a/dart/common/detail/ConnectionBody.cpp +++ b/dart/common/detail/ConnectionBody.cpp @@ -38,30 +38,12 @@ namespace common { namespace signal { namespace detail { -//============================================================================== -ConnectionBodyBase::ConnectionBodyBase() : mIsConnected(true) -{ - // Do nothing -} - //============================================================================== ConnectionBodyBase::~ConnectionBodyBase() { // Do nothing } -//============================================================================== -void ConnectionBodyBase::disconnect() -{ - mIsConnected = false; -} - -//============================================================================== -bool ConnectionBodyBase::isConnected() const -{ - return mIsConnected; -} - } // namespace detail } // namespace signal diff --git a/dart/common/detail/ConnectionBody.hpp b/dart/common/detail/ConnectionBody.hpp index 2e5ee0c18959e..8d723b8a918e6 100644 --- a/dart/common/detail/ConnectionBody.hpp +++ b/dart/common/detail/ConnectionBody.hpp @@ -46,70 +46,83 @@ class ConnectionBodyBase { public: /// Constructor - ConnectionBodyBase(); + ConnectionBodyBase() = default; /// Destructor virtual ~ConnectionBodyBase(); /// Disconnect - void disconnect(); - - /// Get true if this connection body is connected to the signal - bool isConnected() const; - -protected: - /// Connection flag - bool mIsConnected; + virtual void disconnect() = 0; }; /// class ConnectionBody -template -class ConnectionBody : public ConnectionBodyBase +template +class ConnectionBody final + : public ConnectionBodyBase, + public std::enable_shared_from_this> { public: /// Constructor given slot - ConnectionBody(const SlotType& _slot); + ConnectionBody(SignalType& signal, const SlotType& _slot); /// Move constructor given slot - ConnectionBody(SlotType&& _slot); + ConnectionBody(SignalType& signal, SlotType&& _slot); /// Destructor virtual ~ConnectionBody(); + /// Disconnect + void disconnect() override; + /// Get slot - const SlotType& getSlot(); + const SlotType& getSlot() const; private: + /// Signal of this connection + SignalType& mSignal; + // Holding the reference of the Signal is safe because the lifetime of this + // ConnectionBody is always shorter than the Signal since the Signal has the + // ownership of this ConnectionBody. + /// Slot SlotType mSlot; }; //============================================================================== -template -ConnectionBody::ConnectionBody(const SlotType& _slot) - : ConnectionBodyBase(), mSlot(_slot) +template +ConnectionBody::ConnectionBody( + SignalType& signal, const SlotType& _slot) + : ConnectionBodyBase(), mSignal(signal), mSlot(_slot) { // Do nothing } //============================================================================== -template -ConnectionBody::ConnectionBody(SlotType&& _slot) - : ConnectionBodyBase(), mSlot(std::forward(_slot)) +template +ConnectionBody::ConnectionBody( + SignalType& signal, SlotType&& _slot) + : ConnectionBodyBase(), mSignal(signal), mSlot(std::forward(_slot)) { // Do nothing } //============================================================================== -template -ConnectionBody::~ConnectionBody() +template +ConnectionBody::~ConnectionBody() { // Do nothing } //============================================================================== -template -const SlotType& ConnectionBody::getSlot() +template +void ConnectionBody::disconnect() +{ + mSignal.disconnect(this->shared_from_this()); +} + +//============================================================================== +template +const SlotType& ConnectionBody::getSlot() const { return mSlot; } diff --git a/dart/common/detail/Signal.hpp b/dart/common/detail/Signal.hpp index 4b94dfbe5ad8b..0efa4126e5d3e 100644 --- a/dart/common/detail/Signal.hpp +++ b/dart/common/detail/Signal.hpp @@ -56,7 +56,7 @@ Signal<_Res(_ArgTypes...), Combiner>::~Signal() template class Combiner> Connection Signal<_Res(_ArgTypes...), Combiner>::connect(const SlotType& _slot) { - auto newConnectionBody = std::make_shared(_slot); + auto newConnectionBody = std::make_shared(*this, _slot); mConnectionBodies.insert(newConnectionBody); return Connection(std::move(newConnectionBody)); @@ -66,8 +66,8 @@ Connection Signal<_Res(_ArgTypes...), Combiner>::connect(const SlotType& _slot) template class Combiner> Connection Signal<_Res(_ArgTypes...), Combiner>::connect(SlotType&& _slot) { - auto newConnectionBody - = std::make_shared(std::forward(_slot)); + auto newConnectionBody = std::make_shared( + *this, std::forward(_slot)); mConnectionBodies.insert(newConnectionBody); return Connection(std::move(newConnectionBody)); @@ -81,6 +81,14 @@ void Signal<_Res(_ArgTypes...), Combiner>::disconnect( _connection.disconnect(); } +//============================================================================== +template class Combiner> +void Signal<_Res(_ArgTypes...), Combiner>::disconnect( + const std::shared_ptr& connectionBody) +{ + mConnectionBodies.erase(connectionBody); +} + //============================================================================== template class Combiner> void Signal<_Res(_ArgTypes...), Combiner>::disconnectAll() @@ -92,28 +100,14 @@ void Signal<_Res(_ArgTypes...), Combiner>::disconnectAll() template class Combiner> void Signal<_Res(_ArgTypes...), Combiner>::cleanupConnections() { - // Counts all the connected conection bodies - for (const auto& connectionBody : mConnectionBodies) - { - if (!connectionBody->isConnected()) - mConnectionBodies.erase(connectionBody); - } + // Do nothing } //============================================================================== template class Combiner> std::size_t Signal<_Res(_ArgTypes...), Combiner>::getNumConnections() const { - std::size_t numConnections = 0; - - // Counts all the connected conection bodies - for (const auto& connectionBody : mConnectionBodies) - { - if (connectionBody->isConnected()) - ++numConnections; - } - - return numConnections; + return mConnectionBodies.size(); } //============================================================================== @@ -124,17 +118,9 @@ _Res Signal<_Res(_ArgTypes...), Combiner>::raise(ArgTypes&&... _args) std::vector res(mConnectionBodies.size()); auto resIt = res.begin(); - for (auto itr = mConnectionBodies.begin(); itr != mConnectionBodies.end();) + for (const auto& connectionBody : mConnectionBodies) { - if ((*itr)->isConnected()) - { - *(resIt++) = (*itr)->getSlot()(std::forward(_args)...); - ++itr; - } - else - { - mConnectionBodies.erase(itr++); - } + *(resIt++) = connectionBody->getSlot()(std::forward(_args)...); } return Combiner::process(res.begin(), resIt); @@ -166,7 +152,7 @@ Signal::~Signal() template Connection Signal::connect(const SlotType& _slot) { - auto newConnectionBody = std::make_shared(_slot); + auto newConnectionBody = std::make_shared(*this, _slot); mConnectionBodies.insert(newConnectionBody); return Connection(std::move(newConnectionBody)); @@ -176,8 +162,8 @@ Connection Signal::connect(const SlotType& _slot) template Connection Signal::connect(SlotType&& _slot) { - auto newConnectionBody - = std::make_shared(std::forward(_slot)); + auto newConnectionBody = std::make_shared( + *this, std::forward(_slot)); mConnectionBodies.insert(newConnectionBody); return Connection(std::move(newConnectionBody)); @@ -190,6 +176,14 @@ void Signal::disconnect(const Connection& _connection) const _connection.disconnect(); } +//============================================================================== +template +void Signal::disconnect( + const std::shared_ptr& connectionBody) +{ + mConnectionBodies.erase(connectionBody); +} + //============================================================================== template void Signal::disconnectAll() @@ -201,28 +195,14 @@ void Signal::disconnectAll() template void Signal::cleanupConnections() { - // Counts all the connected conection bodies - for (const auto& connectionBody : mConnectionBodies) - { - if (!connectionBody->isConnected()) - mConnectionBodies.erase(connectionBody); - } + // Do nothing } //============================================================================== template std::size_t Signal::getNumConnections() const { - std::size_t numConnections = 0; - - // Counts all the connected conection bodies - for (const auto& connectionBody : mConnectionBodies) - { - if (connectionBody->isConnected()) - ++numConnections; - } - - return numConnections; + return mConnectionBodies.size(); } //============================================================================== @@ -230,17 +210,9 @@ template template void Signal::raise(ArgTypes&&... _args) { - for (auto itr = mConnectionBodies.begin(); itr != mConnectionBodies.end();) + for (const auto& connectionBody : mConnectionBodies) { - if ((*itr)->isConnected()) - { - (*itr)->getSlot()(std::forward(_args)...); - ++itr; - } - else - { - mConnectionBodies.erase(itr++); - } + connectionBody->getSlot()(std::forward(_args)...); } } diff --git a/unittests/unit/test_Signal.cpp b/unittests/unit/test_Signal.cpp index c0845856d9858..aeda3f07fd051 100644 --- a/unittests/unit/test_Signal.cpp +++ b/unittests/unit/test_Signal.cpp @@ -190,6 +190,7 @@ TEST(Signal, ConnectionLifeTime) // scopedConnection disconnected connection2 when it was destroyed. } EXPECT_FALSE(connection2.isConnected()); + EXPECT_EQ(signal->getNumConnections(), 1); delete signal; From c8078011193c6a0647faa9066f66333257f6b048 Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Sat, 9 May 2020 22:03:47 -0700 Subject: [PATCH 2/3] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8c2e3711a36f..0cf1c1346e4c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * Common * Removed use of boost::filesystem in public APIs: [#1417](https://github.com/dartsim/dart/pull/1417) + * Changed Signal to remove connection when they're being disconnected: [#1462](https://github.com/dartsim/dart/pull/1462) * Collision From 21822b800fd171ab4950368ed933ded9da803ce7 Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Sun, 10 May 2020 12:02:11 -0700 Subject: [PATCH 3/3] Remove redundant template parameter --- dart/common/Signal.hpp | 4 +-- dart/common/detail/ConnectionBody.hpp | 28 +++++++++++---------- python/dartpy/collision/CollisionResult.cpp | 6 ++--- python/dartpy/dynamics/Shape.cpp | 3 +-- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/dart/common/Signal.hpp b/dart/common/Signal.hpp index 0a27922fe5598..9680a96ce2fd3 100644 --- a/dart/common/Signal.hpp +++ b/dart/common/Signal.hpp @@ -117,7 +117,7 @@ class Signal<_Res(_ArgTypes...), Combiner> using SlotType = std::function; using SignalType = Signal<_Res(_ArgTypes...), Combiner>; - using ConnectionBodyType = signal::detail::ConnectionBody; + using ConnectionBodyType = signal::detail::ConnectionBody; using ConnectionSetType = std::set< std::shared_ptr, std::owner_less>>; @@ -173,7 +173,7 @@ class Signal using SlotType = std::function; using SignalType = Signal; - using ConnectionBodyType = signal::detail::ConnectionBody; + using ConnectionBodyType = signal::detail::ConnectionBody; using ConnectionSetType = std::set< std::shared_ptr, std::owner_less>>; diff --git a/dart/common/detail/ConnectionBody.hpp b/dart/common/detail/ConnectionBody.hpp index 8d723b8a918e6..db47180c4fff4 100644 --- a/dart/common/detail/ConnectionBody.hpp +++ b/dart/common/detail/ConnectionBody.hpp @@ -56,12 +56,14 @@ class ConnectionBodyBase }; /// class ConnectionBody -template +template class ConnectionBody final : public ConnectionBodyBase, - public std::enable_shared_from_this> + public std::enable_shared_from_this> { public: + using SlotType = typename SignalType::SlotType; + /// Constructor given slot ConnectionBody(SignalType& signal, const SlotType& _slot); @@ -89,8 +91,8 @@ class ConnectionBody final }; //============================================================================== -template -ConnectionBody::ConnectionBody( +template +ConnectionBody::ConnectionBody( SignalType& signal, const SlotType& _slot) : ConnectionBodyBase(), mSignal(signal), mSlot(_slot) { @@ -98,31 +100,31 @@ ConnectionBody::ConnectionBody( } //============================================================================== -template -ConnectionBody::ConnectionBody( - SignalType& signal, SlotType&& _slot) +template +ConnectionBody::ConnectionBody(SignalType& signal, SlotType&& _slot) : ConnectionBodyBase(), mSignal(signal), mSlot(std::forward(_slot)) { // Do nothing } //============================================================================== -template -ConnectionBody::~ConnectionBody() +template +ConnectionBody::~ConnectionBody() { // Do nothing } //============================================================================== -template -void ConnectionBody::disconnect() +template +void ConnectionBody::disconnect() { mSignal.disconnect(this->shared_from_this()); } //============================================================================== -template -const SlotType& ConnectionBody::getSlot() const +template +const typename ConnectionBody::SlotType& +ConnectionBody::getSlot() const { return mSlot; } diff --git a/python/dartpy/collision/CollisionResult.cpp b/python/dartpy/collision/CollisionResult.cpp index da5a2799839bd..91abfed61e916 100644 --- a/python/dartpy/collision/CollisionResult.cpp +++ b/python/dartpy/collision/CollisionResult.cpp @@ -80,9 +80,9 @@ void CollisionResult(py::module& m) +[](const dart::collision::CollisionResult* self) -> bool { return self->isCollision(); }) - .def( - "clear", - +[](dart::collision::CollisionResult* self) { self->clear(); }); + .def("clear", +[](dart::collision::CollisionResult* self) { + self->clear(); + }); } } // namespace python diff --git a/python/dartpy/dynamics/Shape.cpp b/python/dartpy/dynamics/Shape.cpp index e4cbc4a1915e7..5147c7b546360 100644 --- a/python/dartpy/dynamics/Shape.cpp +++ b/python/dartpy/dynamics/Shape.cpp @@ -324,8 +324,7 @@ void Shape(py::module& m) return self->getType(); }, ::py::return_value_policy::reference_internal) - .def( - "update", +[](dart::dynamics::MeshShape* self) { self->update(); }) + .def("update", +[](dart::dynamics::MeshShape* self) { self->update(); }) .def( "notifyAlphaUpdated", +[](dart::dynamics::MeshShape* self, double alpha) {