Skip to content

Commit

Permalink
Add methods for silencing system-order ambiguity warnings (bevyengine…
Browse files Browse the repository at this point in the history
…#6158)

# Background

Incremental implementation of bevyengine#4299. The code is heavily borrowed from that PR.

# Objective

The execution order ambiguity checker often emits false positives, since bevy is not aware of invariants upheld by the user.

## Solution

Title

---

## Changelog

+ Added methods `SystemDescriptor::ignore_all_ambiguities` and `::ambiguous_with`. These allow you to silence warnings for specific system-order ambiguities.

## Migration Guide

***Note for maintainers**: This should replace the migration guide for bevyengine#5916*

Ambiguity sets have been replaced with a simpler API.

```rust
// These systems technically conflict, but we don't care which order they run in.
fn jump_on_click(mouse: Res<Input<MouseButton>>, mut transforms: Query<&mut Transform>) { ... }
fn jump_on_spacebar(keys: Res<Input<KeyCode>>, mut transforms: Query<&mut Transform>) { ... }

//
// Before

#[derive(AmbiguitySetLabel)]
struct JumpSystems;

app
  .add_system(jump_on_click.in_ambiguity_set(JumpSystems))
  .add_system(jump_on_spacebar.in_ambiguity_set(JumpSystems));

//
// After

app
  .add_system(jump_on_click.ambiguous_with(jump_on_spacebar))
  .add_system(jump_on_spacebar);

```
  • Loading branch information
JoJoJet authored and james7132 committed Oct 28, 2022
1 parent 7d87bb8 commit 3c8c528
Show file tree
Hide file tree
Showing 3 changed files with 198 additions and 3 deletions.
138 changes: 136 additions & 2 deletions crates/bevy_ecs/src/schedule/ambiguity_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ use bevy_utils::tracing::info;
use fixedbitset::FixedBitSet;

use crate::component::ComponentId;
use crate::schedule::{SystemContainer, SystemStage};
use crate::schedule::{AmbiguityDetection, GraphNode, SystemContainer, SystemStage};
use crate::world::World;

use super::SystemLabelId;

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct SystemOrderAmbiguity {
segment: SystemStageSegment,
Expand Down Expand Up @@ -194,6 +196,24 @@ impl SystemStage {
/// along with specific components that have triggered the warning.
/// Systems must be topologically sorted beforehand.
fn find_ambiguities(systems: &[SystemContainer]) -> Vec<(usize, usize, Vec<ComponentId>)> {
// Check if we should ignore ambiguities between `system_a` and `system_b`.
fn should_ignore(system_a: &SystemContainer, system_b: &SystemContainer) -> bool {
fn should_ignore_inner(
system_a_detection: &AmbiguityDetection,
system_b_labels: &[SystemLabelId],
) -> bool {
match system_a_detection {
AmbiguityDetection::Check => false,
AmbiguityDetection::IgnoreAll => true,
AmbiguityDetection::IgnoreWithLabel(labels) => {
labels.iter().any(|l| system_b_labels.contains(l))
}
}
}
should_ignore_inner(&system_a.ambiguity_detection, system_b.labels())
|| should_ignore_inner(&system_b.ambiguity_detection, system_a.labels())
}

let mut all_dependencies = Vec::<FixedBitSet>::with_capacity(systems.len());
let mut all_dependants = Vec::<FixedBitSet>::with_capacity(systems.len());
for (index, container) in systems.iter().enumerate() {
Expand Down Expand Up @@ -235,7 +255,8 @@ fn find_ambiguities(systems: &[SystemContainer]) -> Vec<(usize, usize, Vec<Compo
for index_b in full_bitset.difference(&relations)
// .take(index_a)
{
if !processed.contains(index_b) {
if !processed.contains(index_b) && !should_ignore(&systems[index_a], &systems[index_b])
{
let system_a = &systems[index_a];
let system_b = &systems[index_b];
if system_a.is_exclusive() || system_b.is_exclusive() {
Expand Down Expand Up @@ -471,4 +492,117 @@ mod tests {

assert_eq!(test_stage.ambiguity_count(&world), 0);
}

#[test]
fn ignore_all_ambiguities() {
let mut world = World::new();
world.insert_resource(R);

let mut test_stage = SystemStage::parallel();
test_stage
.add_system(resmut_system.ignore_all_ambiguities())
.add_system(res_system)
.add_system(nonsend_system);

test_stage.run(&mut world);

assert_eq!(test_stage.ambiguity_count(&world), 0);
}

#[test]
fn ambiguous_with_label() {
let mut world = World::new();
world.insert_resource(R);

#[derive(SystemLabel)]
struct IgnoreMe;

let mut test_stage = SystemStage::parallel();
test_stage
.add_system(resmut_system.ambiguous_with(IgnoreMe))
.add_system(res_system.label(IgnoreMe))
.add_system(nonsend_system.label(IgnoreMe));

test_stage.run(&mut world);

assert_eq!(test_stage.ambiguity_count(&world), 0);
}

#[test]
fn ambiguous_with_system() {
let mut world = World::new();

let mut test_stage = SystemStage::parallel();
test_stage
.add_system(write_component_system.ambiguous_with(read_component_system))
.add_system(read_component_system);

test_stage.run(&mut world);

assert_eq!(test_stage.ambiguity_count(&world), 0);
}

fn system_a(_res: ResMut<R>) {}
fn system_b(_res: ResMut<R>) {}
fn system_c(_res: ResMut<R>) {}
fn system_d(_res: ResMut<R>) {}
fn system_e(_res: ResMut<R>) {}

// Tests that the correct ambiguities were reported in the correct order.
#[test]
fn correct_ambiguities() {
use super::*;

let mut world = World::new();
world.insert_resource(R);

let mut test_stage = SystemStage::parallel();
test_stage
.add_system(system_a)
.add_system(system_b)
.add_system(system_c.ignore_all_ambiguities())
.add_system(system_d.ambiguous_with(system_b))
.add_system(system_e.after(system_a));

test_stage.run(&mut world);

let ambiguities = test_stage.ambiguities(&world);
assert_eq!(
ambiguities,
vec![
SystemOrderAmbiguity {
system_names: [
"bevy_ecs::schedule::ambiguity_detection::tests::system_a".to_string(),
"bevy_ecs::schedule::ambiguity_detection::tests::system_b".to_string()
],
conflicts: vec!["bevy_ecs::schedule::ambiguity_detection::tests::R".to_string()],
segment: SystemStageSegment::Parallel,
},
SystemOrderAmbiguity {
system_names: [
"bevy_ecs::schedule::ambiguity_detection::tests::system_a".to_string(),
"bevy_ecs::schedule::ambiguity_detection::tests::system_d".to_string()
],
conflicts: vec!["bevy_ecs::schedule::ambiguity_detection::tests::R".to_string()],
segment: SystemStageSegment::Parallel,
},
SystemOrderAmbiguity {
system_names: [
"bevy_ecs::schedule::ambiguity_detection::tests::system_b".to_string(),
"bevy_ecs::schedule::ambiguity_detection::tests::system_e".to_string()
],
conflicts: vec!["bevy_ecs::schedule::ambiguity_detection::tests::R".to_string()],
segment: SystemStageSegment::Parallel,
},
SystemOrderAmbiguity {
system_names: [
"bevy_ecs::schedule::ambiguity_detection::tests::system_d".to_string(),
"bevy_ecs::schedule::ambiguity_detection::tests::system_e".to_string()
],
conflicts: vec!["bevy_ecs::schedule::ambiguity_detection::tests::R".to_string()],
segment: SystemStageSegment::Parallel,
},
]
);
}
}
6 changes: 5 additions & 1 deletion crates/bevy_ecs/src/schedule/system_container.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use crate::{
component::ComponentId,
query::Access,
schedule::{GraphNode, RunCriteriaLabelId, SystemDescriptor, SystemLabelId},
schedule::{
AmbiguityDetection, GraphNode, RunCriteriaLabelId, SystemDescriptor, SystemLabelId,
},
system::System,
};
use std::borrow::Cow;
Expand All @@ -16,6 +18,7 @@ pub struct SystemContainer {
labels: Vec<SystemLabelId>,
before: Vec<SystemLabelId>,
after: Vec<SystemLabelId>,
pub(crate) ambiguity_detection: AmbiguityDetection,
}

impl SystemContainer {
Expand All @@ -29,6 +32,7 @@ impl SystemContainer {
labels: descriptor.labels,
before: descriptor.before,
after: descriptor.after,
ambiguity_detection: descriptor.ambiguity_detection,
is_exclusive: descriptor.exclusive_insertion_point.is_some(),
}
}
Expand Down
57 changes: 57 additions & 0 deletions crates/bevy_ecs/src/schedule/system_descriptor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@ use crate::{
system::{AsSystemLabel, BoxedSystem, IntoSystem},
};

/// Configures ambiguity detection for a single system.
#[derive(Default)]
pub(crate) enum AmbiguityDetection {
#[default]
Check,
IgnoreAll,
/// Ignore systems with any of these labels.
IgnoreWithLabel(Vec<SystemLabelId>),
}

/// Encapsulates a system and information on when it run in a `SystemStage`.
///
/// Systems can be inserted into 4 different groups within the stage:
Expand Down Expand Up @@ -38,6 +48,7 @@ pub struct SystemDescriptor {
pub(crate) labels: Vec<SystemLabelId>,
pub(crate) before: Vec<SystemLabelId>,
pub(crate) after: Vec<SystemLabelId>,
pub(crate) ambiguity_detection: AmbiguityDetection,
}

impl SystemDescriptor {
Expand All @@ -53,6 +64,7 @@ impl SystemDescriptor {
run_criteria: None,
before: Vec::new(),
after: Vec::new(),
ambiguity_detection: Default::default(),
}
}
}
Expand All @@ -75,6 +87,15 @@ pub trait IntoSystemDescriptor<Params> {
/// Specifies that the system should run after systems with the given label.
fn after<Marker>(self, label: impl AsSystemLabel<Marker>) -> SystemDescriptor;

/// Marks this system as ambiguous with any system with the specified label.
/// This means that execution order between these systems does not matter,
/// which allows [some warnings](crate::schedule::ReportExecutionOrderAmbiguities) to be silenced.
fn ambiguous_with<Marker>(self, label: impl AsSystemLabel<Marker>) -> SystemDescriptor;

/// Specifies that this system should opt out of
/// [execution order ambiguity detection](crate::schedule::ReportExecutionOrderAmbiguities).
fn ignore_all_ambiguities(self) -> SystemDescriptor;

/// Specifies that the system should run with other exclusive systems at the start of stage.
fn at_start(self) -> SystemDescriptor;

Expand Down Expand Up @@ -110,6 +131,26 @@ impl IntoSystemDescriptor<()> for SystemDescriptor {
self
}

fn ambiguous_with<Marker>(mut self, label: impl AsSystemLabel<Marker>) -> SystemDescriptor {
match &mut self.ambiguity_detection {
detection @ AmbiguityDetection::Check => {
*detection =
AmbiguityDetection::IgnoreWithLabel(vec![label.as_system_label().as_label()]);
}
AmbiguityDetection::IgnoreWithLabel(labels) => {
labels.push(label.as_system_label().as_label());
}
// This descriptor is already ambiguous with everything.
AmbiguityDetection::IgnoreAll => {}
}
self
}

fn ignore_all_ambiguities(mut self) -> SystemDescriptor {
self.ambiguity_detection = AmbiguityDetection::IgnoreAll;
self
}

fn at_start(mut self) -> SystemDescriptor {
self.exclusive_insertion_point = Some(ExclusiveInsertionPoint::AtStart);
self
Expand Down Expand Up @@ -154,6 +195,14 @@ where
SystemDescriptor::new(Box::new(IntoSystem::into_system(self))).after(label)
}

fn ambiguous_with<Marker>(self, label: impl AsSystemLabel<Marker>) -> SystemDescriptor {
SystemDescriptor::new(Box::new(IntoSystem::into_system(self))).ambiguous_with(label)
}

fn ignore_all_ambiguities(self) -> SystemDescriptor {
SystemDescriptor::new(Box::new(IntoSystem::into_system(self))).ignore_all_ambiguities()
}

fn at_start(self) -> SystemDescriptor {
SystemDescriptor::new(Box::new(IntoSystem::into_system(self))).at_start()
}
Expand Down Expand Up @@ -191,6 +240,14 @@ impl IntoSystemDescriptor<()> for BoxedSystem<(), ()> {
SystemDescriptor::new(self).after(label)
}

fn ambiguous_with<Marker>(self, label: impl AsSystemLabel<Marker>) -> SystemDescriptor {
SystemDescriptor::new(self).ambiguous_with(label)
}

fn ignore_all_ambiguities(self) -> SystemDescriptor {
SystemDescriptor::new(self).ignore_all_ambiguities()
}

fn at_start(self) -> SystemDescriptor {
SystemDescriptor::new(self).at_start()
}
Expand Down

0 comments on commit 3c8c528

Please sign in to comment.