Modify ConnectionEvents to specify both endpoints, and implement many to many connections #30
Conversation
Currently graph connections are always one output to many inputs. This is appropriate for most "data type values", but not always correct. The motivating example for a place where it is incorrect is "control flow", where many nodes might "output" control flow to the same node's control flow input. This changes that so that it is configurable via data-type whether nodes are one-many, many-one, many-many, or even one-one with the concept of splittable data-types (data types that can be copied from one output to many inputs) and mergeable data-types (data types that can accept many outputs into a single input).
There was a problem hiding this comment.
Still need to finish my review (will get to it later). But I had a question about disconnect events in the meantime.
Also, I think the new changes need to be showcased in the example project. 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?
| if is_connected_input { | ||
| responses.push(NodeResponse::DisconnectEvent(param_id.assume_input())); | ||
| } else { | ||
| responses.push(NodeResponse::ConnectEventStarted(node_id, param_id)); | ||
| } | ||
| let response = match unique_connection { | ||
| Some(AnyParameterId::Input(input)) => NodeResponse::DisconnectEvent { | ||
| input, | ||
| output: param_id.assume_output(), | ||
| }, | ||
| Some(AnyParameterId::Output(output)) => NodeResponse::DisconnectEvent { | ||
| input: param_id.assume_input(), | ||
| output, | ||
| }, | ||
| None => NodeResponse::ConnectEventStarted(node_id, param_id), | ||
| }; | ||
| responses.push(response); |
There was a problem hiding this comment.
How do you disconnect a node when the connection is not unique? I think dragging from an input should start a disconnect event even if the input port has multiple incoming connections.
The logic to select which connection is a bit more nuanced... Here's how blender does it for reference:
In Blender's case, they have some sort of "wide" port and depending on where exactly in the port you drag from you get different connections. Since we don't have this logic here, we could devise a simpler mechanism where you simply disconnect the last connection in the list. This will be a bit iconvenient for users, but I can't think of a better way. I think in the long run we need something like blender's wide ports.
EDIT: Also important to note that for some nodes taking multiple inputs users will care about the order in which those inputs are processed. This again brings us back to blender's solution of the problem.
There was a problem hiding this comment.
Oh no, forgot about this issue for many-many nodes.
So, for many->one types (which is what I'm using this for in my project), the behavior is just the reverse of one->many nodes. Dragging from the input creates a new edge, dragging from the output lets you delete the edge. This feels right to me. I'm not using one-one nodes, but dragging from either output removes the current edge (which also sounds right).
For many-many data-types, I'm inclined to think that the right solution is to make it possible to select edges. Right now it's not possible to delete many-many edges short of deleting nodes. Alternatively it might just be better to not have many-many data types, and use an alternative solution for them (one of blenders... see below).
I didn't realize this, but blender actually supports (at least) two different solutions for many-many edges.
One is a sort of wide port, the other keeps adding more inputs as you fill up the existing inputs.
The latter is something the events changes make pretty easy to implement, I've done it (except for outputs)... and it's also one of the reasons I want #32.
Both of these solutions strike me as a decision that should be made by the port, not data that should attach to the data type. I.e. the same data type should be able to be used with wide and "thin" ports.
Circling back to what to do in this PR.
I think selecting edges and/or wide ports really belongs in a follow up PR (it seems unlikely to be a completely trivial change) - but I should probably make sure that wide ports are at least reasonably simple to implement.
If we don't want to go the selecting edges route, it might make sense to change the API for defining data-types so that many-many datatypes are unrepresentable (which shouldn't forbid the use of many-one data-types with wide-ports to emulate them).
We could also conceivably abandon this approach and just go with wide ports. I think this is a worse solution for control flow like data which really is naturally one->many, but it might have some simplicity benefits for programs that don't need that, and it might be possible to make it configurable enough to get good behavior with a bit of extra work.
There was a problem hiding this comment.
Both of these solutions strike me as a decision that should be made by the port, not data that should attach to the data type. I.e. the same data type should be able to be used with wide and "thin" ports.
You are quite right here, the "cardinality" of a port is a property of a port, so my VectorList datatype example was a bad one. Instead, we should add a port that accepts multiple inputs of type Vector.
it might make sense to change the API for defining data-types so that many-many datatypes are unrepresentable
I think that wouldn't be a very flexible solution. In a regular "data flow" dependency graph, everywhere many-to-one is useful, many-to-many is also useful. Consider this example, here a node like "Join Geometry" would be far less useful if it affected their inputs preventing them from being shared to other outputs. That would be quite a weird thing for a node graph (although Rust has it, feels pretty much like move semantics, and I guess that's why it works well in your case, since you're not modelling data but control flow 🤔)
There was a problem hiding this comment.
After thinking about this a bit more, I think I have come up with a good understanding of the issue and a solution that fits all our use cases 🤔
The common logic in your PR is that dragging away from a port that is "full" (i.e. doesn't accept any more connections) initiates a disconnection event. This works for many-to-one, one-to-many and one-to-one ports, but not for many-to-many ports, because they are never "full". The problem here is that there are two orthogonal concepts that are being mixed together. Let me ellaborate:
We can classify ports in the following ways:
- Ports have order. A port can be ordered or unordered. An ordered port cares about the order of their incoming/outgoing connections, while an unordered port only cares that the connections exist.
- On a different scale, ports have cardinality, which can be one-to-one, many-to-one, one-to-many and many-to-many. This affects the allowed connection and disconnection events on that port.
Your control flow "join" port is a one-to-many unordered port, and blender's "Join Geometry" input is a many-to-many ordered port. Other examples exist, and after giving it some thought I think all combinations can have valid use cases 🤔
In the long term (not for this PR) ordered ports can be represented as wide ports, just like in Blender. This allows users to see the order of the different inputs visually, while unordered ports are just a single dot, just like now.
But then, when it comes to this PR, I think the key to solving our issue is separate the cardinality of a port from its disconnect behavior. When a user drags away from a port, instead of using splittable and mergeable to determine whether we should initiate a connect or disconnect event, we instead add a different set of methods disconnect_when_input and disconnect_when_output, so that users can implement their own behavior.
Your control flow ports could return disconnect_when_input = false and disconnect_when_output = true and this would get you the desired behavior. Meanwhile, in an application closer to blender, where the data flow is being modelled, users can set the usual disconnect_when_input = true and disconnect_when_input = false that would be consistent with one-to-many ports. For now, since there are no wide ports, disconnecting from a "many" port would simply drag the last (or whatever) connection, we can improve this UX later on with wide ports.
There is still room for splittable and mergeable. These fields indicate the cardinality of the port, and that is still checked when finishing the connection event. If a port is not mergeable, you won't add an additional connection to it when the user releases an ongoing connection on top of it.
I think this does not take a lot of extra work and solves the issue for everyone. What do you think?
There was a problem hiding this comment.
I like the idea of having orthogonal traits of order vs cardinality, but I think I would strongly recommend using wide ports regardless of whether the port is ordered or unordered.
I think it's very unlikely that there would be a use case where someone always wants all connections to be pulled off of a port when clicking and dragging on a port. Instead I think "ordered" vs "unordered" should just decide whether or not the library is allowed to reorder the connections in the wide port to eliminate tangling. That would probably be a future feature outside the scope of an initial many-to-many port PR.
To allow users to choose between removing a connection vs creating a connection for wide ports, I recommend a tweak to the UI, drafted in this image:
- Pulling and dragging from the white box area of the
Multi Portwill always begin to create a connection. - Hovering the mouse inside the border of the box will make the whole box highlighted with white.
- Pulling on the unused dot will also begin to create a connection.
- Pulling on the dot of an existing connection will pull that connection off of the port.
- Dragging the dot of another port anywhere inside the white box will create a connection to that port.
- The white box area will grow vertically as more connections are added, and the box will always wrap around all the connections that the port has.
Having this additional square element will also help to visually communicate that multiple connections to this port are allowed. We can have the unused dot disappear when the port reaches its limit for number of connections to communicate that the port does not accept any more connections.
I naively assume that this should be a relatively simple interface to implement. Probably the hardest part will be making the row height for this port dynamically grow.
If this suggestion sounds reasonable then I can try taking a stab at implementing for this PR.
There was a problem hiding this comment.
@mxgrey Right, you have a point there. My original comment was a bit biased by the fact that we didn't have a wide port, but if the plan is to implement it, I agree it makes sense to have it on both cases.
I like your description for the UX of the wide port. I'd be happy to have something like that. Feels more explicit than blender's approach 👍
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. |
I think this raises an excellent point in favor of dropping the idea of |
|
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 |
|
After looking over the existing PR and making a vain attempt to merge Rather than adding |
Yes, unfortunately |
|
Hi @mxgrey! Any news on this? 😄 |
|
I did make a lot of progress which you can see in the 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.
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 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. |
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.