Fix sky rendering bugs

Assets/ScriptableRenderLoop/HDRenderPipeline/HDRenderPipeline.cs

             // TODO: This is the wrong way to handle resize/allocation. We can have several different camera here, mean that the loop on camera will allocate and deallocate
             // the below buffer which is bad. Best is to have a set of buffer for each camera that is persistent and reallocate resource if need
             // For now consider we have only one camera that go to this code, the main one.
-            m_SkyManager.Resize(); // TODO: Also a bad naming, here we just want to realloc texture if skyparameters change (usefull for lookdev)
+            m_SkyManager.Resize(camera.nearClipPlane, camera.farClipPlane); // TODO: Also a bad naming, here we just want to realloc texture if skyparameters change (usefull for lookdev)
 
             if (camera.pixelWidth != m_currentWidth || camera.pixelHeight != m_currentHeight || m_lightLoop.NeedResize())
             {

Assets/ScriptableRenderLoop/HDRenderPipeline/Sky/HDRISky/HDRISkyRenderer.cs

             return GetParameters(skyParameters).skyHDRI != null;
         }
 
+        public override void SetRenderTargets(BuiltinSkyParameters builtinParams)
+        {
+            if (builtinParams.depthBuffer == BuiltinSkyParameters.nullRT)
+            {
+                Utilities.SetRenderTarget(builtinParams.renderContext, builtinParams.colorBuffer);
+            }
+            else
+            {
+                Utilities.SetRenderTarget(builtinParams.renderContext, builtinParams.colorBuffer, builtinParams.depthBuffer);
+            }
+        }
+
         override public void RenderSky(BuiltinSkyParameters builtinParams, SkyParameters skyParameters, bool renderForCubemap)
         {
             HDRISkyParameters hdriSkyParams = GetParameters(skyParameters);

Assets/ScriptableRenderLoop/HDRenderPipeline/Sky/ProceduralSky/ProceduralSkyRenderer.cs

                    allParams.worldRayleighColorRamp != null;
         }
 
+        public override void SetRenderTargets(BuiltinSkyParameters builtinParams)
+        {
+            // We do not bind the depth buffer as a depth-stencil target since it is
+            // bound as a color texture which is then sampled from within the shader.
+            Utilities.SetRenderTarget(builtinParams.renderContext, builtinParams.colorBuffer);
+        }
+
         void SetKeywords(BuiltinSkyParameters builtinParams, ProceduralSkyParameters param)
         {
             // Ensure that all preprocessor symbols are initially undefined.
             */
 
             // Expected to be valid for the sky pass, and invalid for the cube map generation pass.
-            if (builtinParams.depthBuffer != BuiltinSkyParameters.invalidRTI)
+            if (builtinParams.depthBuffer != BuiltinSkyParameters.nullRT)
             {
                 m_ProceduralSkyMaterial.EnableKeyword("PERFORM_SKY_OCCLUSION_TEST");
             }
             }
         }
 
-        void SetUniforms(BuiltinSkyParameters builtinParams, ProceduralSkyParameters param)
+        void SetUniforms(BuiltinSkyParameters builtinParams, ProceduralSkyParameters param, ref MaterialPropertyBlock properties)
-            m_ProceduralSkyMaterial.SetTexture("_Cubemap", param.skyHDRI);
-            m_ProceduralSkyMaterial.SetVector("_SkyParam", new Vector4(param.exposure, param.multiplier, param.rotation, 0.0f));
-            m_ProceduralSkyMaterial.SetMatrix("_ViewProjMatrix", builtinParams.viewProjMatrix);
-            m_ProceduralSkyMaterial.SetMatrix("_InvViewProjMatrix", builtinParams.invViewProjMatrix);
-            m_ProceduralSkyMaterial.SetVector("_CameraPosWS", builtinParams.cameraPosWS);
-            m_ProceduralSkyMaterial.SetVector("_ScreenSize", builtinParams.screenSize);
+            properties.SetTexture("_Cubemap", param.skyHDRI);
+            properties.SetVector("_SkyParam", new Vector4(param.exposure, param.multiplier, param.rotation, 0.0f));
+
+            properties.SetMatrix("_InvViewProjMatrix", builtinParams.invViewProjMatrix);
+            properties.SetVector("_CameraPosWS", builtinParams.cameraPosWS);
+            properties.SetVector("_ScreenSize", builtinParams.screenSize);
 
             m_ProceduralSkyMaterial.SetInt("_AtmosphericsDebugMode", (int)param.debugMode);
 
