[HDRP]Improvements and fixes on Volumes Components Inspectors

- https://fogbugz.unity3d.com/f/cases/1297795/
- https://fogbugz.unity3d.com/f/cases/1303477/
- https://fogbugz.unity3d.com/f/cases/1297789/

Author: alex-vazquez-unity3d

com.unity.render-pipelines.core/CHANGELOG.md

@@ -16,7 +16,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Fixed
 - Fixed the default background color for previews to use the original color.
-- Fixed ACES tonemapping for Nintendo Switch by forcing some shader color conversion functions to full float precision.
+- Fixed spacing between property fields on the Volume Component Editors.
+- Fixed ALL/NONE to maintain the state on the Volume Component Editors.
+- Fixed the selection of the Additional properties from ALL/NONE when the option "Show additional properties" is disabled
+- Fixed ACES tonemaping for Nintendo Switch by forcing some shader color conversion functions to full float precision.
 - Fixed a bug in FreeCamera which would only provide a speed boost for the first frame when pressing the Shfit key.
 
 ### Added

com.unity.render-pipelines.core/Editor/Volume/VolumeComponentEditor.cs

@@ -75,6 +75,26 @@ public VolumeComponentEditorAttribute(Type componentType)
     /// <seealso cref="VolumeComponentEditorAttribute"/>
     public class VolumeComponentEditor
     {
+        class Styles
+        {
+            public static GUIContent overrideSettingText { get; } = EditorGUIUtility.TrTextContent("", "Override this setting for this volume.");
+            public static GUIContent allText { get; } = EditorGUIUtility.TrTextContent("ALL", "Toggle all overrides on. To maximize performances you should only toggle overrides that you actually need.");
+            public static GUIContent noneText { get; } = EditorGUIUtility.TrTextContent("NONE", "Toggle all overrides off.");
+
+            public static string toggleAllText { get; } = L10n.Tr("Toggle All");
+        }
+
+        Vector2? m_OverrideToggleSize;
+        internal Vector2 overrideToggleSize
+        {
+            get
+            {
+                if (!m_OverrideToggleSize.HasValue)
+                    m_OverrideToggleSize = CoreEditorStyles.smallTickbox.CalcSize(Styles.overrideSettingText);
+                return m_OverrideToggleSize.Value;
+            }
+        }
+
         /// <summary>
         /// Specifies the <see cref="VolumeComponent"/> this editor is drawing.
         /// </summary>
@@ -99,10 +119,11 @@ public class VolumeComponentEditor
 
         SerializedProperty m_AdvancedMode;
 
+        List<VolumeParameter> m_VolumeNotAdditionalParameters;
         /// <summary>
         /// Override this property if your editor makes use of the "More Options" feature.
         /// </summary>
-        public virtual bool hasAdvancedMode => false;
+        public virtual bool hasAdvancedMode => target.parameters.Count != m_VolumeNotAdditionalParameters.Count;
 
         /// <summary>
         /// Checks if the editor currently has the "More Options" feature toggled on.
@@ -176,9 +197,18 @@ internal void Init(VolumeComponent target, Editor inspector)
             serializedObject = new SerializedObject(target);
             activeProperty = serializedObject.FindProperty("active");
             m_AdvancedMode = serializedObject.FindProperty("m_AdvancedMode");
+
+            InitParameters();
+
             OnEnable();
         }
 
+        void InitParameters()
+        {
+            m_VolumeNotAdditionalParameters = new List<VolumeParameter>();
+            VolumeComponent.FindParameters(target, m_VolumeNotAdditionalParameters, field => field.GetCustomAttribute<AdditionalPropertyAttribute>() == null);
+        }
+
         void GetFields(object o, List<(FieldInfo, SerializedProperty)> infos, SerializedProperty prop = null)
         {
             if (o == null)
@@ -281,21 +311,68 @@ public virtual string GetDisplayTitle()
             return target.displayName == "" ? ObjectNames.NicifyVariableName(target.GetType().Name) : target.displayName;
         }
 
