[Universal] Make sure upgrading to Universal RP 2021 does not change Spot Light shadow resolutions #3831

Wilfrid-Unity · 2021-03-11T08:05:05Z

Purpose of this PR

#2126 added to Universal RP support for custom shadow resolution for additional lights.
When a Universal RP project is upgraded to a Universal RP package containing those changes, its UniversalRenderPipeline Assets get new "Shadow Resolution Tiers" fields added with default values 256/512/1024 ; and its spot light shadows get assigned the lowest tier resolution.

The lower resolution might cause the visual quality of spot light shadows in a project to look worse after the upgrade.

This pull request ensures that after upgrade, spot light shadows will keep using the same resolution.
This is done by using the highest tier resolution as default.

Taking inspiration from@Verasl suggestion, during upgrade the 3 tier resolutions are also modified according to the additional light shadow atlas resolution set in the asset (respectively: full-res, half-res, quarter res if possible).

Testing status

In a test scene based on UniversalRP template scene added with shadowed spot lights, I locally checked that the changes in this pull request effectively conserve spot light shadow resolutions after the upgrade to Unity 2021.

Comments to reviewers

Effort tracked by internal ticket https://fogbugz.unity3d.com/f/cases/1316540

2021.1 backport pull-request: #3832

github-actions · 2021-03-11T08:05:38Z

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page)
and cancel any jobs that started on Yamato.
See the PR template for more information.
Thank you!

Verasl

LGTM

Verasl

Some minor changes to the upgrader, seems like a merge issue, we really need to update this upgrader, a bunch of 'ifs' is not the way :(

Verasl · 2021-03-16T08:35:10Z

com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs

                k_AssetVersion = 7;
            }

+            if (k_AssetVersion < 8)


Seems there was a merge issue at some point and we have ended up with 2 version 8 upgraders, this one needs to be 9 and moved after the version 8 below it

and further down in static void UpgradeAsset(UniversalRenderPipelineAsset asset) we need to bump the last check to be version 9 not 8 too

Thanks for noticing this.

I fixed it in 1b2caec, can you please check the change?

Should I also bump the version numbers in UpgradeAsset in the 2021.1 backport PR?

Fix looks good, for now we have to hold off back porting to 21.1 since it seems there is another upgrader that hasn't been back ported, this would cause some severe issues with users so I'm looking to get answers on what to do here.

Thanks for the info about 21.1.
It seems that it would be better if both 2021.2 (this PR) and 2021.1 (backport PR) landed together at the same time?
Please notify when you know what more we should do for 21.1.

@lukaschod
Will #3089 be back-ported to branch https://github.com/Unity-Technologies/Graphics/tree/2021.1/staging ?

It seems that UniversalRenderPipelineAsset.k_AssetVersion = 8 means "UniversalRenderPipelineAsset version containing improved shadow fade settings".

(With the current PR, we would have UniversalRenderPipelineAsset.k_AssetVersion = 9 meaning "UniversalRenderPipelineAsset version containing auto-upgraded spot light shadow resolutions".)

Having "auto-upgraded spot light shadow resolutions" available in 2021.2 but not in 2021.1 might be surprising for users ("when upgrading project to 2021.1 nothing happened, but it changed when upgrading again to 2021.2!?"). Therefore I would like to land both pull requests #3831 and #3832 together.

@lukaschod
If you plan to back-port #3089 to 2021.1, then #3832 has to wait (because asset update must happen in increasing version order).
If you don't plan to back-port #3089 to 2021.1, then on 2021.1 we can directly skip UniversalRenderPipelineAsset.k_AssetVersion from 7 to 9, and we will be able to land #3831 and #3832.
Please tell us, so that we can decide which path to take.

Nop I do not plan to backport #3089. It has some core changes and I do not want to introduce that risk.

Thanks a lot for the info Lukas.

I updated UniversalRenderPipelineAsset.k_AssetVersion in my 2021.1 backport pull request to match with this pull request (2021.2).

simon-engelbrecht-soerensen

Appears to be working.

Verasl

LGTM

nigeljw-unity · 2021-03-19T10:14:14Z

com.unity.render-pipelines.universal/Editor/UniversalRenderPipelineAssetEditor.cs

                    var fieldSlot = new Rect(contentRect.x + pixelShift, contentRect.y, num - labelWidth, contentRect.height); // Define the rectangle for the field
                    int value = EditorGUI.DelayedIntField(fieldSlot, values[index].intValue);
