Fix ViewMut::get_or_insert panic

A panic occurred when a component from a more recent entity was already present Also fix a few tracking errors in SparseSet::insert Improve get_or_default and get_or_default_with performance when a value is present Fix #210
leudz · Nov 13, 2024 · 8ab5e27 · 8ab5e27
1 parent b69f272
commit 8ab5e27
Show file tree

Hide file tree

Showing 6 changed files with 349 additions and 102 deletions.
diff --git a/src/add_component.rs b/src/add_component.rs
@@ -38,15 +38,15 @@ impl<T: Component, TRACK> AddComponent<T> for ViewMut<'_, T, TRACK> {
     #[inline]
     #[track_caller]
     fn add_component_unchecked(&mut self, entity: EntityId, component: T) {
-        self.sparse_set.insert(entity, component, self.current);
+        let _ = self.sparse_set.insert(entity, component, self.current);
     }
 }
 
 impl<T: Component, TRACK> AddComponent<T> for &mut ViewMut<'_, T, TRACK> {
     #[inline]
     #[track_caller]
     fn add_component_unchecked(&mut self, entity: EntityId, component: T) {
-        self.sparse_set.insert(entity, component, self.current);
+        let _ = self.sparse_set.insert(entity, component, self.current);
     }
 }
 
@@ -55,7 +55,7 @@ impl<T: Component, TRACK> AddComponent<Option<T>> for ViewMut<'_, T, TRACK> {
     #[track_caller]
     fn add_component_unchecked(&mut self, entity: EntityId, component: Option<T>) {
         if let Some(component) = component {
-            self.sparse_set.insert(entity, component, self.current);
+            let _ = self.sparse_set.insert(entity, component, self.current);
         }
     }
 }
@@ -65,7 +65,7 @@ impl<T: Component, TRACK> AddComponent<Option<T>> for &mut ViewMut<'_, T, TRACK>
     #[track_caller]
     fn add_component_unchecked(&mut self, entity: EntityId, component: Option<T>) {
         if let Some(component) = component {
-            self.sparse_set.insert(entity, component, self.current);
+            let _ = self.sparse_set.insert(entity, component, self.current);
         }
     }
 }
@@ -77,7 +77,7 @@ macro_rules! impl_add_component {
             #[track_caller]
             fn add_component_unchecked(&mut self, entity: EntityId, component: ($($component,)+)) {
                 $(
-                    self.$index.add_component_unchecked(entity, component.$index);
+                    let _ = self.$index.add_component_unchecked(entity, component.$index);
                 )+
             }
         }

diff --git a/src/add_distinct_component.rs b/src/add_distinct_component.rs
@@ -62,8 +62,9 @@ impl<T: Component + PartialEq> AddDistinctComponent for ViewMut<'_, T> {
             }
         }
 
-        self.sparse_set.insert(entity, component, self.current);
-        true
+        self.sparse_set
+            .insert(entity, component, self.current)
+            .was_inserted()
     }
 }
 
@@ -83,8 +84,9 @@ impl<T: Component + PartialEq> AddDistinctComponent for &mut ViewMut<'_, T> {
             }
         }
 
-        self.sparse_set.insert(entity, component, self.current);
-        true
+        self.sparse_set
+            .insert(entity, component, self.current)
+            .was_inserted()
     }
 }
 
