Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Internal link refactor #155

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Internal link refactor #155

wants to merge 9 commits into from

Conversation

Nelarius
Copy link
Owner

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 #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

  • IsLinkStarted when partial link started
  • IsLinkDropped when link is dropped
    • triggers when including_detached_links defined & not from snap
    • triggers when including_detached_links defined & from snap
    • does not trigger from snap
    • triggers when not from snap
  • Repeat IsLinkDropped test with "detach link with modifier" enabled
  • Multi-selecting nodes should not clear link selection
  • 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 added 9 commits April 4, 2022 11:05
- Replaces the link creation states with ones which only use pin ids.
  The fact that snapping and unsnapping from pins used just one state
  made it very difficult to get the interaction management play nice with
  just pin ids.
- The solution was to reimplement the way interaction states are managed.
- A state stack wasn't necessary and introduced needless complexity
- This change removes EnableLinKDetachWithDragClick from the code
  entirely.
- reset unneeded changes in EditorContextCreate()
- remove unused <algorithm> include
- render minimap on top of other node editor elements
- solve the wiggling minimap link issue
@Nelarius Nelarius changed the title Big link refactor Internal link refactor May 31, 2022
@Nelarius Nelarius added this to the 0.6 milestone May 31, 2022
@Auburn
Copy link
Collaborator

Auburn commented May 31, 2022

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?

@Nelarius
Copy link
Owner Author

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.

@juliusl
Copy link

juliusl commented Oct 31, 2022

@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?

@Auburn
Copy link
Collaborator

Auburn commented Apr 20, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants