Skip to content

[3.x] Use less blur for distant directional shadow splits#60186

Open
Calinou wants to merge 1 commit into
godotengine:3.xfrom
Calinou:directional-shadow-distant-split-lower-blur-3.x
Open

[3.x] Use less blur for distant directional shadow splits#60186
Calinou wants to merge 1 commit into
godotengine:3.xfrom
Calinou:directional-shadow-distant-split-lower-blur-3.x

Conversation

@Calinou

@Calinou Calinou commented Apr 12, 2022

Copy link
Copy Markdown
Member

3.x version of #48776.

This makes the transition between shadow splits less noticeable, especially when the expensive Blend Splits property is disabled.

This is only effective when the shadow filter mode is PCF5 or PCF13.

Testing project: test_shadow_b_3.x.zip

Preview

GLES3 (PCF13)

Before After
2022-04-13_17 55 55 2022-04-13_17 55 43
Old revision of this PR

GLES3

Before After
2022-04-13_00 30 16 2022-04-13_00 30 27

GLES2

Before After
2022-04-13_00 29 10 2022-04-13_00 29 24

@clayjohn clayjohn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two problems here:

  1. Using a lower form of PCF for further splits results on visibly worse quality shadows, I don't think we should force that on users
  2. Sampling textures in if/else blocks can be a problem on older GPUs as the GPU ends up running both sides of the branch (and thus waiting on the results of both texture lookups). There is a reason this section of code is structured to do the texture lookups after the branching. On high-end GPUs the difference likely won't be noticable, but on low end or older GPUs this can really hurt performance.

A better approach is to do what you did in 4.0 and adjust the directional_shadow_pixel_size based on which split you are in. This avoids both issues above and should result in the same benefits as your 4.0 PR.

@lawnjelly

Copy link
Copy Markdown
Member

Yes I have to say that the before version looks better to my eye, especially in GLES2. The splits may be less noticeable, but the shadows look more blocky.

@Calinou Calinou changed the title Use less blur for distant directional shadow splits (3.x) Use less blur for distant directional shadow splits in GLES3 (3.x) Apr 13, 2022
@Calinou Calinou force-pushed the directional-shadow-distant-split-lower-blur-3.x branch from 62a3a67 to 8c76e87 Compare April 13, 2022 15:57
@Calinou

Calinou commented Apr 13, 2022

Copy link
Copy Markdown
Member Author

I applied @clayjohn's suggestions. It seems to be working well and performs identically compared to 3.x, but I could only apply the suggestion to GLES3. Therefore, this pull request no longer has an impact when using the GLES2 renderer.

PS: Seeing this directional_shadow_pixel_size makes me think we could expose a per-light shadow blur property in 3.x when using GLES3 🙂
It's likely best to wait for #54355 to be merged first though.

Edit: Video of this pull request with #54355's PCF25 shadow filtering on top:

vid.mp4

@Calinou Calinou force-pushed the directional-shadow-distant-split-lower-blur-3.x branch from 8c76e87 to 19075dd Compare April 14, 2022 16:21
@Calinou

Calinou commented Apr 14, 2022

Copy link
Copy Markdown
Member Author

I've gotten it to work in GLES2 again – it looks good from my testing. Please check if the approach used in GLES2's scene.glsl has any adverse effects 🙂

This makes the transition between shadow splits less noticeable,
especially when the expensive Blend Splits property is disabled.

This is only effective when the shadow filter mode is PCF5 or PCF13.
@Calinou Calinou force-pushed the directional-shadow-distant-split-lower-blur-3.x branch from 19075dd to 0cd4e10 Compare April 14, 2022 16:25

@clayjohn clayjohn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks much better now. Let's discuss this with other rendering contributors and users. I'm concerned that the visual change is too much to justify making this change to a stable release. I see the benefit of making ths shadow blur stable as it is in 4.0, but, in my opinion, the before scenes look a little nicer than after. This is likely due to how PCF works and may not be avoidable. As it is purely a visuel change (performance should be about the same) I want to make sure we get as much input as we can.

@Calinou

Calinou commented Apr 19, 2022

Copy link
Copy Markdown
Member Author

I see the benefit of making ths shadow blur stable as it is in 4.0, but, in my opinion, the before scenes look a little nicer than after. This is likely due to how PCF works and may not be avoidable.

The benefit of this PR should be much more noticeable in motion, as the difference between splits becomes much less noticeable when the camera moves. Screenshots don't do great justice for this PR 🙂

@Calinou Calinou changed the title Use less blur for distant directional shadow splits in GLES3 (3.x) Use less blur for distant directional shadow splits (3.x) Apr 19, 2022
@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 2, 2022
@Calinou Calinou changed the title Use less blur for distant directional shadow splits (3.x) [3.x] Use less blur for distant directional shadow splits Jun 27, 2023
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