forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Refactor build_schedule and related errors (#9579)

Browse Source 
# Objective

- break up large build_schedule system to make it easier to read
- Clean up related error messages.
- I have a follow up PR that adds the schedule name to the error
messages, but wanted to break this up from that.

## Changelog

- refactor `build_schedule` to be easier to read

## Sample Error Messages

Dependency Cycle
```text
thread 'main' panicked at 'System dependencies contain cycle(s).
schedule has 1 before/after cycle(s):
cycle 1: system set 'A' must run before itself
system set 'A'
 ... which must run before system set 'B'
 ... which must run before system set 'A'

', crates\bevy_ecs\src\schedule\schedule.rs:228:13
```
```text
thread 'main' panicked at 'System dependencies contain cycle(s).
schedule has 1 before/after cycle(s):
cycle 1: system 'foo' must run before itself
system 'foo'
 ... which must run before system 'bar'
 ... which must run before system 'foo'

', crates\bevy_ecs\src\schedule\schedule.rs:228:13
```
Hierarchy Cycle
```text
thread 'main' panicked at 'System set hierarchy contains cycle(s).
schedule has 1 in_set cycle(s):
cycle 1: set 'A' contains itself
set 'A'
 ... which contains set 'B'
 ... which contains set 'A'

', crates\bevy_ecs\src\schedule\schedule.rs:230:13
```

System Type Set
```text
thread 'main' panicked at 'Tried to order against `SystemTypeSet(fn foo())` in a schedule that has more than one `SystemTypeSet(fn foo())` instance. `SystemTypeSet(fn foo())` is a `SystemTypeSet` and cannot be used for ordering if ambiguous. Use a different set without this restriction.', crates\bevy_ecs\src\schedule\schedule.rs:230:13
```

Hierarchy Redundancy
```text
thread 'main' panicked at 'System set hierarchy contains redundant edges.
hierarchy contains redundant edge(s) -- system set 'X' cannot be child of set 'A', longer path exists
', crates\bevy_ecs\src\schedule\schedule.rs:230:13
```

Systems have ordering but interset
```text
thread 'main' panicked at '`A` and `C` have a `before`-`after` relationship (which may be transitive) but share systems.', crates\bevy_ecs\src\schedule\schedule.rs:227:51
```

Cross Dependency
```text
thread 'main' panicked at '`A` and `B` have both `in_set` and `before`-`after` relationships (these might be transitive). This combination is unsolvable as a system cannot run before or after a set it belongs to.', crates\bevy_ecs\src\schedule\schedule.rs:230:13
```

Ambiguity
```text
thread 'main' panicked at 'Systems with conflicting access have indeterminate run order.
1 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- res_mut and res_ref
    conflict on: ["bevymark::ambiguity::X"]
', crates\bevy_ecs\src\schedule\schedule.rs:230:13
```
This commit is contained in:
Mike 2023-08-27 10:54:59 -07:00 committed by GitHub
parent 9d804a231e
commit 7b6f8659f8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 244 additions and 160 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
crates/bevy_ecs/src/schedule/mod.rs
View File

@ -541,7 +541,10 @@ mod tests {
schedule.configure_set(TestSet::B.after(TestSet::A)); schedule.configure_set(TestSet::B.after(TestSet::A));
let result = schedule.initialize(&mut world); let result = schedule.initialize(&mut world);
assert!(matches!(result, Err(ScheduleBuildError::DependencyCycle))); assert!(matches!(
result,
Err(ScheduleBuildError::DependencyCycle(_))
));
fn foo() {} fn foo() {}
fn bar() {} fn bar() {}
@ -551,7 +554,10 @@ mod tests {
schedule.add_systems((foo.after(bar), bar.after(foo))); schedule.add_systems((foo.after(bar), bar.after(foo)));
let result = schedule.initialize(&mut world); let result = schedule.initialize(&mut world);
assert!(matches!(result, Err(ScheduleBuildError::DependencyCycle))); assert!(matches!(
result,
Err(ScheduleBuildError::DependencyCycle(_))
));
} }
#[test] #[test]
@ -570,7 +576,7 @@ mod tests {
schedule.configure_set(TestSet::B.in_set(TestSet::A)); schedule.configure_set(TestSet::B.in_set(TestSet::A));
let result = schedule.initialize(&mut world); let result = schedule.initialize(&mut world);
assert!(matches!(result, Err(ScheduleBuildError::HierarchyCycle))); assert!(matches!(result, Err(ScheduleBuildError::HierarchyCycle(_))));
} }
#[test] #[test]
@ -645,7 +651,7 @@ mod tests {
let result = schedule.initialize(&mut world); let result = schedule.initialize(&mut world);
assert!(matches!( assert!(matches!(
result, result,
Err(ScheduleBuildError::HierarchyRedundancy) Err(ScheduleBuildError::HierarchyRedundancy(_))
)); ));
} }
@ -707,7 +713,7 @@ mod tests {
schedule.add_systems((res_ref, res_mut)); schedule.add_systems((res_ref, res_mut));
let result = schedule.initialize(&mut world); let result = schedule.initialize(&mut world);
assert!(matches!(result, Err(ScheduleBuildError::Ambiguity))); assert!(matches!(result, Err(ScheduleBuildError::Ambiguity(_))));
} }
} }
} }

