[Merged by Bors] - Add FXAA postprocessing #6393

DGriffin91 · 2022-10-28T03:24:14Z

Objective

Add post processing passes for FXAA (Fast Approximate Anti-Aliasing)
Add example comparing MSAA and FXAA

Solution

When the FXAA plugin is added, passes for FXAA are inserted between the main pass and the tonemapping pass. Supports using either HDR or LDR output from the main pass.

Changelog

Add a new FXAANode that runs after the main pass when the FXAA plugin is added.

crates/bevy_core_pipeline/src/fxaa/fxaa.wgsl

cart

Great work!

I think the blit / intermediate tonemapping stuff can / should be removed in favor of running this after tonemapping (and thus, after effects like bloom). This cuts down on the complexity / work being done. This is how Unity and Wicked Engine do it, and is also how I generally see FXAA being used in the wild (it is often applied in userspace after all engine rendering has finished).

I think we can also simplify the FXAA pipeline generation stuff using specialization.

This should also be ported to #6415, which I made in direct response to this PR's intermediate texture creation. See the description for details.

I've addressed all of these things on my local branch. I'll push the changes after #6415 is merged.

examples/wasm/assets

crates/bevy_core_pipeline/src/fxaa/mod.rs

examples/3d/fxaa.rs

IceSentry · 2022-10-31T23:18:18Z

crates/bevy_core_pipeline/src/fxaa/node.rs

+        let source = post_process.source;
+        let destination = post_process.destination;


Is there a reason for this or is it just to have shorter variable names?

As far as I understand, yes.

Yup just that

crates/bevy_core_pipeline/src/fxaa/mod.rs

IceSentry

A couple of general questions and I'm not familiar with the math, so I can't comment on that part but overall this looks great.

DGriffin91 · 2022-11-01T02:48:17Z

Something a bit odd: the difference between HDR vs LDR is now greater than it was before, with the old to_ldr & blit passes (97251f4). When set to ultra, may of the colored squares now still show aliasing only when using HDR.

(View at original size or larger to see aliasing)

Before without hdr (97251f4)

Before with hdr (97251f4)

After without hdr (2bd5fd8)

After with hdr (2bd5fd8)

cart · 2022-11-01T08:00:36Z

Hmm nice catch. I can repro on my machine.

The old non-hdr pipeline looks like this:

main_pass_with_reinhard_tonemapping(Rgba8UnormSrgb) ->
Blit(RgbaUnormSrgb->Rgba8UnormSrgb) ->
FXAA(Rgba8UnormSrgb->Rgb8UnormSrgb)

FXAA's inputs are "reinhard tonemapped Rgba8UnormSrgb"
New non-hdr pipeline is exactly the same, but without the blit

The old hdr pipeline looked like this:

main_pass_without_tonemapping(Rgba16Float) ->
to_ldr_with_reversible_tonemapping(Rgba16Float->Rgba16Float) ->
FXAA_then_reversed_reversible_tonemap(Rgba16Float->Rgba16Float) ->
reinhard_tonemap_to_ldr(Rgba16Float->Rgba8UnormSrgb)

FXAA's inputs are "reversible-tonemapped Rgba16Float"

The new hdr pipeline looks like this:

main_pass_without_tonemapping(Rgba16Float) ->
reinhard_tonemapping (Rgba16Float -> Rgba16Float) -> 
FXAA(Rgba16Float->Rgba16Float)

FXAA's inputs are "reinhard tonemapped Rgba16Float"
It could be that this combination of things is what makes this different.

Worth poking around a bit.

cart · 2022-11-01T08:26:49Z

Lowering EDGE_THRESH_ULTRA to 0.043 does bring "new hdr" more in-line with the old results. But it does feel like we should be able to use the same threshold values between hdr and ldr pipelines, given that they should be the "same reinhard-mapped ldr colorspace" at this point in the pipeline.

cart · 2022-11-01T08:52:36Z

As this has to do with the threshold, here are the lumaRange values visualized:

Old - LDR

Old - HDR

New - LDR

New - HDR

cart · 2022-11-01T08:56:30Z

Notably, the "new hdr" image does have lower lumaRange values in the cube areas that have more aliasing, so that makes sense.
However interestingly in general the "old hdr" pipeline seems like the biggest outlier when it comes to "average lumaRange".

cart · 2022-11-01T23:14:19Z

Ok after much investigation, we've come to the following conclusions:

There are background color differences between HDR and LDR. This is because when we tonemap in the LDR main pass / pbr shader, we don't apply tonemapping to the clear color, but when we tonemap in a post processing step (the current HDR pipeline), we do. This could be fixed by tonemapping the clear color on the CPU for LDR or by rendering the background color in a shader (making it not a "clear color" anymore).
There are color differences in the lens of the flight helmet. This is because we are tonemapping each transparent fragment in the LDR main pass, but we are tonemapping the final composited pixel in the HDR post processing tonemapping. HDR is "correct" here. The incorrectness in main-pass tonemapping is inherent to the approach / not really solvable.
The differences in lumaRange between HDR and LDR seem to be precision issues between the HDR and LDR targets. Things like moving tonemapping to the main pass for HDR and using a non-srgb target in LDR did not resolve the issue. We will just need to accept this and provide additional configuration levels for FXAA that allow HDR FXAA to be tuned appropriately.

