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 port of AMD's Robust Contrast Adaptive Sharpening #7422

Merged
merged 23 commits into from
Apr 2, 2023

Conversation

Elabajaba
Copy link
Contributor

@Elabajaba Elabajaba commented Jan 30, 2023

Objective

TAA, FXAA, and some other post processing effects can cause the image to become blurry. Sharpening helps to counteract that.

Solution

This is a port of AMD's Contrast Adaptive Sharpening (I ported it from the SweetFX version, which is still MIT licensed). CAS is a good sharpening algorithm that is better at avoiding the full screen oversharpening artifacts that simpler algorithms tend to create.

This is a port of AMD's Robust Contrast Adaptive Sharpening (RCAS) which they developed for FSR 1 (and continue to use in FSR 2). RCAS is a good sharpening algorithm that is better at avoiding the full screen oversharpening artifacts that simpler algorithms tend to create.


Future Work

  • Consider porting this to a compute shader for potentially better performance. (In my testing it is currently ridiculously cheap (0.01ms in Bistro at 1440p where I'm GPU bound), so this wasn't a priority, especially since it would increase complexity due to still needing the non-compute version for webgl2 support).

Changelog

  • Added Contrast Adaptive Sharpening.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Jan 31, 2023
@Elabajaba Elabajaba marked this pull request as ready for review January 31, 2023 06:16
@Elabajaba Elabajaba changed the title Add AMD's Contrast Adaptive Sharpening Add port of AMD's Contrast Adaptive Sharpening Jan 31, 2023
@IceSentry IceSentry requested a review from JMS55 January 31, 2023 21:52
Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Awesome work, the added sharpness is a definite improvement :)

Would be nice to have this as a compute shader, with an IFDEF keeping the old version for WebGL. That can be a future PR though.

@Elabajaba
Copy link
Contributor Author

Elabajaba commented Feb 3, 2023

What do people think about shortening the internal names from ContrastAdaptiveSharpening to CAS (eg. ViewContrastAdaptiveSharpeningPipeline -> ViewCASPipeline, ContrastAdaptiveSharpeningNode -> CASNode).

I think it's probably better to not change the user facing ContrastAdaptiveSharpeningSettings to CASSettings so that it keeps sharpening in the name.

edit: Though CAS is probably more recognizable than ContrastAdaptiveSharpening since that's what it's called in options menus, and is what AMD advertises it as.

@JMS55
Copy link
Contributor

JMS55 commented Feb 3, 2023

I like keeping the public API with the full name, but it's totally fine to shorten the internal type names. I did the same for TAA and SSAO.

We may even want to rename it to just SharpeningSettings or something - the contrast adaptive part could just be an implementation detail.

@JMS55 JMS55 added this to the 0.10 milestone Feb 6, 2023
@JMS55
Copy link
Contributor

JMS55 commented Feb 6, 2023

@Elabajaba needs rebasing due to stageless.

@Elabajaba
Copy link
Contributor Author

Elabajaba commented Feb 6, 2023

Rebased, just not sure if I need to add #[reflect(Component)] to ContrastAdaptiveSharpeningSettings (like what is added in #7527). Added #[reflect(Component)] so reflection will work.

Since this is also modifying the FXAA example (and TAA removes it), this either needs to be merged before TAA and then add the sharpening system into the new anti-aliasing example, or I can rebase it onto TAA and it can be merged after (or just change it to use the AA example once TAA is merged).

@superdump superdump self-requested a review February 7, 2023 12:41
@Elabajaba Elabajaba force-pushed the cas branch 2 times, most recently from 92fa303 to 7a44693 Compare February 12, 2023 21:16
@alice-i-cecile
Copy link
Member

@Elabajaba can you please resolve the comments you've addressed to help reviewers?

Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

looks good, and shows a decent improvement with fxaa. i guess it'll be an even better impact with taa. i didn't grok the shader math but it looks in line with the reference.

it's not a matter for this PR, but i think we need to put some thought into node ordering. more nodes is making construction of core pipeline increasingly fragile; omitting a node (like FXAA) will cause ordering issues with nodes that order relative to it.
this isn't a problem as long as all these things are always in the core pipeline but it is not scalable and not open to non-core modification... something we can improve as part of rendergraph yeeting maybe.

@JMS55
Copy link
Contributor

JMS55 commented Mar 12, 2023

it's not a matter for this PR, but i think we need to put some thought into node ordering. more nodes is making construction of core pipeline increasingly fragile; omitting a node (like FXAA) will cause ordering issues with nodes that order relative to it.
this isn't a problem as long as all these things are always in the core pipeline but it is not scalable and not open to non-core modification... something we can improve as part of rendergraph yeeting maybe.

Strongly agreed, that's part of what I opened the render node issues about. Imo, we should have labels be separate from nodes. So we always add labels and can order relative to them, but it doesn't matter if the node is in the graph or not. We should also pull out the node ordering into the core_3d file, instead of defining it per-plugin, which is more fragile.

@Elabajaba
Copy link
Contributor Author

This has been updated to use Robust Contrast Adaptive Sharpening (RCAS) instead of CAS. RCAS is newer and was developed for FSR 1 (it's also used in FSR2), and uses fewer texture samples compared to CAS (5 instead of 9).

@Elabajaba Elabajaba changed the title Add port of AMD's Contrast Adaptive Sharpening Add port of AMD's Robust Contrast Adaptive Sharpening Mar 29, 2023
@Elabajaba Elabajaba requested review from robtfm and JMS55 and removed request for superdump, robtfm and JMS55 March 30, 2023 01:17
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Minor changes, basically looks good. :) Thanks!

@JMS55
Copy link
Contributor

JMS55 commented Apr 1, 2023

Maybe we should add CAS to the TAA/FXAA bundles. We should at least link to CAS from the TAA docs, where it mentions the softness of TAA.

Also, thoughts on calling the user-facing types just Sharpening?

@superdump
Copy link
Contributor

Maybe we should add CAS to the TAA/FXAA bundles. We should at least link to CAS from the TAA docs, where it mentions the softness of TAA.

Sharpening by default makes sense if they are practically blurry. I didn't really feel that our TAA looked blurry with our implementation / configuration, but I haven't had chance to play with it much yet. And I think last time I checked, FXAA at higher quality settings didn't look blurry, but lower quality settings did. However... I have mostly been looking at it on a MacBook Pro with hidpi display, so things will naturally look sharper. I should test it without the winit scale factor 1.0 setting.

Also, thoughts on calling the user-facing types just Sharpening?

I could imagine there being other sharpening methods long-term. I think it's better to keep specific implementations specifically-named and then when it becomes clear what the situation is, use an enum to group the variants. I don't think we're there yet though. So what I mean is, build the individual pieces of functionality with full flexibility, and then add convenience API on top when it makes sense to do so.

@Elabajaba
Copy link
Contributor Author

Changed it to use a linear 0.0 to 1.0 value for sharpening strength, visually it looks pretty linear imo.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 2, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 2, 2023
Merged via the queue into bevyengine:main with commit 09f1bd0 Apr 2, 2023
@DGriffin91
Copy link
Contributor

@superdump

And I think last time I checked, FXAA at higher quality settings didn't look blurry, but lower quality settings did.

This is actually backwards. FXAA is both sharper and faster at lower settings. (Part of why I didn't use the word quality in the plugin, but sensitivity instead)

I agree that FXAA shouldn't enable CAS by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants