storage: valid state on component removal (allows component destructors to access pools)

src/entt/entity/sparse_set.hpp

@@ -173,11 +173,25 @@ class basic_sparse_set {
     }
 
 protected:
-    /*! @brief Swaps two entities in the internal packed array. */
-    virtual void swap_at(const std::size_t, const std::size_t) {}
+    /**
+     * @brief Swaps two entities in the internal packed array.
+     * @param lhs A valid position of an entity within storage.
+     * @param rhs A valid position of an entity within storage.
+     */
+    virtual void swap_at(const std::size_t lhs, const std::size_t rhs) {}
+
+    /**
+     * @brief Attempts to remove an entity from the internal packed array.
+     * @param pos A valid position of an entity within storage.
+     */
+    virtual void swap_and_pop(const std::size_t pos) {}
 
-    /*! @brief Attempts to remove an entity from the internal packed array. */
-    virtual void swap_and_pop(const std::size_t, void *) {}
+    /**
+     * @brief Last chance to use an entity that is about to be removed.
+     * @param entity A valid entity identifier.
+     * @param ud Optional user data that are forwarded as-is to derived classes.
+     */
+    virtual void about_to_remove(const Entity entity, void *ud) {}
 
 public:
     /*! @brief Underlying entity identifier. */
@@ -445,19 +459,23 @@ public:
      */
     void remove(const entity_type entt, void *ud = nullptr) {
         ENTT_ASSERT(contains(entt), "Set does not contain entity");
-        auto &ref = sparse[page(entt)][offset(entt)];
 
         // last chance to use the entity for derived classes and mixins, if any
-        swap_and_pop(size_type{to_integral(ref)}, ud);
+        about_to_remove(entt, ud);
+
+        auto &ref = sparse[page(entt)][offset(entt)];
+        const auto pos = size_type{to_integral(ref)};
 
         const auto other = packed.back();
         sparse[page(other)][offset(other)] = ref;
-        // if it looks weird, imagine what the subtle bugs it prevents are
-        ENTT_ASSERT((packed.back() = entt, true), "");
-        packed[size_type{to_integral(ref)}] = other;
         ref = null;
 
+        // if it looks weird, imagine what the subtle bugs it prevents are
+        ENTT_ASSERT((packed.back() = entt, true), "");
+        packed[pos] = other;
         packed.pop_back();
+
+        swap_and_pop(pos);
     }
 
     /**

src/entt/entity/storage.hpp

@@ -159,22 +159,16 @@ class basic_storage: public basic_sparse_set<Entity> {
     };
 
 protected:
-    /**
-     * @copybrief basic_sparse_set::swap_at
-     * @param lhs A valid position of an entity within storage.
-     * @param rhs A valid position of an entity within storage.
-     */
-    void swap_at(const std::size_t lhs, const std::size_t rhs) {
+    /*! @copydoc basic_sparse_set::swap_at */
+    void swap_at(const std::size_t lhs, const std::size_t rhs) final {
         std::swap(instances[lhs], instances[rhs]);
     }
 
-    /**
-     * @copybrief basic_sparse_set::swap_and_pop
-     * @param pos A valid position of an entity within storage.
-     */
-    void swap_and_pop(const std::size_t pos, void *) {
-        auto other = std::move(instances.back());
-        instances[pos] = std::move(other);
+    /*! @copydoc basic_sparse_set::swap_and_pop */
+    void swap_and_pop(const std::size_t pos) final {
+        // required because pop_back isn't guaranteed to shrink before removing the element
+        [[maybe_unused]] auto other = std::move(instances[pos]);
+        instances[pos] = std::move(instances.back());
         instances.pop_back();
     }
 
@@ -632,17 +626,13 @@ struct storage_adapter_mixin: Type {
  */
 template<typename Type>
 class sigh_storage_mixin final: public Type {
-    /**
-     * @copybrief basic_sparse_set::swap_and_pop
-     * @param pos A valid position of an entity within storage.
-     * @param ud Optional user data that are forwarded as-is to derived classes.
-     */
-    void swap_and_pop(const std::size_t pos, void *ud) final {
+    using Entity = typename Type::entity_type;
+
+    /*! @copydoc basic_sparse_set::about_to_remove */
+    void about_to_remove(const Entity entity, void *ud) final {
         ENTT_ASSERT(ud != nullptr, "Invalid pointer to registry");
-        const auto entity = basic_sparse_set<typename Type::entity_type>::operator[](pos);
         destruction.publish(*static_cast<basic_registry<typename Type::entity_type> *>(ud), entity);
-        // the position may have changed due to the actions of a listener
-        Type::swap_and_pop(this->index(entity), ud);
+        Type::about_to_remove(entity, ud);
     }
 
 public:

test/entt/entity/storage.cpp

@@ -24,6 +24,17 @@ struct throwing_component {
     int data;
 };
 
+struct update_from_destructor {
+    ~update_from_destructor() {
+        if(target != entt::null) {
+            storage->remove(target);
+        }
+    }
+
+    entt::storage<update_from_destructor> *storage{};
+    entt::entity target{entt::null};
+};
+
 TEST(Storage, Functionalities) {
     entt::storage<int> pool;
 
@@ -649,3 +660,28 @@ TEST(Storage, ConstructorExceptionDoesNotAddToStorage) {
 
     ASSERT_TRUE(pool.empty());
 }
+
+TEST(Storage, UpdateFromDestructor) {
+    static constexpr auto size = 10u;
+
+    auto test = [](const auto target) {
+        entt::storage<update_from_destructor> pool;
+
+        for(std::size_t next{}; next < size; ++next) {
+            pool.emplace(entt::entity(next), &pool);
+        }
+
+        for(std::size_t next{}; next < size; ++next) {
+            ASSERT_EQ(pool.get(entt::entity(next)).target, entt::entity{entt::null});
+        }
+
+        pool.get(entt::entity(size/2)).target = target;
+        pool.remove(entt::entity(size/2));
+
+        ASSERT_EQ(pool.size(), size - 1u - (target != entt::null));
+    };
+
+    test(entt::null);
+    test(entt::entity(size - 1u));
+    test(entt::entity{0u});
+}