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] Backport DirectionalLight fade_start property #60246

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Apr 14, 2022

Follow-up to #60186 (can be merged independently).

  • Implement shadow fading when using the Orthogonal shadow mode (like in master).

This allows customizing the distance at which directional shadows start to fade away. Shadow fading will also always start at the same distance now, regardless of the current shadow mode in use.

This is useful for enclosed levels to prevent shadows from fading at all with a well-tuned maximum distance.

The default fade start value (0.8) results in fading happening later in the distance compared to the previous behavior, where fading started from the last shadow split distance (0.6 in PSSM 4 Splits and 0.1 in PSSM 2 Splits). I can decrease this default fade start value if desired, but I think this new default is better suited to most use cases.

Testing project: test_shadow_fade_start_3.x.zip

Preview

Screenshots taken with GLES3, but this feature works the same way in GLES2.

Fade Start = 0.8 (default)

Orthogonal PSSM 2 Splits PSSM 4 Splits
Screenshot_20231120_123516 Screenshot_20231120_123535 Screenshot_20231120_123542

Different Fade Start values with PSSM 4 Splits

0.4 1.0
Screenshot_20231120_123604 Screenshot_20231120_123610

@Calinou Calinou requested review from a team as code owners April 14, 2022 18:02
@Calinou Calinou added this to the 3.5 milestone Apr 14, 2022
@Calinou Calinou force-pushed the directional-light-add-fade-start-3.x branch 2 times, most recently from 8225ea2 to 659ac9e Compare April 15, 2022 16:12
@akien-mga akien-mga modified the milestones: 3.5, 3.x Jul 2, 2022
@Calinou Calinou assigned clayjohn and unassigned clayjohn Jul 4, 2022
@Calinou Calinou force-pushed the directional-light-add-fade-start-3.x branch from 659ac9e to c0e3127 Compare June 27, 2023 07:06
@Calinou Calinou changed the title Backport DirectionalLight fade_start property to 3.x [3.x] Backport DirectionalLight fade_start property to Jun 27, 2023
@lawnjelly
Copy link
Member

Looks ok, but this is breaking I think, in the sense it will change what the directional shadows look like. Would it be worth introducing a compatibility switch in project settings, to change the shaders back and forth? @clayjohn not sure what you think.

@Calinou Calinou changed the title [3.x] Backport DirectionalLight fade_start property to [3.x] Backport DirectionalLight fade_start property Nov 15, 2023
@Calinou
Copy link
Member Author

Calinou commented Nov 15, 2023

Looks ok, but this is breaking I think, in the sense it will change what the directional shadows look like. Would it be worth introducing a compatibility switch in project settings, to change the shaders back and forth? @clayjohn not sure what you think.

We usually accept minor visual changes between minor versions, especially if they'll look better in the vast majority of projects (which I expect to be the case here, as you'll be able to see shadows further with the same performance cost). Tweaking the scene to match the previous visuals is also very fast to do.

Adding a compatibility project setting would be a lot of effort for little gain.

// Using 1.0 would break `smoothstep()` in the shader.
state.scene_shader.set_uniform(SceneShaderGLES2::FADE_FROM, -split_offsets[shadow_count - 1] * MIN(fade_start, 0.999));
state.scene_shader.set_uniform(SceneShaderGLES2::FADE_TO, -split_offsets[shadow_count - 1]);

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to send this? Can we use one of the light_split_offsets in the shader for the furthest value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've copied the 4.x approach, I'm not sure if we can reduce the number of uniforms sent without changing how it looks.

Copy link
Member

Choose a reason for hiding this comment

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

Well for SceneShaderGLES2::FADE_TO I couldn't help noticing we have already sent this value 5 lines above (as split_offsets), and each uniform is additional extra API (especially in GLES2), and can compete for limited space in GPU for highly cached stuff.

The only question is which of these 4 is the furthest split, but I'm presuming this must have been previously calculated in the shader (for it to have worked previously).

There are 2 questions that appear:

  • Is it more expensive to use the existing if clauses than to send as an independent additional uniform (this may be the case, dynamic branches aren't good but maybe we have paid the price already)
  • The existing light_split_offset is stored as highp. This might not be necessary for the calculation so could maybe be downcast.

I guess these can only be answered by performance testing especially on mobile. And I guess could be left to a follow up. I'm not sure whether this was considered in 4.x, but maybe there were additional factors at play.

@lawnjelly
Copy link
Member

Some screenshots showing the improvement would be handy too! 👍

@Calinou Calinou force-pushed the directional-light-add-fade-start-3.x branch from c0e3127 to 5821b62 Compare November 20, 2023 11:30
@Calinou Calinou force-pushed the directional-light-add-fade-start-3.x branch from 5821b62 to 29214e9 Compare November 20, 2023 11:39
@Calinou
Copy link
Member Author

Calinou commented Nov 20, 2023

Some screenshots showing the improvement would be handy too! 👍

I've added screenshots to OP and fixed Fade Start when using the new PSSM 3 Splits shadow mode.

- Implement shadow fading when using the Orthogonal shadow mode
  (like in `master`).

This allows customizing the distance at which directional shadows
start to fade away. Shadow fading will also always start at the same
distance now, regardless of the current shadow mode in use.

This is useful for enclosed levels to prevent shadows from fading
at all with a well-tuned maximum distance.

The default fade start value (0.8) results in fading happening later
in the distance compared to the previous behavior, where fading started
from the last shadow split distance (0.6 in PSSM 4 Splits and
0.1 in PSSM 2 Splits).
@Calinou Calinou force-pushed the directional-light-add-fade-start-3.x branch from 29214e9 to 4fefb13 Compare November 20, 2023 11:42
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Approve but could do with @clayjohn checking as well as he may bring up some things I didn't think of.

@akien-mga akien-mga modified the milestones: 3.x, 3.6 Dec 8, 2023
@akien-mga akien-mga merged commit dc776e4 into godotengine:3.x Feb 7, 2024
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the directional-light-add-fade-start-3.x branch February 8, 2024 10:16
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.

None yet

5 participants