blender
/
blender
134
399
Fork 583
Code Issues 6.3k Pull Requests 753 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 +25 -22
1 changed files with 25 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -2053,19 +2053,22 @@ bool nodeFindNodeTry(bNodeTree *ntree, bNodeSocket *sock, bNode **r_node, int *r
bNode *nodeFindRootParent(bNode *node)
{
if (node->parent) {
return nodeFindRootParent(node->parent);
bNode *parent_iter = node;
while (parent_iter->parent != nullptr) {
parent_iter = parent_iter->parent;
}
return node->type == NODE_FRAME ? node : nullptr;
if (parent_iter->type != NODE_FRAME) {
return nullptr;
}
return parent_iter;
}
bool nodeIsParentAndChild(const bNode *parent, const bNode *child)
{
if (parent == child) {
return true;
}
if (child->parent) {
return nodeIsParentAndChild(parent, child->parent);
for (const bNode *child_iter = child; child_iter; child_iter = child_iter->parent) {
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 (child_iter == parent) {
return true;
}
}
return false;
}
@ -2551,26 +2554,26 @@ void nodeInternalRelink(bNodeTree *ntree, bNode *node)
void nodeToView(const bNode *node, const float x, const float y, float *rx, float *ry)
{
if (node->parent) {
nodeToView(node->parent, x + node->locx, y + node->locy, rx, ry);
}
else {
*rx = x + node->locx;
*ry = y + node->locy;
float mapping_x = 0.0f;
float mapping_y = 0.0f;
for (const bNode *node_iter = node; node_iter; node_iter = node_iter->parent) {
mapping_x += node_iter->locx;
mapping_y += node_iter->locy;
}
*rx = mapping_x + x;
*ry = mapping_y + y;
}
void nodeFromView(const bNode *node, const float x, const float y, float *rx, float *ry)
{
if (node->parent) {
nodeFromView(node->parent, x, y, rx, ry);
*rx -= node->locx;
*ry -= node->locy;
}
else {
*rx = x - node->locx;
*ry = y - node->locy;
float mapping_x = 0.0f;
float mapping_y = 0.0f;
for (const bNode *node_iter = node; node_iter; node_iter = node_iter->parent) {
mapping_x += node_iter->locx;
mapping_y += node_iter->locy;
}
*rx = -mapping_x + x;
*ry = -mapping_y + y;
}
void nodeAttachNode(bNodeTree *ntree, bNode *node, bNode *parent)