386
crates/bevy_ecs/src/schedule/schedule.rs
View File

@ -382,9 +382,7 @@ pub struct ScheduleGraph {
uninit: Vec<(NodeId, usize)>, uninit: Vec<(NodeId, usize)>,
hierarchy: Dag, hierarchy: Dag,
dependency: Dag, dependency: Dag,
dependency_flattened: Dag,
ambiguous_with: UnGraphMap<NodeId, ()>, ambiguous_with: UnGraphMap<NodeId, ()>,
ambiguous_with_flattened: UnGraphMap<NodeId, ()>,
ambiguous_with_all: HashSet<NodeId>, ambiguous_with_all: HashSet<NodeId>,
conflicting_systems: Vec<(NodeId, NodeId, Vec<ComponentId>)>, conflicting_systems: Vec<(NodeId, NodeId, Vec<ComponentId>)>,
changed: bool, changed: bool,
@ -403,9 +401,7 @@ impl ScheduleGraph {
uninit: Vec::new(), uninit: Vec::new(),
hierarchy: Dag::new(), hierarchy: Dag::new(),
dependency: Dag::new(), dependency: Dag::new(),
dependency_flattened: Dag::new(),
ambiguous_with: UnGraphMap::new(), ambiguous_with: UnGraphMap::new(),
ambiguous_with_flattened: UnGraphMap::new(),
ambiguous_with_all: HashSet::new(), ambiguous_with_all: HashSet::new(),
conflicting_systems: Vec::new(), conflicting_systems: Vec::new(),
changed: false, changed: false,
@ -863,45 +859,70 @@ impl ScheduleGraph {
components: &Components, components: &Components,
) -> Result<SystemSchedule, ScheduleBuildError> { ) -> Result<SystemSchedule, ScheduleBuildError> {
// check hierarchy for cycles // check hierarchy for cycles
self.hierarchy.topsort = self self.hierarchy.topsort =
.topsort_graph(&self.hierarchy.graph, ReportCycles::Hierarchy) self.topsort_graph(&self.hierarchy.graph, ReportCycles::Hierarchy)?;
.map_err(|_| ScheduleBuildError::HierarchyCycle)?;
let hier_results = check_graph(&self.hierarchy.graph, &self.hierarchy.topsort); let hier_results = check_graph(&self.hierarchy.graph, &self.hierarchy.topsort);
if self.settings.hierarchy_detection != LogLevel::Ignore self.optionally_check_hierarchy_conflicts(&hier_results.transitive_edges)?;
&& self.contains_hierarchy_conflicts(&hier_results.transitive_edges)
{
self.report_hierarchy_conflicts(&hier_results.transitive_edges);
if matches!(self.settings.hierarchy_detection, LogLevel::Error) {
return Err(ScheduleBuildError::HierarchyRedundancy);
}
}
// remove redundant edges // remove redundant edges
self.hierarchy.graph = hier_results.transitive_reduction; self.hierarchy.graph = hier_results.transitive_reduction;
// check dependencies for cycles // check dependencies for cycles
self.dependency.topsort = self self.dependency.topsort =
.topsort_graph(&self.dependency.graph, ReportCycles::Dependency) self.topsort_graph(&self.dependency.graph, ReportCycles::Dependency)?;
.map_err(|_| ScheduleBuildError::DependencyCycle)?;
// check for systems or system sets depending on sets they belong to // check for systems or system sets depending on sets they belong to
let dep_results = check_graph(&self.dependency.graph, &self.dependency.topsort); let dep_results = check_graph(&self.dependency.graph, &self.dependency.topsort);
for &(a, b) in dep_results.connected.iter() { self.check_for_cross_dependencies(&dep_results, &hier_results.connected)?;
if hier_results.connected.contains(&(a, b)) || hier_results.connected.contains(&(b, a))
{
let name_a = self.get_node_name(&a);
let name_b = self.get_node_name(&b);
return Err(ScheduleBuildError::CrossDependency(name_a, name_b));
}
}
// map all system sets to their systems // map all system sets to their systems
// go in reverse topological order (bottom-up) for efficiency // go in reverse topological order (bottom-up) for efficiency
let (set_systems, set_system_bitsets) =
self.map_sets_to_systems(&self.hierarchy.topsort, &self.hierarchy.graph);
self.check_order_but_intersect(&dep_results.connected, &set_system_bitsets)?;
// check that there are no edges to system-type sets that have multiple instances
self.check_system_type_set_ambiguity(&set_systems)?;
let dependency_flattened = self.get_dependency_flattened(&set_systems);
// topsort
let mut dependency_flattened_dag = Dag {
topsort: self.topsort_graph(&dependency_flattened, ReportCycles::Dependency)?,
graph: dependency_flattened,
};
let flat_results = check_graph(
&dependency_flattened_dag.graph,
&dependency_flattened_dag.topsort,
);
// remove redundant edges
dependency_flattened_dag.graph = flat_results.transitive_reduction;
// flatten: combine `in_set` with `ambiguous_with` information
let ambiguous_with_flattened = self.get_ambiguous_with_flattened(&set_systems);
// check for conflicts
let conflicting_systems =
self.get_conflicting_systems(&flat_results.disconnected, &ambiguous_with_flattened);
self.optionally_check_conflicts(&conflicting_systems, components)?;
self.conflicting_systems = conflicting_systems;
// build the schedule
Ok(self.build_schedule_inner(dependency_flattened_dag, hier_results.reachable))
}
fn map_sets_to_systems(
&self,
hierarchy_topsort: &[NodeId],
hierarchy_graph: &GraphMap<NodeId, (), Directed>,
) -> (HashMap<NodeId, Vec<NodeId>>, HashMap<NodeId, FixedBitSet>) {
let mut set_systems: HashMap<NodeId, Vec<NodeId>> = let mut set_systems: HashMap<NodeId, Vec<NodeId>> =
HashMap::with_capacity(self.system_sets.len()); HashMap::with_capacity(self.system_sets.len());
let mut set_system_bitsets = HashMap::with_capacity(self.system_sets.len()); let mut set_system_bitsets = HashMap::with_capacity(self.system_sets.len());
for &id in self.hierarchy.topsort.iter().rev() { for &id in hierarchy_topsort.iter().rev() {
if id.is_system() { if id.is_system() {
continue; continue;
} }
@ -909,11 +930,7 @@ impl ScheduleGraph {
let mut systems = Vec::new(); let mut systems = Vec::new();
let mut system_bitset = FixedBitSet::with_capacity(self.systems.len()); let mut system_bitset = FixedBitSet::with_capacity(self.systems.len());
for child in self for child in hierarchy_graph.neighbors_directed(id, Direction::Outgoing) {
.hierarchy
.graph
.neighbors_directed(id, Direction::Outgoing)
{
match child { match child {
NodeId::System(_) => { NodeId::System(_) => {
systems.push(child); systems.push(child);
@ -931,47 +948,13 @@ impl ScheduleGraph {
set_systems.insert(id, systems); set_systems.insert(id, systems);
set_system_bitsets.insert(id, system_bitset); set_system_bitsets.insert(id, system_bitset);
} }
(set_systems, set_system_bitsets)
}
// check that there is no ordering between system sets that intersect fn get_dependency_flattened(
for (a, b) in dep_results.connected.iter() { &self,
if !(a.is_set() && b.is_set()) { set_systems: &HashMap<NodeId, Vec<NodeId>>,
continue; ) -> GraphMap<NodeId, (), Directed> {
}
let a_systems = set_system_bitsets.get(a).unwrap();
let b_systems = set_system_bitsets.get(b).unwrap();
if !(a_systems.is_disjoint(b_systems)) {
return Err(ScheduleBuildError::SetsHaveOrderButIntersect(
self.get_node_name(a),
self.get_node_name(b),
));
}
}
// check that there are no edges to system-type sets that have multiple instances
for (&id, systems) in set_systems.iter() {
let set = &self.system_sets[id.index()];
if set.is_system_type() {
let instances = systems.len();
let ambiguous_with = self.ambiguous_with.edges(id);
let before = self
.dependency
.graph
.edges_directed(id, Direction::Incoming);
let after = self
.dependency
.graph
.edges_directed(id, Direction::Outgoing);
let relations = before.count() + after.count() + ambiguous_with.count();
if instances > 1 && relations > 0 {
return Err(ScheduleBuildError::SystemTypeSetAmbiguity(
self.get_node_name(&id),
));
}
}
}
// flatten: combine `in_set` with `before` and `after` information // flatten: combine `in_set` with `before` and `after` information
// have to do it like this to preserve transitivity // have to do it like this to preserve transitivity
let mut dependency_flattened = self.dependency.graph.clone(); let mut dependency_flattened = self.dependency.graph.clone();
@ -1003,21 +986,13 @@ impl ScheduleGraph {
} }
} }
// topsort dependency_flattened
self.dependency_flattened.topsort = self }
.topsort_graph(&dependency_flattened, ReportCycles::Dependency)
.map_err(|_| ScheduleBuildError::DependencyCycle)?;
self.dependency_flattened.graph = dependency_flattened;
let flat_results = check_graph( fn get_ambiguous_with_flattened(
&self.dependency_flattened.graph, &self,
&self.dependency_flattened.topsort, set_systems: &HashMap<NodeId, Vec<NodeId>>,
); ) -> GraphMap<NodeId, (), Undirected> {
// remove redundant edges
self.dependency_flattened.graph = flat_results.transitive_reduction;
// flatten: combine `in_set` with `ambiguous_with` information
let mut ambiguous_with_flattened = UnGraphMap::new(); let mut ambiguous_with_flattened = UnGraphMap::new();
for (lhs, rhs, _) in self.ambiguous_with.all_edges() { for (lhs, rhs, _) in self.ambiguous_with.all_edges() {
match (lhs, rhs) { match (lhs, rhs) {
@ -1044,12 +1019,17 @@ impl ScheduleGraph {
} }
} }
self.ambiguous_with_flattened = ambiguous_with_flattened; ambiguous_with_flattened
}
// check for conflicts fn get_conflicting_systems(
&self,
flat_results_disconnected: &Vec<(NodeId, NodeId)>,
ambiguous_with_flattened: &GraphMap<NodeId, (), Undirected>,
) -> Vec<(NodeId, NodeId, Vec<ComponentId>)> {
let mut conflicting_systems = Vec::new(); let mut conflicting_systems = Vec::new();
for &(a, b) in &flat_results.disconnected { for &(a, b) in flat_results_disconnected {
if self.ambiguous_with_flattened.contains_edge(a, b) if ambiguous_with_flattened.contains_edge(a, b)
|| self.ambiguous_with_all.contains(&a) || self.ambiguous_with_all.contains(&a)
|| self.ambiguous_with_all.contains(&b) || self.ambiguous_with_all.contains(&b)
{ {
@ -1070,18 +1050,15 @@ impl ScheduleGraph {
} }
} }
if self.settings.ambiguity_detection != LogLevel::Ignore conflicting_systems
&& self.contains_conflicts(&conflicting_systems) }
{
self.report_conflicts(&conflicting_systems, components);
if matches!(self.settings.ambiguity_detection, LogLevel::Error) {
return Err(ScheduleBuildError::Ambiguity);
}
}
self.conflicting_systems = conflicting_systems;
// build the schedule fn build_schedule_inner(
let dg_system_ids = self.dependency_flattened.topsort.clone(); &self,
dependency_flattened_dag: Dag,
hier_results_reachable: FixedBitSet,
) -> SystemSchedule {
let dg_system_ids = dependency_flattened_dag.topsort.clone();
let dg_system_idx_map = dg_system_ids let dg_system_idx_map = dg_system_ids
.iter() .iter()
.cloned() .cloned()
@ -1120,14 +1097,12 @@ impl ScheduleGraph {
let mut system_dependencies = Vec::with_capacity(sys_count); let mut system_dependencies = Vec::with_capacity(sys_count);
let mut system_dependents = Vec::with_capacity(sys_count); let mut system_dependents = Vec::with_capacity(sys_count);
for &sys_id in &dg_system_ids { for &sys_id in &dg_system_ids {
let num_dependencies = self let num_dependencies = dependency_flattened_dag
.dependency_flattened
.graph .graph
.neighbors_directed(sys_id, Direction::Incoming) .neighbors_directed(sys_id, Direction::Incoming)
.count(); .count();
let dependents = self let dependents = dependency_flattened_dag
.dependency_flattened
.graph .graph
.neighbors_directed(sys_id, Direction::Outgoing) .neighbors_directed(sys_id, Direction::Outgoing)
.map(|dep_id| dg_system_idx_map[&dep_id]) .map(|dep_id| dg_system_idx_map[&dep_id])
@ -1145,7 +1120,7 @@ impl ScheduleGraph {
let bitset = &mut systems_in_sets_with_conditions[i]; let bitset = &mut systems_in_sets_with_conditions[i];
for &(col, sys_id) in &hg_systems { for &(col, sys_id) in &hg_systems {
let idx = dg_system_idx_map[&sys_id]; let idx = dg_system_idx_map[&sys_id];
let is_descendant = hier_results.reachable[index(row, col, hg_node_count)]; let is_descendant = hier_results_reachable[index(row, col, hg_node_count)];
bitset.set(idx, is_descendant); bitset.set(idx, is_descendant);
} }
} }
@ -1160,12 +1135,12 @@ impl ScheduleGraph {
.enumerate() .enumerate()
.take_while(|&(_idx, &row)| row < col) .take_while(|&(_idx, &row)| row < col)
{ {
let is_ancestor = hier_results.reachable[index(row, col, hg_node_count)]; let is_ancestor = hier_results_reachable[index(row, col, hg_node_count)];
bitset.set(idx, is_ancestor); bitset.set(idx, is_ancestor);
} }
} }
Ok(SystemSchedule { SystemSchedule {
systems: Vec::with_capacity(sys_count), systems: Vec::with_capacity(sys_count),
system_conditions: Vec::with_capacity(sys_count), system_conditions: Vec::with_capacity(sys_count),
set_conditions: Vec::with_capacity(set_with_conditions_count), set_conditions: Vec::with_capacity(set_with_conditions_count),
@ -1175,7 +1150,7 @@ impl ScheduleGraph {
system_dependents, system_dependents,
sets_with_conditions_of_systems, sets_with_conditions_of_systems,
systems_in_sets_with_conditions, systems_in_sets_with_conditions,
}) }
} }
fn update_schedule( fn update_schedule(
@ -1292,20 +1267,36 @@ impl ScheduleGraph {
} }
} }
fn contains_hierarchy_conflicts(&self, transitive_edges: &[(NodeId, NodeId)]) -> bool { /// If [`ScheduleBuildSettings::hierarchy_detection`] is [`LogLevel::Ignore`] this check
if transitive_edges.is_empty() { /// is skipped.
return false; fn optionally_check_hierarchy_conflicts(
&self,
transitive_edges: &[(NodeId, NodeId)],
) -> Result<(), ScheduleBuildError> {
if self.settings.hierarchy_detection == LogLevel::Ignore || transitive_edges.is_empty() {
return Ok(());
} }
true let message = self.get_hierarchy_conflicts_error_message(transitive_edges);
match self.settings.hierarchy_detection {
LogLevel::Ignore => unreachable!(),
LogLevel::Warn => {
error!("{}", message);
Ok(())
}
LogLevel::Error => Err(ScheduleBuildError::HierarchyRedundancy(message)),
}
} }
fn report_hierarchy_conflicts(&self, transitive_edges: &[(NodeId, NodeId)]) { fn get_hierarchy_conflicts_error_message(
&self,
transitive_edges: &[(NodeId, NodeId)],
) -> String {
let mut message = String::from("hierarchy contains redundant edge(s)"); let mut message = String::from("hierarchy contains redundant edge(s)");
for (parent, child) in transitive_edges { for (parent, child) in transitive_edges {
writeln!( writeln!(
message, message,
" -- {:?} '{:?}' cannot be child of set '{:?}', longer path exists", " -- {} `{}` cannot be child of set `{}`, longer path exists",
self.get_node_kind(child), self.get_node_kind(child),
self.get_node_name(child), self.get_node_name(child),
self.get_node_name(parent), self.get_node_name(parent),
@ -1313,7 +1304,7 @@ impl ScheduleGraph {
.unwrap(); .unwrap();
} }
error!("{}", message); message
} }
/// Tries to topologically sort `graph`. /// Tries to topologically sort `graph`.
@ -1329,7 +1320,7 @@ impl ScheduleGraph {
&self, &self,
graph: &DiGraphMap<NodeId, ()>, graph: &DiGraphMap<NodeId, ()>,
report: ReportCycles, report: ReportCycles,
) -> Result<Vec<NodeId>, Vec<Vec<NodeId>>> { ) -> Result<Vec<NodeId>, ScheduleBuildError> {
// Tarjan's SCC algorithm returns elements in *reverse* topological order. // Tarjan's SCC algorithm returns elements in *reverse* topological order.
let mut tarjan_scc = TarjanScc::new(); let mut tarjan_scc = TarjanScc::new();
let mut top_sorted_nodes = Vec::with_capacity(graph.node_count()); let mut top_sorted_nodes = Vec::with_capacity(graph.node_count());
@ -1355,39 +1346,43 @@ impl ScheduleGraph {
cycles.append(&mut simple_cycles_in_component(graph, scc)); cycles.append(&mut simple_cycles_in_component(graph, scc));
} }
match report { let error = match report {
ReportCycles::Hierarchy => self.report_hierarchy_cycles(&cycles), ReportCycles::Hierarchy => ScheduleBuildError::HierarchyCycle(
ReportCycles::Dependency => self.report_dependency_cycles(&cycles), self.get_hierarchy_cycles_error_message(&cycles),
} ),
ReportCycles::Dependency => ScheduleBuildError::DependencyCycle(
self.get_dependency_cycles_error_message(&cycles),
),
};
Err(sccs_with_cycles) Err(error)
} }
} }
/// Logs details of cycles in the hierarchy graph. /// Logs details of cycles in the hierarchy graph.
fn report_hierarchy_cycles(&self, cycles: &[Vec<NodeId>]) { fn get_hierarchy_cycles_error_message(&self, cycles: &[Vec<NodeId>]) -> String {
let mut message = format!("schedule has {} in_set cycle(s):\n", cycles.len()); let mut message = format!("schedule has {} in_set cycle(s):\n", cycles.len());
for (i, cycle) in cycles.iter().enumerate() { for (i, cycle) in cycles.iter().enumerate() {
let mut names = cycle.iter().map(|id| self.get_node_name(id)); let mut names = cycle.iter().map(|id| self.get_node_name(id));
let first_name = names.next().unwrap(); let first_name = names.next().unwrap();
writeln!( writeln!(
message, message,
"cycle {}: set '{first_name}' contains itself", "cycle {}: set `{first_name}` contains itself",
i + 1, i + 1,
) )
.unwrap(); .unwrap();
writeln!(message, "set '{first_name}'").unwrap(); writeln!(message, "set `{first_name}`").unwrap();
for name in names.chain(std::iter::once(first_name)) { for name in names.chain(std::iter::once(first_name)) {
writeln!(message, " ... which contains set '{name}'").unwrap(); writeln!(message, " ... which contains set `{name}`").unwrap();
} }
writeln!(message).unwrap(); writeln!(message).unwrap();
} }
error!("{}", message); message
} }
/// Logs details of cycles in the dependency graph. /// Logs details of cycles in the dependency graph.
fn report_dependency_cycles(&self, cycles: &[Vec<NodeId>]) { fn get_dependency_cycles_error_message(&self, cycles: &[Vec<NodeId>]) -> String {
let mut message = format!("schedule has {} before/after cycle(s):\n", cycles.len()); let mut message = format!("schedule has {} before/after cycle(s):\n", cycles.len());
for (i, cycle) in cycles.iter().enumerate() { for (i, cycle) in cycles.iter().enumerate() {
let mut names = cycle let mut names = cycle
@ -1396,36 +1391,119 @@ impl ScheduleGraph {
let (first_kind, first_name) = names.next().unwrap(); let (first_kind, first_name) = names.next().unwrap();
writeln!( writeln!(
message, message,
"cycle {}: {first_kind} '{first_name}' must run before itself", "cycle {}: {first_kind} `{first_name}` must run before itself",
i + 1, i + 1,
) )
.unwrap(); .unwrap();
writeln!(message, "{first_kind} '{first_name}'").unwrap(); writeln!(message, "{first_kind} `{first_name}`").unwrap();
for (kind, name) in names.chain(std::iter::once((first_kind, first_name))) { for (kind, name) in names.chain(std::iter::once((first_kind, first_name))) {
writeln!(message, " ... which must run before {kind} '{name}'").unwrap(); writeln!(message, " ... which must run before {kind} `{name}`").unwrap();
} }
writeln!(message).unwrap(); writeln!(message).unwrap();
} }
error!("{}", message); message
} }
fn contains_conflicts(&self, conflicts: &[(NodeId, NodeId, Vec<ComponentId>)]) -> bool { fn check_for_cross_dependencies(
if conflicts.is_empty() { &self,
return false; dep_results: &CheckGraphResults<NodeId>,
hier_results_connected: &HashSet<(NodeId, NodeId)>,
) -> Result<(), ScheduleBuildError> {
for &(a, b) in dep_results.connected.iter() {
if hier_results_connected.contains(&(a, b)) || hier_results_connected.contains(&(b, a))
{
let name_a = self.get_node_name(&a);
let name_b = self.get_node_name(&b);
return Err(ScheduleBuildError::CrossDependency(name_a, name_b));
}
} }
true Ok(())
} }
fn report_conflicts( fn check_order_but_intersect(
&self,
dep_results_connected: &HashSet<(NodeId, NodeId)>,
set_system_bitsets: &HashMap<NodeId, FixedBitSet>,
) -> Result<(), ScheduleBuildError> {
// check that there is no ordering between system sets that intersect
for (a, b) in dep_results_connected.iter() {
if !(a.is_set() && b.is_set()) {
continue;
}
let a_systems = set_system_bitsets.get(a).unwrap();
let b_systems = set_system_bitsets.get(b).unwrap();
if !(a_systems.is_disjoint(b_systems)) {
return Err(ScheduleBuildError::SetsHaveOrderButIntersect(
self.get_node_name(a),
self.get_node_name(b),
));
}
}
Ok(())
}
fn check_system_type_set_ambiguity(
&self,
set_systems: &HashMap<NodeId, Vec<NodeId>>,
) -> Result<(), ScheduleBuildError> {
for (&id, systems) in set_systems.iter() {
let set = &self.system_sets[id.index()];
if set.is_system_type() {
let instances = systems.len();
let ambiguous_with = self.ambiguous_with.edges(id);
let before = self
.dependency
.graph
.edges_directed(id, Direction::Incoming);
let after = self
.dependency
.graph
.edges_directed(id, Direction::Outgoing);
let relations = before.count() + after.count() + ambiguous_with.count();
if instances > 1 && relations > 0 {
return Err(ScheduleBuildError::SystemTypeSetAmbiguity(
self.get_node_name(&id),
));
}
}
}
Ok(())
}
/// if [`ScheduleBuildSettings::ambiguity_detection`] is [`LogLevel::Ignore`], this check is skipped
fn optionally_check_conflicts(
&self,
conflicts: &[(NodeId, NodeId, Vec<ComponentId>)],
components: &Components,
) -> Result<(), ScheduleBuildError> {
if self.settings.ambiguity_detection == LogLevel::Ignore || conflicts.is_empty() {
return Ok(());
}
let message = self.get_conflicts_error_message(conflicts, components);
match self.settings.ambiguity_detection {
LogLevel::Ignore => Ok(()),
LogLevel::Warn => {
warn!("{}", message);
Ok(())
}
LogLevel::Error => Err(ScheduleBuildError::Ambiguity(message)),
}
}
fn get_conflicts_error_message(
&self, &self,
ambiguities: &[(NodeId, NodeId, Vec<ComponentId>)], ambiguities: &[(NodeId, NodeId, Vec<ComponentId>)],
components: &Components, components: &Components,
) { ) -> String {
let n_ambiguities = ambiguities.len(); let n_ambiguities = ambiguities.len();
let mut string = format!( let mut message = format!(
"{n_ambiguities} pairs of systems with conflicting data access have indeterminate execution order. \ "{n_ambiguities} pairs of systems with conflicting data access have indeterminate execution order. \
Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n", Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n",
); );
@ -1437,22 +1515,22 @@ impl ScheduleGraph {
debug_assert!(system_a.is_system(), "{name_a} is not a system."); debug_assert!(system_a.is_system(), "{name_a} is not a system.");
debug_assert!(system_b.is_system(), "{name_b} is not a system."); debug_assert!(system_b.is_system(), "{name_b} is not a system.");
writeln!(string, " -- {name_a} and {name_b}").unwrap(); writeln!(message, " -- {name_a} and {name_b}").unwrap();
if !conflicts.is_empty() { if !conflicts.is_empty() {
let conflict_names: Vec<_> = conflicts let conflict_names: Vec<_> = conflicts
.iter() .iter()
.map(|id| components.get_name(*id).unwrap()) .map(|id| components.get_name(*id).unwrap())
.collect(); .collect();
writeln!(string, " conflict on: {conflict_names:?}").unwrap(); writeln!(message, " conflict on: {conflict_names:?}").unwrap();
} else { } else {
// one or both systems must be exclusive // one or both systems must be exclusive
let world = std::any::type_name::<World>(); let world = std::any::type_name::<World>();
writeln!(string, " conflict on: {world}").unwrap(); writeln!(message, " conflict on: {world}").unwrap();
} }
} }
warn!("{}", string); message
} }
fn traverse_sets_containing_node(&self, id: NodeId, f: &mut impl FnMut(NodeId) -> bool) { fn traverse_sets_containing_node(&self, id: NodeId, f: &mut impl FnMut(NodeId) -> bool) {
@ -1485,33 +1563,33 @@ pub enum ScheduleBuildError {
#[error("`{0:?}` contains itself.")] #[error("`{0:?}` contains itself.")]
HierarchyLoop(String), HierarchyLoop(String),
/// The hierarchy of system sets contains a cycle. /// The hierarchy of system sets contains a cycle.
#[error("System set hierarchy contains cycle(s).")] #[error("System set hierarchy contains cycle(s).\n{0}")]
HierarchyCycle, HierarchyCycle(String),
/// The hierarchy of system sets contains redundant edges. /// The hierarchy of system sets contains redundant edges.
/// ///
/// This error is disabled by default, but can be opted-in using [`ScheduleBuildSettings`]. /// This error is disabled by default, but can be opted-in using [`ScheduleBuildSettings`].
#[error("System set hierarchy contains redundant edges.")] #[error("System set hierarchy contains redundant edges.\n{0}")]
HierarchyRedundancy, HierarchyRedundancy(String),
/// A system (set) has been told to run before itself. /// A system (set) has been told to run before itself.
#[error("`{0:?}` depends on itself.")] #[error("`{0:?}` depends on itself.")]
DependencyLoop(String), DependencyLoop(String),
/// The dependency graph contains a cycle. /// The dependency graph contains a cycle.
#[error("System dependencies contain cycle(s).")] #[error("System dependencies contain cycle(s).\n{0}")]
DependencyCycle, DependencyCycle(String),
/// Tried to order a system (set) relative to a system set it belongs to. /// Tried to order a system (set) relative to a system set it belongs to.
#[error("`{0:?}` and `{1:?}` have both `in_set` and `before`-`after` relationships (these might be transitive). This combination is unsolvable as a system cannot run before or after a set it belongs to.")] #[error("`{0}` and `{1}` have both `in_set` and `before`-`after` relationships (these might be transitive). This combination is unsolvable as a system cannot run before or after a set it belongs to.")]
CrossDependency(String, String), CrossDependency(String, String),
/// Tried to order system sets that share systems. /// Tried to order system sets that share systems.
#[error("`{0:?}` and `{1:?}` have a `before`-`after` relationship (which may be transitive) but share systems.")] #[error("`{0}` and `{1}` have a `before`-`after` relationship (which may be transitive) but share systems.")]
SetsHaveOrderButIntersect(String, String), SetsHaveOrderButIntersect(String, String),
/// Tried to order a system (set) relative to all instances of some system function. /// Tried to order a system (set) relative to all instances of some system function.
#[error("Tried to order against `fn {0:?}` in a schedule that has more than one `{0:?}` instance. `fn {0:?}` is a `SystemTypeSet` and cannot be used for ordering if ambiguous. Use a different set without this restriction.")] #[error("Tried to order against `{0}` in a schedule that has more than one `{0}` instance. `{0}` is a `SystemTypeSet` and cannot be used for ordering if ambiguous. Use a different set without this restriction.")]
SystemTypeSetAmbiguity(String), SystemTypeSetAmbiguity(String),
/// Systems with conflicting access have indeterminate run order. /// Systems with conflicting access have indeterminate run order.
/// ///
/// This error is disabled by default, but can be opted-in using [`ScheduleBuildSettings`]. /// This error is disabled by default, but can be opted-in using [`ScheduleBuildSettings`].
#[error("Systems with conflicting access have indeterminate run order.")] #[error("Systems with conflicting access have indeterminate run order.\n{0}")]
Ambiguity, Ambiguity(String),
/// Tried to run a schedule before all of its systems have been initialized. /// Tried to run a schedule before all of its systems have been initialized.
#[error("Systems in schedule have not been initialized.")] #[error("Systems in schedule have not been initialized.")]
Uninitialized, Uninitialized,

2
crates/bevy_ecs/src/schedule/set.rs
View File

@ -71,7 +71,7 @@ impl<T: 'static> SystemTypeSet<T> {
impl<T> Debug for SystemTypeSet<T> { impl<T> Debug for SystemTypeSet<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_tuple("SystemTypeSet") f.debug_tuple("SystemTypeSet")
.field(&std::any::type_name::<T>()) .field(&format_args!("fn {}()", &std::any::type_name::<T>()))
.finish() .finish()
} }
} }