Apply codebase changes in preparation for `StandardMaterial` transmission #8704

coreh · 2023-05-28T21:19:52Z

Objective

Make StandardMaterial Light Transmission #8015 easier to review;

Solution

This commit contains changes not directly related to transmission required by StandardMaterial Light Transmission #8015, in easier-to-review, one-change-per-commit form.

Changelog

Fixed

Clear motion vector prepass using 0.0 instead of 1.0, to avoid TAA artifacts on transparent objects against the background;

Added

The E mathematical constant is now available for use in shaders, exposed under bevy_pbr::utils;
A new TAA shader def is now available, for conditionally enabling shader logic via #ifdef when TAA is enabled; (e.g. for jittering texture samples)
A new FallbackImageZero resource is introduced, for when a fallback image filled with zeroes is required;
A new RenderPhase<I>::render_range() method is introduced, for render phases that need to render their items in multiple parceled out “steps”;

Changed

The MainTargetTextures struct now holds both Texture and TextureViews for the main textures;
The fog shader functions under bevy_pbr::fog now take the a Fog structure as their first argument, instead of relying on the global fog uniform;
The main textures can now be used as copy sources;

Migration Guide

ViewTarget::main_texture() and ViewTarget::main_texture_other() now return &Texture instead of &TextureView. If you were relying on these methods, replace your usage with ViewTarget::main_texture_view()and ViewTarget::main_texture_other_view(), respectively;
ViewTarget::sampled_main_texture() now returns Option<&Texture> instead of a Option<&TextureView>. If you were relying on this method, replace your usage with ViewTarget::sampled_main_texture_view();
The apply_fog(), linear_fog(), exponential_fog(), exponential_squared_fog() and atmospheric_fog() functions now take a configurable Fog struct. If you were relying on them, update your usage by adding the global fog uniform as their first argument;

alice-i-cecile · 2023-05-28T21:40:44Z

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, 16, F);
+all_tuples!(impl_exclusive_system_function, 0, 17, F);


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

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)

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.

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.

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.

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.

crates/bevy_core_pipeline/src/prepass/node.rs

crates/bevy_render/src/render_phase/mod.rs

crates/bevy_render/src/texture/fallback_image.rs

nicopap · 2023-05-29T05:08:03Z

crates/bevy_render/src/render_phase/mod.rs

+        for item in self
+            .items
+            .iter()
+            .skip(range.start)
+            .take(range.end - range.start)


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()

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.

nicopap

The only blocker for me is incrementing the size of trait impl tuple generation, it should stay at 16. Otherwise, all the other changes looks high quality (with some non-blocking reservations)

nicopap · 2023-05-29T05:14:45Z

crates/bevy_render/src/texture/fallback_image.rs

+/// 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);


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.

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?

I think it's fine as-is for now

nicopap · 2023-05-29T05:18:44Z

crates/bevy_pbr/src/render/fog.wgsl

@@ -6,6 +6,7 @@
 // https://iquilezles.org/articles/fog/ (Atmospheric Fog and Scattering)

 fn scattering_adjusted_fog_color(
+    fog: Fog,


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?

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.

nicopap · 2023-05-29T05:23:40Z

crates/bevy_render/src/view/mod.rs

+    sampled: Option<Texture>,
+    sampled_view: Option<TextureView>,


This invites possibly invalid state. (one is Some, the other None). I'd suggest defining a TextureAndView then you'd have

struct TextureAndView { texture: Texture, view: TextureView } struct MainTargetTextures { a: TextureAndView, b: TextureAndView, sampled: Option<TextureAndView>, }

Ended up reusing CachedTexture which is pretty much that, and in fact it's what the methods we use to set up the texture return us, that way the setup code is also shorter/cleaner.

Agreed, in my DLSS fork which needs the Texture, I have the same changes of making this hold a CachedTexture.

nicopap · 2023-05-29T05:26:00Z

crates/bevy_render/src/view/mod.rs

        if self.main_texture.load(Ordering::SeqCst) == 0 {
            &self.main_textures.b
        } else {
            &self.main_textures.a
        }
    }

+    /// The "main" unsampled texture.


IMO this should explain the difference with main_texture. It's surprising as a user to see two seemingly identical functions with the same documentation that returns two different types. What should I pick? I think a view is a subset of the texture right? Why would I use the Texture version over this one?

I don't think it's really necessary. It's just a wgpu thing - texture views are for binding, textures are for copy operations mostly and not much else.

I actually don't fully understand the distinction either, except that some APIs require Texture and others require TextureView. I guess it probably becomes more useful when you have an atlas or would like to "slice" portions of your texture for something, but I don't understand why most calls can't take a TextureView directly instead of a Texture. copy_texture_to_texture() is especially weird given that it takes a texture and a region, which could be better served by a TextureView instead?

TextureView lets you do things like bind specific mip levels of a texture separately. They also correspond to different lower-level DX/VK/Metal objects, which I imagine is the real reason why WebGPU has both.

Why not just return CachedTexture and let users pick what they need at the callsite?

This reverts commit b6e564a.

…able

coreh · 2023-05-30T00:17:53Z

@alice-i-cecile @nicopap Reverted the tuple size bump, and applied the feedback from the reviews.

JMS55 · 2023-05-29T22:57:42Z

crates/bevy_render/src/view/mod.rs

        if self.main_texture.load(Ordering::SeqCst) == 0 {
            &self.main_textures.b
        } else {
            &self.main_textures.a
        }
    }

+    /// The "main" unsampled texture.


I don't think it's really necessary. It's just a wgpu thing - texture views are for binding, textures are for copy operations mostly and not much else.

JMS55 · 2023-05-29T22:58:29Z

