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.

Sign up for GitHub

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

[Merged by Bors] - Fixed criteria-less systems being re-ran unnecessarily #1754

Closed
wants to merge 7 commits into from
Closed

[Merged by Bors] - Fixed criteria-less systems being re-ran unnecessarily #1754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
890b0dc
Fixed criteria-less systems being re-ran unnecessarily; regression test.
Ratysz Mar 25, 2021
051a7ff
Rust 2021.
Ratysz Mar 25, 2021
a426ccd
Come on, Clippy.
Ratysz Mar 25, 2021
6e7a51b
Removed superfluous stage run criteria re-run.
Ratysz Mar 25, 2021
c6fa8cf
stage loop
cart Mar 25, 2021
e903da0
fix NoAndCheckAgain
cart Mar 25, 2021
c53223c
fmt
cart Mar 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 104 additions & 84 deletions crates/bevy_ecs/src/schedule/stage.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,12 +747,6 @@ impl Stage for SystemStage {
self.world_id = Some(world.id());
}

if let ShouldRun::No | ShouldRun::NoAndCheckAgain =
self.stage_run_criteria.should_run(world)
{
return;
}

if self.systems_modified {
self.initialize_systems(world);
self.rebuild_orders_and_dependencies();
Expand All @@ -767,101 +761,127 @@ impl Stage for SystemStage {
self.executor_modified = false;
}

// Evaluate run criteria.
for index in 0..self.run_criteria.len() {
let (run_criteria, tail) = self.run_criteria.split_at_mut(index);
let mut criteria = &mut tail[0];
match &mut criteria.inner {
RunCriteriaInner::Single(system) => criteria.should_run = system.run((), world),
RunCriteriaInner::Piped {
input: parent,
system,
..
} => criteria.should_run = system.run(run_criteria[*parent].should_run, world),
let mut run_stage_loop = true;
while run_stage_loop {
let should_run = self.stage_run_criteria.should_run(world);
match should_run {
ShouldRun::No => return,
ShouldRun::NoAndCheckAgain => continue,
ShouldRun::YesAndCheckAgain => (),
ShouldRun::Yes => {
run_stage_loop = false;
}
};

// Evaluate system run criteria.
for index in 0..self.run_criteria.len() {
let (run_criteria, tail) = self.run_criteria.split_at_mut(index);
let mut criteria = &mut tail[0];
match &mut criteria.inner {
RunCriteriaInner::Single(system) => criteria.should_run = system.run((), world),
RunCriteriaInner::Piped {
input: parent,
system,
..
} => criteria.should_run = system.run(run_criteria[*parent].should_run, world),
}
}
}

let mut has_work = true;
while has_work {
fn should_run(
container: &impl SystemContainer,
run_criteria: &[RunCriteriaContainer],
) -> bool {
matches!(
container
.run_criteria()
.map(|index| run_criteria[index].should_run),
None | Some(ShouldRun::Yes) | Some(ShouldRun::YesAndCheckAgain)
)
}
let mut run_system_loop = true;
let mut default_should_run = ShouldRun::Yes;
while run_system_loop {
run_system_loop = false;

fn should_run(
container: &impl SystemContainer,
run_criteria: &[RunCriteriaContainer],
default: ShouldRun,
) -> bool {
matches!(
container
.run_criteria()
.map(|index| run_criteria[index].should_run)
.unwrap_or(default),
ShouldRun::Yes | ShouldRun::YesAndCheckAgain
)
}

// Run systems that want to be at the start of stage.
for container in &mut self.exclusive_at_start {
if should_run(container, &self.run_criteria) {
container.system_mut().run(world);
// Run systems that want to be at the start of stage.
for container in &mut self.exclusive_at_start {
if should_run(container, &self.run_criteria, default_should_run) {
container.system_mut().run(world);
}
}
}

// Run parallel systems using the executor.
// TODO: hard dependencies, nested sets, whatever... should be evaluated here.
for container in &mut self.parallel {
container.should_run = should_run(container, &self.run_criteria);
}
self.executor.run_systems(&mut self.parallel, world);
// Run parallel systems using the executor.
// TODO: hard dependencies, nested sets, whatever... should be evaluated here.
for container in &mut self.parallel {
container.should_run =
should_run(container, &self.run_criteria, default_should_run);
}
self.executor.run_systems(&mut self.parallel, world);

// Run systems that want to be between parallel systems and their command buffers.
for container in &mut self.exclusive_before_commands {
if should_run(container, &self.run_criteria) {
container.system_mut().run(world);
// Run systems that want to be between parallel systems and their command buffers.
for container in &mut self.exclusive_before_commands {
if should_run(container, &self.run_criteria, default_should_run) {
container.system_mut().run(world);
}
}
}

// Apply parallel systems' buffers.
for container in &mut self.parallel {
if container.should_run {
container.system_mut().apply_buffers(world);
// Apply parallel systems' buffers.
for container in &mut self.parallel {
if container.should_run {
container.system_mut().apply_buffers(world);
}
}
}

// Run systems that want to be at the end of stage.
for container in &mut self.exclusive_at_end {
if should_run(container, &self.run_criteria) {
container.system_mut().run(world);
// Run systems that want to be at the end of stage.
for container in &mut self.exclusive_at_end {
if should_run(container, &self.run_criteria, default_should_run) {
container.system_mut().run(world);
}
}
}

// Check for old component and system change ticks
self.check_change_ticks(world);

// Reevaluate run criteria.
has_work = false;
let run_criteria = &mut self.run_criteria;
for index in 0..run_criteria.len() {
let (run_criteria, tail) = run_criteria.split_at_mut(index);
let criteria = &mut tail[0];
match criteria.should_run {
ShouldRun::No => (),
ShouldRun::Yes => criteria.should_run = ShouldRun::No,
ShouldRun::YesAndCheckAgain | ShouldRun::NoAndCheckAgain => {
match &mut criteria.inner {
RunCriteriaInner::Single(system) => {
criteria.should_run = system.run((), world)
// Check for old component and system change ticks
self.check_change_ticks(world);

// Evaluate run criteria.
let run_criteria = &mut self.run_criteria;
for index in 0..run_criteria.len() {
let (run_criteria, tail) = run_criteria.split_at_mut(index);
let criteria = &mut tail[0];
match criteria.should_run {
ShouldRun::No => (),
ShouldRun::Yes => criteria.should_run = ShouldRun::No,
ShouldRun::YesAndCheckAgain | ShouldRun::NoAndCheckAgain => {
match &mut criteria.inner {
RunCriteriaInner::Single(system) => {
criteria.should_run = system.run((), world)
}
RunCriteriaInner::Piped {
input: parent,
system,
..
} => {
criteria.should_run =
system.run(run_criteria[*parent].should_run, world)
}
}
RunCriteriaInner::Piped {
input: parent,
system,
..
} => {
criteria.should_run =
system.run(run_criteria[*parent].should_run, world)
match criteria.should_run {
ShouldRun::Yes => {
run_system_loop = true;
}
ShouldRun::YesAndCheckAgain | ShouldRun::NoAndCheckAgain => {
run_system_loop = true;
}
ShouldRun::No => (),
}
}
match criteria.should_run {
ShouldRun::Yes | ShouldRun::YesAndCheckAgain => has_work = true,
ShouldRun::No | ShouldRun::NoAndCheckAgain => (),
}
}
}

// after the first loop, default to not running systems without run criteria
default_should_run = ShouldRun::No;
}
}
}
Expand Down
29 changes: 29 additions & 0 deletions crates/bevy_ecs/src/schedule/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,4 +652,33 @@ mod test {
&MyState::Final
);
}

#[test]
fn issue_1753() {
#[derive(Clone, PartialEq, Eq, Debug, Hash)]
enum AppState {
Main,
}

fn should_run_once(mut flag: ResMut<bool>, test_name: Res<&'static str>) {
assert!(!*flag, "{:?}", *test_name);
*flag = true;
}

let mut world = World::new();
world.insert_resource(State::new(AppState::Main));
world.insert_resource(false);
world.insert_resource("control");
let mut stage = SystemStage::parallel().with_system(should_run_once.system());
stage.run(&mut world);
assert!(*world.get_resource::<bool>().unwrap(), "after control");

world.insert_resource(false);
world.insert_resource("test");
let mut stage = SystemStage::parallel()
.with_system_set(State::<AppState>::get_driver())
.with_system(should_run_once.system());
stage.run(&mut world);
assert!(*world.get_resource::<bool>().unwrap(), "after test");
}
}