Bug with "Cached Lights" in 2D, destroying the illumination system

[caioraphael1]

Tested versions

I'm using 4.3-dev5, but the issue has been reproducible since 4.0.3, at least.

System information

Windows 10, Ryzen 3 3300x, 16gb RAM 2666mhz, GTX 1660 6gb.

Issue description

I've been having this problem for over a year now, testing it since the 4.0.3, but I'm still having problems without any improvement across any release.

The bug consists of some sort of cached information about pass textures used in the Light2D node, making the light emitted to be REALLY messed up.

The following video showcases the problem:

2024-04-19.11-52-59.mp4

At first glance, it doesn't seem to be having any issues, but if you make this light interact with a collective of other lights, you absolutely will lose your mind, as you'll see all the lights flickering, turning on and off. This all happens WITHIN the editor. It's enough to zoom in and out to see all the lights going crazy, or just pan the viewport.

2024-04-19.12-18-36.mp4

(I'm using the same shader from the first video)

I've spent a lot of time trying to grasp some sort of understanding about this bug, but without much luck, so my ability to explain it is limited.

I've tried creating 5 new brand projects after deleting the AppData/Local/Godot and AppData/Roaming/Godot, and all the released versions since 4.0.3, and the bug still happens even after all of this.

I've sent these new projects (with NO alteration in the Project Settings and any Node configuration whatsoever) to different people, and they all have the same visual bug as I'm having.

There's a demo below to showcase the bug.

Steps to reproduce

To make it happen is enough to:

• Create a Light2D node, attach a texture.

• Duplicate the node or create a new Light2D node.

• Try changing the texture in any of those nodes.

• ~That should be enough to really mess things up.

• To visualize the error, use the shader shown in the videos above.

Minimal reproduction project (MRP)

Debug - Glitchy Lights v3.zip

[clayjohn]

After debugging further, I am not sure this is a bug.

Internally, the way the light shader works is that the light texture is sampled for every pixel of the shader, your light shader runs, then if the sample is outside of the light, then the alpha value is set to 0 (so it doesn't contribute to the image). However, that point is never reached because you are discarding before that happens.

At the same time, I think it is very bad that we are sampling every light texture and running the light shader for every pixel of an object. I suspect it will be a lot faster to avoid running the light shader when the pixel does not intersect with the light. If we avoid running the light shader for pixels outside of the light, then this issue will go away.

In summary, I think that the current bad behaviour is actually behaving as "intended". However, I think the current behaviour is bad from a performance point of view and should be changed.

Here is a diff that fixes this issue. I'm not 100% sure its the final solution. But I think it should be okay for now:

diff --git a/servers/rendering/renderer_rd/shaders/canvas.glsl b/servers/rendering/renderer_rd/shaders/canvas.glsl
index 235c772e2d..dbff09c301 100644
--- a/servers/rendering/renderer_rd/shaders/canvas.glsl
+++ b/servers/rendering/renderer_rd/shaders/canvas.glsl
@@ -665,6 +665,12 @@ void main() {
 
                vec2 tex_uv = (vec4(vertex, 0.0, 1.0) * mat4(light_array.data[light_base].texture_matrix[0], light_array.data[light_base].texture_matrix[1], vec4(0.0, 0.0, 1.0, 0.0), vec4(0.0, 0.0, 0.0, 1.0))).xy; //multiply inverse given its transposed. Optimizer removes useless operations.
                vec2 tex_uv_atlas = tex_uv * light_array.data[light_base].atlas_rect.zw + light_array.data[light_base].atlas_rect.xy;
+
+               if (any(lessThan(tex_uv, vec2(0.0, 0.0))) || any(greaterThanEqual(tex_uv, vec2(1.0, 1.0)))) {
+                       //if outside the light texture, light color is zero
+                       continue;
+               }
+
                vec4 light_color = textureLod(sampler2D(atlas_texture, texture_sampler), tex_uv_atlas, 0.0);
                vec4 light_base_color = light_array.data[light_base].color;
 
@@ -689,10 +695,6 @@ void main() {
                        light_color.rgb *= base_color.rgb;
                }
 #endif
-               if (any(lessThan(tex_uv, vec2(0.0, 0.0))) || any(greaterThanEqual(tex_uv, vec2(1.0, 1.0)))) {
-                       //if outside the light texture, light color is zero
-                       light_color.a = 0.0;
-               }
 
                if (bool(light_array.data[light_base].flags & LIGHT_FLAGS_HAS_SHADOW)) {
                        vec2 shadow_pos = (vec4(shadow_vertex, 0.0, 1.0) * mat4(light_array.data[light_base].shadow_matrix[0], light_array.data[light_base].shadow_matrix[1], vec4(0.0, 0.0, 1.0, 0.0), vec4(0.0, 0.0, 0.0, 1.0))).xy; //multiply inverse given its transposed. Optimizer removes useless operations.

Hmmm early testing shows this is an optimization that we want to do. I tested very quickly on my laptop (with an iGPU using a modified version of the MRP from #81152 and i nearly double performance with this diff

[caioraphael1]

I used this set of changes you made, but the weird bug still happens. I have 1 day of experience with the source code, so it's possible I did something wrong.

[caioraphael1]

I guess that the spaces where there shouldn't be light may actually have light information in these spaces, which is correctly detected by the ColorRect shader. Even tho this light is not visible without the ColoRect shader, this light information messes up with other lights, etc.
Visually this is what it seems to me, but I might be misunderstanding the problem

[clayjohn]

@caioraphael1 A few things to check:

1. What file did you make that change to? Identical code appears in the Compatibility renderer and in the RD renderer, so you could have mistakenly changes compatibility code and then tested RD or vice versa
2. How did you build/run the engine? Is it possible you accidentally ran an old version of the engine? You should double check the timestamp on the version you tested to ensure that it is the new version that included your change

[caioraphael1]

You were right, there was a bug that was stopping the building process mid-way.

With the changes, the shader shows that all the lighting is correct! But, as shown in the video, the lights are still behaving weirdly when zooming or panning the view, just like before the change. Also, the changes only work for Forward+, but I guess this is expected.

2024-04-19.19-20-43.mp4

[clayjohn]

With the changes, the shader shows that all the lighting is correct! But, as shown in the video, the lights are still behaving weirdly when zooming or panning the view, just like before the change.

How many lights do you have in that scene? Remember that each surface can only have up to 16 lights displayed at once. If you have more than 16 lights touching a surface, then the lights will flicker

Also, the changes only work for Forward+, but I guess this is expected.

Ya, you can duplicate the changes in the compatibility renderer though to get the same benefit.

[caioraphael1]

Remember that each surface can only have up to 16 lights displayed at once. If you have more than 16 lights touching a surface, then the lights will flicker.

I'm a bit confused, should I be cautious with the amount of lights within the viewport, or just the amount of lights touching the same Sprite2D? Also, can I increase the maximum to handle more than 16?

[caioraphael1]

Is there also a way I can profile the amount of lights within this condition?

[clayjohn]

I'm a bit confused, should I be cautious with the amount of lights within the viewport, or just the amount of lights touching the same Sprite2D?

Both, the limit per-viewport is 256 lights on desktop and 64 on mobile.

The limit of 16 is per draw call. So that means only 16 can be visible on the same sprite. For Tilemaps this can get tricky as the limit is per Tilemaps quadrant.

Also, can I increase the maximum to handle more than 16?

No

[caioraphael1]

Oooooooh ok, I see. I just changed the huge repeating texture I was using for a tilemap with a small quadrant size, and now all the lights are working correctly, finally!!!!!

I'm really glad! Thanks a lot!

I'd love to know how you knew where to go in the source code, I've been messing around with it for a while, debugging everywhere but I didn't even get close to this .glsl file you used for the fix.

But anyway, thanks again!

[djrain]

Also, can I increase the maximum to handle more than 16?

Just wanna add that we've also started to run into this, 16 is really not very many. Still hoping we can perhaps move to deferred rendering...

[clayjohn]

Also, can I increase the maximum to handle more than 16?

Just wanna add that we've also started to run into this, 16 is really not very many. Still hoping we can perhaps move to deferred rendering...

I've been thinking about that more lately. It would certainly have a higher base cost than what we currently do. But it might be a big win in a lot of cases, especially for things like tilemaps. I wish I had the time to make a proof of concept.

Its probably worth making a detailed proposal. I'll bet someone will be willing to take the time to explore the idea more and figure out if performance gains are possible