Fix: Broken node tool group data-block references

Currently the node tool node group and data-blocks referenced by it
may not be part of the active dependency graph. This means we
cannot retrieve their evaluated geometry when executing the operator.
Since operators almost always use the evaluated geometry of other
objects, and since geometry nodes is mostly set up to deal with
evaluated data-blocks currently, this must be fixed.

Instead, set up a temporary dependency graph and add the selected
objects and the data-blocks used by the node group. That graph is
evaluated to give simple access to evaluated data-blocks.

Unfortunately this will cause more work than necessary in a few ways:
1. Selected objects are reevaluated an extra time before execution.
2. All data-blocks referenced by the group are completely evaluated again.
3. The node group itself is reevaluated, which recreates the function graph.

These may or may not become bottlenecks in the future, but it's best to
keep it simple late in the release process. And between a completely
broken feature and a potentially slow feature, the choice is clear!

Pull Request: https://projects.blender.org/blender/blender/pulls/114293

source/blender/editors/geometry/node_group_operator.cc

@@ -39,6 +39,7 @@
 #include "DNA_scene_types.h"
 
 #include "DEG_depsgraph.hh"
+#include "DEG_depsgraph_build.hh"
 #include "DEG_depsgraph_query.hh"
 
 #include "RNA_access.hh"
@@ -167,6 +168,7 @@ static void store_result_geometry(Main &bmain,
                                   Object &object,
                                   bke::GeometrySet geometry)
 {
+  geometry.ensure_owns_direct_data();
   switch (object.type) {
     case OB_CURVES: {
       Curves &curves = *static_cast<Curves *>(object.data);
@@ -225,10 +227,34 @@ static void store_result_geometry(Main &bmain,
   }
 }
 
+/**
+ * Create a dependency graph referencing all data-blocks used by the tree, and all selected
+ * objects. Adding the selected objects is necessary because they are currently compared by pointer
+ * to other evaluated objects inside of geometry nodes.
+ */
+static Depsgraph *build_depsgraph_from_indirect_ids(Main &bmain,
+                                                    Scene &scene,
+                                                    ViewLayer &view_layer,
+                                                    const bNodeTree &node_tree_orig,
+                                                    const Span<const Object *> objects)
+{
+  Set<ID *> ids_for_relations;
+  bool needs_own_transform_relation = false;
+  nodes::find_node_tree_dependencies(node_tree_orig, ids_for_relations, needs_own_transform_relation);
+
+  Vector<const ID *> ids;
+  ids.append(&node_tree_orig.id);
+  ids.extend(objects.cast<const ID *>());
+  ids.insert(ids.size(), ids_for_relations.begin(), ids_for_relations.end());
+
+  Depsgraph *depsgraph = DEG_graph_new(&bmain, &scene, &view_layer, DAG_EVAL_VIEWPORT);
+  DEG_graph_build_from_ids(depsgraph, const_cast<ID **>(ids.data()), ids.size());
+  return depsgraph;
+}
+
 static int run_node_group_exec(bContext *C, wmOperator *op)
 {
   Main *bmain = CTX_data_main(C);
-  Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C);
   Scene *scene = CTX_data_scene(C);
   ViewLayer *view_layer = CTX_data_view_layer(C);
   Object *active_object = CTX_data_active_object(C);
@@ -240,11 +266,24 @@ static int run_node_group_exec(bContext *C, wmOperator *op)
   }
   const eObjectMode mode = eObjectMode(active_object->mode);
 
-  const bNodeTree *node_tree = get_node_group(*C, *op->ptr, op->reports);
-  if (!node_tree) {
+  const bNodeTree *node_tree_orig = get_node_group(*C, *op->ptr, op->reports);
+  if (!node_tree_orig) {
     return OPERATOR_CANCELLED;
   }
 
+  uint objects_len = 0;
+  Object **objects = BKE_view_layer_array_from_objects_in_mode_unique_data(
+      scene, view_layer, CTX_wm_view3d(C), &objects_len, mode);
+  BLI_SCOPED_DEFER([&]() { MEM_SAFE_FREE(objects); });
+
+  Depsgraph *depsgraph = build_depsgraph_from_indirect_ids(
+      *bmain, *scene, *view_layer, *node_tree_orig, {objects, objects_len});
+  DEG_evaluate_on_refresh(depsgraph);
+  BLI_SCOPED_DEFER([&]() { DEG_graph_free(depsgraph); });
+
+  const bNodeTree *node_tree = reinterpret_cast<const bNodeTree *>(
+      DEG_get_evaluated_id(depsgraph, const_cast<ID *>(&node_tree_orig->id)));
+
   const nodes::GeometryNodesLazyFunctionGraphInfo *lf_graph_info =
       nodes::ensure_geometry_nodes_lazy_function_graph(*node_tree);
   if (lf_graph_info == nullptr) {
@@ -257,10 +296,6 @@ static int run_node_group_exec(bContext *C, wmOperator *op)
     return OPERATOR_CANCELLED;
   }
 
-  uint objects_len = 0;
-  Object **objects = BKE_view_layer_array_from_objects_in_mode_unique_data(
-      scene, view_layer, CTX_wm_view3d(C), &objects_len, mode);
-
   OperatorComputeContext compute_context(op->type->idname);
 
   for (Object *object : Span(objects, objects_len)) {
@@ -269,8 +304,8 @@ static int run_node_group_exec(bContext *C, wmOperator *op)
     }
     nodes::GeoNodesOperatorData operator_eval_data{};
     operator_eval_data.depsgraph = depsgraph;
-    operator_eval_data.self_object = object;
-    operator_eval_data.scene = scene;
+    operator_eval_data.self_object = DEG_get_evaluated_object(depsgraph, object);
+    operator_eval_data.scene = DEG_get_evaluated_scene(depsgraph);
 
     bke::GeometrySet geometry_orig = get_original_geometry_eval_copy(*object);
 
@@ -290,8 +325,6 @@ static int run_node_group_exec(bContext *C, wmOperator *op)
     WM_event_add_notifier(C, NC_GEOM | ND_DATA, object->data);
   }
 
-  MEM_SAFE_FREE(objects);
-
   return OPERATOR_FINISHED;
 }
 

