mirror of
https://github.com/bevyengine/bevy
synced 2024-11-10 07:04:33 +00:00
Reduce internal system order ambiguities, and add an example explaining them (#7383)
# Objective - Bevy should not have any "internal" execution order ambiguities. These clutter the output of user-facing error reporting, and can result in nasty, nondetermistic, very difficult to solve bugs. - Verifying this currently involves repeated non-trivial manual work. ## Solution - [x] add an example to quickly check this - ~~[ ] ensure that this example panics if there are any unresolved ambiguities~~ - ~~[ ] run the example in CI 😈~~ There's one tricky ambiguity left, between UI and animation. I don't have the tools to fix this without system set configuration, so the remaining work is going to be left to #7267 or another PR after that. ``` 2023-01-27T18:38:42.989405Z INFO bevy_ecs::schedule::ambiguity_detection: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems: * Parallel systems: -- "bevy_animation::animation_player" and "bevy_ui::flex::flex_node_system" conflicts: ["bevy_transform::components::transform::Transform"] ``` ## Changelog Resolved internal execution order ambiguities for: 1. Transform propagation (ignored, we need smarter filter checking). 2. Gamepad processing (fixed). 3. bevy_winit's window handling (fixed). 4. Cascaded shadow maps and perspectives (fixed). Also fixed a desynchronized state bug that could occur when the `Window` component is removed and then added to the same entity in a single frame.
This commit is contained in:
parent
cfc56cca2f
commit
5d514fb24f
8 changed files with 134 additions and 14 deletions
10
Cargo.toml
10
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"
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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,
|
||||
|
|
|
@ -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),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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")]
|
||||
|
|
|
@ -87,14 +87,18 @@ pub struct WindowTitleCache(HashMap<Entity, String>);
|
|||
|
||||
pub(crate) fn despawn_window(
|
||||
closed: RemovedComponents<Window>,
|
||||
window_entities: Query<&Window>,
|
||||
mut close_events: EventWriter<WindowClosed>,
|
||||
mut winit_windows: NonSendMut<WinitWindows>,
|
||||
) {
|
||||
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 });
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
|
85
examples/ecs/nondeterministic_system_order.rs
Normal file
85
examples/ecs/nondeterministic_system_order.rs
Normal file
|
@ -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::<A>()
|
||||
.init_resource::<B>()
|
||||
// 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<A>) {}
|
||||
|
||||
fn writes_a(mut a: ResMut<A>) {
|
||||
a.0 += 1;
|
||||
}
|
||||
|
||||
fn adds_one_to_b(mut b: ResMut<B>) {
|
||||
b.0 = b.0.saturating_add(1);
|
||||
}
|
||||
|
||||
fn doubles_b(mut b: ResMut<B>) {
|
||||
// This will overflow pretty rapidly otherwise
|
||||
b.0 = b.0.saturating_mul(2);
|
||||
}
|
||||
|
||||
fn reads_b(b: Res<B>) {
|
||||
// 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<A>, b: Res<B>) {
|
||||
// Only display the first few steps to avoid burying the ambiguities in the console
|
||||
if b.0 < 10 {
|
||||
info!("{}, {}", a.0, b.0);
|
||||
}
|
||||
}
|
Loading…
Reference in a new issue