Skip to content

NOM-3927 - Fix FXAATask so it works with non-square viewports #48

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

Merged
merged 8 commits into from
Aug 18, 2025

Conversation

fanciful-marmot
Copy link
Contributor

Prior to this commit, it looks like a single float was used to scale UVs to try to find neighbouring pixels based on this section

    // Sample current and neighboring pixels luminance in axis aligned space.
    vec4 extent;
    extent.xy = uvOut + vec2(-0.5, -0.5) * uResolution;
    extent.zw = uvOut + vec2( 0.5,  0.5) * uResolution;

Presumably for FXAA to work you want to find the direct neighbours of the pixel you're working on to decide how to blur. Depending on the actual resolution, uResolution would have to changed if that's the case. For example, at a resolution of 100x100, you'd want the delta UV to be +/- (0.01, 0.01) to find the direct neighbours so uResolution would have to be 0.02.
However, if the resolution is 1000x1000, you'd want the delta UV to be +/- (0.001, 0.001) to find the direct neighbours so uResolution would have to be 0.002. If you'd used the same uResolution you had used for the 100x100 example, you'd be sampling 10 pixels away which doesn't seem right.

So uResolution should be adjusted based on the actual resolution. This leads to the next problem of it being a single float. If you have a non-square aspect ratio you'd get the direct neighbours for one dimension but be off for the other.

This PR changes uResolution to vec2 instead of float and updates the examples to demonstrate how this value should be set.

NOTE

I changed the example and the test to not overdo the effect. We can make it overdo it by setting using a number > 1 in 1.0f / framing.dataWindow.GetWidth() if we want.

Also the tests that do run will fail until I update the baseline images. I'll need to let it fail once to get it for all platforms.

Copy link
Contributor

@erikaharrison-adsk erikaharrison-adsk left a comment

Choose a reason for hiding this comment

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

Thanks for this fix! Looks good to me, using width/height instead of a shared fixed value. May want to ping Jochen directly as he worked more closely on this.

@fanciful-marmot
Copy link
Contributor Author

@erikaharrison-adsk I fully expected the tests to fail for other platforms since I didn't update the baseline images there. Are those tests not run? I thought the fxaa test was only skipped on OSX

@hodoulp
Copy link
Contributor

hodoulp commented Aug 11, 2025

Are the values used to test FXAA produce a baseline image different enough to compare with a utest without FXAA?

@fanciful-marmot
Copy link
Contributor Author

Are the values used to test FXAA produce a baseline image different enough to compare with a utest without FXAA?

Yes, I made them realistic values for tests this time. I can change it to be exaggerated if that's preferred but since the parameter is meant to be the conversion from uv to pixel giving an incorrect value seemed like it would be confusing if someone used it as a reference for how to use the task

Signed-off-by: Patrick Hodoul <[email protected]>
Signed-off-by: Patrick Hodoul <[email protected]>
@hodoulp
Copy link
Contributor

hodoulp commented Aug 13, 2025

@fanciful-marmot You can now merge.

@fanciful-marmot fanciful-marmot merged commit b51e059 into main Aug 18, 2025
10 checks passed
@fanciful-marmot fanciful-marmot deleted the fanciful-marmot/fxaa-resolution-fix branch August 18, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants