bevy/crates/bevy_ecs/src/schedule/ambiguity_detection.rs
Carter Anderson dc3f801239 Exclusive Systems Now Implement System. Flexible Exclusive System Params (#6083)
# Objective

The [Stageless RFC](https://github.com/bevyengine/rfcs/pull/45) involves allowing exclusive systems to be referenced and ordered relative to parallel systems. We've agreed that unifying systems under `System` is the right move.

This is an alternative to #4166 (see rationale in the comments I left there). Note that this builds on the learnings established there (and borrows some patterns).

## Solution

This unifies parallel and exclusive systems under the shared `System` trait, removing the old `ExclusiveSystem` trait / impls. This is accomplished by adding a new `ExclusiveFunctionSystem` impl similar to `FunctionSystem`. It is backed by `ExclusiveSystemParam`, which is similar to `SystemParam`. There is a new flattened out SystemContainer api (which cuts out a lot of trait and type complexity). 

This means you can remove all cases of `exclusive_system()`:

```rust
// before
commands.add_system(some_system.exclusive_system());
// after
commands.add_system(some_system);
```

I've also implemented `ExclusiveSystemParam` for `&mut QueryState` and `&mut SystemState`, which makes this possible in exclusive systems:

```rust
fn some_exclusive_system(
    world: &mut World,
    transforms: &mut QueryState<&Transform>,
    state: &mut SystemState<(Res<Time>, Query<&Player>)>,
) {
    for transform in transforms.iter(world) {
        println!("{transform:?}");
    }
    let (time, players) = state.get(world);
    for player in players.iter() {
        println!("{player:?}");
    }
}
```

Note that "exclusive function systems" assume `&mut World` is present (and the first param). I think this is a fair assumption, given that the presence of `&mut World` is what defines the need for an exclusive system.

I added some targeted SystemParam `static` constraints, which removed the need for this:
``` rust
fn some_exclusive_system(state: &mut SystemState<(Res<'static, Time>, Query<&'static Player>)>) {}
```

## Related

- #2923
- #3001
- #3946

## Changelog

- `ExclusiveSystem` trait (and implementations) has been removed in favor of sharing the `System` trait.
- `ExclusiveFunctionSystem` and `ExclusiveSystemParam` were added, enabling flexible exclusive function systems
- `&mut SystemState` and `&mut QueryState` now implement `ExclusiveSystemParam`
- Exclusive and parallel System configuration is now done via a unified `SystemDescriptor`, `IntoSystemDescriptor`, and `SystemContainer` api.

## Migration Guide

Calling `.exclusive_system()` is no longer required (or supported) for converting exclusive system functions to exclusive systems:

```rust
// Old (0.8)
app.add_system(some_exclusive_system.exclusive_system());
// New (0.9)
app.add_system(some_exclusive_system);
```

Converting "normal" parallel systems to exclusive systems is done by calling the exclusive ordering apis:

```rust
// Old (0.8)
app.add_system(some_system.exclusive_system().at_end());
// New (0.9)
app.add_system(some_system.at_end());
```

Query state in exclusive systems can now be cached via ExclusiveSystemParams, which should be preferred for clarity and performance reasons:
```rust
// Old (0.8)
fn some_system(world: &mut World) {
  let mut transforms = world.query::<&Transform>();
  for transform in transforms.iter(world) {
  }
}
// New (0.9)
fn some_system(world: &mut World, transforms: &mut QueryState<&Transform>) {
  for transform in transforms.iter(world) {
  }
}
```
2022-09-26 23:57:07 +00:00

501 lines
16 KiB
Rust

use bevy_utils::tracing::info;
use fixedbitset::FixedBitSet;
use crate::component::ComponentId;
use crate::schedule::{SystemContainer, SystemStage};
use crate::world::World;
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
struct SystemOrderAmbiguity {
segment: SystemStageSegment,
// Note: In order for comparisons to work correctly,
// `system_names` and `conflicts` must be sorted at all times.
system_names: [String; 2],
conflicts: Vec<String>,
}
/// Which part of a [`SystemStage`] was a [`SystemOrderAmbiguity`] detected in?
#[derive(Debug, PartialEq, Eq, Clone, Copy, PartialOrd, Ord, Hash)]
enum SystemStageSegment {
Parallel,
ExclusiveAtStart,
ExclusiveBeforeCommands,
ExclusiveAtEnd,
}
impl SystemStageSegment {
pub fn desc(&self) -> &'static str {
match self {
SystemStageSegment::Parallel => "Parallel systems",
SystemStageSegment::ExclusiveAtStart => "Exclusive systems at start of stage",
SystemStageSegment::ExclusiveBeforeCommands => {
"Exclusive systems before commands of stage"
}
SystemStageSegment::ExclusiveAtEnd => "Exclusive systems at end of stage",
}
}
}
impl SystemOrderAmbiguity {
fn from_raw(
system_a_index: usize,
system_b_index: usize,
component_ids: Vec<ComponentId>,
segment: SystemStageSegment,
stage: &SystemStage,
world: &World,
) -> Self {
use SystemStageSegment::*;
// TODO: blocked on https://github.com/bevyengine/bevy/pull/4166
// We can't grab the system container generically, because .parallel_systems()
// and the exclusive equivalent return a different type,
// and SystemContainer is not object-safe
let (system_a_name, system_b_name) = match segment {
Parallel => {
let system_container = stage.parallel_systems();
(
system_container[system_a_index].name(),
system_container[system_b_index].name(),
)
}
ExclusiveAtStart => {
let system_container = stage.exclusive_at_start_systems();
(
system_container[system_a_index].name(),
system_container[system_b_index].name(),
)
}
ExclusiveBeforeCommands => {
let system_container = stage.exclusive_before_commands_systems();
(
system_container[system_a_index].name(),
system_container[system_b_index].name(),
)
}
ExclusiveAtEnd => {
let system_container = stage.exclusive_at_end_systems();
(
system_container[system_a_index].name(),
system_container[system_b_index].name(),
)
}
};
let mut system_names = [system_a_name.to_string(), system_b_name.to_string()];
system_names.sort();
let mut conflicts: Vec<_> = component_ids
.iter()
.map(|id| world.components().get_info(*id).unwrap().name().to_owned())
.collect();
conflicts.sort();
Self {
system_names,
conflicts,
segment,
}
}
}
impl SystemStage {
/// Logs execution order ambiguities between systems.
///
/// The output may be incorrect if this stage has not been initialized with `world`.
pub fn report_ambiguities(&self, world: &World) {
debug_assert!(!self.systems_modified);
use std::fmt::Write;
let ambiguities = self.ambiguities(world);
if !ambiguities.is_empty() {
let mut string = "Execution order ambiguities detected, you might want to \
add an explicit dependency relation between some of these systems:\n"
.to_owned();
let mut last_segment_kind = None;
for SystemOrderAmbiguity {
system_names: [system_a, system_b],
conflicts,
segment,
} in &ambiguities
{
// If the ambiguity occurred in a different segment than the previous one, write a header for the segment.
if last_segment_kind != Some(segment) {
writeln!(string, " * {}:", segment.desc()).unwrap();
last_segment_kind = Some(segment);
}
writeln!(string, " -- {:?} and {:?}", system_a, system_b).unwrap();
if !conflicts.is_empty() {
writeln!(string, " conflicts: {conflicts:?}").unwrap();
}
}
info!("{}", string);
}
}
/// Returns all execution order ambiguities between systems.
///
/// Returns 4 vectors of ambiguities for each stage, in the following order:
/// - parallel
/// - exclusive at start,
/// - exclusive before commands
/// - exclusive at end
///
/// The result may be incorrect if this stage has not been initialized with `world`.
fn ambiguities(&self, world: &World) -> Vec<SystemOrderAmbiguity> {
let parallel = find_ambiguities(&self.parallel).into_iter().map(
|(system_a_index, system_b_index, component_ids)| {
SystemOrderAmbiguity::from_raw(
system_a_index,
system_b_index,
component_ids.to_vec(),
SystemStageSegment::Parallel,
self,
world,
)
},
);
let at_start = find_ambiguities(&self.exclusive_at_start).into_iter().map(
|(system_a_index, system_b_index, component_ids)| {
SystemOrderAmbiguity::from_raw(
system_a_index,
system_b_index,
component_ids,
SystemStageSegment::ExclusiveAtStart,
self,
world,
)
},
);
let before_commands = find_ambiguities(&self.exclusive_before_commands)
.into_iter()
.map(|(system_a_index, system_b_index, component_ids)| {
SystemOrderAmbiguity::from_raw(
system_a_index,
system_b_index,
component_ids,
SystemStageSegment::ExclusiveBeforeCommands,
self,
world,
)
});
let at_end = find_ambiguities(&self.exclusive_at_end).into_iter().map(
|(system_a_index, system_b_index, component_ids)| {
SystemOrderAmbiguity::from_raw(
system_a_index,
system_b_index,
component_ids,
SystemStageSegment::ExclusiveAtEnd,
self,
world,
)
},
);
let mut ambiguities: Vec<_> = at_start
.chain(parallel)
.chain(before_commands)
.chain(at_end)
.collect();
ambiguities.sort();
ambiguities
}
/// Returns the number of system order ambiguities between systems in this stage.
///
/// The result may be incorrect if this stage has not been initialized with `world`.
#[cfg(test)]
fn ambiguity_count(&self, world: &World) -> usize {
self.ambiguities(world).len()
}
}
/// Returns vector containing all pairs of indices of systems with ambiguous execution order,
/// 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>)> {
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() {
let mut dependencies = FixedBitSet::with_capacity(systems.len());
for &dependency in container.dependencies() {
dependencies.union_with(&all_dependencies[dependency]);
dependencies.insert(dependency);
all_dependants[dependency].insert(index);
}
all_dependants.push(FixedBitSet::with_capacity(systems.len()));
all_dependencies.push(dependencies);
}
for index in (0..systems.len()).rev() {
let mut dependants = FixedBitSet::with_capacity(systems.len());
for dependant in all_dependants[index].ones() {
dependants.union_with(&all_dependants[dependant]);
dependants.insert(dependant);
}
all_dependants[index] = dependants;
}
let mut all_relations = all_dependencies
.drain(..)
.zip(all_dependants.drain(..))
.enumerate()
.map(|(index, (dependencies, dependants))| {
let mut relations = FixedBitSet::with_capacity(systems.len());
relations.union_with(&dependencies);
relations.union_with(&dependants);
relations.insert(index);
relations
})
.collect::<Vec<FixedBitSet>>();
let mut ambiguities = Vec::new();
let full_bitset: FixedBitSet = (0..systems.len()).collect();
let mut processed = FixedBitSet::with_capacity(systems.len());
for (index_a, relations) in all_relations.drain(..).enumerate() {
// TODO: prove that `.take(index_a)` would be correct here, and uncomment it if so.
for index_b in full_bitset.difference(&relations)
// .take(index_a)
{
if !processed.contains(index_b) {
let system_a = &systems[index_a];
let system_b = &systems[index_b];
if system_a.is_exclusive() || system_b.is_exclusive() {
ambiguities.push((index_a, index_b, Vec::new()));
} else {
let a_access = systems[index_a].component_access();
let b_access = systems[index_b].component_access();
let conflicts = a_access.get_conflicts(b_access);
if !conflicts.is_empty() {
ambiguities.push((index_a, index_b, conflicts));
}
}
}
}
processed.insert(index_a);
}
ambiguities
}
#[cfg(test)]
mod tests {
// Required to make the derive macro behave
use crate as bevy_ecs;
use crate::event::Events;
use crate::prelude::*;
#[derive(Resource)]
struct R;
#[derive(Component)]
struct A;
#[derive(Component)]
struct B;
// An event type
struct E;
fn empty_system() {}
fn res_system(_res: Res<R>) {}
fn resmut_system(_res: ResMut<R>) {}
fn nonsend_system(_ns: NonSend<R>) {}
fn nonsendmut_system(_ns: NonSendMut<R>) {}
fn read_component_system(_query: Query<&A>) {}
fn write_component_system(_query: Query<&mut A>) {}
fn with_filtered_component_system(_query: Query<&mut A, With<B>>) {}
fn without_filtered_component_system(_query: Query<&mut A, Without<B>>) {}
fn event_reader_system(_reader: EventReader<E>) {}
fn event_writer_system(_writer: EventWriter<E>) {}
fn event_resource_system(_events: ResMut<Events<E>>) {}
fn read_world_system(_world: &World) {}
fn write_world_system(_world: &mut World) {}
// Tests for conflict detection
#[test]
fn one_of_everything() {
let mut world = World::new();
world.insert_resource(R);
world.spawn(A);
world.init_resource::<Events<E>>();
let mut test_stage = SystemStage::parallel();
test_stage
// nonsendmut system deliberately conflicts with resmut system
.add_system(resmut_system)
.add_system(write_component_system)
.add_system(event_writer_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 0);
}
#[test]
fn read_only() {
let mut world = World::new();
world.insert_resource(R);
world.spawn(A);
world.init_resource::<Events<E>>();
let mut test_stage = SystemStage::parallel();
test_stage
.add_system(empty_system)
.add_system(empty_system)
.add_system(res_system)
.add_system(res_system)
.add_system(nonsend_system)
.add_system(nonsend_system)
.add_system(read_component_system)
.add_system(read_component_system)
.add_system(event_reader_system)
.add_system(event_reader_system)
.add_system(read_world_system)
.add_system(read_world_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 0);
}
#[test]
fn read_world() {
let mut world = World::new();
world.insert_resource(R);
world.spawn(A);
world.init_resource::<Events<E>>();
let mut test_stage = SystemStage::parallel();
test_stage
.add_system(resmut_system)
.add_system(write_component_system)
.add_system(event_writer_system)
.add_system(read_world_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 3);
}
#[test]
fn resources() {
let mut world = World::new();
world.insert_resource(R);
let mut test_stage = SystemStage::parallel();
test_stage.add_system(resmut_system).add_system(res_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 1);
}
#[test]
fn nonsend() {
let mut world = World::new();
world.insert_resource(R);
let mut test_stage = SystemStage::parallel();
test_stage
.add_system(nonsendmut_system)
.add_system(nonsend_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 1);
}
#[test]
fn components() {
let mut world = World::new();
world.spawn(A);
let mut test_stage = SystemStage::parallel();
test_stage
.add_system(read_component_system)
.add_system(write_component_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 1);
}
#[test]
#[ignore = "Known failing but fix is non-trivial: https://github.com/bevyengine/bevy/issues/4381"]
fn filtered_components() {
let mut world = World::new();
world.spawn(A);
let mut test_stage = SystemStage::parallel();
test_stage
.add_system(with_filtered_component_system)
.add_system(without_filtered_component_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 0);
}
#[test]
fn events() {
let mut world = World::new();
world.init_resource::<Events<E>>();
let mut test_stage = SystemStage::parallel();
test_stage
// All of these systems clash
.add_system(event_reader_system)
.add_system(event_writer_system)
.add_system(event_resource_system);
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 3);
}
#[test]
fn exclusive() {
let mut world = World::new();
world.insert_resource(R);
world.spawn(A);
world.init_resource::<Events<E>>();
let mut test_stage = SystemStage::parallel();
test_stage
// All 3 of these conflict with each other
.add_system(write_world_system)
.add_system(write_world_system.at_end())
.add_system(res_system.at_start())
// These do not, as they're in different segments of the stage
.add_system(write_world_system.at_start())
.add_system(write_world_system.before_commands());
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 3);
}
// Tests for silencing and resolving ambiguities
#[test]
fn before_and_after() {
let mut world = World::new();
world.init_resource::<Events<E>>();
let mut test_stage = SystemStage::parallel();
test_stage
.add_system(event_reader_system.before(event_writer_system))
.add_system(event_writer_system)
.add_system(event_resource_system.after(event_writer_system));
test_stage.run(&mut world);
assert_eq!(test_stage.ambiguity_count(&world), 0);
}
}