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 (3.x, GLES3 only) #47416

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Mar 27, 2021

3.x version of #47401.

This is an older, easier to implement variant of CAS as a pure fragment shader. It doesn't support upscaling, but we won't make use of it (at least for now).

The sharpening intensity can be adjusted on a per-Viewport basis. For the root viewport, it can be adjusted in the Project Settings.

Since textureLodOffset() isn't available in GLES2, there is no way to support contrast-adaptive sharpening in GLES2.

Preview

FXAA is enabled on both screenshots.

Slider comparison

Sharpening disabled

Disabled

Sharpening enabled (intensity 1.0)

Enabled

@Calinou
Copy link
Member Author

Calinou commented Mar 29, 2021

This PR likely suffers from the same HDR issue as the Vulkan version, so I'll mark it as a draft until we find a way to fix it.

Edit: Fixed by #47401 (review), thanks @fire 🙂

@Calinou Calinou marked this pull request as draft March 29, 2021 14:42
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening-3.x branch from 0b0b91e to dbbaaf8 Compare April 5, 2021 12:21
@Calinou Calinou marked this pull request as ready for review April 5, 2021 12:21
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this looks fine. I can't comment on the method but if it is pretty standard then that seems safe to try out, and it seems to get removed from the shader when set to 0.0 so that is good.

If it were me I might be tempted to enclose the apply_cas in the shader with #ifdef but that's because I don't trust shader compilers. And the other functions aren't doing this I guess lol.

I don't know if @clayjohn has any preferences before trying it out.

@clayjohn
Copy link
Member

clayjohn commented May 25, 2021

As before, I think the code looks fine, but we absolutely need a test scene that utilizes CAS properly to verify that it is working correctly. We can't merge graphics features without testing them.

A proper test scene for CAS is one that has textures with high frequency detail that get blurred out by FXAA. Upon turning CAS on it should restore some of the high-frequency detail while maintaining the edge antialiasing. A test scene with untextured blocks only shows that CAS adds some amount of aliasing to edges. We need a test scene that shows that this implementation of CAS sharpens high-frequency textures more than it aliases edges.

edit Ah, so sorry, my finger slipped. I did not mean to close this. I support the PR and I think it is likely ready to merge, but it needs to be tested before we do so.

@clayjohn clayjohn closed this May 25, 2021
@clayjohn clayjohn reopened this May 25, 2021
@Calinou
Copy link
Member Author

Calinou commented May 25, 2021

@clayjohn I made a test project for the master version of this PR: https://github.com/Calinou/test_contrast_adaptive_sharpening

@clayjohn
Copy link
Member

clayjohn commented May 26, 2021

I think CAS needs to run in its own pass after tonemapping for 2 reasons:

  1. It needs to read the output after FXAA is applied (currently only the central pixel "e" uses the FXAA colour)
  2. It needs the input to be linear and 0-1 range otherwise we will get ringing artifacts.

For the Vulkan version you can avoid the extra pass by writing out the results to local memory and placing a barrier after tonemapping. For GLES3, you will need to break it into 2 passes. First pass should run as normal, but write to an intermediate buffer, then the second pass should apply CAS by itself.

For a more detailed explanation of the above, the notes in the original implementation are really helpful

@Calinou
Copy link
Member Author

Calinou commented May 26, 2021

@clayjohn Thanks for the insight! Unfortunately, I don't know how to use local memory and barriers (I have no actual Vulkan experience).

As for running CAS in a second pass in GLES3, wouldn't that decrease performance noticeably? It also doesn't sound easy to introduce multi-pass post-processing shaders in Godot right now. Having that knowledge would be useful for implementing SMAA, so I'm all for it 😛

Due to this, I'll need to have someone else finish this feature if we want it to be merged in a reasonable timeframe – unless we're OK with merging this PR in its current state. I think it certainly works right now, even though it's not ideal. Ringing artifacts will generally not be as noticeable with intensities around 0.5.

@akien-mga
Copy link
Member

There are some style issues to fix, and docs to sync. Otherwise is this ready to merge / get tested for 3.4?

@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening-3.x branch from dbbaaf8 to d3edf6c Compare June 21, 2021 12:42
@Calinou Calinou requested a review from a team as a code owner June 21, 2021 12:42
@Calinou
Copy link
Member Author

Calinou commented Jun 21, 2021

I rebased and added documentation for the newly exposed properties/methods.

This is an older, easier to implement variant of CAS as a pure
fragment shader. It doesn't support upscaling, but we won't make
use of it (at least for now).

The sharpening intensity can be adjusted on a per-Viewport basis.
For the root viewport, it can be adjusted in the Project Settings.

Since `textureLodOffset()` isn't available in GLES2, there is no
way to support contrast-adaptive sharpening in GLES2.
@Calinou Calinou force-pushed the add-contrast-adaptive-sharpening-3.x branch from d3edf6c to a9c0c54 Compare June 21, 2021 13:37
@akien-mga akien-mga merged commit 7c6bdea into godotengine:3.x Aug 10, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

I might have merged a bit fast since I'm not sure @clayjohn's feedback was addressed. But since this is opt in, I guess we can get it tested and bugfix/adapt the implementation based on user reports in 3.4 betas?

akien-mga added a commit that referenced this pull request Aug 10, 2021
@Calinou
Copy link
Member Author

Calinou commented Aug 10, 2021

I might have merged a bit fast since I'm not sure @clayjohn's feedback was addressed. But since this is opt in, I guess we can get it tested and bugfix/adapt the implementation based on user reports in 3.4 betas?

clayjohn's feedback doesn't seem to be essential to get working CAS (even with FXAA enabled), but it would look more correct in certain situations. From my testing, CAS looked good enough already, so I would wait and see if anyone has problems with the current implementation.

@Calinou
Copy link
Member Author

Calinou commented Sep 13, 2021

For future reference, I pushed a 3.x-compatible version of the testing project: https://github.com/Calinou/test_contrast_adaptive_sharpening/tree/3.x

sairam4123 pushed a commit to sairam4123/godot that referenced this pull request Nov 10, 2021
lekoder pushed a commit to KoderaSoftwareUnlimited/godot that referenced this pull request Dec 18, 2021
@Calinou Calinou changed the title Add support for contrast-adaptive sharpening in 3D (GLES3 only) Add support for contrast-adaptive sharpening in 3D (3.x, GLES3 only) Jan 20, 2023
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.

4 participants