+        void AddToogleState(GUIContent content, bool state)
+        {
+            bool allOverridesSameState = AreOverridesTo(state);
+            if (GUILayout.Toggle(allOverridesSameState, content, CoreEditorStyles.miniLabelButton, GUILayout.ExpandWidth(false)) && !allOverridesSameState)
+                SetOverridesTo(state);
+        }
+
         void TopRowFields()
         {
             using (new EditorGUILayout.HorizontalScope())
             {
-                if (GUILayout.Button(EditorGUIUtility.TrTextContent("All", "Toggle all overrides on. To maximize performances you should only toggle overrides that you actually need."), CoreEditorStyles.miniLabelButton, GUILayout.Width(17f), GUILayout.ExpandWidth(false)))
-                    SetAllOverridesTo(true);
+                AddToogleState(Styles.allText, true);
+                AddToogleState(Styles.noneText, false);
+            }
+        }
+
+        /// <summary>
+        /// Checks if all the visible parameters have the given state
+        /// </summary>
+        /// <param name="state">The state to check</param>
+        internal bool AreOverridesTo(bool state)
+        {
+            if (hasAdvancedMode && isInAdvancedMode)
+                return AreAllOverridesTo(state);
+
+            for (int i = 0; i < m_VolumeNotAdditionalParameters.Count; ++i)
+            {
+                if (m_VolumeNotAdditionalParameters[i].overrideState != state)
+                    return false;
+            }
+            return true;
+        }
 
-                if (GUILayout.Button(EditorGUIUtility.TrTextContent("None", "Toggle all overrides off."), CoreEditorStyles.miniLabelButton, GUILayout.Width(32f), GUILayout.ExpandWidth(false)))
-                    SetAllOverridesTo(false);
+        /// <summary>
+        /// Sets the given state to all the visible parameters
+        /// </summary>
+        /// <param name="state">The state to check</param>
+        internal void SetOverridesTo(bool state)
+        {
+            if (hasAdvancedMode && isInAdvancedMode)
+                SetAllOverridesTo(state);
+            else
+            {
+                Undo.RecordObject(target, Styles.toggleAllText);
+                target.SetOverridesTo(m_VolumeNotAdditionalParameters, state);
+                serializedObject.Update();
             }
         }
 
+        internal bool AreAllOverridesTo(bool state)
+        {
+            for (int i = 0; i < target.parameters.Count; ++i)
+            {
+                if (target.parameters[i].overrideState != state)
+                    return false;
+            }
+            return true;
+        }
+
         internal void SetAllOverridesTo(bool state)
         {
-            Undo.RecordObject(target, "Toggle All");
+            Undo.RecordObject(target, Styles.toggleAllText);
             target.SetAllOverridesTo(state);
             serializedObject.Update();
         }
@@ -423,9 +500,14 @@ protected void PropertyField(SerializedDataParameter property, GUIContent title)
         /// <param name="property">The property to draw the override checkbox for</param>
         protected void DrawOverrideCheckbox(SerializedDataParameter property)
         {
-            var overrideRect = GUILayoutUtility.GetRect(17f, 17f, GUILayout.ExpandWidth(false));
-            overrideRect.yMin += 4f;
-            property.overrideState.boolValue = GUI.Toggle(overrideRect, property.overrideState.boolValue, EditorGUIUtility.TrTextContent("", "Override this setting for this volume."), CoreEditorStyles.smallTickbox);
+            // Create a rect with the width of the Toggle "ALL" and the height + vspacing of the property that is being overriden
+            float height = EditorGUI.GetPropertyHeight(property.value) + EditorGUIUtility.standardVerticalSpacing;
+            var overrideRect = GUILayoutUtility.GetRect(Styles.allText, CoreEditorStyles.miniLabelButton, GUILayout.Height(height), GUILayout.ExpandWidth(false));
+
+            // also center vertically the checkbox
+            overrideRect.yMin += height * 0.5f - overrideToggleSize.y * 0.5f;
+
+            property.overrideState.boolValue = GUI.Toggle(overrideRect, property.overrideState.boolValue, Styles.overrideSettingText, CoreEditorStyles.smallTickbox);
         }
     }
 }

com.unity.render-pipelines.core/Runtime/Common/CoreAttributes.cs

@@ -13,4 +13,12 @@ public class DisplayInfoAttribute : Attribute
         /// <summary>Display order used in UI.</summary>
         public int order;
     }
+
+    /// <summary>
+    /// Attribute used to customize UI display to allow properties only be visible when "Show Additional Properties" is selected
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Field)]
+    public class AdditionalPropertyAttribute : Attribute
+    {
+    }
 }

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

@@ -81,7 +81,10 @@ public class VolumeComponent : ScriptableObject
         /// <summary>
         /// Extracts all the <see cref="VolumeParameter"/>s defined in this class and nested classes.
         /// </summary>
