Skip to content

Commit

Permalink
Test for ambiguous system ordering in CI (bevyengine#13950)
Browse files Browse the repository at this point in the history
Progress towards bevyengine#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]>
  • Loading branch information
Vrixyz and janhohenheim authored Jul 17, 2024
1 parent fa855f7 commit 26fc4c7
Show file tree
Hide file tree
Showing 4 changed files with 144 additions and 2 deletions.
2 changes: 2 additions & 0 deletions .github/example-run/ambiguity_detection.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
(
)
9 changes: 9 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ ron = "0.8.0"
flate2 = "1.0"
serde = { version = "1", features = ["derive"] }
bytemuck = "1.7"
bevy_render = { path = "crates/bevy_render", version = "0.15.0-dev", default-features = false }
# Needed to poll Task examples
futures-lite = "2.0.1"
async-std = "1.12"
Expand Down Expand Up @@ -2980,6 +2981,14 @@ description = "Demonstrates customizing default window settings"
category = "Window"
wasm = true

[[example]]
name = "ambiguity_detection"
path = "tests/ecs/ambiguity_detection.rs"
doc-scrape-examples = true

[package.metadata.example.ambiguity_detection]
hidden = true

[[example]]
name = "resizing"
path = "tests/window/resizing.rs"
Expand Down
7 changes: 5 additions & 2 deletions crates/bevy_dev_tools/src/ci_testing/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mod systems;
pub use self::config::*;

use bevy_app::prelude::*;
use bevy_ecs::schedule::IntoSystemConfigs;
use bevy_time::TimeUpdateStrategy;
use std::time::Duration;

Expand Down Expand Up @@ -46,9 +47,11 @@ impl Plugin for CiTestingPlugin {
fixed_frame_time,
)));
}

app.add_event::<CiTestingCustomEvent>()
.insert_resource(config)
.add_systems(Update, systems::send_events);
.add_systems(
Update,
systems::send_events.before(bevy_window::close_when_requested),
);
}
}
128 changes: 128 additions & 0 deletions tests/ecs/ambiguity_detection.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
//! A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
//! This is run in CI.
use bevy::{
ecs::schedule::{InternedScheduleLabel, LogLevel, ScheduleBuildSettings, ScheduleLabel},
prelude::*,
utils::HashMap,
};
use bevy_render::{pipelined_rendering::RenderExtractApp, Render, RenderApp};

/// FIXME: bevy should not have any ambiguities, but it takes time to clean these up,
/// so we're just ignoring those for now.
///
/// See [#7386](https://github.com/bevyengine/bevy/issues/7386) for relevant issue.
pub fn get_ignored_ambiguous_system_schedules() -> Vec<Box<dyn ScheduleLabel>> {
vec![
Box::new(First),
Box::new(PreUpdate),
Box::new(Update),
Box::new(PostUpdate),
Box::new(Last),
Box::new(ExtractSchedule),
Box::new(Render),
]
}

/// A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
/// This is run in CI.
pub fn main() {
let mut app = App::new();
app.add_plugins(DefaultPlugins);

let sub_app = app.main_mut();
configure_ambiguity_detection(sub_app);
let sub_app = app.sub_app_mut(RenderApp);
configure_ambiguity_detection(sub_app);
let sub_app = app.sub_app_mut(RenderExtractApp);
configure_ambiguity_detection(sub_app);

app.finish();
app.cleanup();
app.update();

let sub_app = app.main();

let ignored_schedules = get_ignored_ambiguous_system_schedules();

let ambiguities = count_ambiguities(sub_app);
let mut unexpected_ambiguities = vec![];
for (&label, &count) in ambiguities.0.iter() {
if ignored_schedules
.iter()
.any(|label_to_ignore| **label_to_ignore == *label)
{
continue;
}
if count != 0 {
unexpected_ambiguities.push(label);
}
}
assert_eq!(
unexpected_ambiguities.len(),
0,
"Main app has unexpected ambiguities among these schedules: {:?}.\n\
More Details:\n{:#?}",
unexpected_ambiguities,
ambiguities
);

let total_ambiguities = ambiguities.total();
assert_eq!(
total_ambiguities, 82,
"Main app does not have an expected conflicting systems count, \
you might consider verifying if it's normal, or change the expected number.\n\
Details:\n{:#?}",
ambiguities
);

// RenderApp is not checked here, because it is not within the App at this point.
let sub_app = app.sub_app(RenderExtractApp);

let ambiguities = count_ambiguities(sub_app);
let total_ambiguities = ambiguities.total();
assert_eq!(
total_ambiguities, 0,
"RenderExtractApp contains conflicting systems.",
);
}

/// Contains the number of conflicting systems per schedule.
#[derive(Debug, Deref, DerefMut)]
struct AmbiguitiesCount(pub HashMap<InternedScheduleLabel, usize>);

impl AmbiguitiesCount {
fn total(&self) -> usize {
self.values().sum()
}
}

fn configure_ambiguity_detection(sub_app: &mut SubApp) {
let ignored_ambiguous_systems = get_ignored_ambiguous_system_schedules();
let mut schedules = sub_app.world_mut().resource_mut::<Schedules>();
for (_, schedule) in schedules.iter_mut() {
if ignored_ambiguous_systems
.iter()
.any(|label| **label == *schedule.label())
{
// Note: you can remove this bypass to get full details about ambiguities.
continue;
}
schedule.set_build_settings(ScheduleBuildSettings {
ambiguity_detection: LogLevel::Warn,
use_shortnames: false,
..default()
});
}
}

/// Returns the number of conflicting systems per schedule.
fn count_ambiguities(sub_app: &SubApp) -> AmbiguitiesCount {
let schedules = sub_app.world().resource::<Schedules>();
let mut ambiguities = HashMap::new();
for (_, schedule) in schedules.iter() {
let ambiguities_in_schedule = schedule.graph().conflicting_systems().len();
ambiguities.insert(schedule.label(), ambiguities_in_schedule);
}
AmbiguitiesCount(ambiguities)
}

0 comments on commit 26fc4c7

Please sign in to comment.