[XPIPELINE-97] Polish HDRP Global Settings #4012

jenniferd-unity · 2021-03-26T12:31:49Z

Purpose of this PR

This PR aims to finish and polish the implementation of HDRP Global Settings for 2021.2.
Here is a list of changes:

Code-only changes:

Refactored code to use m_globalSettings instead of HDRenderPipelineGlobalSettings.instance for partial class of HDRenderPipeline [suggested by Julien Ignace]
Removed internal API HDRenderPipeline.defaultAsset
Renamed SerializedFrameSettings defaultFrameSettings; to defaultCameraFrameSettings
Cleaned a bit HDRenderPipeline localized strings (removed unnecessary ones)
Renamed VolumeProfileLookDev to LookDevVolumeProfile - as suggested by Remi Slysz

Fixed broken fixes:

Wizard - profile creation: default PR 3873 - previously fixed by Julien Ignace
DXR - missing resources depending on Quality Level: 1320304 - previously fixed by Anis Benyoub
1313426

Fixes:

HDRP Global Settings Asset deletion when HDRP is active or not (the asset is recreated only if HDRP is active + an WARNING message displays in the console)

(Example showing error message when switching from URP to HDRP and no HDRP Global Settings asset was assigned in the Graphics Settings)
Warnings and Info messages in the HDRP Settings menu are clearer
Fixed HDRP Asset creation when HDRP Global Settings was not present (for ex. my project was on URP only)
Frame Settings refactor (see below)
incl. fix for https://fogbugz.unity3d.com/f/cases/1323947/

Frame Settings refactor (see here):

Renamed in the HDRP Settings window Frame Settings to Frame Settings (Default Values)
made sure the value displayed in the Custom Frame Settings showed the value of the HDRP Global Settings asset
Changed: you can always edit a setting no matter the active HDRP Asset
Changed: when you edit Custom Frame Settings, you can always override values no matter the parent or HDRP Asset. it is disabled only when you cannot change them because it has no impact.
Changed: Virtual Texturing setting is no longer disabled in the UI but sanitized instead, better tooltip to show
Improved a few tooltips - still more to do but will be another PR
Checked the Debug Render Pipeline menu to ensure values shown are still correct

Example of how Frame Settings behave now (Camera / HDRP Global Settings):

Example of changes:

Testing status

Tested on Windows Editor for local test project and Template HD.
Tried also a build of Template HD.

Known issues: https://fogbugz.unity3d.com/f/cases/1323947/
Important to test that broken fixes are actually fixed + Frame Settings behave as expected

Comments to reviewers

This is the last PR for HDRP Global Settings planned before cut-off.

…rPIpelineGlobalSettings.instance

…eted

…rriden

RSlysz · 2021-03-26T15:05:42Z

com.unity.render-pipelines.high-definition/Editor/Lighting/Reflection/ReflectionMenuItem.cs

-            var hdrp = HDRenderPipeline.defaultAsset;
-            var material = hdrp != null ? hdrp.GetDefaultMirrorMaterial() : null;
-
+            var material = HDRenderPipeline.currentAsset?.GetDefaultMirrorMaterial();


So we switch from default (gfx one) to current (potentially quality). Is this ok ?

Both are fine, since GetDefaultMirrorMaterial will get the HDRP Global Settings one ;)

i could actually rewrite that to grab the HDRP Global Settings one!

fixed in upcoming commit

RSlysz · 2021-03-26T15:16:12Z

...y.render-pipelines.high-definition/Editor/RenderPipeline/Settings/FrameSettingsUI.Drawers.cs

