Skip to content
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

Despaghettify NavigationServer path queries #100129

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Dec 6, 2024

Slaying the spaghetti monster

spaghetti
This PR tries to slay a monster that has haunted us for months and years, blocking a not insignificant amount of other navigation PRs.

In the land of Nav for the longest time a map path function housed a spaghetti monster. It was growing year by year, step by step, each adventuring contributor adding just a little remembrance. Until the day the monster has grown over everyones head into a frankenstein. Barely anyone dared to touch it anymore. Eventually they all did run away in terror watching the horror they had created in their foolery. Things changed little for years until the day the monster slayer arrived ...

This monster made both the path query and map get_path() function so hard to work with and iterate. It was basically not rebase-able without adding 101 new bugs and merge conflicts. Git went full hallucination all the time alone and could never handle changes on those mega functions. It was an absolute wrecking ball for keeping branches in sync or updated.

So this PR splits the monolithic map_get_path() query function into smaller static sub-functions. Each function dedicated to its own little task in the path query. This gives a better base that contributors actually can work on or performance test.

Due to the split and other related changes it is now also possible to add new parameters for pathfinding in a reasonable way, e.g. new pathfinding algorithms or post processing options. One example is included with the No-postprocessing.

Compatibility

The current NavigationServer.map_get_path() function is kept for compatibility. Since it now needs to create new objects all the time to forward to the new functions it is better for performance to switch to using NavigationServer.query_path().

In general there are not noticeable compatibility breaks. Some of the functions need to have their const removed because they now actually need to mod some of the parameter and results objects. The new callback parameters are optional. Apart from that everything stays mostly the same as all the major changes are internal.

Manicas that need to run more than 4 threads on pathfinding at the same time on the same map need to go to the ProjectSettings and adjust a new setting. See PathQuerySlots description below.

Query callbacks

Adds the optional query callbacks to the query functions already.

I considered them to be added by the async queries PR that will follow that absolutely needs them but since we are already breaking comp on those functions in this PR might as well add them right away as they are actually useful for users.

Option to disable the path postprocessing

With the new found options for parameters this PR adds finally the option to disable the entire path postprocessing for better debug paths. This means after the postprocessing for corridorfunnel and edgecentered there is now also the debug option to have no postprocessing.

none_postprocessing

This way you can now actually see the raw path corridor that the pathfinding takes. This is valuable information as it can visually explain why the pathfinding decided to pick certain directions that may not be that obvious or look reasonable when hidden behind postprocessing.

The new PATH_POSTPROCESSING_NONE is available on both the NavigationAgent2D/3D nodes as well as the NavigationPathQueryParameters2D/3D.

Note that it would have made far more sense to have the NONE option at enum 0 instead of 2 but that would break comp.

PathQuerySlots

This implements the final piece of reduz old NavigationServer optimization tech draft https://files.godot.foundation/s/e49dZLC6JBeo7NK as everything else regarding the NavigationServer has been added by other PRs before already.

As part of improving performance and reducing allocs added PathQuerySlots to be used by each navigation map. The slots hold the query related temp map data so the data can be reused between queries. When a query comes in it picks a free slot to do the path query. If more queries come in than free slots are available, e.g. more threads doing queries all at the same time, they will have to wait at a semaphore until one thread frees a slot. These slots will become far more important with the map iteration PR in the future as at that point threads absolutely need their own data copy.

The default slot count per map is 4 so that up to 4 threads can run queries in full parallel on the same map. This amount is customizeable in the ProjectSettings with navigation/pathfinding/max_threads and defaults to 1 slot on OS without thread support. Adding more slots costs more memory and very slightly increases the sync time of the navigation map.

Internal Refactoring

Adds a new internal NavMeshPathQueryTask3D struct that combines all the data used in a query. This is done so things can be easily passed around to subfunctions for path building. Also to collect all the path related data before returned by the updated query result.

This was actually a major problem with the old query function that used a bool for the post-processing and had no option for pathfinder parameters. Removed the two old internal structs that no longer served any purpose with this change.

Moved the internal simplify path code to the navmesh queries class as it is used as part of the path post-processing and can make use of internal LocalVector, not needing to go through the entire API with COW vectors.

Removes COW Vectors or other unnecessary memory allocs across the internal board where possible, e.g. replaces them with LocalVectors.

Note that because I had to slice all this from far bigger PRs some parts look like kinda out of place. Some parts I pushed to other PRs. E.g. the new sub functions still have a lot of parameters that could have be part of the task struct. At that point I was just exhausted and did not want to touch that code further in this PR risking even bigger bug surface. That is for another day ...

@smix8 smix8 added this to the 4.4 milestone Dec 6, 2024
@smix8 smix8 requested review from a team as code owners December 6, 2024 23:51
@smix8 smix8 force-pushed the pathunspaghettification branch 4 times, most recently from 14c15c8 to 03bd32a Compare December 7, 2024 12:21
@smix8 smix8 force-pushed the pathunspaghettification branch 7 times, most recently from f979e88 to f9951a9 Compare December 10, 2024 23:11
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean to me, and looks like everything is retained properly from my own work splitting these things (which I'll have to update following this!)

Copy link
Contributor

@Scony Scony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did just a brief check - well done!

Despaghettify NavigationServer path queries.
@smix8 smix8 force-pushed the pathunspaghettification branch from f9951a9 to 4764794 Compare December 11, 2024 21:29
@Repiteo Repiteo merged commit 153ef23 into godotengine:master Dec 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 11, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants