-
-
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
Avoid calculating dynamic lights when lights are already baked using the static bake mode in the Forward+ renderer #96771
Conversation
For context, there's already a skip in place here for directional lights: godot/servers/rendering/renderer_rd/shaders/forward_clustered/scene_forward_clustered.glsl Lines 1853 to 1855 in 694d3c2
It does the job when shadows are enabled, but not when shadows are disabled. This PR fixes double lighting in Forward+ when using a DirectionalLight3D with shadows disabled (although note that its shadows will be baked anyway). To clarify, the bug I mentioned in #85145 does not occur in Forward+ (neither in 4.3.stable nor I've tested this PR locally and it fixes double lighting when shadows are disabled on a DirectionalLight3D in Forward+. |
That other check only disables shadows. The lighting was added in either case. For the mobile renderer the check shouldn't be in the shader, it should be excluding the light from the light list when pairing with the object so we don't get any overhead from the light at all |
e69c1db
to
e226b07
Compare
@Calinou Updated! In the end this was the situation:
It should be ready to go now! |
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 mostly works as expected in all rendering methods. Performance increases as expected when baking with the Static bake mode as opposed to the default Dynamic.
Testing project: test_lightmapgi_mobile.zip
There's one issue where DirectionalLight3D with shadow disabled still applies double lighting, but only in Compatibility:
DirectionalLight3D shadow enabled (Compatibility)
DirectionalLight3D shadow enabled (Compatibility)
OmniLight3D and SpotLight3D don't have this issue in Compatibility.
…I using the static bake mode
e226b07
to
9320865
Compare
@Calinou Thanks for checking! It turned out that the vertex lighting codepath in the GLES3 shader was missing the check. I added it and removed the static checks for omnilights and spotlights since they weren't necessary anymore. This should be ready to go now! |
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.
Works great now 🙂
@@ -2211,6 +2211,10 @@ void fragment_shader(in SceneData scene_data) { | |||
continue; //not masked | |||
} | |||
|
|||
if (directional_lights.data[i].bake_mode == LIGHT_BAKE_STATIC && bool(instances.data[instance_index].flags & INSTANCE_FLAGS_USE_LIGHTMAP)) { | |||
continue; // Statically baked light and object uses lightmap, skip |
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.
continue; // Statically baked light and object uses lightmap, skip | |
continue; // Statically baked light and object uses lightmap, skip. |
Thanks! |
Fixes an issue related to: #85145
When baking a light with the static bake mode, the light is calculated twice. Once when baking and once at runtime. The light values are added together resulting in double lighting.
Shadows are already skipped using the exact same code, but the actual lighting was still being added.
Opening as draft as I need to check the mobile renderer and the compatibility renderer