Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
iwanders opened this issue Apr 12, 2024 · 2 comments
Open

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

iwanders opened this issue Apr 12, 2024 · 2 comments

Comments

@iwanders
Copy link

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 #7 however, and today I saw a huge commit landed with 4f9bff3, 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:

2024_04_11_showcase_vertical_input_output_snarl.mp4

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 593dc04, 48e754c and 48e754c show the rough approach, changed a bit more later, outputs in bc28582.
  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 f099569.
  3. Introduced a SimpleWire boolean in the snarl style to ensure vertical wires were smooth. 082e7d0
  4. Order has a big influence in behaviour trees, so I added support to swap wires by dragging them onto each other, in 5b3335a and a95d609
  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. 7cbffb0, and d9ee7fe 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, d0f7d59 and 704cc05, and then the same for inputs with 8df1fa9
  7. I also started work on selection, with 9395eff, 99a20f9 and fix for drag this evening, just to make it look better; 92353fd
  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 618f99f which doesn't add more elements to draw1 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.

  • Hint at implementing has_body() for the show_body() function d5b7117, I originally forgot this as I expected show_body() to always be ran.
  • Prevent rendering the body if the node is closed, cpu & space saving; 792eb23.
  • Made title non-selectable with .selectable(false) such that you can drag the node if you click on the node title (folded into bc28582), significant quality of life change.

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.

Footnotes

  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
Copy link
Owner

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
Copy link
Author

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 8df1fa9, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants