From 4ac2a635569756d44dc237d812219700ff74f40c Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 3 Sep 2024 16:24:34 -0400 Subject: [PATCH] Remove all existing system order ambiguities in `DefaultPlugins` (#15031) # Objective As discussed in https://github.com/bevyengine/bevy/issues/7386, system order ambiguities within `DefaultPlugins` are a source of bugs in the engine and badly pollute diagnostic output for users. We should eliminate them! This PR is an alternative to #15027: with all external ambiguities silenced, this should be much less prone to merge conflicts and the test output should be much easier for authors to understand. Note that system order ambiguities are still permitted in the `RenderApp`: these need a bit of thought in terms of how to test them, and will be fairly involved to fix. While these aren't *good*, they'll generally only cause graphical bugs, not logic ones. ## Solution All remaining system order ambiguities have been resolved. Review this PR commit-by-commit to see how each of these problems were fixed. ## Testing `cargo run --example ambiguity_detection` passes with no panics or logging! --- crates/bevy_animation/src/lib.rs | 2 +- .../bevy_app/src/terminal_ctrl_c_handler.rs | 2 +- crates/bevy_asset/src/lib.rs | 6 ++++- crates/bevy_dev_tools/src/ci_testing/mod.rs | 16 +++++++++++-- crates/bevy_gizmos/src/aabb.rs | 1 + crates/bevy_gizmos/src/lib.rs | 3 +++ crates/bevy_internal/src/default_plugins.rs | 5 ++++ crates/bevy_pbr/src/lib.rs | 14 ++++++++++- crates/bevy_pbr/src/light/mod.rs | 7 ++++++ crates/bevy_picking/src/lib.rs | 2 -- crates/bevy_render/src/view/visibility/mod.rs | 7 +++++- crates/bevy_ui/src/lib.rs | 7 ++++-- tests/ecs/ambiguity_detection.rs | 24 +++++++++---------- 13 files changed, 73 insertions(+), 23 deletions(-) diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs index e3ba7836f7..83dbded00d 100755 --- a/crates/bevy_animation/src/lib.rs +++ b/crates/bevy_animation/src/lib.rs @@ -1234,7 +1234,7 @@ impl Plugin for AnimationPlugin { ( advance_transitions, advance_animations, - animate_targets, + animate_targets.after(bevy_render::mesh::morph::inherit_weights), expire_completed_transitions, ) .chain() diff --git a/crates/bevy_app/src/terminal_ctrl_c_handler.rs b/crates/bevy_app/src/terminal_ctrl_c_handler.rs index 54e0bc5338..8bd90ffaa6 100644 --- a/crates/bevy_app/src/terminal_ctrl_c_handler.rs +++ b/crates/bevy_app/src/terminal_ctrl_c_handler.rs @@ -48,7 +48,7 @@ impl TerminalCtrlCHandlerPlugin { } /// Sends a [`AppExit`] event when the user presses `Ctrl+C` on the terminal. - fn exit_on_flag(mut events: EventWriter) { + pub fn exit_on_flag(mut events: EventWriter) { if SHOULD_EXIT.load(Ordering::Relaxed) { events.send(AppExit::from_code(130)); } diff --git a/crates/bevy_asset/src/lib.rs b/crates/bevy_asset/src/lib.rs index 6c790b2b47..d49bae9e63 100644 --- a/crates/bevy_asset/src/lib.rs +++ b/crates/bevy_asset/src/lib.rs @@ -222,7 +222,11 @@ impl Plugin for AssetPlugin { .init_asset::<()>() .add_event::() .configure_sets(PreUpdate, TrackAssets.after(handle_internal_asset_events)) - .add_systems(PreUpdate, handle_internal_asset_events) + // `handle_internal_asset_events` requires the use of `&mut World`, + // and as a result has ambiguous system ordering with all other systems in `PreUpdate`. + // This is virtually never a real problem: asset loading is async and so anything that interacts directly with it + // needs to be robust to stochastic delays anyways. + .add_systems(PreUpdate, handle_internal_asset_events.ambiguous_with_all()) .register_type::(); } } diff --git a/crates/bevy_dev_tools/src/ci_testing/mod.rs b/crates/bevy_dev_tools/src/ci_testing/mod.rs index 59949fa0bf..d6d5c6d4be 100644 --- a/crates/bevy_dev_tools/src/ci_testing/mod.rs +++ b/crates/bevy_dev_tools/src/ci_testing/mod.rs @@ -6,7 +6,7 @@ mod systems; pub use self::config::*; use bevy_app::prelude::*; -use bevy_ecs::schedule::IntoSystemConfigs; +use bevy_ecs::prelude::*; use bevy_render::view::screenshot::trigger_screenshots; use bevy_time::TimeUpdateStrategy; use std::time::Duration; @@ -54,7 +54,19 @@ impl Plugin for CiTestingPlugin { Update, systems::send_events .before(trigger_screenshots) - .before(bevy_window::close_when_requested), + .before(bevy_window::close_when_requested) + .in_set(SendEvents), ); + + // The offending system does not exist in the wasm32 target. + // As a result, we must conditionally order the two systems using a system set. + #[cfg(not(target_arch = "wasm32"))] + app.configure_sets( + Update, + SendEvents.before(bevy_app::TerminalCtrlCHandlerPlugin::exit_on_flag), + ); } } + +#[derive(SystemSet, Debug, Clone, PartialEq, Eq, Hash)] +struct SendEvents; diff --git a/crates/bevy_gizmos/src/aabb.rs b/crates/bevy_gizmos/src/aabb.rs index 1b62775245..6496dc9125 100644 --- a/crates/bevy_gizmos/src/aabb.rs +++ b/crates/bevy_gizmos/src/aabb.rs @@ -40,6 +40,7 @@ impl Plugin for AabbGizmoPlugin { config.config::().1.draw_all }), ) + .after(bevy_render::view::VisibilitySystems::CalculateBounds) .after(TransformSystem::TransformPropagate), ); } diff --git a/crates/bevy_gizmos/src/lib.rs b/crates/bevy_gizmos/src/lib.rs index c30d0b0931..10f7e9cdc9 100644 --- a/crates/bevy_gizmos/src/lib.rs +++ b/crates/bevy_gizmos/src/lib.rs @@ -242,6 +242,9 @@ impl AppGizmoBuilder for App { handles.list.insert(TypeId::of::(), None); handles.strip.insert(TypeId::of::(), None); + // These handles are safe to mutate in any order + self.allow_ambiguous_resource::(); + self.init_resource::>() .init_resource::>() .init_resource::>>() diff --git a/crates/bevy_internal/src/default_plugins.rs b/crates/bevy_internal/src/default_plugins.rs index a84bbdb573..7aadcb50fb 100644 --- a/crates/bevy_internal/src/default_plugins.rs +++ b/crates/bevy_internal/src/default_plugins.rs @@ -90,6 +90,11 @@ impl Plugin for IgnoreAmbiguitiesPlugin { bevy_animation::advance_animations, bevy_ui::ui_layout_system, ); + app.ignore_ambiguity( + bevy_app::PostUpdate, + bevy_animation::animate_targets, + bevy_ui::ui_layout_system, + ); } } } diff --git a/crates/bevy_pbr/src/lib.rs b/crates/bevy_pbr/src/lib.rs index ea84c49f18..5deaa88590 100644 --- a/crates/bevy_pbr/src/lib.rs +++ b/crates/bevy_pbr/src/lib.rs @@ -340,10 +340,22 @@ impl Plugin for PbrPlugin { ) .chain(), ) + .configure_sets( + PostUpdate, + SimulationLightSystems::UpdateDirectionalLightCascades + .ambiguous_with(SimulationLightSystems::UpdateDirectionalLightCascades), + ) + .configure_sets( + PostUpdate, + SimulationLightSystems::CheckLightVisibility + .ambiguous_with(SimulationLightSystems::CheckLightVisibility), + ) .add_systems( PostUpdate, ( - add_clusters.in_set(SimulationLightSystems::AddClusters), + add_clusters + .in_set(SimulationLightSystems::AddClusters) + .after(CameraUpdateSystem), assign_objects_to_clusters .in_set(SimulationLightSystems::AssignLightsToClusters) .after(TransformSystem::TransformPropagate) diff --git a/crates/bevy_pbr/src/light/mod.rs b/crates/bevy_pbr/src/light/mod.rs index d895ac7672..d0db103c93 100644 --- a/crates/bevy_pbr/src/light/mod.rs +++ b/crates/bevy_pbr/src/light/mod.rs @@ -496,12 +496,19 @@ pub enum ShadowFilteringMethod { Temporal, } +/// System sets used to run light-related systems. #[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)] pub enum SimulationLightSystems { AddClusters, AssignLightsToClusters, + /// System order ambiguities between systems in this set are ignored: + /// each [`build_directional_light_cascades`] system is independent of the others, + /// and should operate on distinct sets of entities. UpdateDirectionalLightCascades, UpdateLightFrusta, + /// System order ambiguities between systems in this set are ignored: + /// the order of systems within this set is irrelevant, as the various visibility-checking systesms + /// assumes that their operations are irreversible during the frame. CheckLightVisibility, } diff --git a/crates/bevy_picking/src/lib.rs b/crates/bevy_picking/src/lib.rs index cd75515c97..034ecfd9e8 100644 --- a/crates/bevy_picking/src/lib.rs +++ b/crates/bevy_picking/src/lib.rs @@ -225,7 +225,6 @@ impl Plugin for PickingPlugin { First, (PickSet::Input, PickSet::PostInput) .after(bevy_time::TimeSystem) - .ambiguous_with(bevy_asset::handle_internal_asset_events) .after(bevy_ecs::event::EventUpdates) .chain(), ) @@ -239,7 +238,6 @@ impl Plugin for PickingPlugin { // Eventually events will need to be dispatched here PickSet::Last, ) - .ambiguous_with(bevy_asset::handle_internal_asset_events) .chain(), ) .register_type::() diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index 345055ca3d..8181d3c911 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -278,7 +278,11 @@ pub enum VisibilitySystems { /// [`hierarchy`](bevy_hierarchy). VisibilityPropagate, /// Label for the [`check_visibility`] system updating [`ViewVisibility`] - /// of each entity and the [`VisibleEntities`] of each view. + /// of each entity and the [`VisibleEntities`] of each view.\ + /// + /// System order ambiguities between systems in this set are ignored: + /// the order of systems within this set is irrelevant, as [`check_visibility`] + /// assumes that its operations are irreversible during the frame. CheckVisibility, } @@ -294,6 +298,7 @@ impl Plugin for VisibilityPlugin { .before(CheckVisibility) .after(TransformSystem::TransformPropagate), ) + .configure_sets(PostUpdate, CheckVisibility.ambiguous_with(CheckVisibility)) .add_systems( PostUpdate, ( diff --git a/crates/bevy_ui/src/lib.rs b/crates/bevy_ui/src/lib.rs index 012917fae8..a9dcc9e270 100644 --- a/crates/bevy_ui/src/lib.rs +++ b/crates/bevy_ui/src/lib.rs @@ -164,7 +164,9 @@ impl Plugin for UiPlugin { update_target_camera_system.in_set(UiSystem::Prepare), ui_layout_system .in_set(UiSystem::Layout) - .before(TransformSystem::TransformPropagate), + .before(TransformSystem::TransformPropagate) + // Text and Text2D operate on disjoint sets of entities + .ambiguous_with(bevy_text::update_text2d_layout), ui_stack_system .in_set(UiSystem::Stack) // the systems don't care about stack index @@ -226,7 +228,8 @@ fn build_text_interop(app: &mut App) { .in_set(UiSystem::PostLayout) .after(bevy_text::remove_dropped_font_atlas_sets) // Text2d and bevy_ui text are entirely on separate entities - .ambiguous_with(bevy_text::update_text2d_layout), + .ambiguous_with(bevy_text::update_text2d_layout) + .ambiguous_with(bevy_text::calculate_bounds_text2d), ), ); diff --git a/tests/ecs/ambiguity_detection.rs b/tests/ecs/ambiguity_detection.rs index a8fcf44fc0..39cb05a5cf 100644 --- a/tests/ecs/ambiguity_detection.rs +++ b/tests/ecs/ambiguity_detection.rs @@ -9,18 +9,22 @@ use bevy::{ prelude::*, utils::HashMap, }; -use bevy_render::{pipelined_rendering::RenderExtractApp, RenderApp}; +use bevy_render::pipelined_rendering::RenderExtractApp; 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); + let main_app = app.main_mut(); + configure_ambiguity_detection(main_app); + let render_extract_app = app.sub_app_mut(RenderExtractApp); + configure_ambiguity_detection(render_extract_app); + + // Ambiguities in the RenderApp are currently allowed. + // Eventually, we should forbid these: see https://github.com/bevyengine/bevy/issues/7386 + // Uncomment the lines below to show the current ambiguities in the RenderApp. + // let sub_app = app.sub_app_mut(bevy_render::RenderApp); + // configure_ambiguity_detection(sub_app); app.finish(); app.cleanup(); @@ -29,11 +33,7 @@ fn main() { let main_app_ambiguities = count_ambiguities(app.main()); assert_eq!( main_app_ambiguities.total(), - // This number *should* be zero. - // Over time, we are working to reduce the number: your PR should not increase it. - // If you decrease this by fixing an ambiguity, reduce the number to prevent regressions. - // See https://github.com/bevyengine/bevy/issues/7386 for progress. - 44, + 0, "Main app has unexpected ambiguities among the following schedules: \n{:#?}.", main_app_ambiguities, );