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

Improvements to SpotLight3D and OmniLight3D's shadows #51335

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

JFonS
Copy link
Contributor

@JFonS JFonS commented Aug 6, 2021

Based on #51025, should be merged first.

Here's a quick comparison using the default bias values:

Before, current master:

bad.mp4

After:

good.mp4

Fixed issues

Lack of precision in cube map mode

Cubemap shadows were using 0.01 as znear regardless of the light radius (zfar), so for large lights precision would go drastically down. I changed it so znear scales relative to the light's radius (0.005*radius) and acne cause by precision issues was gone. This means large lights can't be placed too close to shadow casters (i.e 0.5 meters for a 100 radius light) but dual paraboloid mode doesn't have this restriction, so users can go with that if needed. A probably better fix would be to use logarithmic depth or reverse Z buffer in order to keep a constant znear and still distribute precision evenly over the shadow map, but it can be done in a separate PR.

Squashed paraboloids

In order to fit both paraboloids on a single atlas square we were stacking them up in Y, resulting in two squashed 2:1 ratio paraboloids. This created aliasing and consequently acne in the shadows. I tried scaling the bias amount in the Y axis to compensate for that, but it didn't seem to work. The only solution I found was to use two square atlas regions, one for each paraboloid.

Shadowmap atlas bleeding

Since shadow atlas regions are adjacent to each other, and we use bilinear sampling, there's bleeding across them. I noticed this because sampling one paraboloid was picking data from the other. I added 1px padding to omni lights, but I think this will also be a problem for other light types, it needs to be looked into.

Shadow blur issues

Shadow blur had a couple of issues. The sampling radius was constant while the paraboloids have more pixel density near the Z axis, so it resulted in an inconsistent blurring size depending on the light's orientation. This was easy to fix by scaling the sampling radius according to the Z coordinates of the shadow ray. The other issue appeared because blur samples were not clamped inside the two circles that form the paraboloid, so with a large blur radius samples ended up outside the paraboloid regions and picked up invalid data. I added some code to detect such cases and move the sample point to the other paraboloid, and the issue is gone.

Biasing tweaks

With all the above issues fixed, I was able to tweak both regular bias and normal bias. I made sure they worked independently of the light's radius, and so far I haven't found any cases where I needed to change the default values.

Remaining issues

Direct paraboloid mode

The acne issues weren't as severe in this mode, and some of the fixes I did also apply, but I didn't attempt to fix the artifacts that appear near the OmniLight's local Z plane, where the two paraboloids meet. There's also a lot of distortion in meshes that have long edges without subdivisions, but that's inherent to the way we render the paraboloids. That means this mode will be left as a faster but less accurate mode, it can be made to work in many situations, but it needs care when choosing the light's orientation and properly subdivided geometry.

Soft shadow darkening

This is a general issue for all light types. Increasing the radius of soft shadows (or shadow blur) results in a general darkening of the light. This happens because we take samples in a radius around the shaded surface point but then compare them all to the same depth, so samples that fall between the light and the shaded point are treated as shadowed even if they are not. In order to fix that, we should adjust the threshold depth of each sample. We can probably assume the surface to be flat, get the global position for each sample and recompute the depth threshold from there, but I haven't really tested it.

Edit: I fixed the shadow atlas allocation for omni lights, so now they correctly claim two atlas regions and avoid overlaps. I also went over the SpotLight3Ds shadows. I added the same treatment to the znear distance to improve precision and fixed a couple of bugs regarding normal biasing.

@clayjohn
Copy link
Member

clayjohn commented Aug 6, 2021

This happens because we take samples in a radius around the shaded surface point but then compare them all to the same depth, so samples that fall between the light and the shaded point are treated as shadowed even if they are not. In order to fix that, we should adjust the threshold depth of each sample. We can probably assume the surface to be flat, get the global position for each sample and recompute the depth threshold from there, but I haven't really tested it.

I read an interesting slide show recently that discussed this in the context of sampling for SSAO. I'll dig it up and see if it will be helpful. From what I remember, they found that the flat surface assumption led to bad acne in certain situations. They came up with a more clever approach.

edit: I found the paper, the clever approach was to assume the surface was flat. The reason it was so clever is that the approach to SSAO used did not have access to the normal buffer, so the flat surface assumption needed some clever projection of existing samples. In this case, the flat surface assumption is much easier and IMO makes sense :)

