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

[3.x] Add PCF25 shadow filtering option in the GLES3 renderer #54355

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Oct 28, 2021

This is a high-quality shadow filter which results in smooth, stable shadows even for dynamic objects.

Thanks to early bailing, PCF25 is faster than the old PCF13 implementation while looking better. It's also not that much slower compared to the new PCF13 implementation.

Preview

Disabled PCF5 PCF13 PCF25
2021-10-28_16 55 12 2021-10-28_16 54 44 2021-10-28_16 57 18 2021-10-28_16 55 42

PCF13 with 2048 shadow size (faster but less smooth)

shadow-filtering-pcf13-2048.mp4

PCF25 with 4096 shadow size (slower but smoother)

shadow-filtering-pcf25-4096.mp4

@lawnjelly
Copy link
Member

Bit of a bodge, but as this is an enum rather than needing conditionals for each (i.e. you are either using 0, 5, 13 or 25) maybe you can get by with two conditional flags...

00 = 0
01 = 5
10 = 13
11 = 25

But yeah sounds like it could do with some looking at the conditional code to see if there's a better solution longterm.

@Calinou
Copy link
Member Author

Calinou commented Apr 13, 2022

I applied @lawnjelly's solution to get the configuration option to work. I tested the PR on both GLES3 and GLES2, with and without asynchronous shader compilation. It works as expected in both renderers.

Note that in GLES2, the PCF25 option is equivalent to the PCF13 option. It's possible to implement a higher quality shadow filter for GLES2, but it'd be very slow on the target hardware.

@Calinou Calinou force-pushed the shadow-filter-add-pcf25 branch from cc577b6 to 85329ec Compare April 13, 2022 19:05
@Calinou Calinou marked this pull request as ready for review April 13, 2022 19:05
@Calinou Calinou requested review from a team as code owners April 13, 2022 19:05
@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 2, 2022
@Calinou Calinou changed the title Add PCF25 shadow filtering option in the GLES3 renderer [3.x] Add PCF25 shadow filtering option in the GLES3 renderer Jun 27, 2023
This is a high-quality shadow filter which results in smooth,
stable shadows even for dynamic objects.

Thanks to early bailing, PCF25 is faster than the old PCF13
implementation while looking better. It's also not that much slower
compared to the new PCF13 implementation.
@Calinou Calinou force-pushed the shadow-filter-add-pcf25 branch from 85329ec to 13e679a Compare May 18, 2024 17:21
@Calinou
Copy link
Member Author

Calinou commented May 18, 2024

Rebased and tested again, it works as expected.

I've changed the early bailing implementation to also take the center sample into account (also for the existing PCF13). This is a tad slower as it requires one more sample, but it leads to fewer artifacts, particularly with PCF25. This matches the implementation used in the 4.x Compatibility rendering method.

#ifdef SHADOW_MODE_PCF_13 //ubershader-runtime
float avg = textureProj(shadow, vec4(pos, depth, 1.0));
#ifdef SHADOW_MODE_PCF_LOW //ubershader-runtime
#ifdef SHADOW_MODE_PCF_HIGH //ubershader-runtime
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this? 🤔

#if defined(SHADOW_MODE_PCF_LOW) && defined(SHADOW_MODE_PCF_LOW)

This pattern seems to be used elsewhere in the shaders and it seems easier to follow.

Copy link
Member Author

@Calinou Calinou May 30, 2024

Choose a reason for hiding this comment

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

I think this messes with //ubershader-runtime, unfortunately. We can use this approach in 4.x though where ubershaders aren't used in Compatibility.

@lawnjelly
Copy link
Member

lawnjelly commented May 29, 2024

I tried this out and it seems to work fine.

Not really for this PR, but it strikes me at 25 samples, we should be making the PCF radius adjustable per light. With a uniform multiplier in sample_shadow() for shadow_pixel_size we could adjust the softness, and trade off with the PCF samples.

Also I do wonder for PCF 25 whether a regular pattern gives the best results versus random (even fixed random pattern). Again maybe this should be adjustable in the future. Hopefully we can greatly improve shadows in 3.7.

This is PCF25 with a 5x multiplier for shadow_pixel_size. Much softer and quite reasonable, except the regular sample pattern is artifacty, especially on axis aligned edges, see the difference between the building and the side of the car.

Screenshot from 2024-05-29 11-22-56

@Calinou
Copy link
Member Author

Calinou commented May 29, 2024

Not really for this PR, but it strikes me at 25 samples, we should be making the PCF radius adjustable per light. With a uniform multiplier in sample_shadow() for shadow_pixel_size we could adjust the softness, and trade off with the PCF samples.

There's an app PR for it 🙂 #60243

This feature also exists in 4.x already, but the Light3D Shadow Blur property only affects Forward+ and Mobile right now. I started working on a 4.x implementation for Compatibility, but didn't get it working yet: https://github.com/Calinou/godot/tree/compatibility-shadow-blur

Also I do wonder for PCF 25 whether a regular pattern gives the best results versus random (even fixed random pattern).

#53967 can help further improve shadow filtering quality at the cost of visible noise, like in Forward+/Mobile in 4.x.

@lawnjelly
Copy link
Member

There's an app PR for it 🙂 #60243

Great! I'm keen for us to improve the shadows in 3.7, including e.g. fixed filter per light or PCSS or similar, and normal bias offset. We'll probably discuss these PRs in more detail over coming months and decide which approaches to go for.

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.

3 participants