Cleanup: Reduce nesteds in node.cc #105509

Iliya Katushenock · 2023-03-06T20:57:21+01:00

Iliya Katushenock commented

2023-03-06 20:57:21 +01:00

Decompose most of the nesting in the code to make the code
more consistent along the line of program execution.
Mainly achieved through:

Remove redundant Else
Invert Condition
Add temporary variable to redistribute and name conditions

Decompose most of the nesting in the code to make the code more consistent along the line of program execution. Mainly achieved through: - Remove redundant Else - Invert Condition - Add temporary variable to redistribute and name conditions

Iliya Katushenock added 1 commit 2023-03-06 20:57:31 +01:00

c564f9d138 init commit

Iliya Katushenock added 1 commit 2023-03-06 21:10:28 +01:00

ee933cf0b6 cleaning of InternalRelink

Iliya Katushenock added this to the Nodes & Physics project 2023-03-06 21:10:40 +01:00

Iliya Katushenock requested review from Hans Goudey 2023-03-06 21:10:45 +01:00

Iliya Katushenock added 1 commit 2023-03-07 20:35:24 +01:00

8fa5f65d6b Merge branch 'main' into cleanup_reduce_nesteds

Hans Goudey requested changes 2023-03-08 21:45:53 +01:00

Hans Goudey left a comment

Thanks for the PR. To be honest though, it's quite hard to justify spend time reviewing this sort of cleanup. Compared to the amount of time review will take, the benefit is quite low. And the patch is large enough that it becomes a bit overwhelming about half-way through.

source/blender/blenkernel/intern/node.cc Outdated

						
				@ -1409,0 +1407,4 @@

				    return true;

				  }

				  if (!node->is_group()) {

Hans Goudey commented

2023-03-08 21:42:38 +01:00

Better to keep all of the checks that are only relevant to a group node in one block:

if (node->is_group()) {
  if (group_invalid_1) {
    return false;
  }
  if (group_invalid_2) {
    return false;
  }
}

if (some other invalid check in the future) {
  return false;
}

return true;

Better to keep all of the checks that are only relevant to a group node in one block: ```cpp if (node->is_group()) { if (group_invalid_1) { return false; } if (group_invalid_2) { return false; } } if (some other invalid check in the future) { return false; } return true;

mod_moder marked this conversation as resolved

source/blender/blenkernel/intern/node.cc Outdated

						
				@ -2096,1 +2106,3 @@

				            ntree, reversed ? link->fromnode : link->tonode, callback, userdata, reversed);

				    /* TODO (mod_moder): This case less likely than the case below. Very less likely. Can swap.

				     * Need a benchmark.*/

				    if (ELEM(nullptr, link->tonode, link->fromnode)) {

Hans Goudey commented

2023-03-08 21:42:34 +01:00

Since recently, we can assume that both tonode and fromnode will be non-null. This check is left over from the time when the currently dragged links were also stored in the node tree.

Since recently, we can assume that both `tonode` and `fromnode` will be non-null. This check is left over from the time when the currently dragged links were also stored in the node tree.

mod_moder marked this conversation as resolved

source/blender/blenkernel/intern/node.cc Outdated

						
				@ -2244,2 +2280,3 @@

				    const char *disabled_hint;

				    if (ntype->type == type && (!ntype->poll || ntype->poll(ntype, ntree, &disabled_hint))) {

				    const bool can_by_created = ntype->poll(ntype, ntree, &disabled_hint);

Hans Goudey commented

2023-03-08 21:44:29 +01:00

I think the code was easier to read with ntype->poll && !ntype->poll(...) on the same line.

I think the code was easier to read with `ntype->poll && !ntype->poll(...)` on the same line.

mod_moder marked this conversation as resolved

Iliya Katushenock commented

2023-03-09 20:15:45 +01:00

I would like to complete my list of things that can be cleared in this file though (I can send it to you for approval if you want).
Reducing nesting is very useful for simplifying code. Already now it allows you to remove old unused things. Also, there is one TODO (maybe not noticed by you) that could potentially make a strong optimization (albeit not a very heavily loaded place in the code).
So this seems to have some potential for performance improvements as well.
In addition, it is also a fix for potential bugs.

I would like to complete my list of things that can be cleared in this file though (I can send it to you for approval if you want). Reducing nesting is very useful for simplifying code. Already now it allows you to remove old unused things. Also, there is one TODO (maybe not noticed by you) that could potentially make a strong optimization (albeit not a very heavily loaded place in the code). So this seems to have some potential for performance improvements as well. In addition, it is also a fix for potential bugs.

Iliya Katushenock added 2 commits 2023-03-09 20:53:15 +01:00

64c8585e0b Merge branch 'main' into cleanup_reduce_nesteds

b72c7ce7b6 several requested changes

Iliya Katushenock requested review from Hans Goudey 2023-03-10 09:44:43 +01:00

Iliya Katushenock added 1 commit 2023-03-10 09:45:13 +01:00

f9f051b9cb Merge branch 'main' into cleanup_reduce_nesteds

Iliya Katushenock added 1 commit 2023-03-20 11:13:52 +01:00

e14a74501b Merge branch 'main' into cleanup_reduce_nesteds

Iliya Katushenock added 1 commit 2023-03-22 21:28:00 +01:00

de40b9bfa6 Merge branch 'main' into cleanup_reduce_nesteds

Iliya Katushenock added 2 commits 2023-04-01 22:16:51 +02:00

0bddb3afc2 Merge branch 'main' into cleanup_reduce_nesteds

f1bb598a00 delete todo

Iliya Katushenock added 1 commit 2023-04-16 16:52:28 +02:00

bf8df15144 Merge branch 'main' into cleanup_reduce_nesteds

Hans Goudey approved these changes 2023-04-16 21:54:52 +02:00

source/blender/blenkernel/intern/node.cc Outdated

						
				@ -3748,0 +3758,4 @@

				  if (!stype.use_link_limits_of_type) {

				    return sock->limit;

				  }

				  if (sock->in_out == SOCK_IN) {

Hans Goudey commented

2023-04-16 21:54:32 +02:00

Personally I find this easier to read than what you've done here. It's a simple check and splitting it over multiple lines makes it seem more complex than it is.
return sock->in_out == SOCK_IN ? stype.input_link_limit : stype.output_link_limit;

Personally I find this easier to read than what you've done here. It's a simple check and splitting it over multiple lines makes it seem more complex than it is. ` return sock->in_out == SOCK_IN ? stype.input_link_limit : stype.output_link_limit;`

mod_moder marked this conversation as resolved

Iliya Katushenock added 1 commit 2023-04-16 22:40:32 +02:00

814b9c36d7 undo that line

Hans Goudey merged commit 9d4949f80b into main

2023-04-16 22:53:20 +02:00

Hans Goudey referenced this issue from a commit

2023-04-16 22:53:21 +02:00

Cleanup: Reduce nesting in node.cc

Iliya Katushenock deleted branch cleanup_reduce_nesteds

2023-04-16 22:56:29 +02: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: Reduce nesteds in node.cc #105509