-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Change navigation map synchronization to an async process #100497
Conversation
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.
Huge one ! Congratulations !
1c52ba7
to
652c5ad
Compare
Pushed the changes. Also added the missing NavigationServer2D API functions. As mentioned in the comments the edge connection margin loop and cell check should get their own PR as that is just old legacy code that I only copied over. |
8d96aa3
to
2ecd489
Compare
2ecd489
to
8e03ce0
Compare
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.
LGTM
8e03ce0
to
59d80d6
Compare
Changes the navigation map synchronization to an async process to avoid stalling the main thread.
59d80d6
to
d51615b
Compare
Thanks! |
Will tracking a dynamic target cause it to be lost or out of sync? In this PR. |
What this PR changed is how the navmesh graph is build and synchronized. Your target tracking happens outside the navigation system and as such is unaffected. |
Changes the navigation map synchronization to an async process to avoid stalling the main thread.
Fixes #96820 with new iteration slots that avoid stalling query threads.
Resolves #96483
Bugsquad edit: Fixes #100716
Performance
Pretty good.
To be clear this PR does largely not improve the performance of the underlying code of the actual map sync (something for future PRs). In fact the navigation map sync might be even slightly slower on simple maps or single-threaded builds. because the sync needs to do a few more new things.
What this PR primary does is that it pushes the majority of all the time consuming sync work to a background thread unblocking the main thread. So any project that can use threads will see a giant performance boost on larger navigation maps.
Since the sync happenes in the middle of the physics process step this also avoids final performance death from running into a physics process death spiral.
What this PR also does is solve a majority of all the thread locking cases as the map no longer write locks in full until the sync is finished. Threads can now query the navigation map more or less unimpeded by locks.
How does it work?
This PR explained in one picture:
Each navigation map now has 2 iterations, aka the map navmesh graph and everything that needs to be out of harm's way from other navobjects getting changed or deleted while threads use said graph.
One iteration is active and all new queries route to it. The Second iteration waits until the navigation map changes and builds on a background thread a new map graph. When finished the two iterations swap places by changing the current iteration slot index routing new queries to the newer iteration. This way long running pathfinding threads are able to finish without causing long thread locks. As soon as all old threads have stopped using the now free iteration the cycle continues.
This way
Now what async in general means is that there is always at least some delay between the map change and until the map update is actually registered due to the thread sync. This was already the case due to the physics process sync but now the delay will vary depending on what is going on on the map.
The delay extends when the map is more complex and has more things to process. The delay also extends if long running path query threads block the old iteration for a while until the iteration is free again. In this case the map sync checks if the old iteration is unused. While it is still used by a thread user instead of write blocking it it skips the sync and checks again on the next sync.
New settings and API
This PR adds the new map property
use_async_iterations
and related ProjectSettings and NavigationServer API to push the sync to an async process run on a background thread. It is enabled by default but if you need the old main thread sync back for your project you can disable it.For new navigation maps this uses the ProjectSettings value from
navigation/world/map_use_async_iterations
.The async sync can also be toggled for each map with the
NavigationServer.map_set_use_async_iterations(map, enabled)
function.Spaghetti Notes
The old owner pointer spaghetti used with the NavRegions and NavLinks was async unworkable as they could turn invalid while threads were still using them. This is why the new structs
NavRegionIteration
andNavLinkIteration
were added.Those are full snapshots of the regions and links used by the navigation map that can not be touched by user changes anymore as long as the map iteration needs them. This is also why the iteration update functions do full copies with them so nothing uses or points back at the NavRegion or NavLink except a single HashMap that has a pointer to id mapping. Previously all those things just pointed to the owner to avoid data duplicates but the owner could get changed or deleted in the middle of threaded queries.