duplicate node index when removing and adding nodes

[viperscape]

I haven't figured out what is wrong, but to recreate issue:

1. Start mush
2. Remove any node but the latest created, so node 0
3. Add new node, you'll see duplicate node indices on the node labels; in this case two node 3's

[stjahns]

Ah, figured this would be a problem: "Removing nodes or edges may shift other indices" http://bluss.github.io/petulant-avenger-graphlibrary/target/doc/petgraph/graph/struct.Graph.html

[stjahns]

This gives us a bit of a problem where its difficult to have a mapping between NodeContainers and the underlying petgraph Node data, since the node indices are free to change after deletion. I have a few different rough ideas for dealing with this, just to get the juices flowing:

1. Construct all the NodeContainers from the underlying graph data each time we draw the graph. This way we would never have to keep track of node indices for more than one 'update'. Problem with this is that then all relevant state needs to be stored within the underlying graph node, including 'dragging', 'hovering' etc.
2. Rather than using NodeIndex to track the underlying nodes, store some kind of GUID/UUID in the node data to keep track of it. This kinda sucks because to fetch a specific node from the graph requires iterating through all nodes and checking their identifiers.
3. Write our own graph structure using HashMaps to map a UUID to a node/edge instance. When loading an editor, convert a given petgraph (or any other graph-like structure) to our own graph structure and use that as the underlying data structure, only converting back to a petgraph graph (or decision tree, or blend tree, or whatever) on save. Drawback of this is that we don't get a lot of niceties from petgraph (iterators over node neighbours, etc) that could come in handy, so we may need to reimplement them for our own graph data structure.

[stjahns]

4 For whatever graph structure that is to be edited, construct an internal petgraph graph where each node wraps the data in the corresponding node in the original graph, with additional state information added, such as its corresponding Conrod widget ID, hover / dragging state flags, etc. Similar to idea 1, where nothing should need to keep track of these internal nodes if the internal nodes are tracking their states, we just build all the widgets according to the current structure of the internal graph at the time of drawing. Although I'm not quite sure how to handle deleting multiple nodes at the same time (if we want to implement that) using this design.

Personally leaning towards option 3 or 4, with preference to 4 if it's possible... Haven't fully thought things through yet.

[viperscape]

I like 4 as well, 3 is a good idea too as it is then totally agnostic in a way. With op 4: we can hold state in an inherited struct similar to how I handled intrinsics, this way all that needs to be implemented in the custom node by the user is the base impl for intrinsics (though I should call that something else). I need some time to mull over all of what you said though

[stjahns]

Just saw your message. I'm relatively happy with my PR, interested in your thoughts.

[viperscape]

Is the graph rebuild potentially slow? Also, I'm not sure we have to update petgraph node position coordinates until the drag is complete, so we could use the conrod uid pulled from the node before drag-- maybe this is a good middle ground for that scenario?

edit: So we could optionally:

1. get uid from node from graph
2. store in tool pane temporarily during drag
3. when drag is complete, update petgraph node with new position
4. re render from petgraph as usual

[stjahns]

I'm not too worried about the speed, I'm expecting to eventually have to build a new internal graph anyhow from some other graph-ish structure, for example to edit something like a blend tree definition

[stjahns]

As for your other question, are you talking about storing the UiId of the node widget or the NodeIndex of the petgraph node? It might be safe to store the NodeIndex if we can assume no nodes are deleted in the meantime, not sure if it's worth it though.