bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
32.96k stars 3.2k forks source link

False positives for Added/Changed on first system run #13426

Closed SpecificProtagonist closed 1 week ago

SpecificProtagonist commented 2 weeks ago

Bevy version

2aed777

What you did

I spawned an entity with a component:

#[derive(Component)]
struct Foo;
let mut world = World::new();
world.spawn(Foo);

Then ran a system that checks whether said component has been added since the last time it ran:

let sys = world.register_system(|query: Query<(), Added<Foo>>| assert!(query.is_empty()));
loop {
    world.run_system(sys).unwrap();
}

What went wrong

From the documentation of Added:

A filter on a component that only retains results added after the system last ran.

(and similarly for Changed: "[…] or mutably dereferenced after the system last ran.")

This never happened, yet the entity did not get filtered out the first time the system ran.

Additional information

SystemMeta::last_run can not represent the case that the system hasn't been run yet and therefore falsely gets initialized to 0.

alice-i-cecile commented 1 week ago

Definitely a bug. I wonder if we should be storing an Option<NonZero<u64>> or the equivalent inside of our ticks to resolve cases like this correctly.

Although simply swapping to u64 ticks may be enough to fix this.

SpecificProtagonist commented 1 week ago

Although simply swapping to u64 ticks may be enough to fix this.

Hmm, how would that help here?

alice-i-cecile commented 1 week ago

Oh, I see the problem here. I think the docs are wrong. The actual behavior (and the one that will generally be desired) is "detect added/changed components exactly once". Is there are reason why you want to exclude entities that were added before the system was run?

SpecificProtagonist commented 1 week ago

Yes: Because the system isn't active from the beginning. I do think this is useful behavior. But I can work around this by checking SystemChangeTick, and the current behavior is also useful and wouldn't have an easy workaround, so correcting the docs is the best thing to do here.