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

Add shadow_caster_mask to Light3D. #85338

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

EMBYRDEV
Copy link
Contributor

@EMBYRDEV EMBYRDEV commented Nov 25, 2023

Implements godotengine/godot-proposals#3606 & godotengine/godot-proposals#8509

Tested on Forward+, Mobile & Compatibility renderers.

I feel this is a very useful change for optimisation when using dynamic shadows in projects and is required to provide a cheaper way of preventing lights bleeding into other rooms, even when they dont need detailed shadows otherwise.

This is more fine grained than just disabling shadows on the MeshInstances as you may want the ability for some lights to cast shadows for that object but not others.

See below, the SpotLight3D is set to only cast shadows for Layer 2. It defaults to existing behaviour (all layers that the light affects).
image

@EMBYRDEV
Copy link
Contributor Author

One point of discussion on this might be do we want this mask to be ANDed with the existing light cull mask (what we do in this PR) or if it should be entirely standalone to allow people setting certian meshes to not be lit by this light but will still cast shadows (I don't see many use cases for this).

@EMBYRDEV EMBYRDEV changed the title Add shadow_caster_mask to Light3D. Add shadow_caster_mask to Light3D. Nov 26, 2023
@EMBYRDEV
Copy link
Contributor Author

There seems to be a reasonable demand for this feature, any word on the reviews? Would love to make it into 4.3.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected.

Testing project: test_shadow_caster_mask.zip

Example with the green object not casting shadows for the directional light only, and the purple OmniLight having a projector but not casting shadows for any objects:

Forward+

Screenshot_20240113_000621

Mobile

Screenshot_20240113_000642

Compatibility

Light projectors aren't supported when using Compatibility.

Screenshot_20240113_000659


One point of discussion on this might be do we want this mask to be ANDed with the existing light cull mask (what we do in this PR) or if it should be entirely standalone to allow people setting certian meshes to not be lit by this light but will still cast shadows (I don't see many use cases for this).

I think the current behavior in this PR is fine; I can't think of any use cases for having an object not be lit by a light but still cast a shadow. We can revisit this later if there's enough demand.

@@ -376,6 +376,8 @@ class RenderingServerDefault : public RenderingServer {
FUNC2(light_set_cull_mask, RID, uint32_t)
FUNC5(light_set_distance_fade, RID, bool, float, float, float)
FUNC2(light_set_reverse_cull_face_mode, RID, bool)
FUNC2(light_set_shadow_caster_mask, RID, uint32_t)
FUNC1RC(uint32_t, light_get_shadow_caster_mask, RID)
Copy link
Member

@Calinou Calinou Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be exposed? Other light storage methods don't have a getter exposed, presumably to avoid pipeline stalls when the method is called.

cc @clayjohn

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @Calinou says for getters it is usually preferable to cache scene side and return that rather than query the servers, which can cause a sync and stall the pipeline.

@@ -354,6 +363,9 @@ void Light3D::_bind_methods() {
ClassDB::bind_method(D_METHOD("set_shadow_reverse_cull_face", "enable"), &Light3D::set_shadow_reverse_cull_face);
ClassDB::bind_method(D_METHOD("get_shadow_reverse_cull_face"), &Light3D::get_shadow_reverse_cull_face);

ClassDB::bind_method(D_METHOD("set_shadow_caster_mask", "enable"), &Light3D::set_shadow_caster_mask);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ClassDB::bind_method(D_METHOD("set_shadow_caster_mask", "enable"), &Light3D::set_shadow_caster_mask);
ClassDB::bind_method(D_METHOD("set_shadow_caster_mask", "caster_mask"), &Light3D::set_shadow_caster_mask);

@EMBYRDEV
Copy link
Contributor Author

EMBYRDEV commented Mar 4, 2024

NOTE: This isn't abandoned, I've just been sidetracked with other work in the meantime. I will revisit and incorporate feedback soon.

@Mimisor7
Copy link

Mimisor7 commented Apr 7, 2024

NOTE: This isn't abandoned, I've just been sidetracked with other work in the meantime. I will revisit and incorporate feedback soon.

Glad to hear that someone is working on this feature! I was just having problems with unwanted shadows cast from billboarded and shaded Sprite3Ds (that also appear to be self-shadowing...) in my 3D project, and I believe that this feature would likely resolve it.
I wish to see this feature implemented!

@sebialex
Copy link

Any chance we could get something like this for 2D lights?

@EvanNoelGames
Copy link

Very important feature I've been hoping for, really hope this makes it into 4.3!

@Calinou
Copy link
Member

Calinou commented Jun 13, 2024

Any chance we could get something like this for 2D lights?

Maybe, but this should be requested in a proposal first as it's entirely separate functionality.

Very important feature I've been hoping for, really hope this makes it into 4.3!

4.3 is in feature freeze, so any new features may only be merged in 4.4 at the earliest.

If you need this feature right now, you can checkout the PR's branch and compile the editor and any export templates you might need for your project and specify them as custom export templates in the Export dialog.

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Jul 24, 2024
@EMBYRDEV EMBYRDEV requested review from a team as code owners August 11, 2024 21:53
@EMBYRDEV
Copy link
Contributor Author

EMBYRDEV commented Oct 9, 2024

Any chance this will make it in for 4.4? Seems like everything has been addressed.

@clayjohn
Copy link
Member

clayjohn commented Oct 9, 2024

@EMBYRDEV I fully intend to merge this for 4.4! Sorry for my delay in reviewing

@huwpascoe
Copy link
Contributor

Confirmed working including rebased.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with the rendering team at today's rendering meeting and confirmed that the implementation is correct!

Sorry for the delay in reviewing

@Repiteo Repiteo merged commit cfc05c5 into godotengine:master Oct 24, 2024
18 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 24, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

Add per-light shadow cull masks to control which objects cast shadows
10 participants