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

Move deband to end of tonemapping. #66317

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Sep 23, 2022

Fixes #66316

This avoids artifacts when using adjustments and color correction

Dither was incorrectly added before the tonemapping step (and thus before the adjustments and color correction). Dither should be applied right before quantization (i.e. before writing to the LDR framebuffer). Otherwise the debanding effect becomes stronger or weaker depending on the scene brightness and the adjustments/color correction steps.

As a bonus this also increases the quality of the debanding (but only very slightly).

Before:

Screenshot from 2022-09-23 11-48-48

After:
Screenshot from 2022-09-23 11-50-23

Without the extreme adjustments:
Before:

Screenshot from 2022-09-23 11-41-19

After (if you zoom in you will notice a very small increase in quality here):

Screenshot from 2022-09-23 11-40-18

This change is not relevant for GLES3 in 4.0, but is relevant for 3.x. It won't be able to be cherrypicked but it is a simple fix

This avoids artifacts when using adjustments and color correction
@clayjohn clayjohn requested a review from a team as a code owner September 23, 2022 18:54
@clayjohn clayjohn added this to the 4.0 milestone Sep 23, 2022
@@ -498,5 +492,11 @@ void main() {
color.rgb = apply_color_correction(color.rgb);
}

if (params.use_debanding) {
Copy link
Member

@Calinou Calinou Sep 23, 2022

Choose a reason for hiding this comment

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

Doing this means debanding is applied after tonemapping – can this be a problem with certain tonemappers?

Edit: In the 3.x version of this PR, this seems to work out just fine (tested with Filmic and ACES Fitted tonemappers).

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it shouldn't be a problem. Debanding should not be done before tonemapping anyway regardless of the tonemapper used. Debanding needs to be done right before quantizing.

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Sep 24, 2022

I have noticed an issue with Debanding in some lighting conditions that aren't present with Debanding Off:
I don't think much can be done, as this is an optical illusion. (there is seemingly movement with Debanding even on a still image)
Debanding On
image
Debanding Off
image

The issue is also present without this PR, though it's much harder to see due to the much bigger issue that this PR is fixing.

@Calinou
Copy link
Member

Calinou commented Sep 24, 2022

(there is seemingly movement with Debanding even on a still image)

Are you using a monitor on a VGA connection, or a monitor with a 6-bit + FRC panel (which can't display 8-bit color without dithering on its own)? Both can result in visible noise appearing on static images. Many low-end monitors (typically those with TN panels) are 6-bit + FRC, not 8-bit or more.

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Sep 24, 2022

using DVI on a ViewSonic VX2433wm, which is 1080p 60Hz LCD (which is a cheap, nearly 15 years old TN panel)

@Calinou
Copy link
Member

Calinou commented Sep 24, 2022

using DVI on a ViewSonic VX2433wm, which is 1080p 60Hz LCD (which is a cheap, nearly 15 years old TN panel)

This is indeed a monitor with 6-bit + FRC color:

Colour Support: | 16.7M (6-bits + Hi FRC)

@akien-mga akien-mga merged commit 5ecaa67 into godotengine:master Sep 27, 2022
@akien-mga
Copy link
Member

Thanks!

@clayjohn clayjohn deleted the debanding-bug branch November 15, 2022 18:33
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.

Adjustments unintentionally affect Debanding
4 participants