Fix T100521: Node doesn't belong to frame when added through link drag search
AbandonedPublic
Actions

Authored by Dominik Fill (dominikfill) on Sep 5 2022, 10:29 PM.

Details

Reviewers

None

Group Reviewers

User Interface
Geometry Nodes

Summary

Fix for T100521: When creating a node through link drag search, inside a frame node, the created node did not become attached to the frame node.

before (3.2.2)	after

Not sure if

ntree.ensure_topology_cache();

is necessary.

Diff Detail

Repository

rB Blender

Branch

dev-T100521-link-node-drag-search-node-to-frame (branched from master)

Build Status

Buildable 23625
Build 23625: arc lint + arc unit

Event Timeline

Dominik Fill (dominikfill) requested review of this revision.Sep 5 2022, 10:29 PM

Dominik Fill (dominikfill) created this revision.

Harbormaster completed remote builds in B23625: Diff 55334.Sep 5 2022, 10:29 PM

Dominik Fill (dominikfill) changed the visibility from "Public (No Login Required)" to "No One".Sep 5 2022, 10:29 PM

Dominik Fill (dominikfill) retitled this revision from fix for T100521 to Fix: T100521 Node doesn't belong to frame when added through link drag search.Sep 5 2022, 10:36 PM

Dominik Fill (dominikfill) edited the summary of this revision. (Show Details)

Dominik Fill (dominikfill) added a reviewer: User Interface.

Dominik Fill (dominikfill) edited the summary of this revision. (Show Details)

Dominik Fill (dominikfill) changed the visibility from "No One" to "Public (No Login Required)".

Dominik Fill (dominikfill) retitled this revision from Fix: T100521 Node doesn't belong to frame when added through link drag search to Fix T100521: Node doesn't belong to frame when added through link drag search.Sep 5 2022, 10:38 PM

Dominik Fill (dominikfill) edited the summary of this revision. (Show Details)

Thanks for the patch!

ensure_topology_cache is necessary, since nodes_by_type uses the cache.
What do you think about extracting some of the logic from the reroute operator (the other place this happens) so we can duplicate less of the code?
Maybe a function that has a Span<bNode *> frame_nodes argument or something like that.

One thing I noticed about this patch is, that it bases the attachment to the frame node on the position the new node is originally created. I'm not sure that's always the desired behavior since you can still move the node after adding it.
So adding a node in front of a frame node via drag link search will add it to the frame no matter what and dragging a node created via drag link search outside a frame onto the frame, will not add the node to the frame (see video below).

Instead of redoing the attachment logic here, we could maybe just call the NODE_OT_translate_attach operator that already handles the frame attachment?

source/blender/editors/space_node/link_drag_search.cc
242	I mean here, where we're calling the transform operator anyway.

Dominik Fill (dominikfill) added a reviewer: Geometry Nodes.Sep 5 2022, 11:04 PM

@Leon Schittek (lone_noel) 's approach does work, I'm not yet familiar enough with the intricacies of the nodes code to decide which approach is more desirable...

Oh, nice! I'd say @Leon Schittek (lone_noel)'s approach is preferrable since it reuses existing generic behavior.

I'm not sure if the frame nodes are sorted, is this taken into account in any way, or can the binding occur not to the top or bottom frame, but to a random one?

source/blender/editors/space_node/link_drag_search.cc
225	Do I need to call copying the span into a new vector here? source/blender/makesdna/DNA_node_types.h$647
226–227	If the index is no longer used anywhere, maybe for (const bNode *frame_node : frame_nodes) { ... }