+                hasMixedValues: serialized.maximumLODLevel.hasMultipleDifferentValues);
+
+            area.AmmendInfo(FrameSettingsField.MaterialQualityLevel,
+                overridedDefaultValue: defaultFrameSettings?.materialQuality.Into() ?? MaterialQualityMode.Medium,


Why it should be Medium above all other possibility? I mean if defaultFrameSettings is null, it means we are displaying the version in the global settings. So why it should be linked to Medium quality mode? This seams strange. At least it need comments to explain why we chose this default here.

we needed to chose a value, i didnt know which one to use so i chose medium

I think this need to be checked, perhaps ask TA and UX for this point.

But it can be fixed in a later PR perhaps

RSlysz · 2021-03-26T15:21:35Z

...nity.render-pipelines.high-definition/Editor/RenderPipeline/Settings/FrameSettingsUI.Skin.cs

        const string renderingSettingsHeaderContent = "Rendering";
        const string lightSettingsHeaderContent = "Lighting";


Should this be localized?

yes! will fix

RSlysz · 2021-03-26T15:22:53Z

...der-pipelines.high-definition/Editor/RenderPipeline/Settings/OverridableFrameSettingsArea.cs

            bool enabled = field.IsOverrideableWithDependencies(serializedFrameSettings, defaultFrameSettings);
            withOverride &= enabled & GUI.enabled;
-            bool shouldBeDisabled = withOverride || !enabled || !GUI.enabled;
+            bool shouldBeDisabled = (withOverride || !enabled || !GUI.enabled);


uneeded parenthesis

RSlysz · 2021-03-26T15:27:05Z

...s.high-definition/Editor/RenderPipeline/Settings/SerializedHDRenderPipelineGlobalSettings.cs

            volumeProfileDefault  = serializedObject.FindProperty("m_VolumeProfileDefault");
-            volumeProfileLookDev  = serializedObject.FindProperty("m_VolumeProfileLookDev");
+            lookDevVolumeProfile  = serializedObject.FindProperty("m_LookDevVolumeProfile");


ha sorry but now we have another name inconsistency here:
LookDevVolumeProfile
VolumeProfileDefault

Should this be defaultVolumeProfile?

i could yes. will update

Wait if i change it, i need to obsolete and automatically rename no?

Is there any release with this version landed?
yes -> yes you need to
no -> just change it quickly before any release land

those changes landed in srp2core so i did a migration pass in updated file

RSlysz · 2021-03-26T15:30:21Z

...-pipelines.high-definition/Editor/RenderPipelineResources/DefaultSettingsVolumeProfile.asset

+    m_OverrideState: 0
+    m_Value: 10


here too: value changed. Please check

will remove this file from the PR - i am not supposed to change it

RSlysz · 2021-03-26T15:30:42Z

...-pipelines.high-definition/Editor/RenderPipelineResources/DefaultSettingsVolumeProfile.asset

    m_Value: 1
  m_Resolution:
-    m_OverrideState: 0
+    m_OverrideState: 1


here too: value changed. Please check

RSlysz · 2021-03-26T15:30:48Z

...-pipelines.high-definition/Editor/RenderPipelineResources/DefaultSettingsVolumeProfile.asset

+    m_Value: 0
  m_HighQualityFiltering:
-    m_OverrideState: 0
+    m_OverrideState: 1


here too: value changed. Please check

RSlysz · 2021-03-26T15:32:32Z

...-pipelines.high-definition/Editor/RenderPipelineResources/DefaultSettingsVolumeProfile.asset

+  cameraMotionBlur:
+    m_OverrideState: 0
+    m_Value: 1
+  specialCameraClampMode:
+    m_OverrideState: 0
+    m_Value: 0
+  cameraVelocityClamp:
+    m_OverrideState: 0
+    m_Value: 0.05
+  cameraTranslationVelocityClamp:
+    m_OverrideState: 0
+    m_Value: 0.05


also for all added entry (here and bellow), check if it is still the default from code

com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs

RSlysz · 2021-03-29T07:25:05Z

Better :)
Check value in asset change and add a comment on why default on MaterialQualityMode.Medium and it will be good :)

jenniferd-unity · 2021-03-29T10:54:04Z

...nity.render-pipelines.high-definition/Editor/RenderPipeline/Settings/DefaultSettingsPanel.cs

            internal static readonly GUIContent lensAttenuationModeContentLabel = EditorGUIUtility.TrTextContent("Lens Attenuation Mode", "Set the attenuation mode of the lens that is used to compute exposure. With imperfect lens some energy is lost when converting from EV100 to the exposure multiplier.");

            internal static readonly GUIContent diffusionProfileSettingsLabel = EditorGUIUtility.TrTextContent("Diffusion Profile Assets");
+            internal static readonly string warningHdrpNotActive = "No HD Render Pipeline currently active. Verify your Graphics Settings and active Quality Level.";