cart · 2022-11-01T23:16:48Z

There was also a bug in how we applied tonemapping in the main pass (we skipped it in some cases). I have fixed it in the commit above.

cart · 2022-11-02T06:51:11Z

bors r+

# Objective - Add post processing passes for FXAA (Fast Approximate Anti-Aliasing) - Add example comparing MSAA and FXAA ## Solution When the FXAA plugin is added, passes for FXAA are inserted between the main pass and the tonemapping pass. Supports using either HDR or LDR output from the main pass. --- ## Changelog - Add a new FXAANode that runs after the main pass when the FXAA plugin is added. Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2022-11-02T07:12:12Z

Pull request successfully merged into main.

Build succeeded:

# Objective - Add post processing passes for FXAA (Fast Approximate Anti-Aliasing) - Add example comparing MSAA and FXAA ## Solution When the FXAA plugin is added, passes for FXAA are inserted between the main pass and the tonemapping pass. Supports using either HDR or LDR output from the main pass. --- ## Changelog - Add a new FXAANode that runs after the main pass when the FXAA plugin is added. Co-authored-by: Carter Anderson <mcanders1@gmail.com>

add fxaa

9bbde40

DGriffin91 commented Oct 28, 2022

View reviewed changes

crates/bevy_core_pipeline/src/fxaa/fxaa.wgsl Outdated Show resolved Hide resolved

clippy

97251f4

alice-i-cecile added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Oct 29, 2022

cart reviewed Oct 30, 2022

View reviewed changes

examples/wasm/assets Outdated Show resolved Hide resolved

crates/bevy_core_pipeline/src/fxaa/mod.rs Outdated Show resolved Hide resolved

cart added 9 commits October 29, 2022 19:27

Rework ViewTarget to better support post processing

2a2e302

Port FXAA

b2a7d04

Merge remote-tracking branch 'origin/main' into pr/DGriffin91/6393

1d2473a

remove duplicate imports

632c250

remove tonemap fn from wgsl

c5eb0a8

remove FxaaTexture

75f2bfa

Delete assets symlink

5ff721b

re-add assets symlink

1404067

Add FxaaPlugin automatically

2b8ee41

IceSentry reviewed Oct 31, 2022

View reviewed changes

examples/3d/fxaa.rs Outdated Show resolved Hide resolved

IceSentry reviewed Oct 31, 2022

View reviewed changes

crates/bevy_core_pipeline/src/fxaa/mod.rs Outdated Show resolved Hide resolved

IceSentry approved these changes Oct 31, 2022

View reviewed changes

DGriffin91 added 3 commits October 31, 2022 18:53

better options layout

81de581

remove unused get_settings Fxaa impl

4fd111e

clean up shader a bit

2bd5fd8

Move tonemapping outside of runtime conditions in pbr shader

5e04772

DGriffin91 and others added 4 commits November 1, 2022 17:01

add turbo

f5965db

reuse cube material

438dd08

turbo -> extreme

702a7bd

Update readme

2c3f099

bors bot changed the title ~~Add FXAA postprocessing~~ [Merged by Bors] - Add FXAA postprocessing Nov 2, 2022

bors bot closed this Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add FXAA postprocessing #6393

[Merged by Bors] - Add FXAA postprocessing #6393

DGriffin91 commented Oct 28, 2022

cart left a comment

IceSentry Oct 31, 2022

DGriffin91 Nov 1, 2022

cart Nov 1, 2022

IceSentry left a comment

DGriffin91 commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022 •

edited

cart commented Nov 1, 2022

cart commented Nov 2, 2022

bors bot commented Nov 2, 2022

		let source = post_process.source;
		let destination = post_process.destination;

[Merged by Bors] - Add FXAA postprocessing #6393

[Merged by Bors] - Add FXAA postprocessing #6393

Conversation

DGriffin91 commented Oct 28, 2022

Objective

Solution

Changelog

cart left a comment

Choose a reason for hiding this comment

IceSentry Oct 31, 2022

Choose a reason for hiding this comment

DGriffin91 Nov 1, 2022

Choose a reason for hiding this comment

cart Nov 1, 2022

Choose a reason for hiding this comment

IceSentry left a comment

Choose a reason for hiding this comment

DGriffin91 commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022

cart commented Nov 1, 2022 • edited

cart commented Nov 1, 2022

cart commented Nov 2, 2022

bors bot commented Nov 2, 2022

cart commented Nov 1, 2022 •

edited