godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Replace custom implementation of FXAA with the reference implementation #9300

Open mrjustaguy opened 6 months ago

mrjustaguy commented 6 months ago

Describe the project you are working on

3D Game

Describe the problem or limitation you are having in your project

Our simple FXAA implementation has a ton of artifacts not present in reference FXAA, and is much less stable and effective.

For example there are multiple cases where it's eating into objects or creating things that don't exist, as seen in the images of the following proposals

https://github.com/godotengine/godot/issues/83089 https://github.com/godotengine/godot/issues/64985

For the Hello World, do note how the H has some fade around the top and bottom, and is slightly fuzzy.. Do note, that issue had a PR that slightly reduced the effect, but it is still there, and is contributing to actually significant temporal stability issues, making FXAA on worse to no AA

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The Nvidia's Reference FXAA isn't cut down into oblivion and broken.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

see https://developer.download.nvidia.com/assets/gamedev/files/sdk/11/FXAA_WhitePaper.pdf

If this enhancement will not be used often, can it be worked around with a few lines of script?

by writing your own post processing, however is already in core, just broken beyond usability.

Is there a reason why this should be core and not an add-on in the asset library?

it's in Core, just broken.

clayjohn commented 6 months ago

I think that is a good idea. But we need to be careful about licence issues, that's what stopped us from using it in the first place. That gist looks like it's ripped from the SDK which likely carries a licence

mrjustaguy commented 6 months ago

I've removed it given it doesn't even seem to be accurate, according to files from Rise of Nations I've found, which look like the proper official implementation, based on the white paper, and the much more professional documentation written in.

regarding License. FXAA 3 is public domain according to https://en.wikipedia.org/wiki/Fast_approximate_anti-aliasing Not sure how compatible FXAA 3.11 is though, with it's 3-clause BSD license

Calinou commented 6 months ago

Not sure how compatible FXAA 3.11 is though, with it's 3-clause BSD license

We already use 3-clause BSD libraries in Godot, so it's fine. It's an OSI-approved license.

mrjustaguy commented 6 months ago

so it's only down to tracking down the ACTUAL Nvidia FXAA source code...

Calinou commented 6 months ago

so it's only down to tracking down the ACTUAL Nvidia FXAA source code...

I've found it here, but weirdly enough, it doesn't have the BSD license header: https://gist.github.com/kosua20/0c506b81b3812ac900048059d2383126

I've also found a copy where PS3/Xbox 360-optimized codepaths were stripped: https://github.com/vispy/experimental/blob/master/fsaa/fxaa.glsl

mrjustaguy commented 6 months ago

https://github.com/kosua20/Rendu/blob/master/resources/common/shaders/screens/fxaa.frag found linked under the Gist seems to be it from what I can tell based on the code and the paper..

mrjustaguy commented 6 months ago

I'm currently working on porting the shader in the last comment, Got it somewhat working, and it is a big improvement when it does work, however for some reason very bright or very dark edges seem to just get no AA right now, and Multiview isn't implemented yet

Update: Ok I've fixed that issue I was having with it not applying AA in very bright and very dark edges, I'm done for the day, I'll open a PR with Multiview support added in starting next week.. It looks much, much better compared to current FXAA, resolving all of the issues mentioned above

No AA: image

New Implementation: image

Old Implementation: image

Do note, the Green Quad isn't going into HDR values so no fireflies.. Here is Old implementation with HDR quad image

New implementation looks like no AA is applied to it, but this is expected behavior, the results with MSAA are quite similar to no AA too, for this reason I haven't bothered to add the HDR Quad New implementation for comparison.