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

PCF For DirectionalLight/SpotLight Shadows #8006

Merged
merged 482 commits into from
Oct 7, 2023

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Mar 9, 2023

Objective

Solution

  • Implements "The Witness"'s shadow map sampling technique.
    • Ported from @superdump's old branch, all credit to them :)
  • Implements "Call of Duty: Advanced Warfare"'s stochastic shadow map sampling technique when the velocity prepass is enabled, for use with TAA.
    • Uses interleaved gradient noise to generate a random angle, and then averages 8 samples in a spiral pattern, rotated by the random angle.
    • I also tried spatiotemporal blue noise, but it was far too noisy to be filtered by TAA alone. In the future, we should try spatiotemporal blue noise + a specialized shadow denoiser such as https://gpuopen.com/fidelityfx-denoiser/#shadow. This approach would also be useful for hybrid rasterized applications with raytraced shadows.
    • The COD presentation has an interesting temporal dithering of the noise for use with temporal supersampling that we should revisit when we get DLSS/FSR/other TSR.

Changelog

  • Added ShadowFilteringMethod. Improved directional light and spotlight shadow edges to be less aliased.

Migration Guide

  • Shadows cast by directional lights or spotlights now have smoother edges. To revert to the old behavior, add ShadowFilteringMethod::Hardware2x2 to your cameras.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 30, 2023
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

Basically ready to approve this, just a few nits/minor comments.

And we must make an issue and remember to implement Receiver Plane Depth Bias ( https://web.archive.org/web/20230309054654/http://developer.amd.com/wordpress/media/2012/10/Isidoro-ShadowMapping.pdf slides 36-38). We could additionally implement Adaptive Depth Bias for Shadows ( https://jcgt.org/published/0003/04/08/ ) with the tweaks from Adaptive Depth Bias for Soft Shadows ( https://w3-o.cs.hm.edu/users/nischwit/public_html/AdaptiveDepthBias_WSCG.pdf ) separately from this PR.

Comment on lines 8 to 9
// NOTE: Due to non-uniform control flow above, we must use the level variant of the texture
// sampler to avoid use of implicit derivatives causing possible undefined behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think now this is split into its own function that this is no longer true. I have some memory of the control flow only applying within a function scope. Though I also remember thinking that was weird given it sounds like shader compilers generally inline everything? @cwfitzgerald - is the non-uniform control flow for the entire execution of the shader after entering the region of non-uniform control flow? Or is it scoped to functions? Or is the validation scoped to functions but the rule does apply?

Copy link
Contributor

Choose a reason for hiding this comment

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

cwfitzgerald confirmed that naga should emit validation errors if the texture sampling is done inside non-uniform control flow even if the code calls to another function.

Still, I think the Level variant makes sense regardless for shadow maps as they are not mipmapped and don't need gradients / mip level calculation to be done.

}

// https://web.archive.org/web/20230210095515/http://the-witness.net/news/2013/09/shadow-mapping-summary-part-1
fn sample_shadow_map_castano_thirteen(light_local: vec2<f32>, depth: f32, array_index: i32) -> f32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

This implements the 5x5 filter size but the implementation in MJP's has code for 3, 5, 7. We could add options for 3 and 7 later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5x5 I found to give good results, didn't feel the need to do 3 or 7 atm, but yeah we can add it trivially in the future (at the cost of more pipeline key bits).

Copy link
Contributor

Choose a reason for hiding this comment

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

Or some view flags or something.

crates/bevy_pbr/src/render/shadows.wgsl Outdated Show resolved Hide resolved
// NOTE: Due to non-uniform control flow above, we must use the level variant of the texture
// sampler to avoid use of implicit derivatives causing possible undefined behavior.
#ifdef NO_ARRAY_TEXTURES_SUPPORT
return textureSampleCompareLevel(
Copy link
Contributor

Choose a reason for hiding this comment

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

See my other comment on this in the place where the code was moved. I want to double-check with cwfitzgerald. :)

