Generalizing the diagram capabilities

[mxgrey]

I'm very excited to learn about this crate! I'm going to be working on an event graph framework and I'd love to be able to build on top of egui_node_graph and contribute back feature enhancements.

I'd like to list out a couple of changes to egui_node_graph that I think would be necessary to fit our needs so I can understand whether you'd be amenable to the changes (or have better ideas for how to do things) before I dive straight into making a PR. I'm happy to create a separate issue ticket for each of these items, but I thought I'd bundle it all up in one discussion topic for now since it all relates to my specific needs.

1. Function instead of Eq for testing data compatibility

It seems the DataTypeTrait uses its PartialEq + Eq trait bounds to determine if two data types are compatible. In my use case, there are separate sets of types for inputs and outputs. Certain output types can connect to certain input types, and it's not a 1:1 relationship. It would be helpful if DataTypeTrait exposed a method like

fn is_compatible(&self, from_output: &Self) -> bool

where self would be an input port and from_output is an output port, and it returns a bool for whether they can connect.

One solution would be to have this:

pub trait DataTypeCompatibility<UserState> {
  fn is_compatible(&self, from_output: &Self) -> bool;
}

impl<T: PartialEq + Eq, UserState> DataTypeCompatibility<UserState> for T {
  fn is_compatible(&self, from_output: &Self) -> bool {
    *self == *from_output
  }
}

pub trait DataTypeTrait<UserState>: DataTypeCompatibility<UserState> {
  ...
}

Alternatively if Eq is a trait that DataTypeTrait always requires for some other purpose, then we could have fn is_compatible(...) be a method of DataTypeTrait with a default implementation of *self == *from_output which users can override for more custom behavior.

2. Custom symbol rendering

To give a greater range of expression for inputs and outputs it would be nice if we could customize the port drawing of each data type instead of only differentiating them by color.

Maybe we could have something like

pub trait PortRender<UserState> {
  fn draw(ui: &mut Ui, user_state: &UserState, port_pos: Pos2);
}

pub trait CircularPortRender<UserState> {
  fn data_type_color(&self, user_state: &UserState) -> egui::Color32;
}

impl<UserState> PortRender<UserState> for CircularPortRender<UserState> {
  fn draw(ui: &mut Ui, user_state: &UserState, port_pos: Pos2) {
    /* insert existing implementation here */
  }
}

pub trait DataTypeTrait<UserState>: PortRender<UserState> {
 ...
}

3. Anchor points for connections

It's really great that loops are supported, for example:

However as you can see there's an ugly cross-over since the simple Bezier curve has trouble going backwards. It would be great if we could click+drag on a connection line to create an anchor point for the Bezier curve to manually pull it around the the boxes.

An even nicer long-term solution would be to use a little path search algorithm to find a collision-free path for the connection, but that's obviously a much more advanced feature which could be added later on top of the anchor point feature.

[setzer22]

Hi! Thanks for your interest in the library 😄 This idea of event graphs for robots sounds pretty exciting!

The things you are mentioning are all very well aligned with my vision for this library. And, in fact, all three features are things I'd been considering before. The implementation you suggest looks exactly as I would've gone for, so looking good :)

I do not have a lot of time to add new features right now, but I'd be more than happy to review and accept any PRs you send with these changes!

[mxgrey]

Awesome!

I might start by helping along this PR #30 since it's also relevant to my needs, and I'd like to avoid merge conflicts.

For the data type compatibility checking, do you prefer having a separate trait for that or just having a new method with a default implementation that uses Eq? I haven't figured out yet if the Eq trait bound is required for anything else in the data type system.

[setzer22]

That'd be great! 🎉

I might start by helping along this PR #30

Oh, yeah, that seems to have been stalled. I'm not sure if the original author intends on submitting the changes, but feel free to just drop in there and continue discussion 👍

For the data type compatibility checking, do you prefer having a separate trait for that or just having a new method with a default implementation that uses Eq? I haven't figured out yet if the Eq trait bound is required for anything else in the data type system.

The Eq trait bound was just for the compatibility checking. I don't think we can provide a default implementation only for implementors that implement Eq in a backwards compatible way, right? In that case, dropping the Eq requirement and adding the is_compatible method sounds reasonable

[mxgrey]

Introducing the DataTypeCompatibility trait and giving it an automatic implementation for all types that have the Eq trait would keep the API backwards compatible (... at least I think... I'd have to try it out to be completely sure).

The catch is it would prevent the user from customizing their data type compatibility behavior if their struct happens to derive the Eq trait. I can imagine cases where a user wants their data type descriptor to support the equality trait but also customize its compatibility behavior. So maybe breaking the API a little bit is okay to have the right level of flexibility.

To help API migration I could look into providing a derive macro that can be applied to types with the Eq trait so that they keep the old behavior. I haven't implemented one of those before, but it's worth learning how to.

[setzer22]

The catch is it would prevent the user from customizing their data type compatibility behavior if their struct happens to derive the Eq trait.

Yes, this is the main issue right there. I can imagine that directly affects your use case, for example. The fact that a user happens to derive Eq on a struct shouldn't change this library's semantics in any special way.

To help API migration I could look into providing a derive macro

I think that's a bit too much. The implementation itself is three lines of code, not much more than what a macro would be. And having a derive macro requires bundling a separate egui_node_graph_derives crate due to how Rust procedural macros work, so that would be extra maintenance effort for a very small added value