source/blender/modifiers/intern/MOD_nodes.cc

@@ -112,98 +112,6 @@ static void init_data(ModifierData *md)
   nmd->runtime->cache = std::make_shared<bake::ModifierCache>();
 }
 
-static void add_used_ids_from_sockets(const ListBase &sockets, Set<ID *> &ids)
-{
-  LISTBASE_FOREACH (const bNodeSocket *, socket, &sockets) {
-    switch (socket->type) {
-      case SOCK_OBJECT: {
-        if (Object *object = ((bNodeSocketValueObject *)socket->default_value)->value) {
-          ids.add(&object->id);
-        }
-        break;
-      }
-      case SOCK_COLLECTION: {
-        if (Collection *collection = ((bNodeSocketValueCollection *)socket->default_value)->value)
-        {
-          ids.add(&collection->id);
-        }
-        break;
-      }
-      case SOCK_MATERIAL: {
-        if (Material *material = ((bNodeSocketValueMaterial *)socket->default_value)->value) {
-          ids.add(&material->id);
-        }
-        break;
-      }
-      case SOCK_TEXTURE: {
-        if (Tex *texture = ((bNodeSocketValueTexture *)socket->default_value)->value) {
-          ids.add(&texture->id);
-        }
-        break;
-      }
-      case SOCK_IMAGE: {
-        if (Image *image = ((bNodeSocketValueImage *)socket->default_value)->value) {
-          ids.add(&image->id);
-        }
-        break;
-      }
-    }
-  }
-}
-
-/**
- * \note We can only check properties here that cause the dependency graph to update relations when
- * they are changed, otherwise there may be a missing relation after editing. So this could check
- * more properties like whether the node is muted, but we would have to accept the cost of updating
- * relations when those properties are changed.
- */
-static bool node_needs_own_transform_relation(const bNode &node)
-{
-  if (node.type == GEO_NODE_COLLECTION_INFO) {
-    const NodeGeometryCollectionInfo &storage = *static_cast<const NodeGeometryCollectionInfo *>(
-        node.storage);
-    return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE;
-  }
-
-  if (node.type == GEO_NODE_OBJECT_INFO) {
-    const NodeGeometryObjectInfo &storage = *static_cast<const NodeGeometryObjectInfo *>(
-        node.storage);
-    return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE;
-  }
-
-  if (node.type == GEO_NODE_SELF_OBJECT) {
-    return true;
-  }
-  if (node.type == GEO_NODE_DEFORM_CURVES_ON_SURFACE) {
-    return true;
-  }
-
-  return false;
-}
-
-static void process_nodes_for_depsgraph(const bNodeTree &tree,
-                                        Set<ID *> &ids,
-                                        bool &r_needs_own_transform_relation,
-                                        Set<const bNodeTree *> &checked_groups)
-{
-  if (!checked_groups.add(&tree)) {
-    return;
-  }
-
-  tree.ensure_topology_cache();
-  for (const bNode *node : tree.all_nodes()) {
-    add_used_ids_from_sockets(node->inputs, ids);
-    add_used_ids_from_sockets(node->outputs, ids);
-    r_needs_own_transform_relation |= node_needs_own_transform_relation(*node);
-  }
-
-  for (const bNode *node : tree.group_nodes()) {
-    if (const bNodeTree *sub_tree = reinterpret_cast<const bNodeTree *>(node->id)) {
-      process_nodes_for_depsgraph(*sub_tree, ids, r_needs_own_transform_relation, checked_groups);
-    }
-  }
-}
-
 static void find_used_ids_from_settings(const NodesModifierSettings &settings, Set<ID *> &ids)
 {
   IDP_foreach_property(
@@ -259,9 +167,7 @@ static void update_depsgraph(ModifierData *md, const ModifierUpdateDepsgraphCont
   bool needs_own_transform_relation = false;
   Set<ID *> used_ids;
   find_used_ids_from_settings(nmd->settings, used_ids);
-  Set<const bNodeTree *> checked_groups;
-  process_nodes_for_depsgraph(
-      *nmd->node_group, used_ids, needs_own_transform_relation, checked_groups);
+  nodes::find_node_tree_dependencies(*nmd->node_group, used_ids, needs_own_transform_relation);
 
   if (ctx->object->type == OB_CURVES) {
     Curves *curves_id = static_cast<Curves *>(ctx->object->data);

source/blender/nodes/NOD_geometry_nodes_execute.hh

@@ -7,6 +7,7 @@
 #include "BLI_compute_context.hh"
 #include "BLI_function_ref.hh"
 #include "BLI_multi_value_map.hh"
+#include "BLI_set.hh"
 
 #include "BKE_idprop.hh"
 #include "BKE_node.h"
@@ -29,6 +30,10 @@ class GeoModifierLog;
 
 namespace blender::nodes {
 
+void find_node_tree_dependencies(const bNodeTree &tree,
+                           Set<ID *> &r_ids,
+                           bool &r_needs_own_transform_relation);
+
 StringRef input_use_attribute_suffix();
 StringRef input_attribute_name_suffix();
 

source/blender/nodes/intern/geometry_nodes_execute.cc

@@ -30,6 +30,106 @@ namespace geo_log = blender::nodes::geo_eval_log;
 
 namespace blender::nodes {
 
+static void add_used_ids_from_sockets(const ListBase &sockets, Set<ID *> &ids)
+{
+  LISTBASE_FOREACH (const bNodeSocket *, socket, &sockets) {
+    switch (socket->type) {
+      case SOCK_OBJECT: {
+        if (Object *object = ((bNodeSocketValueObject *)socket->default_value)->value) {
+          ids.add(reinterpret_cast<ID *>(object));
+        }
+        break;
+      }
+      case SOCK_COLLECTION: {
+        if (Collection *collection = ((bNodeSocketValueCollection *)socket->default_value)->value)
+        {
+          ids.add(reinterpret_cast<ID *>(collection));
+        }
+        break;
+      }
+      case SOCK_MATERIAL: {
+        if (Material *material = ((bNodeSocketValueMaterial *)socket->default_value)->value) {
+          ids.add(reinterpret_cast<ID *>(material));
+        }
+        break;
+      }
+      case SOCK_TEXTURE: {
+        if (Tex *texture = ((bNodeSocketValueTexture *)socket->default_value)->value) {
+          ids.add(reinterpret_cast<ID *>(texture));
+        }
+        break;
+      }
+      case SOCK_IMAGE: {
+        if (Image *image = ((bNodeSocketValueImage *)socket->default_value)->value) {
+          ids.add(reinterpret_cast<ID *>(image));
+        }
+        break;
+      }
+    }
+  }
+}
+
+/**
+ * \note We can only check properties here that cause the dependency graph to update relations when
+ * they are changed, otherwise there may be a missing relation after editing. So this could check
+ * more properties like whether the node is muted, but we would have to accept the cost of updating
+ * relations when those properties are changed.
+ */
+static bool node_needs_own_transform_relation(const bNode &node)
+{
+  if (node.type == GEO_NODE_COLLECTION_INFO) {
+    const NodeGeometryCollectionInfo &storage = *static_cast<const NodeGeometryCollectionInfo *>(
+        node.storage);
+    return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE;
+  }
+
+  if (node.type == GEO_NODE_OBJECT_INFO) {
+    const NodeGeometryObjectInfo &storage = *static_cast<const NodeGeometryObjectInfo *>(
+        node.storage);
+    return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE;
+  }
+
+  if (node.type == GEO_NODE_SELF_OBJECT) {
+    return true;
+  }
+  if (node.type == GEO_NODE_DEFORM_CURVES_ON_SURFACE) {
+    return true;
+  }
+
+  return false;
+}
+
+static void process_nodes_for_depsgraph(const bNodeTree &tree,
+                                        Set<ID *> &ids,
+                                        bool &r_needs_own_transform_relation,
+                                        Set<const bNodeTree *> &checked_groups)
+{
+  if (!checked_groups.add(&tree)) {
+    return;
+  }
+
+  tree.ensure_topology_cache();
+  for (const bNode *node : tree.all_nodes()) {
+    add_used_ids_from_sockets(node->inputs, ids);
+    add_used_ids_from_sockets(node->outputs, ids);
+    r_needs_own_transform_relation |= node_needs_own_transform_relation(*node);
+  }
+
+  for (const bNode *node : tree.group_nodes()) {
+    if (const bNodeTree *sub_tree = reinterpret_cast<const bNodeTree *>(node->id)) {
+      process_nodes_for_depsgraph(*sub_tree, ids, r_needs_own_transform_relation, checked_groups);
+    }
+  }
+}
+
+void find_node_tree_dependencies(const bNodeTree &tree,
+                           Set<ID *> &r_ids,
+                           bool &r_needs_own_transform_relation)
+{
+  Set<const bNodeTree *> checked_groups;
+  process_nodes_for_depsgraph(tree, r_ids, r_needs_own_transform_relation, checked_groups);
+}
+
 StringRef input_use_attribute_suffix()
 {
   return "_use_attribute";