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

Add support for contrast-adaptive sharpening in 3D #47401

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Mar 26, 2021

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

Disabled

Sharpening enabled (intensity 1.0)

Enabled

@clayjohn
Copy link
Member

I have two concerns:

  1. The CAS in the example image is introducing significant darkening that wasn't present in the original image
  2. The test case is totally untextured, but CAS is made to restore contrast in textures that have been blurred by FXAA.

@Calinou
Copy link
Member Author

Calinou commented Mar 28, 2021

  1. The CAS in the example image is introducing significant darkening that wasn't present in the original image

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 😛

@fire
Copy link
Member

fire commented Mar 29, 2021

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

@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from 94b0c55 to b9bd3dd Compare March 29, 2021 14:37
@Calinou
Copy link
Member Author

Calinou commented Mar 29, 2021

I've made a test project where you can easily toggle FXAA, CAS and camera animation: https://github.com/Calinou/test_contrast_adaptive_sharpening

One issue is that HDR highlights are lost when CAS is enabled (look at the marble floor glow while toggling CAS). We need to fix this before this can be merged, so I'll mark this PR as a draft.

Edit: Fixed in #47401 (review).

@Calinou Calinou marked this pull request as draft March 29, 2021 14:41
@Calinou Calinou marked this pull request as ready for review April 5, 2021 12:16
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from b9bd3dd to f1b09e5 Compare April 5, 2021 12:16
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from f1b09e5 to 095e78b Compare April 21, 2021 14:51
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from 095e78b to ba0e113 Compare May 5, 2021 22:29
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from ba0e113 to c29193c Compare June 21, 2021 13:36
@Calinou Calinou requested a review from a team as a code owner June 21, 2021 13:36
@Calinou
Copy link
Member Author

Calinou commented Jun 21, 2021

Rebased and tested again, it works successfully.

@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from c29193c to 6d2ba32 Compare August 10, 2021 17:56
@Calinou
Copy link
Member Author

Calinou commented Aug 10, 2021

Rebased and tested again, it works successfully.

Sharpening (intensity = 1.0, the maximum)

The artifacts in the distance are due to broken VRAM-compressed mipmaps, and are not related to this PR.

Filmic tonemapping (whitepoint = 6)

2021-08-10_19 53 03

ACES tonemapping (whitepoint = 0.5)

2021-08-10_19 53 37

@clayjohn
Copy link
Member

Zooming in on the image you can see the artifacts that result from CAS the way it is implemented right now. You essentially end up with a black outline on sharp corners.

128910830-72eea63b-a8f7-4634-95b6-6898f2a4491b

@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from 6d2ba32 to 3b1eaff Compare August 18, 2021 15:24
@Calinou
Copy link
Member Author

Calinou commented Aug 18, 2021

Rebased and applied the fix from #51841. I tested the fix on master and it works as expected.

@Calinou
Copy link
Member Author

Calinou commented Sep 13, 2021

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.

@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch 2 times, most recently from e3e8b2b to 892ee67 Compare May 3, 2022 18:11
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from 892ee67 to d6234dc Compare June 9, 2022 23:14
@Calinou
Copy link
Member Author

Calinou commented Jun 9, 2022

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.

@akien-mga
Copy link
Member

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.
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening branch from d6234dc to 9d97caa Compare June 17, 2022 14:44
@Calinou
Copy link
Member Author

Calinou commented Jun 17, 2022

Rebased and tested again on latest master on https://github.com/Calinou/godot-reflection.

Might be worth making some visualizations of scenes with TAA with and without CAS.

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.

Copy link
Member

@clayjohn clayjohn left a 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.

@Calinou
Copy link
Member Author

Calinou commented Jun 18, 2022

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.

Does this mean I need to create a shared vec3 variable with a size that matches the viewport's width and height? In this case, I'd need to resize it when the viewport size changes, leading to a shader recompilation.

@clayjohn
Copy link
Member

clayjohn commented Jun 19, 2022

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.
https://developer.amd.com/wordpress/media/2012/10/Efficient%20Compute%20Shader%20Programming.pps

edit: 9x9 should say 10x10, you need one pixel of padding on all sides

@clayjohn clayjohn modified the milestones: 4.0, 4.x Aug 18, 2022
@clayjohn
Copy link
Member

Tagging for 4.x. If this is finished before 4.0 we can merge it, but there is no rush.

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

Successfully merging this pull request may close these issues.

Integrate contrast-adaptive sharpening (CAS) in the Vulkan renderer
4 participants