@JMS55 JMS55 requested a review from superdump October 1, 2023 21:23
crates/bevy_pbr/src/render/shadow_sampling.wgsl Outdated Show resolved Hide resolved
// NOTE: Due to non-uniform control flow above, we must use the level variant of the texture
// sampler to avoid use of implicit derivatives causing possible undefined behavior.
#ifdef NO_ARRAY_TEXTURES_SUPPORT
return textureSampleCompareLevel(
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned, bother blocks are needed and both should be textureSampleCompareLevel.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 7, 2023
Merged via the queue into bevyengine:main with commit 1f95a48 Oct 7, 2023
21 checks passed
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
# Objective

- Improve antialiasing for non-point light shadow edges.
- Very partially addresses
bevyengine#3628.

## Solution

- Implements "The Witness"'s shadow map sampling technique.
  - Ported from @superdump's old branch, all credit to them :)
- Implements "Call of Duty: Advanced Warfare"'s stochastic shadow map
sampling technique when the velocity prepass is enabled, for use with
TAA.
- Uses interleaved gradient noise to generate a random angle, and then
averages 8 samples in a spiral pattern, rotated by the random angle.
- I also tried spatiotemporal blue noise, but it was far too noisy to be
filtered by TAA alone. In the future, we should try spatiotemporal blue
noise + a specialized shadow denoiser such as
https://gpuopen.com/fidelityfx-denoiser/#shadow. This approach would
also be useful for hybrid rasterized applications with raytraced
shadows.
- The COD presentation has an interesting temporal dithering of the
noise for use with temporal supersampling that we should revisit when we
get DLSS/FSR/other TSR.

---

## Changelog

* Added `ShadowFilteringMethod`. Improved directional light and
spotlight shadow edges to be less aliased.

## Migration Guide

* Shadows cast by directional lights or spotlights now have smoother
edges. To revert to the old behavior, add
`ShadowFilteringMethod::Hardware2x2` to your cameras.

---------

Co-authored-by: IceSentry <[email protected]>
Co-authored-by: Daniel Chia <[email protected]>
Co-authored-by: robtfm <[email protected]>
Co-authored-by: Brandon Dyer <[email protected]>
Co-authored-by: Edgar Geier <[email protected]>
Co-authored-by: Robert Swain <[email protected]>
Co-authored-by: Elabajaba <[email protected]>
Co-authored-by: IceSentry <[email protected]>
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

- Improve antialiasing for non-point light shadow edges.
- Very partially addresses
bevyengine#3628.

## Solution

- Implements "The Witness"'s shadow map sampling technique.
  - Ported from @superdump's old branch, all credit to them :)
- Implements "Call of Duty: Advanced Warfare"'s stochastic shadow map
sampling technique when the velocity prepass is enabled, for use with
TAA.
- Uses interleaved gradient noise to generate a random angle, and then
averages 8 samples in a spiral pattern, rotated by the random angle.
- I also tried spatiotemporal blue noise, but it was far too noisy to be
filtered by TAA alone. In the future, we should try spatiotemporal blue
noise + a specialized shadow denoiser such as
https://gpuopen.com/fidelityfx-denoiser/#shadow. This approach would
also be useful for hybrid rasterized applications with raytraced
shadows.
- The COD presentation has an interesting temporal dithering of the
noise for use with temporal supersampling that we should revisit when we
get DLSS/FSR/other TSR.

---

## Changelog

* Added `ShadowFilteringMethod`. Improved directional light and
spotlight shadow edges to be less aliased.

## Migration Guide

* Shadows cast by directional lights or spotlights now have smoother
edges. To revert to the old behavior, add
`ShadowFilteringMethod::Hardware2x2` to your cameras.

---------

Co-authored-by: IceSentry <[email protected]>
Co-authored-by: Daniel Chia <[email protected]>
Co-authored-by: robtfm <[email protected]>
Co-authored-by: Brandon Dyer <[email protected]>
Co-authored-by: Edgar Geier <[email protected]>
Co-authored-by: Robert Swain <[email protected]>
Co-authored-by: Elabajaba <[email protected]>
Co-authored-by: IceSentry <[email protected]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- Improve antialiasing for non-point light shadow edges.
- Very partially addresses
bevyengine#3628.

## Solution

- Implements "The Witness"'s shadow map sampling technique.
  - Ported from @superdump's old branch, all credit to them :)
- Implements "Call of Duty: Advanced Warfare"'s stochastic shadow map
sampling technique when the velocity prepass is enabled, for use with
TAA.
- Uses interleaved gradient noise to generate a random angle, and then
averages 8 samples in a spiral pattern, rotated by the random angle.
- I also tried spatiotemporal blue noise, but it was far too noisy to be
filtered by TAA alone. In the future, we should try spatiotemporal blue
noise + a specialized shadow denoiser such as
https://gpuopen.com/fidelityfx-denoiser/#shadow. This approach would
also be useful for hybrid rasterized applications with raytraced
shadows.
- The COD presentation has an interesting temporal dithering of the
noise for use with temporal supersampling that we should revisit when we
get DLSS/FSR/other TSR.

---

## Changelog

* Added `ShadowFilteringMethod`. Improved directional light and
spotlight shadow edges to be less aliased.

## Migration Guide

* Shadows cast by directional lights or spotlights now have smoother
edges. To revert to the old behavior, add
`ShadowFilteringMethod::Hardware2x2` to your cameras.

---------

Co-authored-by: IceSentry <[email protected]>
Co-authored-by: Daniel Chia <[email protected]>
Co-authored-by: robtfm <[email protected]>
Co-authored-by: Brandon Dyer <[email protected]>
Co-authored-by: Edgar Geier <[email protected]>
Co-authored-by: Robert Swain <[email protected]>
Co-authored-by: Elabajaba <[email protected]>
Co-authored-by: IceSentry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants