Skip to content

Conversation

hybridherbst
Copy link
Contributor

The current custom lighting implementation didn't support additional light shadows. They are turned off on the UniversalRenderPipelineAsset, but since the existing ShaderGraph implementation did contain the relevant keywords I assume they are supposed to work.

This PR adds support for additional light shadows by calling the right GetAdditionalLight overload that takes shadowCoords. There's probably an edge case missing for lightmapped objects that are supposed to also receive mixed lighting with shadows from additional lights, but I think for now it's nice to have shadows when wanted :)

Before / After:
20210608-014912_Unity
20210608-014803_Unity

@CLAassistant
Copy link

CLAassistant commented Jun 7, 2021

CLA assistant check
All committers have signed the CLA.

@ciro-unity ciro-unity self-assigned this Jun 14, 2021
@ciro-unity ciro-unity added art Art or music assets, or tweaks enhancement New feature or request and removed art Art or music assets, or tweaks labels Jun 14, 2021
@ciro-unity
Copy link
Contributor

Not sure if needed. We generally don't have spotlights, I don't see an use for them in this game, and the point lights (torches, the phoenix chick, etc.) don't cast a shadow. As you know, point light shadows only came in more recent URP versions.

@hybridherbst
Copy link
Contributor Author

hybridherbst commented Jun 14, 2021

I saw that the ShaderGraph shaders defined the ADDITIONAL_LIGHT_SHADOWS keyword so it seems it was used at some point (or someone tried - as it was probably never working so far).

Not sure either if it's needed here, but it doesn't hurt either (if additional light shadows are off the code isn't used at all, conditional compilation for the win) – people are gonna look at Open Project as a reference (many people wait for a proper toon shader for URP) so I think a bit more complete feature support is nice to have.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants