imnodes icon indicating copy to clipboard operation
imnodes copied to clipboard

Internal link refactor

Open Nelarius opened this issue 2 years ago • 4 comments

This pr contributes three things:

  • Moves links internally from an object pool to a simple vector. This makes links truly immediate-mode.
  • Rewrites link, node, pin interaction.
  • Removes the attribute flag EnableLinkDetachWithDragClick.

@Auburn, if you are still around, wondering if you could give this branch a spin. You’ve been very good at spotting broken interaction code, and this branch rewrites everything 😓 I will leave this pr open for a while while I continue my own tests.

More details about each change below.

Move links from the object pool to a vector

This change boils down to

// in `ImNodesEditorContext`
- ImObjectPool<ImLinkData> Links;

// in `ImNodesContext`
+ ImVector<ImLink> Links;

When BeginNodeEditor is called, the vector is resized to zero. Links are appended to the vector in ImNodes::Link(). In EndNodeEditor, we render them. Links are now truly immediate-mode and really simple.

Subtle bugs occurring on id reuse, such as this https://github.com/Nelarius/imnodes/issues/91 no longer apply to links. The goal is to apply this change for nodes and pins as well.

Rebuild link, node, pin interaction

  • Needed to accommodate the new way of storing links in the interaction code as links are internally identified in a different manner now.
  • The existing state machine was difficult to change. This pr introduces a more fine-grained state machine to make the logic of the link interaction states much simpler.

Remove the EnableLinkDetachWithDragClick attribute flag

  • This attribute was removed to simplify the interaction state machine.
  • Hovering over pins, links, and nodes can now be made totally exclusive, as we don’t need to detect the link + pin hovered state for this interaction.
  • Using the feature felt finicky, as activation required clicking and dragging in the pin hover radius.

Test checklist

  • [x] IsLinkStarted when partial link started
  • [x] IsLinkDropped when link is dropped
    • [x] triggers when including_detached_links defined & not from snap
    • [x] triggers when including_detached_links defined & from snap
    • [x] does not trigger from snap
    • [x] triggers when not from snap
  • [x] Repeat IsLinkDropped test with "detach link with modifier" enabled
  • [x] Multi-selecting nodes should not clear link selection
  • [x] Multi-selecting links should not clear node selection
  • [ ] Auto-panning when dragging nodes
  • [ ] Auto-panning when box-selecting nodes
  • [ ] Auto-panning when dragging links
  • [ ] Minimap viewport interaction
    • [ ] Clicking minimap node centers on node
    • [ ] Dragging viewport in minimap
  • [ ] Minimap renders selected nodes
  • [ ] Clicking and dragging nodes on top of minimap
  • [ ] Dragging links on top of minimap

Nelarius avatar May 31 '22 05:05 Nelarius

Hi, I gave this a quick test run, these are the things I noted:

  • Removing EnableLinkDetachWithDragClick is a big loss for UX, tweaking and deleting links is much more clunky without it. I don't think I would use this new version without this functionality.
  • IsLinkedCreated works slightly differently now, it used to always return the output pin as the start and input as the end, now it is dependant on how the link was made. Not really an issue, probably makes more sense this way, just thought I'd mention it.
  • Snapping a link onto an occupied pin causes a debug assert
  • Can not longer drag the node graph viewport around with any of the mouse buttons, not sure what it is bound to now?

Auburn avatar May 31 '22 13:05 Auburn

Thanks for the feedback, really appreciate it! I will definitely look into adding EnableLinkDetachWithDragClick back, I had a (wrong) hunch that it wasn't used much.

IsLinkedCreated works slightly differently now

Yeah, that's a good point. ImNodes::Link() is really simple now and just stores the ids in the order received. The exact behaviour of the IsLinkStarted function could be specified.

Nelarius avatar May 31 '22 17:05 Nelarius

@Nelarius Hey there, If I'm reading this PR right, it looks like we'll be able to customize the bezier curve's path from pin-pin? For example, if we have an output pin that needs to travel to an input pin to the left of it, we can make the curve arc upwards instead of connect in a straight line?

juliusl avatar Oct 31 '22 21:10 juliusl

Hey @Nelarius is there any further progress on this update? Getting this and zooming functionality would be a huge upgrade for this library.

Without zooming support I'll probably have to switch to a different node library which would be a shame. I have locally merged latest onto the zooming PR and got that working. Also attempted to fix the mouse positioning bugs but not had any success.

Any input you have on this would be great! Thanks again

Auburn avatar Apr 20 '23 09:04 Auburn