Skip to content

Remove all existing system order ambiguities in DefaultPlugins #15031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_animation/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_app/src/terminal_ctrl_c_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_asset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>();
}
}
Expand Down
16 changes: 14 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,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;
Expand Down Expand Up @@ -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;
1 change: 1 addition & 0 deletions crates/bevy_gizmos/src/aabb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl Plugin for AabbGizmoPlugin {
config.config::<AabbGizmoConfigGroup>().1.draw_all
}),
)
.after(bevy_render::view::VisibilitySystems::CalculateBounds)
.after(TransformSystem::TransformPropagate),
);
}
Expand Down
3 changes: 3 additions & 0 deletions crates/bevy_gizmos/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>>>()
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_internal/src/default_plugins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Comment on lines +93 to +97
Copy link
Member

@Vrixyz Vrixyz Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the previous context comments, that said I can understand they are difficult to balance for maintainability and correctness.

does that refer to all external ambiguities silenced ?

Copy link
Member Author

@alice-i-cecile alice-i-cecile Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was justified using the comments just a few lines up (line 85), which state that UI cannot be animated :) I'd like to change that eventually of course!

}
}
}
Expand Down
14 changes: 13 additions & 1 deletion crates/bevy_pbr/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions crates/bevy_pbr/src/light/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines +504 to 507
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(out of scope for this PR)

is this an opportunity to add a macro to mark that ambiguity is allowed there ? (for better type safety, avoid outdated comments)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, possibly. "Ignore all ambiguities with self in set" isn't a common pattern, but it's not a rare one.

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

Expand Down
2 changes: 0 additions & 2 deletions crates/bevy_picking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
)
Expand All @@ -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>()
Expand Down
7 changes: 6 additions & 1 deletion crates/bevy_render/src/view/visibility/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand All @@ -294,6 +298,7 @@ impl Plugin for VisibilityPlugin {
.before(CheckVisibility)
.after(TransformSystem::TransformPropagate),
)
.configure_sets(PostUpdate, CheckVisibility.ambiguous_with(CheckVisibility))
.add_systems(
PostUpdate,
(
Expand Down
7 changes: 5 additions & 2 deletions crates/bevy_ui/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these two should be in a single set to ignore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly: I wanted to keep the changes relatively minimal though.

),
);

Expand Down
24 changes: 12 additions & 12 deletions tests/ecs/ambiguity_detection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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,
);
Expand Down