Lights with energy set to 0 are not culled from rendering (GLES3 + GLES2)

[Calinou]

3.x version of https://github.com/godotengine/godot/issues/56633. I'm opening a separate issue as this needs to be implemented separately from master.

Godot version

3.4.2.stable, 3.x (0b54b3d05)

System information

Fedora 34, GeForce GTX 1080 (NVIDIA 495.46)

Issue description

Lights with energy set to 0 are not culled from rendering. This means performance won't improve when you set a light's energy to 0. Instead, you need to set the light's visible property to false. This can lead to unexpectedly low performance when animating lights in and out using an AnimationPlayer.

This occurs both with the GLES3 and GLES2 renderers.

Lights energy = 1

Lights energy = 0

Look at the FPS counter in the top-left corner and compare with the above screenshot:

Steps to reproduce

• Add any kind of 3D light node.
• Set its Energy property to 0.
• Notice performance will be the same as if Energy was set to 1.

Minimal reproduction project

test_light_hide_3.x.zip

The MRP has 1 DirectionalLight with shadows enabled, 6 red OmniLights with shadows enabled and 6 green SpotLights with shadows enabled.

• D: Toggle DirectionalLight's visible property.
• O: Toggle OmniLights' visible property.
• S: Toggle SpotLights' visible property.
• E: Toggle all lights' energy property between 1.0 and 0.0.

[lawnjelly]

Can an option be to simply note this in the documentation?

I say this because there are a number of changes that occur when a node is set to invisible, and these might be difficult to simulate (/ produce bug prone spaghetti code) without changing the visible flag when light energy is set to 0.0. However there is a danger that changing this flag automatically could be confusing from a user perspective.

Also consider that a user might want to do something like have a strobe light, or lightning, and the change of visibility state may not be cost free.

[Listwon]

I forgot about this PR https://github.com/godotengine/godot/pull/52251. It covers the basic fix, but it's not compatible with masked lights and wasn't checked with custom shaders, so I changed PR to draft.

[lawnjelly]

I forgot about this PR #52251. It covers the basic fix, but it's not compatible with masked lights and wasn't checked with custom shaders, so I changed PR to draft.

Situation with 2D lights is quite different, however the point carries over to 3D - if we can do small changes in the renderer at low level we can probably considerably reduce the cost of 3D lights when set to energy zero (like e.g not rendering shadow maps and the light on surfaces etc). That is worth doing I expect, if a higher level approach isn't pursued (and if it is done at higher level, there is no need).

Removing them from the BVH and pairing though is the obvious high level approach (i.e. culling stage) to get them to have zero cost, and I'm unsure if this would be desirable in some situations (e.g. the flashing lights, dynamic lights etc), because there will be a cost to this switching on / off, in terms of the tree and pairing / unpairing. This high level change could be done at the level of the BVH (with some modifications) or perhaps better even higher at the scene level, the Light node could e.g. manage the visibility of the RID manually (rather than directly from the visible flag), or have two visible flags on a node, and only send the visible to the VisualServer if both the user and app level flag are set.

I suspect doing it at high level is one of those things that could be an optimization in some situations, but a de-optimization in others. Overall I'd be slightly more in favour of putting in some low level checks (like @Listwon 's) in the renderer (to get most of the benefit), then put in the docs that the way to totally deactivate the Lights is to make them invisible rather than set energy to zero. But it's arguable either way.

Another alternative is to do it at high level (e.g. in the Light Node), but add a note to the docs that flipping from 0.0 energy to non zero has a cost, and therefore for flashing lights, users should put a lower cap of e.g. 0.001 energy to prevent the switching taking place.

[Calinou]

Another alternative is to do it at high level (e.g. in the Light Node), but add a note to the docs that flipping from 0.0 energy to non zero has a cost, and therefore for flashing lights, users should put a lower cap of e.g. 0.001 energy to prevent the switching taking place.

I think this is preferable from an user perspective, especially since toggling a light's visibility will hide its children (whereas setting its energy to 0 won't). Sometimes, you don't want to hide a light's child nodes.

It might be worth attempting this for 3.6 beta and see if there are any performance regressions caused by this change.