Cleanup: Use utility function to find groups in node tree #104465

Merged
Hans Goudey merged 23 commits from mod_moder/blender:is_contains into main 2023-02-10 17:30:57 +01:00
7 changed files with 54 additions and 50 deletions

View File

@ -35,19 +35,10 @@ def draw_node_group_add_menu(context, layout):
if node_tree:
from nodeitems_builtins import node_tree_group_type
def contains_group(nodetree, group):
if nodetree == group:
return True
for node in nodetree.nodes:
if node.bl_idname in node_tree_group_type.values() and node.node_tree is not None:
if contains_group(node.node_tree, group):
return True
return False
groups = [
group for group in context.blend_data.node_groups
if (group.bl_idname == node_tree.bl_idname and
not contains_group(group, node_tree) and
not group.contains_tree(node_tree) and
not group.name.startswith('.'))
]
if groups:

View File

@ -76,21 +76,11 @@ def node_group_items(context):
yield NodeItemCustom(draw=lambda self, layout, context: layout.separator())
def contains_group(nodetree, group):
if nodetree == group:
return True
else:
for node in nodetree.nodes:
if node.bl_idname in node_tree_group_type.values() and node.node_tree is not None:
if contains_group(node.node_tree, group):
return True
return False
for group in context.blend_data.node_groups:
if group.bl_idname != ntree.bl_idname:
continue
# filter out recursive groups
if contains_group(group, ntree):
if group.contains_tree(ntree):
continue
# filter out hidden nodetrees
if group.name.startswith('.'):

View File

@ -504,7 +504,13 @@ struct bNodeTree *ntreeFromID(struct ID *id);
void ntreeFreeLocalNode(struct bNodeTree *ntree, struct bNode *node);
void ntreeFreeLocalTree(struct bNodeTree *ntree);
struct bNode *ntreeFindType(struct bNodeTree *ntree, int type);
bool ntreeHasTree(const struct bNodeTree *ntree, const struct bNodeTree *lookup);
/**
* Check recursively if a node tree contains another.
mod_moder marked this conversation as resolved
Review

Check recursively if a node tree contains another.

`Check recursively if a node tree contains another.`
*/
bool ntreeContainsTree(const struct bNodeTree *tree_to_search_in,
Review

I think it would be better to use names like tree_to_search_in and tree_to_search_for. I find that the most clear.

I think it would be better to use names like `tree_to_search_in` and `tree_to_search_for`. I find that the most clear.

This only for backend part, or in python is too?
parm = RNA_def_pointer(func, "sub_tree", "NodeTree", "Node Tree", "Node tree for recursive check");

This only for backend part, or in python is too? `parm = RNA_def_pointer(func, "sub_tree", "NodeTree", "Node Tree", "Node tree for recursive check");`
const struct bNodeTree *tree_to_search_for);
void ntreeUpdateAllNew(struct Main *main);
void ntreeUpdateAllUsers(struct Main *main, struct ID *id);

View File

@ -3457,21 +3457,41 @@ bNode *ntreeFindType(bNodeTree *ntree, int type)
return nullptr;
}
mod_moder marked this conversation as resolved
Review

This should internally avoid to enter the same node group multiple times. For very large node trees, with lots of nesting that could be quite heavy.

This should internally avoid to enter the same node group multiple times. For very large node trees, with lots of nesting that could be quite heavy.

When i was upload this as diff (https://archive.blender.org/developer/D16870) i pointed, that map (set) for avoiding already checked groups and output of groups stack can be added in future as speed improvement. You want to i do that in this request?

When i was upload this as diff (https://archive.blender.org/developer/D16870) i pointed, that map (set) for avoiding already checked groups and output of groups stack can be added in future as speed improvement. You want to i do that in this request?
Review

Ah I see, well either add this to the pull request description or just do the code change now already. Don't see much of a reason not to do it now already.

Ah I see, well either add this to the pull request description or just do the code change now already. Don't see much of a reason not to do it now already.
Timer 'Cache speed': (Average: 16473 ns, Min: 300 ns, Last: 44000 ns)
Timer 'Cache speed': (Average: 1100 ns, Min: 1100 ns, Last: 1100 ns)
Timer 'Cache speed': (Average: 0.2 ms, Min: 0.2 ms, Last: 0.2 ms)
Timer 'Normal speed': (Average: 2.1 ms, Min: 200 ns, Last: 136.1 ms) -- Open searcher
Timer 'Normal speed': (Average: 1200 ns, Min: 1200 ns, Last: 1200 ns) -- Insert other one group in this.
Timer 'Normal speed': (Average: 135.6 ms, Min: 135.6 ms, Last: 135.6 ms) -- Insert this group in other one
``` Timer 'Cache speed': (Average: 16473 ns, Min: 300 ns, Last: 44000 ns) Timer 'Cache speed': (Average: 1100 ns, Min: 1100 ns, Last: 1100 ns) Timer 'Cache speed': (Average: 0.2 ms, Min: 0.2 ms, Last: 0.2 ms) ``` ``` Timer 'Normal speed': (Average: 2.1 ms, Min: 200 ns, Last: 136.1 ms) -- Open searcher Timer 'Normal speed': (Average: 1200 ns, Min: 1200 ns, Last: 1200 ns) -- Insert other one group in this. Timer 'Normal speed': (Average: 135.6 ms, Min: 135.6 ms, Last: 135.6 ms) -- Insert this group in other one ```
bool ntreeHasTree(const bNodeTree *ntree, const bNodeTree *lookup)
static bool ntree_contains_tree_exec(const bNodeTree *tree_to_search_in,
const bNodeTree *tree_to_search_for,
Set<const bNodeTree *> &already_passed)
{
if (ntree == lookup) {
if (tree_to_search_in == tree_to_search_for) {
mod_moder marked this conversation as resolved
Review

reinterpret_cast<bNodeTree *> -> reinterpret_cast<const bNodeTree *>

`reinterpret_cast<bNodeTree *>` -> `reinterpret_cast<const bNodeTree *>`
return true;
}
for (const bNode *node : ntree->all_nodes()) {
if (ELEM(node->type, NODE_GROUP, NODE_CUSTOM_GROUP) && node->id) {
if (ntreeHasTree((bNodeTree *)node->id, lookup)) {
return true;
}
tree_to_search_in->ensure_topology_cache();
for (const bNode *node_group : tree_to_search_in->group_nodes()) {
const bNodeTree *sub_tree_search_in = reinterpret_cast<const bNodeTree *>(node_group->id);
if (!sub_tree_search_in) {
continue;
}
if (!already_passed.add(sub_tree_search_in)) {
continue;
}
if (ntree_contains_tree_exec(sub_tree_search_in, tree_to_search_for, already_passed)) {
return true;
}
}
return false;
}
bool ntreeContainsTree(const bNodeTree *tree_to_search_in, const bNodeTree *tree_to_search_for)
{
if (tree_to_search_in == tree_to_search_for) {
return true;
}
Set<const bNodeTree *> already_passed;
return ntree_contains_tree_exec(tree_to_search_in, tree_to_search_for, already_passed);
}
bNodeLink *nodeFindLink(bNodeTree *ntree, const bNodeSocket *from, const bNodeSocket *to)
{
LISTBASE_FOREACH (bNodeLink *, link, &ntree->links) {

View File

@ -715,7 +715,7 @@ void ED_node_set_active(
if ((node->flag & NODE_ACTIVE_TEXTURE) && !was_active_texture) {
/* If active texture changed, free glsl materials. */
LISTBASE_FOREACH (Material *, ma, &bmain->materials) {
if (ma->nodetree && ma->use_nodes && ntreeHasTree(ma->nodetree, ntree)) {
if (ma->nodetree && ma->use_nodes && ntreeContainsTree(ma->nodetree, ntree)) {
GPU_material_free(&ma->gpumaterial);
/* Sync to active texpaint slot, otherwise we can end up painting on a different slot
@ -734,7 +734,7 @@ void ED_node_set_active(
}
LISTBASE_FOREACH (World *, wo, &bmain->worlds) {
if (wo->nodetree && wo->use_nodes && ntreeHasTree(wo->nodetree, ntree)) {
if (wo->nodetree && wo->use_nodes && ntreeContainsTree(wo->nodetree, ntree)) {
GPU_material_free(&wo->gpumaterial);
}
}

View File

@ -1094,24 +1094,6 @@ void NODE_OT_group_make(wmOperatorType *ot)
/** \name Group Insert Operator
* \{ */
static bool node_tree_contains_tree_recursive(const bNodeTree &ntree_to_search_in,
const bNodeTree &ntree_to_search_for)
{
if (&ntree_to_search_in == &ntree_to_search_for) {
return true;
}
ntree_to_search_in.ensure_topology_cache();
for (const bNode *node : ntree_to_search_in.group_nodes()) {
if (node->id) {
if (node_tree_contains_tree_recursive(*reinterpret_cast<bNodeTree *>(node->id),
ntree_to_search_for)) {
return true;
}
}
}
return false;
}
static int node_group_insert_exec(bContext *C, wmOperator *op)
{
SpaceNode *snode = CTX_wm_space_node(C);
@ -1133,7 +1115,7 @@ static int node_group_insert_exec(bContext *C, wmOperator *op)
if (!group->is_group() || group->id == nullptr) {
continue;
}
if (node_tree_contains_tree_recursive(*reinterpret_cast<bNodeTree *>(group->id), *ngroup)) {
if (ntreeContainsTree(reinterpret_cast<bNodeTree *>(group->id), ngroup)) {
BKE_reportf(
op->reports, RPT_WARNING, "Can not insert group '%s' in '%s'", group->name, gnode->name);
return OPERATOR_CANCELLED;

View File

@ -1435,6 +1435,11 @@ static void rna_NodeTree_active_output_set(PointerRNA *ptr, int value)
}
}
static bool rna_NodeTree_contains_tree(bNodeTree *tree, bNodeTree *sub_tree)
{
return ntreeContainsTree(tree, sub_tree);
}
static bNodeSocket *rna_NodeTree_inputs_new(
bNodeTree *ntree, Main *bmain, ReportList *reports, const char *type, const char *name)
{
@ -12719,6 +12724,16 @@ static void rna_def_nodetree(BlenderRNA *brna)
parm = RNA_def_pointer(func, "context", "Context", "", "");
RNA_def_parameter_flags(parm, PROP_NEVER_NULL, PARM_REQUIRED);
func = RNA_def_function(srna, "contains_tree", "rna_NodeTree_contains_tree");
RNA_def_function_ui_description(
func,
"Check if the node tree contains another. Used to avoid creating recursive node groups");
parm = RNA_def_pointer(
func, "sub_tree", "NodeTree", "Node Tree", "Node tree for recursive check");
mod_moder marked this conversation as resolved
Review

Currently this uses the term group and tree. While we generally use those terms synonymously, it would be better to be consistent within a single function declaration.

Currently this uses the term `group` and `tree`. While we generally use those terms synonymously, it would be better to be consistent within a single function declaration.
RNA_def_parameter_flags(parm, PROP_NEVER_NULL, PARM_REQUIRED);
parm = RNA_def_property(func, "contained", PROP_BOOLEAN, PROP_NONE);
RNA_def_function_return(func, parm);
/* registration */
prop = RNA_def_property(srna, "bl_idname", PROP_STRING, PROP_NONE);
RNA_def_property_string_sdna(prop, NULL, "typeinfo->idname");