From 1676c06386e84bebf69a73e9ededd50a00ee87f9 Mon Sep 17 00:00:00 2001
From: Jesse Yurkovich <jesse.y@gmail.com>
Date: Mon, 7 Oct 2024 18:08:22 +0200
Subject: [PATCH] Fix #128504: check instancer purpose and visibility before
 processing

During stage load we first look for Prototype prims which are used for
instancing. However, if the instancer itself at the root of that
Prototype hierarchy would later be excluded because of either purpose or
visibility checks, the Prototypes would still be processed. These would
correctly be imported but would end up orphaned because nothing would
later add them to the view layer. Code expecting these objects to be
found within the scene would then fail.

In Animal Logic ALab this situation played out where there was a
`/root/instancer` prim with purpose "render" and we proceeded to load
the prims under the `/root/instancer/Prototypes/...` hierarchy. Since we
were attempting to load just the "proxy" purpose, the `/root/instancer`
was later excluded and the orphaning of those prototypes happened.

The change here moves `collect_point_instancer_proto_paths` to a method
of `USDStageReader` so it can now access `include_by_purpose` and
`include_by_visibility`. And these are now used to prevent unnecessary
Prototype loading.

Pull Request: https://projects.blender.org/blender/blender/pulls/128564
---
 .../blender/io/usd/intern/usd_reader_stage.cc | 60 +++++++++++--------
 .../blender/io/usd/intern/usd_reader_stage.hh |  1 +
 2 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/source/blender/io/usd/intern/usd_reader_stage.cc b/source/blender/io/usd/intern/usd_reader_stage.cc
index d8cdfd7544f..107690380e1 100644
--- a/source/blender/io/usd/intern/usd_reader_stage.cc
+++ b/source/blender/io/usd/intern/usd_reader_stage.cc
@@ -107,29 +107,6 @@ static void set_instance_collection(
   }
 }
 
-static void collect_point_instancer_proto_paths(const pxr::UsdPrim &prim, UsdPathSet &r_paths)
-{
-  /* Note that we use custom filter flags to allow traversing undefined prims,
-   * because prototype prims may be defined as overs which are skipped by the
-   * default predicate. */
-  pxr::Usd_PrimFlagsConjunction filter_flags = pxr::UsdPrimIsActive && pxr::UsdPrimIsLoaded &&
-                                               !pxr::UsdPrimIsAbstract;
-
-  pxr::UsdPrimSiblingRange children = prim.GetFilteredChildren(filter_flags);
-
-  for (const auto &child_prim : children) {
-    if (pxr::UsdGeomPointInstancer instancer = pxr::UsdGeomPointInstancer(child_prim)) {
-      pxr::SdfPathVector paths;
-      instancer.GetPrototypesRel().GetTargets(&paths);
-      for (const pxr::SdfPath &path : paths) {
-        r_paths.add(path);
-      }
-    }
-
-    collect_point_instancer_proto_paths(child_prim, r_paths);
-  }
-}
-
 USDStageReader::USDStageReader(pxr::UsdStageRefPtr stage,
                                const USDImportParams &params,
                                const ImportSettings &settings)
@@ -755,6 +732,39 @@ void USDStageReader::create_point_instancer_proto_readers(const UsdPathSet &prot
   }
 }
 
+void USDStageReader::collect_point_instancer_proto_paths(const pxr::UsdPrim &prim,
+                                                         UsdPathSet &r_paths) const
+{
+  /* Note that we use custom filter flags to allow traversing undefined prims,
+   * because prototype prims may be defined as overs which are skipped by the
+   * default predicate. */
+  pxr::Usd_PrimFlagsConjunction filter_flags = pxr::UsdPrimIsActive && pxr::UsdPrimIsLoaded &&
+                                               !pxr::UsdPrimIsAbstract;
+
+  pxr::UsdPrimSiblingRange children = prim.GetFilteredChildren(filter_flags);
+
+  for (const auto &child_prim : children) {
+    if (pxr::UsdGeomPointInstancer instancer = pxr::UsdGeomPointInstancer(child_prim)) {
+      /* We should only collect the prototype paths from this instancer if it would be included
+       * by our purpose and visiblity checks, matching what is inside #collect_readers. */
+      if (!include_by_purpose(instancer)) {
+        continue;
+      }
+      if (!include_by_visibility(instancer)) {
+        continue;
+      }
+
+      pxr::SdfPathVector paths;
+      instancer.GetPrototypesRel().GetTargets(&paths);
+      for (const pxr::SdfPath &path : paths) {
+        r_paths.add(path);
+      }
+    }
+
+    collect_point_instancer_proto_paths(child_prim, r_paths);
+  }
+}
+
 UsdPathSet USDStageReader::collect_point_instancer_proto_paths() const
 {
   UsdPathSet result;
@@ -763,12 +773,12 @@ UsdPathSet USDStageReader::collect_point_instancer_proto_paths() const
     return result;
   }
 
-  io::usd::collect_point_instancer_proto_paths(stage_->GetPseudoRoot(), result);
+  collect_point_instancer_proto_paths(stage_->GetPseudoRoot(), result);
 
   std::vector<pxr::UsdPrim> protos = stage_->GetPrototypes();
 
   for (const pxr::UsdPrim &proto_prim : protos) {
-    io::usd::collect_point_instancer_proto_paths(proto_prim, result);
+    collect_point_instancer_proto_paths(proto_prim, result);
   }
 
   return result;
diff --git a/source/blender/io/usd/intern/usd_reader_stage.hh b/source/blender/io/usd/intern/usd_reader_stage.hh
index 8323789c807..2df9bb106a1 100644
--- a/source/blender/io/usd/intern/usd_reader_stage.hh
+++ b/source/blender/io/usd/intern/usd_reader_stage.hh
@@ -184,6 +184,7 @@ class USDStageReader {
    *         does not contain any point instancers.
    */
   UsdPathSet collect_point_instancer_proto_paths() const;
+  void collect_point_instancer_proto_paths(const pxr::UsdPrim &prim, UsdPathSet &r_paths) const;
 
   /**
    * Populate the instancer_proto_readers_ map for the prototype prims