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

Use lower shadow normal bias for distant directional shadow splits #60178

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 12, 2022

Follow-up to #48776.

This reduces the visual discrepancy between shadow splits when Normal Bias is greater than 0. Shadow acne at a distance may be slightly more present, but if the bias and normal bias values are tuned to avoid acne up close, acne will usually not be present in distant splits.

Tested on desktop (PCSS + PCF) and mobile (PCF) backends, with and without Blend Splits enabled. Performance is identical compared to before.

Testing project: test_shadow_splits.zip

Preview

Stills (from the MRP)

Blend Splits disabled

Before  After
Screenshot_20241213_222624 Screenshot_20241213_222851

Blend Splits enabled

Before After
Screenshot_20241213_222631 Screenshot_20241213_222856

In motion

Normal Bias was increased to 10 to exagerate the difference. Usually, the difference isn't as pronounced, but it can still be significant when using Normal Bias set to 2 as done in #55757.

Before

directionallight3d-normal-bias-before.mp4

After

directionallight3d-normal-bias-after.mp4

@Calinou Calinou requested a review from a team as a code owner April 12, 2022 15:02
@Calinou Calinou added this to the 4.0 milestone Apr 12, 2022
@mrjustaguy
Copy link
Contributor

mrjustaguy commented Apr 12, 2022

This causes Shadow Acne, especially the lower the value of Split 1 is (changing other splits also changes things, but not as much as Split 1 seems to). This also hinders current mitigation of issue that prompted godotengine/godot-proposals#4387 making it worse

Here's a Test Scene, open in current master (with the blur PR merged) and this PR
Test.zip

@mrjustaguy
Copy link
Contributor

Mitigations for the test example were found, shrinking objects slightly or using godotengine/godot-proposals#4517 techniques (reversing face culling and shrinking slightly) helps, and generally as the Acne becomes visible only on faces viewed at grazing angles (by the light) it generally isn't actually that much of a problem.

Still The new behavior should be documented with mitigations, for those that do run into these edge cases if this is merged, until the issue gets resolved, if it ever does.

@KeyboardDanni
Copy link
Contributor

This causes Shadow Acne, especially the lower the value of Split 1 is (changing other splits also changes things, but not as much as Split 1 seems to). This also hinders current mitigation of issue that prompted godotengine/godot-proposals#4387 making it worse

Perhaps this should be exposed as a tweakable then? I've found that setting high normal bias values magically erases peter panning while allowing bias to be set higher, meaning that shadow acne is also defeated. The shadow improvements are specifically why I'd like to migrate to Godot 4, as reverse face culling introduces multiple issues that can't all be resolved by just changing the geometry. And for my application, I can get away with lower bias as I will be opting for little to no shadow filtering, meaning less shadow acne.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@clayjohn clayjohn modified the milestones: 4.1, 4.x May 23, 2023
@mrjustaguy
Copy link
Contributor

Is this PR going to get looked at again anytime soon?

@QbieShay QbieShay modified the milestones: 4.x, 4.4 Jun 19, 2024
@clayjohn
Copy link
Member

We took a look at this in today's render meeting. It seems that the same change can be made on the CPU side to avoid the extra work at run time

light_data.shadow_normal_bias[j] = light->param[RS::LIGHT_PARAM_SHADOW_NORMAL_BIAS] * light_instance->shadow_transform[j].shadow_texel_size;

Also, from mrjustaguys testing it sounds like this introduces shadow acne. We definitely don't want to add something that creates shadow acne

@Calinou Calinou force-pushed the directional-shadow-normal-bias-split branch from c81d758 to 7bdd358 Compare December 13, 2024 15:20
@mrjustaguy
Copy link
Contributor

mrjustaguy commented Dec 13, 2024

I've retested this PR and I've found some flaws in my original testing methodology
While yes, distant splits have a lower normal bias and thus have more acne, the only reason why it's visible is because the sun has 100x normal energy. at normal energy levels the acne is barely visible on a flat solid color white plane under the problematic grazing angles to the sun, especially if you add a texture to the plane like most games will have anyway.

Comparing the Quality of the two options (No reduction in normal bias vs reduction in normal bias for distant splits) the issues of detached splits is significantly more clear, and appear almost always (to an annoying degree, with no mitigations but to drop normal bias and get acne everywhere), whereas the added shadow acne only becomes visible under extreme conditions (sufficiently large Distant geometry that's probably untextured and on grazing angles with the sun, where the sun is sufficiently bright)

The mitigations of the two paths are dropping Normal Bias without the PR which introduces heavy acne everywhere and requires Bias increase to reduce and introduce peter panning, or with the PR to just tweak Biasing settings and introduce peter panning.
With the Given that without the PR you basically lose Normal Bias if you wish to not have your splits have a disconnect, it results in much worse overall results and less flexibility compared to the PR which allows for using Normal Bias without the issue

I want to Stress this point, The most likely scenario for seeing acne added by this PR are open world day-night cycles, given that it's best shown on very large flat(ish) planes with minimal discernable texture detail to mask acne. This acne in the distant splits would naturally be however hidden by the fact that as night falls, and the sun is at such grazing angles, you'd also want your sun to have very low light energy by that point (for the lighting in general to just look good) at which even seeing actual shadows becomes hard close by, let alone some acne far off in the distance.

@Calinou
Copy link
Member Author

Calinou commented Dec 13, 2024

Rebased and tested again on Forward+/Mobile with and without Blend Splits (in various split configurations), it works as expected. I've incorporated @clayjohn's suggestion so that no shader changes are required (visuals are unchanged).

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Discussed in the rendering meeting.

@badsectoracula Will test this on another scene and see how it looks. But ultimately we are happy to merge this for 4.5 and see if anyone complains

@Calinou Calinou force-pushed the directional-shadow-normal-bias-split branch from 7bdd358 to acf9e0e Compare January 21, 2025 00:50
This reduces the visual discrepancy between shadow splits when
Normal Bias is greater than 0. Shadow acne at a distance may
be slightly more present, but if the bias and normal bias values are
tuned to avoid acne up close, acne will usually not be present in
distant splits.
@Calinou Calinou force-pushed the directional-shadow-normal-bias-split branch from acf9e0e to dfc05ca Compare January 21, 2025 00:58
@Repiteo Repiteo modified the milestones: 4.4, 4.x Feb 24, 2025
@clayjohn
Copy link
Member

@badsectoracula Did you ever get a chance to test this?

@badsectoracula
Copy link
Contributor

badsectoracula commented Mar 16, 2025

In pretty much all the tests i could think of this PR was fine and didn't really show much of a visual difference. The biggest difference was when i tried with a large area, expanded distance and a directional light that casts long shadows in which case this PR was actually an improvement. Check the two screenshots below, the first one is the current master and the second is master with this PR applied. The shadow shapes look more "correct" with the PR to me and in one case (red arrow) the current behavior in master "breaks" the shape whereas with the PR it looks correct:

Current master:
image

With the PR applied:
image

@clayjohn clayjohn modified the milestones: 4.x, 4.5 Mar 16, 2025
@Repiteo Repiteo merged commit 70d3727 into godotengine:master Mar 17, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 17, 2025

Thanks!

@Calinou Calinou deleted the directional-shadow-normal-bias-split branch March 17, 2025 17:30
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.

8 participants