From 2893dc8ab7bbf9b2079c153edd307176b3e4210f Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Tue, 31 Oct 2023 17:16:48 +0100 Subject: [PATCH] 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 --- .../editors/geometry/node_group_operator.cc | 55 ++++++++-- source/blender/modifiers/intern/MOD_nodes.cc | 96 +---------------- .../nodes/NOD_geometry_nodes_execute.hh | 5 + .../nodes/intern/geometry_nodes_execute.cc | 100 ++++++++++++++++++ 4 files changed, 150 insertions(+), 106 deletions(-) diff --git a/source/blender/editors/geometry/node_group_operator.cc b/source/blender/editors/geometry/node_group_operator.cc index 3006377d9ce..75312c8ccdb 100644 --- a/source/blender/editors/geometry/node_group_operator.cc +++ b/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(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 objects) +{ + Set 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 ids; + ids.append(&node_tree_orig.id); + ids.extend(objects.cast()); + 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(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( + DEG_get_evaluated_id(depsgraph, const_cast(&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; } diff --git a/source/blender/modifiers/intern/MOD_nodes.cc b/source/blender/modifiers/intern/MOD_nodes.cc index 28403304dc0..1a1263b096f 100644 --- a/source/blender/modifiers/intern/MOD_nodes.cc +++ b/source/blender/modifiers/intern/MOD_nodes.cc @@ -112,98 +112,6 @@ static void init_data(ModifierData *md) nmd->runtime->cache = std::make_shared(); } -static void add_used_ids_from_sockets(const ListBase &sockets, Set &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( - node.storage); - return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE; - } - - if (node.type == GEO_NODE_OBJECT_INFO) { - const NodeGeometryObjectInfo &storage = *static_cast( - 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 &ids, - bool &r_needs_own_transform_relation, - Set &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(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 &ids) { IDP_foreach_property( @@ -259,9 +167,7 @@ static void update_depsgraph(ModifierData *md, const ModifierUpdateDepsgraphCont bool needs_own_transform_relation = false; Set used_ids; find_used_ids_from_settings(nmd->settings, used_ids); - Set 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(ctx->object->data); diff --git a/source/blender/nodes/NOD_geometry_nodes_execute.hh b/source/blender/nodes/NOD_geometry_nodes_execute.hh index 2e269409201..54952203324 100644 --- a/source/blender/nodes/NOD_geometry_nodes_execute.hh +++ b/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 &r_ids, + bool &r_needs_own_transform_relation); + StringRef input_use_attribute_suffix(); StringRef input_attribute_name_suffix(); diff --git a/source/blender/nodes/intern/geometry_nodes_execute.cc b/source/blender/nodes/intern/geometry_nodes_execute.cc index 4dc5b862a95..1fbd35f13a9 100644 --- a/source/blender/nodes/intern/geometry_nodes_execute.cc +++ b/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 &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(object)); + } + break; + } + case SOCK_COLLECTION: { + if (Collection *collection = ((bNodeSocketValueCollection *)socket->default_value)->value) + { + ids.add(reinterpret_cast(collection)); + } + break; + } + case SOCK_MATERIAL: { + if (Material *material = ((bNodeSocketValueMaterial *)socket->default_value)->value) { + ids.add(reinterpret_cast(material)); + } + break; + } + case SOCK_TEXTURE: { + if (Tex *texture = ((bNodeSocketValueTexture *)socket->default_value)->value) { + ids.add(reinterpret_cast(texture)); + } + break; + } + case SOCK_IMAGE: { + if (Image *image = ((bNodeSocketValueImage *)socket->default_value)->value) { + ids.add(reinterpret_cast(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( + node.storage); + return storage.transform_space == GEO_NODE_TRANSFORM_SPACE_RELATIVE; + } + + if (node.type == GEO_NODE_OBJECT_INFO) { + const NodeGeometryObjectInfo &storage = *static_cast( + 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 &ids, + bool &r_needs_own_transform_relation, + Set &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(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 &r_ids, + bool &r_needs_own_transform_relation) +{ + Set checked_groups; + process_nodes_for_depsgraph(tree, r_ids, r_needs_own_transform_relation, checked_groups); +} + StringRef input_use_attribute_suffix() { return "_use_attribute";