-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Implement percentage-closer soft shadows (PCSS). #13497
Conversation
5167130
to
0377266
Compare
[*Percentage-closer soft shadows*] are a technique from 2004 that allow shadows to become blurrier farther from the objects that cast them. It works by introducing a *blocker search* step that runs before the normal shadow map sampling. The blocker search step detects the difference between the depth of the fragment being rasterized and the depth of the nearby samples in the depth buffer. Larger depth differences result in a larger penumbra and therefore a blurrier shadow. To enable PCSS, fill in the `soft_shadow_size` value in `DirectionalLight`, `PointLight`, or `SpotLight`, as appropriate. This shadow size value represents the size of the light and should be tuned as appropriate for your scene. Higher values result in a wider penumbra (i.e. blurrier shadows). When using PCSS, temporal shadow maps (`ShadowFilteringMethod::Temporal`) are recommended. If you don't use `ShadowFilteringMethod::Temporal` and instead use `ShadowFilteringMethod::Gaussian`, Bevy will use the same technique as `Temporal`, but the result won't vary over time. This produces a rather noisy result. Doing better would likely require downsampling the shadow map, which would be complex and slower (and would require PR bevyengine#13003 to land first). In addition to PCSS, this commit makes the near Z plane for the shadow map configurable on a per-light basis. Previously, it had been hardcoded to 0.1 meters. This change was necessary to make the point light shadow map in the example look reasonable, as otherwise the shadows appeared far too aliased. A new example, `pcss`, has been added. It demonstrates the percentage-closer soft shadow technique with directional lights, point lights, spot lights, non-temporal operation, and temporal operation. The assets are my original work. Both temporal and non-temporal shadows are rather noisy in the example, and, as mentioned before, this is unavoidable without downsampling the depth buffer, which we can't do yet. Note also that the shadows don't look particularly great for point lights; the example simply isn't an ideal scene for them. Nevertheless, I felt that the benefits of the ability to do a side-by-side comparison of directional and point lights outweighed the unsightliness of the point light shadows in that example, so I kept the point light feature in. Fixes bevyengine#3631. [*Percentage-closer soft shadows*]: https://developer.download.nvidia.com/shaderlibrary/docs/shadow_PCSS.pdf
I don't know the domain well- why do the shadows here: Have curve-like imperfections? I noticed the article you linked has this image which has a different type of noise: Is it related to the geometry which casts the shadow or something else? Also I was wondering how the shadows perform when the light source is moving. pcss-example-undulate.patch.txt Feel free to pcss-undulate.mp4I noticed an effect that spot lights have this effect where it looks like the shadow density changes a lot, perhaps related to shadow map resolution? |
The patterns are because of the interleaved gradient noise. This is generally considered more desirable than the white noise that it looks like your other example has. There isn't a whole lot we can do about the noise in this patch without generating mip chains for the shadow map, which would require #13003 (and even if that were in I wouldn't want to do it in the first patch because this patch is too big as it is). When I asked in Discord I was told that PCSS in games tends to be noisy; that's just the way it is. I think PCSS is mostly used in directional lights in practice. The results for point lights in particular just aren't as good. |
I considered allowing the user to move the light but I realized that this will cause problems with the Z near value, which is carefully tuned so that the palm tree makes maximum use of the shadow map. So I'm less inclined to make the light movable, though I could be convinced otherwise. |
I suspect what's going wrong with spot lights is that the blocker search starts missing blockers. I'm not sure if this is something that can reasonably be fixed; it's always possible that the blocker search can miss things. The solution is for artists to tweak the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out that Godot has PCSS and they have an example showcasing it: https://github.com/godotengine/godot-demo-projects/tree/master/3d/lights_and_shadows
I put the GLTF files in this PR in that project and dropped the palm tree into Godot and tried experimenting with the PCSS lights in that example.
I couldn't try the spot light (likely due to godotengine/godot#80137) but I saw similar results when using their omni light (point light) and their sun (directional light).
I learned that it's very finicky to get decent looking shadows, and I had to resort to a mix of tuning their shadow blur and their shadow opacity (not sure if we have that in Bevy yet) and only specific combos of those along with light strength and positioning yielded OK results.
I'm convinced that PCSS is hard to get right (as a user) and that this PR adds value and if we can get better noise patterns further down the line that's really great.
EDIT: When we have a Bevy editor where we can interactively tune parameters like I could in Godot it will be a lot easier to spot problems so I'm not worried about any minor issues right now.
Approving on the basis of the above, as well as:
- Code quality to me looks great
- Adds great docs
- Great example
I haven't verified any math or looked in depth at the shaders- hoping rendering maintainers will do so.
Yeah. Unity doesn't implement PCSS at all and in this PR I pretty much learned why. PCSS is just really hard to tune. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the code looks good to me. The new docs for the existing shadow stuff is really nice.
As mentioned on discord, the volumetric_fog and trransmission example currently crash. Once those are fixed and assuming conflicts are fixed I'll approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, just a small nit about clarifying some unused uncommented fields.
pub(crate) pad_a: f32, | ||
pub(crate) pad_b: f32, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is this intended to make the structure's size a multiple of 4 x f32? If so a comment and maybe a test might be good, assuming it's important and relied upon elsewhere. Maybe something like this would also make the intention clearer and more maintainable:
pub(crate) pad_a: f32, | |
pub(crate) pad_b: f32, | |
pub(crate) _reserved: [f32; 2], |
The test could be as simple as this:
#[cfg(test)]
const_assert_eq!(size_of::<GpuClusterableObject>() % size_of::<Vec4>(), 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test isn't that necessary for that. Things break pretty quickly if the padding is wrong anyway. We have padding in a bunch of places for rendering code because of some webgl limitations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, no problem if it’s convention - although should it be commented all the same? I was thinking the test wasn't so much about ensuring it breaks, but ensuring the issue is determined statically and in the right location to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, just the fact that it's called pad_x
is enough documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Righto, again no problem if it's an established convention in the codebase.
I was initially unsure whether this was perhaps something like padding from the edge of the shadow when sampling to avoid boundary conditions. It required me to check it wasn't used to understand its purpose and intention, but if I was adding a new field I may not have checked so thoroughly and just added new fields rather than removing these.
Again, it's a nit, and not a huge issue. I just saw it as something that could be made clearer. My intention if it's clearer is that more people can get across these changes, review, and contribute. Being knowledgable about graphics doesn't mean you're knowledgable about conventions in this codebase, or rust, etc. My understanding of Rust convention, for example, is that fields like this would have an underscore prefix _pad_a
to indicate they're intentionally unused.
Please don't merge this yet as spot shadows are broken somehow due to merge fallout. |
4, | ||
5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a change for this PR, but after this is merged would a PR be welcome to replace these values with constants? and defined in the shader with defines like this:
@group(0) @binding(#TONEMAPPING_LUT_TEXTURE_BINDING_INDEX) var dt_lut_texture: texture_3d<f32>; | |
@group(0) @binding(#TONEMAPPING_LUT_SAMPLER_BINDING_INDEX) var dt_lut_sampler: sampler; |
It might reduce merge conflict churn when there's lots of unrelated changes in a PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I recall, non-boolean defines are kind of annoying in our PBR shaders at the moment, but sure, it could be done in the future.
Rebased and comments addressed. I think this is OK to go unless it needs a rereview. |
@alice-i-cecile I think I fixed DX12. |
This failure is because |
Should be fixed again. |
I love WebGL2 🥲 Thanks for fixing this up <3 |
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1688 if you'd like to help out. |
Percentage-closer soft shadows are a technique from 2004 that allow shadows to become blurrier farther from the objects that cast them. It works by introducing a blocker search step that runs before the normal shadow map sampling. The blocker search step detects the difference between the depth of the fragment being rasterized and the depth of the nearby samples in the depth buffer. Larger depth differences result in a larger penumbra and therefore a blurrier shadow.
To enable PCSS, fill in the
soft_shadow_size
value inDirectionalLight
,PointLight
, orSpotLight
, as appropriate. This shadow size value represents the size of the light and should be tuned as appropriate for your scene. Higher values result in a wider penumbra (i.e. blurrier shadows).When using PCSS, temporal shadow maps (
ShadowFilteringMethod::Temporal
) are recommended. If you don't useShadowFilteringMethod::Temporal
and instead useShadowFilteringMethod::Gaussian
, Bevy will use the same technique asTemporal
, but the result won't vary over time. This produces a rather noisy result. Doing better would likely require downsampling the shadow map, which would be complex and slower (and would require PR #13003 to land first).In addition to PCSS, this commit makes the near Z plane for the shadow map configurable on a per-light basis. Previously, it had been hardcoded to 0.1 meters. This change was necessary to make the point light shadow map in the example look reasonable, as otherwise the shadows appeared far too aliased.
A new example,
pcss
, has been added. It demonstrates the percentage-closer soft shadow technique with directional lights, point lights, spot lights, non-temporal operation, and temporal operation. The assets are my original work.Both temporal and non-temporal shadows are rather noisy in the example, and, as mentioned before, this is unavoidable without downsampling the depth buffer, which we can't do yet. Note also that the shadows don't look particularly great for point lights; the example simply isn't an ideal scene for them. Nevertheless, I felt that the benefits of the ability to do a side-by-side comparison of directional and point lights outweighed the unsightliness of the point light shadows in that example, so I kept the point light feature in.
Fixes #3631.
Changelog
Added
Percentage-closer soft shadows (PCSS) are now supported, allowing shadows to become blurrier as they stretch away from objects. To use them, set the
soft_shadow_size
field inDirectionalLight
,PointLight
, orSpotLight
, as applicable.The near Z value for shadow maps is now customizable via the
shadow_map_near_z
field inDirectionalLight
,PointLight
, andSpotLight
.Screenshots
PCSS off:
PCSS on: