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 a project setting to disable soft shadow dithering #53961

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Oct 18, 2021

Soft shadow dithering can look bad in scenes with large flat color areas, especially at lower viewport resolutions or when not using FXAA/TAA.

This adds a project setting that can be used to globally disable soft shadow dithering.

This closes #53534.

Testing project: test_shadow_disable_dither.zip

Preview

Click to view at full size. Viewport resolution is 3840×2160 – dithering will be more noticeable at lower rendering resolutions.

The GPU used for performance figures in the top-right corner is a GeForce RTX 4090.

Soft Shadow Filter Quality = Low

Dithering disabled Dithering enabled (default)
Screenshot_20230427_221323 webp Screenshot_20230427_221339 webp

Soft Shadow Filter Quality = Ultra

Excuse the incorrect rendering, it's due to how I take screenshots. The performance figures are valid though.

Dithering disabled Dithering enabled (default)
Screenshot_20230427_221513 webp Screenshot_20230427_221449 webp

@Ansraer
Copy link
Contributor

Ansraer commented Oct 24, 2021

Have you considered adding high-quality PCF for this use case?

@Calinou
Copy link
Member Author

Calinou commented Oct 24, 2021

Have you considered adding high-quality PCF for this use case?

To be fair, I'm not sure if we actually need to go through the effort of maintaining a separate PCF implementation. I'd prefer if we could just disable the noise pattern and perhaps use a lower blur factor automatically when it's disabled.

We already have 4 different functions to sample shadowmaps (PCSS-like directional, non-PCSS directional, PCSS-like point, non-PCSS point). Having PCF implementations would require even more functions in the shader.

@clayjohn
Copy link
Member

clayjohn commented Oct 24, 2021

I have a feeling that a PCF style filtering will look better when not using rotations than the Vogel-spiral. That being said, it probably isn't worth adding another code path unless the quality difference is substantial.

For this PR to be acceptable, it needs to actually disable the per-pixel rotations, not just set the rotation angle to 0.5. As it stands right now, we are still doing multiple trigonometric functions and a matrix multiplication for each sample even though the rotation is fixed. At the very least, you should replace the matrix constructor with an identity matrix to avoid the trig functions, for example. This:

mat2 disk_rotation;
{
	float r = quick_hash(gl_FragCoord.xy) * 2.0 * M_PI;
	float sr = sin(r);
	float cr = cos(r);
	disk_rotation = mat2(vec2(cr, -sr), vec2(sr, cr));
}

could become:

mat2 disk_rotation = mat2(1.0);
if (sc_shadow_rotation) {
	float r = quick_hash(gl_FragCoord.xy) * 2.0 * M_PI;
	float sr = sin(r);
	float cr = cos(r);
	disk_rotation = mat2(vec2(cr, -sr), vec2(sr, cr));
}

To save on occupancy, you could eliminate the disk_rotation altogether and have two different codepaths, one that rotates samples in the for loop and another that doesnt.

IF PCF really does look better, then we can additionally fill the elements of the shadow kernel with lookup locations suitable for PCF. That avoids having another code path for PCF in the shader.

@Calinou Calinou marked this pull request as draft November 22, 2021 17:16
@Calinou Calinou force-pushed the add-shadow-dither-toggle branch 2 times, most recently from 29182fe to 436cd9d Compare November 22, 2021 17:22
@Calinou Calinou marked this pull request as ready for review November 22, 2021 17:22
@Calinou
Copy link
Member Author

Calinou commented Nov 22, 2021

I disabled the disk rotation as suggested by @clayjohn. It seems to look better now 🙂
See OP for updated comparison screenshots.

It seems the performance difference is also quite noticeable. On a GTX 1080 in the editor viewport, I can save 0.15-0.25 ms of GPU time by disabling shadow dithering. This makes me wonder if shadow dithering should be disabled by default, given not everyone is fond of the noisy appearance of soft shadows.

In the project running in 2560×1440, this would likely translate to 0.3-0.4 ms of GPU time saved given the rendering resolution is higher than in the editor viewport. This is starting to be quite a significant difference.

@akien-mga
Copy link
Member

We discussed this in a PR review meeting, current status is that we're waiting on TAA implementation to see if this would still be needed or not.

@mrjustaguy
Copy link
Contributor

mrjustaguy commented Apr 27, 2023

any status updates on this?

I know TAA is getting a FSR 2.2 makeover treatment which might with some modifications to the dithering remove the dithering noise for TAA, but for those not using TAA this still makes sense to allow disabling it, and/or #58771

@Saul2022
Copy link

I know TAA is getting a FSR 2.2 makeover treatment which might with some modifications to the dithering remove the dithering noise for TAA, but for those not using TAA this still makes sense to allow disabling it, and/or #58771

I agree and perfomance wise enabling fsr 2.2 while it improves dittering and prob edges, it can be more perfomamce intensive for weaker gpu’s, sadly fsr is not the universal solution and this is still needed for more perfomance and flexibility.

@Calinou Calinou force-pushed the add-shadow-dither-toggle branch from 65e08ea to ab6e9f4 Compare April 27, 2023 20:19
@Calinou
Copy link
Member Author

Calinou commented Apr 27, 2023

Rebased and tested again, it works as expected.

I agree that TAA or FSR 2 isn't an end-all-be-all solution to smooth shadow rendering, so we should provide an option for non-dithered shadow mapping. Games with stylized art direction tend to fare better with "just" MSAA, since they don't have to deal with much specular aliasing if at all.

I wouldn't bother adding a dedicated codepath for non-dithered shadow sampling, since this isn't something that will be used very often (and the saved performance from disabling dithering often allows you to pick a higher shadow quality preset).

Soft shadow dithering can look bad in scenes with large flat color areas,
especially at lower viewport resolutions or when not using FXAA/TAA.

This adds a project setting that can be used to globally disable
soft shadow dithering.
@Calinou Calinou force-pushed the add-shadow-dither-toggle branch from ab6e9f4 to 79412df Compare May 19, 2023 22:38
@clayjohn clayjohn modified the milestones: 4.1, 4.x May 23, 2023
@FyiurAmron
Copy link

@Calinou sorry if this is a dumb question, but is there any chance on this getting merged in the coming future, or is it still waiting for TAA implementation or some other related code?

@Calinou
Copy link
Member Author

Calinou commented Jan 26, 2024

or is it still waiting for TAA implementation or some other related code?

Yes: #61834

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.

Vulkan: soft shadows appear grainy, even on "Ultra" quality setting
8 participants