Skip to content
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

Visual layers are ignored for DirectionalLight #74545

Closed
m4nu3lf opened this issue Mar 7, 2023 · 2 comments · Fixed by #95379
Closed

Visual layers are ignored for DirectionalLight #74545

m4nu3lf opened this issue Mar 7, 2023 · 2 comments · Fixed by #95379

Comments

@m4nu3lf
Copy link
Contributor

m4nu3lf commented Mar 7, 2023

Godot version

4.0 stable

System information

ArchLinux

Issue description

Changing visual layers on directional lights has no effect; the directional light is always rendered. This differs from OmniLights and SpotLights, which are only rendered if they have overlapping layers as the Camera's cull layers.

Steps to reproduce

Ensure the renderer is Forward+. Add a directional light to the scene, a mesh and a camera. Set the visual layers of the DirectialLight and the cull mask on the camera to have no common flags. The DirectionalLight is still rendered.

Minimal reproduction project

directional_light_layers.zip

@majikayogames
Copy link
Contributor

I needed this for hiding directional lights on other side of portals and for my purposes made this one line fix here in RenderingServer's RendererSceneCull::_render_scene:

majikayogames@1a75fbd

https://www.youtube.com/watch?v=trNFxV6KRMs

Although I'm not sure if this is the right solution. Right above that function we have _scene_cull where it does a whole list of checks which are also not accounted for for DirectionalLight3D's:

#define HIDDEN_BY_VISIBILITY_CHECKS (visibility_flags == InstanceData::FLAG_VISIBILITY_DEPENDENCY_HIDDEN_CLOSE_RANGE || visibility_flags == InstanceData::FLAG_VISIBILITY_DEPENDENCY_HIDDEN)
#define LAYER_CHECK (cull_data.visible_layers & idata.layer_mask)
#define IN_FRUSTUM(f) (cull_data.scenario->instance_aabbs[i].in_frustum(f))
#define VIS_RANGE_CHECK ((idata.visibility_index == -1) || _visibility_range_check<false>(cull_data.scenario->instance_visibility[idata.visibility_index], cull_data.cam_transform.origin, cull_data.visibility_viewport_mask) == 0)
#define VIS_PARENT_CHECK (_visibility_parent_check(cull_data, idata))
#define VIS_CHECK (visibility_check < 0 ? (visibility_check = (visibility_flags != InstanceData::FLAG_VISIBILITY_DEPENDENCY_NEEDS_CHECK || (VIS_RANGE_CHECK && VIS_PARENT_CHECK))) : visibility_check)
#define OCCLUSION_CULLED (cull_data.occlusion_buffer != nullptr && (cull_data.scenario->instance_data[i].flags & InstanceData::FLAG_IGNORE_OCCLUSION_CULLING) == 0 && cull_data.occlusion_buffer->is_occluded(cull_data.scenario->instance_aabbs[i].bounds, cull_data.cam_transform.origin, inv_cam_transform, *cull_data.camera_matrix, z_near))

as they are not handled in this function but rather right afterwards and added to the cull with the result from the above for loop:

scene_cull_result.light_instances.push_back(directional_lights[i]);

It looks like they are not considered in the _scene_cull function. It looks like the relevant checks would be the layer mask one and the visibility parent one, which it also doesn't work for. If we want to keep this separate we could do add a visibility parent check and that layer_mask check I added above in the for loop for directional lights. For that we would need to rearrange _render_scene a bit to have the cull_data available to call _visibility_parent_check(cull_data, E) as well.

I can make a PR if this is sufficient or I can edit it a bit if needed.

@SlashScreen
Copy link
Contributor

Ran into this issue as well. Any reason this shouldn't become a PR? (In the meantime, I can compile a version with the patch for myself.) Do directional lights need to be culled? They are, by design, pretty omnipresent, and I can't think of a situation off the top of my head where you would have a directional light and not know for certain that it can be culled until the shadowmap has already been rendered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants