Skip to content

Conversation

Wilfrid-Unity
Copy link
Contributor

@Wilfrid-Unity Wilfrid-Unity commented Oct 8, 2020

Purpose of this PR

This pull request allows Universal RP additional (i.e punctual) lights to specify custom shadow resolutions.

In current URP version, all additional (i.e punctual) shadowed lights use shadow maps of the same resolution (scaled to fit in the common shadow atlas).
Custom shadow resolutions make it possible to give more importance to some lights in the scene, ensuring their shadows will look as good as possible.

image

U.I and assets

The interface provided by this pull request to control shadow resolution ; is similar to the one used in HDRP.

Different shadow resolution tiers can be defined in the Universal RP asset file.

image

Each light in the scene can specify one of the different resolution tiers, or a custom resolution.

image

Assumptions and specs

  • The default resolution tier values are defined here (currently 256/512/1024).
  • The default custom resolution (for lights not using the pre-set tiers) is defined here (currently 128).
  • All shadow map resolutions are rounded to power-of-two values.
  • If the shadow atlas (common to all additional lights) is too small to contain all the shadow maps needed in a frame, they will be scaled down using a common ratio, until they all fit (logic implemented here ("AtlasLayout" in AdditionalLightsShadowCasterPass.cs) ).

Testing status

This pull request add test "146-AdditionalLightShadowCustomResolution" to the UniversalGraphicsTests suite, whose purpose is to check different shadow resolutions can be used.

image

Comments to reviewers

I started this work on top of branch universal/point-light-shadows.
Point Light Shadows branch has not yet been merged to trunk, pull request #1616 is still waiting for some approvals now.

To reduce the amount of changes to review, I created this draft pull request that currently only contains the commits related to custom shadow resolutions. The plan is to target it to trunk later (after #1616 has been merged).
I would appreciate reviews around the changes in UniversalRenderPipelineAsset.cs and the code that updates UniversalAdditionalLightData.

…nt lights

Like with spot lights, resolution for each shadow map is computed every frame in order to fit all of them in the common shadow atlas.

Some of the current limitations:
- ShadowBias possibly needs more precise values
- FovBias possibly needs more precise values
- Unused shadow faces allocate a slot in the common shadow atlas
- Draw calls are generated even for shadow frustums that do not contain shadow casters
- Not tested: Standalone players
- Not tested: Dynamic scenes
- Not tested: Performances
- Not tested: Code path using Structured Buffers to pass shadow information to GPU (during shadow pass and lighting loop)
- Not tested: Deferred Renderer
Normal bias for point lights now behaves similarly as for spot lights
…- This makes point light shadows look closer to spot light shadows
…dow normal bias - This fixes the issue of shadows that changed when point light was rotated.

Built-In RP uses a similar approach to apply punctual light shadow normal bias. See UnityWorldSpaceLightDir called by UnityClipSpaceShadowCasterPos in UnityCG.cginc:
https://github.cds.internal.unity3d.com/unity/unity/commit/93015b67a6e1afdfe00867fdd3a336eaa835f54d#diff-c2818feb2f37e566cf36ca5cab57a99eR533
https://github.cds.internal.unity3d.com/unity/unity/blob/93015b67a6e1afdfe00867fdd3a336eaa835f54d/External/shaderlab/CGIncludes/UnityCG.cginc#L76
…s, to avoid missing shadows at cube face boundaries
…adow data to the lighting shaders + Fixed indexing issue when using non-shadow-casting additional light

Some known issues still happening:
- If only a single light in the scene is set to use soft shadows, all punctual light shadows will be soft (even if their setting is "Hard")
- Toggling setting "Soft Shadows" in UniversalRP asset also impacts shape of shadows from punctual light set to cast "Hard Shadows"
…ot light shadow's Normal Bias to match reference image - Since revision ed8fcbe Normal Bias is applied using actual spot light direction
… use per-vertex light direction when applying shadow normal bias)
…t to ScriptableRenderContext.DrawShadows - Reduces the number of shadow casters rendered to the shadow map
…nd 143_SSAO_DepthNormals_Orthographic directional light shadows Normal Bias to match reference images - Since revision ed8fcbe formula to apply Normal Bias changed slightly
…dditional light shadows

