[3.2] Various light culling fixes #46694

JFonS · 2021-03-05T14:40:42Z

GLES3

This commit makes it possible to disable 3D directional lights by using the light's cull mask. It also automatically disables directionals when the object has baked lighting and the light is set to "bake all".

While these changes work fine, they have some downsides due to the current structure of the GLES3 renderer:

Object sorting is done only once for all directional lights, so there is no way to sort the enabled/disabled objects for each light.
This results in more GL state changes and can have some performance hit (only happens if directional lights affect some objects and not others).
Environment lighting is always done in the first pass, regardless of the corresponding directional light being enabled or not.
This means in some cases there will be a first pass with env light only (disabled light) and a second pass with an enabled light only, instead of merging the two passes in a single one.

GLES2

Added a check for the light cull mask, since it was previously ignored.

Fixes #46332. Fixes #19438.

clayjohn · 2021-03-05T16:59:39Z

Object sorting is done only once for all directional lights, so there is no way to sort the enabled/disabled objects for each light.
This results in more GL state changes and can have some performance hit (only happens if directional lights affect some objects and not others).

When you say "more GL state changes" you mean "unnecessary GL state changes right"? You aren't increasing the number of GL state changes by using the light mask, you just aren't necessarily decreasing state changes resulting in unnecessary changes, right?

JFonS · 2021-03-05T18:26:56Z

The concept of necessary and unnecessary is a bit fuzzy...

Before these changes all directional lights affected all objects, so there were no state changes triggered by using a cull mask.

If we merge this PR, certain lights can be culled for certain objects, so even in the same directional pass, the state can change.

At the time of writing the commit message I thought this would result in a bunch of GL state changes, since there is no way to correctly sort the objects for all lights at once. But after your comment I realized we just need to sort the objects respect to the first light in the list, since the rest of passes (the additive ones) just skip culled lights.

So I will take a look at adding this sorting and the state changes should be minimized.

We still don't solve the issue with the first pass always being in charge of the environment lighting, but in this case performance will stay the same as before, It's just a missed optimization opportunity.

clayjohn · 2021-03-05T18:55:04Z

I think I misunderstood you before. You are talking about the USE_LIGHT_DIRECTIONAL state changing between draw calls. Indeed, it would lead to wastefully changing state when the cull_mask is used for the first directional light in the scene. Once you add in the sorting for that, this should be good to go!

We still don't solve the issue with the first pass always being in charge of the environment lighting, but in this case performance will stay the same as before, It's just a missed optimization opportunity.

I agree. I don't think this can be easily solved with the current architecture in 3.2. I wouldn't worry about it

akien-mga · 2021-03-07T13:29:19Z

The regression test project + sanitizer found a heap-use-after-free, see logs in https://github.com/godotengine/godot/pull/46694/checks?check_run_id=2047848252

Here's what the CI runs if you want to try it locally:

godot/.github/workflows/linux_builds.yml

Lines 189 to 210 in 3d5f964

    
                 - name: Compilation 
        
                   env: 
        
                     SCONS_CACHE: ${{github.workspace}}/.scons_cache/ 
        
                   run: | 
        
                     scons tools=yes target=debug use_asan=yes use_ubsan=yes 
        
                     ls -l bin/ 
        
                 # Download and test project to check leaks and invalid memory usage. 
        
                 # CI has no audio device, so use the Dummy audio driver to avoid spurious error messages. 
        
                 - name: Importing and running project project 
        
                   run: | 
        
                     wget2 https://github.com/qarmin/RegressionTestProject/archive/3.2.zip 
        
                     unzip 3.2.zip 
        
                     mv "RegressionTestProject-3.2" "test_project" 
        
                     echo "----- Open editor to check for memory leaks -----" 
        
                     DRI_PRIME=0 xvfb-run bin/godot.x11.tools.64s --audio-driver Dummy -e -q --path test_project 2>&1 | tee sanitizers_log.txt || true 
        
                     misc/scripts/check_ci_log.py sanitizers_log.txt 
        
                     echo "----- Run and test project -----" 
        
                     DRI_PRIME=0 xvfb-run bin/godot.x11.tools.64s 30 --video-driver GLES3 --audio-driver Dummy --path test_project 2>&1 | tee sanitizers_log.txt || true 
        
                     misc/scripts/check_ci_log.py sanitizers_log.txt

JFonS · 2021-03-07T14:25:54Z

@akien-mga I noticed... I will take a look on monday.

clayjohn

Approved subject to Akien's comment about the heap use after free.

akien-mga · 2021-03-08T13:47:04Z

drivers/gles3/rasterizer_scene_gles3.cpp

