Skip to content

Commit 26fc4c7

Browse files
Vrixyzjanhohenheim
andauthored
Test for ambiguous system ordering in CI (#13950)
Progress towards #7386. Following discussion https://discord.com/channels/691052431525675048/1253260494538539048/1253387942311886960 This Pull Request adds an example to detect system order ambiguities, and also asserts none exist. A lot of schedules are ignored in ordered to have the test passing, we should thrive to make them pass, but in other pull requests. <details><summary>example output <b>summary</b>, without ignored schedules</summary> <p> ```txt $ cargo run --example ambiguity_detection 2>&1 | grep -C 1 "pairs of syst" 2024-06-21T13:17:55.776585Z WARN bevy_ecs::schedule::schedule: Schedule First has ambiguities. 1 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_time::time_system (in set TimeSystem) and bevy_ecs::event::event_update_system (in set EventUpdates) -- 2024-06-21T13:17:55.782265Z WARN bevy_ecs::schedule::schedule: Schedule PreUpdate has ambiguities. 11 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_pbr::prepass::update_mesh_previous_global_transforms and bevy_asset::server::handle_internal_asset_events -- 2024-06-21T13:17:55.809516Z WARN bevy_ecs::schedule::schedule: Schedule PostUpdate has ambiguities. 63 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_ui::accessibility::image_changed and bevy_ecs::schedule::executor::apply_deferred -- 2024-06-21T13:17:55.816287Z WARN bevy_ecs::schedule::schedule: Schedule Last has ambiguities. 3 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_gizmos::update_gizmo_meshes<bevy_gizmos::aabb::AabbGizmoConfigGroup> (in set UpdateGizmoMeshes) and bevy_gizmos::update_gizmo_meshes<bevy_gizmos::light::LightGizmoConfigGroup> (in set UpdateGizmoMeshes) -- 2024-06-21T13:17:55.831074Z WARN bevy_ecs::schedule::schedule: Schedule ExtractSchedule has ambiguities. 296 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these: -- bevy_render::extract_component::extract_components<bevy_sprite::SpriteSource> and bevy_render::render_asset::extract_render_asset<bevy_sprite::mesh2d::material::PreparedMaterial2d<bevy_sprite::mesh2d::color_material::ColorMaterial>> ``` </p> </details> To try locally: ```sh CI_TESTING_CONFIG="./.github/example-run/ambiguity_detection.ron" cargo run --example ambiguity_detection --features "bevy_ci_testing,trace,trace_chrome" ``` --------- Co-authored-by: Jan Hohenheim <[email protected]>
1 parent fa855f7 commit 26fc4c7

File tree

4 files changed

+144
-2
lines changed

4 files changed

+144
-2
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
(
2+
)

Cargo.toml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,7 @@ ron = "0.8.0"
365365
flate2 = "1.0"
366366
serde = { version = "1", features = ["derive"] }
367367
bytemuck = "1.7"
368+
bevy_render = { path = "crates/bevy_render", version = "0.15.0-dev", default-features = false }
368369
# Needed to poll Task examples
369370
futures-lite = "2.0.1"
370371
async-std = "1.12"
@@ -2980,6 +2981,14 @@ description = "Demonstrates customizing default window settings"
29802981
category = "Window"
29812982
wasm = true
29822983

2984+
[[example]]
2985+
name = "ambiguity_detection"
2986+
path = "tests/ecs/ambiguity_detection.rs"
2987+
doc-scrape-examples = true
2988+
2989+
[package.metadata.example.ambiguity_detection]
2990+
hidden = true
2991+
29832992
[[example]]
29842993
name = "resizing"
29852994
path = "tests/window/resizing.rs"

crates/bevy_dev_tools/src/ci_testing/mod.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ mod systems;
66
pub use self::config::*;
77

88
use bevy_app::prelude::*;
9+
use bevy_ecs::schedule::IntoSystemConfigs;
910
use bevy_time::TimeUpdateStrategy;
1011
use std::time::Duration;
1112

@@ -46,9 +47,11 @@ impl Plugin for CiTestingPlugin {
4647
fixed_frame_time,
4748
)));
4849
}
49-
5050
app.add_event::<CiTestingCustomEvent>()
5151
.insert_resource(config)
52-
.add_systems(Update, systems::send_events);
52+
.add_systems(
53+
Update,
54+
systems::send_events.before(bevy_window::close_when_requested),
55+
);
5356
}
5457
}

