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

Apply codebase changes in preparation for StandardMaterial transmission #8704

Merged
merged 14 commits into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/bevy_core_pipeline/src/bloom/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,9 @@ impl ViewNode for BloomNode {
BindGroupEntry {
binding: 0,
// Read from main texture directly
resource: BindingResource::TextureView(view_target.main_texture()),
resource: BindingResource::TextureView(
view_target.main_texture_view(),
),
},
BindGroupEntry {
binding: 1,
Expand Down
5 changes: 3 additions & 2 deletions crates/bevy_core_pipeline/src/prepass/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,10 @@ impl ViewNode for PrepassNode {
view: &view_motion_vectors_texture.default_view,
resolve_target: None,
ops: Operations {
// Blue channel doesn't matter, but set to 1.0 for possible faster clear
// 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
coreh marked this conversation as resolved.
Show resolved Hide resolved
// https://gpuopen.com/performance/#clears
load: LoadOp::Clear(Color::rgb_linear(1.0, 1.0, 1.0).into()),
load: LoadOp::Clear(Color::rgb_linear(0.0, 0.0, 0.0).into()),
store: true,
},
},
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/upscaling/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ impl ViewNode for UpscalingNode {
LoadOp::Clear(Default::default())
};

let upscaled_texture = target.main_texture();
let upscaled_texture = target.main_texture_view();

let mut cached_bind_group = self.cached_texture_bind_group.lock().unwrap();
let bind_group = match &mut *cached_bind_group {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/exclusive_function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, 16, F);
all_tuples!(impl_exclusive_system_function, 0, 17, F);
Copy link
Member

@alice-i-cecile alice-i-cecile May 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems out of place here and wasn't mentioned in the change log.

Copy link
Contributor Author

@coreh coreh May 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was also divided on wether or not to include this one. One of the systems I update in the other PR to add transmission ends up needing more than 16, but since it's technically a change unrelated to transmission I wasn't sure it would make sense to keep it there or here.

I didn't include it in the change log because it was minor and mostly internal/not user-facing, but I can also add a note about it. It's probably very obscure though, and users might be confused on why 17, and not, say, 32 (My understanding is that compile-time memory and CPU usage grows non-linearly with it, so we want to keep it low)

Copy link
Member

@alice-i-cecile alice-i-cecile May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that system should just use a custom (derived) SystemParam instead. That's the idiomatic way to handle the problem, so we should use it internally instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I haven't used that yet; I'll try looking into it. I agree just bumping the limit arbitrarily is probably not a good solution, especially since next time when we add another feature we'll just have to bump it again.

Copy link
Contributor

@nicopap nicopap May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you can also nest your parameters in tuples

fn my_system(
  (first_param, second_param): (Res<First>, ResMut<Second>),
  third_param: Query<((&Also, &Works), (&WithQuery, &Parameters))>,
  // ...
) {
  // ...
}

SystemParam is better, but if you don't want to bother with them, you can use tuples. Increasing this number generates a n² amount of code and slows down compile time disproportionately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooo, I really like this tuple-based solution, if y'all are okay with me using it instead of SystemParams, I'd very much prefer it, since SystemParams feels a bit verbose for this scenario.

2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/exclusive_system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,4 @@ macro_rules! impl_exclusive_system_param_tuple {
};
}

all_tuples!(impl_exclusive_system_param_tuple, 0, 16, P);
all_tuples!(impl_exclusive_system_param_tuple, 0, 17, P);
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, 16, F);
all_tuples!(impl_system_function, 0, 17, F);
2 changes: 1 addition & 1 deletion crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1410,7 +1410,7 @@ macro_rules! impl_system_param_tuple {
};
}

all_tuples!(impl_system_param_tuple, 0, 16, P);
all_tuples!(impl_system_param_tuple, 0, 17, P);

