From b4da0962c46d0c1511a941d2643e787346ad5ecb Mon Sep 17 00:00:00 2001 From: Jeongseok Lee Date: Mon, 11 May 2020 21:09:03 -0700 Subject: [PATCH] Update signal to remove a connection when disconnected (#1462) --- CHANGELOG.md | 1 + dart/common/Signal.cpp | 17 +--- dart/common/Signal.hpp | 18 ++++- dart/common/detail/ConnectionBody.cpp | 18 ----- dart/common/detail/ConnectionBody.hpp | 63 +++++++++------ dart/common/detail/Signal.hpp | 88 +++++++-------------- python/dartpy/collision/CollisionResult.cpp | 6 +- python/dartpy/dynamics/Shape.cpp | 3 +- unittests/unit/test_Signal.cpp | 1 + 9 files changed, 94 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4379c10c3e70b..0ca385645328f 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 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..9680a96ce2fd3 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..db47180c4fff4 100644 --- a/dart/common/detail/ConnectionBody.hpp +++ b/dart/common/detail/ConnectionBody.hpp @@ -46,70 +46,85 @@ 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: + using SlotType = typename SignalType::SlotType; + /// 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 typename ConnectionBody::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/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) { 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;