First approach for bevy_graph

[DasLixou]

Objective

First approach for bevy_graph from #6719.

Tasks

• Add all graphs from @maniwani in `bevy_graph` Requirements Gathering #6719 (reply in thread)
  • Simple
    • SimpleListGraph
    • SimpleMapGraph
    • maniwani's SimpleLinkedListGraph (will do later)
    • SimpleMatrixGraph (will do later)
  • Multi
    • MultiListGraph
    • MultiMapGraph
    • maniwani's MultiLinkedListGraph (will do later)
    • MultiMatrixGraph (will do later)
• Directed and Undirected Edges
• Algos
  • BFS
  • DFS
• Tests
• Documentation
• Benches
• Think and talk more about API with the contributors who need them
• Try to use bevy_graph where possible already
• Implement all other features requested from `bevy_graph` Requirements Gathering #6719

---

I don't have much experience with graphs but i wanted to learn them so when you want to give feedback or help, I'll be happy 😅

[alice-i-cecile]

@cart could we get a A-Pointers and A-Graph tag? Those areas are reliably missing area labels and it would be helpful for filtering.

[DasLixou]

Sorry for the delay here. This is a very big chunk of code and it doesn't unlock any immediate new Bevy features, so it has been hard to prioritize (and I've been extremely focused on getting the bevy asset rework MVP out).

That being said, having a go-to graph library for bevy is an important investment in the future. I do want to get this reviewed and merged in the near future, but I'll personally need at least another week or two.

I understand. Bevy asset rework sounds interesting :o will it also have preconverting to other formats which are easier?

[cart]

I understand. Bevy asset rework sounds interesting :o will it also have preconverting to other formats which are easier?

Yup it will enable writing asset loaders that do this!

[jguhlin]

Hey, I'm quite keen for this feature, is there anything I could do to help or should it wait until this PR has been accepted?

[DasLixou]

Hey, I'm quite keen for this feature, is there anything I could do to help or should it wait until this PR has been accepted?

The base PR is pretty much done now. It's a little older code, maybe there are some things that are now safe to do. Also I'm not 100% about some unsafe things. So reviewing the unsafe things and testing would be helpful 😄

[DasLixou]

So im quite happy now about this PR but since the from @cart mentioned asset rework isn't done yet thus it didn't manage to get into 0.11 I think this will get delayed again, right?

[alice-i-cecile]

@dmyyy, I'd be interested in your opinions on this proposed API :)

[dmyyy]

@dmyyy, I'd be interested in your opinions on this proposed API :)

I'm working on graph related stuff in Bevy using petgraph - I'll see if I can dogfood some of this work.

[dmyyy]

Why is this function unsafe? I'm relatively new to rust to rust but from what I understand you only use unsafe for "unsafe superpowers" which I don't think we need here.

[DasLixou]

It's unsafe because you as the user shouldn't play with it. It's needed for the iterator which turns EdgeIdx into EdgeMut because we can't directly have two mut borrows of the graph even if we need different fields. So I did this to expose a field

[alice-i-cecile]

That justification feels more like an argument to have it private, than for it to be unsafe. Unsafe is strictly used in Rust for things that can directly cause memory safety problems when used, which doesn't seem to fit here.

[dmyyy]

In my case I'm filtering the graph for some specific nodes. I need the indices later on (to get neighbors of that specific node etc.). I feel like this use case is pretty common (the function hands back an immutable reference so I don't think the user can do anything bad).

[DasLixou]

Yeah i probably should make a sub-trait which is private and exposes the field.
@dmyyy do you think i should add the EdgeIdx into the Edge{Ref/Mut} types?

[dmyyy]

I'll be reviewing bits and pieces here and there as I integrate this with my project (so it might take me a while to get through everything).

As an aside - any new graph-using features should probably be implemented using bevy_graph - might be the only sane way to review all of this code.

[dmyyy]

I'm curious about the rationale for using SlotMap over HashMap/IndexMap (like in the petgraph GraphMap implementation). I found an interesting github issue here from a polygonal mesh processing crate.

The TLDR is that they assumed SlotMap would be faster than HashMap for their graph storage but after benchmarking found AHash HashMap to be ~25% faster than SlotMap. We can allow using different hashes if we need cryptographically secure hashes. They go on to list several reasons why HashMaps are more flexible and might be easier to work with. I'm partial to what IndexMap promises (fast iteration, high density/low space overhead) - seems like a good fit for a game engine.

[DasLixou]

I'm curious about the rational for using SlotMap over HashMap/IndexMap (like in the petgraph GraphMap implementation). I found an interesting github issue here from a polygonal mesh processing crate.

The TLDR is that they assumed SlotMap would be faster than HashMap for their graph storage but after benchmarking found AHash HashMap to be ~25% faster than SlotMap. We can allow using different hashes if we need cryptographically secure hashes. They go on to list several reasons why HashMaps are more flexible and might be easier to work with. I'm partial to what IndexMap promises (fast iteration, high density/low space overhead) - seems like a good fit for a game engine.

@dmyyy the question is how this will be implemented. I need something for nodes and edges which works as a key, is persistent, fast, and is unique. I don't think HashMap/IndexMap works here, because that would be indexed by either the hash of e.g. the node (but what if i want 2 nodes with the value '42'?) or the index, which could move when adding / removing. (gotta say i never worked with IndexMap, thats just what i got after reading the main doc page).

[dmyyy]

I need something for nodes and edges which works as a key, is persistent, fast, and is unique.

The solution petgraph uses is forcing the node weight N to be the key (implement Hash + Eq) and having it be Copy. This is what it looks like. nodes stores all the neighbors of the current Node.

#[derive(Clone)]
pub struct GraphMap<N, E, Ty> {
    nodes: IndexMap<N, Vec<(N, CompactDirection)>>,
    edges: IndexMap<(N, N), E>,
    ty: PhantomData<Ty>,
}

The downside is node weights have to be primitive which isn't always the case. We can do something like this (looks like what you have today but with HashMap instead of SlotMap).

#[derive(Clone)]
pub struct GraphMap<N, E, Ty> {
    nodes: IndexMap<NodeIdx, N>
    edges: IndexMap<(NodeIdx, NodeIdx), E>,
    neighbors: IndexMap<NodeIdx, Vec<(NodeIdx, Direction)>>,
}

[dmyyy]

I've looked through quite a bit of this pr - the code quality looks good but I have a couple concerns about the direction of this feature.

