Skip to content

Commit ca3fa9d

Browse files
alice-i-cecilealice-i-cecile
committed
Move ambiguity detection into its own file (#5918)
# Objective This code is very disjoint, and the `stage.rs` file that it's in is already very long. All I've done is move the code and clean up the compiler errors that result. Followup to #5916, split out from #4299.
1 parent c96b7ff commit ca3fa9d

File tree

3 files changed

+148
-144
lines changed

3 files changed

+148
-144
lines changed

‎crates/bevy_ecs/src/schedule/ambiguity_detection.rs‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
use bevy_utils::tracing::info;
2+
use fixedbitset::FixedBitSet;
3+
4+
use crate::component::ComponentId;
5+
use crate::schedule::{SystemContainer, SystemStage};
6+
use crate::world::World;
7+
8+
impl SystemStage {
9+
/// Logs execution order ambiguities between systems. System orders must be fresh.
10+
pub fn report_ambiguities(&self, world: &World) {
11+
debug_assert!(!self.systems_modified);
12+
use std::fmt::Write;
13+
fn write_display_names_of_pairs(
14+
string: &mut String,
15+
systems: &[impl SystemContainer],
16+
mut ambiguities: Vec<(usize, usize, Vec<ComponentId>)>,
17+
world: &World,
18+
) {
19+
for (index_a, index_b, conflicts) in ambiguities.drain(..) {
20+
writeln!(
21+
string,
22+
" -- {:?} and {:?}",
23+
systems[index_a].name(),
24+
systems[index_b].name()
25+
)
26+
.unwrap();
27+
if !conflicts.is_empty() {
28+
let names = conflicts
29+
.iter()
30+
.map(|id| world.components().get_info(*id).unwrap().name())
31+
.collect::<Vec<_>>();
32+
writeln!(string, " conflicts: {:?}", names).unwrap();
33+
}
34+
}
35+
}
36+
let parallel = find_ambiguities(&self.parallel);
37+
let at_start = find_ambiguities(&self.exclusive_at_start);
38+
let before_commands = find_ambiguities(&self.exclusive_before_commands);
39+
let at_end = find_ambiguities(&self.exclusive_at_end);
40+
if !(parallel.is_empty()
41+
&& at_start.is_empty()
42+
&& before_commands.is_empty()
43+
&& at_end.is_empty())
44+
{
45+
let mut string = "Execution order ambiguities detected, you might want to \
46+
add an explicit dependency relation between some of these systems:\n"
47+
.to_owned();
48+
if !parallel.is_empty() {
49+
writeln!(string, " * Parallel systems:").unwrap();
50+
write_display_names_of_pairs(&mut string, &self.parallel, parallel, world);
51+
}
52+
if !at_start.is_empty() {
53+
writeln!(string, " * Exclusive systems at start of stage:").unwrap();
54+
write_display_names_of_pairs(
55+
&mut string,
56+
&self.exclusive_at_start,
57+
at_start,
58+
world,
59+
);
60+
}
61+
if !before_commands.is_empty() {
62+
writeln!(string, " * Exclusive systems before commands of stage:").unwrap();
63+
write_display_names_of_pairs(
64+
&mut string,
65+
&self.exclusive_before_commands,
66+
before_commands,
67+
world,
68+
);
69+
}
70+
if !at_end.is_empty() {
71+
writeln!(string, " * Exclusive systems at end of stage:").unwrap();
72+
write_display_names_of_pairs(&mut string, &self.exclusive_at_end, at_end, world);
73+
}
74+
info!("{}", string);
75+
}
76+
}
77+
}
78+
79+
/// Returns vector containing all pairs of indices of systems with ambiguous execution order,
80+
/// along with specific components that have triggered the warning.
81+
/// Systems must be topologically sorted beforehand.
82+
fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec<ComponentId>)> {
83+
let mut all_dependencies = Vec::<FixedBitSet>::with_capacity(systems.len());
84+
let mut all_dependants = Vec::<FixedBitSet>::with_capacity(systems.len());
85+
for (index, container) in systems.iter().enumerate() {
86+
let mut dependencies = FixedBitSet::with_capacity(systems.len());
87+
for &dependency in container.dependencies() {
88+
dependencies.union_with(&all_dependencies[dependency]);
89+
dependencies.insert(dependency);
90+
all_dependants[dependency].insert(index);
91+
}
92+
93+
all_dependants.push(FixedBitSet::with_capacity(systems.len()));
94+
all_dependencies.push(dependencies);
95+
}
96+
for index in (0..systems.len()).rev() {
97+
let mut dependants = FixedBitSet::with_capacity(systems.len());
98+
for dependant in all_dependants[index].ones() {
99+
dependants.union_with(&all_dependants[dependant]);
100+
dependants.insert(dependant);
101+
}
102+
all_dependants[index] = dependants;
103+
}
104+
let mut all_relations = all_dependencies
105+
.drain(..)
106+
.zip(all_dependants.drain(..))
107+
.enumerate()
108+
.map(|(index, (dependencies, dependants))| {
109+
let mut relations = FixedBitSet::with_capacity(systems.len());
110+
relations.union_with(&dependencies);
111+
relations.union_with(&dependants);
112+
relations.insert(index);
113+
relations
114+
})
115+
.collect::<Vec<FixedBitSet>>();
116+
let mut ambiguities = Vec::new();
117+
let full_bitset: FixedBitSet = (0..systems.len()).collect();
118+
let mut processed = FixedBitSet::with_capacity(systems.len());
119+
for (index_a, relations) in all_relations.drain(..).enumerate() {
120+
// TODO: prove that `.take(index_a)` would be correct here, and uncomment it if so.
121+
for index_b in full_bitset.difference(&relations)
122+
// .take(index_a)
123+
{
124+
if !processed.contains(index_b) {
125+
let a_access = systems[index_a].component_access();
126+
let b_access = systems[index_b].component_access();
127+
if let (Some(a), Some(b)) = (a_access, b_access) {
128+
let conflicts = a.get_conflicts(b);
129+
if !conflicts.is_empty() {
130+
ambiguities.push((index_a, index_b, conflicts));
131+
}
132+
} else {
133+
ambiguities.push((index_a, index_b, Vec::new()));
134+
}
135+
}
136+
}
137+
processed.insert(index_a);
138+
}
139+
ambiguities
140+
}

