Skip to content

Commit

Permalink
port old ambiguity tests over (bevyengine#9617)
Browse files Browse the repository at this point in the history
# Objective

- Some of the old ambiguity tests didn't get ported over during schedule
v3.

## Solution

- Port over tests from
https://github.com/bevyengine/bevy/blob/15ee98db8d1c6705111e0f11a8fc240ceaf9f2db/crates/bevy_ecs/src/schedule/ambiguity_detection.rs#L279-L612
with minimal changes
- Make a method to convert the ambiguity conflicts to a string for
easier verification of correct results.
  • Loading branch information
hymm authored and Ray Redondo committed Jan 9, 2024
1 parent 11112ab commit 4b0d5a4
Show file tree
Hide file tree
Showing 2 changed files with 356 additions and 13 deletions.
330 changes: 330 additions & 0 deletions crates/bevy_ecs/src/schedule/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -716,4 +716,334 @@ mod tests {
assert!(matches!(result, Err(ScheduleBuildError::Ambiguity(_))));
}
}

mod system_ambiguity {
// 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
#[derive(Event)]
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 schedule = Schedule::default();
schedule
// nonsendmut system deliberately conflicts with resmut system
.add_systems((resmut_system, write_component_system, event_writer_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

#[test]
fn read_only() {
let mut world = World::new();
world.insert_resource(R);
world.spawn(A);
world.init_resource::<Events<E>>();

let mut schedule = Schedule::default();
schedule.add_systems((
empty_system,
empty_system,
res_system,
res_system,
nonsend_system,
nonsend_system,
read_component_system,
read_component_system,
event_reader_system,
event_reader_system,
read_world_system,
read_world_system,
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

#[test]
fn read_world() {
let mut world = World::new();
world.insert_resource(R);
world.spawn(A);
world.init_resource::<Events<E>>();

let mut schedule = Schedule::default();
schedule.add_systems((
resmut_system,
write_component_system,
event_writer_system,
read_world_system,
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 3);
}

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

let mut schedule = Schedule::default();
schedule.add_systems((resmut_system, res_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 1);
}

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

let mut schedule = Schedule::default();
schedule.add_systems((nonsendmut_system, nonsend_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 1);
}

#[test]
fn components() {
let mut world = World::new();
world.spawn(A);

let mut schedule = Schedule::default();
schedule.add_systems((read_component_system, write_component_system));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 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 schedule = Schedule::default();
schedule.add_systems((
with_filtered_component_system,
without_filtered_component_system,
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

#[test]
fn events() {
let mut world = World::new();
world.init_resource::<Events<E>>();

let mut schedule = Schedule::default();
schedule.add_systems((
// All of these systems clash
event_reader_system,
event_writer_system,
event_resource_system,
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 3);
}

#[test]
fn exclusive() {
let mut world = World::new();
world.insert_resource(R);
world.spawn(A);
world.init_resource::<Events<E>>();

let mut schedule = Schedule::default();
schedule.add_systems((
// All 3 of these conflict with each other
write_world_system,
write_world_system,
res_system,
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 3);
}

// Tests for silencing and resolving ambiguities
#[test]
fn before_and_after() {
let mut world = World::new();
world.init_resource::<Events<E>>();

let mut schedule = Schedule::default();
schedule.add_systems((
event_reader_system.before(event_writer_system),
event_writer_system,
event_resource_system.after(event_writer_system),
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

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

let mut schedule = Schedule::default();
schedule.add_systems((
resmut_system.ambiguous_with_all(),
res_system,
nonsend_system,
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

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

#[derive(SystemSet, Hash, PartialEq, Eq, Debug, Clone)]
struct IgnoreMe;

let mut schedule = Schedule::default();
schedule.add_systems((
resmut_system.ambiguous_with(IgnoreMe),
res_system.in_set(IgnoreMe),
nonsend_system.in_set(IgnoreMe),
));

let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

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

let mut schedule = Schedule::default();
schedule.add_systems((
write_component_system.ambiguous_with(read_component_system),
read_component_system,
));
let _ = schedule.initialize(&mut world);

assert_eq!(schedule.graph().conflicting_systems().len(), 0);
}

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

#[derive(ScheduleLabel, Hash, PartialEq, Eq, Debug, Clone)]
struct TestSchedule;

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>) {}

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

let mut schedule = Schedule::new(TestSchedule);
schedule.add_systems((
system_a,
system_b,
system_c.ambiguous_with_all(),
system_d.ambiguous_with(system_b),
system_e.after(system_a),
));

schedule.graph_mut().initialize(&mut world);
let _ = schedule
.graph_mut()
.build_schedule(world.components(), &TestSchedule.dyn_clone());

let ambiguities: Vec<_> = schedule
.graph()
.conflicts_to_string(world.components())
.collect();

let expected = &[
(
"system_d".to_string(),
"system_a".to_string(),
vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
),
(
"system_d".to_string(),
"system_e".to_string(),
vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
),
(
"system_b".to_string(),
"system_a".to_string(),
vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
),
(
"system_b".to_string(),
"system_e".to_string(),
vec!["bevy_ecs::schedule::tests::system_ambiguity::R"],
),
];

// ordering isn't stable so do this
for entry in expected {
assert!(ambiguities.contains(entry));
}
}
}
}
39 changes: 26 additions & 13 deletions crates/bevy_ecs/src/schedule/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1553,21 +1553,11 @@ impl ScheduleGraph {
Consider adding `before`, `after`, or `ambiguous_with` relationships between these:\n",
);

for (system_a, system_b, conflicts) in ambiguities {
let name_a = self.get_node_name(system_a);
let name_b = self.get_node_name(system_b);

debug_assert!(system_a.is_system(), "{name_a} is not a system.");
debug_assert!(system_b.is_system(), "{name_b} is not a system.");

for (name_a, name_b, conflicts) in self.conflicts_to_string(components) {
writeln!(message, " -- {name_a} and {name_b}").unwrap();
if !conflicts.is_empty() {
let conflict_names: Vec<_> = conflicts
.iter()
.map(|id| components.get_name(*id).unwrap())
.collect();

writeln!(message, " conflict on: {conflict_names:?}").unwrap();
if !conflicts.is_empty() {
writeln!(message, " conflict on: {conflicts:?}").unwrap();
} else {
// one or both systems must be exclusive
let world = std::any::type_name::<World>();
Expand All @@ -1578,6 +1568,29 @@ impl ScheduleGraph {
message
}

/// convert conflics to human readable format
pub fn conflicts_to_string<'a>(
&'a self,
components: &'a Components,
) -> impl Iterator<Item = (String, String, Vec<&str>)> + 'a {
self.conflicting_systems
.iter()
.map(move |(system_a, system_b, conflicts)| {
let name_a = self.get_node_name(system_a);
let name_b = self.get_node_name(system_b);

debug_assert!(system_a.is_system(), "{name_a} is not a system.");
debug_assert!(system_b.is_system(), "{name_b} is not a system.");

let conflict_names: Vec<_> = conflicts
.iter()
.map(|id| components.get_name(*id).unwrap())
.collect();

(name_a, name_b, conflict_names)
})
}

fn traverse_sets_containing_node(&self, id: NodeId, f: &mut impl FnMut(NodeId) -> bool) {
for (set_id, _, _) in self.hierarchy.graph.edges_directed(id, Direction::Incoming) {
if f(set_id) {
Expand Down

0 comments on commit 4b0d5a4

Please sign in to comment.