blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 780 Projects 19 Wiki Activity

Cleanup: Don't do recursion where possible in node.cc #105394

Merged
Hans Goudey merged 25 commits from mod_moder/blender:cleanup_bke_nodes_unwrap_recursion into main 2023-03-06 18:39:30 +01:00
Conversation 1 Commits 25 Files Changed 1 +11 -31
1 changed files with 11 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 998a044cf8 - Show all commits

42
source/blender/blenkernel/intern/node.cc
View File

@ -2053,29 +2053,22 @@ bool nodeFindNodeTry(bNodeTree *ntree, bNodeSocket *sock, bNode **r_node, int *r
bNode *nodeFindRootParent(bNode *node)
{
bNode *t_node = node;
while (t_node->parent != nullptr) {
t_node = t_node->parent;
bNode *parent_iter = node;
while (parent_iter->parent != nullptr) {
parent_iter = parent_iter->parent;
}
if (t_node->type != NODE_FRAME) {
if (parent_iter->type != NODE_FRAME) {
return nullptr;
}
return t_node;
return parent_iter;
}
bool nodeIsChildOf(const bNode *parent, const bNode *child)
mod_moder marked this conversation as resolved
Leon Schittek commented 2023-03-03 10:14:38 +01:00
Review
Copy Link

I think nodeIsChildOf and nodeAttachNodeCheck do the same thing - just the order of child and parent in the function parameters is different.

Maybe this can be deduplicated? I think the naming nodeIsChildOf is much better anyway. nodeAttachNodeCheck doesn't really tell you what it's doing.

I think `nodeIsChildOf` and `nodeAttachNodeCheck` do the same thing - just the order of child and parent in the function parameters is different. Maybe this can be deduplicated? I think the naming `nodeIsChildOf` is much better anyway. `nodeAttachNodeCheck` doesn't really tell you what it's doing.
Iliya Katushenock commented 2023-03-03 13:00:23 +01:00
Review
Copy Link

Hello, yes, I am also a little frustrated by this state of affairs.

It seems like it would be much better to put the data about parenting in a separate structure that would belong to the node tree ... although I don't see much benefit here either /

The difference is that one loop just looks for the root and the other does the comparison of each element in the process...

The most general solution would be a template iterator over all nodes of this singly linked list.
But here I'm not sure that for the sake of 4 functions of working with parents, we need to make an iterator function for all parents, like something like

template<typename Node, typename Function>
Node &node_for_each_parent(Node &node, Function func){
    Node *t_node = &node;
    while(t_node->parent != nullptr){
        t_node = t_node->parent;
        func(*t_node)
    }
    return *t_node
}

bNode *nodeFindRootParent (bNode *node){
    return node_for_each_parent(*node, [](const bNode & /*t_parent*/){});
}

bool nodeIsChildOf (const bNode *parent, const bNode *child){
    if (parent == child){
        return true;
    }

    node_for_each_parent(*child, [parent](const bNode &t_parent){
        if (parent == &t_parent) {
            return true;
        }
    });

    return false;
}

static void nodeViewMapping(const bNode &node, float &mapping_x, float &mapping_y)
{
    mapping_x += node.locx;
    mapping_y += node.locy;

    node_for_each_parent(node, [mapping_x, mapping_y](const bNode &t_parent){
        mapping_x += t_parent.locx;
        mapping_y += t_parent.locy;
    });
}
Hello, yes, I am also a little frustrated by this state of affairs. It seems like it would be much better to put the data about parenting in a separate structure that would belong to the node tree ... although I don't see much benefit here either / The difference is that one loop just looks for the root and the other does the comparison of each element in the process... The most general solution would be a template iterator over all nodes of this singly linked list. But here I'm not sure that for the sake of 4 functions of working with parents, we need to make an iterator function for all parents, like something like ``` template<typename Node, typename Function> Node &node_for_each_parent(Node &node, Function func){ Node *t_node = &node; while(t_node->parent != nullptr){ t_node = t_node->parent; func(*t_node) } return *t_node } ``` ``` bNode *nodeFindRootParent (bNode *node){ return node_for_each_parent(*node, [](const bNode & /*t_parent*/){}); } ``` ``` bool nodeIsChildOf (const bNode *parent, const bNode *child){ if (parent == child){ return true; } node_for_each_parent(*child, [parent](const bNode &t_parent){ if (parent == &t_parent) { return true; } }); return false; } ``` ``` static void nodeViewMapping(const bNode &node, float &mapping_x, float &mapping_y) { mapping_x += node.locx; mapping_y += node.locy; node_for_each_parent(node, [mapping_x, mapping_y](const bNode &t_parent){ mapping_x += t_parent.locx; mapping_y += t_parent.locy; }); } ```
Iliya Katushenock commented 2023-03-03 19:23:23 +01:00
Review
Copy Link

@lone_noel I once again delved into the code, and realized that I was blind and reading your comment, i simply did not understand that you pointed to a function that I did not even touch on in this PR!
I'm looking at the related code, and I'm having a nightmare with parts that are always inaccessible and so on... well, I'll try to look into it. But for now, it seems like it could be a separate cleanup.

@lone_noel I once again delved into the code, and realized that I was blind and reading your comment, i simply did not understand that you pointed to a function that I did not even touch on in this PR! I'm looking at the related code, and I'm having a nightmare with parts that are always inaccessible and so on... well, I'll try to look into it. But for now, it seems like it could be a separate cleanup.
👍 1
{
if (parent == child) {
return true;
}
const bNode *t_child = child;
while (t_child->parent != nullptr) {
t_child = t_child->parent;
if (t_child == parent) {
for (const bNode *parent_iter = child; parent_iter; parent_iter = parent_iter->parent) {
if (parent_iter == parent) {
return true;
}
}
@ -2567,16 +2560,9 @@ void nodeInternalRelink(bNodeTree *ntree, bNode *node)
*/
static void nodeViewMapping(const bNode *node, float &mapping_x, float &mapping_y)
{
const bNode *t_node = node;
mapping_x += t_node->locx;
mapping_y += t_node->locy;
while (t_node->parent != nullptr) {
t_node = t_node->parent;
mapping_x += t_node->locx;
mapping_y += t_node->locy;
for (const bNode *node_iter = node; node_iter; node_iter = node_iter->parent) {
mapping_x += node_iter->locx;
mapping_y += node_iter->locy;
}
}
@ -2602,13 +2588,7 @@ void nodeFromView(const bNode *node, const float x, const float y, float *rx, fl
bool nodeAttachNodeCheck(const bNode *node, const bNode *parent)
{
for (const bNode *parent_iter = node; parent_iter; parent_iter = parent_iter->parent) {
if (parent_iter == parent) {
return true;
}
}
return false;
return nodeIsChildOf(parent, node);
}
void nodeAttachNode(bNodeTree *ntree, bNode *node, bNode *parent)