New Dijkstra2D/3D classes to implement breadth-first pathfinding

[halgriffiths]

What is it?

This PR implements Dijkstra's Algorithm for one-to-many pathfinding as a new Reference-derived class.
Currently the only in-engine pathfinding class is Astar, which offers one-to-one pathfinding only, so this represents a nice functionality boost.

Dijkstra's algorithm is super useful for (eg) tower defense games, or any other case when you have many entities all pathfinding to the same location.

Example

To test things, I made a little test scene which you can play with. The little numbers are the calculated distance to the target.
Download: example.zip

Example

Note on reviewing

This PR's new files are structured almost identically to the pre-existing astar.h and astar.cpp. I know it might look a bit scary to review, but if you diff dijkstra.cpp with astar.cpp you will find that most of the code is identical :-)
For example: https://www.diffchecker.com/p5UEsAXc

At some point in the future we could cut down on this duplicated boilerplate by creating a base "PathfindingGraph" class with both AStar and Dijkstra as child classes, but thats a ticket for another day methinks.

This change could in theory be backported to 3.x, it should be fully compatible.

[halgriffiths]

One thing I am not sure about is the question of when to do path recalculation.

The key benefit of Dijkstra's algorithm is that you only have to calculate distances for the whole graph with recalculate() once as an upfront cost, and then subsequent calls to get_path_points(from_id) should be lightning fast.

Currently I require the user to specify when they want to call recalculate(), and if they try to pathfind on an out-of-date graph then I return an error.

Pros	Cons
User has complete control over when they want to call recalculate(), and can choose to defer it to less-busy moments	A bit clumsy for the user to have to remember to call recalculate() manually themselves

Alternatively, I could make it so that if the user tries to pathfind on an out-of-date graph then we automatically call recalculate(), which would have the effect that the first get_path_points() call might be unusually slow, but would make it easier for users.
The main cost here will be that the user might not always need to have the graph recalculated every time it changes, so that might be an annoying performance cost.

[halgriffiths]

PS: I'm not going to bother to add documentation until this PR is approved/rejected

[Calinou]

Out of curiosity, how does Dijkstra navigation compare to using a NavigationMesh?

[fire-forge]

This looks very useful. Dijkstra's Algorithm also has lots of uses in procedural generation.

I'm not going to bother to add documentation until this PR is approved/rejected

That's fine, but the basic XML files without descriptions should at least be added. This is very easy to do since static checks will generate them automatically from once they get past the formatting and style issues. See the Documentation Checks section in https://github.com/godotengine/godot/runs/7809541457?check_suite_focus=true.

At some point in the future we could cut down on this duplicated boilerplate by creating a base "PathfindingGraph" class with both AStar and Dijkstra as child classes, but thats a ticket for another day methinks.

I agree that that would be a good change. It would also lower the difficulty of adding other pathfinding algorithms with scripts or extensions, similar to what was done recently with the Noise class.

[halgriffiths]

Out of curiosity, how does Dijkstra navigation compare to using a NavigationMesh?

To be honest, I don't actually understand how NavMeshs work.
Looks like the relevant code is https://github.com/AndreaCatania/godot/blob/383c583a0b46b36ab9b0de57d0f3f7bdecb62fc8/modules/gdnavigation/nav_map.cpp#L82

I think its one-to-one (albeit with caching) though, which means that Dijkstra is always going to be more efficient for one-to-many situations (citation needed).

[halgriffiths]

Barebones docs have been added. Will fill out properly later.

[smix8]

I know it might look a bit scary to review, but if you diff dijkstra.cpp with astar.cpp you will find that most of the code is identical :-)

That is actually my main problem with this pr, Dijkstra and AStar are just two algorithm for the same graph-thing with some changed meta data for the points so making this its own class without a shared bases is like 90% redundance. What should have happend is a shared "PointNavigation" class (that could have also fixed the majority of AStar class flaws in one sweep) and added the algorithm as an option or extend it to the 4 AStar/Dijkstra classes if preferred.

Out of curiosity, how does Dijkstra navigation compare to using a NavigationMesh?

That are two different things, Dijkstra is basically AStar without the heuristic so the search pattern is different and the navigationmesh is just a representation of a graph that works for both AStar or Dijstkra or many other algorithm. Dijkstra without early break (when it finds the first path) is one of the slowest algorithm available but it finds the near perfect shortest path even with complicated point connections while AStar creates very strange / ugly paths on complicated layouts, e.g. teleport connections or complex point weights. If the AStar heuristic is changed so much that it avoids those pathfinding flaws it turns into Dijkstra.

The oversimplified gist is, if you want quality or preprocess a navigation setup you use dijkstra, if runtime speed is the main concern you use AStar.

[halgriffiths]

That is actually my main problem with this pr, Dijkstra and AStar are just two algorithm for the same graph-thing with some changed meta data for the points so making this its own class without a shared bases is like 90% redundance. What should have happend is a shared "PointNavigation" class (that could have also fixed the majority of AStar class flaws in one sweep) and added the algorithm as an option or extend it to the 4 AStar/Dijkstra classes if preferred.

Personally I think that including a refactor of Astar2D and Astar3D would make this PR too big and unwieldy, as well as opinionated. It also would make reviewing more difficult, and I always feel too guilty to ask open-source reviewers to do too much. I would prefer to split it, and then I (or somebody else) can come back to do the refactor in a second PR.

[Zireael07]

we could cut down on this duplicated boilerplate by creating a base "PathfindingGraph"

Referring you to godotengine/godot-proposals#3848

[MGilleronFJ]

I have nothing against Dijkstra, but one concern I share with the last comment of the initial post is that it's an entire different class just for that algorithm, which ends up with a very similar (if not identical) data structure as the AStar class. That means if we need more than one algorithm to traverse a graph, we have to create the same graph twice. See also godotengine/godot-proposals#3848 (and think about the many ways of traversing a graph and returning information)

[halgriffiths]

My fear with the idea of requiring an implementing a base graph type before we merge this ticket, is that then this ticket will never be finished.

There has been an outstanding proposal for a graph structure for 4 years now (#15647) and it has never really gone anywhere. The problem is that there are so many ways to implement a graph (and so many case-by-case optimisation opportunities), nobody seems able to decide which is the "proper" way to do it, hence nothing gets done.

I would rather get Dijkstra into the codebase now (since if Astar is fine then this is probably fine too), and then discussions about generalised graph types can happen later without delaying things.

[Zireael07]

That's true, too.

This PR could go in and then someone will probably be motivated to unite code, as by then we'll likely also have the jps A* variant in.

[Nosliwnayr]

If I remember correctly from my college class where we talked about pathfinding, Dijkstra's is equivalent to A* without a heuristic.
In that case you might be able to re-use the existing pathfinding code instead of copy pasting it into two places?

[vpellen]

This has been turning over in my head for a while lately. I feel like when you're working with Djikstra Maps, the pathfinding aspect is almost secondary.

This is an idle thought: What about a function within the A* graph (and grid?) classes that generates a pollable Djikstra Map?

Here's my thinking on implementation: you call a function on the A* class, specifying a starting point (or multiple, actually) and a maximum search depth/distance. It spits out a class called a DjikstraMap<2/3>D that's little more than a wrapper on a hash map of ID-to-location/cost pairs. You can poll that map and get a path from any point to the origin (or the nearest origin, if you generated the map from multiple), or get a dictionary of every point on the map with its respective distances to the origin.

It solves the problem of cache updating by moving it into the hands of the user, and writing methods for the existing classes would likely produce a far smaller PR with less redundancy.