Fix T89415: Update multi input indices after deleting a node
ClosedPublic
Actions

Authored by Wannes Malfait (Wannes) on Jun 26 2021, 5:51 PM.

Details

Reviewers

Fabian Schempp (fabian_schempp)
Jacques Lucke (JacquesLucke)

Maniphest Tasks

T89415: Deleting a node plugged into a multi-input doesn't rearrange the remaining noodles

Commits

rB31fcb934a4c1: Fix T89415: update multi input indices after deleting a node

Summary

When deleting a node, links attached to that node are deleted, but if one
of those links was connected to a multi input socket, the indices of the
other links connected to it were not updated. This adds updates both in
the case of a normal delete, as well as after a delete with reconnect.

Diff Detail

Repository

rB Blender

Branch

multi_input_indices (branched from master)

Build Status

Buildable 15985
Build 15985: arc lint + arc unit

Event Timeline

Wannes Malfait (Wannes) requested review of this revision.Jun 26 2021, 5:51 PM

Wannes Malfait (Wannes) created this revision.

Harbormaster completed remote builds in B15443: Diff 38787.Jun 26 2021, 5:51 PM

Wannes Malfait (Wannes) added reviewers: Fabian Schempp (fabian_schempp), Jacques Lucke (JacquesLucke).Jun 26 2021, 5:52 PM

Wannes Malfait (Wannes) added a project: Nodes.

Wannes Malfait (Wannes) added a task: T89415: Deleting a node plugged into a multi-input doesn't rearrange the remaining noodles.

Wannes Malfait (Wannes) retitled this revision from Fix T89453: Update multi input indices after deleting a node to Fix T89415: Update multi input indices after deleting a node.

Thanks for working on that. So far it looks good to me. Just a tiny cleanup and a question.

source/blender/blenkernel/intern/node.cc
2523	Only from curiosity. When in the deleting process is this function called when nodeUnlinkNode is not? I just ask because (without knowing the details) it seems that nodeUnlinkNode iterates over all links in the tree. So maybe it's enough to adjust indices there?
source/blender/editors/space_node/node_edit.cc
1836 ↗	(On Diff #38787)	unrelated change?

This revision now requires changes to proceed.Jun 27 2021, 10:05 AM

Cleanup: undo accidental line removal

Harbormaster completed remote builds in B15444: Diff 38803.Jun 27 2021, 1:29 PM

Wannes Malfait (Wannes) added inline comments.Jun 27 2021, 1:30 PM

source/blender/blenkernel/intern/node.cc
2523	This happens in delete with reconnect (CTRL + X). The nodeInternalRelink function tries to merge a link going into the node to one going out of the node to be deleted. If there is no ingoing link, it deletes the outgoing link, which is why the nodeUnlinkNode function doesn't see that link anymore, and the update has to happen here. I noticed this by accident when pressing ctrl+x instead of x while testing and saw that the update didn't happen.

Fabian Schempp (fabian_schempp) accepted this revision.Jun 29 2021, 10:12 PM

This revision is now accepted and ready to land.Jun 29 2021, 10:12 PM

Jacques Lucke (JacquesLucke) accepted this revision.Jul 23 2021, 11:07 AM

Jacques Lucke (JacquesLucke) added inline comments.

source/blender/blenkernel/intern/node.cc
2519	What does this assert protect against? I wonder if we would loose anything by just not having it.

Wannes Malfait (Wannes) added inline comments.Jul 23 2021, 11:18 AM

source/blender/blenkernel/intern/node.cc
2519	I think I added that mainly as an insurance that my logic was correct while testing, and just thought that it might be nice to leave it there. But realistically this can only fail if `deleted_index` <0, which would indicate an error somewhere else in the code, which should not be checked for here. So I think it's fine to just remove it.