Loop Nodes #4

Denys Hsu · 2023-06-12T16:39:09+02:00

Denys Hsu commented

2023-06-12 16:39:09 +02:00

What's supported:

Loops attached to each other
Nested Loops
Calculations within loops
Forward Solver

Untested / out of scope:

Setting attributes within loops
Backward Solver

Concept:
Once a loop is found, it gets executed in one (recursive) call (if it contains nested loops).

...At first I attempted to make loops separated from the main loop event. While independent loops are easy to handle when standing alone, they lead to a lot of edge cases especially when nested.

For debugging purposes currently the "iteration" attribute of the nodes is visible in developer mode and can be reseted with the "Prepare Nodes" button. Also the example is mainly for debugging.

Future PR: "Group Inputs/Outputs" for loops so it's possible to have by example have "3x vectors" as "one" input and "2x rotations" as "one" output.

What's supported: - Loops attached to each other - Nested Loops - Calculations within loops - Forward Solver Untested / out of scope: - Setting attributes within loops - Backward Solver Concept: Once a loop is found, it gets executed in one (recursive) call (if it contains nested loops). ...At first I attempted to make loops separated from the main loop event. While independent loops are easy to handle when standing alone, they lead to a lot of edge cases especially when nested. For debugging purposes currently the "iteration" attribute of the nodes is visible in developer mode and can be reseted with the "Prepare Nodes" button. Also the example is mainly for debugging. Future PR: "Group Inputs/Outputs" for loops so it's possible to have by example have "3x vectors" as "one" input and "2x rotations" as "one" output.

LoopNode.png

129 KiB

Denys Hsu added 3 commits 2023-06-12 16:39:11 +02:00

init loop nodes 9eccd9d8aa

allow deep nesting ce870e8f61

object node and refactoring 764420af01

Denys Hsu changed title from ~~WIP: Loop Nodes~~ to Loop Nodes

2023-06-12 16:50:30 +02:00

Sybren A. Stüvel requested review from Sybren A. Stüvel 2023-06-13 10:08:02 +02:00

Sybren A. Stüvel requested changes 2023-06-13 11:51:27 +02:00

Sybren A. Stüvel left a comment

Thansk for the PR, it's super interesting. I couldn't do a full review yet, but this at least covers some superficial things I noticed while reading through the code.

To me the required changes you had to do to implement this shows that the current code should be refactored. I think the RigNodesNodeTree and AbstractRigNodesNode are getting too big and too coupled when it comes to the execution flow. Probably best to extract that to a class that's responsible for the overall tree execution. That would be for a different PR though. Just wanted to mention this as it's been on the back of my mind for a while now.

Thansk for the PR, it's super interesting. I couldn't do a full review yet, but this at least covers some superficial things I noticed while reading through the code. To me the required changes you had to do to implement this shows that the current code should be refactored. I think the `RigNodesNodeTree` and `AbstractRigNodesNode` are getting too big and too coupled when it comes to the execution flow. Probably best to extract that to a class that's responsible for the overall tree execution. That would be for a different PR though. Just wanted to mention this as it's been on the back of my mind for a while now.

nodes.py

						
				@ -72,0 +76,4 @@

				        output_nodes = list()

				        for node in self.nodes:

				            if isinstance(node, LoopOutputNode):

				                output_nodes.append(node)

Sybren A. Stüvel commented

2023-06-13 11:35:58 +02:00

This can be written as:

output_nodes = [node for node in self.nodes
                if isinstance(node, LoopOutputNode)]

And since it's only being iterated over once, there even no need to construct a list, so a generator expression does the trick as well:

output_nodes = (node for node in self.nodes
                if isinstance(node, LoopOutputNode))

This can be written as: ```py output_nodes = [node for node in self.nodes if isinstance(node, LoopOutputNode)] ``` And since it's only being iterated over once, there even no need to construct a list, so a generator expression does the trick as well: ```py output_nodes = (node for node in self.nodes if isinstance(node, LoopOutputNode)) ```

cgtinker marked this conversation as resolved

nodes.py

						
				@ -72,0 +82,4 @@

				        for output_node in output_nodes:

				            input_node = output_node._get_connection_node()

				            nodes = list(output_node._get_loop_nodes(input_node))

				            for node in nodes:

Sybren A. Stüvel commented

2023-06-13 11:38:07 +02:00

There is no need to construct a list here, just loop over the generator returned by output_node._get_loop_nodes()

There is no need to construct a list here, just loop over the generator returned by `output_node._get_loop_nodes()`

cgtinker marked this conversation as resolved

nodes.py

						
				@ -72,0 +96,4 @@

				    ) -> deque["AbstractRigNodesNode"]:

				        """Runs (nested) loops.

				        :return: Node(s) to visit next."""

Sybren A. Stüvel commented

2023-06-13 11:40:51 +02:00

