From 7cd2ee2bbd6003f45b4873b66a3022d5764e510d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 2 Mar 2023 17:19:47 +0000 Subject: [PATCH 01/11] Use default-implemented methods for `IntoSystemConfig<>` (#7870) # Objective The trait `IntoSystemConfig<>` requires each implementer to repeat every single member method, even though they can all be implemented by just deferring to `SystemConfig`. ## Solution Add default implementations to most member methods. --- crates/bevy_ecs/src/schedule/config.rs | 195 ++++++------------------- 1 file changed, 47 insertions(+), 148 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs index 899985fb22b43..54eb9f863f5b7 100644 --- a/crates/bevy_ecs/src/schedule/config.rs +++ b/crates/bevy_ecs/src/schedule/config.rs @@ -82,112 +82,60 @@ fn ambiguous_with(graph_info: &mut GraphInfo, set: BoxedSystemSet) { /// Types that can be converted into a [`SystemSetConfig`]. /// /// This has been implemented for all types that implement [`SystemSet`] and boxed trait objects. -pub trait IntoSystemSetConfig { +pub trait IntoSystemSetConfig: Sized { /// Convert into a [`SystemSetConfig`]. #[doc(hidden)] fn into_config(self) -> SystemSetConfig; /// Add to the provided `set`. - #[track_caller] - fn in_set(self, set: impl FreeSystemSet) -> SystemSetConfig; - /// Add to the provided "base" `set`. For more information on base sets, see [`SystemSet::is_base`]. - #[track_caller] - fn in_base_set(self, set: impl BaseSystemSet) -> SystemSetConfig; - /// Add this set to the schedules's default base set. - fn in_default_base_set(self) -> SystemSetConfig; - /// Run before all systems in `set`. - fn before(self, set: impl IntoSystemSet) -> SystemSetConfig; - /// Run after all systems in `set`. - fn after(self, set: impl IntoSystemSet) -> SystemSetConfig; - /// Run the systems in this set only if the [`Condition`] is `true`. - /// - /// The `Condition` will be evaluated at most once (per schedule run), - /// the first time a system in this set prepares to run. - fn run_if(self, condition: impl Condition) -> SystemSetConfig; - /// Suppress warnings and errors that would result from systems in this set having ambiguities - /// (conflicting access but indeterminate order) with systems in `set`. - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig; - /// Suppress warnings and errors that would result from systems in this set having ambiguities - /// (conflicting access but indeterminate order) with any other system. - fn ambiguous_with_all(self) -> SystemSetConfig; -} - -impl IntoSystemSetConfig for S { - fn into_config(self) -> SystemSetConfig { - SystemSetConfig::new(Box::new(self)) - } - #[track_caller] fn in_set(self, set: impl FreeSystemSet) -> SystemSetConfig { self.into_config().in_set(set) } - + /// Add to the provided "base" `set`. For more information on base sets, see [`SystemSet::is_base`]. #[track_caller] fn in_base_set(self, set: impl BaseSystemSet) -> SystemSetConfig { self.into_config().in_base_set(set) } - + /// Add this set to the schedules's default base set. fn in_default_base_set(self) -> SystemSetConfig { self.into_config().in_default_base_set() } - + /// Run before all systems in `set`. fn before(self, set: impl IntoSystemSet) -> SystemSetConfig { self.into_config().before(set) } - + /// Run after all systems in `set`. fn after(self, set: impl IntoSystemSet) -> SystemSetConfig { self.into_config().after(set) } - + /// Run the systems in this set only if the [`Condition`] is `true`. + /// + /// The `Condition` will be evaluated at most once (per schedule run), + /// the first time a system in this set prepares to run. fn run_if(self, condition: impl Condition) -> SystemSetConfig { self.into_config().run_if(condition) } - + /// Suppress warnings and errors that would result from systems in this set having ambiguities + /// (conflicting access but indeterminate order) with systems in `set`. fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig { self.into_config().ambiguous_with(set) } - + /// Suppress warnings and errors that would result from systems in this set having ambiguities + /// (conflicting access but indeterminate order) with any other system. fn ambiguous_with_all(self) -> SystemSetConfig { self.into_config().ambiguous_with_all() } } -impl IntoSystemSetConfig for BoxedSystemSet { +impl IntoSystemSetConfig for S { fn into_config(self) -> SystemSetConfig { - SystemSetConfig::new(self) - } - - #[track_caller] - fn in_set(self, set: impl FreeSystemSet) -> SystemSetConfig { - self.into_config().in_set(set) - } - - #[track_caller] - fn in_base_set(self, set: impl BaseSystemSet) -> SystemSetConfig { - self.into_config().in_base_set(set) - } - - fn in_default_base_set(self) -> SystemSetConfig { - self.into_config().in_default_base_set() - } - - fn before(self, set: impl IntoSystemSet) -> SystemSetConfig { - self.into_config().before(set) - } - - fn after(self, set: impl IntoSystemSet) -> SystemSetConfig { - self.into_config().after(set) - } - - fn run_if(self, condition: impl Condition) -> SystemSetConfig { - self.into_config().run_if(condition) - } - - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemSetConfig { - self.into_config().ambiguous_with(set) + SystemSetConfig::new(Box::new(self)) } +} - fn ambiguous_with_all(self) -> SystemSetConfig { - self.into_config().ambiguous_with_all() +impl IntoSystemSetConfig for BoxedSystemSet { + fn into_config(self) -> SystemSetConfig { + SystemSetConfig::new(self) } } @@ -273,33 +221,52 @@ impl IntoSystemSetConfig for SystemSetConfig { /// /// This has been implemented for boxed [`System`](crate::system::System) /// trait objects and all functions that turn into such. -pub trait IntoSystemConfig { +pub trait IntoSystemConfig: Sized +where + Config: IntoSystemConfig<(), Config>, +{ /// Convert into a [`SystemConfig`]. #[doc(hidden)] fn into_config(self) -> Config; /// Add to `set` membership. #[track_caller] - fn in_set(self, set: impl FreeSystemSet) -> Config; + fn in_set(self, set: impl FreeSystemSet) -> Config { + self.into_config().in_set(set) + } /// Add to the provided "base" `set`. For more information on base sets, see [`SystemSet::is_base`]. #[track_caller] - fn in_base_set(self, set: impl BaseSystemSet) -> Config; + fn in_base_set(self, set: impl BaseSystemSet) -> Config { + self.into_config().in_base_set(set) + } /// Don't add this system to the schedules's default set. - fn no_default_base_set(self) -> Config; + fn no_default_base_set(self) -> Config { + self.into_config().no_default_base_set() + } /// Run before all systems in `set`. - fn before(self, set: impl IntoSystemSet) -> Config; + fn before(self, set: impl IntoSystemSet) -> Config { + self.into_config().before(set) + } /// Run after all systems in `set`. - fn after(self, set: impl IntoSystemSet) -> Config; + fn after(self, set: impl IntoSystemSet) -> Config { + self.into_config().after(set) + } /// Run only if the [`Condition`] is `true`. /// /// The `Condition` will be evaluated at most once (per schedule run), /// when the system prepares to run. - fn run_if(self, condition: impl Condition) -> Config; + fn run_if(self, condition: impl Condition) -> Config { + self.into_config().run_if(condition) + } /// Suppress warnings and errors that would result from this system having ambiguities /// (conflicting access but indeterminate order) with systems in `set`. - fn ambiguous_with(self, set: impl IntoSystemSet) -> Config; + fn ambiguous_with(self, set: impl IntoSystemSet) -> Config { + self.into_config().ambiguous_with(set) + } /// Suppress warnings and errors that would result from this system having ambiguities /// (conflicting access but indeterminate order) with any other system. - fn ambiguous_with_all(self) -> Config; + fn ambiguous_with_all(self) -> Config { + self.into_config().ambiguous_with_all() + } } impl IntoSystemConfig for F @@ -309,80 +276,12 @@ where fn into_config(self) -> SystemConfig { SystemConfig::new(Box::new(IntoSystem::into_system(self))) } - - #[track_caller] - fn in_set(self, set: impl FreeSystemSet) -> SystemConfig { - self.into_config().in_set(set) - } - - #[track_caller] - fn in_base_set(self, set: impl BaseSystemSet) -> SystemConfig { - self.into_config().in_base_set(set) - } - - fn no_default_base_set(self) -> SystemConfig { - self.into_config().no_default_base_set() - } - - fn before(self, set: impl IntoSystemSet) -> SystemConfig { - self.into_config().before(set) - } - - fn after(self, set: impl IntoSystemSet) -> SystemConfig { - self.into_config().after(set) - } - - fn run_if(self, condition: impl Condition) -> SystemConfig { - self.into_config().run_if(condition) - } - - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig { - self.into_config().ambiguous_with(set) - } - - fn ambiguous_with_all(self) -> SystemConfig { - self.into_config().ambiguous_with_all() - } } impl IntoSystemConfig<()> for BoxedSystem<(), ()> { fn into_config(self) -> SystemConfig { SystemConfig::new(self) } - - #[track_caller] - fn in_set(self, set: impl FreeSystemSet) -> SystemConfig { - self.into_config().in_set(set) - } - - #[track_caller] - fn in_base_set(self, set: impl BaseSystemSet) -> SystemConfig { - self.into_config().in_base_set(set) - } - - fn no_default_base_set(self) -> SystemConfig { - self.into_config().no_default_base_set() - } - - fn before(self, set: impl IntoSystemSet) -> SystemConfig { - self.into_config().before(set) - } - - fn after(self, set: impl IntoSystemSet) -> SystemConfig { - self.into_config().after(set) - } - - fn run_if(self, condition: impl Condition) -> SystemConfig { - self.into_config().run_if(condition) - } - - fn ambiguous_with(self, set: impl IntoSystemSet) -> SystemConfig { - self.into_config().ambiguous_with(set) - } - - fn ambiguous_with_all(self) -> SystemConfig { - self.into_config().ambiguous_with_all() - } } impl IntoSystemConfig<()> for SystemConfig { From 9153bd0e786df602e8f86f3c564c131f9d738242 Mon Sep 17 00:00:00 2001 From: ickshonpe Date: Thu, 2 Mar 2023 17:37:09 +0000 Subject: [PATCH 02/11] Document the `border` field of `Style`. (#7868) # Objective Document the `border` field of `Style`. --- crates/bevy_ui/src/ui_node.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ui/src/ui_node.rs b/crates/bevy_ui/src/ui_node.rs index 38b88ecbdcbe7..46beae57f8247 100644 --- a/crates/bevy_ui/src/ui_node.rs +++ b/crates/bevy_ui/src/ui_node.rs @@ -275,7 +275,13 @@ pub struct Style { /// ``` /// A node with this style and a parent with dimensions of 300px by 100px, will have calculated padding of 3px on the left, 6px on the right, 9px on the top and 12px on the bottom. pub padding: UiRect, - /// The border of the node + /// The amount of space between the margins of a node and its padding. + /// + /// If a percentage value is used, the percentage is calculated based on the width of the parent node. + /// + /// The size of the node will be expanded if there are constraints that prevent the layout algorithm from placing the border within the existing node boundary. + /// + /// Rendering for borders is not yet implemented. pub border: UiRect, /// Defines how much a flexbox item should grow if there's space available pub flex_grow: f32, From 8b8078d1d02abaec317bbafe620a51c652159ff9 Mon Sep 17 00:00:00 2001 From: Jakob Hellermann Date: Thu, 2 Mar 2023 20:04:03 +0000 Subject: [PATCH 03/11] single parent set for transform propagate (#7869) # Objective - have no system belonging to multiple sets go from ![before](https://user-images.githubusercontent.com/22177966/222439644-7cf2f84e-0839-4703-a7b4-66ffe92c6aa1.png) to ![after](https://user-images.githubusercontent.com/22177966/222439747-37872d59-6b8e-4fff-a579-6d40c38f73d3.png) ## Solution - `propagate_transforms in PropagateTransformSets in TransformSystem::TransformPropagate` instead of ``` propagate_transforms in PropagateTransformSets propagate_transforms in TransformSystem::TransformPropagate PropagateTransformsSet is free ``` Co-authored-by: Jakob Hellermann --- crates/bevy_transform/src/lib.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/crates/bevy_transform/src/lib.rs b/crates/bevy_transform/src/lib.rs index 4ebbc43f0cdfa..e45c648f7264a 100644 --- a/crates/bevy_transform/src/lib.rs +++ b/crates/bevy_transform/src/lib.rs @@ -100,6 +100,7 @@ impl Plugin for TransformPlugin { .add_plugin(ValidParentCheckPlugin::::default()) // add transform systems to startup so the first update is "correct" .configure_set(TransformSystem::TransformPropagate.in_base_set(CoreSet::PostUpdate)) + .configure_set(PropagateTransformsSet.in_set(TransformSystem::TransformPropagate)) .edit_schedule(CoreSchedule::Startup, |schedule| { schedule.configure_set( TransformSystem::TransformPropagate.in_base_set(StartupSet::PostStartup), @@ -113,20 +114,12 @@ impl Plugin for TransformPlugin { .in_set(TransformSystem::TransformPropagate) .ambiguous_with(PropagateTransformsSet), ) - .add_startup_system( - propagate_transforms - .in_set(TransformSystem::TransformPropagate) - .in_set(PropagateTransformsSet), - ) + .add_startup_system(propagate_transforms.in_set(PropagateTransformsSet)) .add_system( sync_simple_transforms .in_set(TransformSystem::TransformPropagate) .ambiguous_with(PropagateTransformsSet), ) - .add_system( - propagate_transforms - .in_set(TransformSystem::TransformPropagate) - .in_set(PropagateTransformsSet), - ); + .add_system(propagate_transforms.in_set(PropagateTransformsSet)); } } From fc7a3bdfc21320dd54e4ede96690da40638bd155 Mon Sep 17 00:00:00 2001 From: JMS55 <47158642+JMS55@users.noreply.github.com> Date: Thu, 2 Mar 2023 22:44:10 +0000 Subject: [PATCH 04/11] Remove dead code after #7784 (#7875) # Objective - Remove dead code after #7784 # Changelog - Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group. # Migration Guide - Removed `SetShadowViewBindGroup`, `queue_shadow_view_bind_group()`, and `LightMeta::shadow_view_bind_group` in favor of reusing the prepass view bind group. --- crates/bevy_pbr/src/material.rs | 10 +---- crates/bevy_pbr/src/render/light.rs | 58 ++--------------------------- 2 files changed, 5 insertions(+), 63 deletions(-) diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index 16213b2bb1ce0..aea0f713a27cf 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -1,7 +1,6 @@ use crate::{ - queue_mesh_view_bind_groups, render, AlphaMode, DrawMesh, DrawPrepass, EnvironmentMapLight, - MeshPipeline, MeshPipelineKey, MeshUniform, PrepassPlugin, RenderLightSystems, - SetMeshBindGroup, SetMeshViewBindGroup, Shadow, + render, AlphaMode, DrawMesh, DrawPrepass, EnvironmentMapLight, MeshPipeline, MeshPipelineKey, + MeshUniform, PrepassPlugin, RenderLightSystems, SetMeshBindGroup, SetMeshViewBindGroup, Shadow, }; use bevy_app::{App, IntoSystemAppConfig, Plugin}; use bevy_asset::{AddAsset, AssetEvent, AssetServer, Assets, Handle}; @@ -205,11 +204,6 @@ where .after(PrepareAssetSet::PreAssetPrepare), ) .add_system(render::queue_shadows::.in_set(RenderLightSystems::QueueShadows)) - .add_system( - render::queue_shadow_view_bind_group:: - .in_set(RenderSet::Queue) - .ambiguous_with(queue_mesh_view_bind_groups), // queue_mesh_view_bind_groups does not read `shadow_view_bind_group`), - ) .add_system(queue_material_meshes::.in_set(RenderSet::Queue)); } diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs index 51cc941d5a84b..6e6783caf676f 100644 --- a/crates/bevy_pbr/src/render/light.rs +++ b/crates/bevy_pbr/src/render/light.rs @@ -8,10 +8,7 @@ use crate::{ }; use bevy_asset::Handle; use bevy_core_pipeline::core_3d::Transparent3d; -use bevy_ecs::{ - prelude::*, - system::{lifetimeless::*, SystemParamItem}, -}; +use bevy_ecs::prelude::*; use bevy_math::{Mat4, UVec3, UVec4, Vec2, Vec3, Vec3Swizzles, Vec4, Vec4Swizzles}; use bevy_render::{ camera::Camera, @@ -20,13 +17,12 @@ use bevy_render::{ render_asset::RenderAssets, render_graph::{Node, NodeRunError, RenderGraphContext, SlotInfo, SlotType}, render_phase::{ - CachedRenderPipelinePhaseItem, DrawFunctionId, DrawFunctions, PhaseItem, RenderCommand, - RenderCommandResult, RenderPhase, TrackedRenderPass, + CachedRenderPipelinePhaseItem, DrawFunctionId, DrawFunctions, PhaseItem, RenderPhase, }, render_resource::*, renderer::{RenderContext, RenderDevice, RenderQueue}, texture::*, - view::{ComputedVisibility, ExtractedView, ViewUniformOffset, ViewUniforms, VisibleEntities}, + view::{ComputedVisibility, ExtractedView, VisibleEntities}, Extract, }; use bevy_transform::{components::GlobalTransform, prelude::Transform}; @@ -579,7 +575,6 @@ impl GlobalLightMeta { #[derive(Resource, Default)] pub struct LightMeta { pub view_gpu_lights: DynamicUniformBuffer, - pub shadow_view_bind_group: Option, } #[derive(Component)] @@ -1549,25 +1544,6 @@ pub fn prepare_clusters( } } -pub fn queue_shadow_view_bind_group( - render_device: Res, - prepass_pipeline: Res>, - mut light_meta: ResMut, - view_uniforms: Res, -) { - if let Some(view_binding) = view_uniforms.uniforms.binding() { - light_meta.shadow_view_bind_group = - Some(render_device.create_bind_group(&BindGroupDescriptor { - entries: &[BindGroupEntry { - binding: 0, - resource: view_binding, - }], - label: Some("shadow_view_bind_group"), - layout: &prepass_pipeline.view_layout, - })); - } -} - #[allow(clippy::too_many_arguments)] pub fn queue_shadows( shadow_draw_functions: Res>, @@ -1772,31 +1748,3 @@ impl Node for ShadowPassNode { Ok(()) } } - -pub struct SetShadowViewBindGroup; -impl RenderCommand for SetShadowViewBindGroup { - type Param = SRes; - type ViewWorldQuery = Read; - type ItemWorldQuery = (); - - #[inline] - fn render<'w>( - _item: &Shadow, - view_uniform_offset: &'_ ViewUniformOffset, - _entity: (), - light_meta: SystemParamItem<'w, '_, Self::Param>, - pass: &mut TrackedRenderPass<'w>, - ) -> RenderCommandResult { - pass.set_bind_group( - I, - light_meta - .into_inner() - .shadow_view_bind_group - .as_ref() - .unwrap(), - &[view_uniform_offset.offset], - ); - - RenderCommandResult::Success - } -} From 73c1ab1d42965e84a1748af24de61eb768d98848 Mon Sep 17 00:00:00 2001 From: TimJentzsch Date: Thu, 2 Mar 2023 22:44:12 +0000 Subject: [PATCH 05/11] Fix `bevy_ui` compile error without `bevy_text` (#7877) # Objective - Fixes #7874. - The `bevy_text` dependency is optional for `bevy_ui`, but the `accessibility` module depended on it. ## Solution - Guard the `accessibility` module behind the `bevy_text` feature and only add the plugin when it's enabled. --- crates/bevy_ui/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ui/src/lib.rs b/crates/bevy_ui/src/lib.rs index ea5a1ba932e71..b668c2b9515a4 100644 --- a/crates/bevy_ui/src/lib.rs +++ b/crates/bevy_ui/src/lib.rs @@ -9,6 +9,7 @@ mod render; mod stack; mod ui_node; +#[cfg(feature = "bevy_text")] mod accessibility; pub mod camera_config; pub mod node_bundles; @@ -103,7 +104,6 @@ impl Plugin for UiPlugin { .register_type::() .register_type::() .register_type::() - .add_plugin(accessibility::AccessibilityPlugin) .configure_set(UiSystem::Focus.in_base_set(CoreSet::PreUpdate)) .configure_set(UiSystem::Flex.in_base_set(CoreSet::PostUpdate)) .configure_set(UiSystem::Stack.in_base_set(CoreSet::PostUpdate)) @@ -124,6 +124,8 @@ impl Plugin for UiPlugin { // they will never observe each other's effects. .ambiguous_with(bevy_text::update_text2d_layout), ); + #[cfg(feature = "bevy_text")] + app.add_plugin(accessibility::AccessibilityPlugin); app.add_system({ let system = widget::update_image_calculated_size_system .in_base_set(CoreSet::PostUpdate) From 85c8fb9dfa53b0d569c8d583d7e84665fdad4f68 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Fri, 3 Mar 2023 14:43:54 +0000 Subject: [PATCH 06/11] Revise docs for system set marker traits (#7882) # Objective #7863 introduced a potential footgun. When trying to incorrectly add a user-defined type using `in_base_set`, the compiler will suggest that the user implement `BaseSystemSet` for their type. This is a reasonable-sounding suggestion, however this is not the correct way to make a base set, and will lead to a confusing panic message when a marker trait is implemented for the wrong type. ## Solution Rewrite the documentation for these traits, making it more clear that `BaseSystemSet` is a marker for types that are already base sets, and not a way to define a base set. --- crates/bevy_ecs/src/schedule/set.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/set.rs b/crates/bevy_ecs/src/schedule/set.rs index ad882f11c1897..23058a3de9fbc 100644 --- a/crates/bevy_ecs/src/schedule/set.rs +++ b/crates/bevy_ecs/src/schedule/set.rs @@ -37,14 +37,18 @@ pub trait SystemSet: DynHash + Debug + Send + Sync + 'static { fn dyn_clone(&self) -> Box; } -/// A system set that is never contained in another set. -/// Systems and other system sets may only belong to one base set. +/// A marker trait for `SystemSet` types where [`is_base`] returns `true`. +/// This should only be implemented for types that satisfy this requirement. +/// It is automatically implemented for base set types by `#[derive(SystemSet)]`. /// -/// This should only be implemented for types that return `true` from [`SystemSet::is_base`]. +/// [`is_base`]: SystemSet::is_base pub trait BaseSystemSet: SystemSet {} -/// System sets that are *not* base sets. This should not be implemented for -/// any types that implement [`BaseSystemSet`]. +/// A marker trait for `SystemSet` types where [`is_base`] returns `false`. +/// This should only be implemented for types that satisfy this requirement. +/// It is automatically implemented for non-base set types by `#[derive(SystemSet)]`. +/// +/// [`is_base`]: SystemSet::is_base pub trait FreeSystemSet: SystemSet {} impl PartialEq for dyn SystemSet { From cb0db07c5b59aff3b2359064712d3ba213891832 Mon Sep 17 00:00:00 2001 From: Edgar Geier Date: Fri, 3 Mar 2023 15:08:54 +0000 Subject: [PATCH 07/11] Fix dependency of shadow mapping on the optional `PrepassPlugin` (#7878) # Objective Unfortunately, there are three issues with my changes introduced by #7784. 1. The changes left some dead code. This is already taken care of here: #7875. 2. Disabling prepass causes failures because the shadow mapping relies on the `PrepassPlugin` now. 3. Custom materials use the `prepass.wgsl` shader, but this does not always define a fragment entry point. This PR fixes 2. and 3. and resolves #7879. ## Solution - Add a regression test with disabled prepass. - Split `PrepassPlugin` into two plugins: - `PrepassPipelinePlugin` contains the part that is required for the shadow mapping to work and is unconditionally added. - `PrepassPlugin` now only adds the systems and resources required for the "real" prepasses. - Add a noop fragment entry point to `prepass.wgsl`, used if `NORMAL_PASS` is not defined. Co-authored-by: Edgar Geier --- Cargo.toml | 7 ++++ crates/bevy_pbr/src/material.rs | 6 +++- crates/bevy_pbr/src/prepass/mod.rs | 52 +++++++++++++++++++++++------- tests/3d/no_prepass.rs | 11 +++++++ 4 files changed, 63 insertions(+), 13 deletions(-) create mode 100644 tests/3d/no_prepass.rs diff --git a/Cargo.toml b/Cargo.toml index aac8ea12e6f46..8068f9ed825ad 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -651,6 +651,13 @@ description = "Showcases wireframe rendering" category = "3D Rendering" wasm = true +[[example]] +name = "no_prepass" +path = "tests/3d/no_prepass.rs" + +[package.metadata.example.no_prepass] +hidden = true + # Animation [[example]] name = "animated_fox" diff --git a/crates/bevy_pbr/src/material.rs b/crates/bevy_pbr/src/material.rs index aea0f713a27cf..82a477c57039d 100644 --- a/crates/bevy_pbr/src/material.rs +++ b/crates/bevy_pbr/src/material.rs @@ -1,6 +1,7 @@ use crate::{ render, AlphaMode, DrawMesh, DrawPrepass, EnvironmentMapLight, MeshPipeline, MeshPipelineKey, - MeshUniform, PrepassPlugin, RenderLightSystems, SetMeshBindGroup, SetMeshViewBindGroup, Shadow, + MeshUniform, PrepassPipelinePlugin, PrepassPlugin, RenderLightSystems, SetMeshBindGroup, + SetMeshViewBindGroup, Shadow, }; use bevy_app::{App, IntoSystemAppConfig, Plugin}; use bevy_asset::{AddAsset, AssetEvent, AssetServer, Assets, Handle}; @@ -207,6 +208,9 @@ where .add_system(queue_material_meshes::.in_set(RenderSet::Queue)); } + // PrepassPipelinePlugin is required for shadow mapping and the optional PrepassPlugin + app.add_plugin(PrepassPipelinePlugin::::default()); + if self.prepass_enabled { app.add_plugin(PrepassPlugin::::default()); } diff --git a/crates/bevy_pbr/src/prepass/mod.rs b/crates/bevy_pbr/src/prepass/mod.rs index acfb160dd35c8..407c357a230e4 100644 --- a/crates/bevy_pbr/src/prepass/mod.rs +++ b/crates/bevy_pbr/src/prepass/mod.rs @@ -59,15 +59,18 @@ pub const PREPASS_BINDINGS_SHADER_HANDLE: HandleUntyped = pub const PREPASS_UTILS_SHADER_HANDLE: HandleUntyped = HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 4603948296044544); -pub struct PrepassPlugin(PhantomData); +/// Sets up everything required to use the prepass pipeline. +/// +/// This does not add the actual prepasses, see [`PrepassPlugin`] for that. +pub struct PrepassPipelinePlugin(PhantomData); -impl Default for PrepassPlugin { +impl Default for PrepassPipelinePlugin { fn default() -> Self { Self(Default::default()) } } -impl Plugin for PrepassPlugin +impl Plugin for PrepassPipelinePlugin where M::Data: PartialEq + Eq + Hash + Clone, { @@ -93,6 +96,34 @@ where Shader::from_wgsl ); + let Ok(render_app) = app.get_sub_app_mut(RenderApp) else { + return; + }; + + render_app + .add_system(queue_prepass_view_bind_group::.in_set(RenderSet::Queue)) + .init_resource::>() + .init_resource::() + .init_resource::>>(); + } +} + +/// Sets up the prepasses for a [`Material`]. +/// +/// This depends on the [`PrepassPipelinePlugin`]. +pub struct PrepassPlugin(PhantomData); + +impl Default for PrepassPlugin { + fn default() -> Self { + Self(Default::default()) + } +} + +impl Plugin for PrepassPlugin +where + M::Data: PartialEq + Eq + Hash + Clone, +{ + fn build(&self, app: &mut bevy_app::App) { let Ok(render_app) = app.get_sub_app_mut(RenderApp) else { return; }; @@ -104,15 +135,11 @@ where .in_set(RenderSet::Prepare) .after(bevy_render::view::prepare_windows), ) - .add_system(queue_prepass_view_bind_group::.in_set(RenderSet::Queue)) .add_system(queue_prepass_material_meshes::.in_set(RenderSet::Queue)) .add_system(sort_phase_system::.in_set(RenderSet::PhaseSort)) .add_system(sort_phase_system::.in_set(RenderSet::PhaseSort)) - .init_resource::>() .init_resource::>() .init_resource::>() - .init_resource::() - .init_resource::>>() .add_render_command::>() .add_render_command::>(); } @@ -254,11 +281,12 @@ where let vertex_buffer_layout = layout.get_layout(&vertex_attributes)?; - // The fragment shader is only used when the normal prepass is enabled or the material uses alpha cutoff values - let fragment = if key - .mesh_key - .intersects(MeshPipelineKey::NORMAL_PREPASS | MeshPipelineKey::ALPHA_MASK) - || blend_key == MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA + // The fragment shader is only used when the normal prepass is enabled + // or the material uses alpha cutoff values and doesn't rely on the standard prepass shader + let fragment = if key.mesh_key.contains(MeshPipelineKey::NORMAL_PREPASS) + || ((key.mesh_key.contains(MeshPipelineKey::ALPHA_MASK) + || blend_key == MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA) + && self.material_fragment_shader.is_some()) { // Use the fragment shader from the material if present let frag_shader_handle = if let Some(handle) = &self.material_fragment_shader { diff --git a/tests/3d/no_prepass.rs b/tests/3d/no_prepass.rs new file mode 100644 index 0000000000000..589cfaa95caa2 --- /dev/null +++ b/tests/3d/no_prepass.rs @@ -0,0 +1,11 @@ +//! A test to confirm that `bevy` allows disabling the prepass of the standard material. +//! This is run in CI to ensure that this doesn't regress again. +use bevy::{pbr::PbrPlugin, prelude::*}; + +fn main() { + App::new() + .add_plugins(DefaultPlugins.set(PbrPlugin { + prepass_enabled: false, + })) + .run(); +} From 8cbc89481b570decc93e8249388defd55877a757 Mon Sep 17 00:00:00 2001 From: robtfm <50659922+robtfm@users.noreply.github.com> Date: Fri, 3 Mar 2023 17:43:58 +0000 Subject: [PATCH 08/11] fix target/attach vecs --- crates/bevy_core_pipeline/src/prepass/node.rs | 37 ++++----- crates/bevy_pbr/src/prepass/mod.rs | 76 +++++++++---------- 2 files changed, 54 insertions(+), 59 deletions(-) diff --git a/crates/bevy_core_pipeline/src/prepass/node.rs b/crates/bevy_core_pipeline/src/prepass/node.rs index ca222d150a099..e1382eafdee22 100644 --- a/crates/bevy_core_pipeline/src/prepass/node.rs +++ b/crates/bevy_core_pipeline/src/prepass/node.rs @@ -74,29 +74,32 @@ impl Node for PrepassNode { } let mut color_attachments = vec![]; - if let Some(view_normals_texture) = &view_prepass_textures.normal { - color_attachments.push(Some(RenderPassColorAttachment { + color_attachments.push(view_prepass_textures.normal.as_ref().map( |view_normals_texture| + RenderPassColorAttachment { view: &view_normals_texture.default_view, resolve_target: None, ops: Operations { load: LoadOp::Clear(Color::BLACK.into()), store: true, }, - })); - } else { - color_attachments.push(None); - } - if let Some(view_motion_vectors_texture) = &view_prepass_textures.motion_vectors { - color_attachments.push(Some(RenderPassColorAttachment { - view: &view_motion_vectors_texture.default_view, - resolve_target: None, - ops: Operations { - // Blue channel dosen't matter, but set to 1.0 for possible faster clear - // https://gpuopen.com/performance/#clears - load: LoadOp::Clear(Color::rgb_linear(1.0, 1.0, 1.0).into()), - store: true, - }, - })); + } + )); + + color_attachments.push(view_prepass_textures.motion_vectors.as_ref().map(|view_motion_vectors_texture| + RenderPassColorAttachment { + view: &view_motion_vectors_texture.default_view, + resolve_target: None, + ops: Operations { + // Blue channel dosen't matter, but set to 1.0 for possible faster clear + // https://gpuopen.com/performance/#clears + load: LoadOp::Clear(Color::rgb_linear(1.0, 1.0, 1.0).into()), + store: true, + }, + })); + + if color_attachments.iter().all(Option::is_none) { + // all attachments are none: clear the attachment list so that no fragment shader is required + color_attachments.clear(); } { diff --git a/crates/bevy_pbr/src/prepass/mod.rs b/crates/bevy_pbr/src/prepass/mod.rs index a91e2f880f48d..bcb79cd6afbeb 100644 --- a/crates/bevy_pbr/src/prepass/mod.rs +++ b/crates/bevy_pbr/src/prepass/mod.rs @@ -148,7 +148,6 @@ where .in_set(RenderSet::Prepare) .after(PrepassLightsViewFlush), ) - .add_system(queue_prepass_view_bind_group::.in_set(RenderSet::Queue)) .add_system(queue_prepass_material_meshes::.in_set(RenderSet::Queue)) .add_system(sort_phase_system::.in_set(RenderSet::PhaseSort)) .add_system(sort_phase_system::.in_set(RenderSet::PhaseSort)) @@ -356,53 +355,46 @@ where let vertex_buffer_layout = layout.get_layout(&vertex_attributes)?; - // The fragment shader is only used when the normal prepass is enabled - // or the material uses alpha cutoff values and doesn't rely on the standard prepass shader - let fragment = if key.mesh_key.intersects(MeshPipelineKey::NORMAL_PREPASS | MeshPipelineKey::MOTION_VECTOR_PREPASS) - || ((key.mesh_key.contains(MeshPipelineKey::ALPHA_MASK) - || blend_key == MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA) - && self.material_fragment_shader.is_some()) - { - // Use the fragment shader from the material if present - let frag_shader_handle = if let Some(handle) = &self.material_fragment_shader { - handle.clone() - } else { - PREPASS_SHADER_HANDLE.typed::() - }; + // Setup prepass fragment targets - normals in slot 0 (or None if not needed), motion vectors in slot 1 + let mut targets = vec![]; + targets.push(key.mesh_key.contains(MeshPipelineKey::NORMAL_PREPASS).then(|| ColorTargetState { + format: NORMAL_PREPASS_FORMAT, + blend: Some(BlendState::REPLACE), + write_mask: ColorWrites::ALL, + }) + ); + + targets.push(key.mesh_key.contains(MeshPipelineKey::MOTION_VECTOR_PREPASS).then(|| ColorTargetState { + format: MOTION_VECTOR_PREPASS_FORMAT, + blend: Some(BlendState::REPLACE), + write_mask: ColorWrites::ALL, + })); - // Setup prepass fragment targets - normals in slot 0 (or None if not needed), motion vectors in slot 1 - let mut targets = vec![]; - if key.mesh_key.contains(MeshPipelineKey::NORMAL_PREPASS) { - targets.push(Some(ColorTargetState { - format: NORMAL_PREPASS_FORMAT, - blend: Some(BlendState::REPLACE), - write_mask: ColorWrites::ALL, - })); - } - if key - .mesh_key - .contains(MeshPipelineKey::MOTION_VECTOR_PREPASS) - { - if !key.mesh_key.contains(MeshPipelineKey::NORMAL_PREPASS) { - targets.push(None); - } + if targets.iter().all(Option::is_none) { + // if no targets are required then clear the list, so that no fragment shader is required + // (though one may still be used for discarding depth buffer writes) + targets.clear(); + } - targets.push(Some(ColorTargetState { - format: MOTION_VECTOR_PREPASS_FORMAT, - blend: Some(BlendState::REPLACE), - write_mask: ColorWrites::ALL, - })); - } + // The fragment shader is only used when the normal prepass or motion vectors prepass + // is enabled or the material uses alpha cutoff values + let fragment_required = key.mesh_key.intersects(MeshPipelineKey::ALPHA_MASK | MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA) + || !targets.is_empty(); - Some(FragmentState { - shader: frag_shader_handle, + let fragment = fragment_required.then(|| { + // Use the fragment shader from the material + let Some(frag_shader_handle) = &self.material_fragment_shader else { + // if a fragment shader is required and not provided, we can't continue + panic!("No prepass fragment shader provided for {}.", std::any::type_name::()); + }; + + FragmentState { + shader: frag_shader_handle.clone(), entry_point: "fragment".into(), shader_defs: shader_defs.clone(), targets, - }) - } else { - None - }; + } + }); // Use the vertex shader from the material if present let vert_shader_handle = if let Some(handle) = &self.material_vertex_shader { From 754192dc986253253758e22d64f64ec7164fc074 Mon Sep 17 00:00:00 2001 From: robtfm <50659922+robtfm@users.noreply.github.com> Date: Fri, 3 Mar 2023 17:44:25 +0000 Subject: [PATCH 09/11] center jitter at zero --- crates/bevy_core_pipeline/src/taa/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_core_pipeline/src/taa/mod.rs b/crates/bevy_core_pipeline/src/taa/mod.rs index 515d2afe9b22e..bc8256ec4a05f 100644 --- a/crates/bevy_core_pipeline/src/taa/mod.rs +++ b/crates/bevy_core_pipeline/src/taa/mod.rs @@ -484,7 +484,7 @@ fn prepare_taa_jitter( let offset = halton_sequence[frame_count.0 as usize % halton_sequence.len()]; for mut jitter in &mut query { - jitter.offset = offset; + jitter.offset = offset - 0.5; } } From a7b010389fb78fa35faedcb14b54f3968067624e Mon Sep 17 00:00:00 2001 From: robtfm <50659922+robtfm@users.noreply.github.com> Date: Fri, 3 Mar 2023 17:46:05 +0000 Subject: [PATCH 10/11] alpha-blend comment --- crates/bevy_core_pipeline/src/taa/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_core_pipeline/src/taa/mod.rs b/crates/bevy_core_pipeline/src/taa/mod.rs index bc8256ec4a05f..31e585ffcf5f4 100644 --- a/crates/bevy_core_pipeline/src/taa/mod.rs +++ b/crates/bevy_core_pipeline/src/taa/mod.rs @@ -136,6 +136,7 @@ pub struct TemporalAntialiasBundle { /// Cannot be used with [`bevy_render::camera::OrthographicProjection`]. /// /// Currently does not support skinned meshes. There will probably be ghosting artifacts if used with them. +/// Does not work well with alpha-blended meshes as it requires depth writing to determine motion. /// /// It is very important that correct motion vectors are written for everything on screen. /// Failure to do so will lead to ghosting artifacts. For instance, if particle effects From 17771e5dc62e06356d3f1a76ba0cf850d82fb686 Mon Sep 17 00:00:00 2001 From: robtfm <50659922+robtfm@users.noreply.github.com> Date: Fri, 3 Mar 2023 18:18:54 +0000 Subject: [PATCH 11/11] ci --- crates/bevy_core_pipeline/src/prepass/node.rs | 38 ++++++++++--------- crates/bevy_pbr/src/prepass/mod.rs | 33 ++++++++++------ 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/crates/bevy_core_pipeline/src/prepass/node.rs b/crates/bevy_core_pipeline/src/prepass/node.rs index e1382eafdee22..0e248b46866db 100644 --- a/crates/bevy_core_pipeline/src/prepass/node.rs +++ b/crates/bevy_core_pipeline/src/prepass/node.rs @@ -74,28 +74,32 @@ impl Node for PrepassNode { } let mut color_attachments = vec![]; - color_attachments.push(view_prepass_textures.normal.as_ref().map( |view_normals_texture| - RenderPassColorAttachment { - view: &view_normals_texture.default_view, + color_attachments.push( + view_prepass_textures + .normal + .as_ref() + .map(|view_normals_texture| RenderPassColorAttachment { + view: &view_normals_texture.default_view, + resolve_target: None, + ops: Operations { + load: LoadOp::Clear(Color::BLACK.into()), + store: true, + }, + }), + ); + + color_attachments.push(view_prepass_textures.motion_vectors.as_ref().map( + |view_motion_vectors_texture| RenderPassColorAttachment { + view: &view_motion_vectors_texture.default_view, resolve_target: None, ops: Operations { - load: LoadOp::Clear(Color::BLACK.into()), + // Blue channel dosen't matter, but set to 1.0 for possible faster clear + // https://gpuopen.com/performance/#clears + load: LoadOp::Clear(Color::rgb_linear(1.0, 1.0, 1.0).into()), store: true, }, - } - )); - - color_attachments.push(view_prepass_textures.motion_vectors.as_ref().map(|view_motion_vectors_texture| - RenderPassColorAttachment { - view: &view_motion_vectors_texture.default_view, - resolve_target: None, - ops: Operations { - // Blue channel dosen't matter, but set to 1.0 for possible faster clear - // https://gpuopen.com/performance/#clears - load: LoadOp::Clear(Color::rgb_linear(1.0, 1.0, 1.0).into()), - store: true, }, - })); + )); if color_attachments.iter().all(Option::is_none) { // all attachments are none: clear the attachment list so that no fragment shader is required diff --git a/crates/bevy_pbr/src/prepass/mod.rs b/crates/bevy_pbr/src/prepass/mod.rs index bcb79cd6afbeb..dabf615216953 100644 --- a/crates/bevy_pbr/src/prepass/mod.rs +++ b/crates/bevy_pbr/src/prepass/mod.rs @@ -357,18 +357,25 @@ where // Setup prepass fragment targets - normals in slot 0 (or None if not needed), motion vectors in slot 1 let mut targets = vec![]; - targets.push(key.mesh_key.contains(MeshPipelineKey::NORMAL_PREPASS).then(|| ColorTargetState { - format: NORMAL_PREPASS_FORMAT, - blend: Some(BlendState::REPLACE), - write_mask: ColorWrites::ALL, - }) + targets.push( + key.mesh_key + .contains(MeshPipelineKey::NORMAL_PREPASS) + .then_some(ColorTargetState { + format: NORMAL_PREPASS_FORMAT, + blend: Some(BlendState::REPLACE), + write_mask: ColorWrites::ALL, + }), + ); + + targets.push( + key.mesh_key + .contains(MeshPipelineKey::MOTION_VECTOR_PREPASS) + .then_some(ColorTargetState { + format: MOTION_VECTOR_PREPASS_FORMAT, + blend: Some(BlendState::REPLACE), + write_mask: ColorWrites::ALL, + }), ); - - targets.push(key.mesh_key.contains(MeshPipelineKey::MOTION_VECTOR_PREPASS).then(|| ColorTargetState { - format: MOTION_VECTOR_PREPASS_FORMAT, - blend: Some(BlendState::REPLACE), - write_mask: ColorWrites::ALL, - })); if targets.iter().all(Option::is_none) { // if no targets are required then clear the list, so that no fragment shader is required @@ -378,7 +385,9 @@ where // The fragment shader is only used when the normal prepass or motion vectors prepass // is enabled or the material uses alpha cutoff values - let fragment_required = key.mesh_key.intersects(MeshPipelineKey::ALPHA_MASK | MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA) + let fragment_required = key + .mesh_key + .intersects(MeshPipelineKey::ALPHA_MASK | MeshPipelineKey::BLEND_PREMULTIPLIED_ALPHA) || !targets.is_empty(); let fragment = fragment_required.then(|| {