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

ambiguous_with breaks run conditions #9114

Closed
NiseVoid opened this issue Jul 11, 2023 · 1 comment · Fixed by #9253
Closed

ambiguous_with breaks run conditions #9114

NiseVoid opened this issue Jul 11, 2023 · 1 comment · Fixed by #9253
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this!
Milestone

Comments

@NiseVoid
Copy link
Contributor

Bevy version

0.11

What you did

I migrated my project to bevy 0.11 and noticed a system in Last started misbehaving despite having the configuration for the set it was in in all schedules.

This is the most simplified version of my code that still produces the bug

#[derive(States, Default, Clone, PartialEq, Eq, Debug, Hash)]
pub enum ConnectionState {
    #[default]
    Menu,
    Connecting,
    Connected,
}

#[derive(SystemSet, Clone, PartialEq, Eq, Debug, Hash)]
pub enum ConnectionSet {
    Menu,
    Connecting,
    Connected,
}

fn main() {
    fn configure_connecting_sets(schedule: &mut Schedule) {
        schedule.configure_set(ConnectionSet::Connected.run_if(in_state(ConnectionState::Connected)));
    }

    let mut app = App::new();

    app.add_state::<ConnectionState>();

    for label in app.world.resource::<bevy::app::MainScheduleOrder>().labels.to_vec() {
        println!("{:?}", label);
        app.edit_schedule(label.dyn_clone(), configure_connecting_sets);
    }

    app.
        .add_plugins(MinimalPlugins.build().disable::<bevy::app::ScheduleRunnerPlugin>())
        .add_plugins(bevy::asset::AssetPlugin::default())
        .add_plugins(bevy::a11y::AccessibilityPlugin)
        .add_plugins(bevy::input::InputPlugin::default())
        .add_plugins(bevy::window::WindowPlugin::default())
        .add_plugins(bevy::winit::WinitPlugin::default())

        .add_systems(Update, (|| {println!("This should not run in Update");}).in_set(ConnectionSet::Connected))
        .add_systems(Last, (|| {println!("This should not run in Last");}).in_set(ConnectionSet::Connected))

        .run();
}

What went wrong

The above code should output:

First
PreUpdate
StateTransition
RunFixedUpdateLoop
Update
PostUpdate
Last

and then stop.

Instead, if the WinitPlugin is enabled it also outputs this every frame:

This should not run in Last

Since the runcondition of the set that system is in is not met, it should not be printing this. Especially considering it only happens in Last while Update works as expected. If the WinitPlugin is disabled and the ScheduleRunnerPlugin is turned back on this problem is gone.

@NiseVoid NiseVoid added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jul 11, 2023
@tim-blackbird
Copy link
Contributor

tim-blackbird commented Jul 11, 2023

Appears to be caused by ambiguous_with, somehow.

Minimal reproduction:

use bevy::prelude::*;

#[derive(SystemSet, Debug, Clone, PartialEq, Eq, Hash)]
struct Set;

fn main() {
    App::new()
        .configure_set(Last, Set.run_if(|| false))
        .add_systems(
            Last,
            (|| println!("This should not print."))
                .ambiguous_with(|| ()) // Remove this line and it no longer prints
                .in_set(Set),
        )
        .run();
}

edit: Reduced to a truly minimal reproduction.

@Selene-Amanita Selene-Amanita added A-Windowing Platform-agnostic interface layer to run your app in P-Regression Functionality that used to work but no longer does. Add a test for this! and removed S-Needs-Triage This issue needs to be labelled labels Jul 11, 2023
@Selene-Amanita Selene-Amanita added this to the 0.11.1 milestone Jul 11, 2023
@alice-i-cecile alice-i-cecile changed the title WinitPlugin somehow breaks run conditions in Last ambiguous_with breaks run conditions Jul 22, 2023
github-merge-queue bot pushed a commit that referenced this issue Aug 3, 2023
# Objective

- Fixes #9114

## Solution

Inside `ScheduleGraph::build_schedule()` the variable `node_count =
self.systems.len() + self.system_sets.len()` is used to calculate the
indices for the `reachable` bitset derived from `self.hierarchy.graph`.
However, the number of nodes inside `self.hierarchy.graph` does not
always correspond to `self.systems.len() + self.system_sets.len()` when
`ambiguous_with` is used, because an ambiguous set is added to
`system_sets` (because we need an `NodeId` for the ambiguity graph)
without adding a node to `self.hierarchy`.

In this PR, we rename `node_count` to the more descriptive name
`hg_node_count` and set it to `self.hierarchy.graph.node_count()`.

---------

Co-authored-by: James Liu <contact@jamessliu.com>
cart pushed a commit that referenced this issue Aug 10, 2023
# Objective

- Fixes #9114

## Solution

Inside `ScheduleGraph::build_schedule()` the variable `node_count =
self.systems.len() + self.system_sets.len()` is used to calculate the
indices for the `reachable` bitset derived from `self.hierarchy.graph`.
However, the number of nodes inside `self.hierarchy.graph` does not
always correspond to `self.systems.len() + self.system_sets.len()` when
`ambiguous_with` is used, because an ambiguous set is added to
`system_sets` (because we need an `NodeId` for the ambiguity graph)
without adding a node to `self.hierarchy`.

In this PR, we rename `node_count` to the more descriptive name
`hg_node_count` and set it to `self.hierarchy.graph.node_count()`.

---------

Co-authored-by: James Liu <contact@jamessliu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants