[2021.2] Control Volume Framework updates #4242

ellioman · 2021-04-20T13:14:56Z

Purpose of this PR

This PR is adding the option to disable volume framework updates in the Camera Inspector and to the URP Asset. It also adds public API on the Camera for users to update the volume or to change the setting.

Things done in this PR

Editor changes

Camera Inspector: Added a Update Mode dropdown
URP Asset: Added a Update Mode dropdown under Advanced

Scripting changes

Public

VolumeManager: Made the stack setter public
VolumeManager: Added a m_DefaultStack variable and cache the original stack in that.
VolumeManager: Added a public ResetMainStack() function.
Camera: Added a public function GetVolumeFrameworkUpdateMode()
Camera: Added a public function SetVolumeFrameworkUpdateMode(VolumeFrameworkUpdateMode mode)
Camera: Added a public function UpdateVolumeStack()
Camera: Added a public function UpdateVolumeStack(UniversalAdditionalCameraData cameraData)
AdditionalCameraData: Added an public property requiresVolumeFrameworkUpdate
AdditionalCameraData: Added an public property volumeStack
UniversalRenderPipelineAsset: Added a public enum VolumeFrameworkUpdateMode
UniversalRenderPipelineAsset: Added a public property volumeFrameworkUpdateMode

Internal

Camera: Added an internal function GetVolumeLayerMaskAndTrigger(UniversalAdditionalCameraData cameraData, out LayerMask layerMask, out Transform trigger)
AdditionalCameraData: Added an internal property volumeFrameworkUpdateMode

Other changes

Testing: Created a new test scene using a global and local volume with three cameras using different masks. This test scene will be enabled in a follow-up PR.
Other: Added some Documentation that were missing for some public properties

UI Changes

Camera Inspector

Before

After

URP Asset

Before

After

Testing

What has been tested

Tested locally in the editor (OS X)

What needs testing

Editor: Test if Volumes are always updated when not in playmode
Editor: Test if Volumes are updated in playmode when EveryFrame is selected on the camera and/or URP Asset
Editor: Test if Volumes are not updated in playmode when ViaScripting is selected on the camera and/or URP Asset
Editor/Builds: Scripting: Camera: Test Getting & Setting VolumeFrameworkUpdateMode on the camera.
Editor/Builds: Scripting: Camera: Test if the two UpdateVolumeStack() functions do not do anything when it's set to EveryFrame
Editor/Builds: Scripting: Camera: Test if the two UpdateVolumeStack() functions update the correct volume stack when it's set to Via Scripting (Test with multiple cameras with various mask settings)
Editor/Builds: Scripting: AdditionalCameraData: Test if requiresVolumeFrameworkUpdate returns correct results

Later work

A follow-up PR will add the new test scene to our CI

ellioman · 2021-04-20T14:11:38Z

com.unity.render-pipelines.universal/Runtime/UniversalAdditionalCameraData.cs

+        /// <summary>
+        /// Returns true if this camera should automatically replace NaN/Inf in shaders by a black pixel to avoid breaking some effects.
+        /// </summary>


Extra documentation that bothered me when looking at the code.

ellioman · 2021-04-20T14:11:46Z

com.unity.render-pipelines.universal/Runtime/UniversalAdditionalCameraData.cs

+        /// <summary>
+        /// Returns true if this camera applies 8-bit dithering to the final render to reduce color banding
+        /// </summary>


Extra documentation that bothered me when looking at the code.

ellioman · 2021-04-20T14:12:02Z

com.unity.render-pipelines.universal/Runtime/UniversalAdditionalCameraData.cs

+        /// <summary>
+        /// Returns the selected scene-layers affecting this camera.
+        /// </summary>


Extra documentation that bothered me when looking at the code.

It's a bit vague IMO. In HDRP this field is documented like this:

Layer mask used to select which volumes will influence this camera.

@oleks-k I like the suggestion here. What are your thoughts?