‎crates/bevy_ecs/src/schedule/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
//! When using Bevy ECS, systems are usually not run directly, but are inserted into a
44
//! [`Stage`], which then lives within a [`Schedule`].
55
6+
mod ambiguity_detection;
67
mod executor;
78
mod executor_parallel;
89
pub mod graph_utils;

‎crates/bevy_ecs/src/schedule/stage.rs‎

Lines changed: 7 additions & 144 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,8 @@ use crate::{
1414
world::{World, WorldId},
1515
};
1616
use bevy_ecs_macros::Resource;
17-
use bevy_utils::{
18-
tracing::{info, warn},
19-
HashMap, HashSet,
20-
};
17+
use bevy_utils::{tracing::warn, HashMap, HashSet};
2118
use downcast_rs::{impl_downcast, Downcast};
22-
use fixedbitset::FixedBitSet;
23-
use std::fmt::Debug;
2419

2520
use super::IntoSystemDescriptor;
2621

@@ -67,16 +62,16 @@ pub struct SystemStage {
6762
/// Topologically sorted run criteria of systems.
6863
run_criteria: Vec<RunCriteriaContainer>,
6964
/// Topologically sorted exclusive systems that want to be run at the start of the stage.
70-
exclusive_at_start: Vec<ExclusiveSystemContainer>,
65+
pub(super) exclusive_at_start: Vec<ExclusiveSystemContainer>,
7166
/// Topologically sorted exclusive systems that want to be run after parallel systems but
7267
/// before the application of their command buffers.
73-
exclusive_before_commands: Vec<ExclusiveSystemContainer>,
68+
pub(super) exclusive_before_commands: Vec<ExclusiveSystemContainer>,
7469
/// Topologically sorted exclusive systems that want to be run at the end of the stage.
75-
exclusive_at_end: Vec<ExclusiveSystemContainer>,
70+
pub(super) exclusive_at_end: Vec<ExclusiveSystemContainer>,
7671
/// Topologically sorted parallel systems.
77-
parallel: Vec<ParallelSystemContainer>,
72+
pub(super) parallel: Vec<ParallelSystemContainer>,
7873
/// Determines if the stage was modified and needs to rebuild its graphs and orders.
79-
systems_modified: bool,
74+
pub(super) systems_modified: bool,
8075
/// Determines if the stage's executor was changed.
8176
executor_modified: bool,
8277
/// Newly inserted run criteria that will be initialized at the next opportunity.
@@ -453,7 +448,7 @@ impl SystemStage {
453448
&& self.uninitialized_before_commands.is_empty()
454449
&& self.uninitialized_at_end.is_empty()
455450
);
456-
fn unwrap_dependency_cycle_error<Node: GraphNode, Output, Labels: Debug>(
451+
fn unwrap_dependency_cycle_error<Node: GraphNode, Output, Labels: std::fmt::Debug>(
457452
result: Result<Output, DependencyGraphError<Labels>>,
458453
nodes: &[Node],
459454
nodes_description: &'static str,
@@ -505,75 +500,6 @@ impl SystemStage {
505500
);
506501
}
507502

508-
/// Logs execution order ambiguities between systems. System orders must be fresh.
509-
fn report_ambiguities(&self, world: &World) {
510-
debug_assert!(!self.systems_modified);
511-
use std::fmt::Write;
512-
fn write_display_names_of_pairs(
513-
string: &mut String,
514-
systems: &[impl SystemContainer],
515-
mut ambiguities: Vec<(usize, usize, Vec<ComponentId>)>,
516-
world: &World,
517-
) {
518-
for (index_a, index_b, conflicts) in ambiguities.drain(..) {
519-
writeln!(
520-
string,
521-
" -- {:?} and {:?}",
522-
systems[index_a].name(),
523-
systems[index_b].name()
524-
)
525-
.unwrap();
526-
if !conflicts.is_empty() {
527-
let names = conflicts
528-
.iter()
529-
.map(|id| world.components().get_info(*id).unwrap().name())
530-
.collect::<Vec<_>>();
531-
writeln!(string, " conflicts: {:?}", names).unwrap();
532-
}
533-
}
534-
}
535-
let parallel = find_ambiguities(&self.parallel);
536-
let at_start = find_ambiguities(&self.exclusive_at_start);
537-
let before_commands = find_ambiguities(&self.exclusive_before_commands);
538-
let at_end = find_ambiguities(&self.exclusive_at_end);
539-
if !(parallel.is_empty()
540-
&& at_start.is_empty()
541-
&& before_commands.is_empty()
542-
&& at_end.is_empty())
543-
{
544-
let mut string = "Execution order ambiguities detected, you might want to \
545-
add an explicit dependency relation between some of these systems:\n"
546-
.to_owned();
547-
if !parallel.is_empty() {
548-
writeln!(string, " * Parallel systems:").unwrap();
549-
write_display_names_of_pairs(&mut string, &self.parallel, parallel, world);
550-
}
551-
if !at_start.is_empty() {
552-
writeln!(string, " * Exclusive systems at start of stage:").unwrap();
553-
write_display_names_of_pairs(
554-
&mut string,
555-
&self.exclusive_at_start,
556-
at_start,
557-
world,
558-
);
559-
}
560-
if !before_commands.is_empty() {
561-
writeln!(string, " * Exclusive systems before commands of stage:").unwrap();
562-
write_display_names_of_pairs(
563-
&mut string,
564-
&self.exclusive_before_commands,
565-
before_commands,
566-
world,
567-
);
568-
}
569-
if !at_end.is_empty() {
570-
writeln!(string, " * Exclusive systems at end of stage:").unwrap();
571-
write_display_names_of_pairs(&mut string, &self.exclusive_at_end, at_end, world);
572-
}
573-
info!("{}", string);
574-
}
575-
}
576-
577503
fn check_uses_resource(&self, resource_id: ComponentId, world: &World) {
578504
debug_assert!(!self.systems_modified);
579505
for system in &self.parallel {
@@ -709,69 +635,6 @@ fn process_systems(
709635
Ok(())
710636
}
711637

712-
/// Returns vector containing all pairs of indices of systems with ambiguous execution order,
713-
/// along with specific components that have triggered the warning.
714-
/// Systems must be topologically sorted beforehand.
715-
fn find_ambiguities(systems: &[impl SystemContainer]) -> Vec<(usize, usize, Vec<ComponentId>)> {
716-
let mut all_dependencies = Vec::<FixedBitSet>::with_capacity(systems.len());
717-
let mut all_dependants = Vec::<FixedBitSet>::with_capacity(systems.len());
718-
for (index, container) in systems.iter().enumerate() {
719-
let mut dependencies = FixedBitSet::with_capacity(systems.len());
720-
for &dependency in container.dependencies() {
721-
dependencies.union_with(&all_dependencies[dependency]);
722-
dependencies.insert(dependency);
723-
all_dependants[dependency].insert(index);
724-
}
725-
726-
all_dependants.push(FixedBitSet::with_capacity(systems.len()));
727-
all_dependencies.push(dependencies);
728-
}
729-
for index in (0..systems.len()).rev() {
730-
let mut dependants = FixedBitSet::with_capacity(systems.len());
731-
for dependant in all_dependants[index].ones() {
732-
dependants.union_with(&all_dependants[dependant]);
733-
dependants.insert(dependant);
734-
}
735-
all_dependants[index] = dependants;
736-
}
737-
let mut all_relations = all_dependencies
738-
.drain(..)
739-
.zip(all_dependants.drain(..))
740-
.enumerate()
741-
.map(|(index, (dependencies, dependants))| {
742-
let mut relations = FixedBitSet::with_capacity(systems.len());
743-
relations.union_with(&dependencies);
744-
relations.union_with(&dependants);
745-
relations.insert(index);
746-
relations
747-
})
748-
.collect::<Vec<FixedBitSet>>();
749-
let mut ambiguities = Vec::new();
750-
let full_bitset: FixedBitSet = (0..systems.len()).collect();
751-
let mut processed = FixedBitSet::with_capacity(systems.len());
752-
for (index_a, relations) in all_relations.drain(..).enumerate() {
753-
// TODO: prove that `.take(index_a)` would be correct here, and uncomment it if so.
754-
for index_b in full_bitset.difference(&relations)
755-
// .take(index_a)
756-
{
757-
if !processed.contains(index_b) {
758-
let a_access = systems[index_a].component_access();
759-
let b_access = systems[index_b].component_access();
760-
if let (Some(a), Some(b)) = (a_access, b_access) {
761-
let conflicts = a.get_conflicts(b);
762-
if !conflicts.is_empty() {
763-
ambiguities.push((index_a, index_b, conflicts));
764-
}
765-
} else {
766-
ambiguities.push((index_a, index_b, Vec::new()));
767-
}
768-
}
769-
}
770-
processed.insert(index_a);
771-
}
772-
ambiguities
773-
}
774-
775638
impl Stage for SystemStage {
776639
fn run(&mut self, world: &mut World) {
777640
if let Some(world_id) = self.world_id {

0 commit comments

Comments
 (0)