Details

Reviewers

Jacques Lucke (JacquesLucke)
Hans Goudey (HooglyBoogly)

Maniphest Tasks

T102740: Blender allows you to put a node group into itself

Commits

rB4121e32edd91: Fix T102740: don't allow inserting group into itself

Summary

This patch adds pre-validation of selected nodes by the insert operator to the group to not allow the group to be inserted into itself.

Diff Detail

Event Timeline

Iliya Katueshenock (Moder) requested review of this revision.Nov 24 2022, 1:41 PM

Iliya Katueshenock (Moder) created this revision.

Iliya Katueshenock (Moder) edited the summary of this revision. (Show Details)Nov 24 2022, 1:44 PM

Iliya Katueshenock (Moder) added a task: T102740: Blender allows you to put a node group into itself.

Iliya Katueshenock (Moder) updated this revision to Diff 57986.Nov 24 2022, 1:54 PM

Iliya Katueshenock (Moder) retitled this revision from Fix T102740: Haven't insert group into itself to Fix T102740: Don't insert the group into yourself.

Iliya Katueshenock (Moder) retitled this revision from Fix T102740: Don't insert the group into yourself to Fix T102740: Don't insert the group into itself.Nov 24 2022, 1:56 PM

Iliya Katueshenock (Moder) edited the summary of this revision. (Show Details)Nov 24 2022, 2:01 PM

Iliya Katueshenock (Moder) added a reviewer: Hans Goudey (HooglyBoogly).Nov 25 2022, 7:48 PM

Iliya Katueshenock (Moder) planned changes to this revision.Nov 26 2022, 11:43 PM

Iliya Katueshenock (Moder) updated this revision to Diff 58069.Nov 27 2022, 1:36 AM

Move if (gnode->id) above, out of loop

Jacques Lucke (JacquesLucke) added inline comments.Nov 28 2022, 1:58 PM

source/blender/editors/space_node/node_group.cc
686	Is this redundant with the check below?

Iliya Katueshenock (Moder) added inline comments.Nov 28 2022, 3:58 PM

source/blender/editors/space_node/node_group.cc
686	This is for a separate, slightly different bug report.

But, if necessary, I planned to move node_group_make_node_tree_is_contains function to ED_node for the python api in the future. In such a case, this function could generate a text version of the message about how the groups overlap. For example Can not insert group Group.005 in Group.001 becouse Group.001->Group.002->Group.003->Group.004->Group.005

No need to make this more complex now. Generally seems good. Will test tomorrow. The function names could probably be improved.

Iliya Katueshenock (Moder) updated this revision to Diff 58122.Nov 28 2022, 5:46 PM

Iliya Katueshenock (Moder) marked an inline comment as done.

I cleaned it up a bit more, does this make sense to you: P3359?

I wanted to make sure that there is no unnecessary check, but I didn’t even think about reusing node_group_make_node.

I will need to remember reinterpret_cast so I don't run into problems (tried to use static cast at first but didn't work)

Forgot one)

Still, I think it's better to agree with your naming / commenting option. At least in the formulation of an understandable comment, I'm definitely weak

rBSab4926bcffee: Fix: Various mishandling of node identifiers and vector

Merge master
This is a slightly specific operation. In the new approach to the insert test, it is more logical to raise this check to an insert statement, since the extra call to create a new group is redundant.

Add comment

One comment inline, other than that this looks good, thanks.

source/blender/editors/space_node/node_group.cc
1204–1211	IMO it's a bit clearer if there's just a `if (node->is_group() && node->id) {` check inside this look, that saves creating the temporary set (and having to come up with a name for it, and having to use brackets to make a separate scope for it).

Iliya Katueshenock (Moder) updated this revision to Diff 58279.Dec 3 2022, 11:28 AM

Iliya Katueshenock (Moder) marked an inline comment as done.

Hans Goudey (HooglyBoogly) added inline comments.Dec 5 2022, 3:38 PM

source/blender/editors/space_node/node_group.cc
1200	This if statement seems weird to me. It looks like it's checking "if the node isn't a group node or it doesn't have a data-block" which doesn't sound right. I'd think this should check "if the node is a group node and it has a data-block, which is the check I posted in my previous comment. Am I missing something?