I would suggest this description (does it return a list?):

Returns the list of Layers selected in the Layer property of this Camera.

Is it technically correct?

ellioman · 2021-04-20T14:12:07Z

com.unity.render-pipelines.universal/Runtime/UniversalAdditionalCameraData.cs

+        /// <summary>
+        /// Returns the transform that will act as a trigger for volume blending.
+        /// </summary>


Extra documentation that bothered me when looking at the code.

erikabar · 2021-04-21T08:45:19Z

@Unity-Technologies/mobile-qa could you test if this PR does not regress anything on mobile side?

svens-unity

This is looking good :) Main question I have is how we expect users to update volumes via scripting - what is the API point for this. At first thought, I would expect a call on Camera or similar to update the volume for that camera that instant. Furthermore, another call to update all volumes stacks at once.

com.unity.render-pipelines.universal/Runtime/UniversalAdditionalCameraData.cs

svens-unity · 2021-04-21T08:40:21Z

com.unity.render-pipelines.universal/Runtime/UniversalAdditionalCameraData.cs

+        /// Returns the current volume stack used by this camera
+        /// </summary>
+        VolumeStack m_VolumeStack = null;
+        public VolumeStack volumeStack


At the moment this is the main location where we expect users to interact with when using ViaScripting?

(As you know but replying in case other reviewers are thinking the same)
We now have functions on the Camera to update the volume framework.
If users would like to change the stack used by this camera, then they have the possibility here to replace the volumeStack with their custom one.

com.unity.render-pipelines.universal/Runtime/UniversalRenderPipelineCore.cs

com.unity.render-pipelines.core/Runtime/Volume/VolumeManager.cs

com.unity.render-pipelines.universal/Runtime/UniversalAdditionalCameraData.cs

com.unity.render-pipelines.universal/Runtime/UniversalRenderPipeline.cs

Verasl

LGTM

oleks-k

Looks good!

erikabar

No issues found. Tested Scripting API and Settings in the inspector with one camera, multiple cameras, camera stacking.
Test doc: https://docs.google.com/document/d/1oMyqz56wyz0r2PZtnZnn1Zkp53GggA-nKg1kJon9FMI/edit#

aleksandrasdzikia

Looks good on mobile side and no issues found. Used Erika's provided project to test different scenarios. Regarding performance, as discussed with Elvar, this will be done later due to lack of time, but performance should only be improved due to given option for user to disable or control the volume frame work updates. Other than that, looks good.

# Conflicts: # com.unity.render-pipelines.universal/Editor/Camera/UniversalRenderPipelineCameraEditor.cs

alex-vazquez

Please change the DrawPopUp

alex-vazquez · 2021-05-07T10:58:01Z

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


            // Dropdown menu options
            public static string[] mainLightOptions = { "Disabled", "Per Pixel" };
+            public static string[] volumeFrameworkUpdateOptions = { "Every Frame", "Via Scripting" };


You can use this approach: https://unity.slack.com/archives/C20E70CAC/p1615891650014300

do in a next PR

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

