-
-
Notifications
You must be signed in to change notification settings - Fork 21.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
[3.x] Add support for 3 directional shadow splits #75468
Conversation
Great work! This would be useful to have in 4.x as well, as sometimes 3 splits is exactly what you need in a scene as a compromise between performance and quality. |
@@ -386,7 +386,7 @@ void DirectionalLight::_bind_methods() { | |||
ClassDB::bind_method(D_METHOD("is_blend_splits_enabled"), &DirectionalLight::is_blend_splits_enabled); | |||
|
|||
ADD_GROUP("Directional Shadow", "directional_shadow_"); | |||
ADD_PROPERTY(PropertyInfo(Variant::INT, "directional_shadow_mode", PROPERTY_HINT_ENUM, "Orthogonal (Fast),PSSM 2 Splits (Average),PSSM 4 Splits (Slow)"), "set_shadow_mode", "get_shadow_mode"); | |||
ADD_PROPERTY(PropertyInfo(Variant::INT, "directional_shadow_mode", PROPERTY_HINT_ENUM, "Orthogonal (Fast),PSSM 2 Splits (Average),PSSM 3 Splits (Slow),PSSM 4 Splits (Very Slow)"), "set_shadow_mode", "get_shadow_mode"); |
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.
This breaks compatibility AFAIK, as we save integer values in scene/resource files.
So a Godot 3.5 user having configured 4 splits will find themselves automatically moved to 3 splits when opening their project in 3.6, if this is merged as is. We could consider this intentional and document it clearly as such, but we should discuss it.
An option to prevent the compatibility breakage is to add the 3 splits option at the end of the enum (both here and in the actual C++ enum), so the integer values aren't changed. It will look a bit out of order, but preserves compatibility.
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.
So a Godot 3.5 user having configured 4 splits will find themselves automatically moved to 3 splits when opening their project in 3.6, if this is merged as is.
The default is 4 splits (2
in the enum), so it's not saved to the scene file. This conveniently avoids this issue, as non-default values (Orthogonal 0
/PSSM 2 Splits 1
) are stored in indices before the enum order is broken.
The only case where breakage might occur is when a user sets the shadow mode using an integer value directly (not using the enum constant), but it's very unlikely.
Before merging we need to have a sense of the performance of doing this vs 2 and 4 splits. As a general rule of thumb, we shouldn't merge performance optimizations without profiling first. |
I've created a testing project: test_shadow_3_splits_3.x.zip Test results on a GeForce RTX 4090 in 3840×2160: GLES3
GLES2
In scenes with lots of instances (= lots of draw calls), there's a definitive performance improvement when using 3 splits as opposed to 4 with the same Shadow Max Distance property. What's strange is that official 3.5.2 releases are much faster in both GLES3 and GLES2, despite my self-compiled binary using GLES3
GLES2
This is also an issue with my self-compiled builds with the commit from this PR reverted, so I don't know what's causing this. Either way, I can still measure a performance improvement – the figures with a local build and the PR reverted are identical to the results displayed in the first two tables. Note that godotengine/godot-proposals#3464 could help improve the appearance of PSSM 2 Splits out of the box, making it usable in more situations without requiring a manual adjustment. That said, 3 splits is still visually superior in the vast majority of cases. |
Rendering meeting today: All are happy with this feature, but the enums should preserve backward compatibility, hence:
Even though it is slightly strange. |
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.
Based on what @Calinou says about the previous default value (2) not being serialized, this would seem to resolve the issue about breaking compat.
Thanks! |
Currently, Godot only supports either 1, 2, or 4 shadow cascades.
However, later splits are exponentially more expensive, so especially for games with large & complex scenes having to render multiple cascades with very large shadow-frustums is not ideal.
This PR makes it possible to use three cascades. While doing so results in a slight loss of shadow quality no longer having to render an entire cascade can speed up shadow performance by 15-20% (according to Renderdoc).
This PR was sponsored by Ramatak with 💚