-
Notifications
You must be signed in to change notification settings - Fork 855
[2022.1] Add Shadow and Additional light variant stripping #5544
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
[2022.1] Add Shadow and Additional light variant stripping #5544
Conversation
Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed. URP Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure. |
…d as it is not used there
com.unity.render-pipelines.universal/Shaders/Particles/ParticlesSimpleLit.shader
Show resolved
Hide resolved
…h when regular one fails
…universal/strip-off-shadows-addlights
com.unity.render-pipelines.universal/Runtime/Passes/MainLightShadowCasterPass.cs
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.universal/Editor/ShaderPreprocessor.cs
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.universal/Runtime/Passes/AdditionalLightsShadowCasterPass.cs
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.universal/Runtime/Passes/AdditionalLightsShadowCasterPass.cs
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.universal/Runtime/UniversalRenderer.cs
Outdated
Show resolved
Hide resolved
com.unity.testing.urp/Scripts/Runtime/CustomRenderPipeline/CustomRenderer.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments that I want you to take a look at before approving.
…ipAdditionalLightOffVariants
…universal/strip-off-shadows-addlights # Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with some comments.
com.unity.render-pipelines.universal/Runtime/Passes/AdditionalLightsShadowCasterPass.cs
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.universal/Runtime/Passes/AdditionalLightsShadowCasterPass.cs
Show resolved
Hide resolved
internal void SetupForEmptyRendering() | ||
{ | ||
m_MainLightShadowmapTexture = ShadowUtils.GetTemporaryShadowTexture(1, 1, k_ShadowmapBufferBits); | ||
m_CreateEmptyShadowmap = true; | ||
useNativeRenderPass = false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: When reviewing it felt confusing to you have two initialization steps in setup and execute and the initialization fields differ between main and additional light shadow pass. So it seems arbitrary at first.
F.ex in the additional shadow pass you create and initialize the texture in the setup, in the main pass you create the texture in setup but initialize it in execute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need to have the separate setup initalization, this adds more code to tests and renderer setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
com.unity.render-pipelines.universal/Shaders/Particles/ParticlesSimpleLit.shader
Show resolved
Hide resolved
|
||
SetupShaderLightConstants(cmd, ref renderingData); | ||
|
||
bool lightCountCheck = (m_AdditionalLightsAlwaysEnabled && renderingData.lightData.maxPerObjectAdditionalLightsCount > 0) || additionalLightsCount > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can additionalLightsCount
be changed at runtime and you get into a situation that the keyword and stripped and at runtime you try to enable it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add additional check to prevent this
…universal/strip-off-shadows-addlights # Conflicts: # com.unity.render-pipelines.universal/CHANGELOG.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Tested URP template and Viking Village. Visual correctness looks good, no performance regression. Played around with different settings, didn't find any issues.
URP_Foundation on Android_OpenGLES3_Standalone_il2cpp_Linear on version trunk URP_PostPro on Android_Vulkan_Standalone_il2cpp_Linear on version trunk URP_PostPro on OSX_Metal_playmode_mono_Linear on version trunk URP_PostPro on Win_DX11_playmode_XR_mono_Linear on version trunk URP_Terrain on Android_OpenGLES3_Standalone_il2cpp_Linear on version trunk Universal_Stereo on Win__Standalone_mono_Linear on version trunk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Purpose of this PR
This is follow up of #5060.
Stripping disabled variants if feature is enabled:
Testing status
Empty project with most features enabled lit shader:
Before: 93
After 48
Foundation:
Before: 44k (10k lit)
After: 24k (5k lit)
Comments to reviewers
Notes for the reviewers you have assigned.