Add HDR and bloom to Pipelined rendering #2876

ChangeCaps · 2021-09-26T23:06:50Z

Objective

Adds HDR and bloom to pipelined rendering.

Solution

Change MainPassDriverNode to accept an HDR target for 3d

Add HdrTextureNode that outputs an HDR texture with the same size as the swap chain texture

Remove tone mapping from pbr shader

Add BloomNode that applies bloom to an HDR texture using down sampling and up samling
same method as described in (https://www.youtube.com/watch?v=tI70-HIc5ro).

Add BloomSettings resource to allow for easy modification of the bloom effect.

Add ToneMapping node that combines the HDR target with the swap chain texture while also applying
tone mapping (ACES).

Add example to show off the bloom effect

pipelined/bevy_hdr/Cargo.toml

pipelined/bevy_hdr/src/tone_mapping.wgsl

mtsr · 2021-09-27T09:22:06Z

I’d love to see some contrast in the example. Maybe adding a foreground object that’s much less brightly lit that partially occludes on of the spheres.

mtsr

Thanks for the awesome PR!

Is it desirable to make the HDR target and the bloom pass fully optional and if so, is there an ergonomic way to achieve it?

pipelined/bevy_core_pipeline/Cargo.toml

pipelined/bevy_hdr/src/bloom.wgsl

mtsr · 2021-09-27T09:34:55Z

pipelined/bevy_hdr/src/bloom.wgsl

+
+	let scale = texel_size;
+
+	let a = textureSample(org, sampler, in.uv + vec2<f32>(-1.0, -1.0) * scale);


There’s a lot of texture fetches, which if I understand correctly is one of the more expensive operations. Did you consider any other approaches and if so can you say something about the trade-offs and why you settled on this approach?

I just went with the implementation detailed in the video. The reasons given there are the following:
"the overlapping further breaks the grid nature of pyramid approaches"
"Works better than comparable sized filters"
"Eliminates pulsating artifacts and temporal stability issues"

That being said, it is probably worth considering implementing a way to choose a cheaper filter if desired.
I personally don't think it's necessary since the filters are being run on comparatively small textures.

CptPotato · 2021-09-27T14:09:39Z

pipelined/bevy_hdr/src/tone_mapping.wgsl

+	var yxy: vec3<f32> = rgb_to_yxy(color.rgb);
+
+	yxy = vec3<f32>(tonemap_aces(yxy.r), yxy.gb);
+
+	let rgb = yxy_to_rgb(yxy);
+
+	return vec4<f32>(rgb, color.a);


Out of curiosity; How does the transformation into Yxy space affect tonemapping? I've usually seen it being applied in RGB to each channel individually.

I don't want to say that this approach is "incorrect" but I feel like it will not result in the colors desaturating as they get brighter (which is usually the desired behavior).

Another small note: I personally would miss some configuration for the tonemapping/camera parameters, mainly

exposure level (just a multiplier)

white point (could be hardcoded to a sensible default)

But these could probably be added in a separate PR along with some other useful options (gamma, contrast, tint, ...)

Out of curiosity; How does the transformation into Yxy space affect tonemapping? I've usually seen it being applied in RGB to each channel individually.

I don't want to say that this approach is "incorrect" but I feel like it will not result in the colors desaturating as they get brighter (which is usually the desired behavior).

The more I think about it, the more it makes sense to do tone mapping in rgb, this would also make it easier to swap between curves.

jakobhellermann · 2021-10-04T14:54:29Z

The render graph looks like this now:

Before

cart

In general the approach used here looks good to me (although I haven't yet done a thorough review of the bloom algorithms). There are a number of changes required to bring this in line with the latest pipelined-rendering changes though (and there will be more once we merge the shader imports pr later today). I'm happy to pick up that work if you want, as we'll either need to get this merged in the next day or so, or hold off until after the pipelined-rendering -> main merge.

cart · 2021-11-17T00:58:17Z

examples/3d/3d_scene_pipelined.rs

    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
 ) {
+    // configure bloom threshold
+    bloom_settings.threshold = 10.0;


Given that we have a separate bloom example, I don't think its worth the added complexity to configure this here. I'd like 3d_scene_pipelined to be scoped to whats currently in it (and nothing else). If we can't set up 3d_scene_pipelined to look reasonable with the defaults, they aren't good defaults.

cart · 2021-11-17T01:02:19Z

pipelined/bevy_hdr/src/bloom_node.rs

+    pub up_sample_scale: f32,
+}
+
+impl Default for BloomSettings {


I think these defaults might need a little tweaking (or maybe the example needs tweaking to be less intense?):

cart · 2021-11-17T22:40:05Z

examples/3d/3d_scene_pipelined_bloom.rs

+    PipelinedDefaultPlugins,
+};
+
+fn main() {


We should do manual bloom configuration here to show what that looks like (and make this a suitable replacement for the code I want removed from 3d_scene_pipelined).

cart · 2021-11-17T22:40:47Z

examples/3d/3d_scene_pipelined_bloom.rs

@@ -0,0 +1,74 @@
+use bevy::{


Lets rename this example to bloom_pipelined.rs to better illustrate the intended purpose of the example / help scope it down

cart · 2021-11-17T22:44:45Z

pipelined/bevy_core_pipeline/src/main_pass_driver.rs

            let depth_texture = world.entity(*camera_3d).get::<ViewDepthTexture>().unwrap();
-            let extracted_window = extracted_windows.get(&extracted_camera.window_id).unwrap();
-            let swap_chain_texture = extracted_window.swap_chain_frame.as_ref().unwrap().clone();
+            let hdr_target = graph.get_input_texture(Self::HDR_TARGET).unwrap().clone();


For improved ergonomics and legibility, we've adopted a more entity-driven approach for Views (and their render targets). This is contributes to a good portion of the new conflicts with pipelined-rendering. I think we'll want adapt these changes to be more in-line with that new model, but it will likely require extending Views / ViewTargets to accommodate this use case.

cart · 2021-11-17T22:51:21Z

pipelined/bevy_core_pipeline/src/lib.rs

        graph.add_node(node::MAIN_PASS_DEPENDENCIES, EmptyNode);
        graph.add_node(node::MAIN_PASS_DRIVER, MainPassDriverNode);
        graph
            .add_node_edge(node::MAIN_PASS_DEPENDENCIES, node::MAIN_PASS_DRIVER)
            .unwrap();
+        graph


I think this "bloom registration logic" should be moved to bevy_pbr2. bevy_core_pipeline is intended to be unopinionated / provide core pass types only / be independent of specific render algorithms. HDR setup is probably the exception (as I think it should be tied to View configuration). I think that should probably live where Views are defined in bevy_render2 (or maybe in bevy_core_pipeline)

For rafx, SDR / HDR is configurable. If HDR has some significant performance impact somewhere then I suppose it would make sense to make it configurable.

cart · 2021-11-17T22:53:01Z

pipelined/bevy_core_pipeline/src/lib.rs

+            )
+            .unwrap();
+
+        graph.add_node(node::BLOOM, BloomNode::default());


Ultimately I think we'll want to provide some sort of PostProcessingNode that runs a "centralized" post processing stack. I think managing the graph directly is probably too complicated to coordinate across contexts. But for now I think the current approach is fine. Designing a modular post processing interface deserves its own pr.

cart · 2021-11-17T22:53:31Z

pipelined/bevy_pbr2/src/render/pbr.wgsl

@@ -590,8 +590,8 @@ fn fragment(in: FragmentInput) -> [[location(0)]] vec4<f32> {
                emissive.rgb * output_color.a,
            output_color.a);

-        // tone_mapping
-        output_color = vec4<f32>(reinhard_luminance(output_color.rgb), output_color.a);
+        // tone_mapping (done later in the pipeline)


Lets just remove the tone mapping code from here if we don't plan on using it.

cart · 2021-11-17T23:16:20Z

examples/3d/3d_scene_pipelined.rs

    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
 ) {
+    // configure bloom threshold
+    bloom_settings.threshold = 10.0;
+
    // ground plane
    commands.spawn_bundle(PbrBundle {


We do have noticeable bloom flickering for moving lights. To an extent, it seems unavoidable and a natural part of the effect, but the way it surfaces here seems a bit unnatural. Curious to see if other engines have the issue (to this extent).

flicker.mp4

The most "out of place" instance of this starts on the green sphere at around the 7 second mark.

I think this kind of specular aliasing is a bit difficult to completely eliminate. Especially with very low roughness values (where all of the specular energy is concentrated into a tiny speck, smaller than a pixel). Also The bloom intensity seems very high in this scene, increasing the effect further.

I'm not eintirely sure what kind of curve this filter produces: bloom.wgsl:119-128
~~But at a quick glance it seems to grow x², which will exaggerate the flickering of small, bright areas more.~~ (Edit: it does not grow by x²)

So all in all: the low roughness (concentrated specular), grazing angle (high reflectance) and high bloom intensity result in a very problematic case for flickering.

As for possible solutions to reduce the effect (other than adjusting bloom parameters) I see limiting the pixel brightness (maybe also based on the neighbouring pixels) or temporal filtering.
Personally I think I'd tweak or change the high-pass filter and limit the specular aliasing by using less problematic meshes and/or materials, but that might not be a solution for everyone.

superdump · 2021-12-10T10:26:20Z

I just wanted to note that @aclysma shared some information on Discord in #rendering-dev about methods of reducing flickering due to the roughness value's contribution to the NDF part of the BRDF calculation, as well as some additional filtering that specifically helps with bloom. Worth looking up I think.

superdump · 2021-12-10T10:27:19Z

@ChangeCaps could you rebase this on top of main?

CptPotato · 2021-12-10T10:56:19Z

I just wanted to note that @aclysma shared some information on Discord in #rendering-dev about methods of reducing flickering due to the roughness value's contribution to the NDF part of the BRDF calculation, as well as some additional filtering that specifically helps with bloom. Worth looking up I think.

This sounds great. Maybe it's worth a separate PR?

I also want to suggest to give the tonemapping in this PR another look. I don't want to sound too negative but the implementation and resulting effect just doesn't look quite right to me. I could also give some assistance if that helps.

superdump · 2021-12-11T15:49:02Z

I also want to suggest to give the tonemapping in this PR another look. I don't want to sound too negative but the implementation and resulting effect just doesn't look quite right to me. I could also give some assistance if that helps.

You mean doing tonemapping in RGB instead of Yxy?

CptPotato · 2021-12-11T15:57:36Z

I also want to suggest to give the tonemapping in this PR another look. I don't want to sound too negative but the implementation and resulting effect just doesn't look quite right to me. I could also give some assistance if that helps.

You mean doing tonemapping in RGB instead of Yxy?

That is my main concern, yes.

Other than that I'd recommend to add (as mentioned above)

an exposure factor
a white cutoff value (possibly user configurable, but hardcoded works too)

Attempt to make features like bloom #2876 easier to implement. **This PR:** - Moves the tonemapping from `pbr.wgsl` into a separate pass - also add a separate upscaling pass after the tonemapping which writes to the swap chain (enables resolution-independant rendering and post-processing after tonemapping) - adds a `hdr` bool to the camera which controls whether the pbr and sprite shaders render into a `Rgba16Float` texture **Open questions:** - ~should the 2d graph work the same as the 3d one?~ it is the same now - ~The current solution is a bit inflexible because while you can add a post processing pass that writes to e.g. the `hdr_texture`, you can't write to a separate `user_postprocess_texture` while reading the `hdr_texture` and tell the tone mapping pass to read from the `user_postprocess_texture` instead. If the tonemapping and upscaling render graph nodes were to take in a `TextureView` instead of the view entity this would almost work, but the bind groups for their respective input textures are already created in the `Queue` render stage in the hardcoded order.~ solved by creating bind groups in render node **New render graph:** ![render_graph](https://user-images.githubusercontent.com/22177966/147767249-57dd4229-cfab-4ec5-9bf3-dc76dccf8e8b.png) <details> <summary>Before</summary> ![render_graph_old](https://user-images.githubusercontent.com/22177966/147284579-c895fdbd-4028-41cf-914c-e1ffef60e44e.png) </details> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

Attempt to make features like bloom bevyengine#2876 easier to implement. **This PR:** - Moves the tonemapping from `pbr.wgsl` into a separate pass - also add a separate upscaling pass after the tonemapping which writes to the swap chain (enables resolution-independant rendering and post-processing after tonemapping) - adds a `hdr` bool to the camera which controls whether the pbr and sprite shaders render into a `Rgba16Float` texture **Open questions:** - ~should the 2d graph work the same as the 3d one?~ it is the same now - ~The current solution is a bit inflexible because while you can add a post processing pass that writes to e.g. the `hdr_texture`, you can't write to a separate `user_postprocess_texture` while reading the `hdr_texture` and tell the tone mapping pass to read from the `user_postprocess_texture` instead. If the tonemapping and upscaling render graph nodes were to take in a `TextureView` instead of the view entity this would almost work, but the bind groups for their respective input textures are already created in the `Queue` render stage in the hardcoded order.~ solved by creating bind groups in render node **New render graph:** ![render_graph](https://user-images.githubusercontent.com/22177966/147767249-57dd4229-cfab-4ec5-9bf3-dc76dccf8e8b.png) <details> <summary>Before</summary> ![render_graph_old](https://user-images.githubusercontent.com/22177966/147284579-c895fdbd-4028-41cf-914c-e1ffef60e44e.png) </details> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective - Adds a bloom pass for HDR-enabled Camera3ds. - Supersedes (and all credit due to!) #3430 and #2876 ![image](https://user-images.githubusercontent.com/47158642/198698783-228edc00-20b5-4218-a613-331ccd474f38.png) ## Solution - A threshold is applied to isolate emissive samples, and then a series of downscale and upscaling passes are applied and composited together. - Bloom is applied to 2d or 3d Cameras with hdr: true and a BloomSettings component. --- ## Changelog - Added a `core_pipeline::bloom::BloomSettings` component. - Added `BloomNode` that runs between the main pass and tonemapping. - Added a `BloomPlugin` that is loaded as part of CorePipelinePlugin. - Added a bloom example project. Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com> Co-authored-by: DGriffin91 <github@dgdigital.net>

Attempt to make features like bloom bevyengine#2876 easier to implement. **This PR:** - Moves the tonemapping from `pbr.wgsl` into a separate pass - also add a separate upscaling pass after the tonemapping which writes to the swap chain (enables resolution-independant rendering and post-processing after tonemapping) - adds a `hdr` bool to the camera which controls whether the pbr and sprite shaders render into a `Rgba16Float` texture **Open questions:** - ~should the 2d graph work the same as the 3d one?~ it is the same now - ~The current solution is a bit inflexible because while you can add a post processing pass that writes to e.g. the `hdr_texture`, you can't write to a separate `user_postprocess_texture` while reading the `hdr_texture` and tell the tone mapping pass to read from the `user_postprocess_texture` instead. If the tonemapping and upscaling render graph nodes were to take in a `TextureView` instead of the view entity this would almost work, but the bind groups for their respective input textures are already created in the `Queue` render stage in the hardcoded order.~ solved by creating bind groups in render node **New render graph:** ![render_graph](https://user-images.githubusercontent.com/22177966/147767249-57dd4229-cfab-4ec5-9bf3-dc76dccf8e8b.png) <details> <summary>Before</summary> ![render_graph_old](https://user-images.githubusercontent.com/22177966/147284579-c895fdbd-4028-41cf-914c-e1ffef60e44e.png) </details> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective - Adds a bloom pass for HDR-enabled Camera3ds. - Supersedes (and all credit due to!) bevyengine#3430 and bevyengine#2876 ![image](https://user-images.githubusercontent.com/47158642/198698783-228edc00-20b5-4218-a613-331ccd474f38.png) ## Solution - A threshold is applied to isolate emissive samples, and then a series of downscale and upscaling passes are applied and composited together. - Bloom is applied to 2d or 3d Cameras with hdr: true and a BloomSettings component. --- ## Changelog - Added a `core_pipeline::bloom::BloomSettings` component. - Added `BloomNode` that runs between the main pass and tonemapping. - Added a `BloomPlugin` that is loaded as part of CorePipelinePlugin. - Added a bloom example project. Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com> Co-authored-by: DGriffin91 <github@dgdigital.net>

bjorn3 reviewed Sep 27, 2021

View reviewed changes

pipelined/bevy_hdr/Cargo.toml Outdated Show resolved Hide resolved

bjorn3 reviewed Sep 27, 2021

View reviewed changes

pipelined/bevy_hdr/src/tone_mapping.wgsl Outdated Show resolved Hide resolved

mtsr reviewed Sep 27, 2021

View reviewed changes

CptPotato reviewed Sep 27, 2021

View reviewed changes

cart mentioned this pull request Sep 30, 2021

Renderer Rework: Initial Merge Tracking Issue #2535

Closed

64 tasks

inodentry added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Needs-Review labels Oct 1, 2021

CptPotato mentioned this pull request Oct 1, 2021

ACES tonemapping #2264

Closed

superdump mentioned this pull request Oct 12, 2021

PBR / Clustered Forward Rendering #179

Closed

cart reviewed Nov 17, 2021

View reviewed changes

cart removed the S-Needs-Review label Dec 16, 2021

This was referenced Dec 23, 2021

[Merged by Bors] - separate tonemapping and upscaling passes #3425

Closed

Add bloom postprocessing based on #2876 #3430

Closed

ChangeCaps closed this Sep 19, 2022

ChangeCaps force-pushed the pipelined-rendering branch from 2c11ca0 to 85c37e6 Compare September 19, 2022 02:09

JMS55 mentioned this pull request Oct 28, 2022

[Merged by Bors] - Bloom #6397

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HDR and bloom to Pipelined rendering #2876

Add HDR and bloom to Pipelined rendering #2876

ChangeCaps commented Sep 26, 2021 •

edited

Loading

mtsr commented Sep 27, 2021

mtsr left a comment •

edited

Loading

mtsr Sep 27, 2021 •

edited

Loading

ChangeCaps Sep 27, 2021

CptPotato Sep 27, 2021 •

edited

Loading

CptPotato Sep 27, 2021

ChangeCaps Sep 27, 2021

jakobhellermann commented Oct 4, 2021

cart left a comment

cart Nov 17, 2021

cart Nov 17, 2021

cart Nov 17, 2021

cart Nov 17, 2021

cart Nov 17, 2021

cart Nov 17, 2021

superdump Dec 11, 2021

cart Nov 17, 2021

cart Nov 17, 2021

cart Nov 17, 2021

cart Nov 17, 2021

CptPotato Nov 18, 2021 •

edited

Loading

superdump commented Dec 10, 2021

superdump commented Dec 10, 2021

CptPotato commented Dec 10, 2021 •

edited

Loading

superdump commented Dec 11, 2021

CptPotato commented Dec 11, 2021


		let scale = texel_size;

		let a = textureSample(org, sampler, in.uv + vec2<f32>(-1.0, -1.0) * scale);

Add HDR and bloom to Pipelined rendering #2876

Add HDR and bloom to Pipelined rendering #2876

Conversation

ChangeCaps commented Sep 26, 2021 • edited Loading

Objective

Solution

mtsr commented Sep 27, 2021

mtsr left a comment • edited Loading

Choose a reason for hiding this comment

mtsr Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CptPotato Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobhellermann commented Oct 4, 2021

cart left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CptPotato Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

superdump commented Dec 10, 2021

superdump commented Dec 10, 2021

CptPotato commented Dec 10, 2021 • edited Loading

superdump commented Dec 11, 2021

CptPotato commented Dec 11, 2021

ChangeCaps commented Sep 26, 2021 •

edited

Loading

mtsr left a comment •

edited

Loading

mtsr Sep 27, 2021 •

edited

Loading

CptPotato Sep 27, 2021 •

edited

Loading

CptPotato Nov 18, 2021 •

edited

Loading

CptPotato commented Dec 10, 2021 •

edited

Loading