PistonDevelopers / mush

gui for dialogue graphs, et al.
Apache License 2.0
12 stars 2 forks source link

DO NOT MERGE First attempt at adding/removing connections #22

Closed stjahns closed 8 years ago

stjahns commented 9 years ago

Not really happy with this, but pushed this as food for thought / discussion. It "works", but there's a few problems that I'll point out below.

Basic idea - Each node has a [+] button for new connections, clicking one and then another adds a connection between the nodes. For now, just adding a Button widget for each edge, where the button removes the edge

graph

Removing nodes and edges is really awkward, since it can cause the indices for the remaining nodes/edges to change. Also, having the build_ui methods require mutable references makes the borrow checker angry if you try to refer to data from both nodes and edges at the same time (see trying to build the edge button at the midpoint between the edges nodes)

I think a better approach would be to build the UI with immutable references to the graph data. Instead the react closures to add/delete nodes and edges will push anEditCommand struct/enum to a shared CommandQueue. This would also allow us to implement undo/redo, which is something we should be thinking about now rather than later, as it will have a major effect on how we architect this.

However, that also has problems, as an 'AddEdgeCommand' will need to track the relevant node indices, and that command will live longer than those indices remain correct if nodes are removed, bringing us back to the original problem where we need some kind of permanent identifier for nodes and edges :/

viperscape commented 9 years ago

I need to look closer at petgraph, I find it surprising that the NodeIndex changes-- which is the issue correct? I really like the idea of a command deque; in addition to this, we can save the entire queue to a log to rebuild at any point in time, as well as run through it and then have a final graph to export when ready.

viperscape commented 8 years ago

This PR ended up being superseded, closing this down.