Skip to content

Commit

Permalink
Fix Sparse Change Detection (bevyengine#6896)
Browse files Browse the repository at this point in the history
# Objective
bevyengine#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.
  • Loading branch information
james7132 authored and ItsDoot committed Feb 1, 2023
1 parent a142797 commit 5129787
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 1 deletion.
84 changes: 84 additions & 0 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,90 @@ mod tests {
assert_eq!(get_filtered::<Changed<B>>(&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<F: ReadOnlyWorldQuery>(world: &mut World) -> Vec<Entity> {
world
.query_filtered::<Entity, F>()
.iter(world)
.collect::<Vec<Entity>>()
}

assert_eq!(
get_filtered::<Changed<SparseStored>>(&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::<Changed<SparseStored>>(&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::<Changed<SparseStored>>(&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::<Changed<SparseStored>>(&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::<Changed<SparseStored>>(&mut world),
vec![e3],
"e1 should no longer be returned"
);

world.clear_trackers();

assert!(get_filtered::<Changed<SparseStored>>(&mut world).is_empty());

let e4 = world.spawn_empty().id();

world.entity_mut(e4).insert(SparseStored(0));
assert_eq!(get_filtered::<Changed<SparseStored>>(&mut world), vec![e4]);
assert_eq!(get_filtered::<Added<SparseStored>>(&mut world), vec![e4]);

world.entity_mut(e4).insert(A(1));
assert_eq!(get_filtered::<Changed<SparseStored>>(&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::<Added<SparseStored>>(&mut world).is_empty());
assert_eq!(get_filtered::<Changed<SparseStored>>(&mut world), vec![e4]);
}

#[test]
fn empty_spawn() {
let mut world = World::default();
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
}
}
Expand Down

0 comments on commit 5129787

Please sign in to comment.