Delete components after killing entities to avoid clearing components when the entity has the wrong generation

.github/workflows/ci.yml

@@ -25,7 +25,7 @@ jobs:
       fail-fast: true
       matrix:
         os: [macos-latest, windows-latest, ubuntu-latest]
-        toolchain: [stable, beta, nightly, 1.60.0]
+        toolchain: [stable, beta, nightly, 1.65.0]
     steps:
       - uses: actions/checkout@v2
 
@@ -56,7 +56,8 @@ jobs:
       #  if: matrix.toolchain == 'stable' && matrix.os == 'ubuntu-latest'
 
       # run tests
-      - run: cargo install cargo-hack
+      - name: install cargo-hack
+        uses: taiki-e/install-action@cargo-hack
       - run: cargo hack test --workspace --each-feature
         if: matrix.toolchain == 'nightly'
       - run: cargo hack test --workspace --each-feature --skip nightly

CHANGELOG.md

@@ -1,8 +1,12 @@
-# 0.19.0 (2022-07-15)
+# 0.19.0 (2023-06-10)
 
+* Bump MSRV to 1.65.0 ([#766])
+* Added index where entity deletion stopped to the error returned from `WorldExt::delete_entities` ([#766])
+* Fix bug where deleting an entity with the wrong generation could clear the components of an existing entity. ([#766])
 * Bump shred to version `0.14.1`, MSRV to 1.60.0 ([shred changelog][shred-changelog], [#756])
 
 [#756]: https://github.com/amethyst/specs/pull/756
+[#766]: https://github.com/amethyst/specs/pull/766
 
 # 0.18.0 (2022-07-02)
 

Cargo.toml

@@ -14,7 +14,7 @@ license = "MIT OR Apache-2.0"
 authors = ["slide-rs hackers"]
 include = ["src", "README.md", "LICENSE-MIT", "LICENSE-APACHE"]
 edition = "2021"
-rust-version = "1.60.0"
+rust-version = "1.65.0"
 
 # the `storage_cmp` and `storage_sparse` benches are called from `benches_main`
 autobenches = false

README.md

@@ -35,7 +35,7 @@ Unlike most other ECS libraries out there, it provides
       other and you can use barriers to force several stages in system execution
 * high performance for real-world applications
 
-Minimum Rust version: 1.40
+Minimum Rust version: 1.65
 
 ## [Link to the book][book]
 

deny.toml

@@ -28,3 +28,7 @@ allow = ["MIT", "Apache-2.0", "Unlicense", "BSD-3-Clause"]
 
 # We want really high confidence when inferring licenses from text
 confidence-threshold = 0.93
+
+exceptions = [
+    { allow = ["Unicode-DFS-2016"], name = "unicode-ident" },
+]

src/world/entity.rs

@@ -60,12 +60,16 @@ pub(crate) struct Allocator {
 
 impl Allocator {
     /// Kills a list of entities immediately.
-    pub fn kill(&mut self, delete: &[Entity]) -> Result<(), WrongGeneration> {
-        for &entity in delete {
+    ///
+    /// If an entity with an outdated generation is encountered, the index of
+    /// that entity within the provided slice is returned (entities after this
+    /// index are not killed).
+    pub fn kill(&mut self, delete: &[Entity]) -> Result<(), (WrongGeneration, usize)> {
+        for (index, &entity) in delete.iter().enumerate() {
             let id = entity.id() as usize;
 
             if !self.is_alive(entity) {
-                return self.del_err(entity);
+                return Err((self.del_err(entity), index));
             }
 
             self.alive.remove(entity.id());
@@ -89,22 +93,22 @@ impl Allocator {
     /// maintained).
     pub fn kill_atomic(&self, e: Entity) -> Result<(), WrongGeneration> {
         if !self.is_alive(e) {
-            return self.del_err(e);
+            return Err(self.del_err(e));
         }
 
         self.killed.add_atomic(e.id());
 
         Ok(())
     }
 
-    pub(crate) fn del_err(&self, e: Entity) -> Result<(), WrongGeneration> {
-        Err(WrongGeneration {
+    pub(crate) fn del_err(&self, e: Entity) -> WrongGeneration {
+        WrongGeneration {
             action: "delete",
             actual_gen: self.generations[e.id() as usize]
                 .0
                 .unwrap_or_else(Generation::one),
             entity: e,
-        })
+        }
     }
 
     /// Return `true` if the entity is alive.

src/world/world_ext.rs

@@ -259,7 +259,11 @@ pub trait WorldExt {
     fn delete_entity(&mut self, entity: Entity) -> Result<(), WrongGeneration>;
 
     /// Deletes the specified entities and their components.
-    fn delete_entities(&mut self, delete: &[Entity]) -> Result<(), WrongGeneration>;
+    ///
+    /// If an entity with an outdated generation is encountered, the index of
+    /// that entity within the provided slice is returned (entities after this
+    /// index are not deleted).
+    fn delete_entities(&mut self, delete: &[Entity]) -> Result<(), (WrongGeneration, usize)>;
 
     /// Deletes all entities and their components.
     fn delete_all(&mut self);
@@ -315,8 +319,6 @@ impl WorldExt for World {
     {
         self.entry()
             .or_insert_with(move || MaskedStorage::<T>::new(storage()));
-        self.entry::<MetaTable<dyn AnyStorage>>()
-            .or_insert_with(Default::default);
         self.fetch_mut::<MetaTable<dyn AnyStorage>>()
             .register(&*self.fetch::<MaskedStorage<T>>());
     }
@@ -369,12 +371,18 @@ impl WorldExt for World {
 
     fn delete_entity(&mut self, entity: Entity) -> Result<(), WrongGeneration> {
         self.delete_entities(&[entity])
+            .map_err(|(wrong_gen, _)| wrong_gen)
     }
 
-    fn delete_entities(&mut self, delete: &[Entity]) -> Result<(), WrongGeneration> {
-        self.delete_components(delete);
-
-        self.entities_mut().alloc.kill(delete)
+    fn delete_entities(&mut self, delete: &[Entity]) -> Result<(), (WrongGeneration, usize)> {
+        let res = self.entities_mut().alloc.kill(delete);
+        if let Err((wrong_gen, failed_index)) = res {
+            self.delete_components(&delete[..failed_index]);
+            Err((wrong_gen, failed_index))
+        } else {
+            self.delete_components(delete);
+            Ok(())
+        }
     }
 
     fn delete_all(&mut self) {
@@ -406,8 +414,6 @@ impl WorldExt for World {
     }
 
     fn delete_components(&mut self, delete: &[Entity]) {
-        self.entry::<MetaTable<dyn AnyStorage>>()
-            .or_insert_with(Default::default);
         for storage in self.fetch_mut::<MetaTable<dyn AnyStorage>>().iter_mut(self) {
             storage.drop(delete);
         }

tests/tests.rs

@@ -53,6 +53,35 @@ fn task_panics() {
         .dispatch(&mut world);
 }
 
+#[test]
+fn delete_wrong_gen() {
+    let mut world = create_world();
+
+    // create
+    let entity_a = world.create_entity().with(CompInt(7)).build();
+    assert_eq!(
+        world.read_component::<CompInt>().get(entity_a),
+        Some(&CompInt(7))
+    );
+    // delete
+    assert!(world.delete_entity(entity_a).is_ok());
+    assert_eq!(world.read_component::<CompInt>().get(entity_a), None);
+    // create
+    let entity_b = world.create_entity().with(CompInt(6)).build();
+    assert_eq!(
+        world.read_component::<CompInt>().get(entity_b),
+        Some(&CompInt(6))
+    );
+    assert_eq!(world.read_component::<CompInt>().get(entity_a), None);
+    // delete stale
+    assert!(world.delete_entity(entity_a).is_err());
+    assert_eq!(
+        world.read_component::<CompInt>().get(entity_b),
+        Some(&CompInt(6))
+    );
+    assert_eq!(world.read_component::<CompInt>().get(entity_a), None);
+}
+
 #[test]
 fn dynamic_create() {
     struct Sys;