crates/bevy_render/src/view/mod.rs

+    sampled: Option<Texture>,
+    sampled_view: Option<TextureView>,


Agreed, in my DLSS fork which needs the Texture, I have the same changes of making this hold a CachedTexture.

JMS55 · 2023-05-29T22:59:54Z

crates/bevy_render/src/texture/fallback_image.rs

+/// 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);


I think it's fine as-is for now

JMS55 · 2023-05-29T23:01:47Z

crates/bevy_render/src/render_phase/mod.rs

@@ -89,6 +89,29 @@ impl<I: PhaseItem> RenderPhase<I> {
            draw_function.draw(world, render_pass, view, item);
        }
    }
+
+    /// Renders a range of the [`PhaseItem`]s using their corresponding draw functions.
+    pub fn render_range<'w>(


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

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

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.

crates/bevy_render/src/texture/fallback_image.rs

IceSentry

LGTM

A bunch of nice and simple improvements.

coreh added 9 commits May 28, 2023 17:04

Hold both TextureViews and Textures on MainTargetTextures

0a47196

Clear motion vector to 0.0, fixing TAA artifacts against the background

eadb217

Expose TAA as a shader def

348e2ce

Introduce FallbackImageZero

a12419d

Add RenderPhase<I>::render_range() method

77b94e4

Take fog as an argument instead of relying on fog uniform

a39364f

Define E constant in bevy_pbr::utils

b5ddbd7

Allow usage of main textures as copy sources

ac820b4

Bump system tuple param limits

b6e564a

alice-i-cecile reviewed May 28, 2023

View reviewed changes

crates/bevy_core_pipeline/src/prepass/node.rs Show resolved Hide resolved

alice-i-cecile reviewed May 28, 2023

View reviewed changes

crates/bevy_render/src/render_phase/mod.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 28, 2023

View reviewed changes

crates/bevy_render/src/texture/fallback_image.rs Outdated Show resolved Hide resolved

Apply PR feedback

cec61c9

nicopap reviewed May 29, 2023

View reviewed changes

nicopap suggested changes May 29, 2023

View reviewed changes

coreh added 4 commits May 29, 2023 18:48

Revert "Bump system tuple param limits"

0aac272

This reverts commit b6e564a.

Rename argument to avoid accidental confusion with uniform

6103ec0

Use Option<CachedTexture> type, making invalid states not represent…

e61f78d

…able

Do not silently ignore out of bounds ranges

2106a53

coreh mentioned this pull request May 29, 2023

StandardMaterial Light Transmission #8015

Merged

20 tasks

JMS55 suggested changes May 30, 2023

View reviewed changes

JMS55 approved these changes May 30, 2023

View reviewed changes

IceSentry reviewed May 30, 2023

View reviewed changes

crates/bevy_render/src/texture/fallback_image.rs Show resolved Hide resolved

IceSentry approved these changes May 30, 2023

View reviewed changes

nicopap approved these changes May 30, 2023

View reviewed changes

nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 30, 2023

alice-i-cecile added this pull request to the merge queue May 30, 2023

Merged via the queue into bevyengine:main with commit 292e069 May 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply codebase changes in preparation for `StandardMaterial` transmission #8704

Apply codebase changes in preparation for `StandardMaterial` transmission #8704

coreh commented May 28, 2023

alice-i-cecile May 28, 2023 •

edited

Loading

coreh May 28, 2023 •

edited

Loading

alice-i-cecile May 29, 2023 •

edited

Loading

coreh May 29, 2023

nicopap May 29, 2023 •

edited

Loading

coreh May 29, 2023

nicopap May 29, 2023

coreh May 29, 2023 •

edited

Loading

nicopap left a comment

nicopap May 29, 2023 •

edited

Loading

coreh May 29, 2023

JMS55 May 29, 2023

nicopap May 29, 2023

coreh May 29, 2023

nicopap May 29, 2023

coreh May 29, 2023

JMS55 May 29, 2023

nicopap May 29, 2023

JMS55 May 29, 2023

coreh May 30, 2023 •

edited

Loading

JMS55 May 30, 2023

IceSentry May 30, 2023

coreh commented May 30, 2023

JMS55 May 29, 2023

JMS55 May 29, 2023

JMS55 May 29, 2023

JMS55 May 29, 2023

coreh May 30, 2023 •

edited

Loading

IceSentry left a comment

Apply codebase changes in preparation for StandardMaterial transmission #8704

Apply codebase changes in preparation for StandardMaterial transmission #8704

Conversation

coreh commented May 28, 2023

Objective

Solution

Changelog

Fixed

Added

Changed

Migration Guide

alice-i-cecile May 28, 2023 • edited Loading

Choose a reason for hiding this comment

coreh May 28, 2023 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicopap May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreh May 29, 2023 • edited Loading

Choose a reason for hiding this comment

nicopap left a comment

Choose a reason for hiding this comment

nicopap May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreh May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreh commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreh May 30, 2023 • edited Loading

Choose a reason for hiding this comment

IceSentry left a comment

Choose a reason for hiding this comment

Apply codebase changes in preparation for `StandardMaterial` transmission #8704

Apply codebase changes in preparation for `StandardMaterial` transmission #8704

alice-i-cecile May 28, 2023 •

edited

Loading

coreh May 28, 2023 •

edited

Loading

alice-i-cecile May 29, 2023 •

edited

Loading

nicopap May 29, 2023 •

edited

Loading

coreh May 29, 2023 •

edited

Loading

nicopap May 29, 2023 •

edited

Loading

coreh May 30, 2023 •

edited

Loading

coreh May 30, 2023 •

edited

Loading