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

Iliya Katushenock · 2023-03-03T06:54:40+01:00

Iliya Katushenock commented

2023-03-03 06:54:40 +01:00

Cleaning. Don't call recursion where it's redundant.
The recursive algorithm can carry dangerous behavior
due to stack growth and overflow. The probability is
low for something like the frame nodes. But using a
loop is cheap, providing O(N = const) memory cost.

A loop through the links in a singly linked list is sufficient.
The use of 2d vectors for location mapping and other things can be separate.

Cleaning. Don't call recursion where it's redundant. The recursive algorithm can carry dangerous behavior due to stack growth and overflow. The probability is low for something like the frame nodes. But using a loop is cheap, providing O(N = const) memory cost. A loop through the links in a singly linked list is sufficient. The use of 2d vectors for location mapping and other things can be separate.

Iliya Katushenock added 16 commits 2023-03-03 06:54:50 +01:00

c6b68d52c4 cleanup: try to use const allways

37edaecb50 Merge branch 'main' into use_const_in_bke_node_cc

198d6a876f Merge branch 'main' into use_const_in_bke_node_cc

90c1420a0c merge with main, delete const's for by-valuy passed args in declarations, other changes

1c2410f01e Merge branch 'main' into use_const_in_bke_node_cc

31a251b785 Merge branch 'use_const_in_bke_node_cc' of projects.blender.org:mod_moder/blender into use_const_in_bke_node_cc

0842acd101 delete one missing/

7936795011 delete one missing//

fd47bdba91 Merge branch 'main' into use_const_in_bke_node_cc

d4a14a605f Merge branch 'main' into use_const_in_bke_node_cc

97cc782cbc Merge branch 'main' into use_const_in_bke_node_cc

f38f25f4f4 delete nodeUnlinkNode from this commit

617f383216 Merge branch 'main' into use_const_in_bke_node_cc

652360c42c init commit

52bf2eb5b4 Merge branch 'main' into use_const_in_bke_node_cc

458a3df5b0 fix merge conflict

Iliya Katushenock added 1 commit 2023-03-03 07:02:50 +01:00

15d9f70010 fix merge conflict in merge main

Iliya Katushenock added this to the Nodes & Physics project 2023-03-03 07:03:04 +01:00

Iliya Katushenock requested review from Hans Goudey 2023-03-03 07:03:11 +01:00

Leon Schittek reviewed 2023-03-03 10:14:38 +01:00

source/blender/blenkernel/intern/node.cc

						
				@ -2060,3 +2065,4 @@

				  return t_node;

				}

				bool nodeIsChildOf(const bNode *parent, const bNode *child)

Leon Schittek commented

2023-03-03 10:14:38 +01:00

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

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

@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

mod_moder marked this conversation as resolved

Iliya Katushenock added 1 commit 2023-03-03 19:24:46 +01:00

c9e74dfc4b Merge branch 'main' into cleanup_bke_nodes_unwrap_recursion

Iliya Katushenock added 1 commit 2023-03-03 19:58:56 +01:00

998a044cf8 some vars renaming, reuse nodeIsChildOf in nodeAttachNodeCheck

Iliya Katushenock referenced this pull request

2023-03-03 21:08:48 +01:00

Cleanup: Deduplicate node parent checking functions #105413

Hans Goudey commented

2023-03-03 23:04:54 +01:00

It would be nice for the description to mention why removing recursion is better.

👍 1

Iliya Katushenock added 1 commit 2023-03-04 01:37:15 +01:00

8cc0fdd697 merge main, fix merge conflict

Iliya Katushenock added 1 commit 2023-03-04 01:38:05 +01:00

11ff4d517d Merge branch 'main' into cleanup_bke_nodes_unwrap_recursion

Iliya Katushenock added 2 commits 2023-03-05 17:28:05 +01:00

5c80f637e7 Merge branch 'main' into cleanup_bke_nodes_unwrap_recursion

ed5d5c9fed some little changes

Iliya Katushenock referenced this pull request

2023-03-05 17:46:10 +01:00

Cleanup: nodeToView & nodeFromView usage #105469

Hans Goudey requested changes 2023-03-05 23:21:51 +01:00

Hans Goudey left a comment

Looks like an improvement.
This is a bit nitpicky, but I don't think the whitespace (blank lines) you added to the functions is necessary. Better to avoid it and keep the functions shorter. These are really small blocks of code, so separating them more isn't so helpful.

Looks like an improvement. This is a bit nitpicky, but I don't think the whitespace (blank lines) you added to the functions is necessary. Better to avoid it and keep the functions shorter. These are really small blocks of code, so separating them more isn't so helpful.

Iliya Katushenock added 2 commits 2023-03-06 18:31:02 +01:00

627ae6162c Merge branch 'main' into cleanup_bke_nodes_unwrap_recursion

63b717b40f delete extra whitespaces

Hans Goudey approved these changes 2023-03-06 18:37:42 +01:00

Hans Goudey merged commit b756915206 into main

2023-03-06 18:39:30 +01:00

Hans Goudey referenced this issue from a commit

2023-03-06 18:39:30 +01:00

Cleanup: Avoid recursion for node parenting

Iliya Katushenock deleted branch cleanup_bke_nodes_unwrap_recursion

2023-03-06 18:44:26 +01:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

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