From 456971381c1c51edc6df46bbcbfccd5a7392aca7 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 27 Oct 2022 12:56:03 +0000 Subject: [PATCH] Resolve most remaining execution-order ambiguities (#6341) # Objective Bevy's internal plugins have lots of execution-order ambiguities, which makes the ambiguity detection tool very noisy for our users. ## Solution Silence every last ambiguity that can currently be resolved. Each time an ambiguity is silenced, it is accompanied by a comment describing why it is correct. This description should be based on the public API of the respective systems. Thus, I have added documentation to some systems describing how they use some resources. # Future work Some ambiguities remain, due to issues out of scope for this PR. * The ambiguity checker does not respect `Without<>` filters, leading to false positives. * Ambiguities between `bevy_ui` and `bevy_animation` cannot be resolved, since neither crate knows that the other exists. We will need a general solution to this problem. --- crates/bevy_pbr/src/lib.rs | 6 ++++- crates/bevy_pbr/src/light.rs | 6 ++++- crates/bevy_render/src/camera/camera.rs | 5 +++++ crates/bevy_render/src/camera/projection.rs | 16 ++++++++++++-- crates/bevy_render/src/view/visibility/mod.rs | 13 +++++++++-- crates/bevy_text/src/lib.rs | 10 +++++++-- crates/bevy_text/src/text2d.rs | 5 +++++ crates/bevy_ui/src/lib.rs | 22 ++++++++++++++++--- crates/bevy_ui/src/widget/image.rs | 5 +++-- crates/bevy_ui/src/widget/text.rs | 5 +++++ 10 files changed, 80 insertions(+), 13 deletions(-) diff --git a/crates/bevy_pbr/src/lib.rs b/crates/bevy_pbr/src/lib.rs index 4a2fa66ec4..12988ab56d 100644 --- a/crates/bevy_pbr/src/lib.rs +++ b/crates/bevy_pbr/src/lib.rs @@ -160,7 +160,11 @@ impl Plugin for PbrPlugin { .label(SimulationLightSystems::UpdateLightFrusta) // This must run after CheckVisibility because it relies on ComputedVisibility::is_visible() .after(VisibilitySystems::CheckVisibility) - .after(TransformSystem::TransformPropagate), + .after(TransformSystem::TransformPropagate) + // We assume that no entity will be both a directional light and a spot light, + // so these systems will run indepdently of one another. + // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. + .ambiguous_with(update_spot_light_frusta), ) .add_system_to_stage( CoreStage::PostUpdate, diff --git a/crates/bevy_pbr/src/light.rs b/crates/bevy_pbr/src/light.rs index 344fff88cb..83b27cfdc2 100644 --- a/crates/bevy_pbr/src/light.rs +++ b/crates/bevy_pbr/src/light.rs @@ -1416,7 +1416,11 @@ pub fn update_directional_light_frusta( &mut Frustum, &ComputedVisibility, ), - Or<(Changed, Changed)>, + ( + Or<(Changed, Changed)>, + // Prevents this query from conflicting with camera queries. + Without, + ), >, ) { for (transform, directional_light, mut frustum, visibility) in &mut views { diff --git a/crates/bevy_render/src/camera/camera.rs b/crates/bevy_render/src/camera/camera.rs index 676ffe650d..88273f31f8 100644 --- a/crates/bevy_render/src/camera/camera.rs +++ b/crates/bevy_render/src/camera/camera.rs @@ -389,6 +389,11 @@ impl RenderTarget { /// the app, as well as the runtime-selected [`Projection`]. The system runs during the /// [`CoreStage::PostUpdate`] stage. /// +/// ## World Resources +/// +/// [`Res>`](Assets) -- For cameras that render to an image, this resource is used to +/// inspect information about the render target. This system will not access any other image assets. +/// /// [`OrthographicProjection`]: crate::camera::OrthographicProjection /// [`PerspectiveProjection`]: crate::camera::PerspectiveProjection /// [`Projection`]: crate::camera::Projection diff --git a/crates/bevy_render/src/camera/projection.rs b/crates/bevy_render/src/camera/projection.rs index c60b902924..87953a4134 100644 --- a/crates/bevy_render/src/camera/projection.rs +++ b/crates/bevy_render/src/camera/projection.rs @@ -19,6 +19,9 @@ impl Default for CameraProjectionPlugin { } } +/// Label for [`camera_system`], shared accross all `T`. +/// +/// [`camera_system`]: crate::camera::camera_system #[derive(SystemLabel, Clone, Eq, PartialEq, Hash, Debug)] pub struct CameraUpdateSystem; @@ -27,13 +30,22 @@ impl Plugin for CameraPro app.register_type::() .add_startup_system_to_stage( StartupStage::PostStartup, - crate::camera::camera_system::, + crate::camera::camera_system:: + .label(CameraUpdateSystem) + // We assume that each camera will only have one projection, + // so we can ignore ambiguities with all other monomorphizations. + // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. + .ambiguous_with(CameraUpdateSystem), ) .add_system_to_stage( CoreStage::PostUpdate, crate::camera::camera_system:: .label(CameraUpdateSystem) - .after(ModifiesWindows), + .after(ModifiesWindows) + // We assume that each camera will only have one projection, + // so we can ignore ambiguities with all other monomorphizations. + // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. + .ambiguous_with(CameraUpdateSystem), ); } } diff --git a/crates/bevy_render/src/view/visibility/mod.rs b/crates/bevy_render/src/view/visibility/mod.rs index d6979ac5e0..3912774f64 100644 --- a/crates/bevy_render/src/view/visibility/mod.rs +++ b/crates/bevy_render/src/view/visibility/mod.rs @@ -194,14 +194,23 @@ impl Plugin for VisibilityPlugin { update_frusta:: .label(UpdateOrthographicFrusta) .after(camera_system::) - .after(TransformSystem::TransformPropagate), + .after(TransformSystem::TransformPropagate) + // We assume that no camera will have more than one projection component, + // so these systems will run independently of one another. + // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. + .ambiguous_with(update_frusta::) + .ambiguous_with(update_frusta::), ) .add_system_to_stage( CoreStage::PostUpdate, update_frusta:: .label(UpdatePerspectiveFrusta) .after(camera_system::) - .after(TransformSystem::TransformPropagate), + .after(TransformSystem::TransformPropagate) + // We assume that no camera will have more than one projection component, + // so these systems will run independently of one another. + // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. + .ambiguous_with(update_frusta::), ) .add_system_to_stage( CoreStage::PostUpdate, diff --git a/crates/bevy_text/src/lib.rs b/crates/bevy_text/src/lib.rs index cd5b8abb75..7c71aa8155 100644 --- a/crates/bevy_text/src/lib.rs +++ b/crates/bevy_text/src/lib.rs @@ -29,7 +29,7 @@ pub mod prelude { use bevy_app::prelude::*; use bevy_asset::AddAsset; use bevy_ecs::{schedule::IntoSystemDescriptor, system::Resource}; -use bevy_render::{RenderApp, RenderStage}; +use bevy_render::{camera::CameraUpdateSystem, RenderApp, RenderStage}; use bevy_sprite::SpriteSystem; use bevy_window::ModifiesWindows; use std::num::NonZeroUsize; @@ -80,7 +80,13 @@ impl Plugin for TextPlugin { .insert_resource(TextPipeline::default()) .add_system_to_stage( CoreStage::PostUpdate, - update_text2d_layout.after(ModifiesWindows), + update_text2d_layout + .after(ModifiesWindows) + // Potential conflict: `Assets` + // In practice, they run independently since `bevy_render::camera_update_system` + // will only ever observe its own render target, and `update_text2d_layout` + // will never modify a pre-existing `Image` asset. + .ambiguous_with(CameraUpdateSystem), ); if let Ok(render_app) = app.get_sub_app_mut(RenderApp) { diff --git a/crates/bevy_text/src/text2d.rs b/crates/bevy_text/src/text2d.rs index ee3663ac59..f63a3def9a 100644 --- a/crates/bevy_text/src/text2d.rs +++ b/crates/bevy_text/src/text2d.rs @@ -145,6 +145,11 @@ pub fn extract_text2d_sprite( /// Updates the layout and size information whenever the text or style is changed. /// This information is computed by the `TextPipeline` on insertion, then stored. +/// +/// ## World Resources +/// +/// [`ResMut>`](Assets) -- This system only adds new [`Image`] assets. +/// It does not modify or observe existing ones. #[allow(clippy::too_many_arguments)] pub fn update_text2d_layout( mut commands: Commands, diff --git a/crates/bevy_ui/src/lib.rs b/crates/bevy_ui/src/lib.rs index 6e47967359..e1c73540c3 100644 --- a/crates/bevy_ui/src/lib.rs +++ b/crates/bevy_ui/src/lib.rs @@ -12,7 +12,7 @@ pub mod entity; pub mod update; pub mod widget; -use bevy_render::extract_component::ExtractComponentPlugin; +use bevy_render::{camera::CameraUpdateSystem, extract_component::ExtractComponentPlugin}; pub use flex::*; pub use focus::*; pub use geometry::*; @@ -104,11 +104,27 @@ impl Plugin for UiPlugin { CoreStage::PostUpdate, widget::text_system .before(UiSystem::Flex) - .after(ModifiesWindows), + .after(ModifiesWindows) + // Potential conflict: `Assets` + // In practice, they run independently since `bevy_render::camera_update_system` + // will only ever observe its own render target, and `widget::text_system` + // will never modify a pre-existing `Image` asset. + .ambiguous_with(CameraUpdateSystem) + // Potential conflict: `Assets` + // Since both systems will only ever insert new [`Image`] assets, + // they will never observe each other's effects. + .ambiguous_with(bevy_text::update_text2d_layout), ) .add_system_to_stage( CoreStage::PostUpdate, - widget::image_node_system.before(UiSystem::Flex), + widget::image_node_system + .before(UiSystem::Flex) + // Potential conflicts: `Assets` + // They run independently since `widget::image_node_system` will only ever observe + // its own UiImage, and `widget::text_system` & `bevy_text::update_text2d_layout` + // will never modify a pre-existing `Image` asset. + .ambiguous_with(bevy_text::update_text2d_layout) + .ambiguous_with(widget::text_system), ) .add_system_to_stage( CoreStage::PostUpdate, diff --git a/crates/bevy_ui/src/widget/image.rs b/crates/bevy_ui/src/widget/image.rs index 7519d4966e..2a8d806e59 100644 --- a/crates/bevy_ui/src/widget/image.rs +++ b/crates/bevy_ui/src/widget/image.rs @@ -2,12 +2,13 @@ use crate::{CalculatedSize, Size, UiImage, Val}; use bevy_asset::Assets; use bevy_ecs::{ component::Component, - query::With, + query::{With, Without}, reflect::ReflectComponent, system::{Query, Res}, }; use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize}; use bevy_render::texture::Image; +use bevy_text::Text; use serde::{Deserialize, Serialize}; /// Describes how to resize the Image node @@ -22,7 +23,7 @@ pub enum ImageMode { /// Updates calculated size of the node based on the image provided pub fn image_node_system( textures: Res>, - mut query: Query<(&mut CalculatedSize, &UiImage), With>, + mut query: Query<(&mut CalculatedSize, &UiImage), (With, Without)>, ) { for (mut calculated_size, image) in &mut query { if let Some(texture) = textures.get(image) { diff --git a/crates/bevy_ui/src/widget/text.rs b/crates/bevy_ui/src/widget/text.rs index 7c9076a584..37cd2340fe 100644 --- a/crates/bevy_ui/src/widget/text.rs +++ b/crates/bevy_ui/src/widget/text.rs @@ -39,6 +39,11 @@ pub fn text_constraint(min_size: Val, size: Val, max_size: Val, scale_factor: f6 /// Updates the layout and size information whenever the text or style is changed. /// This information is computed by the `TextPipeline` on insertion, then stored. +/// +/// ## World Resources +/// +/// [`ResMut>`](Assets) -- This system only adds new [`Image`] assets. +/// It does not modify or observe existing ones. #[allow(clippy::too_many_arguments)] pub fn text_system( mut commands: Commands,