diff --git a/Cargo.toml b/Cargo.toml index 3abe9ba37a18a..dec2d5c128464 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1454,6 +1454,16 @@ description = "Shows a visualization of gamepad buttons, sticks, and triggers" category = "Tools" wasm = false +[[example]] +name = "nondeterministic_system_order" +path = "examples/ecs/nondeterministic_system_order.rs" + +[package.metadata.example.nondeterministic_system_order] +name = "Nondeterministic System Order" +description = "Systems run in paralell, but their order isn't always deteriministic. Here's how to detect and fix this." +category = "ECS (Entity Component System)" +wasm = false + [[example]] name = "3d_rotation" path = "examples/transforms/3d_rotation.rs" diff --git a/crates/bevy_input/src/lib.rs b/crates/bevy_input/src/lib.rs index fccc2027f56b4..46c7e51127adb 100644 --- a/crates/bevy_input/src/lib.rs +++ b/crates/bevy_input/src/lib.rs @@ -83,9 +83,17 @@ impl Plugin for InputPlugin { CoreStage::PreUpdate, SystemSet::new() .with_system(gamepad_event_system) - .with_system(gamepad_button_event_system.after(gamepad_event_system)) - .with_system(gamepad_axis_event_system.after(gamepad_event_system)) .with_system(gamepad_connection_system.after(gamepad_event_system)) + .with_system( + gamepad_button_event_system + .after(gamepad_event_system) + .after(gamepad_connection_system), + ) + .with_system( + gamepad_axis_event_system + .after(gamepad_event_system) + .after(gamepad_connection_system), + ) .label(InputSystem), ) // touch diff --git a/crates/bevy_pbr/src/lib.rs b/crates/bevy_pbr/src/lib.rs index 0035a9ed273aa..245f683ef62b6 100644 --- a/crates/bevy_pbr/src/lib.rs +++ b/crates/bevy_pbr/src/lib.rs @@ -194,7 +194,8 @@ impl Plugin for PbrPlugin { CoreStage::PostUpdate, update_directional_light_cascades .label(SimulationLightSystems::UpdateDirectionalLightCascades) - .after(TransformSystem::TransformPropagate), + .after(TransformSystem::TransformPropagate) + .after(CameraUpdateSystem), ) .add_system_to_stage( CoreStage::PostUpdate, diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index c68f01b2e0b5b..4aab52830c0c5 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -19,6 +19,7 @@ use bevy_app::prelude::*; use bevy_ecs::prelude::*; use bevy_hierarchy::ValidParentCheckPlugin; use prelude::{GlobalTransform, Transform}; +use systems::{propagate_transforms, sync_simple_transforms}; /// A [`Bundle`] of the [`Transform`] and [`GlobalTransform`] /// [`Component`](bevy_ecs::component::Component)s, which describe the position of an entity. @@ -101,19 +102,26 @@ impl Plugin for TransformPlugin { // add transform systems to startup so the first update is "correct" .add_startup_system_to_stage( StartupStage::PostStartup, - systems::sync_simple_transforms.label(TransformSystem::TransformPropagate), + sync_simple_transforms + .label(TransformSystem::TransformPropagate) + // FIXME: https://github.com/bevyengine/bevy/issues/4381 + // These systems cannot access the same entities, + // due to subtle query filtering that is not yet correctly computed in the ambiguity detector + .ambiguous_with(propagate_transforms), ) .add_startup_system_to_stage( StartupStage::PostStartup, - systems::propagate_transforms.label(TransformSystem::TransformPropagate), + propagate_transforms.label(TransformSystem::TransformPropagate), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::sync_simple_transforms.label(TransformSystem::TransformPropagate), + sync_simple_transforms + .label(TransformSystem::TransformPropagate) + .ambiguous_with(propagate_transforms), ) .add_system_to_stage( CoreStage::PostUpdate, - systems::propagate_transforms.label(TransformSystem::TransformPropagate), + propagate_transforms.label(TransformSystem::TransformPropagate), ); } } diff --git a/crates/bevy_winit/src/lib.rs b/crates/bevy_winit/src/lib.rs index 878ee72d4c532..544db1b6f268c 100644 --- a/crates/bevy_winit/src/lib.rs +++ b/crates/bevy_winit/src/lib.rs @@ -25,8 +25,8 @@ use bevy_utils::{ Instant, }; use bevy_window::{ - CursorEntered, CursorLeft, CursorMoved, FileDragAndDrop, Ime, ModifiesWindows, - ReceivedCharacter, RequestRedraw, Window, WindowBackendScaleFactorChanged, + exit_on_all_closed, CursorEntered, CursorLeft, CursorMoved, FileDragAndDrop, Ime, + ModifiesWindows, ReceivedCharacter, RequestRedraw, Window, WindowBackendScaleFactorChanged, WindowCloseRequested, WindowCreated, WindowFocused, WindowMoved, WindowResized, WindowScaleFactorChanged, }; @@ -55,8 +55,11 @@ impl Plugin for WinitPlugin { CoreStage::PostUpdate, SystemSet::new() .label(ModifiesWindows) - .with_system(changed_window) - .with_system(despawn_window), + // exit_on_all_closed only uses the query to determine if the query is empty, + // and so doesn't care about ordering relative to changed_window + .with_system(changed_window.ambiguous_with(exit_on_all_closed)) + // Update the state of the window before attempting to despawn to ensure consistent event ordering + .with_system(despawn_window.after(changed_window)), ); #[cfg(target_arch = "wasm32")] diff --git a/crates/bevy_winit/src/system.rs b/crates/bevy_winit/src/system.rs index 9f914ab93f0cc..708c803451a64 100644 --- a/crates/bevy_winit/src/system.rs +++ b/crates/bevy_winit/src/system.rs @@ -87,14 +87,18 @@ pub struct WindowTitleCache(HashMap); pub(crate) fn despawn_window( closed: RemovedComponents, + window_entities: Query<&Window>, mut close_events: EventWriter, mut winit_windows: NonSendMut, ) { for window in closed.iter() { info!("Closing window {:?}", window); - - winit_windows.remove_window(window); - close_events.send(WindowClosed { window }); + // Guard to verify that the window is in fact actually gone, + // rather than having the component added and removed in the same frame. + if !window_entities.contains(window) { + winit_windows.remove_window(window); + close_events.send(WindowClosed { window }); + } } } diff --git a/examples/README.md b/examples/README.md index aa8db5dae3754..11a3c64351bed 100644 --- a/examples/README.md +++ b/examples/README.md @@ -202,6 +202,7 @@ Example | Description [Generic System](../examples/ecs/generic_system.rs) | Shows how to create systems that can be reused with different types [Hierarchy](../examples/ecs/hierarchy.rs) | Creates a hierarchy of parents and children entities [Iter Combinations](../examples/ecs/iter_combinations.rs) | Shows how to iterate over combinations of query results +[Nondeterministic System Order](../examples/ecs/nondeterministic_system_order.rs) | Systems run in paralell, but their order isn't always deteriministic. Here's how to detect and fix this. [Parallel Query](../examples/ecs/parallel_query.rs) | Illustrates parallel queries with `ParallelIterator` [Removal Detection](../examples/ecs/removal_detection.rs) | Query for entities that had a specific component removed in a previous stage during the current frame [Startup System](../examples/ecs/startup_system.rs) | Demonstrates a startup system (one that runs once when the app starts up) diff --git a/examples/ecs/nondeterministic_system_order.rs b/examples/ecs/nondeterministic_system_order.rs new file mode 100644 index 0000000000000..4054b4d2f8927 --- /dev/null +++ b/examples/ecs/nondeterministic_system_order.rs @@ -0,0 +1,85 @@ +//! By default, Bevy systems run in parallel with each other. +//! Unless the order is explicitly specified, their relative order is nondeterministic. +//! +//! In many cases, this doesn't matter and is in fact desirable! +//! Consider two systems, one which writes to resource A, and the other which writes to resource B. +//! By allowing their order to be arbitrary, we can evaluate them greedily, based on the data that is free. +//! Because their data accesses are **compatible**, there is no **observable** difference created based on the order they are run. +//! +//! But if instead we have two systems mutating the same data, or one reading it and the other mutating, +//! then the actual observed value will vary based on the nondeterministic order of evaluation. +//! These observable conflicts are called **system execution order ambiguities**. +//! +//! This example demonstrates how you might detect and resolve (or silence) these ambiguities. + +use bevy::{ecs::schedule::ReportExecutionOrderAmbiguities, prelude::*}; + +fn main() { + App::new() + // This resource controls the reporting strategy for system execution order ambiguities + .insert_resource(ReportExecutionOrderAmbiguities) + .init_resource::() + .init_resource::() + // This pair of systems has an ambiguous order, + // as their data access conflicts, and there's no order between them. + .add_system(reads_a) + .add_system(writes_a) + // This pair of systems has conflicting data access, + // but it's resolved with an explicit ordering: + // the .after relationship here means that we will always double after adding. + .add_system(adds_one_to_b) + .add_system(doubles_b.after(adds_one_to_b)) + // This system isn't ambiguous with adds_one_to_b, + // due to the transitive ordering created by our constraints: + // if A is before B is before C, then A must be before C as well. + .add_system(reads_b.after(doubles_b)) + // This system will conflict with all of our writing systems + // but we've silenced its ambiguity with adds_one_to_b. + // This should only be done in the case of clear false positives: + // leave a comment in your code justifying the decision! + .add_system(reads_a_and_b.ambiguous_with(adds_one_to_b)) + // Be mindful, internal ambiguities are reported too! + // If there are any ambiguities due solely to DefaultPlugins, + // or between DefaultPlugins and any of your third party plugins, + // please file a bug with the repo responsible! + // Only *you* can prevent nondeterministic bugs due to greedy parallelism. + .add_plugins(DefaultPlugins) + .run(); +} + +#[derive(Resource, Debug, Default)] +struct A(usize); + +#[derive(Resource, Debug, Default)] +struct B(usize); + +// Data access is determined solely on the basis of the types of the system's parameters +// Every implementation of the `SystemParam` and `WorldQuery` traits must declare which data is used +// and whether or not it is mutably accessed. +fn reads_a(_a: Res) {} + +fn writes_a(mut a: ResMut) { + a.0 += 1; +} + +fn adds_one_to_b(mut b: ResMut) { + b.0 = b.0.saturating_add(1); +} + +fn doubles_b(mut b: ResMut) { + // This will overflow pretty rapidly otherwise + b.0 = b.0.saturating_mul(2); +} + +fn reads_b(b: Res) { + // This invariant is always true, + // because we've fixed the system order so doubling always occurs after adding. + assert!((b.0 % 2 == 0) || (b.0 == usize::MAX)); +} + +fn reads_a_and_b(a: Res, b: Res) { + // Only display the first few steps to avoid burying the ambiguities in the console + if b.0 < 10 { + info!("{}, {}", a.0, b.0); + } +}