I'm a little bit worried about the scope of this pr - I've read through the issue and found a lot of discussion about different graph data structures that exist (list, map, matrix, multi, hyper etc.). I feel like the focus shouldn't be on implementing everything under the sun that's tangentially related to graph theory (there's a lot).

The problem is there isn't an immediate need tied to this work right now. I definitely want something that's nicer to use than petgraph but I'm not sure if it's the right move to merge things that we "might" need in the future (referring to multi graphs) without a clear use case they will be used for (where work for that feature has at the very least started design/development).

Reading through other comments in the issue I wonder if we even want a generic graph library. I would love something like this for game dev, but looking at the requirements for Audio for example make it seem like we might be better off with graph implementations tailored and optimized to their specific use cases in the engine.

Implementation should ideally be driven by a need in the engine (otherwise why is this here vs. being in a third party crate?). I think most use cases in game engines/games are well served by a SimpleGraphMap - we should identify upcoming work (Animation? @james7132) and see if we can get an MVP bevy_graph with a SimpleGraphMap and required algorithms (DFS, BFS, etc.) using that as a forcing function.

I feel like there should be more discussion around this in general - would be happy to hear other opinions @alice-i-cecile @maniwani @cart

Edit:
There should also be some more alignment on the bevy_graph vision - if I want an algorithm (say A star) but there's no use case for it in the engine - can I have it added to bevy_graph? What about a more esoteric algorithm - where do we draw the line? Is this a general graph crate users can use for games? Or strictly used for in-engine use cases?

[alice-i-cecile]

I generally agree with that analysis: I'd like to ship something small and functional in this PR and evolve from there based on our needs and those of external users.

[cart]

I also agree with the general assessment. Thats the main reason I've continually put off reviewing this. It might be what we want eventually, but we have no current need driving it, so how will we know? This is also a ton of code to review and maintain without a use case.

I think we should close this, with the general plan being to try using it partially (or fully) whenever the need arises in a specific Bevy feature. The developer(s) building a feature that need a generic graph can depend on the code from this branch and see what works. From there we can determine what is needed, what is not, and what needs changing. We can then create a new PR with whatever comes from that.

@alice-i-cecile it will be on us to remember to direct people to this code to avoid re-inventing the wheel. And we need to make sure @DasLixou gets attribution if we build on this (both on github and when the feature is released).

[maniwani]

I agree that we currently don't have a pressing need for a bevy_graph and that's probably a fair reason to close this PR. But it's definitely wrong to think there's no motivation for one at all.

(Also, sorry you were kept waiting for so long, @DasLixou.)

---

The original reason I got behind "a bevy_graph should exist" was system scheduling. I "made do" with petgraph when I implemented schedule V3, but since then there's been generic graph code living in bevy_ecs.

It's undeniable that various engine crates involve (or are likely to involve) graphs. And they don't (or won't) do anything special with those graphs. They should use a shared library, but there are none besides petgraph (whose API is really bad). This PR was the start to a "proper" one.

I'm a little bit worried about the scope of this pr - I've read through the issue and found a lot of discussion about different graph data structures that exist (list, map, matrix, multi, hyper etc.). I feel like the focus shouldn't be on implementing everything under the sun that's tangentially related to graph theory (there's a lot).

The scope of what this PR had implemented—a few basic graph data structures and DFS/BFS—was fine in my opinion. It's a complete mischaracterization to suggest it was attempting "everything tangentially related to graph theory".

The "simple/multi" and "directed/undirected" are just two mathematical properties of graphs. The "list/map/matrix" are ways to represent one. I do agree that the "map" representation could serve most of our uses (I did say so early on).

Reading through other comments in the issue I wonder if we even want a generic graph library. I would love something like this for game dev, but looking at the requirements for audio for example make it seem like we might be better off with graph implementations tailored and optimized to their specific use cases in the engine.

"Tailored" and "optimized" in what way? Does audio (or any other application) do something with graphs that no other relevant application does? In a way that no other application does it? I doubt it. None of the potential users of bevy_graph are special. A graph of audio modifiers looks the same as a graph of shaders. They're just interpreted differently.

When you look beyond the superficial differences, they're the same. Graphs and their algorithms are primitives. (This is similar to how "volume control", "animation", "pathing", and many other things boil down to interpolating curves/splines.)

Implementation should ideally be driven by a need in the engine (otherwise why is this here vs. being in a third party crate?).

I don't think the shared graph library would need the bevy_ moniker. But it wouldn't be strange if the bevy organization releases and maintains one as no one else is writing a library that will serve our needs.

[dmyyy]

It's a complete mischaracterization to suggest it was attempting "everything tangentially related to graph theory".

Sorry if what I wrote was unclear - I was more so referring to the linked issue with regards to listing graph types that can be useful. The scope I'm worried about is referring to the amount of code we would be merging without a clear client/consumer lined up.

"Tailored" and "optimized" in what way?

This was an off the cuff comment after reading through the issue - a lot of the domain-specific asks like

Preferably a way to do change detection on parts of the graph, tracable back up to the root nodes. This can help optimize the traversals.

for Animation made me question the overall generalizability and wonder if different areas might be better suited with their own implementations.

They should use a shared library, but there are none besides petgraph (whose API is really bad). This PR was the start to a "proper" one.

Totally agree - I'm not a fan of petgraph's api either. I am interested in helping drive some of this work which is why I proposed identifying future work that needs graphs and packaging this with that work. I also think it's worth aligning on bevy_graph's vision - where the crate should live, and who its customers are (engine-only, vs. engine and games) etc.

Edit:

The original reason I got behind "a bevy_graph should exist" was system scheduling. I "made do" with petgraph when I implemented schedule V3, but since then there's been generic graph code living in bevy_ecs.

Maybe a good first step would be using parts of this pr and splitting out the generic graph code in bevy_ecs into bevy_graph.

[maniwani]

I was more so referring to the linked issue with regards to listing graph types that can be useful.

Ah, I gotcha.

Animation made me question the overall generalizability and wonder if different areas might be better suited with their own implementations.

I'm not an animation (or audio/rendering) expert, so I'll try to keep an open-mind, but I'm very skeptical.

Just to go with that example, change detection seems like something you can store on a node or edge type (maybe with some kind of custom "visitor"). I can't see it "shaking up" the storage itself or traversal in a fundamental way.

In an earlier version of schedule V3, I made dependency graph updates "incremental" (parts that didn't change could be skipped when solving the graph again) and it was still "on top of" petgraph. IIRC all I did was add a "dirtied" field to my node type and use BFS to propagate it (up the system set hierarchy).

Like, system scheduling is a "power user" of graphs and graph algorithms, but I don't think the ones from petgraph could have been optimized any better (performance-wise) for it.

So yeah, graphs seem like curves/splines to me. Context doesn't change what a spline is or what an efficient implementation of "spline math" looks like, only what a spline can represent.

---

Side note: I think ECS relationships will replace standalone graph structures in many use cases. Scheduling is one of them. That said, I still see use for "standalone" graphs, and one such use would even be part of internal relationship logic.

[mockersf]

I haven't use petgraph much, so I won't judge this PR API compared to it. But if this PR is complete enough, it could be released as an independent crate, and people can start experimenting with it in Bevy without it being in tree. And it could benefit other people too.

[DasLixou]

@mockersf do you mean like i should "host" it or how?

[mockersf]

you could publish it on crates.io, with a different name than bevy_graph though

[cart]

I think @DasLixou publishing as a separate crate (with a different name) so we can experiment with adoption + upstreaming is a great middle ground solution.