Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
SpecificProtagonist opened this issue May 19, 2024 · 4 comments · Fixed by #13458
Closed

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

SpecificProtagonist opened this issue May 19, 2024 · 4 comments · Fixed by #13458
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Comments

@SpecificProtagonist
Copy link
Contributor

SpecificProtagonist commented May 19, 2024

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.

@SpecificProtagonist SpecificProtagonist added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels May 19, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Triage This issue needs to be labelled labels May 21, 2024
@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 21, 2024

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
Copy link
Contributor Author

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

Hmm, how would that help here?

@alice-i-cecile
Copy link
Member

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?

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation S-Needs-Investigation This issue requires detective work to figure out what's going wrong and removed C-Bug An unexpected or incorrect behavior S-Needs-Design This issue requires design work to think about how it would best be accomplished labels May 21, 2024
@SpecificProtagonist
Copy link
Contributor Author

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.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Investigation This issue requires detective work to figure out what's going wrong labels May 21, 2024
github-merge-queue bot pushed a commit that referenced this issue May 21, 2024
# Objective

Fixes #13426

## Solution

Correct documentation to describe current behavior

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants