Implementation of SRP Global Settings for HDRP #3054

jenniferd-unity · 2021-01-11T16:58:17Z

Purpose of this PR

Presentation powerpoint here
This is the first implementation of HDRP Global Settings as an asset separate from the HDRPAsset. Further development is planned.

[UI updated on Feb 26th]

I have moved from the HDRenderPipeline.defaultAsset various settings:

Resources (Editor/RayTracing/Player)
Default Volume Profile for Scenes and LookDev
Layer names
Shader Variant Log level
Default Frame Settings for Camera, Reflection Probes
Lens Attenuation
Diffusion Profile Settings List
Custom Post Processes Lists

No behaviors should be changed on any HDRP project with those changes except:

the HDRPAsset no longer contains settings that were duplicated for the default asset
there is always a HDRenderPipelineGlobalSettings asset active
this asset can be viewed from the Project Settings > Graphics window
individual HDRenderPipelineGlobalSettings assets have their own inspector.

When upgrading to this new version:

a new asset should be created in the HDDefaultResources folder called HDRenderPipelineGlobalSettings .asset
values of the HDRPAsset saved in GraphicsSettings compatible are migrated to this new asset

The create menu can be found here:

When deleting the active Global Settings asset, a new one will be created and an error message displayed:
https://user-images.githubusercontent.com/71718938/108837110-145aa980-75d2-11eb-8a70-6c66236f2941.mp4

Asset creation workflow: set as active Asset if created from the HDRP Settings menu only.
https://user-images.githubusercontent.com/71718938/108840941-4faba700-75d7-11eb-853e-0cb2d87876d8.mp4

Testing status

Tried the following locally - on Windows only:

Editor: upgrading HDRP template, Spaceship demo, small local projects,
Creating new project from Hub,
Creating players for some projects (template HD and spaceship demo),
Running HDRP tests, RayTracing tests.

Comments to reviewers

This is the first implementation of HDRP Global Settings as an asset separate from the HDRPAsset. Further development is planned.

JIRA

https://jira.unity3d.com/browse/XPIPELINE-38
https://unity3d.atlassian.net/browse/UR-1967

This reverts commit c4a0df6.

# Conflicts: # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDRaytracingReflection.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Settings/FrameSettingsHistory.cs # com.unity.render-pipelines.high-definition/Runtime/Sky/StaticLightingSky.cs

alelievr

Left a few comments, probably worth it to write an upgrade guide too (I think you can put it here: https://github.com/Unity-Technologies/Graphics/blob/master/com.unity.render-pipelines.high-definition/Documentation~/Upgrading-from-2021.1-to-2021.2.md)

com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDAssetFactory.cs

com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDDefaultSettingsEditor.cs

com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDEditorUtils.cs

...ity.render-pipelines.high-definition/Editor/RenderPipeline/Settings/EditorDefaultSettings.cs

...ines.high-definition/Runtime/Material/DiffusionProfile/DiffusionProfileSettings.Migration.cs

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

...ines.high-definition/Runtime/Material/DiffusionProfile/DiffusionProfileSettings.Migration.cs

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

alex-vazquez

Good clean up :) Minor comments.

...y.render-pipelines.high-definition/Runtime/RenderPipeline/Settings/RenderPipelineSettings.cs

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

RSlysz

Sadly, hard to read due to the amount of change but seams globally good.

com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDDefaultSettingsEditor.cs

RSlysz · 2021-01-20T14:30:54Z

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

            {
                case Kind.Default:
-                    hdrpAsset.defaultVolumeProfile = profile;
+                    if (settings != null)


Perhaps you should check this first things in this method and exit quickly if not true then. (or you will try letter to EditorUtility.SetDirty(null); )

This is still valid. Verify if you can dirty null.

I changed this in the last commit.

...ity.render-pipelines.high-definition/Editor/RenderPipeline/Settings/EditorDefaultSettings.cs

...ity.render-pipelines.high-definition/Runtime/RenderPipeline/Settings/FrameSettingsHistory.cs

…decal layer names to exist on DefaultSettings

alex-vazquez

Really minor thinks. Nice PR :)

com.unity.render-pipelines.high-definition/Editor/BuildProcessors/HDRPPreprocessShaders.cs

com.unity.render-pipelines.high-definition/Editor/RenderPipeline/HDAssetFactory.cs

...ender-pipelines.high-definition/Editor/RenderPipeline/Settings/SerializedHDGlobalSettings.cs

# 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/Runtime/Compositor/CompositionManager.cs # com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/LightLoop.cs # com.unity.render-pipelines.high-definition/Runtime/Material/SubsurfaceScattering/SubsurfaceScatteringManagerRT.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDRaytracingAmbientOcclusion.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDRaytracingReflection.cs

…yles.globalSettingsIcon)