Note for reviewers: Currently error and warning messages in C# are left unlocalized.
See https://confluence.unity3d.com/display/LOC/Editor+Localization+Coding+Guidelines

…o be commited

…sset

JulienIgnace-Unity · 2021-03-29T11:52:25Z

...nity.render-pipelines.high-definition/Editor/RenderPipeline/Settings/DefaultSettingsPanel.cs

        {
        }

+        void Draw_Warnings(ref SerializedHDRenderPipelineGlobalSettings serialized, Editor owner)


I think code convention does not allow '_' inside names

The rest of the file is like that and other windows UI code, so it should be good no?

Well, according to code convention, we should not use _ except for prefixing https://ono.unity3d.com/unity-extra/unity-meta/raw/@/ReferenceSource/CSharp/Assets/CSharpReference.cs

So I assum it was already wrong at the time :/

Removed _ in function names Fixed issue when created HDRP Asset where HDRP Global Settings was not created Removed changes on Default LookDev Profile Changed Error to Warning when creating automatically Global Settings asset for the user

RSlysz

Seams good. Check with UX/TA for default on medium quality for material. This need to be clarified (can be in later PR)

# Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

eh-unity

LGTM.

eh-unity · 2021-03-31T16:19:39Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.cs

-                // If ray tracing is not enabled we do not want to have ray tracing resources referenced
-                m_GlobalSettings.ClearRayTracingResources();
+                int qualityLevelCount = QualitySettings.names.Length;
+                for (int i = 0; i < qualityLevelCount && !requiresRayTracingResources; ++i)


Maybe just use break in the if instead of second condition of !requiresRayTracingResources
I think it's a bit easier to read that way.

iM0ve

Testing: Done

Whats tested:

Regression testing
- Creating a new HDRP project and running the DXR wizard
- Using Frame settings on multiple cameras
- Frame settings can override Default settings. ex. If you turn on Opaque on camera but turn it off in Default - opaque objects will be rendered.
- Switching quality settings Low->Med->High on Template. Works as intended.
- Debug Scene camera and Other cameras correctly display active Frame Settings
- If something is Off in the HDRP asset, but On in Frame settings. The feature will not be present in the scene, as expected.
- Real-world usage: Upgrading Amalienborg, changing Frame Settings
- Building a player
- Changing Frame settings via Inspector and via Project Settings tab
- Editing light layer names
Every fix as requested in PR
- Wizard - profile creation: default PR 3873 - previously fixed by Julien Ignace
- Building DXR HDRP template on High preset
- Null ref when URP and HDRP are installed 1313426
- HDRP Global Settings Asset is no longer created when URP or Default is the current renderer
- HDRP Default settings not saving, case 1323947
Frame Settings refactor
- Frame Settings is renamed
- You can always edit a setting no matter the active HDRP Asset
- you edit Custom Frame Settings, you can always override values no matter the parent or HDRP Asset. it is disabled only when you cannot change them because it has no impact.

Issues blocking the PR:

[Fixed] Frame settings can not be saved

j0qFUlOGMR.mp4

[Fixed] Error on reflection probe change

BKk2Dsm7yb.mp4

Issues I found that I will file fogbugz for, not blocking the PR:

The path no longer exists described in the warning

Global settings ignore the name specified in HDRP wizard

LCgkLo0eFZ.mp4

# Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md # com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs # com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Window.cs

… folder

…iew closed but HD Settings tab is opened

jenniferd-unity · 2021-04-08T13:05:08Z

Submitted fixes for the 3 issues reported @iM0ve and synced to latest master yesterday :)
Changes reviewed by @RSlysz too.
will relaunch a yamato but we should be good for another QA pass

iM0ve

Updated the review above. Should be good to merge now.
Filed 2 fogbugz for cosmetic issues:
https://fogbugz.unity3d.com/f/cases/1327978/
https://fogbugz.unity3d.com/f/cases/1327981/

jenniferd-unity added 12 commits March 24, 2021 12:28

Use m_globalSettings inside HDRenderPipeline class instead of HDRende…

a5e1836

…rPIpelineGlobalSettings.instance

Removed defaultAsset from HDRenderPipeline

5c96a13

Renamed defaultFrameSettings to mention Camera

f6cf788

Fix HDRP Wizard Volume Profiles workflow

4fed323

Fixed again https://fogbugz.unity3d.com/f/cases/1320304/

9604052

First pass on Frame Settings refactor

cbf8569

[Frame Settings] Grayed out UI when parent item is disabled

7c2e35a

Improved warnings and errors when HDRP Global Settings asset gets del…

49fa862

…eted

Changed UI for Frame Settings to never disable when they could be ove…

52679e7

…rriden

More Frame Settings UI cleanup

684b4c3

Cleaning

9f6daed

Merge branch 'master' into hd/global-settings-polish

5a76ce4

github-actions bot added the HDRP label Mar 26, 2021

jenniferd-unity added 2 commits March 26, 2021 15:18

[Fix] Migrating empty diffusion profile list not error-ing

8b81fae

Renamed VolumeProfileLookDev to LookDevVolumeProfile for consistency

478b4e4

jenniferd-unity requested review from JulienIgnace-Unity and RSlysz March 26, 2021 14:37

(Re-)Added checks for before accessing HDRP's resources

488fb2d

RSlysz reviewed Mar 26, 2021

View reviewed changes

Additional cleanup

b1745e4

Updated documentation and changelog

f8d66d2

jenniferd-unity commented Mar 29, 2021

View reviewed changes

Revert changes to DefaultSettingsVolumeProfile.asset - not supposed t…

c463aca

…o be commited

jenniferd-unity requested a review from JordanL8 March 29, 2021 11:08

Cleanup to reference HDRenderPIpelineGlobalSettings instead of HDRP A…

de26136

…sset

JulienIgnace-Unity reviewed Mar 29, 2021

View reviewed changes

jenniferd-unity requested review from a team and eh-unity March 29, 2021 16:36

Cleanup

07fd186

Removed _ in function names Fixed issue when created HDRP Asset where HDRP Global Settings was not created Removed changes on Default LookDev Profile Changed Error to Warning when creating automatically Global Settings asset for the user

RSlysz approved these changes Mar 30, 2021

View reviewed changes

Minor rewording

55436b5

JordanL8 approved these changes Mar 30, 2021

View reviewed changes

jenniferd-unity self-assigned this Mar 30, 2021

JulienIgnace-Unity approved these changes Mar 30, 2021

View reviewed changes

jenniferd-unity added 2 commits March 31, 2021 11:33

Merge branch 'master' into hd/global-settings-polish

1786f69

# Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md

Forgot to push this

20779cf

jenniferd-unity marked this pull request as ready for review March 31, 2021 13:58

eh-unity approved these changes Mar 31, 2021

View reviewed changes

iM0ve reviewed Apr 1, 2021

View reviewed changes

jenniferd-unity added 4 commits April 7, 2021 09:43

Merge branch 'master' into hd/global-settings-polish

7fe0485

# Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md # com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs # com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Window.cs

[Fix] Wizard now fixes Global Settings Asset creation in the settings…

67f5a10

… folder

Making sure to load/create the HDRP Global Settings when Scene/Game V…

27bcaf4

…iew closed but HD Settings tab is opened

Dirtying cannot be done during OnEnable. Delaying the call.

6fe43eb

jenniferd-unity requested a review from iM0ve April 8, 2021 13:05

Null check on migration

29db241

iM0ve approved these changes Apr 9, 2021

View reviewed changes

sebastienlagarde merged commit 67cb89a into master Apr 9, 2021

sebastienlagarde deleted the hd/global-settings-polish branch April 9, 2021 17:56

		const string renderingSettingsHeaderContent = "Rendering";
		const string lightSettingsHeaderContent = "Lighting";

[XPIPELINE-97] Polish HDRP Global Settings #4012

[XPIPELINE-97] Polish HDRP Global Settings #4012

Uh oh!

Conversation

jenniferd-unity commented Mar 26, 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!

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!

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!

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!

Uh oh!

RSlysz commented Mar 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

RSlysz left a comment

Choose a reason for hiding this comment

Uh oh!

eh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iM0ve left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenniferd-unity commented Apr 8, 2021

jenniferd-unity commented Mar 26, 2021 •

edited

Loading

JulienIgnace-Unity Mar 29, 2021 •

edited

Loading

iM0ve left a comment •

edited

Loading