+            var pixelRect = new Rect(0f, 0f, builtinParams.screenSize.x, builtinParams.screenSize.y);
+            var scale = 1.0f; //(float)(int)occlusionDownscale;
+            var depthTextureScaledTexelSize = new Vector4(scale / pixelRect.width,
+                                                          scale / pixelRect.height,
+                                                         -scale / pixelRect.width,
+                                                         -scale / pixelRect.height);
+            properties.SetVector("_DepthTextureScaledTexelSize", depthTextureScaledTexelSize);
+
             /*
             m_ProceduralSkyMaterial.SetFloat("_ShadowBias", useOcclusion ? occlusionBias : 1f);
             m_ProceduralSkyMaterial.SetFloat("_ShadowBiasIndirect", useOcclusion ? occlusionBiasIndirect : 1f);
             m_ProceduralSkyMaterial.SetVector("_OcclusionTexture_TexelSize", ???);
             */
-
-            var pixelRect = new Rect(0f, 0f, builtinParams.screenSize.x, builtinParams.screenSize.y);
-            var scale = 1.0f; //(float)(int)occlusionDownscale;
-            var depthTextureScaledTexelSize = new Vector4(scale / pixelRect.width,
-                                                          scale / pixelRect.height,
-                                                         -scale / pixelRect.width,
-                                                         -scale / pixelRect.height);
-            m_ProceduralSkyMaterial.SetVector("_DepthTextureScaledTexelSize", depthTextureScaledTexelSize);
 
             m_ProceduralSkyMaterial.SetFloat("_WorldScaleExponent", param.worldScaleExponent);
             m_ProceduralSkyMaterial.SetFloat("_WorldNormalDistanceRcp", 1f / param.worldNormalDistance);
         {
             ProceduralSkyParameters proceduralSkyParams = GetParameters(skyParameters);
 
+            MaterialPropertyBlock properties = new MaterialPropertyBlock();
+
-            SetUniforms(builtinParams, proceduralSkyParams);
+            SetUniforms(builtinParams, proceduralSkyParams, ref properties);
-            if (builtinParams.depthBuffer != BuiltinSkyParameters.invalidRTI)
+
+            // Since we use the material for rendering the sky both into the cubemap, and
+            // during the fullscreen pass, setting the 'PERFORM_SKY_OCCLUSION_TEST' keyword has no effect.
+            if (builtinParams.depthBuffer != BuiltinSkyParameters.nullRT)
-                cmd.SetGlobalFloat("_DisableSkyOcclusionTest", 0.0f);
+                properties.SetFloat("_DisableSkyOcclusionTest", 0.0f);
-                // For some reason, disabling the 'PERFORM_SKY_OCCLUSION_TEST' keyword has no effect.
-                cmd.SetGlobalFloat("_DisableSkyOcclusionTest", 1.0f);
+                properties.SetFloat("_DisableSkyOcclusionTest", 1.0f);
-            cmd.DrawMesh(builtinParams.skyMesh, Matrix4x4.identity, m_ProceduralSkyMaterial);
+
+            cmd.DrawMesh(builtinParams.skyMesh, Matrix4x4.identity, m_ProceduralSkyMaterial, 0, 0, properties);
             builtinParams.renderContext.ExecuteCommandBuffer(cmd);
             cmd.Dispose();
         }

Assets/ScriptableRenderLoop/HDRenderPipeline/Sky/ProceduralSky/Resources/SkyProcedural.shader

             float4 _CameraPosWS;
 
             float4x4 _InvViewProjMatrix;
-            float4x4 _ViewProjMatrix;
 
             float _DisableSkyOcclusionTest;
 
                 float3 rotDirY = float3(sinPhi, 0, cosPhi);
                 dir = float3(dot(rotDirX, dir), dir.y, dot(rotDirY, dir));
 
-               // input.positionCS is SV_Position
+                // input.positionCS is SV_Position
-                // If the sky box is too far away (depth set to 0), the resulting look is too foggy.
-                const float skyDepth = 0.002;
+                // An arbitrary value attempting to match the size of the sky mesh from the Blacksmith demo.
+                const float skyDepth = 0.00025;
 
                 #ifdef PERFORM_SKY_OCCLUSION_TEST
                     // Determine whether the sky is occluded by the scene geometry.
                     skyTexWeight = 1.0;
                 }
 
-                UpdatePositionInput(depthRaw, _InvViewProjMatrix, _ViewProjMatrix, posInput);
+                // Since we only need the world space position, so we don't pass the view-projection matrix.
+                UpdatePositionInput(depthRaw, _InvViewProjMatrix, k_identity4x4, posInput);
 
                 float4 c1, c2, c3;
                 VolundTransferScatter(posInput.positionWS, c1, c2, c3);
                 #endif
 
                 float3 skyColor = float3(0.0, 0.0, 0.0);
