From b14edbe1f43eff2ff4bd82a743d2446682fc4f67 Mon Sep 17 00:00:00 2001
From: Michele Caini <michele.caini@gmail.com>
Date: Sun, 16 Jun 2019 18:39:56 +0200
Subject: [PATCH] review: observer

---
 src/entt/entity/observer.hpp  | 102 ++++++++++++++++++----------------
 test/entt/entity/observer.cpp |  11 ++++
 2 files changed, 65 insertions(+), 48 deletions(-)

diff --git a/src/entt/entity/observer.hpp b/src/entt/entity/observer.hpp
index b51d5b85d..31682e794 100644
--- a/src/entt/entity/observer.hpp
+++ b/src/entt/entity/observer.hpp
@@ -168,40 +168,44 @@ class basic_observer {
     using traits_type = entt_traits<Entity>;
     using payload_type = std::uint32_t;
 
-    template<std::size_t Index, typename>
+    template<typename>
     struct matcher_handler;
 
-    template<std::size_t Index, typename... Reject, typename... Require, typename AnyOf>
-    struct matcher_handler<Index, matcher<matcher<type_list<Reject...>, type_list<Require...>>, AnyOf>> {
+    template<typename... Reject, typename... Require, typename AnyOf>
+    struct matcher_handler<matcher<matcher<type_list<Reject...>, type_list<Require...>>, AnyOf>> {
+        template<std::size_t Index>
         static void maybe_valid_if(basic_observer *obs, const basic_registry<Entity> &reg, const Entity entt) {
             if(reg.template has<Require...>(entt) && !(reg.template has<Reject>(entt) || ...)) {
                 (obs->view.has(entt) ? obs->view.get(entt) : obs->view.construct(entt)) |= (1 << Index);
             }
         }
 
+        template<std::size_t Index>
         static void discard_if(basic_observer *obs, const basic_registry<Entity> &, const Entity entt) {
             if(auto *value = obs->view.try_get(entt); value && !(*value &= (~(1 << Index)))) {
                 obs->view.destroy(entt);
             }
         }
 
-        static void disconnect(basic_registry<Entity> &reg, const basic_observer &obs) {
-            reg.template on_replace<AnyOf>().disconnect(&obs);
-            reg.template on_destroy<AnyOf>().disconnect(&obs);
-            (reg.template on_destroy<Require>().disconnect(&obs), ...);
-            (reg.template on_construct<Reject>().disconnect(&obs), ...);
+        template<std::size_t Index>
+        static void connect(basic_observer &obs, basic_registry<Entity> &reg) {
+            (reg.template on_destroy<Require>().template connect<&discard_if<Index>>(&obs), ...);
+            (reg.template on_construct<Reject>().template connect<&discard_if<Index>>(&obs), ...);
+            reg.template on_replace<AnyOf>().template connect<&maybe_valid_if<Index>>(&obs);
+            reg.template on_destroy<AnyOf>().template connect<&discard_if<Index>>(&obs);
         }
 
-        static void connect(basic_observer &obs, basic_registry<Entity> &reg) {
-            reg.template on_replace<AnyOf>().template connect<&maybe_valid_if>(&obs);
-            reg.template on_destroy<AnyOf>().template connect<&discard_if>(&obs);
-            (reg.template on_destroy<Require>().template connect<&discard_if>(&obs), ...);
-            (reg.template on_construct<Reject>().template connect<&discard_if>(&obs), ...);
+        static void disconnect(basic_observer &obs, basic_registry<Entity> &reg) {
+            (reg.template on_destroy<Require>().disconnect(&obs), ...);
+            (reg.template on_construct<Reject>().disconnect(&obs), ...);
+            reg.template on_replace<AnyOf>().disconnect(&obs);
+            reg.template on_destroy<AnyOf>().disconnect(&obs);
         }
     };
 
-    template<std::size_t Index, typename... Reject, typename... Require, typename... NoneOf, typename... AllOf>
-    struct matcher_handler<Index, matcher<matcher<type_list<Reject...>, type_list<Require...>>, type_list<NoneOf...>, type_list<AllOf...>>> {
+    template<typename... Reject, typename... Require, typename... NoneOf, typename... AllOf>
+    struct matcher_handler<matcher<matcher<type_list<Reject...>, type_list<Require...>>, type_list<NoneOf...>, type_list<AllOf...>>> {
+        template<std::size_t Index>
         static void maybe_valid_if(basic_observer *obs, const basic_registry<Entity> &reg, const Entity entt) {
             if(reg.template has<AllOf...>(entt) && !(reg.template has<NoneOf>(entt) || ...)
                     && reg.template has<Require...>(entt) && !(reg.template has<Reject>(entt) || ...))
@@ -210,41 +214,43 @@ class basic_observer {
             }
         }
 
+        template<std::size_t Index>
         static void discard_if(basic_observer *obs, const basic_registry<Entity> &, const Entity entt) {
             if(auto *value = obs->view.try_get(entt); value && !(*value &= (~(1 << Index)))) {
                 obs->view.destroy(entt);
             }
         }
 
-        static void disconnect(basic_registry<Entity> &reg, const basic_observer &obs) {
-            ((reg.template on_construct<AllOf>().disconnect(&obs)), ...);
-            ((reg.template on_destroy<AllOf>().disconnect(&obs)), ...);
-            ((reg.template on_construct<NoneOf>().disconnect(&obs)), ...);
-            ((reg.template on_destroy<NoneOf>().disconnect(&obs)), ...);
-            (reg.template on_destroy<Require>().disconnect(&obs), ...);
-            (reg.template on_construct<Reject>().disconnect(&obs), ...);
+        template<std::size_t Index>
+        static void connect(basic_observer &obs, basic_registry<Entity> &reg) {
+            (reg.template on_destroy<Require>().template connect<&discard_if<Index>>(&obs), ...);
+            (reg.template on_construct<Reject>().template connect<&discard_if<Index>>(&obs), ...);
+            (reg.template on_construct<AllOf>().template connect<&maybe_valid_if<Index>>(&obs), ...);
+            (reg.template on_destroy<NoneOf>().template connect<&maybe_valid_if<Index>>(&obs), ...);
+            (reg.template on_destroy<AllOf>().template connect<&discard_if<Index>>(&obs), ...);
+            (reg.template on_construct<NoneOf>().template connect<&discard_if<Index>>(&obs), ...);
         }
 
-        static void connect(basic_observer &obs, basic_registry<Entity> &reg) {
-            (reg.template on_construct<AllOf>().template connect<&maybe_valid_if>(&obs), ...);
-            (reg.template on_destroy<NoneOf>().template connect<&maybe_valid_if>(&obs), ...);
-            (reg.template on_destroy<AllOf>().template connect<&discard_if>(&obs), ...);
-            (reg.template on_construct<NoneOf>().template connect<&discard_if>(&obs), ...);
-            (reg.template on_destroy<Require>().template connect<&discard_if>(&obs), ...);
-            (reg.template on_construct<Reject>().template connect<&discard_if>(&obs), ...);
+        static void disconnect(basic_observer &obs, basic_registry<Entity> &reg) {
+            (reg.template on_destroy<Require>().disconnect(&obs), ...);
+            (reg.template on_construct<Reject>().disconnect(&obs), ...);
+            (reg.template on_construct<AllOf>().disconnect(&obs), ...);
+            (reg.template on_destroy<NoneOf>().disconnect(&obs), ...);
+            (reg.template on_destroy<AllOf>().disconnect(&obs), ...);
+            (reg.template on_construct<NoneOf>().disconnect(&obs), ...);
         }
     };
 
-    template<auto... Disconnect>
-    static void disconnect(basic_registry<Entity> &reg, const basic_observer &obs) {
-        (Disconnect(reg, obs), ...);
+    template<typename... Matcher>
+    static void disconnect(basic_observer &obs, basic_registry<Entity> &reg) {
+        (matcher_handler<Matcher>::disconnect(obs, reg), ...);
     }
 
     template<typename... Matcher, std::size_t... Index>
     void connect(basic_registry<Entity> &reg, std::index_sequence<Index...>) {
         static_assert(sizeof...(Matcher) < std::numeric_limits<payload_type>::digits);
-        release = &basic_observer::disconnect<&matcher_handler<Index, Matcher>::disconnect...>;
-        (matcher_handler<Index, Matcher>::connect(*this, reg), ...);
+        (matcher_handler<Matcher>::template connect<Index>(*this, reg), ...);
+        release = &basic_observer::disconnect<Matcher...>;
     }
 
 public:
@@ -265,18 +271,6 @@ public:
     /*! @brief Default move constructor, deleted on purpose. */
     basic_observer(basic_observer &&) = delete;
 
-    /**
-     * @brief Default copy assignment operator, deleted on purpose.
-     * @return This observer.
-     */
-    basic_observer & operator=(const basic_observer &) = delete;
-
-    /**
-     * @brief Default move assignment operator, deleted on purpose.
-     * @return This observer.
-     */
-    basic_observer & operator=(basic_observer &&) = delete;
-
     /**
      * @brief Creates an observer and connects it to a given registry.
      * @tparam Matcher Types of matchers to use to initialize the observer.
@@ -294,6 +288,18 @@ public:
     /*! @brief Default destructor. */
     ~basic_observer() = default;
 
+    /**
+     * @brief Default copy assignment operator, deleted on purpose.
+     * @return This observer.
+     */
+    basic_observer & operator=(const basic_observer &) = delete;
+
+    /**
+     * @brief Default move assignment operator, deleted on purpose.
+     * @return This observer.
+     */
+    basic_observer & operator=(basic_observer &&) = delete;
+
     /**
      * @brief Connects an observer to a given registry.
      * @tparam Matcher Types of matchers to use to initialize the observer.
@@ -310,7 +316,7 @@ public:
     /*! @brief Disconnects an observer from the registry it keeps track of. */
     void disconnect() {
         if(release) {
-            release(*target, *this);
+            release(*this, *target);
             release = nullptr;
         }
     }
@@ -420,7 +426,7 @@ public:
 
 private:
     basic_registry<entity_type> *target;
-    void(* release)(basic_registry<Entity> &, const basic_observer &);
+    void(* release)(basic_observer &, basic_registry<Entity> &);
     storage<entity_type, payload_type> view;
 };
 
diff --git a/test/entt/entity/observer.cpp b/test/entt/entity/observer.cpp
index 9562552ed..aa40b5b65 100644
--- a/test/entt/entity/observer.cpp
+++ b/test/entt/entity/observer.cpp
@@ -122,6 +122,12 @@ TEST(Observer, AllOfFiltered) {
     registry.remove<double>(entity);
 
     ASSERT_TRUE(observer.empty());
+
+    observer.disconnect();
+    registry.remove<int>(entity);
+    registry.assign<int>(entity);
+
+    ASSERT_TRUE(observer.empty());
 }
 
 TEST(Observer, Observe) {
@@ -195,6 +201,11 @@ TEST(Observer, ObserveFiltered) {
     registry.remove<double>(entity);
 
     ASSERT_TRUE(observer.empty());
+
+    observer.disconnect();
+    registry.replace<int>(entity);
+
+    ASSERT_TRUE(observer.empty());
 }
 
 TEST(Observer, AllOfObserve) {