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

Fix AgX sigmoid contrast curve approximation #101406

Merged
merged 1 commit into from
Jan 11, 2025

Conversation

allenwp
Copy link
Contributor

@allenwp allenwp commented Jan 10, 2025

Not cherry-pickable to 4.3, as AgX is only in 4.4.

This changes the polynomial function so that a lower input always results in a lower output and vice-versa. Additionally, the new function returns a value that is much closer to 1.0 when given an input of 1.0.

The polynomial regression for the AgX sigmoid contrast curve was my first attempt because I was rushed to get the change in before going on holiday. Upon review, I discovered a mistake that causes incorrect behavior in some cases.

Given:

x = agx_default_contrast_approx(a)
y = agx_default_contrast_approx(b)

For all values of a and b in the range [0.0, 1.0], x should always be less than y when a is less than b.

This is not the case with the current function in master for near-zero values. Additionally, when given an input of 1.0 the current function returns 1.00927498 when it should return 1.0.

After having more time to fiddle with polynomial regression calculators, I generated a new formula that fits the logical requirements of a tonemapper’s contrast function and gives an output of 1.0001 when given an input of 1.0.

image

This new function reduces banding in the transition from green to white in the Compatibility renderer:

master this PR
image image

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

This only results in a very slight visual difference, so nothing should require manual adjustments from users:

master This PR
master PR

Remember to update tonemap_inc.glsl for Compatibility too.

This changes the polynomial function so that a lower input always results in a lower output and vice-versa. Additionally, the new function returns a value that is much closer to 1.0 when given an input of 1.0.
@allenwp allenwp force-pushed the fix-agx-contrast-curve branch from 5dafd4b to 77ddaaa Compare January 10, 2025 23:17
@clayjohn clayjohn marked this pull request as ready for review January 10, 2025 23:37
@clayjohn clayjohn requested a review from a team as a code owner January 10, 2025 23:37
@allenwp
Copy link
Contributor Author

allenwp commented Jan 10, 2025

Thanks! I've pushed the updates to the Compatibility renderer and have finished reviewing my Forward+ test images to ensure there are no unexpected changes. I see that it's already been moved to "ready for review" and approved. Thanks! I'm happy my hastily put together first attempt at an approximation isn't going to stick around for any length of time 🙂

@akien-mga akien-mga merged commit 62a5ea6 into godotengine:master Jan 11, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@allenwp allenwp deleted the fix-agx-contrast-curve branch January 11, 2025 22:07
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