-
-
Notifications
You must be signed in to change notification settings - Fork 19.8k
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
Add a partial path return option for AStar #88047
Add a partial path return option for AStar #88047
Conversation
When running the CI checks in my fork's branch, it failed the GDExtension compatibility
I am not sure how to fix this, I thought GDExtension would use the modified ClassDB bindings, but I don't really know how GDExtension works. Advice on how to resolve that would be greatly appreciated! |
See #80410 for example. You need to add |
Premature approval for the feature as this has been requested many times. Can't say much about the technical detail or style as the AStar classes are not really my habitat. I would even go as far and do a small compatibility break here on this and make it the default to always return the partial path if the target can not be reached. That is what the majority of (new) users expect and is also how the NavigationServer path queries all behave. That the path returned empty when the target could not be reached always irritated and annoyed people. |
Many thanks to everyone for the suggestions regarding naming/style! I've made those changes.
Thank you! I think I've done this correctly. The validation script no longer returned any errors related to these classes when I ran it locally at least.
If others agree I can set the default to true! I just defaulted to the most compatible option to start with. |
8d530a7
to
219286a
Compare
"Small compatibility break" is an understatement. This will break success checks for all existing Checking for emptiness was the recommended approach in Godot 4.0, 4.1 and 4.2. I don't think changing this to accomodate for new users is a good idea. Besides, we don't even know how many people would be irritated/annoyed if a simple emptiness check would now become a "either empty or last path is destination" check. |
219286a
to
60a2578
Compare
c7f3563
to
2bc15d7
Compare
0bbba5f
to
583ba15
Compare
Just because I'm a little unclear on Godot dev cycles, is 4.3 "feature frozen" and this wouldn't be merged until 4.4 begins (and thus 4.3 is released)? Mainly checking to know if I should keep clearing the conflicts with the 4.2-stable.expected file every few days to keep it ready to merge, or if the expectation that it's gonna wait until 4.4 then I can leave it for a while. |
There's still some time for 4.3 features, we're not in feature freeze yet. The feature freeze will start with 4.3 beta 1, probably in a month. But you don't need to rebase frequently to solve the conflicts on that compat file, first we need some maintainers familiar with that feature to approve the changes. Then you can do a final rebase to make it merge ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the navigation feature. Users ask regularly about a partial path option with the astar classes to be added. Tested and at least from my view it works as expected.
Not sure about code or compatibility detail. Others need to check that part.
583ba15
to
51e529e
Compare
* AStar2D, AStar3D and AStarGrid2D now can return a partial path if the destination point isn't reachable but still in the map. This option is available for both get_point_path and get_id_path
51e529e
to
aa1bbe1
Compare
Thanks! And congrats for your first merged Godot contribution 🎉 |
This change should address and close godotengine/godot-proposals#277 by adding an optional parameter to
get_point_path
andget_id_path
forAStar2D
,AStar3D
, andAStarGrid2D
that when true will return the partial path instead of an empty path.By having this be an optional parameter this should not have any compatibility breakages, and for those who rely on having an empty path returned if no complete path is available will not have their code affected. Those who want/need partial paths however can simply pass true in to get them.
Note: This will not return a partial path to a location not in the AStar point list, the destination point still has to be a point. This is especially helpful for games where points may become disconnected due to other points being disabled during play while still permitting a path that gets close to the destination point.
I also went ahead and updated the xml docs for the affected classes to mention this new possibility. If I understand correctly the Godot docs page uses these to auto-generated so a doc update for that shouldn't be necessarily.
The original implementation was modified from KnightNine's branch located here, however there were a bunch of other changes for other features in that branch and I wanted to only address the original proposal issue. They used a separate method as suggested in the proposal issue, but I felt that an optional parameter made more sense and to prevent having extra data structures to store the partial paths.
Example
Download: astar_partial_test.zip
I used the navigation2d demo project to test
AStarGrid2D
. I testedAStar3D
using a personal project and I cannot share the code of that, but it behaved as expected. I have included the modified demo project for ease of testing. I did not have time to modify this demo project to explicitly showcase it working with AStar2/3D.This is the test using the attached demo project. The E tile was the destination tile. This tile is a part of the AStarGrid2D's points, but is surrounded by impassible/disabled points isolating it. The path gets as close as it can.
This example shows the
AStar3D
test. As movement costs are not 1 for all, I added a few annotations. The infinity over the black tile with brown objects on it is a tile that is not in the map. The tiles with a 2 on them have a cost of 2. The blue tile with the red line was the tile selected. This tile is in the AStar point list, but is not connected to any points. The white arrows follow the path generated by AStar. As you can see it gets close to the target tile. as expected.3.x
I am uncertain on compatibility for 3.x as I do not use it nor did I test this. My brief cursory glance shows multiple commits to the AStar classes since 3.x, so it is possible due to this it cannot be cherry-picked, but I really admit I am uncertain on how that process works.