Formatting: when writing multi-line docstrings, the closing """ should be on a line of its own (see PEP 257)

Formatting: when writing multi-line docstrings, the closing `"""` should be on a line of its own (see [PEP 257](https://peps.python.org/pep-0257/#multi-line-docstrings))

Denys Hsu commented

2023-06-13 13:59:12 +02:00

Sorry, still stumbling.

cgtinker marked this conversation as resolved

nodes.py

						
				@ -200,2 +293,4 @@

				        return sock

				    @property

				    def _inputs(self):

Sybren A. Stüvel commented

2023-06-13 11:48:12 +02:00

What's the reason to have these properties? They don't seem to add anything.

Denys Hsu commented

2023-06-13 13:34:42 +02:00

Those properties are for the loop nodes. The idea has been to just "ignore the first socket" of the loop nodes by default so I can easily figure out what's the connected loop node have an easier time searching for nodes within loop. To do so, the loop nodes return:
self.inputs[1:]

The bpy.types.Node type seems to overwrite properties it introduces. I'd have preferred to do something like:

@bpy.types.Nodes.inputs.getter or @AbstractRigNodesNode.inputs.getter
def inputs(self):
   ...

I also considered to overwriting the generators...

ED: I can remove the properties and just overwrite the exec_order_prerequisites for the LoopInputNode, that's probably cleaner.

Those properties are for the loop nodes. The idea has been to just "ignore the first socket" of the loop nodes by default so I can easily figure out what's the connected loop node have an easier time searching for nodes within loop. To do so, the loop nodes return: `self.inputs[1:]` The bpy.types.Node type seems to overwrite properties it introduces. I'd have preferred to do something like: ``` @bpy.types.Nodes.inputs.getter or @AbstractRigNodesNode.inputs.getter def inputs(self): ... ``` I also considered to overwriting the generators... ED: I can remove the properties and just overwrite the `exec_order_prerequisites` for the `LoopInputNode`, that's probably cleaner.

Sybren A. Stüvel commented

2023-06-16 11:21:57 +02:00

I don't think it's a good idea to have this different behaviour with just an underscore difference in the name. This should be handled with something that's more clearly an overridable getter of sorts.

nodes.py Outdated

						
				@ -202,0 +306,4 @@

				        """Generator, yields nodes that should be executed before the loop output node."""

				        nodes = list(self._connected_nodes(self._inputs, "from_socket"))

				        if not input_node in nodes and len(nodes) > 0:

Sybren A. Stüvel commented

2023-06-13 11:42:45 +02:00

Replace not X in Y with X not in Y.
Don't count the number of nodes unless you want to know the count. In this case and nodes will be enough to express 'list is not empty'
A 'list is not empty' check is likely to be faster than a list lookup, so do that first

The result:

if nodes and input_node not in nodes:
    ...

- Replace `not X in Y` with `X not in Y`. - Don't count the number of nodes unless you want to know the count. In this case `and nodes` will be enough to express 'list is not empty' - A 'list is not empty' check is likely to be faster than a list lookup, so do that first The result: ```py if nodes and input_node not in nodes: ... ```

Denys Hsu commented

2023-06-13 13:39:02 +02:00

I agree, actually rewrote that part completely. It skipped nodes when there were looping nodes within the loops.

ED: Due to the new logic I think it shouldn't be to hard to implement the points that have been out of scope. For the backwards solver that should only change how to search for innern loop nodes. But I'm not sure if I'd even like to have "setting attributes within loops" to be supported - doesn't make a lot of sense especially for nested loops.

I agree, actually rewrote that part completely. It skipped nodes when there were looping nodes within the loops. ED: Due to the new logic I think it shouldn't be to hard to implement the points that have been out of scope. For the backwards solver that should only change how to search for innern loop nodes. But I'm not sure if I'd even like to have "setting attributes within loops" to be supported - doesn't make a lot of sense especially for nested loops.

cgtinker marked this conversation as resolved

nodes.py Outdated

						
				@ -261,2 +373,3 @@

				        finally:

				            self.has_run = True

				            self.iterations -= 1

				            if self.iterations == 0:

Sybren A. Stüvel commented

2023-06-13 11:48:59 +02:00

To account for potential issues, change the comparison to <= 0. That could prevent some infinite loops in case self.iterations becomes negative.

To account for potential issues, change the comparison to `<= 0`. That could prevent some infinite loops in case `self.iterations` becomes negative.

cgtinker marked this conversation as resolved

Denys Hsu added 1 commit 2023-06-13 13:18:25 +02:00

fix looping nodes cycles in loops 7d6e455518

Denys Hsu added 1 commit 2023-06-13 13:48:32 +02:00

small refactoring 30fd4a91da

Denys Hsu commented

2023-06-13 14:09:50 +02:00

Thanks, moving out the execution would definitely clean this up. I also considered to add an AbstractLoopNode because the _get_innern_loop_nodes method, _inputs and _outputs properties are not required for other nodes.

Thanks, moving out the execution would definitely clean this up. I also considered to add an `AbstractLoopNode` because the `_get_innern_loop_nodes` method, `_inputs` and `_outputs` properties are not required for other nodes.

Denys Hsu requested review from Sybren A. Stüvel 2023-06-13 14:43:16 +02:00

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u loop:cgtinker-loop

git checkout cgtinker-loop

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: dr.sybren/rignodes#4

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Loop Nodes #4

Checkout