Remove all existing system order ambiguities in DefaultPlugins (#15031

)

# Objective

As discussed in #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

@@ -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()

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<AppExit>) {
+ pub fn exit_on_flag(mut events: EventWriter<AppExit>) {
  if SHOULD_EXIT.load(Ordering::Relaxed) {
  events.send(AppExit::from_code(130));
  }

crates/bevy_asset/src/lib.rs

@@ -222,7 +222,11 @@ impl Plugin for AssetPlugin {
  .init_asset::<()>()
  .add_event::<UntypedAssetLoadFailedEvent>()
  .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::<AssetPath>();
  }
 }

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;

crates/bevy_gizmos/src/aabb.rs

@@ -40,6 +40,7 @@ impl Plugin for AabbGizmoPlugin {
  config.config::<AabbGizmoConfigGroup>().1.draw_all
  }),
  )
+ .after(bevy_render::view::VisibilitySystems::CalculateBounds)
  .after(TransformSystem::TransformPropagate),
  );
  }

crates/bevy_gizmos/src/lib.rs

@@ -242,6 +242,9 @@ impl AppGizmoBuilder for App {
  handles.list.insert(TypeId::of::<Config>(), None);
  handles.strip.insert(TypeId::of::<Config>(), None);
 
+ // These handles are safe to mutate in any order
+ self.allow_ambiguous_resource::<LineGizmoHandles>();
+
  self.init_resource::<GizmoStorage<Config, ()>>()
  .init_resource::<GizmoStorage<Config, Fixed>>()
  .init_resource::<GizmoStorage<Config, Swap<Fixed>>>()

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,
+ );
  }
  }
 }

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)

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,
 }
 

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::<pointer::PointerId>()

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,
  (

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),
  ),
  );
 

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,
  );