blitzarx1 / egui_graphs

Interactive graph visualization widget for rust powered by egui and petgraph
https://docs.rs/crate/egui_graphs
MIT License
419 stars 30 forks source link

Added serialization attr for default stuff and some constructor helpers when creating nodes #134

Closed patrik-cihal closed 11 months ago

patrik-cihal commented 11 months ago

Actually with_location doesn't make much sense if I need to bind it first... I'll remove it..

blitzarx1 commented 11 months ago

Good point. These api is in refactor now. I am not sure about bind approach either.

patrik-cihal commented 11 months ago

Yeah what is it for?

blitzarx1 commented 11 months ago

It is mainly needed to bind user created node to the NodeIndex of the node from the graph. Because we can not simultaneously create node weight and bind node index inside this weight .

patrik-cihal commented 11 months ago

Does the node need to know its own index?

blitzarx1 commented 11 months ago

It helps to simplify many internal processes. For node this is not much beneficial, bot for edge it is very beneficial. Because for drawing we use order of and edge to bend it accordingly. And to compute order we need always to be aware of edges endpoints and compute their order on every frame - for that we would need to pass graph reference inside drawing context, which would make generic system even more complicated.

But I think I will rethink this approach soon, now when we have more or less stable implementation of NodeDisplay and EdgeDisplay trait, I think I can find a way to optimize it and keep only order prop inside of edge and recompute it only if graph is changed (edge/node added or deleted)

patrik-cihal commented 11 months ago

Yes it is so hard to even add a node. Perhaps I'm doing something wrong...

let mut new_node = Node::new(NodeData{ content: "".into() } );
                    new_node.set_location(Pos2::new(new_pos.x, new_pos.y));
                    self.gc.write().unwrap().g.add_node(new_node);

And it hasn't even been binded now.

blitzarx1 commented 11 months ago

Yep, I will think about some macros or further refactorings. But I think your case can be covered with transform::add_node or transform::add_node_custom

patrik-cihal commented 11 months ago

I would simply avoid unbinded nodes altogether. If something is unbinded it can be represted just as NodeData.

patrik-cihal commented 11 months ago

Oh nvm that's prob not possible.

blitzarx1 commented 11 months ago

I will definitely prioritize usability before coming stable.

patrik-cihal commented 11 months ago

What do you mean by that? Do you think unbinded nodes are necessary?

blitzarx1 commented 11 months ago

135 Added our concerns hers.

No I just agree that usability is becoming an issue, and I need to work in this direction before releasing stable versions. If you would have any proposals in this regard, we can move the discussion to #135

About binding nodes I am almost sure that this can be avoided and just have one-step process for creating, modifying or deleting node.