Fix ambient hemisphere lighting mixing up diffuse & specular (#444)

Fixes #444

Components/Hlms/Pbs/include/OgreHlmsPbs.h

@@ -92,14 +92,26 @@ namespace Ogre
             /// Use fixed-colour ambient lighting when upper hemisphere = lower hemisphere,
             /// use hemisphere lighting when they don't match.
             /// Disables ambient lighting if the colours are black.
-            AmbientAuto,
+            AmbientAutoNormal,
+
+            /// Same as AmbientAutoNormal but inverts specular & diffuse calculations.
+            /// See AmbientHemisphereInverted.
+            ///
+            /// This version was called "AmbientAuto" in OgreNext <= 2.3
+            AmbientAutoInverted,
 
             /// Force fixed-colour ambient light. Only uses the upper hemisphere paramter.
             AmbientFixed,
 
             /// Force hemisphere ambient light. Useful if you plan on adjusting the colours
             /// dynamically very often and this might cause swapping shaders.
-            AmbientHemisphere,
+            AmbientHemisphereNormal,
+
+            /// Same as AmbientHemisphereNormal, but inverts the specular & diffuse calculations
+            /// which can be visually more pleasant, or you just need it to look how it was in 2.3.
+            ///
+            /// This version was called "AmbientHemisphere" in OgreNext <= 2.3
+            AmbientHemisphereInverted,
 
             /// Uses spherical harmonics
             AmbientSh,
@@ -630,6 +642,7 @@ namespace Ogre
         static const IdString ExponentialShadowMaps;
 
         static const IdString AmbientHemisphere;
+        static const IdString AmbientHemisphereInverted;
         static const IdString AmbientSh;
         static const IdString AmbientShMonochrome;
         static const IdString LightProfilesTexture;

Components/Hlms/Pbs/src/OgreHlmsPbs.cpp

@@ -180,6 +180,7 @@ namespace Ogre
     const IdString PbsProperty::LtcTextureAvailable = IdString( "ltc_texture_available" );
     const IdString PbsProperty::AmbientFixed = IdString( "ambient_fixed" );
     const IdString PbsProperty::AmbientHemisphere = IdString( "ambient_hemisphere" );
+    const IdString PbsProperty::AmbientHemisphereInverted = IdString( "ambient_hemisphere_inverted" );
     const IdString PbsProperty::AmbientSh = IdString( "ambient_sh" );
     const IdString PbsProperty::AmbientShMonochrome = IdString( "ambient_sh_monochrome" );
     const IdString PbsProperty::TargetEnvprobeMap = IdString( "target_envprobe_map" );
@@ -330,7 +331,7 @@ namespace Ogre
         mDefaultBrdfWithDiffuseFresnel( false ),
         mShadowFilter( PCF_3x3 ),
         mEsmK( 600u ),
-        mAmbientLightMode( AmbientAuto )
+        mAmbientLightMode( AmbientAutoNormal )
     {
         memset( mDecalsTextures, 0, sizeof( mDecalsTextures ) );
 
@@ -1581,7 +1582,7 @@ namespace Ogre
             if( mLtcMatrixTexture )
                 setProperty( PbsProperty::LtcTextureAvailable, 1 );
 
-            if( mAmbientLightMode == AmbientAuto )
+            if( mAmbientLightMode == AmbientAutoNormal || mAmbientLightMode == AmbientAutoInverted )
             {
                 if( upperHemisphere == lowerHemisphere )
                 {
@@ -1592,14 +1593,21 @@ namespace Ogre
                 }
                 else
                 {
-                    ambientMode = AmbientHemisphere;
+                    if( mAmbientLightMode == AmbientAutoInverted )
+                        ambientMode = AmbientHemisphereNormal;
+                    else
+                        ambientMode = AmbientHemisphereInverted;
                 }
             }
 
             if( ambientMode == AmbientFixed )
                 setProperty( PbsProperty::AmbientFixed, 1 );
-            if( ambientMode == AmbientHemisphere )
+            if( ambientMode == AmbientHemisphereNormal || ambientMode == AmbientHemisphereInverted )
+            {
                 setProperty( PbsProperty::AmbientHemisphere, 1 );
+                if( ambientMode == AmbientHemisphereInverted )
+                    setProperty( PbsProperty::AmbientHemisphereInverted, 1 );
+            }
             if( ambientMode == AmbientSh || ambientMode == AmbientShMonochrome )
             {
                 setProperty( PbsProperty::AmbientSh, 1 );
@@ -1845,14 +1853,15 @@ namespace Ogre
                 mapSize += 4u * 4u;
 
             // vec3 ambientUpperHemi + float envMapScale
-            if( ambientMode == AmbientFixed || ambientMode == AmbientHemisphere || envMapScale != 1.0f ||
-                vctNeedsAmbientHemi )
+            if( ( ambientMode >= AmbientFixed && ambientMode <= AmbientHemisphereInverted ) ||
+                envMapScale != 1.0f || vctNeedsAmbientHemi )
             {
                 mapSize += 4 * 4;
             }
 
             // vec3 ambientLowerHemi + padding + vec3 ambientHemisphereDir + padding
-            if( ambientMode == AmbientHemisphere || vctNeedsAmbientHemi )
+            if( ambientMode == AmbientHemisphereNormal || ambientMode == AmbientHemisphereInverted ||
+                vctNeedsAmbientHemi )
             {
                 mapSize += 8 * 4;
             }
@@ -2243,8 +2252,8 @@ namespace Ogre
             }
 
             // vec3 ambientUpperHemi + padding
-            if( ambientMode == AmbientFixed || ambientMode == AmbientHemisphere || envMapScale != 1.0f ||
-                vctNeedsAmbientHemi )
+            if( ( ambientMode >= AmbientFixed && ambientMode <= AmbientHemisphereInverted ) ||
+                envMapScale != 1.0f || vctNeedsAmbientHemi )
             {
                 *passBufferPtr++ = static_cast<float>( upperHemisphere.r );
                 *passBufferPtr++ = static_cast<float>( upperHemisphere.g );
@@ -2253,7 +2262,8 @@ namespace Ogre
             }
 
             // vec3 ambientLowerHemi + padding + vec3 ambientHemisphereDir + padding
-            if( ambientMode == AmbientHemisphere || vctNeedsAmbientHemi )
+            if( ambientMode == AmbientHemisphereNormal || ambientMode == AmbientHemisphereInverted ||
+                vctNeedsAmbientHemi )
             {
                 *passBufferPtr++ = static_cast<float>( lowerHemisphere.r );
                 *passBufferPtr++ = static_cast<float>( lowerHemisphere.g );

Docs/src/manual/GiMethods.md

@@ -18,7 +18,7 @@ This is just a solid colour applied uniformly to the entire scene. Very basic
 
 ## Hemisphere {#GiAmbientLightingHemisphere}
 
-Use `SceneManager::setAmbientLight` and set `upperHemisphere` and `lowerHemisphere` to the different values and set `HlmsPbs::setAmbientLightMode` to either `AmbientAuto` or `AmbientHemisphere`.
+Use `SceneManager::setAmbientLight` and set `upperHemisphere` and `lowerHemisphere` to the different values and set `HlmsPbs::setAmbientLightMode` to either `AmbientAuto*` or `AmbientHemisphere*`.
 
 Hemisphere lighting is supposed to be set to the colour of the sky or sun in upperHemisphere, and the colour of the ground in lower hemisphere to mimic a single bounce coming from the ground.
 
@@ -313,4 +313,4 @@ If you want something accurate, pretty and runs reasonably fast with reasonably
 If you need the best possible quality or require to change lighting at runtime, then VCT (or IFD + VCT) is the best solution.
 
 Please note that many of these techniques can be combined together (e.g. PCC and per-pixel PCC usually can be paired with anything) but they won't necessarily end up in pleasant-looking results,
-and certain combinations may cause shaders to compile (it could be an Ogre bug so if you find it please report it [in the forums](https://forums.ogre3d.org/viewforum.php?f=25) or in [Github](https://github.com/OGRECave/ogre-next/issues))
+and certain combinations may cause shaders to compile (it could be an Ogre bug so if you find it please report it [in the forums](https://forums.ogre3d.org/viewforum.php?f=25) or in [Github](https://github.com/OGRECave/ogre-next/issues))

Docs/src/manual/PbsChanges30.md

@@ -166,3 +166,48 @@ The [second change](https://github.com/OGRECave/ogre-next/commit/4859a0a6cc622a1
 
 **With Schlick Fresnel using F0:**
 ![](PbsChanges30/OgreIblSpecularFresnelF0.png)
+
+
+# Hemisphere Ambient Lighting changes
+
+If you were using one of the following settings:
+
+ - Ogre::SceneManager::setAmbientLight() (mandatory)
+ - Ogre::HlmsPbs::setAmbientLightMode( Ogre::HlmsPbs::AmbientAuto ) (the default setting)
+ - Ogre::HlmsPbs::setAmbientLightMode( Ogre::HlmsPbs::AmbientHemisphere )
+
+Then you are affected.
+
+[Hemisphere Ambient Lighting](Ogre::HlmsPbs::AmbientLightMode::AmbientHemisphereNormal) is a cheap trick.
+
+Therefore there is no true "right way" of doing it.
+
+Nonetheless user Th3V1kt0r [spotted we had swapped diffuse & specular calculations](https://github.com/OGRECave/ogre-next/pull/444).
+
+In some cases the difference may be negligible, but depending on how strong your ambient lighting is (controlled via Ogre::SceneManager::setAmbientLight), this can result in visible changes:
+
+![](PbsChanges30/AmbientHemisphereLighting.png)
+
+Due to how considerable the differences are, and how visually pleasant the old method was; **we are keeping both models but default to the new one**.
+
+| Old Value (<= 2.3)| New Value (>= 3.0)        | Description                                                                                                     |
+|-------------------|---------------------------|-----------------------------------------------------------------------------------------------------------------|
+| #N / A            | AmbientAutoNormal         | Did not exist. Default                                                                                          |
+| AmbientAuto       | AmbientAutoInverted       | Inverts diffuse & specular calculations. This was the default value in OgreNext 2.3                             |
+| #N / A            | AmbientHemisphereNormal   | Did not exist.                                                                                                  |
+| AmbientHemisphere | AmbientHemisphereInverted | Inverts diffuse & specular calculations. This is how hemisphere ambient lighting was calculated in OgreNext 2.3 |
+
+Thus if you wish ambient lighting to look how it was before, simply call:
+
+```cpp
+hlmsPbs->setAmbientLightMode( Ogre::HlmsPbs::AmbientAutoInverted );
+```
+
+The most simple explanation of why the old method may be more pleasant is that ambient hemisphere lighting
+is a very cheap trick to simulate Global Illumination.
+
+Given how simple it was (it's just the blending of two colours across two hemispheres), this mistake made
+lighting look more rich and dynamic, even if wrong.
+
+Thus the correct calculations look more dull unless the data is sourced from something more accurate like
+a cubemap, spherical harmonics, or true path/raytracing.

Samples/Media/Hlms/Pbs/Any/AmbientLighting_piece_ps.any

@@ -72,15 +72,22 @@
 	@end
 
 	@property( ambient_hemisphere )
+		@property( ambient_hemisphere_inverted )
+			float tmpAmbientWS = ambientWD;
+			float tmpAmbientWD = ambientWS;
+		@else
+			float tmpAmbientWS = ambientWS;
+			float tmpAmbientWD = ambientWD;
+		@end
 		@property( vct_num_probes )
 			//Only use ambient lighting if object is outside any VCT probe
 			if( vctSpecular.w == 0 )
 			{
 		@end
 				pixelData.envColourS += lerp( midf3_c( passBuf.ambientLowerHemi.xyz ),
-											  midf3_c( passBuf.ambientUpperHemi.xyz ), ambientWD );
+											  midf3_c( passBuf.ambientUpperHemi.xyz ), tmpAmbientWS );
 				pixelData.envColourD += lerp( midf3_c( passBuf.ambientLowerHemi.xyz ),
-											  midf3_c( passBuf.ambientUpperHemi.xyz ), ambientWS );
+											  midf3_c( passBuf.ambientUpperHemi.xyz ), tmpAmbientWD );
 		@property( vct_num_probes )
 			}
 		@end

Samples/Media/Hlms/Terra/Any/800.PixelShader_piece_ps.any

@@ -367,24 +367,36 @@
 		@insertpiece( DoPlanarReflectionsPS )
 
 		@property( ambient_hemisphere )
+			@property( ambient_hemisphere_inverted )
+				float tmpAmbientWS = ambientWD;
+				float tmpAmbientWD = ambientWS;
+			@else
+				float tmpAmbientWS = ambientWS;
+				float tmpAmbientWD = ambientWD;
+			@end
+
 			@property( use_envprobe_map || hlms_use_ssr || use_planar_reflections || vct_num_probes )
 				@property( vct_num_probes )
 					//Only use ambient lighting if object is outside any VCT probe
 					if( vctSpecular.w == 0 )
 					{
 				@end
 						pixelData.envColourS += lerp( midf3_c( passBuf.ambientLowerHemi.xyz ),
-													  midf3_c( passBuf.ambientUpperHemi.xyz ), ambientWD );
+													  midf3_c( passBuf.ambientUpperHemi.xyz ),
+													  tmpAmbientWS );
 						pixelData.envColourD += lerp( midf3_c( passBuf.ambientLowerHemi.xyz ),
-													  midf3_c( passBuf.ambientUpperHemi.xyz ), ambientWS );
+													  midf3_c( passBuf.ambientUpperHemi.xyz ),
+													  tmpAmbientWD );
 				@property( vct_num_probes )
 					}
 				@end
 			@else
 				pixelData.envColourS = lerp( midf3_c( passBuf.ambientLowerHemi.xyz ),
-											 midf3_c( passBuf.ambientUpperHemi.xyz ), ambientWD );
+											 midf3_c( passBuf.ambientUpperHemi.xyz ),
+											 tmpAmbientWS );
 				pixelData.envColourD = lerp( midf3_c( passBuf.ambientLowerHemi.xyz ),
-											 midf3_c( passBuf.ambientUpperHemi.xyz ), ambientWS );
+											 midf3_c( passBuf.ambientUpperHemi.xyz ),
+											 tmpAmbientWD );
 			@end
 		@end
 		@property( ambient_fixed && vct_num_probes )