@@ -114,3 +116,144 @@ macro_rules! add_component {
 }
 
 add_component![(ViewA, 0); (ViewB, 1) (ViewC, 2) (ViewD, 3) (ViewE, 4) (ViewF, 5) (ViewG, 6) (ViewH, 7) (ViewI, 8) (ViewJ, 9)];
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{track, Component, EntitiesViewMut, Get, ViewMut, World};
+
+    #[derive(PartialEq, Debug)]
+    struct USIZE(usize);
+
+    impl Component for USIZE {
+        type Tracking = track::InsertionAndModification;
+    }
+
+    /// Make sure that:
+    /// - `add_distinct_component_unchecked` inserts the component when no component is present.
+    /// - it correctly reports the success.
+    /// - the component is flagged as inserted.
+    /// - the component is not flagged as modified.
+    #[test]
+    fn add_no_component() {
+        let mut world = World::new();
+
+        let eid = world.add_entity(());
+
+        let mut usizes = world.borrow::<ViewMut<'_, USIZE>>().unwrap();
+
+        let was_inserted = usizes.add_distinct_component_unchecked(eid, USIZE(0));
+
+        assert!(was_inserted);
+        assert_eq!(usizes.get(eid).unwrap(), &USIZE(0));
+        assert!(usizes.is_inserted(eid));
+        assert!(!usizes.is_modified(eid));
+    }
+
+    /// Make sure that:
+    /// - `add_distinct_component_unchecked` inserts the component when a distinct component is present.
+    /// - it correctly reports the success.
+    /// - the component is not flagged as inserted.
+    /// - the component is flagged as modified.
+    #[test]
+    fn add_distinct_component() {
+        let mut world = World::new();
+
+        let eid = world.add_entity(USIZE(1));
+
+        world.run(|usizes: ViewMut<'_, USIZE>| {
+            usizes.clear_all_inserted();
+        });
+
+        let mut usizes = world.borrow::<ViewMut<'_, USIZE>>().unwrap();
+
+        let was_inserted = usizes.add_distinct_component_unchecked(eid, USIZE(0));
+
+        assert!(was_inserted);
+        assert_eq!(usizes.get(eid).unwrap(), &USIZE(0));
+        assert!(!usizes.is_inserted(eid));
+        assert!(usizes.is_modified(eid));
+    }
+
+    /// Make sure that:
+    /// - `add_distinct_component_unchecked` does not insert the component when an equal component is present.
+    /// - it correctly reports the failure.
+    /// - the component is not flagged as inserted.
+    /// - the component is not flagged as modified.
+    #[test]
+    fn add_identical_component() {
+        let mut world = World::new();
+
+        let eid = world.add_entity(USIZE(0));
+
+        world.run(|usizes: ViewMut<'_, USIZE>| {
+            usizes.clear_all_inserted();
+        });
+
+        let mut usizes = world.borrow::<ViewMut<'_, USIZE>>().unwrap();
+
+        let was_inserted = usizes.add_distinct_component_unchecked(eid, USIZE(0));
+
+        assert!(!was_inserted);
+        assert!(!usizes.is_inserted(eid));
+        assert!(!usizes.is_modified(eid));
+    }
+
+    /// Make sure that:
+    /// - `add_distinct_component_unchecked` does not insert the component when a component from an entity with a larger generation is present.
+    /// - it correctly reports the failure.
+    /// - the component is not flagged as inserted.
+    /// - the component is not flagged as modified.
+    #[test]
+    fn add_smaller_gen_component() {
+        let mut world = World::new();
+
+        let eid1 = world.add_entity(());
+        world.delete_entity(eid1);
+        let eid2 = world.add_entity(USIZE(1));
+
+        assert_eq!(eid1.index(), eid2.index());
+
+        let mut usizes = world.borrow::<ViewMut<'_, USIZE>>().unwrap();
+
+        let was_inserted = usizes.add_distinct_component_unchecked(eid1, USIZE(0));
+
+        assert!(usizes.get(eid1).is_err());
+        assert_eq!(usizes.get(eid2).unwrap(), &USIZE(1));
+
+        assert!(!was_inserted);
+        assert!(!usizes.is_inserted(eid1));
+        assert!(!usizes.is_modified(eid1));
+    }
+
+    /// Make sure that:
+    /// - `add_distinct_component_unchecked` inserts the component when a component from an entity with a smaller generation is present.
+    /// - it correctly reports the success.
+    /// - the component is flagged as inserted.
+    /// - the component is not flagged as modified.
+    #[test]
+    fn add_larger_gen_component() {
+        let mut world = World::new();
+
+        let eid1 = world.add_entity(USIZE(1));
+
+        world.run(|mut entities: EntitiesViewMut<'_>| {
+            entities.delete_unchecked(eid1);
+        });
+
+        let eid2 = world.add_entity(());
+
+        assert_eq!(eid1.index(), eid2.index());
+
+        let mut usizes = world.borrow::<ViewMut<'_, USIZE>>().unwrap();
+
+        let was_inserted = usizes.add_distinct_component_unchecked(eid2, USIZE(0));
+
+        assert!(usizes.get(eid1).is_err());
+        assert_eq!(usizes.get(eid2).unwrap(), &USIZE(0));
+
+        assert!(was_inserted);
+        assert!(usizes.is_inserted(eid2));
+        assert!(!usizes.is_modified(eid2));
+    }
+}
diff --git a/src/add_entity.rs b/src/add_entity.rs
@@ -33,7 +33,7 @@ impl<T: Component, TRACK> AddEntity for &mut ViewMut<'_, T, TRACK> {
     #[inline]
     #[track_caller]
     fn add_entity(storage: &mut Self, entity: EntityId, component: Self::Component) {
-        storage
+        let _ = storage
             .sparse_set
             .insert(entity, component, storage.current);
     }

diff --git a/src/sparse_set/add_component.rs b/src/sparse_set/add_component.rs
@@ -34,7 +34,8 @@ impl<T: Send + Sync + Component> TupleAddComponent for T {
     ) {
         all_storages
             .exclusive_storage_or_insert_mut(StorageId::of::<SparseSet<T>>(), SparseSet::new)
-            .insert(entity, self, current);
+            .insert(entity, self, current)
+            .assert_inserted();
     }
 }
 
@@ -50,7 +51,8 @@ impl<T: Send + Sync + Component> TupleAddComponent for Option<T> {
         if let Some(component) = self {
             all_storages
                 .exclusive_storage_or_insert_mut(StorageId::of::<SparseSet<T>>(), SparseSet::new)
-                .insert(entity, component, current);
+                .insert(entity, component, current)
+                .assert_inserted();
         }
     }
 }