forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Resolve most remaining execution-order ambiguities (#6341)

Browse Source 
# 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.
This commit is contained in:
JoJoJet 2022-10-27 12:56:03 +00:00
parent 0934abc6bb
commit 456971381c
10 changed files with 80 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
crates/bevy_pbr/src/lib.rs
View File

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

6
crates/bevy_pbr/src/light.rs
View File

@ -1416,7 +1416,11 @@ pub fn update_directional_light_frusta(
&mut Frustum,
&ComputedVisibility,
),
Or<(Changed<GlobalTransform>, Changed<DirectionalLight>)>,
(
Or<(Changed<GlobalTransform>, Changed<DirectionalLight>)>,
// Prevents this query from conflicting with camera queries.
Without<Camera>,
),
>,
) {
for (transform, directional_light, mut frustum, visibility) in &mut views {

5
crates/bevy_render/src/camera/camera.rs
View File

@ -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<Image>>`](Assets<Image>) -- 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

16
crates/bevy_render/src/camera/projection.rs
View File

@ -19,6 +19,9 @@ impl<T: CameraProjection> Default for CameraProjectionPlugin<T> {
}
}
/// Label for [`camera_system<T>`], shared accross all `T`.
///
/// [`camera_system<T>`]: crate::camera::camera_system
#[derive(SystemLabel, Clone, Eq, PartialEq, Hash, Debug)]
pub struct CameraUpdateSystem;
@ -27,13 +30,22 @@ impl<T: CameraProjection + Component + GetTypeRegistration> Plugin for CameraPro
app.register_type::<T>()
.add_startup_system_to_stage(
StartupStage::PostStartup,
crate::camera::camera_system::<T>,
crate::camera::camera_system::<T>
.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::<T>
.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),
);
}
}

13
crates/bevy_render/src/view/visibility/mod.rs
View File

@ -194,14 +194,23 @@ impl Plugin for VisibilityPlugin {
update_frusta::<OrthographicProjection>
.label(UpdateOrthographicFrusta)
.after(camera_system::<OrthographicProjection>)
.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::<PerspectiveProjection>)
.ambiguous_with(update_frusta::<Projection>),
)
.add_system_to_stage(
CoreStage::PostUpdate,
update_frusta::<PerspectiveProjection>
.label(UpdatePerspectiveFrusta)
.after(camera_system::<PerspectiveProjection>)
.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::<Projection>),
)
.add_system_to_stage(
CoreStage::PostUpdate,

10
crates/bevy_text/src/lib.rs
View File

@ -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<Image>`
// 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) {

5
crates/bevy_text/src/text2d.rs
View File

@ -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<Image>>`](Assets<Image>) -- 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,

22
crates/bevy_ui/src/lib.rs
View File

@ -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<Image>`
// 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<Image>`
// 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<Image>`
// 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,

5
crates/bevy_ui/src/widget/image.rs
View File

@ -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<Assets<Image>>,
mut query: Query<(&mut CalculatedSize, &UiImage), With<ImageMode>>,
mut query: Query<(&mut CalculatedSize, &UiImage), (With<ImageMode>, Without<Text>)>,
) {
for (mut calculated_size, image) in &mut query {
if let Some(texture) = textures.get(image) {

5
crates/bevy_ui/src/widget/text.rs
View File

@ -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<Image>>`](Assets<Image>) -- 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,