-        static void GetParameters(object o, List<VolumeParameter> parameters)
+        /// <param name="o">The object to find the parameters</param>
+        /// <param name="parameters">The list filled with the parameters.</param>
+        /// <param name="filter">If you want to filter the parameters</param>
+        internal static void FindParameters(object o, List<VolumeParameter> parameters, Func<FieldInfo, bool> filter = null)
         {
             if (o == null)
                 return;
@@ -93,9 +96,12 @@ static void GetParameters(object o, List<VolumeParameter> parameters)
             foreach (var field in fields)
             {
                 if (field.FieldType.IsSubclassOf(typeof(VolumeParameter)))
-                    parameters.Add((VolumeParameter)field.GetValue(o));
+                {
+                    if (filter?.Invoke(field) ?? true)
+                        parameters.Add((VolumeParameter)field.GetValue(o));
+                }
                 else if (!field.FieldType.IsArray && field.FieldType.IsClass)
-                    GetParameters(field.GetValue(o), parameters);
+                    FindParameters(field.GetValue(o), parameters, filter);
             }
         }
 
@@ -109,7 +115,7 @@ protected virtual void OnEnable()
         {
             // Automatically grab all fields of type VolumeParameter for this instance
             var fields = new List<VolumeParameter>();
-            GetParameters(this, fields);
+            FindParameters(this, fields);
             parameters = fields.AsReadOnly();
 
             foreach (var parameter in parameters)
@@ -195,10 +201,14 @@ public virtual void Override(VolumeComponent state, float interpFactor)
         /// <param name="state">The value to set the state of the overrides to.</param>
         public void SetAllOverridesTo(bool state)
         {
-            SetAllOverridesTo(parameters, state);
+            SetOverridesTo(parameters, state);
         }
 
-        void SetAllOverridesTo(IEnumerable<VolumeParameter> enumerable, bool state)
+        /// <summary>
+        /// Sets the override state of the given parameters on this component to a given value.
+        /// </summary>
+        /// <param name="state">The value to set the state of the overrides to.</param>
+        internal void SetOverridesTo(IEnumerable<VolumeParameter> enumerable, bool state)
         {
             foreach (var prop in enumerable)
             {
@@ -213,7 +223,7 @@ void SetAllOverridesTo(IEnumerable<VolumeParameter> enumerable, bool state)
                             .GetValue(prop, null);
 
                     if (innerParams != null)
-                        SetAllOverridesTo(innerParams, state);
+                        SetOverridesTo(innerParams, state);
                 }
             }
         }

com.unity.render-pipelines.core/Tests/Editor/ReflectionUtils.cs

@@ -0,0 +1,79 @@
+using NUnit.Framework;
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Reflection;
+
+namespace UnityEngine.Rendering.Tests
+{
+    public static class ReflectionUtils
+    {
+        /// <summary>
+        /// Calls a private method from a class
+        /// </summary>
+        /// <param name="methodName">The method name</param>
+        /// <param name="args">The arguments to pass to the method</param>
+        public static object Invoke(this object target, string methodName, params object[] args)
+        {
+            Assert.True(target != null, "The target could not be null");
+            Assert.IsNotEmpty(methodName, "The field to set could not be null");
+
+            var mi = target.GetType().GetMethod(methodName, BindingFlags.NonPublic | BindingFlags.Instance);
+            Assert.True(mi != null, $"Could not find method `{methodName}` on object `{target}`");
+            return mi.Invoke(target, args);
+        }
+
+        private static FieldInfo FindField(this Type type, string fieldName)
+        {
+            FieldInfo fi = null;
+
+            while (type != null)
+            {
+                fi = type.GetField(fieldName, BindingFlags.Instance | BindingFlags.NonPublic);
+
+                if (fi != null) break;
+
+                type = type.BaseType;
+            }
+
+            Assert.True(fi != null, $"Could not find method `{fieldName}` on object `{type}`");
+
+            return fi;
+        }
+
+        /// <summary>
+        /// Sets a private field from a class
+        /// </summary>
+        /// <param name="fieldName">The field to change</param>
+        /// <param name="value">The new value</param>
+        public static void SetField(this object target, string fieldName, object value)
+        {
+            Assert.True(target != null, "The target could not be null");
+            Assert.IsNotEmpty(fieldName, "The field to set could not be null");
+            target.GetType().FindField(fieldName).SetValue(target, value);
+        }
+
+        /// <summary>
+        /// Gets the value of a private field from a class
+        /// </summary>
+        /// <param name="fieldName">The field to get</param>
+        public static object GetField(this object target, string fieldName)
+        {
+            Assert.True(target != null, "The target could not be null");
+            Assert.IsNotEmpty(fieldName, "The field to set could not be null");
+            return target.GetType().FindField(fieldName).GetValue(target);
+        }
+
+        /// <summary>
+        /// Gets all the fields from a class
+        /// </summary>
+        public static IEnumerable<FieldInfo> GetFields(this object target)
+        {
+            Assert.True(target != null, "The target could not be null");
+
+            return target.GetType()
+                .GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)
+                .OrderBy(t => t.MetadataToken);
+        }
+    }
+}