-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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 support for contrast-adaptive sharpening in 3D #47401
base: master
Are you sure you want to change the base?
Add support for contrast-adaptive sharpening in 3D #47401
Conversation
1ea19f9
to
94b0c55
Compare
I have two concerns:
|
I think these ringing artifacts are expected when you use a high sharpen intensity. Most of the time, you won't be using 1.0 but something between 0.0 and 1.0 (0.4 is a good start). I intentionally used an extreme value in the screenshot so that you can't miss the effect 😛 |
From a testing standpoint I would pick a scene that is a model of a typical fps scene. The TPS demo isn't typical of a first person pbr view but we have it easily! https://github.com/godotengine/tps-demo |
94b0c55
to
b9bd3dd
Compare
I've made a test project where you can easily toggle FXAA, CAS and camera animation: https://github.com/Calinou/test_contrast_adaptive_sharpening
Edit: Fixed in #47401 (review). |
b9bd3dd
to
f1b09e5
Compare
f1b09e5
to
095e78b
Compare
095e78b
to
ba0e113
Compare
ba0e113
to
c29193c
Compare
Rebased and tested again, it works successfully. |
c29193c
to
6d2ba32
Compare
6d2ba32
to
3b1eaff
Compare
Rebased and applied the fix from #51841. I tested the fix on |
Update: This pull request will most likely not be merged until a decision is made about FSR: #51679 While FSR includes its own RCAS sharpening algortihm, using RCAS in a standalone manner (without upscaling) is not recommended by AMD. Nonetheless, we may still want to do that for simplicity reasons. The FSR pull request needs a rebase following #51870 before it can be merged. |
e3e8b2b
to
892ee67
Compare
892ee67
to
d6234dc
Compare
Rebased and tested again with TAA enabled (and TAA + FXAA), it works as expected. cc @JFonS 🙂 CAS helps restore the sharpness that is lost by enabling TAA, so it becomes more useful, even at native resolution. This is especially true when using both TAA and FXAA at the same time ("TXAA") to better combat aliasing on moving objects. |
Might be worth making some visualizations of scenes with TAA with and without CAS. |
This is an older, easier to implement variant of CAS as a pure fragment shader. It doesn't support upscaling, but the upscaling part of CAS has been superseded by FSR 1.0 anyway. The sharpening intensity can be adjusted on a per-Viewport basis. For the root viewport, it can be adjusted in the Project Settings.
d6234dc
to
9d97caa
Compare
Rebased and tested again on latest
I made a comprehensive set of comparison images, along with recommended sharpening factors for each antialiasing type (0.4 for FXAA, 0.4 for TAA, 0.5 for FXAA + TAA):
TAA with Sharpen 0.4 might look a bit oversharpened on still images, but it definitely helps to have more sharpness in motion as it can get quite blurry when the camera is moving. |
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.
As mentioned on the 3.x version of this CAS needs to be able to read the output pixels after FXAA and Tonemapping have been applied but before the linear->sRGB conversion. You can do that without an extra pass by storing the intermediate results in local shared memory and then in the CAS filter take your samples from LSM instead of reading from the old texture. By doing this you avoid the problem of source_color
not being tonemapped.
Also this PR will have a similar issue to the one we just had with Glow Mix and FXAA. In this case it shouldn't be as bad, but it may lead to undesirable artifacts when CAS is combined with Mix Glow.
Does this mean I need to create a |
No, the shared variable should only be 9x9 vec3s.The shared memory is not shared by the entire shader invocation. It is shared on a compute unit. Since we structure our compute shader work in 8x8 thread groups and the CAS filter only reads neighbouring pixels you just need to do the copy to a 9x9 area for each group. I can send you a nice presentation that explains this in more detail (it also has good images which makes this way easier to understand) Here is the presentation. The example is on calculating a separable blur, but the concept is the same. edit: 9x9 should say 10x10, you need one pixel of padding on all sides |
Tagging for 4.x. If this is finished before 4.0 we can merge it, but there is no rush. |
This is an older, easier to implement variant of CAS as a pure fragment shader. It doesn't support upscaling, but the upscaling part of CAS has been superseded by FSR 1.0 anyway.
The sharpening intensity can be adjusted on a per-Viewport basis. For the root viewport, it can be adjusted in the Project Settings.
This can likely be backported to the GLES3 renderer, but I don't know whether this can work in GLES2.
Edit: Done for GLES3 in #47416. It can't be implemented in GLES2 due to
textureLodOffset()
not being available.Test project: https://github.com/Calinou/test_contrast_adaptive_sharpening
This closes godotengine/godot-proposals#1519.
Preview
FXAA is enabled on both screenshots.
Slider comparison
Sharpening disabled
Sharpening enabled (intensity 1.0)