Skip to content

Conversation

@unity-cchu
Copy link
Contributor

@unity-cchu unity-cchu commented Mar 11, 2021

Checklist for PR maker

  • Have you updated the changelog? Each package has a CHANGELOG.md file.

Purpose of this PR

Refactor of shadow code.
Now renders shadows to a single channel.
Prerendering of shadows can render to different channels in addition to different textures.
Added versioning
Added per light shadow culling
Made shader naming more consistent

Copy link
Contributor

@rolandk-unity rolandk-unity left a comment

Choose a reason for hiding this comment

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

Looks good overall

float lightRadiusSq = light.boundingSphere.radius * light.boundingSphere.radius;
float projectedRadiusSq = m_ProjectedBoundingSphere.radius * m_ProjectedBoundingSphere.radius;

return distanceSq <= (lightRadiusSq + projectedRadiusSq);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? I guess a^2 <= b^2 + c^2 is not the same as a <= b+c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch


private static Material GetProjectedShadowMaterial(this Renderer2DData rendererData, int colorMask)
{
//if (rendererData.projectedShadowMaterial == null || rendererData.projectedShadowShader != rendererData.projectedShadowMaterial.shader)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double checking, do we want to create a new one every time the method is called? If so, maybe renaming to CreateProjectedShadowMaterial could make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

private static Material GetStencilOnlyShadowMaterial(this Renderer2DData rendererData, int colorMask)
{
//if (rendererData.stencilOnlyShadowMaterial == null || rendererData.projectedShadowShader != rendererData.stencilOnlyShadowMaterial.shader)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

See GetProjectedShadowMaterial comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

private static Material GetSpriteSelfShadowMaterial(this Renderer2DData rendererData, int colorMask)
{
//if (rendererData.spriteSelfShadowMaterial == null || rendererData.spriteShadowShader != rendererData.spriteSelfShadowMaterial.shader)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

See GetProjectedShadowMaterial comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

private static Material GetSpriteUnshadowMaterial(this Renderer2DData rendererData, int colorMask)
{
//if (rendererData.spriteUnshadowMaterial == null || rendererData.spriteUnshadowShader != rendererData.spriteUnshadowMaterial.shader)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

See GetProjectedShadowMaterial comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@unity-cchu unity-cchu marked this pull request as ready for review April 22, 2021 12:35
@unity-cchu unity-cchu requested a review from a team as a code owner April 22, 2021 12:35
@unity-cchu unity-cchu added the 2d label Apr 22, 2021
@unity-cchu unity-cchu requested review from a team as code owners April 23, 2021 05:29
@unity-cchu unity-cchu merged commit 6365f24 into master Apr 28, 2021
@unity-cchu unity-cchu deleted the 2d/shadow-refactor-jan branch April 28, 2021 00:39
@tatoforever
Copy link

Folks, why static meshes with baked shadows must also render to shadowmaps in order to receive shadows? Isn't this a bit pointless? What about "let any mesh receive shadows", no matter if they cast shadows or not? Could improve performance as you don't need to render all geometry reciving shadows. I have a custom renderer that sit on top of built-in that ouperforms URP by a large marge (2x - 3x easily) when there's many lights casting shadows cause none of my static geometry renders to depthmaps.

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.

5 participants