Reset ambient probe upon switching to very different skies (#3340)

* Set ambient probe to black when there is a big difference

* Changelog

* Fix a couple of things

* Change name

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

Author: FrancescoC-unity

com.unity.render-pipelines.high-definition/CHANGELOG.md

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fixed an exception when opening the color picker in the material UI (case 1307143).
 - Fixed lights shadow frustum near and far planes.
 - Fixed various issues with non-temporal SSAO and rendergraph.
+- Fix screen being over-exposed when changing very different skies.
 
 ### Changed
 - Removed the material pass probe volumes evaluation mode.

com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs

@@ -169,6 +169,21 @@ public override int GetHashCode()
             return hash;
         }
 
+        /// <summary>
+        /// Determines if the SkySettings is significantly divergent from another. This is going to be used to determine whether
+        /// to reset completely the ambient probe instead of using previous one when waiting for current data upon changes.
+        /// In addition to the checks done with the base function, this HDRISky override checks whether the cubemap parameter
+        /// has changed if both settings are HDRISky.
+        /// </summary>
+        /// <param name="otherSettings">The settings to compare with.</param>
+        /// <returns>Whether the settings are deemed very different.</returns>
+        public override bool SignificantlyDivergesFrom(SkySettings otherSettings)
+        {
+            HDRISky otherHdriSkySettings = otherSettings as HDRISky;
+
+            return base.SignificantlyDivergesFrom(otherSettings) || hdriSky.value != otherHdriSkySettings.hdriSky.value;
+        }
+
         /// <summary>
         /// Returns HDRISkyRenderer type.
         /// </summary>

com.unity.render-pipelines.high-definition/Runtime/Sky/SkyManager.cs

@@ -617,6 +617,11 @@ void AllocateNewRenderingContext(SkyUpdateContext skyContext, int slot, int newH
             if (context.renderingContext == null)
                 context.renderingContext = new SkyRenderingContext(m_Resolution, m_IBLFilterArray.Length, supportConvolution, previousAmbientProbe, name);
 
+            // If we detected a big difference with previous settings, then carrying over the previous ambient probe is probably going to lead to unexpected result.
+            // Instead we at least fallback to a neutral one until async readback has finished.
+            if (skyContext.settingsHadBigDifferenceWithPrev)
+                context.renderingContext.ClearAmbientProbe();
+
             skyContext.cachedSkyRenderingContextId = slot;
         }
 

com.unity.render-pipelines.high-definition/Runtime/Sky/SkyRenderer.cs

@@ -70,24 +70,7 @@ public virtual void PreRenderSky(BuiltinSkyParameters builtinParams) {}
         /// <returns>Returns SkySetting exposure.</returns>
         protected static float GetSkyIntensity(SkySettings skySettings, DebugDisplaySettings debugSettings)
         {
-            float skyIntensity = 1.0f;
-
-            switch (skySettings.skyIntensityMode.value)
-            {
-                case SkyIntensityMode.Exposure:
-                    // Note: Here we use EV100 of sky as a multiplier, so it is the opposite of when use with a Camera
-                    // because for sky/light, higher EV mean brighter, but for camera higher EV mean darker scene
-                    skyIntensity *= ColorUtils.ConvertEV100ToExposure(-skySettings.exposure.value);
-                    break;
-                case SkyIntensityMode.Multiplier:
-                    skyIntensity *= skySettings.multiplier.value;
-                    break;
-                case SkyIntensityMode.Lux:
-                    skyIntensity *= skySettings.desiredLuxValue.value / skySettings.upperHemisphereLuxValue.value;
-                    break;
-            }
-
-            return skyIntensity;
+            return skySettings.GetIntensityFromSettings();
         }
 
         /// <summary>

com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs

@@ -217,6 +217,51 @@ public static int GetUniqueID(Type type)
             return uniqueID;
         }
 
+        /// <summary>
+        /// Returns the sky intensity as determined by this SkySetting.
+        /// </summary>
+        /// <returns>The sky intensity.</returns>
+        public float GetIntensityFromSettings()
+        {
+            float skyIntensity = 1.0f;
+            switch (skyIntensityMode.value)
+            {
+                case SkyIntensityMode.Exposure:
+                    // Note: Here we use EV100 of sky as a multiplier, so it is the opposite of when use with a Camera
+                    // because for sky/light, higher EV mean brighter, but for camera higher EV mean darker scene
+                    skyIntensity *= ColorUtils.ConvertEV100ToExposure(-exposure.value);
+                    break;
+                case SkyIntensityMode.Multiplier:
+                    skyIntensity *= multiplier.value;
+                    break;
+                case SkyIntensityMode.Lux:
+                    skyIntensity *= desiredLuxValue.value / upperHemisphereLuxValue.value;
+                    break;
+            }
+            return skyIntensity;
+        }
+
+        /// <summary>
+        /// Determines if the SkySettings is significantly divergent from another. This is going to be used to determine whether
+        /// to reset completely the ambient probe instead of using previous one when waiting for current data upon changes.
+        /// Override this to have a per-sky specific heuristic.
+        /// </summary>
+        /// <param name="otherSettings">The settings to compare with.</param>
+        /// <returns>Whether the settings are deemed very different.</returns>
+        public virtual bool SignificantlyDivergesFrom(SkySettings otherSettings)
+        {
+            if (otherSettings.GetSkyRendererType() != GetSkyRendererType())
+                return true;
+
+            float thisIntensity = GetIntensityFromSettings();
+            float otherIntensity = otherSettings.GetIntensityFromSettings();
+
+            // This is an arbitrary difference threshold. This needs to be re-evaluated in case it is proven problematic
+            float intensityRatio = thisIntensity > otherIntensity ? (thisIntensity / otherIntensity) : (otherIntensity / thisIntensity);
+            const float ratioThreshold = 3.0f;
+            return intensityRatio > ratioThreshold;
+        }
+
         /// <summary>
         /// Returns the class type of the SkyRenderer associated with this Sky Settings.
         /// </summary>

com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs

@@ -14,6 +14,8 @@ internal class SkyUpdateContext
         public int          skyParametersHash = -1;
         public float        currentUpdateTime = 0.0f;
 
+        public bool        settingsHadBigDifferenceWithPrev { get; private set; }
+
         public SkySettings skySettings
         {
             get { return m_SkySettings; }
@@ -28,6 +30,11 @@ public SkySettings skySettings
                     skyRenderer = null;
                 }
 
+                if (m_SkySettings == null)
+                    settingsHadBigDifferenceWithPrev = true;
+                else
+                    settingsHadBigDifferenceWithPrev = m_SkySettings.SignificantlyDivergesFrom(value);
+
                 if (m_SkySettings == value)
                     return;