Skip to content
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 by Bors] - Resolve most remaining execution-order ambiguities #6341

Closed
wants to merge 18 commits into from
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion crates/bevy_pbr/src/lib.rs
Expand Up @@ -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,
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_pbr/src/light.rs
Expand Up @@ -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 {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_render/src/camera/camera.rs
Expand Up @@ -366,6 +366,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
Expand Down
16 changes: 14 additions & 2 deletions crates/bevy_render/src/camera/projection.rs
Expand Up @@ -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;

Expand All @@ -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 monormophizations.
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
// 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 monormophizations.
JoJoJet marked this conversation as resolved.
Show resolved Hide resolved
// FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481.
.ambiguous_with(CameraUpdateSystem),
);
}
}
Expand Down
13 changes: 11 additions & 2 deletions crates/bevy_render/src/view/visibility/mod.rs
Expand Up @@ -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,
Expand Down
10 changes: 8 additions & 2 deletions crates/bevy_text/src/lib.rs
Expand Up @@ -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;
Expand Down Expand Up @@ -72,7 +72,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) {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_text/src/text2d.rs
Expand Up @@ -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,
Expand Down
22 changes: 19 additions & 3 deletions crates/bevy_ui/src/lib.rs
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_ui/src/widget/image.rs
Expand Up @@ -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
Expand All @@ -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) {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ui/src/widget/text.rs
Expand Up @@ -36,6 +36,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,
Expand Down