Fix T87929: Cycles, missing update when visibility is modified

This issue originates from a missing BVH packing for visibility data
when it is modified.

To fix this, this adds update flags to the managers to carry the
modified visibility information from the Objects' modified flag to the
GeometryManager.

Another set of flags is added to determine which data need to be packed:
geometry, vertices, or visibility. Those flags are then used when
packing the primivites.

Reviewed By: brecht

Maniphest Tasks: T87929

Differential Revision: https://developer.blender.org/D11219

8 files changed, 125 insertions, 48 deletions

diff --git a/intern/cycles/render/geometry.cpp b/intern/cycles/render/geometry.cpp
index bd2b8164cbb..a1bb8b35ca0 100644
--- a/intern/cycles/render/geometry.cpp
+++ b/intern/cycles/render/geometry.cpp
@@ -46,6 +46,12 @@ CCL_NAMESPACE_BEGIN
 
 /* Geometry */
 
+PackFlags operator |=(PackFlags &pack_flags, uint32_t value)
+{
+  pack_flags = (PackFlags)((uint32_t)pack_flags | value);
+  return pack_flags;
+}
+
 NODE_ABSTRACT_DEFINE(Geometry)
 {
   NodeType *type = NodeType::add("geometry_base", NULL);
@@ -1235,7 +1241,16 @@ void GeometryManager::device_update_bvh(Device *device,
 
   const bool can_refit = scene->bvh != nullptr &&
                          (bparams.bvh_layout == BVHLayout::BVH_LAYOUT_OPTIX);
-  const bool pack_all = scene->bvh == nullptr;
+
+  PackFlags pack_flags = PackFlags::PACK_NONE;
+
+  if (scene->bvh == nullptr) {
+    pack_flags |= PackFlags::PACK_ALL;
+  }
+
+  if (dscene->prim_visibility.is_modified()) {
+    pack_flags |= PackFlags::PACK_VISIBILITY;
+  }
 
   BVH *bvh = scene->bvh;
   if (!scene->bvh) {
@@ -1273,10 +1288,14 @@ void GeometryManager::device_update_bvh(Device *device,
 
     pack.root_index = -1;
 
-    if (!pack_all) {
+    if (pack_flags != PackFlags::PACK_ALL) {
       /* if we do not need to recreate the BVH, then only the vertices are updated, so we can
        * safely retake the memory */
       dscene->prim_tri_verts.give_data(pack.prim_tri_verts);
+
+      if ((pack_flags & PackFlags::PACK_VISIBILITY) != 0) {
+        dscene->prim_visibility.give_data(pack.prim_visibility);
+      }
     }
     else {
       /* It is not strictly necessary to skip those resizes we if do not have to repack, as the OS
@@ -1305,13 +1324,20 @@ void GeometryManager::device_update_bvh(Device *device,
     // Iterate over scene mesh list instead of objects, since 'optix_prim_offset' was calculated
     // based on that list, which may be ordered differently from the object list.
     foreach (Geometry *geom, scene->geometry) {
-      if (!pack_all && !geom->is_modified()) {
+      /* Make a copy of the pack_flags so the current geometry's flags do not pollute the others'. */
+      PackFlags geom_pack_flags = pack_flags;
+
+      if (geom->is_modified()) {
+        geom_pack_flags |= PackFlags::PACK_VERTICES;
+      }
+
+      if (geom_pack_flags == PACK_NONE) {
         continue;
       }
 
       const pair<int, uint> &info = geometry_to_object_info[geom];
       pool.push(function_bind(
-          &Geometry::pack_primitives, geom, &pack, info.first, info.second, pack_all));
+          &Geometry::pack_primitives, geom, &pack, info.first, info.second, geom_pack_flags));
     }
     pool.wait_work();
   }
@@ -1346,7 +1372,7 @@ void GeometryManager::device_update_bvh(Device *device,
     dscene->prim_type.steal_data(pack.prim_type);
     dscene->prim_type.copy_to_device();
   }
-  if (pack.prim_visibility.size() && (dscene->prim_visibility.need_realloc() || has_bvh2_layout)) {
+  if (pack.prim_visibility.size() && (dscene->prim_visibility.is_modified() || has_bvh2_layout)) {
     dscene->prim_visibility.steal_data(pack.prim_visibility);
     dscene->prim_visibility.copy_to_device();
   }
@@ -1594,6 +1620,10 @@ void GeometryManager::device_update_preprocess(Device *device, Scene *scene, Pro
     }
   }
 
+  if ((update_flags & VISIBILITY_MODIFIED) != 0) {
+    dscene->prim_visibility.tag_modified();
+  }
+
   if (device_update_flags & ATTR_FLOAT_NEEDS_REALLOC) {
     dscene->attributes_map.tag_realloc();
     dscene->attributes_float.tag_realloc();
@@ -1920,7 +1950,7 @@ void GeometryManager::device_update(Device *device,
    * Also update the BVH if the transformations change, we cannot rely on tagging the Geometry
    * as modified in this case, as we may accumulate displacement if the vertices do not also
    * change. */
-  bool need_update_scene_bvh = (scene->bvh == nullptr || (update_flags & TRANSFORM_MODIFIED) != 0);
+  bool need_update_scene_bvh = (scene->bvh == nullptr || (update_flags & (TRANSFORM_MODIFIED | VISIBILITY_MODIFIED)) != 0);
   {
     scoped_callback_timer timer([scene](double time) {
       if (scene->update_stats) {
diff --git a/intern/cycles/render/geometry.h b/intern/cycles/render/geometry.h
index abdd851a089..4fd867d9894 100644
--- a/intern/cycles/render/geometry.h
+++ b/intern/cycles/render/geometry.h
@@ -43,6 +43,24 @@ class Shader;
 class Volume;
 struct PackedBVH;
 
+/* Flags used to determine which geometry data need to be packed. */
+enum PackFlags : uint32_t {
+  PACK_NONE = 0u,
+
+  /* Pack the geometry information (e.g. triangle or curve keys indices). */
+  PACK_GEOMETRY = (1u << 0),
+
+  /* Pack the vertice, for Meshes and Volumes' bouding meshes. */
+  PACK_VERTICES = (1u << 1),
+
+  /* Pack the visibility flags for each triangle or curve. */
+  PACK_VISIBILITY = (1u << 2),
+
+  PACK_ALL = (PACK_GEOMETRY | PACK_VERTICES | PACK_VISIBILITY),
+};
+
+PackFlags operator |= (PackFlags &pack_flags, uint32_t value);
+
 /* Geometry
  *
  * Base class for geometric types like Mesh and Hair. */
@@ -126,7 +144,7 @@ class Geometry : public Node {
                    int n,
                    int total);
 
-  virtual void pack_primitives(PackedBVH *pack, int object, uint visibility, bool pack_all) = 0;
+  virtual void pack_primitives(PackedBVH *pack, int object, uint visibility, PackFlags pack_flags) = 0;
 
   /* Check whether the geometry should have own BVH built separately. Briefly,
    * own BVH is needed for geometry, if:
@@ -191,6 +209,8 @@ class GeometryManager {
 
     TRANSFORM_MODIFIED = (1 << 10),
 
+    VISIBILITY_MODIFIED = (1 << 11),
+
     /* tag everything in the manager for an update */
     UPDATE_ALL = ~0u,
 
diff --git a/intern/cycles/render/hair.cpp b/intern/cycles/render/hair.cpp
index dad235aa340..1cae16d7f32 100644
--- a/intern/cycles/render/hair.cpp
+++ b/intern/cycles/render/hair.cpp
@@ -494,38 +494,46 @@ void Hair::pack_curves(Scene *scene,
   }
 }
 
-void Hair::pack_primitives(PackedBVH *pack, int object, uint visibility, bool pack_all)
+void Hair::pack_primitives(PackedBVH *pack, int object, uint visibility, PackFlags pack_flags)
 {
   if (curve_first_key.empty())
     return;
 
-  /* If the BVH does not have to be recreated, we can bail out. */
-  if (!pack_all) {
-    return;
+  /* Separate loop as other arrays are not initialized if their packing is not required. */
+  if ((pack_flags & PACK_VISIBILITY) != 0) {
+    unsigned int *prim_visibility = &pack->prim_visibility[optix_prim_offset];
+
+    size_t index = 0;
+    for (size_t j = 0; j < num_curves(); ++j) {
+      Curve curve = get_curve(j);
+      for (size_t k = 0; k < curve.num_segments(); ++k, ++index) {
+        prim_visibility[index] = visibility;
+      }
+    }
   }
 
-  unsigned int *prim_tri_index = &pack->prim_tri_index[optix_prim_offset];
-  int *prim_type = &pack->prim_type[optix_prim_offset];
-  unsigned int *prim_visibility = &pack->prim_visibility[optix_prim_offset];
-  int *prim_index = &pack->prim_index[optix_prim_offset];
-  int *prim_object = &pack->prim_object[optix_prim_offset];
-  // 'pack->prim_time' is unused by Embree and OptiX
-
-  uint type = has_motion_blur() ?
-                  ((curve_shape == CURVE_RIBBON) ? PRIMITIVE_MOTION_CURVE_RIBBON :
-                                                   PRIMITIVE_MOTION_CURVE_THICK) :
-                  ((curve_shape == CURVE_RIBBON) ? PRIMITIVE_CURVE_RIBBON : PRIMITIVE_CURVE_THICK);
-
-  size_t index = 0;
-  for (size_t j = 0; j < num_curves(); ++j) {
-    Curve curve = get_curve(j);
-    for (size_t k = 0; k < curve.num_segments(); ++k, ++index) {
-      prim_tri_index[index] = -1;
-      prim_type[index] = PRIMITIVE_PACK_SEGMENT(type, k);
-      prim_visibility[index] = visibility;
-      // Each curve segment points back to its curve index
-      prim_index[index] = j + prim_offset;
-      prim_object[index] = object;
+  if ((pack_flags & PACK_GEOMETRY) != 0) {
+    unsigned int *prim_tri_index = &pack->prim_tri_index[optix_prim_offset];
+    int *prim_type = &pack->prim_type[optix_prim_offset];
+    int *prim_index = &pack->prim_index[optix_prim_offset];
+    int *prim_object = &pack->prim_object[optix_prim_offset];
+    // 'pack->prim_time' is unused by Embree and OptiX
+
+    uint type = has_motion_blur() ?
+                    ((curve_shape == CURVE_RIBBON) ? PRIMITIVE_MOTION_CURVE_RIBBON :
+                                                     PRIMITIVE_MOTION_CURVE_THICK) :
+                    ((curve_shape == CURVE_RIBBON) ? PRIMITIVE_CURVE_RIBBON : PRIMITIVE_CURVE_THICK);
+
+    size_t index = 0;
+    for (size_t j = 0; j < num_curves(); ++j) {
+      Curve curve = get_curve(j);
+      for (size_t k = 0; k < curve.num_segments(); ++k, ++index) {
+        prim_tri_index[index] = -1;
+        prim_type[index] = PRIMITIVE_PACK_SEGMENT(type, k);
+        // Each curve segment points back to its curve index
+        prim_index[index] = j + prim_offset;
+        prim_object[index] = object;
+      }
     }
   }
 }
diff --git a/intern/cycles/render/hair.h b/intern/cycles/render/hair.h
index 4b949f984e5..9c553624077 100644
--- a/intern/cycles/render/hair.h
+++ b/intern/cycles/render/hair.h
@@ -146,7 +146,7 @@ class Hair : public Geometry {
   /* BVH */
   void pack_curves(Scene *scene, float4 *curve_key_co, float4 *curve_data, size_t curvekey_offset);
 
-  void pack_primitives(PackedBVH *pack, int object, uint visibility, bool pack_all) override;
+  void pack_primitives(PackedBVH *pack, int object, uint visibility, PackFlags pack_flags) override;
 };
 
 CCL_NAMESPACE_END
diff --git a/intern/cycles/render/mesh.cpp b/intern/cycles/render/mesh.cpp
index d5e5b960665..fd9879dd5dd 100644
--- a/intern/cycles/render/mesh.cpp
+++ b/intern/cycles/render/mesh.cpp
@@ -805,7 +805,7 @@ void Mesh::pack_patches(uint *patch_data, uint vert_offset, uint face_offset, ui
   }
 }
 
-void Mesh::pack_primitives(ccl::PackedBVH *pack, int object, uint visibility, bool pack_all)
+void Mesh::pack_primitives(ccl::PackedBVH *pack, int object, uint visibility, PackFlags pack_flags)
 {
   if (triangles.empty())
     return;
@@ -819,28 +819,38 @@ void Mesh::pack_primitives(ccl::PackedBVH *pack, int object, uint visibility, bo
 
   uint type = has_motion_blur() ? PRIMITIVE_MOTION_TRIANGLE : PRIMITIVE_TRIANGLE;
 
-  if (pack_all) {
+  /* Separate loop as other arrays are not initialized if their packing is not required. */
+  if ((pack_flags & PackFlags::PACK_VISIBILITY) != 0) {
+    unsigned int *prim_visibility = &pack->prim_visibility[optix_prim_offset];
+    for (size_t k = 0; k < num_prims; ++k) {
+      prim_visibility[k] = visibility;
+    }
+  }
+
+  if ((pack_flags & PackFlags::PACK_GEOMETRY) != 0) {
     /* Use optix_prim_offset for indexing as those arrays also contain data for Hair geometries. */
     unsigned int *prim_tri_index = &pack->prim_tri_index[optix_prim_offset];
     int *prim_type = &pack->prim_type[optix_prim_offset];
-    unsigned int *prim_visibility = &pack->prim_visibility[optix_prim_offset];
     int *prim_index = &pack->prim_index[optix_prim_offset];
     int *prim_object = &pack->prim_object[optix_prim_offset];
 
     for (size_t k = 0; k < num_prims; ++k) {
-      prim_tri_index[k] = (prim_offset + k) * 3;
-      prim_type[k] = type;
-      prim_index[k] = prim_offset + k;
-      prim_object[k] = object;
-      prim_visibility[k] = visibility;
+      if ((pack_flags & PackFlags::PACK_GEOMETRY) != 0) {
+        prim_tri_index[k] = (prim_offset + k) * 3;
+        prim_type[k] = type;
+        prim_index[k] = prim_offset + k;
+        prim_object[k] = object;
+      }
     }
   }
 
-  for (size_t k = 0; k < num_prims; ++k) {
-    const Mesh::Triangle t = get_triangle(k);
-    prim_tri_verts[k * 3] = float3_to_float4(verts[t.v[0]]);
-    prim_tri_verts[k * 3 + 1] = float3_to_float4(verts[t.v[1]]);
-    prim_tri_verts[k * 3 + 2] = float3_to_float4(verts[t.v[2]]);
+  if ((pack_flags & PackFlags::PACK_VERTICES) != 0) {
+    for (size_t k = 0; k < num_prims; ++k) {
+      const Mesh::Triangle t = get_triangle(k);
+      prim_tri_verts[k * 3] = float3_to_float4(verts[t.v[0]]);
+      prim_tri_verts[k * 3 + 1] = float3_to_float4(verts[t.v[1]]);
+      prim_tri_verts[k * 3 + 2] = float3_to_float4(verts[t.v[2]]);
+    }
   }
 }
 
diff --git a/intern/cycles/render/mesh.h b/intern/cycles/render/mesh.h
index 2b0ff92ab62..23a4b91d626 100644
--- a/intern/cycles/render/mesh.h
+++ b/intern/cycles/render/mesh.h
@@ -232,7 +232,7 @@ class Mesh : public Geometry {
                   size_t tri_offset);
   void pack_patches(uint *patch_data, uint vert_offset, uint face_offset, uint corner_offset);
 
-  void pack_primitives(PackedBVH *pack, int object, uint visibility, bool pack_all) override;
+  void pack_primitives(PackedBVH *pack, int object, uint visibility, PackFlags pack_flags) override;
 
   void tessellate(DiagSplit *split);
 
diff --git a/intern/cycles/render/object.cpp b/intern/cycles/render/object.cpp
index 8b3ac31cf8b..f65f8bc6e90 100644
--- a/intern/cycles/render/object.cpp
+++ b/intern/cycles/render/object.cpp
@@ -220,6 +220,10 @@ void Object::tag_update(Scene *scene)
       flag |= ObjectManager::TRANSFORM_MODIFIED;
     }
 
+    if (visibility_is_modified()) {
+      flag |= ObjectManager::VISIBILITY_MODIFIED;
+    }
+
     foreach (Node *node, geometry->get_used_shaders()) {
       Shader *shader = static_cast<Shader *>(node);
       if (shader->get_use_mis() && shader->has_surface_emission)
@@ -914,6 +918,10 @@ void ObjectManager::tag_update(Scene *scene, uint32_t flag)
       geometry_flag |= GeometryManager::TRANSFORM_MODIFIED;
     }
 
+    if ((flag & VISIBILITY_MODIFIED) != 0) {
+      geometry_flag |= GeometryManager::VISIBILITY_MODIFIED;
+    }
+
     scene->geometry_manager->tag_update(scene, geometry_flag);
   }
 
diff --git a/intern/cycles/render/object.h b/intern/cycles/render/object.h
index 23682270fd1..e4bc7ac3d8e 100644
--- a/intern/cycles/render/object.h
+++ b/intern/cycles/render/object.h
@@ -134,6 +134,7 @@ class ObjectManager {
     OBJECT_MODIFIED = (1 << 5),
     HOLDOUT_MODIFIED = (1 << 6),
     TRANSFORM_MODIFIED = (1 << 7),
+    VISIBILITY_MODIFIED = (1 << 8),
 
     /* tag everything in the manager for an update */
     UPDATE_ALL = ~0u,