[Port] [6000.4] Graphics/SRP/RPF - [UUM-115209] Re-enable the second step of the Render Graph culling algorithm to prevent pass culling regression in HDRP

Author: svc-reach-platform-support

Packages/com.unity.render-pipelines.core/Runtime/RenderGraph/Compiler/NativePassCompiler.cs

@@ -475,58 +475,59 @@ void CullUnusedRenderGraphPasses()
                     return;
 
                 // Must come first
-                // TODO make another subfunction CullRenderGraphPassesWithNoSideEffect() for this first step of the culling stage
-                var ctx = contextData;
+                CullRenderGraphPassesWithNoSideEffect();
 
-                // Cull all passes first
-                ctx.CullAllPasses(true);
-
-                // Flood fill downstream algorithm using BFS,
-                // starting from the passes with side effects (writting to imported texture, not allowed to be culled, globals modification...)
-                // to all their dependencies
-                while (m_HasSideEffectPassIdCullingStack.Count != 0)
-                {
-                    int passId = m_HasSideEffectPassIdCullingStack.Pop();
+                // Second step of the algorithm that comes later
+                CullRenderGraphPassesWritingOnlyUnusedResources();
+            }
+        }
 
-                    ref var passData = ref ctx.passData.ElementAt(passId);
+        void CullRenderGraphPassesWithNoSideEffect()
+        {
+            var ctx = contextData;
 
-                    // We already found this node through another dependency chain
-                    if (!passData.culled) continue;
+            // Cull all passes first
+            ctx.CullAllPasses(true);
 
-                    // Flow upstream from this node
-                    foreach (ref readonly var input in passData.Inputs(ctx))
-                    {
-                        ref var inputVersionedDataRes = ref ctx.resources[input.resource];
+            // Flood fill downstream algorithm using BFS,
+            // starting from the passes with side effects (writting to imported texture, not allowed to be culled, globals modification...)
+            // to all their dependencies
+            while (m_HasSideEffectPassIdCullingStack.Count != 0)
+            {
+                int passId = m_HasSideEffectPassIdCullingStack.Pop();
 
-                        if (inputVersionedDataRes.written)
-                        {
-                            m_HasSideEffectPassIdCullingStack.Push(inputVersionedDataRes.writePassId);
-                        }
-                    }
+                ref var passData = ref ctx.passData.ElementAt(passId);
 
-                    // We need this node, don't cull it
-                    passData.culled = false;
-                }
+                // We already found this node through another dependency chain
+                if (!passData.culled) continue;
 
-                // Update graph based on freshly culled nodes, remove any connection to them
-                // We start from the latest passes to the first ones as we might need to decrement the version number of unwritten resources
-                var numPasses = ctx.passData.Length;
-                for (int passIndex = numPasses - 1; passIndex >= 0; passIndex--)
+                // Flow upstream from this node
+                foreach (ref readonly var input in passData.Inputs(ctx))
                 {
-                    ref readonly var pass = ref ctx.passData.ElementAt(passIndex);
+                    ref var inputVersionedDataRes = ref ctx.resources[input.resource];
 
-                    // Remove the connections from the list so they won't be visited again
-                    if (pass.culled)
+                    if (inputVersionedDataRes.written)
                     {
-                        pass.DisconnectFromResources(ctx);
+                        m_HasSideEffectPassIdCullingStack.Push(inputVersionedDataRes.writePassId);
                     }
                 }
 
-                // Second step of the algorithm, must come after
-                // TODO: The resources culling step is currently disabled due to an issue: https://jira.unity3d.com/projects/SRP/issues/SRP-897
-                // Renabled the resource culling step after addressing the depth attachment problem above.
-                // Renabled the relevent tests
-                // CullRenderGraphPassesWritingOnlyUnusedResources();
+                // We need this node, don't cull it
+                passData.culled = false;
+            }
+
+            // Update graph based on freshly culled nodes, remove any connection to them
+            // We start from the latest passes to the first ones as we might need to decrement the version number of unwritten resources
+            var numPasses = ctx.passData.Length;
+            for (int passIndex = numPasses - 1; passIndex >= 0; passIndex--)
+            {
+                ref readonly var pass = ref ctx.passData.ElementAt(passIndex);
+
+                // Remove the connections from the list so they won't be visited again
+                if (pass.culled)
+                {
+                    pass.DisconnectFromResources(ctx);
+                }
             }
         }
 
@@ -581,24 +582,25 @@ void CullRenderGraphPassesWritingOnlyUnusedResources()
                         producerData.culled = true;
                         producerData.DisconnectFromResources(ctx, unusedVersionedResourceIdCullingStack, type);
                     }