-                float  opacity  = extinction;
+                // Opacity should be proportional to extinction, but this produces wrong results.
+                // It appears what the algorithm computes is not actually extinction.
+                float  opacity  = (1.0 - extinction);
 
                 if (skyTexWeight == 1.0)
                 {

Assets/ScriptableRenderLoop/HDRenderPipeline/Sky/SkyManager.cs

 
     public class BuiltinSkyParameters
     {
-        public Matrix4x4                viewProjMatrix;
         public Matrix4x4                invViewProjMatrix;
         public Vector3                  cameraPosWS;
         public Vector4                  screenSize;
         public RenderTargetIdentifier   colorBuffer;
         public RenderTargetIdentifier   depthBuffer;
 
-        public static RenderTargetIdentifier invalidRTI = -1;
+        public static RenderTargetIdentifier nullRT = -1;
     }
 
     public class SkyManager
             m_CubemapScreenSize = new Vector4((float)resolution, (float)resolution, 1.0f / (float)resolution, 1.0f / (float)resolution);
         }
 
-        void RebuildSkyMeshes()
+        void RebuildSkyMeshes(float nearPlane, float farPlane)
-                Matrix4x4 cubeProj = Matrix4x4.Perspective(90.0f, 1.0f, 0.1f, 1.0f);
+                Matrix4x4 cubeProj = Matrix4x4.Perspective(90.0f, 1.0f, nearPlane, farPlane);
-                            new Vector3(0.0f, 1.0f, 0.0f),
+                            new Vector3(0.0f, 1.0f, 0.0f),
-                            new Vector3(0.0f, 1.0f, 0.0f),
-                            new Vector3(0.0f, 1.0f, 0.0f),
+                            new Vector3(0.0f, -1.0f, 0.0f),
+                            new Vector3(0.0f, -1.0f, 0.0f),
-                            new Vector3(0.0f, 1.0f, 0.0f),
-                            new Vector3(0.0f, 1.0f, 0.0f),
+                            new Vector3(0.0f, -1.0f, 0.0f),
+                            new Vector3(0.0f, -1.0f, 0.0f),
                         };
 
                 for (int i = 0; i < 6; ++i)
                     m_faceCameraInvViewProjectionMatrix[i] = m_faceCameraViewProjectionMatrix[i].inverse;
 
-                    // When rendering into a texture the render will be flip (due to legacy unity openGL behavior), so we need to flip UV here...
-                    m_CubemapFaceMesh[i] = BuildSkyMesh(Vector3.zero, m_faceCameraInvViewProjectionMatrix[i], true);
+                    m_CubemapFaceMesh[i] = BuildSkyMesh(Vector3.zero, m_faceCameraInvViewProjectionMatrix[i], false);
                 }
             }
         }
             Shader.SetGlobalTexture("_SkyTexture", m_SkyboxGGXCubemapRT);
         }
 
-        public void Resize()
+        public void Resize(float nearPlane, float farPlane)
-            RebuildSkyMeshes();
+            RebuildSkyMeshes(nearPlane, farPlane);
         }
 
         public void Build()
         {
             for (int i = 0; i < 6; ++i)
             {
-                Utilities.SetRenderTarget(builtinParams.renderContext, target, ClearFlag.ClearNone, 0, (CubemapFace)i);
-
-                builtinParams.viewProjMatrix = m_faceCameraViewProjectionMatrix[i];
-                builtinParams.depthBuffer = BuiltinSkyParameters.invalidRTI;
+                builtinParams.depthBuffer = BuiltinSkyParameters.nullRT;
+
+                Utilities.SetRenderTarget(builtinParams.renderContext, target, ClearFlag.ClearNone, 0, (CubemapFace)i);
                 m_Renderer.RenderSky(builtinParams, skyParameters, true);
             }
         }
                     m_BuiltinParameters.renderContext = renderContext;
                     m_BuiltinParameters.sunLight = sunLight;
                     m_BuiltinParameters.invViewProjMatrix = camera.invViewProjectionMatrix;
-                    m_BuiltinParameters.viewProjMatrix = camera.viewProjectionMatrix;
                     m_BuiltinParameters.cameraPosWS = camera.camera.transform.position;
                     m_BuiltinParameters.screenSize = camera.screenSize;
                     m_BuiltinParameters.skyMesh = BuildSkyMesh(camera.camera.GetComponent<Transform>().position, m_BuiltinParameters.invViewProjMatrix, false);
-                    Utilities.SetRenderTarget(renderContext, colorBuffer, depthBuffer);
+                    m_Renderer.SetRenderTargets(m_BuiltinParameters);
                     m_Renderer.RenderSky(m_BuiltinParameters, skyParameters, false);
                 }
             }

Assets/ScriptableRenderLoop/HDRenderPipeline/Sky/SkyRenderer.cs

     {
         abstract public void Build();
         abstract public void Cleanup();
+        abstract public void SetRenderTargets(BuiltinSkyParameters builtinParams);
         // renderForCubemap: When rendering into a cube map, no depth buffer is available so user has to make sure not to use depth testing or the depth texture.
         abstract public void RenderSky(BuiltinSkyParameters builtinParams, SkyParameters skyParameters, bool renderForCubemap);
         abstract public bool IsSkyValid(SkyParameters skyParameters);