dr.sybren
/
rignodes
11
14
Fork 2
Code Issues Pull Requests 5 Projects Releases Wiki Activity

Loop Nodes #4

Open
Denys Hsu wants to merge 5 commits from cgtinker/powership:loop into main
pull from: cgtinker/powership:loop
merge into: dr.sybren:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 1 Commits 5 Files Changed 4 +396 -14
Denys Hsu commented 2023-06-12 16:39:09 +02:00
Contributor
Copy Link

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
LoopNode.png
Denys Hsu added 3 commits 2023-06-12 16:39:11 +02:00
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
Owner
Copy Link

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
Owner
Copy Link

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
Owner
Copy Link

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
Owner
Copy Link

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
Author
Contributor
Copy Link

Sorry, still stumbling.

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
Owner
Copy Link

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

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
Author
Contributor
Copy Link

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
Owner
Copy Link

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.

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
Owner
Copy Link
  • 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
Author
Contributor
Copy Link

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
Owner
Copy Link

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
Denys Hsu added 1 commit 2023-06-13 13:48:32 +02:00
Denys Hsu commented 2023-06-13 14:09:50 +02:00
Author
Contributor
Copy Link

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

Merge

Merge the changes and update on Gitea.
git checkout main
git merge --no-ff cgtinker-loop
git checkout main
git merge --ff-only cgtinker-loop
git checkout cgtinker-loop
git rebase main
git checkout main
git merge --no-ff cgtinker-loop
git checkout main
git merge --squash cgtinker-loop
git checkout main
git merge cgtinker-loop
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Sybren A. Stüvel
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
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
No description provided.
Delete Branch "cgtinker/powership:loop"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?