Browse Source

sparse_set: fix cross range-erase can break when using built-in iterators (close #913)

Michele Caini 3 years ago
parent
commit
95b16a0b04
2 changed files with 63 additions and 5 deletions
  1. 7 5
      src/entt/entity/sparse_set.hpp
  2. 56 0
      test/entt/entity/sparse_set.cpp

+ 7 - 5
src/entt/entity/sparse_set.hpp

@@ -238,12 +238,14 @@ protected:
      * @param it An iterator to the element to pop.
      * @param it An iterator to the element to pop.
      */
      */
     void swap_and_pop(const basic_iterator it) {
     void swap_and_pop(const basic_iterator it) {
-        sparse_ref(packed.back()) = entity_traits::combine(static_cast<typename entity_traits::entity_type>(it.index()), entity_traits::to_integral(packed.back()));
-        const auto entt = std::exchange(packed[it.index()], packed.back());
+        auto &self = sparse_ref(*it);
+        const auto entt = entity_traits::to_entity(self);
+        sparse_ref(packed.back()) = entity_traits::combine(entt, entity_traits::to_integral(packed.back()));
+        packed[static_cast<size_type>(entt)] = packed.back();
         // unnecessary but it helps to detect nasty bugs
         // unnecessary but it helps to detect nasty bugs
         ENTT_ASSERT((packed.back() = null, true), "");
         ENTT_ASSERT((packed.back() = null, true), "");
         // lazy self-assignment guard
         // lazy self-assignment guard
-        sparse_ref(entt) = null;
+        self = null;
         packed.pop_back();
         packed.pop_back();
     }
     }
 
 
@@ -252,8 +254,8 @@ protected:
      * @param it An iterator to the element to pop.
      * @param it An iterator to the element to pop.
      */
      */
     void in_place_pop(const basic_iterator it) {
     void in_place_pop(const basic_iterator it) {
-        sparse_ref(*it) = null;
-        packed[it.index()] = std::exchange(free_list, entity_traits::combine(static_cast<typename entity_traits::entity_type>(it.index()), entity_traits::reserved));
+        const auto entt = entity_traits::to_entity(std::exchange(sparse_ref(*it), null));
+        packed[static_cast<size_type>(entt)] = std::exchange(free_list, entity_traits::combine(entt, entity_traits::reserved));
     }
     }
 
 
 protected:
 protected:

+ 56 - 0
test/entt/entity/sparse_set.cpp

@@ -437,6 +437,20 @@ ENTT_DEBUG_TEST(SparseSetDeathTest, Erase) {
     ASSERT_DEATH(set.erase(entt::null), "");
     ASSERT_DEATH(set.erase(entt::null), "");
 }
 }
 
 
+TEST(SparseSet, CrossErase) {
+    entt::sparse_set set;
+    entt::sparse_set other;
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
+
+    set.insert(std::begin(entities), std::end(entities));
+    other.emplace(entities[1u]);
+    set.erase(other.begin(), other.end());
+
+    ASSERT_TRUE(set.contains(entities[0u]));
+    ASSERT_FALSE(set.contains(entities[1u]));
+    ASSERT_EQ(set.data()[0u], entities[0u]);
+}
+
 TEST(SparseSet, StableErase) {
 TEST(SparseSet, StableErase) {
     using traits_type = entt::entt_traits<entt::entity>;
     using traits_type = entt::entt_traits<entt::entity>;
 
 
@@ -561,6 +575,20 @@ ENTT_DEBUG_TEST(SparseSetDeathTest, StableErase) {
     ASSERT_DEATH(set.erase(entt::null), "");
     ASSERT_DEATH(set.erase(entt::null), "");
 }
 }
 
 
+TEST(SparseSet, CrossStableErase) {
+    entt::sparse_set set{entt::deletion_policy::in_place};
+    entt::sparse_set other{entt::deletion_policy::in_place};
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
+
+    set.insert(std::begin(entities), std::end(entities));
+    other.emplace(entities[1u]);
+    set.erase(other.begin(), other.end());
+
+    ASSERT_TRUE(set.contains(entities[0u]));
+    ASSERT_FALSE(set.contains(entities[1u]));
+    ASSERT_EQ(set.data()[0u], entities[0u]);
+}
+
 TEST(SparseSet, Remove) {
 TEST(SparseSet, Remove) {
     using traits_type = entt::entt_traits<entt::entity>;
     using traits_type = entt::entt_traits<entt::entity>;
 
 
@@ -618,6 +646,20 @@ TEST(SparseSet, Remove) {
     ASSERT_EQ(set.remove(entt::null), 0u);
     ASSERT_EQ(set.remove(entt::null), 0u);
 }
 }
 
 
+TEST(SparseSet, CrossRemove) {
+    entt::sparse_set set;
+    entt::sparse_set other;
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
+
+    set.insert(std::begin(entities), std::end(entities));
+    other.emplace(entities[1u]);
+    set.remove(other.begin(), other.end());
+
+    ASSERT_TRUE(set.contains(entities[0u]));
+    ASSERT_FALSE(set.contains(entities[1u]));
+    ASSERT_EQ(set.data()[0u], entities[0u]);
+}
+
 TEST(SparseSet, StableRemove) {
 TEST(SparseSet, StableRemove) {
     using traits_type = entt::entt_traits<entt::entity>;
     using traits_type = entt::entt_traits<entt::entity>;
 
 
@@ -748,6 +790,20 @@ TEST(SparseSet, StableRemove) {
     ASSERT_EQ(set.remove(entt::null), 0u);
     ASSERT_EQ(set.remove(entt::null), 0u);
 }
 }
 
 
+TEST(SparseSet, CrossStableRemove) {
+    entt::sparse_set set{entt::deletion_policy::in_place};
+    entt::sparse_set other{entt::deletion_policy::in_place};
+    entt::entity entities[2u]{entt::entity{3}, entt::entity{42}};
+
+    set.insert(std::begin(entities), std::end(entities));
+    other.emplace(entities[1u]);
+    set.remove(other.begin(), other.end());
+
+    ASSERT_TRUE(set.contains(entities[0u]));
+    ASSERT_FALSE(set.contains(entities[1u]));
+    ASSERT_EQ(set.data()[0u], entities[0u]);
+}
+
 TEST(SparseSet, Compact) {
 TEST(SparseSet, Compact) {
     entt::sparse_set set{entt::deletion_policy::in_place};
     entt::sparse_set set{entt::deletion_policy::in_place};