Test fails if "hard shadows" setting is replaced by "soft shadows" setting
Test fails if "soft shadows" setting is replaced by "hard shadows" setting
Test fails if "no shadows" setting is replaced by "shadows" setting
Test fails if one of the punctual lights in the scene is not rendered
…nstead of _ShadowCastingLightParameters from revision 6341928)

This avoids deprecation of public variables, and makes code easier to understand
Addresses comments:
#1616 (comment)
#1616 (comment)
#1616 (comment)
@Wilfrid-Unity
Copy link
Contributor Author

This to me seems like we should just provide a drop down with preset values, so an enum rather than a int field that automatically gets rounded.

I would suggest changing the custom shadow resolution to a dropdown(eg, 128,256,512,1024,2048, 4096) since we are rounding it to nearest anyway, also we have a max additonal shadow atlas value which means there is no point letting users put in anything higher than 4k.

I also agree with Andre. But that would diverge us from HDRP.

Thanks @Verasl and @martint-unity for this suggestion, that makes a lot of sense when considering Universal RP restrictions.

In this branch, I re-used the U.I implemented in HDRP in order to provide a unified experience between both Render Pipelines.

When opening this pull request, I consulted @TheoWong-pixel who had commented the section about shadow resolution UI in the Tech Design Doc.
My understanding is that HDRP Punctual Lights Shadow Resolution Tiers U.I is currently not ideal: there is a plan to clean it up for Unity Next release ; and this will be the opportunity to harmonize it with Universal RP Additional Lights Shadow Resolution Tiers U.I.
Therefore Theo suggested going on with the current interface for now, but making sure that the behavior about rounding is explicited in the tooltips (which I did implement here and here).

Does this approach look OK to you?

…ional Lights too

Fixes "shadows popping" behavior illustrated by https://github.com/Unity-Technologies/BoatAttack/tree/testing/urp-pointlight-shadows, when changing PipelineAsset's "Shadows Max Distance" setting
…adow-resolution

# Conflicts:
#	com.unity.render-pipelines.universal/Runtime/Passes/AdditionalLightsShadowCasterPass.cs
Solves issue described in https://docs.google.com/document/d/1tkQdYKx2bED5hp3dTmUlkeZS_moiUsRZeMS4hMXHfXk/edit#bookmark=id.dqn8fqx5f45
(i.e: repro by editing TestProjects/UniversalGraphicsTest/Assets/Scenes/010_AdditionalLightsSorted/SortedLightsTestPrefab.prefab to make the meshes cast shadows, and running UniversalGraphicsTest#010_AdditionalLightsSorted)
Solves issue happening when after procedure described in https://docs.google.com/document/d/1tkQdYKx2bED5hp3dTmUlkeZS_moiUsRZeMS4hMXHfXk/edit#bookmark=id.dqn8fqx5f45
(i.e: repro by editing TestProjects/UniversalGraphicsTest/Assets/Scenes/010_AdditionalLightsSorted/SortedLightsTestPrefab.prefab to make the meshes cast shadows, running UniversalGraphicsTest#010_AdditionalLightsSorted, and switching to Scene view)
Base automatically changed from universal/point-light-shadows to master November 30, 2020 10:46
# Conflicts:
#	TestProjects/UniversalGraphicsTest/Assets/Scenes/105_TransparentReceiveShadows.unity
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs
#	com.unity.render-pipelines.universal/Runtime/Passes/AdditionalLightsShadowCasterPass.cs
@Wilfrid-Unity Wilfrid-Unity marked this pull request as ready for review December 7, 2020 02:08
@Wilfrid-Unity Wilfrid-Unity requested a review from a team as a code owner December 7, 2020 02:08
Copy link
Contributor

@martint-unity martint-unity left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Verasl Verasl left a comment

Choose a reason for hiding this comment

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

LGTM
We need to make sure we track the needed UX work in the future

@lukaschod
Copy link
Contributor

Found small UI issue, when changing between no shadow and hard shadow - during transsition effect resolution property has wrong spacing.
ui-issue

Copy link
Contributor

@lukaschod lukaschod left a comment

Choose a reason for hiding this comment

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

Found small issues (Addded as comments), but nothing serious to stop from merging.

Copy link

Choose a reason for hiding this comment

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

Great work! Approved.

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