@JFonS JFonS force-pushed the fix-omni-shadow-bias branch from 7242d82 to ef2ca69 Compare August 9, 2021 09:42
@mrjustaguy
Copy link
Contributor

Current Artifact of this PR (v4.0.dev.custom_build [e456c1c45])has an issue with both The Mobile and Clustered Renderer when Soft Shadows are set to Hard in Project Settings for Omni Lights

Bug

Bug.zip

@JFonS
Copy link
Contributor Author

JFonS commented Aug 10, 2021

@mrjustaguy you are most likely seeing what I mention in the PR description:

I changed it so znear scales relative to the light's radius (0.005*radius) and acne caused by precision issues was gone. This means large lights can't be placed too close to shadow casters (i.e 0.5 meters for a 100 radius light) but dual paraboloid mode doesn't have this restriction, so users can go with that if needed.

In your case, a radius of 500 means anything closer than 2.5 units won't cast any shadows. There may be a way to avoid having this limitation, by using logarithmic depth or a reverse Z buffer, but it needs some research.

@mrjustaguy
Copy link
Contributor

The radius isn't the issue, lowering it doesn't change anything, and this doesn't appear with any sort of soft shadows (low or ultra) in Project settings. Also Dual Paraboloid results in the exact same artifact.. Any Omni Light when Project Settings Soft Shadow Quality is set Hard will have the issue pictured above.

In conclusion, this isn't an issue with that. Yes the radius is large, and it is so because I originally came to see how much it'd impact things, and I've concluded that That limitation is really for me not going to be any sort of a problem, however having an option for tweaking that might be useful for some people's projects.

Aside from this issue when Project Settings Shadows are set to Hard instead of Soft however, everything works as expected and is a Huge improvement on the existing stuff and I haven't been able to find any other issues in my testing (aside from the 2 Known issues you've stated above)

@JFonS
Copy link
Contributor Author

JFonS commented Aug 10, 2021

Alright, I will take a look at the attached project then :)

@JFonS JFonS force-pushed the fix-omni-shadow-bias branch 2 times, most recently from c2551a4 to cd0f5cc Compare August 13, 2021 14:28
@JFonS JFonS changed the title Improvements to OmniLight3D's shadows Improvements to SpotLight3D and OmniLight3D's shadows Aug 13, 2021
@JFonS JFonS marked this pull request as ready for review August 13, 2021 14:29
@JFonS JFonS requested review from a team as code owners August 13, 2021 14:29
@JFonS JFonS force-pushed the fix-omni-shadow-bias branch from cd0f5cc to 017f5a1 Compare August 13, 2021 14:30
@JFonS
Copy link
Contributor Author

JFonS commented Aug 13, 2021

I updated this PR and should be ready for review now. I took the time to go over spot light shadows as well and fixed a couple of things, they should work much better now.

@mrjustaguy The issue you mentioned was indeed a mistake in the hard shadows code path, that should be fixed as well.

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.

Looks great! Good work rooting out tons of difficult to find bugs :)

@JFonS JFonS force-pushed the fix-omni-shadow-bias branch from 017f5a1 to 26d8315 Compare August 13, 2021 21:31
@JFonS
Copy link
Contributor Author

JFonS commented Aug 13, 2021

I updated the PR addressing the feedback. The CI fails on the doctool, but I was looking into it earlier today and it seems to be bogus behavior in the doc generation itself.

Light3D can't be instanced, so in order to get the default property values, it instances one inheriting class that can be instanced. So for OmniLight3D sometimes the default value is checked against OmniLigh3D itself and sometimes against some of the other light types.

@JFonS JFonS force-pushed the fix-omni-shadow-bias branch from 26d8315 to 55d8551 Compare August 19, 2021 10:46
OmniLight3D:
* Fixed lack of precision in cube map mode by scaling the projection's
  znear.
* Fixed aliasing issues by making the paraboloids use two square regions instead of two half
  squares.
* Fixed shadowmap atlas bleeding by adding padding.
* Fixed sihadow blur's inconsistent radius and unclamped sampling.

SpotLight3D:
* Fixed lack of precision by scaling the projection's znear.
* Fixed normal biasing.

Both:
* Tweaked biasing to make sure it works out of the box in most situations.
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