zakarumych / egui-snarl

Customizable egui node-graph library
https://crates.io/crates/egui-snarl
MIT License
252 stars 25 forks source link

[not an issue] showcasing changes for viewing a behaviour tree #15

Open iwanders opened 5 months ago

iwanders commented 5 months ago

Thanks for sharing this library with everyone! Just like @plasticbox I'm trying to use this library for visualising and editing a behaviour tree.

In behaviour tree the order of children is very important and nodes (usually) have just one parent, but can have multiple children, some nodes can just have one child, others can have any number of children. I also wanted my behaviour tree to be able to visualise top-to-bottom, so I made some changes in a fork over the past few weeks, I had intended to make an issue here to possibly contribute back some of my changes, but wanted to get some actual experience using my modifications first to ensure I was happy with it and it worked well. I'm still working on my behaviour tree project, so hadn't gotten around to that yet.

I did subscribe to https://github.com/zakarumych/egui-snarl/issues/7 however, and today I saw a huge commit landed with 4f9bff3ab1ba76585d447d1c40d4abec08e0db79, which will make rebasing my branch a tricky endeavour, if possible at all. With such a large change I'm not sure if I have the time in the near future to consolidate the branches. That's absolutely fine, this is your library, your party, and I'm happy to see active development on it. So I thought I'd just file an issue this evening to showcase what changes I did to get where I'm at, they may provide useful feedback and I guess some of the changes could still be easily applied if you want to incorporate them.

For making this issue, I used this branch, and the tree example, this is the diff of that branch from the branch point.

I made a video, that I think showcases most of the things I've modified:

https://github.com/zakarumych/egui-snarl/assets/1732289/f50186a3-49cf-4b7f-ab92-9ff3e627cefd

Rough changelog, with some commits from the branch. These are commits during development, not the final state on the branch, but it should still give a feel for approaches I took.

  1. I first worked on adding vertical inputs and outputs, I introduced vertical_output for the bottom row and vertical_input at the top, these return Option<PinInfo>, making it an opt-in mechanism. Took a few commits, but 593dc048590021e7d25cdce7ebd38eeae17a3f60, 48e754c5f2ee913cf26bf1918db0b57e95752770 and 48e754c5f2ee913cf26bf1918db0b57e95752770 show the rough approach, changed a bit more later, outputs in bc2858227e5c1f82fc15bac92e2083dcef0f7075.
  2. Modified PinInfo to support only showing while wiring is happening, as well as supporting storing a boolean in them denoting whether they are verticals. Bugfix to ensure populating the pin map for wiring-only pins later in f0995696fb19305ee133e3fd669b7079bf6ccf0f.
  3. Introduced a SimpleWire boolean in the snarl style to ensure vertical wires were smooth. 082e7d0063f3cf83cc10b0a4e023d03488eacbcb
  4. Order has a big influence in behaviour trees, so I added support to swap wires by dragging them onto each other, in 5b3335a26559e0d0141c45bb4ea39b9c576c278c and a95d60990ca145015ab2e5a9bdc27efec3d9be09
  5. Then I added out_pins_connected and in_pins_connected to query the wires that snarl has for a node, that allowed me to easily implement variadic number of inputs and outputs without ever making pins that have wires disappear. 7cbffb0b2dc20fc58d8612e5fe337d17e0cae21f, and d9ee7fe56da3637f33a293e18105ad35c7890258 fix later
  6. Then I discovered that swapping wires wasn't enough, I needed to be able to move them, this required some careful thinking about what to do if the sets overlapped, or didn't overlap, but I think it's working fairly intuitive now, d0f7d5962d6d552f98f0ab6acfc8214734e8eed4 and 704cc05466145234b0259f045ee964d59c7faa49, and then the same for inputs with 8df1fa9dcdb417761e040782b1d056fbdd0e6af7
  7. I also started work on selection, with 9395eff912ba8b397070b72d5abb982f300b94a5, 99a20f9fcbf5c2905e49d7f5527c946a221b53fd and fix for drag this evening, just to make it look better; 92353fd36c08eaf8c80ebfcd8b1d8763792ad39c
  8. I decided it would be up to the viewer itself to decided how to visualise the selection, so I introduced node_stroke, node_fill and selection_pending in 618f99fe0f86174686b8ec2d507ae018d34954cc which doesn't add more elements to draw[^1] and has the benefit that it allows coloring the node stroke based on state in my behaviour tree. Which is something I needed anyway.