-                    else // Producer is (still) necessary, but we might need to remove the read of the previous version coming implicitly with the write of the current version
+                    else 
                     {
+                        // Producer is still necessary for now, but if the previous version is only implicitly read by it to write the unused resource
+                        // then we can consider this version useless as well and add it to the stack.
+                        // We purposefully keep the connection between the producer and this resource nevertheless to ensure proper lifetime handling and attachment setup.
+                        // A more optimal approach memory-wise would be to cut the dependency, decrease the latestVersionNumber of the resource and release it earlier
+                        // but we then need to create a transient resource with the right attachment properties and attach it to the non-culled producer or the native render pass setup will be incorrect.
+
                         // We always add written resource to the stack so versionedIndex > 0
                         var prevVersionedRes = new ResourceHandle(unusedResource, unusedResource.version - 1);
 
-                        // If no explicit read is requested by the user (AccessFlag.Write only), we need to remove the implicit read
-                        // so that we cut cleanly the connection between previous version of the resource and current producer
                         bool isImplicitRead = graph.m_RenderPasses[producerData.passId].implicitReadsList.Contains(prevVersionedRes);
 
                         if (isImplicitRead)
                         {
                             ref var prevVersionedDataRes = ref ctx.resources[prevVersionedRes];
 
-                            // Notify the previous version of this resource that it is not read anymore by this pass
-                            prevVersionedDataRes.RemoveReadingPass(ctx, prevVersionedRes, producerData.passId);
-
-                            // We also need to add the previous version of the resource to the stack IF no other pass than current producer needed it
-                            if (prevVersionedDataRes.written && prevVersionedDataRes.numReaders == 0)
+                            // We add the previous version of the resource to the stack IF no other pass than current producer needs it
+                            if (prevVersionedDataRes.written && prevVersionedDataRes.numReaders == 1)
                             {
                                 unusedVersionedResourceIdCullingStack.Push(prevVersionedRes);
                             }

Packages/com.unity.render-pipelines.core/Tests/Editor/NativePassCompilerRenderGraphTests.cs

@@ -1799,7 +1799,6 @@ public void BreakPasses_WhenNoOrDifferentShadingRateImage()
             }
         }
 
-/* // DepthAttachment bug: https://jira.unity3d.com/projects/SRP/issues/SRP-897
         [Test]
         public void UnusedResourceCulling_CullProducer_WhenVersionsAreNotExplicitlyRead()
         {
@@ -1905,7 +1904,6 @@ public void UnusedResourceCulling_CullProducer_WhenNoneOfItsWrittenResourcesAreE
             // extraBuffer[2] latest version remains at 1
             Assert.AreEqual(result.contextData.UnversionedResourceData(passes[0].attachments[1].handle).latestVersionNumber, 1);
         }
-*/
 
         [Test]
         public void UnusedResourceCulling_DoNotCullProducer_WhenOneOfItsWrittenResourcesIsExplicitlyRead()
@@ -2009,6 +2007,42 @@ public void UnusedResourceCulling_CullProducer_WhenNextVersionOfProducedResource
             Assert.AreEqual(result.contextData.UnversionedResourceData(passes[0].attachments[0].handle).latestVersionNumber, 3);
         }
 
+        [Test]
+        public void UnusedResourceCulling_KeepUnusedLatestVersion_WhenProducerIsNotCulled()
+        {
+            var g = AllocateRenderGraph();
+            var renderTargets = ImportAndCreateRenderTargets(g);
+
+            // This pass implicitly reads version 0 of extraTextures[0] and writes its version 1 that will be implicitly read in the next pass - dependency
+            // It also explicitly reads version 0 of extraTextures[1] and writes its version 1 but none reads it - no side effect
+            // It also implicitly reads version 0 of depthBuffer that is imported in RG but don't write it - no side effect
+            using (var builder = g.AddRasterRenderPass<RenderGraphTestPassData>("TestPass0", out var passData))
+            {
+                builder.SetRenderAttachment(renderTargets.extraTextures[0], 0, AccessFlags.ReadWrite);
+                builder.SetRenderAttachment(renderTargets.backBuffer, 1, AccessFlags.Write);
+                builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
+            }
+
+            // This pass writes version 2 of extraTextures[2] that no one uses
+            // technically we could get rid of this version but we preserve it for now to ensure correct resource lifetime and render pass setup
+            // We explicitly request this pass not to be culled, so it will be preserved
+            using (var builder = g.AddRasterRenderPass<RenderGraphTestPassData>("TestPass1", out var passData))
+            {
+                builder.SetRenderAttachment(renderTargets.extraTextures[0], 0, AccessFlags.Write);
+                builder.SetRenderFunc((RenderGraphTestPassData data, RasterGraphContext context) => { });
+                builder.AllowPassCulling(false);
+            }
+
+            var result = g.CompileNativeRenderGraph(g.ComputeGraphHash());
+            var passes = result.contextData.GetNativePasses();
+
+            // - TestPass1 can NOT be culled
+            // - TestPass1 can NOT be culled as it writes to an imported texture (backbuffer)
+            Assert.IsTrue(passes != null && passes.Count == 1 && passes[0].numGraphPasses == 2);
+            // extraBuffer[0] latest version remains at 2 if no one uses it
+            Assert.AreEqual(result.contextData.UnversionedResourceData(passes[0].attachments[0].handle).latestVersionNumber, 2);
+        }
+
         // Test using a texture as both a texture and render attachment, one will require topleft and one bottom left so this should throw.
         [Test]
         public void TextureUVOrigin_CheckInvalidMixedUVOriginUseTextureCompiler()