From e89f3f74d73dd77f483039612afd2fa1518c6868 Mon Sep 17 00:00:00 2001 From: FrancescoC-Unity Date: Tue, 2 Feb 2021 10:05:32 +0100 Subject: [PATCH 1/4] Set ambient probe to black when there is a big difference --- .../Runtime/Sky/HDRISky/HDRISky.cs | 18 ++++++++ .../Runtime/Sky/SkyManager.cs | 5 +++ .../Runtime/Sky/SkyRenderer.cs | 19 +-------- .../Runtime/Sky/SkySettings.cs | 42 +++++++++++++++++++ .../Runtime/Sky/SkyUpdateContext.cs | 7 ++++ 5 files changed, 73 insertions(+), 18 deletions(-) diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs index 049d153968e..4c1bc719fe1 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs @@ -169,6 +169,24 @@ public override int GetHashCode() return hash; } + /// + /// Determines if the SkySettings is very different 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. + /// + /// The settings to compare with. + /// Whether the settings are deemed very different. + public virtual bool IsVeryDifferentFrom(SkySettings otherSettings) + { + if (otherSettings.GetSkyRendererType() != GetSkyRendererType()) + return true; + + HDRISky otherHdriSkySettings = otherSettings as HDRISky; + + return base.IsVeryDifferentFrom(otherSettings) || hdriSky.value != otherHdriSkySettings.hdriSky.value; + } + /// /// Returns HDRISkyRenderer type. /// diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyManager.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyManager.cs index a4fb5ab9d55..a01f822f74f 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyManager.cs +++ b/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; } diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyRenderer.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyRenderer.cs index 2e7d3fb760a..203ea852c1b 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyRenderer.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyRenderer.cs @@ -70,24 +70,7 @@ public virtual void PreRenderSky(BuiltinSkyParameters builtinParams) {} /// Returns SkySetting exposure. 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(); } /// diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs index bd2980b3ba7..4556ca7f54c 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs @@ -217,6 +217,48 @@ public static int GetUniqueID(Type type) return uniqueID; } + /// + /// Returns the sky intensity as determined by this SkySetting. + /// + /// The sky intensity. + 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; + } + + /// + /// Determines if the SkySettings is very different 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. + /// + /// The settings to compare with. + /// Whether the settings are deemed very different. + public virtual bool IsVeryDifferentFrom(SkySettings otherSettings) + { + 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; + } + /// /// Returns the class type of the SkyRenderer associated with this Sky Settings. /// diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs index a8b57c6c2f9..0433143db0c 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs +++ b/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.IsVeryDifferentFrom(value); + if (m_SkySettings == value) return; From 6392a2973e322f586cba995fd57eff3ca130b680 Mon Sep 17 00:00:00 2001 From: FrancescoC-Unity Date: Tue, 2 Feb 2021 10:07:55 +0100 Subject: [PATCH 2/4] Changelog --- com.unity.render-pipelines.high-definition/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/com.unity.render-pipelines.high-definition/CHANGELOG.md b/com.unity.render-pipelines.high-definition/CHANGELOG.md index 91977245c80..015041311f2 100644 --- a/com.unity.render-pipelines.high-definition/CHANGELOG.md +++ b/com.unity.render-pipelines.high-definition/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed - Fixed an exception when opening the color picker in the material UI (case 1307143). +- Fix screen being over-exposed when changing very different skies. ### Changed - Removed the material pass probe volumes evaluation mode. From 44add963ae7d527df5f8fbaf60de52a939640e25 Mon Sep 17 00:00:00 2001 From: FrancescoC-Unity Date: Tue, 2 Feb 2021 10:28:40 +0100 Subject: [PATCH 3/4] Fix a couple of things --- .../Runtime/Sky/HDRISky/HDRISky.cs | 5 +---- .../Runtime/Sky/SkySettings.cs | 3 +++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs index 4c1bc719fe1..ba489b49f45 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs @@ -177,11 +177,8 @@ public override int GetHashCode() /// /// The settings to compare with. /// Whether the settings are deemed very different. - public virtual bool IsVeryDifferentFrom(SkySettings otherSettings) + public override bool IsVeryDifferentFrom(SkySettings otherSettings) { - if (otherSettings.GetSkyRendererType() != GetSkyRendererType()) - return true; - HDRISky otherHdriSkySettings = otherSettings as HDRISky; return base.IsVeryDifferentFrom(otherSettings) || hdriSky.value != otherHdriSkySettings.hdriSky.value; diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs index 4556ca7f54c..a34b4de433e 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs @@ -250,6 +250,9 @@ public float GetIntensityFromSettings() /// Whether the settings are deemed very different. public virtual bool IsVeryDifferentFrom(SkySettings otherSettings) { + if (otherSettings.GetSkyRendererType() != GetSkyRendererType()) + return true; + float thisIntensity = GetIntensityFromSettings(); float otherIntensity = otherSettings.GetIntensityFromSettings(); From eb0503cdb497505d3d6ac5111bde5c48e9d8ad22 Mon Sep 17 00:00:00 2001 From: FrancescoC-Unity Date: Tue, 2 Feb 2021 13:59:19 +0100 Subject: [PATCH 4/4] Change name --- .../Runtime/Sky/HDRISky/HDRISky.cs | 6 +++--- .../Runtime/Sky/SkySettings.cs | 4 ++-- .../Runtime/Sky/SkyUpdateContext.cs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs index ba489b49f45..675e2dc6d82 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/HDRISky/HDRISky.cs @@ -170,18 +170,18 @@ public override int GetHashCode() } /// - /// Determines if the SkySettings is very different from another. This is going to be used to determine whether + /// 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. /// /// The settings to compare with. /// Whether the settings are deemed very different. - public override bool IsVeryDifferentFrom(SkySettings otherSettings) + public override bool SignificantlyDivergesFrom(SkySettings otherSettings) { HDRISky otherHdriSkySettings = otherSettings as HDRISky; - return base.IsVeryDifferentFrom(otherSettings) || hdriSky.value != otherHdriSkySettings.hdriSky.value; + return base.SignificantlyDivergesFrom(otherSettings) || hdriSky.value != otherHdriSkySettings.hdriSky.value; } /// diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs index a34b4de433e..7cc23422ac1 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkySettings.cs @@ -242,13 +242,13 @@ public float GetIntensityFromSettings() } /// - /// Determines if the SkySettings is very different from another. This is going to be used to determine whether + /// 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. /// /// The settings to compare with. /// Whether the settings are deemed very different. - public virtual bool IsVeryDifferentFrom(SkySettings otherSettings) + public virtual bool SignificantlyDivergesFrom(SkySettings otherSettings) { if (otherSettings.GetSkyRendererType() != GetSkyRendererType()) return true; diff --git a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs index 0433143db0c..ab87d228197 100644 --- a/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs +++ b/com.unity.render-pipelines.high-definition/Runtime/Sky/SkyUpdateContext.cs @@ -33,7 +33,7 @@ public SkySettings skySettings if (m_SkySettings == null) settingsHadBigDifferenceWithPrev = true; else - settingsHadBigDifferenceWithPrev = m_SkySettings.IsVeryDifferentFrom(value); + settingsHadBigDifferenceWithPrev = m_SkySettings.SignificantlyDivergesFrom(value); if (m_SkySettings == value) return;