From 5129787c220dd653c2150de03a4a5bc082429d06 Mon Sep 17 00:00:00 2001 From: James Liu Date: Sat, 10 Dec 2022 09:25:53 +0000 Subject: [PATCH] Fix Sparse Change Detection (#6896) # Objective #6547 accidentally broke change detection for SparseSet components by using `Ticks::from_tick_cells` with the wrong argument order. ## Solution Use the right argument order. Add a regression test. --- crates/bevy_ecs/src/lib.rs | 84 ++++++++++++++++++++++++++++++ crates/bevy_ecs/src/query/fetch.rs | 2 +- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index eb53d91b6f6476..aa6add8c849091 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -960,6 +960,90 @@ mod tests { assert_eq!(get_filtered::>(&mut world), vec![e4]); } + #[test] + fn changed_trackers_sparse() { + let mut world = World::default(); + let e1 = world.spawn(SparseStored(0)).id(); + let e2 = world.spawn(SparseStored(0)).id(); + let e3 = world.spawn(SparseStored(0)).id(); + world.spawn(SparseStored(0)); + + world.clear_trackers(); + + for (i, mut a) in world + .query::<&mut SparseStored>() + .iter_mut(&mut world) + .enumerate() + { + if i % 2 == 0 { + a.0 += 1; + } + } + + fn get_filtered(world: &mut World) -> Vec { + world + .query_filtered::() + .iter(world) + .collect::>() + } + + assert_eq!( + get_filtered::>(&mut world), + vec![e1, e3] + ); + + // ensure changing an entity's archetypes also moves its changed state + world.entity_mut(e1).insert(C); + + assert_eq!(get_filtered::>(&mut world), vec![e3, e1], "changed entities list should not change (although the order will due to archetype moves)"); + + // spawning a new SparseStored entity should not change existing changed state + world.entity_mut(e1).insert(SparseStored(0)); + assert_eq!( + get_filtered::>(&mut world), + vec![e3, e1], + "changed entities list should not change" + ); + + // removing an unchanged entity should not change changed state + assert!(world.despawn(e2)); + assert_eq!( + get_filtered::>(&mut world), + vec![e3, e1], + "changed entities list should not change" + ); + + // removing a changed entity should remove it from enumeration + assert!(world.despawn(e1)); + assert_eq!( + get_filtered::>(&mut world), + vec![e3], + "e1 should no longer be returned" + ); + + world.clear_trackers(); + + assert!(get_filtered::>(&mut world).is_empty()); + + let e4 = world.spawn_empty().id(); + + world.entity_mut(e4).insert(SparseStored(0)); + assert_eq!(get_filtered::>(&mut world), vec![e4]); + assert_eq!(get_filtered::>(&mut world), vec![e4]); + + world.entity_mut(e4).insert(A(1)); + assert_eq!(get_filtered::>(&mut world), vec![e4]); + + world.clear_trackers(); + + // ensure inserting multiple components set changed state for all components and set added + // state for non existing components even when changing archetype. + world.entity_mut(e4).insert(SparseStored(0)); + + assert!(get_filtered::>(&mut world).is_empty()); + assert_eq!(get_filtered::>(&mut world), vec![e4]); + } + #[test] fn empty_spawn() { let mut world = World::default(); diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index 6ca7ddd32e4db8..4a3f49f302d958 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/crates/bevy_ecs/src/query/fetch.rs @@ -771,7 +771,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T { .debug_checked_unwrap(); Mut { value: component.assert_unique().deref_mut(), - ticks: Ticks::from_tick_cells(ticks, fetch.change_tick, fetch.last_change_tick), + ticks: Ticks::from_tick_cells(ticks, fetch.last_change_tick, fetch.change_tick), } } }