Usability problems

[blitzarx1]

Now users are facing very verbose generic definitions and multiple steps to perform simple task on elements/graph updates.

I will keep this ticket here and definitely think of some optimizations of the process or introducing macros before coming to stable releases.

[patrik-cihal]

Yes, so to create edges I'm doing this:

let ei1 = gc.g.add_edge(creating_node_id, NodeIndex::from(payload.id as u32), Edge::new(edge_data));
gc.g.edge_weight_mut(ei1).unwrap().bind(ei1, 0);
if EdgeData::Equivalence == edge_data {
    let ei2 = gc.g.add_edge(NodeIndex::from(payload.id as u32), creating_node_id, Edge::new(edge_data));
    gc.g.edge_weight_mut(ei2).unwrap().bind(ei2, 0);
}

[blitzarx1]

Thanks for raising this issue, such common workflow should be simple. I will prioritize this.

[oisyn]

Having worked with this nice lib the last couple of weeks, I have some thoughts I wanted to share 🙂.

1

I think the new setup with the DisplayNode implementation needing to carry the node's state is a bit weird. It basically results in a partial or complete NodeProps copy that needs to be done each frame. The methods in the DisplayEdge trait take references to the nodes as input parameters. Can't we do the same for DisplayNode?

2

Related to this is that From<NodeData<_>>::from() is called before the node is bound. So any set properties (such as location) isn't even up to date at that point. So you're basically initializing it with default data, and then in the update() get the actual location. I see two potential solutions here:

1. Wrap the Node::display member in an Option or MaybeUninit and initialize it after the transform
2. Drop the member from the node, and create it each time for the GraphView

1 Seems fairly easy to implement, but I think 2 makes more sense as it's a typical "view" thing (but then again, so is a property like location, so the distinction between model and view is not that strictly defined). Also, 2 removes the need to have the NodeDisplay and EdgeDisplay be generic parameters of the graph. The downside for 2 is that you either need to allocate an associated datastructure, or you have to recreate the display nodes each time you call one of its methods (or just make them functions that don't take self, especially if they get a ref to the Node as parameter).

3

It's a bit convoluted to reference other state in the custom draws, to the point where you basically need to add data to each and every node to get access to some other datastructure in your program. I'd suggest adding some custom context to the GraphView, which is then passed to the various callbacks.

4

There has been an explosion in generic parameters that keep needing to be repeated everywhere. Have you considered using a trait that encapsulates all the current generic parameters as type aliases, and then have a single generic parameter that implements this trait?

5

It would be nice if egui_graphs forwarded symbols from petgraph. To be able to use egui_graphs, currently requires you to also include a dependency on petgraph. This is especially true for the traits and types that are being used for the generic parameters in Graph, such as Undirected and DefaultIx. If egui_graphs forwarded any needed symbols, a user won't have to explicitly reference petgraph.

I'll be happy to take on some or all of these points if we can reach a consensus on some spec 🙂

[blitzarx1]

Hey @oisyn, thanks for the feedback!

1. Passing node reference inside DisplayNode implementation was my first attempt to implement it, but I faced difficulties when I added DisplayNode inside of the Node definition, which caused definition of DisplayNode become self referencial in generic types. That's when I decided to decouple NodeProps from node. I agree that it causes excessive copying, but if you could suggest better approach, I am ready to discuss it.
   P.S. I think I will have to remove implementation of DisplayNode from node fields, that will solve the problem. I will need to keep this implementations somewhere in the Graph object. I think we need to use this wrapper more, because this can be our way to store graph state and adding display implementations to it seems reasonable. Because now it just plays role of a wrapper with some useful API, while it can do much more being a mutable graph state living outside the widget. I am already going to store inside it selected and dragged nodes and edges (Keep selected and dragged nodes and edges inside Graph wrapper #141), because now they are stored in the widget state which is not really good.
2. I don't like the whole approach with node binding and I think it can be avoided, this is in my shortlist. But speaking about your approaches number 1 and 2 they both not allow to keep NodeDisplay implementation state inside Node which is very useful for animation drawings or any stateful drawing. Also I'd like to separate Node properties and drawing implementation properties. I see your proposals and I think this needs more comprehensive discussion. If you can just show some POC which satisfies the requirements for stateful drawing and separation graph props of the node from drawing props I will be glad to discuss it.
3. Totally agree. I already have a DrawContext but maybe I need to widen it's usage.
4. Yes, I have tried this approach mid implementing Display trait feature. But I decided to avoid it until we have stabilized version of generics signature. I agree that this is the way to go and we need it before stable version.
5. Sorry I have not done my part of research on it. Thanks for the proposal. Maybe this is a good item for making a PR right away.

Most of these are maturity issues caused by the new …Display trait feature which decouples nodes from their drawing implementations and adds more flexibility for custom drawing implementations. We definitely need to address them before becoming stable.

I am really grateful that you spent time thinking of the ways to make this lib better. Feel free to create issues and PRs on any of the points for further discussion and fixes or we can continue here on some of them.

[Atlas16A]

3. Totally agree. I already have a DrawContext but maybe I need to widen it's usage.

In reference to this, how can I access the DrawContext? Im wanting to draw some extra content to the graphview (Node grouping stuff) but im unsure how to gain access to it to do so.

[blitzarx1]

3. Totally agree. I already have a DrawContext but maybe I need to widen it's usage.

In reference to this, how can I access the DrawContext? Im wanting to draw some extra content to the graphview (Node grouping stuff) but im unsure how to gain access to it to do so.

You can get access to DrawContext implementing NodeDisplay or EdgeDisplay in method fn shapes(ctx: &DrawContext) -> Vec<Shape>. It returns shapes which will be drawn, so you can add to this vector any additional shapes you want or you can use ctx.ctx to get egui::Context and get low level painter from there (ctx.ctx.layer_painter()).