pub mod lifetimeless {
pub type SQuery<Q, F = ()> = super::Query<'static, 'static, Q, F>;
Expand Down
7 changes: 7 additions & 0 deletions crates/bevy_pbr/src/material.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use bevy_app::{App, Plugin};
use bevy_asset::{AddAsset, AssetEvent, AssetServer, Assets, Handle};
use bevy_core_pipeline::{
core_3d::{AlphaMask3d, Opaque3d, Transparent3d},
experimental::taa::TemporalAntiAliasSettings,
prepass::NormalPrepass,
tonemapping::{DebandDither, Tonemapping},
};
Expand Down Expand Up @@ -387,6 +388,7 @@ pub fn queue_material_meshes<M: Material>(
Option<&DebandDither>,
Option<&EnvironmentMapLight>,
Option<&NormalPrepass>,
Option<&TemporalAntiAliasSettings>,
&mut RenderPhase<Opaque3d>,
&mut RenderPhase<AlphaMask3d>,
&mut RenderPhase<Transparent3d>,
Expand All @@ -401,6 +403,7 @@ pub fn queue_material_meshes<M: Material>(
dither,
environment_map,
normal_prepass,
taa_settings,
mut opaque_phase,
mut alpha_mask_phase,
mut transparent_phase,
Expand All @@ -417,6 +420,10 @@ pub fn queue_material_meshes<M: Material>(
view_key |= MeshPipelineKey::NORMAL_PREPASS;
}

if taa_settings.is_some() {
view_key |= MeshPipelineKey::TAA;
}

let environment_map_loaded = match environment_map {
Some(environment_map) => environment_map.is_loaded(&images),
None => false,
Expand Down
13 changes: 9 additions & 4 deletions crates/bevy_pbr/src/render/fog.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// https://iquilezles.org/articles/fog/ (Atmospheric Fog and Scattering)

fn scattering_adjusted_fog_color(
fog: Fog,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the global fog uniform be renamed, so that in future changes to this code, we don't accidentally end up using the uniform instead of the argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up renaming the argument in the fog functions instead, to fog_params. Right now other uniforms have a 1:1 match between their binding name and type name, with only a difference in case, (e.g. point_lights: PointLights and cluster_offsets_and_counts: ClusterOffsetsAndCounts) so ideally I want to preserve that consistency.

scattering: vec3<f32>,
) -> vec4<f32> {
if (fog.directional_light_color.a > 0.0) {
Expand All @@ -20,45 +21,49 @@ fn scattering_adjusted_fog_color(
}

fn linear_fog(
fog: Fog,
input_color: vec4<f32>,
distance: f32,
scattering: vec3<f32>,
) -> vec4<f32> {
var fog_color = scattering_adjusted_fog_color(scattering);
var fog_color = scattering_adjusted_fog_color(fog, scattering);
let start = fog.be.x;
let end = fog.be.y;
fog_color.a *= 1.0 - clamp((end - distance) / (end - start), 0.0, 1.0);
return vec4<f32>(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a);
}

fn exponential_fog(
fog: Fog,
input_color: vec4<f32>,
distance: f32,
scattering: vec3<f32>,
) -> vec4<f32> {
var fog_color = scattering_adjusted_fog_color(scattering);
var fog_color = scattering_adjusted_fog_color(fog, scattering);
let density = fog.be.x;
fog_color.a *= 1.0 - 1.0 / exp(distance * density);
return vec4<f32>(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a);
}

fn exponential_squared_fog(
fog: Fog,
input_color: vec4<f32>,
distance: f32,
scattering: vec3<f32>,
) -> vec4<f32> {
var fog_color = scattering_adjusted_fog_color(scattering);
var fog_color = scattering_adjusted_fog_color(fog, scattering);
let distance_times_density = distance * fog.be.x;
fog_color.a *= 1.0 - 1.0 / exp(distance_times_density * distance_times_density);
return vec4<f32>(mix(input_color.rgb, fog_color.rgb, fog_color.a), input_color.a);
}

fn atmospheric_fog(
fog: Fog,
input_color: vec4<f32>,
distance: f32,
scattering: vec3<f32>,
) -> vec4<f32> {
var fog_color = scattering_adjusted_fog_color(scattering);
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);
return vec4<f32>(
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_pbr/src/render/mesh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,7 @@ bitflags::bitflags! {
const ALPHA_MASK = (1 << 6);
const ENVIRONMENT_MAP = (1 << 7);
const DEPTH_CLAMP_ORTHO = (1 << 8);
const TAA = (1 << 9);
const BLEND_RESERVED_BITS = Self::BLEND_MASK_BITS << Self::BLEND_SHIFT_BITS; // ← Bitmask reserving bits for the blend state
const BLEND_OPAQUE = (0 << Self::BLEND_SHIFT_BITS); // ← Values are just sequential within the mask, and can range from 0 to 3
const BLEND_PREMULTIPLIED_ALPHA = (1 << Self::BLEND_SHIFT_BITS); //
Expand Down Expand Up @@ -799,6 +800,10 @@ impl SpecializedMeshPipeline for MeshPipeline {
shader_defs.push("ENVIRONMENT_MAP".into());
}

if key.contains(MeshPipelineKey::TAA) {
shader_defs.push("TAA".into());
}

let format = if key.contains(MeshPipelineKey::HDR) {
ViewTarget::TEXTURE_FORMAT_HDR
} else {
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_pbr/src/render/pbr.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ fn fragment(in: FragmentInput) -> @location(0) vec4<f32> {

// fog
if (fog.mode != FOG_MODE_OFF && (material.flags & STANDARD_MATERIAL_FLAGS_FOG_ENABLED_BIT) != 0u) {
output_color = apply_fog(output_color, in.world_position.xyz, view.world_position.xyz);
output_color = apply_fog(fog, output_color, in.world_position.xyz, view.world_position.xyz);
}

#ifdef TONEMAP_IN_SHADER
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_pbr/src/render/pbr_functions.wgsl
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ fn pbr(
#endif // PREPASS_FRAGMENT

#ifndef PREPASS_FRAGMENT
fn apply_fog(input_color: vec4<f32>, fragment_world_position: vec3<f32>, view_world_position: vec3<f32>) -> vec4<f32> {
fn apply_fog(fog: Fog, input_color: vec4<f32>, fragment_world_position: vec3<f32>, view_world_position: vec3<f32>) -> vec4<f32> {
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
Expand All @@ -297,13 +297,13 @@ fn apply_fog(input_color: vec4<f32>, fragment_world_position: vec3<f32>, view_wo
}

if fog.mode == FOG_MODE_LINEAR {
return linear_fog(input_color, distance, scattering);
return linear_fog(fog, input_color, distance, scattering);
} else if fog.mode == FOG_MODE_EXPONENTIAL {
return exponential_fog(input_color, distance, scattering);
return exponential_fog(fog, input_color, distance, scattering);
} else if fog.mode == FOG_MODE_EXPONENTIAL_SQUARED {
return exponential_squared_fog(input_color, distance, scattering);
return exponential_squared_fog(fog, input_color, distance, scattering);
} else if fog.mode == FOG_MODE_ATMOSPHERIC {
return atmospheric_fog(input_color, distance, scattering);
return atmospheric_fog(fog, input_color, distance, scattering);
} else {
return input_color;
}
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_pbr/src/render/utils.wgsl
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#define_import_path bevy_pbr::utils

const PI: f32 = 3.141592653589793;
const E: f32 = 2.718281828459045;

fn hsv2rgb(hue: f32, saturation: f32, value: f32) -> vec3<f32> {
let rgb = clamp(
Expand Down
23 changes: 23 additions & 0 deletions crates/bevy_render/src/render_phase/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,29 @@ impl<I: PhaseItem> RenderPhase<I> {
draw_function.draw(world, render_pass, view, item);
}
}

/// 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>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we want some validation here to check that the range is valid.

Also, remind me why render_range() is useful for transmission?

Copy link
Contributor Author

@coreh coreh May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we want some validation here to check that the range is valid.

The updated code with .get(range) now has validation.

Also, remind me why render_range() is useful for transmission?

We use it to implement transmissive "steps", to achieve more than one layer of transparency for transmission. The items are sorted far-to-near, then split into roughly equal ranges and drawn parceled out, with a texture copy between each draw. This is kinda expensive so we default to a single step (equivalent to not using render_range()) but you can configure it via Camera3d if higher fidelity is required.

Note that there are probably better ways to do this range division, including by determining which items' bounds overlap in screen space, making the steps exponentially spaced (so that near objects get more steps) or prioritizing based on roughness, transmission, and screen space coverage, however to start I just went for the most naive approach.

&self,
render_pass: &mut TrackedRenderPass<'w>,
world: &'w World,
view: Entity,
range: Range<usize>,
) {
let draw_functions = world.resource::<DrawFunctions<I>>();
let mut draw_functions = draw_functions.write();
draw_functions.prepare(world);

for item in self
.items
.iter()
.skip(range.start)
.take(range.end - range.start)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

item is Vec here, so this should work. The difference being this skips when range is partially outside of items. (say items.len() == 3 and range = 2..5)

Suggested change
for item in self
.items
.iter()
.skip(range.start)
.take(range.end - range.start)
for item in self.items.get(range).iter().flatten()

Copy link
Contributor Author

@coreh coreh May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't get that to work verbatim because of the nested reference in the type, so I ended up adding a .expect() instead, which will panic if you use an out-of-bounds range, which might also be less confusing to debug than just silently ignoring the range if it's out of bounds.

{
let draw_function = draw_functions.get_mut(item.draw_function()).unwrap();
draw_function.draw(world, render_pass, view, item);
}
}
}

impl<I: BatchedPhaseItem> RenderPhase<I> {
Expand Down
39 changes: 36 additions & 3 deletions crates/bevy_render/src/texture/fallback_image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,21 @@ 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 blending 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);

/// A [`RenderApp`](crate::RenderApp) resource that contains a _zero-filled_ "fallback image",
/// 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
/// or alpha-blending it to other colors a no-op.
#[derive(Resource, Deref)]
pub struct FallbackImageZero(GpuImage);
Copy link
Contributor

@nicopap nicopap May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using "WhiteFallbackImage" and "TransparentBlackFallbackImage"? Though I understand why you'd want to keep the current names over the ones I'm suggesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... I tried out a bunch of different names/orders, but they all felt weird. I didn't want to rename the existing FallbackImage needlessly since that would introduce a breaking change for no good reason, and since there was already FallbackImageCubemap (with the “qualifier” at the end) I just went with the same pattern for consistency.

I can rename all of them if there's significant demand, but since we might want to be able to (eventually) create arbitrary-color fallbacks, maybe we can save the breaking change for then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine as-is for now


/// A [`RenderApp`](crate::RenderApp) resource that contains a "cubemap 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.
Expand All @@ -34,9 +45,10 @@ fn fallback_image_new(
format: TextureFormat,
dimension: TextureViewDimension,
samples: u32,
value: u8,
) -> GpuImage {
// TODO make this configurable
let data = vec![255; format.pixel_size()];
// TODO make this configurable per channel
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved
let data = vec![value; format.pixel_size()];

let extents = Extent3d {
width: 1,
Expand Down Expand Up @@ -92,6 +104,24 @@ impl FromWorld for FallbackImage {
TextureFormat::bevy_default(),
TextureViewDimension::D2,
1,
255,
))
}
}

impl FromWorld for FallbackImageZero {
fn from_world(world: &mut bevy_ecs::prelude::World) -> Self {
let render_device = world.resource::<RenderDevice>();
let render_queue = world.resource::<RenderQueue>();
let default_sampler = world.resource::<DefaultImageSampler>();
Self(fallback_image_new(
render_device,
render_queue,
default_sampler,
TextureFormat::bevy_default(),
TextureViewDimension::D2,
1,
0,
))
}
}
Expand All @@ -108,6 +138,7 @@ impl FromWorld for FallbackImageCubemap {
TextureFormat::bevy_default(),
TextureViewDimension::Cube,
1,
255,
))
}
}
Expand Down Expand Up @@ -148,6 +179,7 @@ impl<'w> FallbackImagesMsaa<'w> {
TextureFormat::bevy_default(),
TextureViewDimension::D2,
sample_count,
255,
)
})
}
Expand All @@ -171,6 +203,7 @@ impl<'w> FallbackImagesDepth<'w> {
TextureFormat::Depth32Float,
TextureViewDimension::D2,
sample_count,
255,
)
})
}
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_render/src/texture/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ impl Plugin for ImagePlugin {
render_app
.insert_resource(DefaultImageSampler(default_sampler))
.init_resource::<FallbackImage>()
.init_resource::<FallbackImageZero>()
.init_resource::<FallbackImageCubemap>()
.init_resource::<FallbackImageMsaaCache>()
.init_resource::<FallbackImageDepthCache>();
Expand Down
Loading