Skip to content

Commit

Permalink
Fix ambiguous_with breaking run conditions (#9253)
Browse files Browse the repository at this point in the history
# 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>
  • Loading branch information
2 people authored and cart committed Aug 10, 2023
1 parent 5b379d9 commit fefa8ea
Showing 1 changed file with 30 additions and 3 deletions.
33 changes: 30 additions & 3 deletions crates/bevy_ecs/src/schedule/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,7 @@ impl ScheduleGraph {

let sys_count = self.systems.len();
let set_with_conditions_count = hg_set_ids.len();
let node_count = self.systems.len() + self.system_sets.len();
let hg_node_count = self.hierarchy.graph.node_count();

// get the number of dependencies and the immediate dependents of each system
// (needed by multi-threaded executor to run systems in the correct order)
Expand Down Expand Up @@ -1152,7 +1152,7 @@ impl ScheduleGraph {
let bitset = &mut systems_in_sets_with_conditions[i];
for &(col, sys_id) in &hg_systems {
let idx = dg_system_idx_map[&sys_id];
let is_descendant = hier_results.reachable[index(row, col, node_count)];
let is_descendant = hier_results.reachable[index(row, col, hg_node_count)];
bitset.set(idx, is_descendant);
}
}
Expand All @@ -1167,7 +1167,7 @@ impl ScheduleGraph {
.enumerate()
.take_while(|&(_idx, &row)| row < col)
{
let is_ancestor = hier_results.reachable[index(row, col, node_count)];
let is_ancestor = hier_results.reachable[index(row, col, hg_node_count)];
bitset.set(idx, is_ancestor);
}
}
Expand Down Expand Up @@ -1577,3 +1577,30 @@ impl ScheduleBuildSettings {
}
}
}

#[cfg(test)]
mod tests {
use crate::{
self as bevy_ecs,
schedule::{IntoSystemConfigs, IntoSystemSetConfig, Schedule, SystemSet},
world::World,
};

// regression test for https://github.com/bevyengine/bevy/issues/9114
#[test]
fn ambiguous_with_not_breaking_run_conditions() {
#[derive(SystemSet, Debug, Clone, PartialEq, Eq, Hash)]
struct Set;

let mut world = World::new();
let mut schedule = Schedule::new();

schedule.configure_set(Set.run_if(|| false));
schedule.add_systems(
(|| panic!("This system must not run"))
.ambiguous_with(|| ())
.in_set(Set),
);
schedule.run(&mut world);
}
}

0 comments on commit fefa8ea

Please sign in to comment.