-		e->sort_key |= SORT_KEY_NO_DIRECTIONAL_FLAG;
+	// We sort only by the first directional light. The rest of directional lights will be drawn in additive passes that are skipped if disabled.
+	if (first_directional_light.is_valid()) {
+		RasterizerStorageGLES3::Light *directional = light_instance_owner.getptr(first_directional_light)->light_ptr;


I guess the heap-use-after-free is here, the saved instance must be pointing at a now freed Light.

I'm not familiar with this code but isn't it possible to use directional_lights[0] (after checking the size I guess) instead of saving the first light RID in first_directional_light?

That's what I was doing before, and saving the RID was an attempt to fix the issue :)

Not sure what's going on... I thought the RID and RID_Owner system would take care of invalidating freed objects, but it doesn't seem to work.

I pushed a last attempt at fixing this, but I don't think it will succeed.

The only other way I can think for fixing this is checking here:

godot/drivers/gles3/rasterizer_scene_gles3.cpp

Lines 4948 to 4953 in 118567c

bool RasterizerSceneGLES3::free(RID p_rid) {

if (light_instance_owner.owns(p_rid)) {

LightInstance *light_instance = light_instance_owner.getptr(p_rid);

If p_rid == first_directional_light and invalidating the stored RID in such case, but it's quite hacky...

Seems like it worked. I will confirm with reduz whether RIDs are meant to be used like this, because I only found one occurrence of a similar check in the same file.

GLES3 changes: This commit makes it possible to disable 3D directional lights by using the light's cull mask. It also automatically disables directionals when the object has baked lighting and the light is set to "bake all". GLES2 changes: Added a check for the light cull mask, since it was previously ignored.

JFonS · 2021-03-08T15:15:19Z

@akien-mga The use-after-free issue is fixed, so this should be good to merge :)

akien-mga · 2021-03-08T15:17:46Z

Thanks!

tavurth · 2022-02-09T08:45:10Z

I'm assuming this PR is not supposed to disable the shadows from affected directional lights?

If it is supposed to then there is some bug in v3.4.3.rc1.official

Calinou · 2022-02-09T16:05:01Z

@tavurth Please continue the discussion in #43827.

tavurth · 2022-02-09T16:07:10Z

@Calinou It's more that I'm confused if this PR should have fixed the issue with shadows or not. 🤔

I attached a sample project to this issue #43827 which seems relevant, but I'm wondering what this PR did

JFonS added bug topic:rendering labels Mar 5, 2021

JFonS added this to the 3.2 milestone Mar 5, 2021

JFonS requested a review from a team March 5, 2021 14:40

JFonS force-pushed the fix_directional_disabling branch from 9bf31db to 5b23482 Compare March 6, 2021 21:19

clayjohn approved these changes Mar 7, 2021

View reviewed changes

JFonS force-pushed the fix_directional_disabling branch 2 times, most recently from 64a07d4 to bfb90ec Compare March 8, 2021 12:49

akien-mga reviewed Mar 8, 2021

View reviewed changes

JFonS force-pushed the fix_directional_disabling branch from bfb90ec to f24f582 Compare March 8, 2021 14:11

akien-mga merged commit 0e0d73c into godotengine:3.2 Mar 8, 2021

This was referenced Mar 8, 2021

Real-time lights on lightmapped surfaces with bake mode "All" are only disabled in GLES2, not GLES3 #46332

Closed

Cull mask in DirectionalLight appears to have no effect #19438

Closed

qarmin mentioned this pull request Mar 13, 2021

Usage of freed memory only in release build #46960

Open

akien-mga modified the milestones: 3.2, 3.3 Apr 20, 2021

JFonS deleted the fix_directional_disabling branch May 4, 2021 07:41

lawnjelly mentioned this pull request May 18, 2021

Portal occlusion culling [3.4] #46130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.2] Various light culling fixes #46694

[3.2] Various light culling fixes #46694

JFonS commented Mar 5, 2021

clayjohn commented Mar 5, 2021

JFonS commented Mar 5, 2021

clayjohn commented Mar 5, 2021

akien-mga commented Mar 7, 2021

JFonS commented Mar 7, 2021

clayjohn left a comment

akien-mga Mar 8, 2021

JFonS Mar 8, 2021

JFonS Mar 8, 2021 •

edited

Loading

JFonS Mar 8, 2021

JFonS Mar 8, 2021

JFonS commented Mar 8, 2021

akien-mga commented Mar 8, 2021

tavurth commented Feb 9, 2022

Calinou commented Feb 9, 2022 •

edited

Loading

tavurth commented Feb 9, 2022

	bool RasterizerSceneGLES3::free(RID p_rid) {

	if (light_instance_owner.owns(p_rid)) {

	LightInstance *light_instance = light_instance_owner.getptr(p_rid);

[3.2] Various light culling fixes #46694

[3.2] Various light culling fixes #46694

Conversation

JFonS commented Mar 5, 2021

clayjohn commented Mar 5, 2021

JFonS commented Mar 5, 2021

clayjohn commented Mar 5, 2021

akien-mga commented Mar 7, 2021

JFonS commented Mar 7, 2021

clayjohn left a comment

Choose a reason for hiding this comment

akien-mga Mar 8, 2021

Choose a reason for hiding this comment

JFonS Mar 8, 2021

Choose a reason for hiding this comment

JFonS Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

JFonS Mar 8, 2021

Choose a reason for hiding this comment

JFonS Mar 8, 2021

Choose a reason for hiding this comment

JFonS commented Mar 8, 2021

akien-mga commented Mar 8, 2021

tavurth commented Feb 9, 2022

Calinou commented Feb 9, 2022 • edited Loading

tavurth commented Feb 9, 2022

JFonS Mar 8, 2021 •

edited

Loading

Calinou commented Feb 9, 2022 •

edited

Loading