Some other minor improvements, that should be easy to incorporate.

Again, most changes pointed at in the commits were later changed in some way. I didn't keep a super clean commit history, but it should still allow you to get a feel how I went about things. So main changes are the following:

  1. Vertical inputs and outputs.
  2. Allow implementing variadic inputs and outputs.
  3. Allow modifying stroke & fill for nodes.
  4. Allow moving & swapping wires.
  5. Simple wire shape that just makes a smooth Bezier without curls.
  6. Approached selection differently; do as much handling in the user-code as possible instead of the library.

I had originally hoped to contribute back most of the changes as individual PRs, but the large divergence between our branches now means that would require more time than I'm willing to commit right now. So, maybe it's best to just read through this and accept this all as feedback and clear indication of how your library provides value to others! Since this is not an issue, feel free to close it as you see fit, if you do want to consume some changes, please feel free.

[^1]: Before picking this library I investigated the options; (egui_node_graph, egui_cable, egui_graphs) and ultimately selected this one to use as a basis because it did provide most things I needed and it was the most performant (though testing with like 60 nodes I was already concerned with CPU load on my pc).

zakarumych commented 5 months ago

Thank you for sharing this. I understand that merging with big changes is tough, sorry for inconvenience.

I intended to support pins on top and bottom of the node, but failed to figure out how to do so yet :) As you can see, there's not much room for pin content. So I postponed it. Maybe leaving them content-less is OK. For style consistency I'd keep them inside node's frame.

I like wire swapping and moving idea, although it's not clear to me how wires behave when multiple are moved.

If you want to modify node's look via SnarlViewer then better do it with everything, not just selection. So UI code can just ask SnarlViewer what Frame it should use for the node and its header and selection style as well. And it'll user one from SnarlStyle by default.

With your permission I'll take some code from your branch to implement some of described features.

iwanders commented 5 months ago

I understand that merging with big changes is tough, sorry for inconvenience.

No worries, like I said, this happens sometimes, especially if people develop large changes in parallel!

Maybe leaving them content-less is OK.

Yeah I think so, in my current version, rendering a small number is reasonable. I expect that for most use cases that want the vertical inputs & outputs, they'll all be of the same 'type' so rendering them without any labels is probably fine.

For style consistency I'd keep them inside node's frame.

I considered this, but for the behaviour tree case I thought it would take too much space and push the name of the node too far down.

I like wire swapping and moving idea, although it's not clear to me how wires behave when multiple are moved.

The last port touched becomes the anchor, then pins are moved relative to the anchor. The actual logic I ended up with is the following;

// If going from output to output, we can do moves and swaps;
// Simple one to one swaps are simple... but moving a block of outputs
// by holding shift is better, it also makes this logic complex :(
// Lets follow the following logic:
//  The last pin on left is called the anchor. It is the last selected pin.
//  The anchor moves to the right out pin, this is the most intuitive.
//  With this logic, we end up with two sets of pins:
//    - The source pins from left_out_pins, with pin indices relative to the anchor.
//    - The destination pins from right_output, pin indices relative to anchor, offset to right out pin.
//  If the source and destination pins are disjoint sets, we swap the pins.
//  If the source and destination pins overlap, we only allow the operation iff the destination pins
//   are empty after the source pins would be disconnected. This avoids having to think about where to move
//   that pin and potentially make a wrong choice, and it also doesn't make assumptions about the 
//   network allowing multiple outputs.

Copied from 8df1fa9dcdb417761e040782b1d056fbdd0e6af7, it did become quite involved. But it does allow moving the ports on the node itself, as well as swapping ports between two nodes. Which is exactly what I wanted for my behaviour tree. I'm not sure if this generalises to well for generic flow graphs though. I think it would be easier to understand how the moving works if the 'anchor' node is drawn a bit different, just as a visual aid.

If you want to modify node's look via SnarlViewer then better do it with everything, not just selection. So UI code can just ask SnarlViewer what Frame it should use for the node and its header and selection style as well.

Yeah that's probably a good idea! I was pondering how I could make a 'nametag' type element in the viewer that takes just a single input or output, to avoid having a very long wire going across a large graph.

With your permission I'll take some code from your branch to implement some of described features.

Yeah go for it, you could attach both our names to commits with Co-authored-by if you wanted to, but no real need from my side.