Dijkstra_map_for_Godot icon indicating copy to clipboard operation
Dijkstra_map_for_Godot copied to clipboard

Is this updated?

Open MoogieOuttaMyDepth opened this issue 1 year ago • 1 comments

I saw there was some discussion about making this work for Godot 4, but I can't find clear information about whether or not it does yet. I'm using 4.2 and it doesn't appear to be available in the Asset store.

Also, I'm not using Mono, does that mean I can't use this?

I really needed an Astar alternative that can weight connections between points. This is all I found, so it'll be a bummer if I can't use it. :/

MoogieOuttaMyDepth avatar Aug 22 '24 18:08 MoogieOuttaMyDepth

Hey there :) Gdext doesn't support optionnal parameter in gdscript yet so we're reluctant so officialy update and push to the asset store just yet.

I'm pretty sure there is a working C# version here: https://github.com/astrale-sharp/Dijkstra_map_for_Godot-1/pull/1

You're going to need to compile it yourself, do you have a rust toolchain installed? Do tell if you run into any issues

astrale-sharp avatar Sep 05 '24 15:09 astrale-sharp

Hi, I'm looking at trying to compile and use this for Godot 4.3. While the branch you linked is supposed to work on godot 4.0 the instructions for compiling it aren't present. I'm not experienced with rust, and so it's difficult to figure out where the process should start. The instructions in that branch haven't been updated either, as they still refer to using GDnative, which to my knowledge stopped being updated and will not be receiving any Godot 4+ support. If you have any time could you give an abbreviated version of the compilation process for the project, and the dependencies it relies on?

greenguy77 avatar Dec 29 '24 18:12 greenguy77

@greenguy77 using cargo build --release, do you run into compilation errors? then i would suggest following latest godot-rust/gdext instructions as to how to setup the resulting binary (target/release/?.so) with godot

https://godot-rust.github.io/book/intro/hello-world.html#the-gdextension-file

astrale-sharp avatar Dec 31 '24 12:12 astrale-sharp

I did run into a compilation error. I'm not sure, but I suspect it may be caused by my Godot version.

Compiling godot-ffi v0.2.2 (https://github.com/godot-rust/gdext.git?branch=master#6a5d19d5) error: failed to run custom build command forgodot-ffi v0.2.2 (https://github.com/godot-rust/gdext.git?branch=master#6a5d19d5)`

Caused by: process didn't exit successfully: C:\Users\redacted\Documents\Dijkstra_map_for_Godot-1\target\release\build\godot-ffi-5c5a55c39c014e0a\build-script-build (exit code: 101) --- stdout Found GODOT4_BIN with path to executable: 'C:\Users\redacted\Desktop\gameDev\Godot_v4.3-stable_win64.exe' cargo:rerun-if-env-changed=GODOT4_BIN cargo:rerun-if-changed=C:\Users\redacted\Desktop\gameDev\Godot_v4.3-stable_win64.exe [stdout] 4.3.stable.official.77dcf97d8 `

greenguy77 avatar Jan 07 '25 15:01 greenguy77

wow that's an awful error, i'll try to reproduce (i finally have my real computer back) give me a few days

astrale-sharp avatar Jan 16 '25 16:01 astrale-sharp

My computer died again so i can't really help there..

astrale-sharp avatar Jan 31 '25 14:01 astrale-sharp

Continuing the Godot 4 update discussion + the duplicate one #121 here:

Seeing as Godot 4 has been released for almost a couple years at this point and gdext still doesn't support optional params (with no indication that it's coming soon), maybe it's time to consider an alternate implementation so we can get this thing released officially? I don't have much Rust experience and only some GDExtension experience from previous work on this project's 4.0 update, but I'd like to suggest a few possible workarounds to this core issue. As a point of reference, here is one of DijkstraMap's functions w/optional params that demonstrates the problem well:

func connect_points(source: int, target: int, weight: float (opt), bidirectional: bool (opt)) -> int

Calling this function in Godot with any missing optional params (e.g. connect_points(0, 1)) causes a crash.

Potential workarounds

  1. Add Rust function wrappers: The idea here is to keep the Godot-facing API fully intact if possible. Would it be possible & practical to expose the API not via gdext but by other means (C++?), and interface with gdext through this simple layer? I assume there could be significant overhead that would make this approach not worthwhile, and more difficult to maintain.
  2. Default params as dictionary payload: As the next best option, the API could be changed to have optional params defined in a dictionary. godot-rust implements a Dictionary Struct which appears to support optional keys as it should. So the revised function could look something like this: func connect_points(source: int, target: int, options: Dictionary { weight: float (opt), bidirectional: bool (opt)}) -> int then the code would need to extract each optional param as such: options.get("weight", 1.0), ideally with some type checking to prevent crashes. This is exactly what the recalculate function already does with its optional_params parameter.
  3. Convert to builder methods: This is similar to how gdext supports default parameters for built-in Godot functions. They use a builder pattern with chained extender methods, which is a pattern similar to Godot's built-in Tween's properties . A converted version of the connect_points function might look like this: func connect_points(0, 1).set_weight(1.0).set_bidirectional(true).run() but frankly this feels quite clunky and seems less ideal than other workarounds.
  4. Multiple function variations: It would be an ugly solution, but you could have multiple functions that act as wrappers in lieu of method overloading, e.g. connect_points(0, 1), connect_points_weighted(0, 1, 1.0), but this can quickly get out of hand.
  5. Simply require all params: With the API as it is, making all parameters required shouldn't be a significant burden for most developers; there aren't that many optional params anyway, and in my experience many of them are used more often than not. I'm already using DijkstraMap this way in my Godot 4 game after migrating from Godot 3, and the upgrade was fairly easy to pull off.

Ultimately, I really don't think an API change should be a dealbreaker at this stage seeing as this would be a significant version update anyway, but that's just my two cents! I'd be happy to help out however I can, even if it's just updating my PR for the non-Rust portion of the codebase.

skison avatar Feb 08 '25 06:02 skison

My 2 cents is to go with options 4 or 5 because I am assuming those are more performant, which matters in the functions that may get used many times in succession, such as connect_points(). (But I don't know much about Rust, so maybe I'm wrong about the builder methods or dictionary approach being slower.) Something like connect_points() only has 4 params, so it's not that big of a deal to have to specify them all. I'm probably doing it in a loop anyway and only actually typing it a couple times.

goblinJoel avatar Feb 11 '25 01:02 goblinJoel

That's a good point, I agree that performance should take priority wherever it would make a difference! I haven't measured the time of Dictionary lookups in gdext (and looking at the gdext docs it's not immediately clear to me how it's implemented), but I think at a larger scale you would be able to see a significant lag between that and regular typed parameters. I have a WIP PR that includes an additional Godot 4 sample scene astar_maze_comparison that lets you generate an arbitrarily large maze and connect it with both DijkstraMap and Godot's AStar implementation to compare performance - this might be helpful for testing different compiled DijkstraMap versions too.

If all params were required, that wouldn't bother me - imo the only default params in the API that I think are a bit confusing are the terrain ids/types (default is -1), so I guess it could be nice to have a non-terrain version of add_point for users who don't care about that feature, but it's really not a necessity. The other defaults (bidirectional/weights) are more self-explanatory, so it doesn't feel out of place to require them even when you don't need them.

skison avatar Feb 11 '25 03:02 skison