Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
997c4d3
Have ConnectEventEnded specify both endpoints
gmorenz Apr 6, 2022
1e76f94
Make DisconnectEvent specify both endpoints
gmorenz Apr 7, 2022
2b1028a
Many to many connections
gmorenz Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 67 additions & 46 deletions egui_node_graph/src/editor_ui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,17 @@ pub type PortLocations = std::collections::HashMap<AnyParameterId, Pos2>;
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum NodeResponse<UserResponse: UserResponseTrait> {
ConnectEventStarted(NodeId, AnyParameterId),
ConnectEventEnded(AnyParameterId),
ConnectEventEnded {
output: OutputId,
input: InputId,
},
CreatedNode(NodeId),
SelectNode(NodeId),
DeleteNode(NodeId),
DisconnectEvent(InputId),
DisconnectEvent {
output: OutputId,
input: InputId,
},
/// Emitted when a node is interacted with, and should be raised
RaiseNode(NodeId),
User(UserResponse),
Expand Down Expand Up @@ -175,24 +181,8 @@ where
NodeResponse::ConnectEventStarted(node_id, port) => {
self.connection_in_progress = Some((node_id, port));
}
NodeResponse::ConnectEventEnded(locator) => {
let in_out = match (
self.connection_in_progress
.map(|(_node, param)| param)
.take()
.expect("Cannot end drag without in-progress connection."),
locator,
) {
(AnyParameterId::Input(input), AnyParameterId::Output(output))
| (AnyParameterId::Output(output), AnyParameterId::Input(input)) => {
Some((input, output))
}
_ => None,
};

if let Some((input, output)) = in_out {
self.graph.add_connection(output, input)
}
NodeResponse::ConnectEventEnded { output, input } => {
self.graph.add_connection(output, input);
}
NodeResponse::CreatedNode(_) => {
//Convenience NodeResponse for users
Expand All @@ -209,15 +199,11 @@ where
}
self.node_order.retain(|id| *id != node_id);
}
NodeResponse::DisconnectEvent(input_id) => {
let corresp_output = self
.graph
.connection(input_id)
.expect("Connection data should be valid");
let other_node = self.graph.get_input(input_id).node();
self.graph.remove_connection(input_id);
NodeResponse::DisconnectEvent { input, output } => {
let other_node = self.graph.get_input(input).node();
self.graph.remove_connection(output, input);
self.connection_in_progress =
Some((other_node, AnyParameterId::Output(corresp_output)));
Some((other_node, AnyParameterId::Output(output)));
}
NodeResponse::RaiseNode(node_id) => {
let old_pos = self
Expand Down Expand Up @@ -341,10 +327,10 @@ where
for (param_name, param_id) in inputs {
if self.graph[param_id].shown_inline {
let height_before = ui.min_rect().bottom();
if self.graph.connection(param_id).is_some() {
ui.label(param_name);
} else {
if self.graph.incoming(param_id).is_empty() {
self.graph[param_id].value.value_widget(&param_name, ui);
} else {
ui.label(param_name);
}
let height_after = ui.min_rect().bottom();
input_port_heights.push((height_before + height_after) / 2.0);
Expand Down Expand Up @@ -384,7 +370,11 @@ where
param_id: AnyParameterId,
port_locations: &mut PortLocations,
ongoing_drag: Option<(NodeId, AnyParameterId)>,
is_connected_input: bool,
// If the datatype of this node restricts it to connecting to
// at most one other node, and there is a connection, then this
// parameter should be Some(PortItIsConnectedTo), otherwise it
// should be None
unique_connection: Option<AnyParameterId>,
) where
DataType: DataTypeTrait,
UserResponse: UserResponseTrait,
Expand All @@ -409,21 +399,32 @@ where
.circle(port_rect.center(), 5.0, port_color, Stroke::none());

if resp.drag_started() {
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);
Comment on lines -412 to +413
Copy link
Owner

@setzer22 setzer22 May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
blender_multi_conn

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image

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.

image

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.

Copy link
Owner

@setzer22 setzer22 May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔)
image

Copy link
Owner

@setzer22 setzer22 May 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
User Interface

  • Pulling and dragging from the white box area of the Multi Port will 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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if let Some((origin_node, origin_param)) = ongoing_drag {
if origin_node != node_id {
// Don't allow self-loops
if graph.any_param_type(origin_param).unwrap() == port_type
&& resp.hovered()
&& ui.input().pointer.any_released()
{
responses.push(NodeResponse::ConnectEventEnded(param_id));
if resp.hovered() &&
ui.input().pointer.any_released() &&
origin_node != node_id && // Don't allow self-loops
graph.any_param_type(origin_param).unwrap() == port_type
{
match (origin_param, param_id) {
(AnyParameterId::Output(output), AnyParameterId::Input(input))
| (AnyParameterId::Input(input), AnyParameterId::Output(output)) => {
responses.push(NodeResponse::ConnectEventEnded { output, input })
}
_ => (),
}
}
}
Expand All @@ -443,6 +444,16 @@ where
InputParamKind::ConnectionOrConstant => true,
};

let unique_connection = if !self.graph.get_input(*param).typ.mergeable() {
self.graph
.incoming(*param)
.first()
.copied()
.map(AnyParameterId::Output)
} else {
None
};

if should_draw {
let pos_left = pos2(port_left, port_height);
draw_port(
Expand All @@ -454,7 +465,7 @@ where
AnyParameterId::Input(*param),
self.port_locations,
self.ongoing_drag,
self.graph.connection(*param).is_some(),
unique_connection,
);
}
}
Expand All @@ -465,6 +476,16 @@ where
.iter()
.zip(output_port_heights.into_iter())
{
let unique_connection = if !self.graph.get_output(*param).typ.splittable() {
self.graph
.outgoing(*param)
.first()
.copied()
.map(AnyParameterId::Input)
} else {
None
};

let pos_right = pos2(port_right, port_height);
draw_port(
ui,
Expand All @@ -475,7 +496,7 @@ where
AnyParameterId::Output(*param),
self.port_locations,
self.ongoing_drag,
false,
unique_connection,
);
}

Expand Down
7 changes: 5 additions & 2 deletions egui_node_graph/src/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,10 @@ pub struct Graph<NodeData, DataType, ValueType> {
pub inputs: SlotMap<InputId, InputParam<DataType, ValueType>>,
/// The [`OutputParam`]s of the graph
pub outputs: SlotMap<OutputId, OutputParam<DataType>>,
// Connects the input of a node, to the output of its predecessor that
// Connects the input of a node, to the output(s) of its predecessor(s) that
// produces it
pub connections: SecondaryMap<InputId, OutputId>,
pub incoming: SecondaryMap<InputId, SVec<OutputId>>,
// Connects the outputs of a node, to the input(s) of its predecessor(s) that
// consumes it
pub outgoing: SecondaryMap<OutputId, SVec<InputId>>,
}
89 changes: 74 additions & 15 deletions egui_node_graph/src/graph_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
use super::*;

impl<NodeData, DataType, ValueType> Graph<NodeData, DataType, ValueType> {
impl<NodeData, DataType, ValueType> Graph<NodeData, DataType, ValueType>
where
DataType: DataTypeTrait,
{
pub fn new() -> Self {
Self {
nodes: SlotMap::default(),
inputs: SlotMap::default(),
outputs: SlotMap::default(),
connections: SecondaryMap::default(),
incoming: SecondaryMap::default(),
outgoing: SecondaryMap::default(),
}
}

Expand Down Expand Up @@ -64,37 +68,90 @@ impl<NodeData, DataType, ValueType> Graph<NodeData, DataType, ValueType> {
}

pub fn remove_node(&mut self, node_id: NodeId) {
self.connections
.retain(|i, o| !(self.outputs[*o].node == node_id || self.inputs[i].node == node_id));
let inputs: SVec<_> = self[node_id].input_ids().collect();
for input in inputs {
self.inputs.remove(input);
self.remove_incoming_connections(input);
}
let outputs: SVec<_> = self[node_id].output_ids().collect();
for output in outputs {
self.outputs.remove(output);
self.remove_outgoing_connections(output);
}
self.nodes.remove(node_id);
}

pub fn remove_connection(&mut self, input_id: InputId) -> Option<OutputId> {
self.connections.remove(input_id)
pub fn remove_connection(&mut self, output_id: OutputId, input_id: InputId) {
self.outgoing[output_id].retain(|&mut x| x != input_id);
self.incoming[input_id].retain(|&mut x| x != output_id);
}

pub fn remove_incoming_connections(&mut self, input_id: InputId) {
if let Some(outputs) = self.incoming.get(input_id) {
for &output in outputs {
self.outgoing[output].retain(|&mut x| x != input_id);
}
}
self.incoming.remove(input_id);
}

pub fn remove_outgoing_connections(&mut self, output_id: OutputId) {
if let Some(inputs) = self.outgoing.get(output_id) {
for &input in inputs {
self.incoming[input].retain(|&mut x| x != output_id);
}
}
self.outgoing.remove(output_id);
}

pub fn iter_nodes(&self) -> impl Iterator<Item = NodeId> + '_ {
self.nodes.iter().map(|(id, _)| id)
}

pub fn add_connection(&mut self, output: OutputId, input: InputId) {
self.connections.insert(input, output);
if self.get_input(input).typ.mergeable() {
self.incoming
.entry(input)
.expect("Old InputId")
.or_default()
.push(output);
} else {
self.remove_incoming_connections(input);
let mut v = SVec::new();
v.push(output);
self.incoming.insert(input, v);
}

if self.get_output(output).typ.splittable() {
self.outgoing
.entry(output)
.expect("Old OutputId")
.or_default()
.push(input);
} else {
self.remove_outgoing_connections(output);
let mut v = SVec::new();
v.push(input);
self.outgoing.insert(output, v);
}
}

pub fn iter_connections(&self) -> impl Iterator<Item = (InputId, OutputId)> + '_ {
self.connections.iter().map(|(o, i)| (o, *i))
self.incoming
.iter()
.flat_map(|(o, inputs)| inputs.iter().map(move |&i| (o, i)))
}

pub fn incoming(&self, input: InputId) -> &[OutputId] {
self.incoming
.get(input)
.map(|x| x.as_slice())
.unwrap_or(&[])
}

pub fn connection(&self, input: InputId) -> Option<OutputId> {
self.connections.get(input).copied()
pub fn outgoing(&self, output: OutputId) -> &[InputId] {
self.outgoing
.get(output)
.map(|x| x.as_slice())
.unwrap_or(&[])
}

pub fn any_param_type(&self, param: AnyParameterId) -> Result<&DataType, EguiGraphError> {
Expand All @@ -114,21 +171,23 @@ impl<NodeData, DataType, ValueType> Graph<NodeData, DataType, ValueType> {
}
}

impl<NodeData, DataType, ValueType> Default for Graph<NodeData, DataType, ValueType> {
impl<NodeData, DataType: DataTypeTrait, ValueType> Default
for Graph<NodeData, DataType, ValueType>
{
fn default() -> Self {
Self::new()
}
}

impl<NodeData> Node<NodeData> {
pub fn inputs<'a, DataType, DataValue>(
pub fn inputs<'a, DataType: DataTypeTrait, DataValue>(
&'a self,
graph: &'a Graph<NodeData, DataType, DataValue>,
) -> impl Iterator<Item = &InputParam<DataType, DataValue>> + 'a {
self.input_ids().map(|id| graph.get_input(id))
}

pub fn outputs<'a, DataType, DataValue>(
pub fn outputs<'a, DataType: DataTypeTrait, DataValue>(
&'a self,
graph: &'a Graph<NodeData, DataType, DataValue>,
) -> impl Iterator<Item = &OutputParam<DataType>> + 'a {
Expand Down
10 changes: 10 additions & 0 deletions egui_node_graph/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ pub trait DataTypeTrait: PartialEq + Eq {

// The name of this datatype
fn name(&self) -> &str;

/// Whether an output of this datatype can be sent to multiple nodes
fn splittable(&self) -> bool {
true
}

/// Whether an input of this datatype can be recieved from multiple nodes
fn mergeable(&self) -> bool {
false
}
}

/// This trait must be implemented for the `NodeData` generic parameter of the
Expand Down
2 changes: 1 addition & 1 deletion egui_node_graph/src/ui_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub struct GraphEditorState<NodeData, DataType, ValueType, NodeTemplate, UserSta
pub user_state: UserState,
}

impl<NodeData, DataType, ValueType, NodeKind, UserState>
impl<NodeData, DataType: DataTypeTrait, ValueType, NodeKind, UserState>
GraphEditorState<NodeData, DataType, ValueType, NodeKind, UserState>
{
pub fn new(default_zoom: f32, user_state: UserState) -> Self {
Expand Down
2 changes: 1 addition & 1 deletion egui_node_graph_example/src/app.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ fn evaluate_input(
let input_id = graph[node_id].get_input(param_name)?;

// The output of another node is connected.
if let Some(other_output_id) = graph.connection(input_id) {
if let Some(&other_output_id) = graph.incoming(input_id).first() {
// The value was already computed due to the evaluation of some other
// node. We simply return value from the cache.
if let Some(other_value) = outputs_cache.get(&other_output_id) {
Expand Down