Skip to content

[Merged by Bors] - Add methods for silencing system-order ambiguity warnings #6158

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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