tests/ecs/ambiguity_detection.rs

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
//! A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
2+
//! This is run in CI.
3+
4+
use bevy::{
5+
ecs::schedule::{InternedScheduleLabel, LogLevel, ScheduleBuildSettings, ScheduleLabel},
6+
prelude::*,
7+
utils::HashMap,
8+
};
9+
use bevy_render::{pipelined_rendering::RenderExtractApp, Render, RenderApp};
10+
11+
/// FIXME: bevy should not have any ambiguities, but it takes time to clean these up,
12+
/// so we're just ignoring those for now.
13+
///
14+
/// See [#7386](https://github.com/bevyengine/bevy/issues/7386) for relevant issue.
15+
pub fn get_ignored_ambiguous_system_schedules() -> Vec<Box<dyn ScheduleLabel>> {
16+
vec![
17+
Box::new(First),
18+
Box::new(PreUpdate),
19+
Box::new(Update),
20+
Box::new(PostUpdate),
21+
Box::new(Last),
22+
Box::new(ExtractSchedule),
23+
Box::new(Render),
24+
]
25+
}
26+
27+
/// A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
28+
/// This is run in CI.
29+
pub fn main() {
30+
let mut app = App::new();
31+
app.add_plugins(DefaultPlugins);
32+
33+
let sub_app = app.main_mut();
34+
configure_ambiguity_detection(sub_app);
35+
let sub_app = app.sub_app_mut(RenderApp);
36+
configure_ambiguity_detection(sub_app);
37+
let sub_app = app.sub_app_mut(RenderExtractApp);
38+
configure_ambiguity_detection(sub_app);
39+
40+
app.finish();
41+
app.cleanup();
42+
app.update();
43+
44+
let sub_app = app.main();
45+
46+
let ignored_schedules = get_ignored_ambiguous_system_schedules();
47+
48+
let ambiguities = count_ambiguities(sub_app);
49+
let mut unexpected_ambiguities = vec![];
50+
for (&label, &count) in ambiguities.0.iter() {
51+
if ignored_schedules
52+
.iter()
53+
.any(|label_to_ignore| **label_to_ignore == *label)
54+
{
55+
continue;
56+
}
57+
if count != 0 {
58+
unexpected_ambiguities.push(label);
59+
}
60+
}
61+
assert_eq!(
62+
unexpected_ambiguities.len(),
63+
0,
64+
"Main app has unexpected ambiguities among these schedules: {:?}.\n\
65+
More Details:\n{:#?}",
66+
unexpected_ambiguities,
67+
ambiguities
68+
);
69+
70+
let total_ambiguities = ambiguities.total();
71+
assert_eq!(
72+
total_ambiguities, 82,
73+
"Main app does not have an expected conflicting systems count, \
74+
you might consider verifying if it's normal, or change the expected number.\n\
75+
Details:\n{:#?}",
76+
ambiguities
77+
);
78+
79+
// RenderApp is not checked here, because it is not within the App at this point.
80+
let sub_app = app.sub_app(RenderExtractApp);
81+
82+
let ambiguities = count_ambiguities(sub_app);
83+
let total_ambiguities = ambiguities.total();
84+
assert_eq!(
85+
total_ambiguities, 0,
86+
"RenderExtractApp contains conflicting systems.",
87+
);
88+
}
89+
90+
/// Contains the number of conflicting systems per schedule.
91+
#[derive(Debug, Deref, DerefMut)]
92+
struct AmbiguitiesCount(pub HashMap<InternedScheduleLabel, usize>);
93+
94+
impl AmbiguitiesCount {
95+
fn total(&self) -> usize {
96+
self.values().sum()
97+
}
98+
}
99+
100+
fn configure_ambiguity_detection(sub_app: &mut SubApp) {
101+
let ignored_ambiguous_systems = get_ignored_ambiguous_system_schedules();
102+
let mut schedules = sub_app.world_mut().resource_mut::<Schedules>();
103+
for (_, schedule) in schedules.iter_mut() {
104+
if ignored_ambiguous_systems
105+
.iter()
106+
.any(|label| **label == *schedule.label())
107+
{
108+
// Note: you can remove this bypass to get full details about ambiguities.
109+
continue;
110+
}
111+
schedule.set_build_settings(ScheduleBuildSettings {
112+
ambiguity_detection: LogLevel::Warn,
113+
use_shortnames: false,
114+
..default()
115+
});
116+
}
117+
}
118+
119+
/// Returns the number of conflicting systems per schedule.
120+
fn count_ambiguities(sub_app: &SubApp) -> AmbiguitiesCount {
121+
let schedules = sub_app.world().resource::<Schedules>();
122+
let mut ambiguities = HashMap::new();
123+
for (_, schedule) in schedules.iter() {
124+
let ambiguities_in_schedule = schedule.graph().conflicting_systems().len();
125+
ambiguities.insert(schedule.label(), ambiguities_in_schedule);
126+
}
127+
AmbiguitiesCount(ambiguities)
128+
}

0 commit comments

Comments
 (0)