Update for Godot 4?
Opened this issue · 17 comments
Hi! This looks like an amazing addon, any chance you'd be willing to update it to Godot 4? I tried downloading and installing it as-is to see if it'd work anyways, but it just threw a bunch of errors once it was imported in. I'm not familiar enough with it to see what's really going wrong besides some smaller errors in the visualization demo code.
I believe the rust bindings for Godot 4 are not quite ready yet (at least last time I checked): see https://github.com/godot-rust/gdext. I think @astrale-sharp is already working on it, but we are waiting for things to stabilize on their end.
That is to say, we only support Godot 3.5 at the moment 😅
Oh right, I forgot the whole extension system was changed! Thanks for the reply. I appreciate the effort y'all are putting into this!
Hey, just checking in to see if there have been any updates on this front? I'm working on a project that uses this heavily for a custom movement system; we would love to migrate to Godot 4 now that it has mostly stabilized, but afaik there isn't a way to get this working with 4 yet, and there aren't any other performant implementations that we're aware of.
It's a great addon and I hope we can continue to use it moving forward!
It could probably be ported to gdext (godot-rust) by now but it would likely need to be updated very regularly since gdext still has an unstable API, I personally don't have that kind of time right now but we gladly accept pull requests!
Please note that web export still doesn't play well with gdext and it should still be considered in development for a while!
That being said, it shouldn't be too hard to port it and update it, If I get some free time (unlikely) I could take care of it but I could also provide reviews/guidance if someone wants to tackle that!
https://github.com/astrale-sharp/Dijkstra_map_for_Godot-1/tree/gdext-port
Most of the porting work is done, but gdext don't support optional arguments in methods yet, which our current api is dependent on.
I feel like we should wait for this to land so we keep our nice to use API.
Wow, great work getting this mostly ported already!
It's a shame that optional params aren't supported yet. Having no experience working with Rust, I had no idea what a rabbit hole that whole topic is; it sounds like there isn't a consensus yet among the gdext devs on how to implement them.
From my perspective, I would have no problem using a "beta" version of the Dijkstramap port for Godot 4 if it means having to provide all arguments each time, as long as I know what the defaults should be. Then when a decision is made on gdext and Dijkstramap is able to be fully ported, they could be removed again. But I am making a big assumption here that the API you are referring to is just the public one; I understand it could be much more complex than that with optional args used internally too.
I'm not sure if I can be much help at this point without Rust knowledge, but at some point I would like to learn it and be able to contribute back to this project!
Dear royalty (just noticed the crown), implementation of default parameters on the gdext side are not a big challenge, we/they just need to think of the design so it's coherent and good long term.
I don't know if I would release a beta to the asset store (I could be convinced), but I would absolutely give clear instructions on the readme and support to those who ask for how to build/use the plugin themselves.
The default parameters only relates to the public API/ Godot side of things
I always welcome PR, although I feel for something large like porting it's better if I just do it myself, this project is also mostly over, other projects I manage or contribute to have "Good First Issues" that are a good place to go look for guidance in learning to contribute, the Rust community is globally very welcoming in my experience ;)
If the port is ready to be used with Godot 4 with some manual setup, then I would greatly appreciate some updated installation instructions in the readme! And it makes sense that an Asset Store release would wait until the API has stabilized again.
@astrale-sharp Hi there, just checking in again to see if there is any update on the WIP Godot 4 implementation/setup documentation? I would love to try building & running it locally if there are some updated instructions I could follow!
The whole process is dependent on the godot version, so godot4 should be in your path when you build and you have to rebuild when changing godot version.
You need to have clang installed on your system.
Hey there again :)
- 1st you should follow the general gdext setup https://godot-rust.github.io/book/intro/setup.html
- Then you should clone my fork here https://github.com/astrale-sharp/Dijkstra_map_for_Godot-1
- run
cargo build(takes a long time) - comment/uncomment not relevant lines from
dijkstra.gdextension - launch the project and make sure everything is working. You should run into problems because optionals are used but not supported ;) and maybe gdscript2.0 compat problem stuff as well
- Copy the dijkstra.gdextension and the target folder where you want and you may use (temporary hack, you should just copy the binaries and change the paths)
I seem to run into a crash and i'm not sure why
Thank you for the instructions - I have gotten around to building the project now, and it looks like I'm almost able to build it fully, but I'm running into a type cast error during the cargo build step.
error[E0605]: non-primitive cast: `ConvertError` as `i32`
--> dijkstra-map-gd\src\lib.rs:879:41
|
879 | .map(|ival| PointId(ival as i32))
| ^^^^^^^^^^^ an `as` expression can only be used to convert between primitive types or to coerce to a specific trait object
For more information about this error, try `rustc --explain E0605`.
error: could not compile `dijkstra_map_gd` (lib) due to previous error
Is this something that requires a configuration on my end to circumvent, or could it be showing up now due to an updated dependency? I'm running this on Windows 10 btw, but I can also test with Ubuntu at a later time.
That's a very strange error 🤔 I'll have to investigate...
Later: I forgot to mention you should use the gdext-port branch! this is probably the source of the error!
I'm way more reactive on discord if you're tired of waiting 2 days or more between my answers !
@ astrale_sharp
What is the status of porting to Godot 4?
It looks like there have been no updates for a year and no roadmap either.
Have you abandoned the project?
Btw thanks for your work
The good news is that DijkstraMap works for Godot 4 😄 but it needs a bit of cleanup for a proper release.
Earlier this year, I helped @astrale-sharp test out an updated version for Godot 4, and I also updated the project's samples (+ added a new demo) to work with it on a test branch. It seems to be fully functional aside from a couple relatively minor breaking changes that either need to be fixed, or announced in a new release & updated API:
- "Optional" parameters are currently not optional, you need to pass your own values in each time.
- The default terrain type (-1) is incorrectly weighted as INF and cannot be overwritten, so you need to use 0 or another terrain ID manually.
- The integer range of allowable point IDs is smaller than it was before, I think this might be due to a previously unsigned int being treated as a signed int somewhere in the rust code.
I think most people shouldn't have any problem working around these issues, in fact I've been using it in my own Godot 4.2 project just fine after migrating it all from Godot 3. Though I will say that the performance is a bit worse than Godot 4's built in AStar nodes, so you might want to stick with those unless you think you'll benefit from this library's extra features, or until someone can get around to optimizing the new DijkstraMap code.
Astrale's code is here, but you will probably want to go here to get my branch that includes the updated project samples and the Windows .dll included in the addons dir. If you want to run this on Linux or another system, you'll need to compile it with cargo build following the steps mentioned earlier in this thread, put it in addons/dijkstra_map/dijkstra_map_library/bin/<system>/libdijkstra_map_gd.<extension>, and double check that addons/dijkstra_map/DijkstraMap.gdextension is pointing to it.
Before the updated code can be released, the breaking changes I mentioned need to be addressed, and we need to either fix or replace the automated documentation, tests, & release pipelines that are currently broken. Unfortunately I have no experience with Rust, but I would be glad to help test updates or modify the GDScript/C#/etc. parts of the codebase further! Hopefully we can get this updated code merged in soon so we can at least have a beta version available in this repo.