-                    values[index].intValue = Mathf.Max(128, Mathf.NextPowerOfTwo(value));
+                    values[index].intValue = Mathf.Max(UniversalAdditionalLightData.AdditionalLightsShadowMinimumResolution, Mathf.NextPowerOfTwo(value));


This isn't from this change, but we really need to get away from defaulting to power of two sizes. Most modern GPU architectures support non power of two sizes, and it is only really relevant for mipmaps and compressed formats.

If it is a atlas fitting limitation, then it is a bit more understandable.

Yes, in the case of Additional Light shadows, it is a limitation of the current (simple) atlas fitting.

nigeljw-unity · 2021-03-19T10:20:19Z

com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs

-            if (asset.k_AssetPreviousVersion < 8)
+            if (asset.k_AssetPreviousVersion < 9)
            {
                // The added feature was reverted, we keep this version to avoid breakage in case somebody already has version 7


Nit: Comment needs updating. Should the comment be generic to say previous version instead of explicitly saying 7 or 8, etc.

nigeljw-unity

LGTM

…dditionalLightData.AdditionalLightsShadowMinimumResolution

…e Spot Light shadow resolutions (internal ticket https://fogbugz.unity3d.com/f/cases/1316540 )

@Verasl

…lues if it seems that they have never been customized (@Verasl suggestion)

…ltShadowResolutionTier... constant definitions outside of #if UNITY_EDITOR block

…nsparentReceiveShadow spot light shadow resolution to higher tier to match previous settings

eh-unity · 2021-03-31T16:49:09Z

com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs

        internal const int k_ShadowCascadeMinCount = 1;
        internal const int k_ShadowCascadeMaxCount = 4;

+        public static readonly int AdditionalLightsDefaultShadowResolutionTierLow = 256;


I'm not sure if I like constant tiers. It doesn't scale very well. IMO user should just be able to put a number or 2^N for pot. It' depends on the platform and content what's suitable.
This is not a deal breaker though.

eh-unity · 2021-03-31T16:50:12Z

com.unity.render-pipelines.universal/Runtime/Data/UniversalRenderPipelineAsset.cs


+            if (k_AssetVersion < 9)
+            {
+                if (m_AdditionalLightsShadowResolutionTierHigh == AdditionalLightsDefaultShadowResolutionTierHigh &&


Maybe add a function "isCustomShadowResolution" or a local boolean at least.

Thanks for the suggestion, I added an intermediate variable in 82f939d

… (comment))

… (comment)) (cherry picked from commit 82f939d)

lukaschod

looks good

phi-lira · 2021-04-06T15:34:03Z

Lots of failed tests. Master should be failing only Hybrid Tests, maybe if you merge master and rerun it will give a better idea.

…on-auto-upgrade

…ith the one in 2021.2 pull request - see conversation #3831 (comment)

Wilfrid-Unity · 2021-04-08T04:24:16Z

Lots of failed tests. Master should be failing only Hybrid Tests, maybe if you merge master and rerun it will give a better idea.

Thanks for the info @phi-lira, I merged main branch and now there are no more failures, apart from Hybrid Tests (or Bokken test infrastructure issues not related to this pull request).

What about branch 2021.1/staging, are there some failures expected on that branch?

Wilfrid-Unity · 2021-04-09T03:58:05Z

Everything looks OK on both 2021.2 and 2021.1, I'm merging the pull requests.

@Verasl

…1 does not change Spot Light shadow resolutions (#3832) * [UniversalRP] Replace some magic numbers by named variable UniversalAdditionalLightData.AdditionalLightsShadowMinimumResolution * [UniversalRP] Make sure upgrading to Universal RP 2021 does not change Spot Light shadow resolutions (internal ticket https://fogbugz.unity3d.com/f/cases/1316540 ) * [UniversalRP] Only upgrade Additional Light Shadow Resolution Tier values if it seems that they have never been customized (@Verasl suggestion) * [UniversalRP] Move UniversalRenderPipelineAsset.AdditionalLightsDefaultShadowResolutionTier... constant definitions outside of #if UNITY_EDITOR block * [UniversalRP] Increase UniversalGraphicsTest_Foundation scene 105_TransparentReceiveShadow spot light shadow resolution to higher tier to match previous settings * Fix asset that was incorrectly edited in e82aa3d * Add local variable to make code easier to read (as suggested in #3831 (comment)) (cherry picked from commit 82f939d) * Update UniversalRenderPipelineAsset.k_AssetPreviousVersion to match with the one in 2021.2 pull request - see conversation #3831 (comment) * [UniversalRP] Increase UniversalGraphicsTest scene 105_TransparentReceiveShadow spot light shadow resolution to higher tier to match previous settings

Wilfrid-Unity added universal needs-backport-2021.1 labels Mar 11, 2021

Wilfrid-Unity requested review from a team as code owners March 11, 2021 08:05

Wilfrid-Unity requested review from Verasl, eh-unity, lukaschod and nigeljw-unity March 11, 2021 08:05

Wilfrid-Unity mentioned this pull request Mar 11, 2021

[Universal] [2021.1 Backport] Make sure upgrading to Universal RP 2021 does not change Spot Light shadow resolutions #3832

Merged

Verasl approved these changes Mar 16, 2021

View reviewed changes

Verasl self-requested a review March 16, 2021 08:34

Verasl suggested changes Mar 16, 2021

View reviewed changes

simon-engelbrecht-soerensen approved these changes Mar 16, 2021

View reviewed changes

Wilfrid-Unity added a commit that referenced this pull request Mar 17, 2021

[UniversalRP] Fix merge error - Addresses comment #3831 (review)

1b2caec

Verasl approved these changes Mar 18, 2021

View reviewed changes

nigeljw-unity reviewed Mar 19, 2021

View reviewed changes

nigeljw-unity approved these changes Mar 19, 2021

View reviewed changes

Wilfrid-Unity requested a review from a team as a code owner March 25, 2021 10:30

Wilfrid-Unity added 6 commits March 25, 2021 11:46

[UniversalRP] Replace some magic numbers by named variable UniversalA…

94393c1

…dditionalLightData.AdditionalLightsShadowMinimumResolution

[UniversalRP] Make sure upgrading to Universal RP 2021 does not chang…

204a9e1

…e Spot Light shadow resolutions (internal ticket https://fogbugz.unity3d.com/f/cases/1316540 )

[UniversalRP] Only upgrade Additional Light Shadow Resolution Tier va…

1f7e953

…lues if it seems that they have never been customized (@Verasl suggestion)

[UniversalRP] Move UniversalRenderPipelineAsset.AdditionalLightsDefau…

cd1b5e8

…ltShadowResolutionTier... constant definitions outside of #if UNITY_EDITOR block

[UniversalRP] Fix merge error - Addresses comment #3831 (review)

aac184c

[UniversalRP] Increase UniversalGraphicsTest_Foundation scene 105_Tra…

43ed67d

…nsparentReceiveShadow spot light shadow resolution to higher tier to match previous settings

Verasl force-pushed the universal/additional-light-shadow-resolution-auto-upgrade branch from c92f728 to 43ed67d Compare March 25, 2021 10:48

theopnv removed the request for review from a team March 25, 2021 16:16

eh-unity approved these changes Mar 31, 2021

View reviewed changes

Add local variable to make code easier to read (as suggested in #3831…

82f939d

… (comment))

Wilfrid-Unity added a commit that referenced this pull request Apr 1, 2021

Add local variable to make code easier to read (as suggested in #3831…

1fbcde0

… (comment)) (cherry picked from commit 82f939d)

lukaschod approved these changes Apr 6, 2021

View reviewed changes

Merge branch 'master' into universal/additional-light-shadow-resoluti…

e6b8db2

…on-auto-upgrade

Wilfrid-Unity added a commit that referenced this pull request Apr 7, 2021

Update UniversalRenderPipelineAsset.k_AssetPreviousVersion to match w…

997afae

…ith the one in 2021.2 pull request - see conversation #3831 (comment)

Wilfrid-Unity merged commit db4edf6 into master Apr 9, 2021

Wilfrid-Unity deleted the universal/additional-light-shadow-resolution-auto-upgrade branch April 9, 2021 03:58

[Universal] Make sure upgrading to Universal RP 2021 does not change Spot Light shadow resolutions #3831

[Universal] Make sure upgrading to Universal RP 2021 does not change Spot Light shadow resolutions #3831

Uh oh!

Conversation

Wilfrid-Unity commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Mar 11, 2021

Uh oh!

Verasl left a comment

Choose a reason for hiding this comment

Uh oh!

Verasl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simon-engelbrecht-soerensen left a comment

Choose a reason for hiding this comment

Uh oh!

Verasl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nigeljw-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukaschod left a comment

Choose a reason for hiding this comment

Uh oh!

phi-lira commented Apr 6, 2021

Uh oh!

Wilfrid-Unity commented Apr 8, 2021

Uh oh!

Wilfrid-Unity commented Apr 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Wilfrid-Unity commented Mar 11, 2021 •

edited

Loading