egui_node_graph icon indicating copy to clipboard operation
egui_node_graph copied to clipboard

Modify ConnectionEvents to specify both endpoints, and implement many to many connections

Open gmorenz opened this issue 2 years ago • 7 comments

This PR could be split into two independent PRs, if you want me to do that let me know. The only reason I'm putting them together is that the last commit depends on the first two (and I don't think github supports stacked PRs?).

The second half is as discussed in #20.

The first half is something I forgot to open an issue on earlier (oops). It's modifying ConnectEventEnded and DisconectEvent to specify both endpoints instead of only the most recent. This simplifies some logic internally to this library, and it means that if the end-application wants to track connection changes it doesn't need to store redundant state about what connection is in progress (the latter of which is why I initially implemented it). The drawback is that if the user code for some reason wants to know on which port the connection ended (instead of just the set of ports a connection was added/removed from) it now needs to track state... I don't imagine that's a common need.

I imagine I could implement many to many connections without implementing the events change, but it simplifies the logic, and I already had it.

gmorenz avatar May 26 '22 02:05 gmorenz

Could you add a new data type, something like a VectorList and a new node that takes that vector list as input and adds all the vectors together?

Hmm, so a few issues

Currently DataType's decide whether or not we can feed them with a Eq implementation, so I can't accept Vector's into a VectorList input. Maybe something to think about changing, but definitely not here. I guess I could make a "convertToList" node though that does a mutli-input thing like in my sequence screenshot in the above discussion (which would be a good example of how to use events, if nothing else).

Lists also strike me as something that belongs in the normal many-one world, because merging them cares about order, I guess I could do something with sets instead though so that's not a huge issue. Sets at least plausibly being a many-many data type with implicit union.

I'll throw together some examples, and we can see what we want to keep.

gmorenz avatar May 26 '22 17:05 gmorenz

Currently DataType's decide whether or not we can feed them with a Eq implementation, so I can't accept Vector's into a VectorList input.

I think this raises an excellent point in favor of dropping the idea of VectorList. Your comment above made me think about this and I think it's best if we keep everything Vector and make the cardinality a property of ports, not datatypes.

setzer22 avatar May 29 '22 15:05 setzer22

Just a heads up, I pushed a minor fix in #36 that will likely conflict with this PR. The code needs to be adapted so that when a node is deleted, the relevant DisconnectEvents are sent to the user. Right now this only happens for inputs, but it will need to happen for outputs as well in the new version.

setzer22 avatar May 29 '22 16:05 setzer22

After looking over the existing PR and making a vain attempt to merge main into this branch, I think I'd like to start from a fresh slate and make a new PR directly off of main that implements my suggestions here.

Rather than adding mergeable and splittable to DataTypeTrait I think I'd like to introduce the concept of a Port to replace the (String, InputId) tuple here. The Port type would specify a name and an Option<usize> connection limit with None indicating no limit to the number of connections.

mxgrey avatar Jul 15 '22 15:07 mxgrey

After looking over the existing PR and making a vain attempt to merge main into this branch, I think I'd like to start from a fresh slate and make a new PR directly off of main that implements my suggestions here.

Rather than adding mergeable and splittable to DataTypeTrait I think I'd like to introduce the concept of a Port to replace the (String, InputId) tuple here. The Port type would specify a name and an Option<usize> connection limit with None indicating no limit to the number of connections.

Yes, unfortunately main has moved quite a bit from this PR. I think your idea looks reasonable so if you find it easier to start a new PR please feel free! 👍

setzer22 avatar Jul 16 '22 14:07 setzer22

Hi @mxgrey! Any news on this? :smile:

setzer22 avatar Sep 15 '22 10:09 setzer22

I did make a lot of progress which you can see in the hierarchical_design branch of my fork. The changes are so extensive that it's not in a usable state yet, and unfortunately I had to shelve the effort because of an onslaught of urgent deadlines at my day job.

I fully intend to resume working on it as soon as time is available, and in the best case scenario I may be able to use time from my day job for that effort.

A quick overview of the design that I'm working towards is that the node graphs will have a more hierarchical architecture.

  • A graph will contain some number of nodes and connections.
  • A node implements the NodeTrait allowing custom node design and contains some number of ports.
  • A port implements the PortTrait allowing custom port design and contains some number of hooks. The exact behavior of a port (e.g. how many hooks it supports) can be customized by the implementer of PortTrait.
  • A hook is a unique ID within a port. The tuple of (node_id, port_id, hook_id) defines a fully unique connection endpoint.
  • A connection contains two (node_id, port_id, hook_id) tuples, one for the output connection and one for the input connection.

Many-to-many, many-to-one, and one-to-many connections are achieved by defining the behavior of each port (e.g. how many hooks are available, and whether that number can grow). Disconnecting a connection is done by requesting the removal of a connection from the (node_id, port_id, hook_id) of the input or output endpoint of a connection (the opposite endpoint will automatically be freed as well). Visually you would click and drag the connection off of the little ball that represents the hook.

I'll open a PR as soon as the changes are usable. I can't offer an estimate on when that will be at the moment because I still have some urgent deadlines, but at the absolute latest I plan on taking time off work in late December, so I'll certainly be able to make quick progress at that point.

mxgrey avatar Sep 15 '22 10:09 mxgrey