…iew suggestions

RSlysz · 2021-03-08T16:28:10Z

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

+        public SerializedProperty renderPipelineResources;
+        public SerializedProperty renderPipelineRayTracingResources;
+
+        public SerializedFrameSettings defaultFrameSettings;


Could be better if contain Camera in this name (distinguish from other default)

RSlysz · 2021-03-08T16:35:40Z

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

+            defaultFrameSettings = new SerializedFrameSettings(serializedObject.FindProperty("m_RenderingPathDefaultCameraFrameSettings"), null); //no overrides in HDRPAsset
+            defaultBakedOrCustomReflectionFrameSettings = new SerializedFrameSettings(serializedObject.FindProperty("m_RenderingPathDefaultBakedOrCustomReflectionFrameSettings"), null); //no overrides in HDRPAsset
+            defaultRealtimeReflectionFrameSettings = new SerializedFrameSettings(serializedObject.FindProperty("m_RenderingPathDefaultRealtimeReflectionFrameSettings"), null); //no overrides in HDRPAsset


Be sure that TitleDrawingScope comply with the null value.

RSlysz · 2021-03-08T16:43:56Z

...y.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipelineAsset.Migration.cs

+        void OnEnable()
+        {
+            k_Migration.Migrate(this);
+        }


[Beeing picky] why this change?

reverted it :)

RSlysz · 2021-03-08T16:47:28Z

com.unity.render-pipelines.high-definition/Runtime/Compositor/CompositionManager.cs

            SetupLayerPriorities();
        }

+        static HDRenderPipelineGlobalSettings m_globalSettings;


If I understand well, there is case where we want to change the global settings (using Unity in dofferent context). If it is registered as static here, it will not adapt except if a domain reload occures. Can we assert this is the case?

this adapts as in RegisterCustomPasses we are checking the value. (lines 937 - 950). Could you confirm it works for you?

# Conflicts: # com.unity.render-pipelines.core/Editor/CoreEditorStyles.cs # com.unity.render-pipelines.high-definition/CHANGELOG.md # com.unity.render-pipelines.high-definition/Documentation~/Upgrading-from-2021.1-to-2021.2.md # com.unity.render-pipelines.high-definition/Editor/RenderPipeline/Settings/FrameSettingsUI.Drawers.cs # com.unity.render-pipelines.high-definition/Editor/Wizard/HDWizard.Configuration.cs # com.unity.render-pipelines.high-definition/Runtime/PostProcessing/PostProcessSystem.RenderGraph.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/PathTracing/PathTracing.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDRaytracingIndirectDiffuse.cs # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Raytracing/HDRaytracingRecursiveRenderer.cs

…tings

remi-chapelain

As agreed with HDRP Team, go for merge ! ✔️
There's still a few minor issues referenced in the test document.

Those shouldn't impact upgrade process and new users from doing anything and have an obvious workaround and/or are easily dismissible by users.
Once the PR is merged, they'll be converted into bugs to prevent them from lingering too long in alpha versions !

# Conflicts: # com.unity.render-pipelines.high-definition/Documentation~/Upgrading-from-2021.1-to-2021.2.md # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipelineAsset.cs

kroatoa · 2021-03-12T13:10:47Z

Hi @jenniferd-unity
You write

Further development is planned.
Does that refer to the rest of the work to get the URP/HDRP compatibilty, or is that work needed to make the feature in this PR complete?

jenniferd-unity · 2021-03-12T13:43:51Z

Hi, yes the follow up is here: https://jira.unity3d.com/browse/XPIPELINE-97.
Also i synced latest master this morning and will fix yamato today :)

kroatoa

thank you for the clarification.

Made sure HDRenderPipelineGlobalSettings are loaded before pulling its setting for shadervariantloglevel.

…or (cannot create an asset while building a player)

# Conflicts: # TestProjects/HDRP_Tests/Assets/GraphicTests/Scenes/5x_SkyAndFog/5011_VolumetricClouds.unity # TestProjects/HDRP_Tests/Assets/GraphicTests/Scenes/5x_SkyAndFog/5011_VolumetricCloudsShadows.unity # 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/Runtime/Debug/LightingDebug.cs

jenniferd-unity added 6 commits December 21, 2020 15:26

Initial commit

d11aba2

Fixed Scene Template to refer to currentAsset instead of defaultAsset

075f822

Fixed light+decal layers serialization

c4a0df6

Revert "Fixed light+decal layers serialization" + a few other fixes

fb7cf04

This reverts commit c4a0df6.

A couple more reference fixed

dce157d

github-actions bot added the HDRP label Jan 11, 2021

Added missing changelog

5f37833

jenniferd-unity changed the title ~~Hd/hdrp default settings~~ Implementation of SRP Default Settings for HDRP Jan 14, 2021

jenniferd-unity requested review from RSlysz, alelievr and alex-vazquez January 18, 2021 16:58

alelievr reviewed Jan 19, 2021

View reviewed changes

alex-vazquez reviewed Jan 20, 2021

View reviewed changes

...y.render-pipelines.high-definition/Runtime/RenderPipeline/Settings/RenderPipelineSettings.cs Outdated Show resolved Hide resolved

...y.render-pipelines.high-definition/Editor/RenderPipeline/Settings/SerializedFrameSettings.cs Outdated Show resolved Hide resolved

RSlysz approved these changes Jan 20, 2021

View reviewed changes

jenniferd-unity added 5 commits February 2, 2021 14:29

Change from RenderPipeline_Default_Settings to Global renaming

b130f54

[Fix] layer names are pulled from the default settings

6a4b136

[Fix] Ensure HDDefault settings before showing menu + refactor light/…

3350f13

…decal layer names to exist on DefaultSettings

Obselete EditorDefaultSettings, fixed a few bad references

2dbd454

Renamed from HDDefaultSettings to HDGlobalSettings

6488f01

jenniferd-unity requested review from RSlysz, alelievr and alex-vazquez February 12, 2021 14:57

alelievr approved these changes Feb 15, 2021

View reviewed changes

alex-vazquez suggested changes Feb 16, 2021

View reviewed changes

jenniferd-unity added 5 commits February 16, 2021 12:11

Renamed from HDGlobalSettings to HDRenderPipelineGlobalSettings

87f9927

Moved HDRenderPipelineGlobalSettingsCreator to its own file

f0f489a

Added a central place to change RPGlobalSettings's icon (CoreEditorSt…

0ab2144

…yles.globalSettingsIcon)

Refactored SerializedHDRenderPipelineGlobalSettings per Alex's PR rev…

de1c55b

…iew suggestions

jenniferd-unity requested a review from RSlysz March 8, 2021 15:32

RSlysz requested changes Mar 8, 2021

View reviewed changes

remi-chapelain self-requested a review March 9, 2021 12:01

A few fixes when migrating from 2020.2 to 2021.2

97a718f

sebastienlagarde requested a review from a team March 10, 2021 09:20

jenniferd-unity added 2 commits March 10, 2021 13:23

New migration workflow - lazy migration from HDRP Asset to Global Set…

2000f9b

…tings

jenniferd-unity requested a review from RSlysz March 10, 2021 13:49

RSlysz approved these changes Mar 10, 2021

View reviewed changes

remi-chapelain approved these changes Mar 10, 2021

View reviewed changes

UnityCatalina self-requested a review March 11, 2021 10:56

UnityCatalina approved these changes Mar 12, 2021

View reviewed changes

Merge branch 'master' into hd/hdrp-default-settings

43ca2f8

# Conflicts: # com.unity.render-pipelines.high-definition/Documentation~/Upgrading-from-2021.1-to-2021.2.md # com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipelineAsset.cs

jenniferd-unity marked this pull request as ready for review March 12, 2021 11:07

github-actions bot added the SRP label Mar 12, 2021

jenniferd-unity added 4 commits March 12, 2021 18:51

Adapting to tests to new workflow

7491a8b

Fix test 4060

151e219

Fixed frame settings for HDRP test projects

aa78c63

Fix for HDRP DXR test 803

0a50d1b

kroatoa approved these changes Mar 16, 2021

View reviewed changes

sebastienlagarde and others added 5 commits March 16, 2021 10:04

Merge branch 'master' into hd/hdrp-default-settings

7044f86

Fix for player build from commandline

4ff810b

Made sure HDRenderPipelineGlobalSettings are loaded before pulling its setting for shadervariantloglevel.

Fix case where building a player without opening it first in the Edit…

262b618

…or (cannot create an asset while building a player)

Adding HDRP Global Settings to HDRP Runtime tests project

62c9baf

sebastienlagarde merged commit 1c1482a into master Mar 18, 2021

sebastienlagarde deleted the hd/hdrp-default-settings branch March 18, 2021 13:08

PaulDemeulenaere mentioned this pull request Mar 26, 2021

[VFX/X-Pipeline] Update HDRP asset #4017

Merged

Implementation of SRP Global Settings for HDRP #3054

Implementation of SRP Global Settings for HDRP #3054

Uh oh!

Conversation

jenniferd-unity commented Jan 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

JIRA

Uh oh!

alelievr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-vazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RSlysz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

alex-vazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

remi-chapelain left a comment

Choose a reason for hiding this comment

Uh oh!

kroatoa commented Mar 12, 2021

Uh oh!

jenniferd-unity commented Mar 12, 2021

Uh oh!

kroatoa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jenniferd-unity commented Jan 11, 2021 •

edited

Loading