…pdates (#5594) * Backport of #4242 * Changelog * Fixing an issue where changing Camera ViaScripting to URP Asset ViaScripting, would update the volumeStack

Initial Commit

e8e2f0c

ellioman added the universal label Apr 20, 2021

ellioman requested a review from Verasl April 20, 2021 13:14

github-actions bot added SRP testing labels Apr 20, 2021

ellioman requested review from a team and oleks-k April 20, 2021 13:17

ellioman removed the testing label Apr 20, 2021

ellioman added 5 commits April 20, 2021 15:30

Renaming an enum and variable

937d9d5

More renaming

9bb76c7

More renaming

e127b22

More renaming

a66704e

Renaming m_DefaultStack to s_DefaultStack

5b44c15

ellioman commented Apr 20, 2021

View reviewed changes

ellioman added 2 commits April 20, 2021 16:14

Docs, cleanup and renaming

295943f

Docs + Function name change

2310e14

ellioman requested review from a team and svens-unity April 20, 2021 14:29

ellioman added 3 commits April 21, 2021 08:36

Scene data changes

072e406

Minor doc change

f3bfa21

Merge branch 'master' into universal/volumes/control-updates

e5ff82f

erikabar requested review from a team and erikabar April 21, 2021 08:41

svens-unity reviewed Apr 21, 2021

View reviewed changes

aleksandrasdzikia requested review from aleksandrasdzikia and removed request for a team April 22, 2021 07:51

TheoWong-pixel approved these changes May 3, 2021

View reviewed changes

Verasl approved these changes May 4, 2021

View reviewed changes

jenniferd-unity approved these changes May 4, 2021

View reviewed changes

kroatoa approved these changes May 4, 2021

View reviewed changes

oleks-k added 2 commits May 5, 2021 13:16

Edited the tooltips.

e652804

Edited the tooltip.

d5d126b

oleks-k approved these changes May 5, 2021

View reviewed changes

erikabar approved these changes May 6, 2021

View reviewed changes

Minor API doc edition.

ad171c0

aleksandrasdzikia approved these changes May 7, 2021

View reviewed changes

ellioman added 2 commits May 7, 2021 11:45

Merge branch 'master' into universal/volumes/control-updates

72f9afa

# Conflicts: # com.unity.render-pipelines.universal/Editor/Camera/UniversalRenderPipelineCameraEditor.cs

Changelog

34fcdee

ellioman requested a review from alex-vazquez May 7, 2021 10:04

alex-vazquez suggested changes May 7, 2021

View reviewed changes

Minor changes from PR Review

1709ddc

alex-vazquez approved these changes May 7, 2021

View reviewed changes

phi-lira marked this pull request as ready for review May 7, 2021 12:17

phi-lira requested a review from a team as a code owner May 7, 2021 12:17

phi-lira approved these changes May 7, 2021

View reviewed changes

github-actions bot added the testing label May 7, 2021

phi-lira merged commit 6865b10 into master May 7, 2021

phi-lira deleted the universal/volumes/control-updates branch May 7, 2021 12:17

ellioman mentioned this pull request May 28, 2021

[2022.1] Adding test scenes for the Volume Update Feature #4708

Closed

ellioman mentioned this pull request Sep 9, 2021

[2022.1] Fix for Volume Updates #5591

Merged

ellioman added a commit that referenced this pull request Sep 9, 2021

Backport of #4242

7ce89d0

ellioman mentioned this pull request Sep 9, 2021

[2021.1 Backport] Control Volume Framework updates + Fix for Volume Updates #5593

Merged

ellioman added a commit that referenced this pull request Sep 9, 2021

Backport of #4242

b66a017

ellioman mentioned this pull request Sep 9, 2021

[2020.3 Backport] Control Volume Framework updates + Fix for Volume Updates #5594

Merged

ellioman added a commit that referenced this pull request Sep 13, 2021

Backport of #4242 (#5593)

14c1eed

[2021.2] Control Volume Framework updates #4242

[2021.2] Control Volume Framework updates #4242

Uh oh!

Conversation

ellioman commented Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Things done in this PR

Editor changes

Scripting changes

Public

Internal

Other changes

UI Changes

Camera Inspector

Before

After

URP Asset

Before

After

Testing

What has been tested

What needs testing

Later work

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!

erikabar commented Apr 21, 2021

Uh oh!

svens-unity left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Verasl left a comment

Choose a reason for hiding this comment

Uh oh!

oleks-k left a comment

Choose a reason for hiding this comment

Uh oh!

erikabar left a comment

Choose a reason for hiding this comment

Uh oh!

aleksandrasdzikia left a comment

Choose a reason for hiding this comment

Uh oh!

alex-vazquez 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!

Uh oh!

Uh oh!

ellioman commented Apr 20, 2021 •

edited

Loading