Probe Volumes: Make all bake requests go through the AdditionalGIBakeRequestsManager. Track the generationIndex in the least significant 8 bits of the lightmapperBakeID, and talk to the lightmapper with this composite ID. This ensures that anytime we call SetAdditionalBakeProbes we generate a completely unique bake request from the perspective of the lightmapper, ensuring that we will always bake this data (because the lightmapper will hash these IDs to determine if it has any work to do). Previously, if we set an ID with new data, or if we cleared an ID then set it with new data, the lightmapper wouldnt track these data changes, and so it would skip baking these actually new bake requests. (#17)

Co-authored-by: pastasfuture <nickb@bonfirestudios.com>

Author: pastasfuture

com.unity.render-pipelines.high-definition/Runtime/Lighting/AdditionalGIBakeRequestsManager.cs

@@ -14,7 +14,8 @@ namespace UnityEngine.Rendering.HighDefinition
     public class AdditionalGIBakeRequestsManager
     {
         // The baking ID for the extra requests
-        internal static readonly int s_BakingID = int.MaxValue;
+        private static int s_AdditionalGIBakeRequestsBakingID = 0;
+        private static readonly int s_LightmapperBakeIDStart = 1;
 
         private static AdditionalGIBakeRequestsManager s_Instance = new AdditionalGIBakeRequestsManager();
         /// <summary>
@@ -56,32 +57,90 @@ private AdditionalGIBakeRequestsManager()
         // Rather than hashing and hoping we don't collide, lets handle this robustly by
         // keeping a dictionary of ProbeVolumeGlobalUniqueID->int bit keys.
         private Dictionary<ProbeVolumeGlobalUniqueID, int> lightmapperBakeIDFromBakeID = new Dictionary<ProbeVolumeGlobalUniqueID, int>(32);
-        private int lightmapperBakeIDNext = 0;
+        private int lightmapperBakeIDNext = s_LightmapperBakeIDStart;
 
-        internal bool TryGetLightmapperBakeIDFromBakeID(ProbeVolumeGlobalUniqueID bakeID, out int lightmapperBakeID)
+        internal void SetAdditionalBakedProbes(ProbeVolumeGlobalUniqueID bakeID, Vector3[] positions)
+        {
+            if (TryGetLightmapperBakeIDFromBakeID(bakeID, out int lightmapperBakeID))
+            {
+                UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(lightmapperBakeID, null);
+
+                // When baking, the lightmapper hashes its state (i.e: the list of all AdditionalBakedProbes requests)
+                // and only bakes data if this hash is not changed.
+                // By storing a generation ID inside of our lightmapperBakeID, we ensure that Sets will always look like a completely new bake request to the lightmapper.
+                // The lightmapper will always bake it.
+                // Without storing this generation index, if we clear our bake request by setting positions to NULL, then set our bake request with valid data,
+                // then bake, the lightmapper will treat the new bake request as an already completed old one, and skip doing any work.
+                // In the future, after proving out this generation based approach, it would be a good idea to move this generation tracking code into the lightmapper,
+                // so that users dont need to do this bookkeeping for the lightmapper - they can simply set and clear requests and always get the correct, fresh results.
+                IncrementLightmapperBakeIDGeneration(bakeID, out lightmapperBakeID);
+
+                if (positions != null)
+                {
+                    UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(lightmapperBakeID, positions);
+                }                
+            }
+        }
+
+        internal bool GetAdditionalBakedProbes(ProbeVolumeGlobalUniqueID bakeID, NativeArray<SphericalHarmonicsL2> outBakedProbeSH, NativeArray<float> outBakedProbeValidity, NativeArray<float> outBakedProbeOctahedralDepth)
+        {
+            bool success = false;
+            if (TryGetLightmapperBakeIDFromBakeID(bakeID, out int lightmapperBakeID))
+            {
+                success = UnityEditor.Experimental.Lightmapping.GetAdditionalBakedProbes(lightmapperBakeID, outBakedProbeSH, outBakedProbeValidity, outBakedProbeOctahedralDepth);
+            }
+            return success;
+        }
+
+        private bool TryGetLightmapperBakeIDFromBakeID(ProbeVolumeGlobalUniqueID bakeID, out int lightmapperBakeID)
         {
             bool success = false;
             if (lightmapperBakeIDFromBakeID.TryGetValue(bakeID, out lightmapperBakeID))
             {
                 success = true;
             }
-            else if (lightmapperBakeIDNext == s_BakingID)
+            // Leave the whole top bit free. We shouldn't encounter it in practice, avoiding it allows us to not worry about handling the signed case.
+            else if (lightmapperBakeIDNext == ((1 << 23) - 1))
             {
                 success = false;
                 lightmapperBakeID = -1;
-                Debug.LogWarningFormat("Error: Used up all lightmapper bake IDs. This should never happen. Somehow all {0} ids have been used up. This must be the result of a bug. Unlikely that you created and baked {0} unique bake requests. Quit and reopen unity to flush all IDs.", s_BakingID - 1);
+                Debug.LogWarningFormat("Error: Used up all lightmapper bake IDs. This should never happen. Somehow all {0} ids have been used up. This must be the result of a bug. Unlikely that you created and baked {0} unique bake requests. Quit and reopen unity to flush all IDs.", (1 << 23) - 1);
             }
             else
             {
                 success = true;
-                lightmapperBakeID = lightmapperBakeIDNext;
+                lightmapperBakeID = lightmapperBakeIDNext << 8;
                 ++lightmapperBakeIDNext;
                 lightmapperBakeIDFromBakeID.Add(bakeID, lightmapperBakeID);
             }
 
             return success;
         }
 
+        private void IncrementLightmapperBakeIDGeneration(ProbeVolumeGlobalUniqueID bakeID, out int lightmapperBakeID)
+        {
+            lightmapperBakeID = -1;
+            if (lightmapperBakeIDFromBakeID.TryGetValue(bakeID, out lightmapperBakeID))
+            {
+                IncrementLightmapperBakeIDGeneration(ref lightmapperBakeID);
+                lightmapperBakeIDFromBakeID[bakeID] = lightmapperBakeID;
+            }
+            else
+            {
+                Debug.Assert(false);
+            }
+        }
+
+        private static void IncrementLightmapperBakeIDGeneration(ref int lightmapperBakeID)
+        {
+            const int MASK = 255;
+            int generationIndex = lightmapperBakeID & MASK;
+            generationIndex = (generationIndex == MASK) ? 0 : (generationIndex + 1);
+
+            lightmapperBakeID &= ~MASK;
+            lightmapperBakeID |= generationIndex;
+        }
+
         /// <summary>
         /// Enqueue a request for probe rendering at the specified location.
         /// </summary>
@@ -214,7 +273,9 @@ internal void AddRequestsToLightmapper()
 
             EnsureRequestPositionsSanitized();
 
-            UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(s_BakingID, m_RequestPositionsSanitized);
+            UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(s_AdditionalGIBakeRequestsBakingID, null);
+            IncrementLightmapperBakeIDGeneration(ref s_AdditionalGIBakeRequestsBakingID);
+            UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(s_AdditionalGIBakeRequestsBakingID, m_RequestPositionsSanitized);
             m_RequestToLightmapperIsSet = true;
 
             UnityEditor.Experimental.Lightmapping.additionalBakedProbesCompleted -= OnAdditionalProbesBakeCompleted;
@@ -223,7 +284,7 @@ internal void AddRequestsToLightmapper()
 
         private void RemoveRequestsFromLightmapper()
         {
-            UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(s_BakingID, null);
+            UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(s_AdditionalGIBakeRequestsBakingID, null);
             m_RequestToLightmapperIsSet = false;
         }
 
@@ -237,7 +298,7 @@ private void OnAdditionalProbesBakeCompleted()
             var validity = new NativeArray<float>(m_RequestPositions.Count, Allocator.Temp, NativeArrayOptions.UninitializedMemory);
             var bakedProbeOctahedralDepth = new NativeArray<float>(m_RequestPositions.Count * 64, Allocator.Temp, NativeArrayOptions.UninitializedMemory);
 
-            if (UnityEditor.Experimental.Lightmapping.GetAdditionalBakedProbes(s_BakingID, sh, validity, bakedProbeOctahedralDepth))
+            if (UnityEditor.Experimental.Lightmapping.GetAdditionalBakedProbes(s_AdditionalGIBakeRequestsBakingID, sh, validity, bakedProbeOctahedralDepth))
             {
                 SetSHCoefficients(sh);
                 PushSHCoefficientsToReflectionProbes();

com.unity.render-pipelines.high-definition/Runtime/Lighting/ProbeVolume/ProbeVolume.cs

@@ -1245,10 +1245,7 @@ internal void ForceBakingDisabled()
             if (!UnityEditor.EditorApplication.isPlaying)
             {
                 // Do not spend time interacting with the baking API if we are in playmode.
-                if (AdditionalGIBakeRequestsManager.instance.TryGetLightmapperBakeIDFromBakeID(GetBakeID(), out int lightmapperBakeID))
-                {
-                    UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(lightmapperBakeID, null);
-                }
+                AdditionalGIBakeRequestsManager.instance.SetAdditionalBakedProbes(GetBakeID(), null);
                 return;
             }
         }
@@ -1359,8 +1356,7 @@ internal void OnProbesBakeCompleted()
 #else
             var octahedralDepth = new NativeArray<float>(numProbes * 8 * 8, Allocator.Temp, NativeArrayOptions.UninitializedMemory);
 #endif
-            bool lightmapperBakeIDExists = AdditionalGIBakeRequestsManager.instance.TryGetLightmapperBakeIDFromBakeID(GetBakeID(), out int lightmapperBakeID);
-            if(lightmapperBakeIDExists && UnityEditor.Experimental.Lightmapping.GetAdditionalBakedProbes(lightmapperBakeID, sh, validity, octahedralDepth))
+            if(AdditionalGIBakeRequestsManager.instance.GetAdditionalBakedProbes(GetBakeID(), sh, validity, octahedralDepth))
             {
                 if (probeVolumeAsset == null)
                 {
@@ -1419,7 +1415,7 @@ internal void OnProbesBakeCompleted()
                 string parentName = (transform.parent == null) ? "null" : transform.parent.name;
                 bool isCompanionGameObject = gameObject.scene.path == "";
                 string companionString = isCompanionGameObject ? "true" : "false";
-                Debug.LogFormat("Failed to get data at id: {0}, with lightmapperBakeID {1}, with probe volume: {2}, and parent: {3}, companionGameObject: {4}", GetBakeID(), lightmapperBakeID, name, parentName, companionString);
+                Debug.LogFormat("Failed to get data at id: {0}, with probe volume: {1}, and parent: {2}, companionGameObject: {3}", GetBakeID(), name, parentName, companionString);
             }
 
             sh.Dispose();
@@ -1499,9 +1495,6 @@ private void SetupProbePositions()
             if (UnityEditor.EditorApplication.isPlaying)
                 return;
 
-            if (!AdditionalGIBakeRequestsManager.instance.TryGetLightmapperBakeIDFromBakeID(GetBakeID(), out int lightmapperBakeID))
-                return;
-
             int probeCount = parameters.resolutionX * parameters.resolutionY * parameters.resolutionZ;
             m_ProbePositions = new Vector3[probeCount];
 
@@ -1529,7 +1522,7 @@ private void SetupProbePositions()
                 }
             }
 
-            UnityEditor.Experimental.Lightmapping.SetAdditionalBakedProbes(lightmapperBakeID, m_ProbePositions);
+            AdditionalGIBakeRequestsManager.instance.SetAdditionalBakedProbes(GetBakeID(), m_ProbePositions);
             bakingEnabled = true;
         }