From cec61c9aaa5ffc445290a692d5b358d73f1520ce Mon Sep 17 00:00:00 2001 From: Marco Buono Date: Sun, 28 May 2023 19:19:01 -0300 Subject: [PATCH 1/5] Apply PR feedback --- crates/bevy_core_pipeline/src/prepass/node.rs | 1 + crates/bevy_render/src/render_phase/mod.rs | 2 +- crates/bevy_render/src/texture/fallback_image.rs | 6 ++++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/bevy_core_pipeline/src/prepass/node.rs b/crates/bevy_core_pipeline/src/prepass/node.rs index 06c09fa4b1..6d3502212d 100644 --- a/crates/bevy_core_pipeline/src/prepass/node.rs +++ b/crates/bevy_core_pipeline/src/prepass/node.rs @@ -67,6 +67,7 @@ impl ViewNode for PrepassNode { view: &view_motion_vectors_texture.default_view, resolve_target: None, ops: Operations { + // Red and Green channels are X and Y components of the motion vectors // Blue channel doesn't matter, but set to 0.0 for possible faster clear // https://gpuopen.com/performance/#clears load: LoadOp::Clear(Color::rgb_linear(0.0, 0.0, 0.0).into()), diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index 1ea9f16245..7c5348ceaa 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -90,7 +90,7 @@ impl RenderPhase { } } - /// Renders a range of the [`PhaseItem`]s using their corresponding draw functions. + /// Renders all [`PhaseItem`]s in the provided `range` (based on their index in `self.items`) using their corresponding draw functions. pub fn render_range<'w>( &self, render_pass: &mut TrackedRenderPass<'w>, diff --git a/crates/bevy_render/src/texture/fallback_image.rs b/crates/bevy_render/src/texture/fallback_image.rs index 59688492bc..dedb266343 100644 --- a/crates/bevy_render/src/texture/fallback_image.rs +++ b/crates/bevy_render/src/texture/fallback_image.rs @@ -17,7 +17,9 @@ use crate::{ /// A [`RenderApp`](crate::RenderApp) resource that contains the default "fallback image", /// which can be used in situations where an image was not explicitly defined. The most common /// use case is [`AsBindGroup`] implementations (such as materials) that support optional textures. -/// [`FallbackImage`] defaults to a 1x1 fully white texture, making multiplying colors with it a no-op. +/// +/// Defaults to a 1x1 fully opaque white texture, (1.0, 1.0, 1.0, 1.0) which makes multiplying +/// it with other colors a no-op. #[derive(Resource, Deref)] pub struct FallbackImage(GpuImage); @@ -25,7 +27,7 @@ pub struct FallbackImage(GpuImage); /// which can be used in place of [`FallbackImage`], when a fully transparent or black fallback /// is required instead of fully opaque white. /// -/// Defaults to a 1x1 fully transparent black texture (0.0, 0.0, 0.0, 0.0), which makes adding +/// Defaults to a 1x1 fully transparent black texture, (0.0, 0.0, 0.0, 0.0) which makes adding /// or alpha-blending it to other colors a no-op. #[derive(Resource, Deref)] pub struct FallbackImageZero(GpuImage); From 0aac2724b9ea7dd959b5d35d2a27cf12e4821893 Mon Sep 17 00:00:00 2001 From: Marco Buono Date: Mon, 29 May 2023 18:48:29 -0300 Subject: [PATCH 2/5] Revert "Bump system tuple param limits" This reverts commit b6e564acac62fd96909e72df0794ad55df9f6831. --- crates/bevy_ecs/src/system/exclusive_function_system.rs | 2 +- crates/bevy_ecs/src/system/exclusive_system_param.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 2 +- crates/bevy_ecs/src/system/system_param.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 2654bf393c..c1c4bff3a2 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -237,4 +237,4 @@ macro_rules! impl_exclusive_system_function { } // Note that we rely on the highest impl to be <= the highest order of the tuple impls // of `SystemParam` created. -all_tuples!(impl_exclusive_system_function, 0, 17, F); +all_tuples!(impl_exclusive_system_function, 0, 16, F); diff --git a/crates/bevy_ecs/src/system/exclusive_system_param.rs b/crates/bevy_ecs/src/system/exclusive_system_param.rs index cb0230e6c1..9f7ccb5ae9 100644 --- a/crates/bevy_ecs/src/system/exclusive_system_param.rs +++ b/crates/bevy_ecs/src/system/exclusive_system_param.rs @@ -86,4 +86,4 @@ macro_rules! impl_exclusive_system_param_tuple { }; } -all_tuples!(impl_exclusive_system_param_tuple, 0, 17, P); +all_tuples!(impl_exclusive_system_param_tuple, 0, 16, P); diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 0a3509fbc0..27593cfef9 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -629,4 +629,4 @@ macro_rules! impl_system_function { // Note that we rely on the highest impl to be <= the highest order of the tuple impls // of `SystemParam` created. -all_tuples!(impl_system_function, 0, 17, F); +all_tuples!(impl_system_function, 0, 16, F); diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 39c1ae5b29..ea35eb06c8 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -1410,7 +1410,7 @@ macro_rules! impl_system_param_tuple { }; } -all_tuples!(impl_system_param_tuple, 0, 17, P); +all_tuples!(impl_system_param_tuple, 0, 16, P); pub mod lifetimeless { pub type SQuery = super::Query<'static, 'static, Q, F>; From 6103ec084a6102fa4dfcf2b300b6514e4fb2ff62 Mon Sep 17 00:00:00 2001 From: Marco Buono Date: Mon, 29 May 2023 19:15:05 -0300 Subject: [PATCH 3/5] Rename argument to avoid accidental confusion with uniform --- crates/bevy_pbr/src/render/fog.wgsl | 40 +++++++++---------- crates/bevy_pbr/src/render/pbr_functions.wgsl | 22 +++++----- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/crates/bevy_pbr/src/render/fog.wgsl b/crates/bevy_pbr/src/render/fog.wgsl index 6d52487c56..e6c1416d93 100644 --- a/crates/bevy_pbr/src/render/fog.wgsl +++ b/crates/bevy_pbr/src/render/fog.wgsl @@ -6,66 +6,66 @@ // https://iquilezles.org/articles/fog/ (Atmospheric Fog and Scattering) fn scattering_adjusted_fog_color( - fog: Fog, + fog_params: Fog, scattering: vec3, ) -> vec4 { - if (fog.directional_light_color.a > 0.0) { + if (fog_params.directional_light_color.a > 0.0) { return vec4( - fog.base_color.rgb - + scattering * fog.directional_light_color.rgb * fog.directional_light_color.a, - fog.base_color.a, + fog_params.base_color.rgb + + scattering * fog_params.directional_light_color.rgb * fog_params.directional_light_color.a, + fog_params.base_color.a, ); } else { - return fog.base_color; + return fog_params.base_color; } } fn linear_fog( - fog: Fog, + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(fog, scattering); - let start = fog.be.x; - let end = fog.be.y; + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let start = fog_params.be.x; + let end = fog_params.be.y; fog_color.a *= 1.0 - clamp((end - distance) / (end - start), 0.0, 1.0); return vec4(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a); } fn exponential_fog( - fog: Fog, + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(fog, scattering); - let density = fog.be.x; + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let density = fog_params.be.x; fog_color.a *= 1.0 - 1.0 / exp(distance * density); return vec4(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a); } fn exponential_squared_fog( - fog: Fog, + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(fog, scattering); - let distance_times_density = distance * fog.be.x; + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let distance_times_density = distance * fog_params.be.x; fog_color.a *= 1.0 - 1.0 / exp(distance_times_density * distance_times_density); return vec4(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a); } fn atmospheric_fog( - fog: Fog, + fog_params: Fog, input_color: vec4, distance: f32, scattering: vec3, ) -> vec4 { - var fog_color = scattering_adjusted_fog_color(fog, scattering); - let extinction_factor = 1.0 - 1.0 / exp(distance * fog.be); - let inscattering_factor = 1.0 - 1.0 / exp(distance * fog.bi); + var fog_color = scattering_adjusted_fog_color(fog_params, scattering); + let extinction_factor = 1.0 - 1.0 / exp(distance * fog_params.be); + let inscattering_factor = 1.0 - 1.0 / exp(distance * fog_params.bi); return vec4( input_color.rgb * (1.0 - extinction_factor * fog_color.a) + fog_color.rgb * inscattering_factor * fog_color.a, diff --git a/crates/bevy_pbr/src/render/pbr_functions.wgsl b/crates/bevy_pbr/src/render/pbr_functions.wgsl index 5fed239bbe..b1306be97e 100644 --- a/crates/bevy_pbr/src/render/pbr_functions.wgsl +++ b/crates/bevy_pbr/src/render/pbr_functions.wgsl @@ -271,7 +271,7 @@ fn pbr( #endif // PREPASS_FRAGMENT #ifndef PREPASS_FRAGMENT -fn apply_fog(fog: Fog, input_color: vec4, fragment_world_position: vec3, view_world_position: vec3) -> vec4 { +fn apply_fog(fog_params: Fog, input_color: vec4, fragment_world_position: vec3, view_world_position: vec3) -> vec4 { let view_to_world = fragment_world_position.xyz - view_world_position.xyz; // `length()` is used here instead of just `view_to_world.z` since that produces more @@ -281,7 +281,7 @@ fn apply_fog(fog: Fog, input_color: vec4, fragment_world_position: vec3(0.0); - if fog.directional_light_color.a > 0.0 { + if fog_params.directional_light_color.a > 0.0 { let view_to_world_normalized = view_to_world / distance; let n_directional_lights = lights.n_directional_lights; for (var i: u32 = 0u; i < n_directional_lights; i = i + 1u) { @@ -291,19 +291,19 @@ fn apply_fog(fog: Fog, input_color: vec4, fragment_world_position: vec3 Date: Mon, 29 May 2023 19:34:28 -0300 Subject: [PATCH 4/5] Use `Option` type, making invalid states not representable --- crates/bevy_render/src/view/mod.rs | 67 ++++++++++++++++-------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/crates/bevy_render/src/view/mod.rs b/crates/bevy_render/src/view/mod.rs index aea1202aeb..e3c66a5519 100644 --- a/crates/bevy_render/src/view/mod.rs +++ b/crates/bevy_render/src/view/mod.rs @@ -13,7 +13,7 @@ use crate::{ render_phase::ViewRangefinder3d, render_resource::{DynamicUniformBuffer, ShaderType, Texture, TextureView}, renderer::{RenderDevice, RenderQueue}, - texture::{BevyDefault, TextureCache}, + texture::{BevyDefault, CachedTexture, TextureCache}, Render, RenderApp, RenderSet, }; use bevy_app::{App, Plugin}; @@ -205,8 +205,11 @@ impl ViewTarget { /// Retrieve this target's color attachment. This will use [`Self::sampled_main_texture_view`] and resolve to [`Self::main_texture`] if /// the target has sampling enabled. Otherwise it will use [`Self::main_texture`] directly. pub fn get_color_attachment(&self, ops: Operations) -> RenderPassColorAttachment { - match &self.main_textures.sampled_view { - Some(sampled_texture_view) => RenderPassColorAttachment { + match &self.main_textures.sampled { + Some(CachedTexture { + default_view: sampled_texture_view, + .. + }) => RenderPassColorAttachment { view: sampled_texture_view, resolve_target: Some(self.main_texture_view()), ops, @@ -230,9 +233,9 @@ impl ViewTarget { /// The "main" unsampled texture. pub fn main_texture(&self) -> &Texture { if self.main_texture.load(Ordering::SeqCst) == 0 { - &self.main_textures.a + &self.main_textures.a.texture } else { - &self.main_textures.b + &self.main_textures.b.texture } } @@ -244,18 +247,18 @@ impl ViewTarget { /// ahead of time. pub fn main_texture_other(&self) -> &Texture { if self.main_texture.load(Ordering::SeqCst) == 0 { - &self.main_textures.b + &self.main_textures.b.texture } else { - &self.main_textures.a + &self.main_textures.a.texture } } /// The "main" unsampled texture. pub fn main_texture_view(&self) -> &TextureView { if self.main_texture.load(Ordering::SeqCst) == 0 { - &self.main_textures.a_view + &self.main_textures.a.default_view } else { - &self.main_textures.b_view + &self.main_textures.b.default_view } } @@ -267,20 +270,26 @@ impl ViewTarget { /// ahead of time. pub fn main_texture_other_view(&self) -> &TextureView { if self.main_texture.load(Ordering::SeqCst) == 0 { - &self.main_textures.b_view + &self.main_textures.b.default_view } else { - &self.main_textures.a_view + &self.main_textures.a.default_view } } /// The "main" sampled texture. pub fn sampled_main_texture(&self) -> Option<&Texture> { - self.main_textures.sampled.as_ref() + self.main_textures + .sampled + .as_ref() + .map(|sampled| &sampled.texture) } /// The "main" sampled texture view. pub fn sampled_main_texture_view(&self) -> Option<&TextureView> { - self.main_textures.sampled_view.as_ref() + self.main_textures + .sampled + .as_ref() + .map(|sampled| &sampled.default_view) } #[inline] @@ -318,13 +327,13 @@ impl ViewTarget { // if the old main texture is a, then the post processing must write from a to b if old_is_a_main_texture == 0 { PostProcessWrite { - source: &self.main_textures.a_view, - destination: &self.main_textures.b_view, + source: &self.main_textures.a.default_view, + destination: &self.main_textures.b.default_view, } } else { PostProcessWrite { - source: &self.main_textures.b_view, - destination: &self.main_textures.a_view, + source: &self.main_textures.b.default_view, + destination: &self.main_textures.a.default_view, } } } @@ -385,12 +394,9 @@ pub fn prepare_view_uniforms( #[derive(Clone)] struct MainTargetTextures { - a: Texture, - a_view: TextureView, - b: Texture, - b_view: TextureView, - sampled: Option, - sampled_view: Option, + a: CachedTexture, + b: CachedTexture, + sampled: Option, /// 0 represents `main_textures.a`, 1 represents `main_textures.b` /// This is shared across view targets with the same render target main_texture: Arc, @@ -458,7 +464,7 @@ fn prepare_view_targets( ..descriptor }, ); - let (sampled_texture, sampled_view) = if msaa.samples() > 1 { + let sampled = if msaa.samples() > 1 { let sampled = texture_cache.get( &render_device, TextureDescriptor { @@ -472,17 +478,14 @@ fn prepare_view_targets( view_formats: descriptor.view_formats, }, ); - (Some(sampled.texture), Some(sampled.default_view)) + Some(sampled) } else { - (None, None) + None }; MainTargetTextures { - a: a.texture, - a_view: a.default_view, - b: b.texture, - b_view: b.default_view, - sampled: sampled_texture, - sampled_view, + a, + b, + sampled, main_texture: Arc::new(AtomicUsize::new(0)), } }); From 2106a53e9e9527e5bca8e8fe2552c4c3c52beaab Mon Sep 17 00:00:00 2001 From: Marco Buono Date: Mon, 29 May 2023 19:54:33 -0300 Subject: [PATCH 5/5] Do not silently ignore out of bounds ranges --- crates/bevy_render/src/render_phase/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_render/src/render_phase/mod.rs b/crates/bevy_render/src/render_phase/mod.rs index 7c5348ceaa..64a66e7a53 100644 --- a/crates/bevy_render/src/render_phase/mod.rs +++ b/crates/bevy_render/src/render_phase/mod.rs @@ -104,9 +104,9 @@ impl RenderPhase { for item in self .items + .get(range) + .expect("`Range` provided to `render_range()` is out of bounds") .iter() - .skip(range.start) - .take(range.end - range.start) { let draw_function = draw_functions.get_mut(item.draw_function()).unwrap(); draw_function.draw(world, render_pass, view, item);