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

Changed/Added/Mutated queries not triggered depending on order of systems #442

Closed
mockersf opened this issue Sep 5, 2020 · 2 comments · Fixed by #451
Closed

Changed/Added/Mutated queries not triggered depending on order of systems #442

mockersf opened this issue Sep 5, 2020 · 2 comments · Fixed by #451
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior

Comments

@mockersf
Copy link
Member

mockersf commented Sep 5, 2020

using branch master at 5f1fef3:

in the following example:

use bevy::prelude::*;

fn main() {
    App::build()
        .add_default_plugins()
        .add_startup_system(setup.system())
        .add_system(spawn_a.system())
        .add_system(check_for_changed.system())
        .add_system(change_a.system())
        .run();
}

#[derive(Debug)]
struct A(u32);

fn setup(mut commands: Commands) {
    commands.spawn((A(0),));
}

fn spawn_a(mut commands: Commands) {
    let new_a = A(1);
    println!("spawning {:?}", new_a);
    commands.spawn((new_a,));
}

fn change_a(mut a: Mut<A>) {
    println!("changing {:?}", *a);
    a.0 = a.0 + 1;
}

fn check_for_changed(a: Changed<A>) {
    println!("changed {:?}", *a);
}

The check_for_changed system is only called for the component spawned in the startup system.

This was not what I expected since the documentation for Changed states that

Query transformer skipping entities that have not been either mutated or added since the last pass of the system

From the following workarounds, it seems to depend on the order of execution of the systems. The state of entities seems to be reseted near the end, and spawning does not happen when the system is executed but later.

Should the documentation be updated to specify that it only concerns changes in the current loop run, or can the Changed query actually keep track of changed since the last execution of the system where it's used?

workarounds

fn main() {
    App::build()
        .add_default_plugins()
        .add_startup_system(setup.system())
        .add_system(spawn_a.system())
        .add_system(change_a.system())
        .add_system(check_for_changed.system())
        .run();
}

If I change the order of my systems so that check_for_changed is after change_a, it will be triggered for changes but not for spawns.

fn main() {
    App::build()
        .add_default_plugins()
        .add_startup_system(setup.system())
        .add_system(spawn_a.system())
        .add_system_to_stage(bevy::app::stage::POST_UPDATE, check_for_changed.system())
        .add_system(change_a.system())
        .run();
}

If I put the system in the stage bevy::app::stage::POST_UPDATE, it now catches both spawning and changes.

@Moxinilian Moxinilian added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Sep 5, 2020
@thlorenz
Copy link

thlorenz commented Sep 5, 2020

I'm running into related issues while creating bevy examples to understand how the different aspects work.

I described the bug in detail in a comment here.

@cart
Copy link
Member

cart commented Sep 6, 2020

This is expected behavior. The current change detection system is very cheap, which is why we can enable it by default. But it comes at the cost that changes are only detected in "downstream" systems and they are reset at the end of each update.

#68 outlines the issue and links to some additional thoughts on it. the only "fix" for this is to maintain per-system change state across frames, but this would require additional allocations and would significantly increase the cost of change detection. Its worth considering adding "opt in" cross frame change detection, but I dont think we can justify the cost of making it the default.

Unfortunately the docs are slightly incorrect (as you pointed out). We should fix those (which ill consider the close-criteria for this issue).

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-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants