-
-
Notifications
You must be signed in to change notification settings - Fork 21.9k
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
Improve split blending logic for Vulkan #82668
Conversation
servers/rendering/renderer_rd/shaders/forward_clustered/scene_forward_clustered.glsl
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/shaders/forward_clustered/scene_forward_clustered.glsl
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/shaders/forward_clustered/scene_forward_clustered.glsl
Outdated
Show resolved
Hide resolved
After doing changes, could you squash the commits? See PR workflow for instructions. |
Improve Split Blending Logic
There, that should be it, I think. |
I believe the C++ side of the culling code should be changed as well, so that splits don't need to include as many shadows. Otherwise, you're leaving performance on the table 🙂 Start from here: godot/servers/rendering/renderer_scene_cull.cpp Line 2118 in 0ca8542
|
I'm not really good with C++, and am even less familiar with the culling logic, so it'd be better if someone else were to figure that out, to claw some free performance with Split Blending that was enabled, as it's not like this PR is negatively affecting performance given the fact it's rendering the same shadow maps as before when Blending is enabled. Given the already fairly small difference Blending On vs Off has, I'm not sure how much of a priority that is right now anyhow, as there are bigger fish to fry |
Yea this PR is ready for review, my attempts at playing with the culling code have been as expected unfruitful. However, Not managing to improve the culling code doesn't Mean it was all for nothing! While scrolling around the code my gut picked up a very tiny error that needed to be fixed in order to fix the very long standing issue with 2 Split Stabilization (#59232).. So I guess that's.. something 🤣 Edit: 2 Split fix PR (#82974) |
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.
Seems fine to me. GLES3 works a bit differently so its okay to leave out. I would like to improve the culling ASAP as this blending logic should allow us to cull may more objects (plus with lawnjelly's recent culling work we should be able to make shadows much faster)
There is an OpenGL variant of this PR: #83253 |
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.
Tested locally, it works as expected. I still think this makes shadows too blurry at a distance, and maybe we should find a halfway compromise (or add a property to adjust how much distant shadows should be blurred).
However, it seems this PR doesn't improve performance in a meaningful way, which surprises me. Worse, this logic noticeably decreases performance when using the Ultra shadow filter quality setting, likely as a result of the increased blur in the distance.
Tested on a RTX 4090 in 3840×2160. DirectionalLight3D shadow blur property is set to 1.5
on all screenshots. See top-left corner of each screenshot for FPS.
Testing project: test_lightmap_preview_bake_4.x_3.zip
Soft Low
Blend Splits Disabled | Enabled, Before | Enabled, After (this PR) |
---|---|---|
![]() |
![]() |
![]() |
Soft Ultra
Blend Splits Disabled | Enabled, Before | Enabled, After (this PR) |
---|---|---|
![]() |
![]() |
![]() |
We likely need to rework in general how we pass the Blur to the GPU, to allow for adding more fine tune controls, and figure out a good UI for that, but that is true of a lot of the Directional Shadow Controls. See godotengine/godot-proposals#8197 for an overview of changes to consider, though this is for a future undertaking, by someone with a bit more experience with handling the UI side of these things.. |
I agree with the final cascade perhaps being a bit too blurry, but other than that I think this PR greatly improves the split blending logic and now it makes sense to enable Blend Splits because it visibly improves the appearance of shadows. |
Shadow blur and Blending distances should likely get exposed to the user in the future in some form, but that's outside the scope of this PR, and my capabilities to expose |
Thanks! |
Resolves: #73677
Implemented for Forward+ and Mobile Renderers.
Restored Old Blur Logic when using blending as it significantly improves the visual presentation compared to blending with the Current Blur Logic, and allows for Directional Shadows over Massive distances at good quality when blending is enabled.
The Blending has been hardcoded to start at 10% away from the end of a given split for simplicity's sake, as for most scenarios it works best and for other values you would typically need to be tweaking it on a per split basis along side having per split blur controls for best results, which would be quite complicated both to implement and for most users to properly use
Blending in action

Blending disabled
