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

update FXAA to be simplier and support transparent backgrounds. #23768

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

bhouston
Copy link
Contributor

@bhouston bhouston commented Mar 22, 2022

Description

This replaces the FXAA code to just the code that is required, rather than hundreds of lines cut and pasted from an NVIDIA example. It also contains an improvement to enable FXAA to work properly with transparent backgrounds (which was noticed here in 2018: #12046 (comment)) and a slight improvement to edge handling. Credit for the improvements go to @DanielSturk.

In order to show that FXAA now works on a transparent background, I modified the main FXAA demo to use a transparent background. Prior to this it was solid white.

This contribution is funded by Threekit.

Before change when rendering on a transparent background (look at the right):
three js WebGL - postprocessing - FXAA - Google Chrome 3_22_2022 4_14_31 PM

After change when rendering on a transparent background (look at the right):
three js WebGL - postprocessing - FXAA - Google Chrome 3_22_2022 4_14_17 PM

@bhouston bhouston force-pushed the fxaa-transparent-bg branch from 3e9d580 to f506dc6 Compare March 22, 2022 20:31
@mrdoob mrdoob added this to the r139 milestone Mar 23, 2022
@mrdoob mrdoob merged commit d4a86dc into mrdoob:dev Mar 23, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 23, 2022

Thanks!

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
…ob#23768)

* update FXAA to be simplier and support transparent backgrounds.

* add in Daniel Sturk FXAA improvements

* add Daniel Sturk credits.
@swift502
Copy link
Contributor

swift502 commented Sep 28, 2024

@bhouston I must say this change significantly reduced the quality of FXAA.

The new version is much blurrier.
cmp1

And edge detection much worse. (increased contrast for visibility)
cmp2

I had to revert to the old version.

While edge detection can still be tweaked in this new version, I find the overall quality much inferior.

I know the original shader is huge and could be slimmed down, but surely we'd want to retain the original quality?

@swift502
Copy link
Contributor

swift502 commented Sep 28, 2024

Namely, I can't find these constant tables anywhere in the new version.

    #define FXAA_QUALITY_P0 1.0
    #define FXAA_QUALITY_P1 1.5
    #define FXAA_QUALITY_P2 2.0
    #define FXAA_QUALITY_P3 4.0
    #define FXAA_QUALITY_P4 12.0

Maybe if we just brought these values back it would fix the blurring issue?
This would be an easy fix, I'll take a look at it.

Edit:

Nah didn't help. There's some other issues with the new version.

@swift502
Copy link
Contributor

swift502 commented Sep 28, 2024

@DanielSturk Pinging you as you're allegedly a co-author.

  • If performance is important, I'd be in favor of reverting the old version.
  • If readability is important, I'd be in favor of find a working clean implementation with compatible licensing. Believe I already found a good one.
  • Fixing this version is another option, but I have no clue how it works.

@DanielSturk
Copy link
Contributor

@swift502
I'm afraid I don't have much to contribute to this discussion. I wrote this code back in 2019 and have little time nowadays to maintain / investigate this. There's of course others who are far more invested than I am in perfecting FXAA and I'll happily leave it to them to do so.
What I can provide is context on the changes I had made to the Nvidia shader. There were a few major issues that were relevant to us when we tried the Nvidia shader in production and thus the goal of my changes was specifically to fix the following:

  • It did not support transparency
  • It compared pixels based on Luminance, thus some edges were entirely missed by the shader
  • It didn't handle edges well unless they were near 45 degrees. ~45deg edges are easy to handle because you only need to tap a few adjacent pixels and take the average of their color. I implemented a pixel-march that walked along a horizontal/vertical edge until it no longer detected the edge (due to it shifting 1 pixel perpendicularly), then used that distance to determine the % by which the pixels on each side of the edge should be mixed.

@swift502
Copy link
Contributor

swift502 commented Sep 30, 2024

@DanielSturk Thanks a lot for the response. I guess my other concern is if it's still fair to call your algorithm FXAA?

  1. You mention Luminance was not doing a good job, but it is mentioned frequently in the FXAA white paper, does your new algorithm completely replace that?

image

  1. Every other implementation I've seen seems to be using these lookup tables, but you also seemed to have abandoned that?
    #define FXAA_QUALITY_P0 1.0
    #define FXAA_QUALITY_P1 1.5
    #define FXAA_QUALITY_P2 2.0
    #define FXAA_QUALITY_P3 4.0
    #define FXAA_QUALITY_P4 12.0

If your algorithm has significant changes from "traditional" FXAA implementations, wouldn't it be more fitting to offer it as another AA method, alongside FXAA and it's flaws?

@DanielSturk
Copy link
Contributor

@swift502

  1. I replaced the luminance comparison with the simple contrast() function I wrote
  2. I replaced that quality parameter with NUM_SAMPLES, an int representing the # of steps to take

I have no opinion on the naming. To clarify, this wasn't an in-depth research project but rather a 2-3 day effort to fix the limitations with the shader that were blocking our company at the time. Afterwards, BenHouston contributed these changes back to mainline in this PR

@bhouston
Copy link
Contributor Author

bhouston commented Oct 1, 2024

IF there is indeed a regression in quality which this approach, I suggest we either undo it, or rename this version and add a new one.

It is a major issue that FXAA doesn't handle transparent backgrounds as currently implemented. It would be amazing if we could keep the traditional FXAA quality with support for transparency.

@swift502
Copy link
Contributor

swift502 commented Oct 1, 2024

@bhouston

if we could keep the traditional FXAA quality with support for transparency

That's my goal, to match the appearance of the Nvidia reference implementation. I just double checked that transparent backgrounds work with my proposed shader, and it seems to. #29524 (comment)

If y'all have some use cases to test out, I'd be happy to make sure it's gonna be a worthy upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants