-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Add HDR and bloom to Pipelined rendering #2876
Add HDR and bloom to Pipelined rendering #2876
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_hdr/src/bloom.wgsl
Outdated
|
||
let scale = texel_size; | ||
|
||
let a = textureSample(org, sampler, in.uv + vec2<f32>(-1.0, -1.0) * scale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
examples/3d/3d_scene_pipelined.rs
Outdated
mut meshes: ResMut<Assets<Mesh>>, | ||
mut materials: ResMut<Assets<StandardMaterial>>, | ||
) { | ||
// configure bloom threshold | ||
bloom_settings.threshold = 10.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pipelined/bevy_hdr/src/bloom_node.rs
Outdated
pub up_sample_scale: f32, | ||
} | ||
|
||
impl Default for BloomSettings { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PipelinedDefaultPlugins, | ||
}; | ||
|
||
fn main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@@ -0,0 +1,74 @@ | |||
use bevy::{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets rename this example to bloom_pipelined.rs
to better illustrate the intended purpose of the example / help scope it down
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
) | ||
.unwrap(); | ||
|
||
graph.add_node(node::BLOOM, BloomNode::default()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets just remove the tone mapping code from here if we don't plan on using it.
examples/3d/3d_scene_pipelined.rs
Outdated
mut meshes: ResMut<Assets<Mesh>>, | ||
mut materials: ResMut<Assets<StandardMaterial>>, | ||
) { | ||
// configure bloom threshold | ||
bloom_settings.threshold = 10.0; | ||
|
||
// ground plane | ||
commands.spawn_bundle(PbrBundle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most "out of place" instance of this starts on the green sphere at around the 7 second mark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
@ChangeCaps could you rebase this on top of main? |
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. |
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)
|
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 <[email protected]>
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 <[email protected]>
# 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 <[email protected]> Co-authored-by: Carter Anderson <[email protected]> Co-authored-by: DGriffin91 <[email protected]>
# 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 <[email protected]> Co-authored-by: Carter Anderson <[email protected]> Co-authored-by: DGriffin91 <[email protected]>
# 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 <[email protected]> Co-authored-by: Carter Anderson <[email protected]> Co-authored-by: DGriffin91 <[email protected]>
# 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 <[email protected]> Co-authored-by: Carter Anderson <[email protected]> Co-authored-by: DGriffin91 <[email protected]>
# 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 <[email protected]> Co-authored-by: Carter Anderson <[email protected]> Co-authored-by: DGriffin91 <[email protected]>
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 <[email protected]>
# 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 <[email protected]> Co-authored-by: Carter Anderson <[email protected]> Co-authored-by: DGriffin91 <[email protected]>
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