NavigationAgents - schedule pathfinds to prevent performance blips

[lawnjelly]

Adds a simple method to limit pathfinds per physics tick.

Pathfinds in the navigation server are currently synchronous, and can cause performance blips and dropped frames if several agents do a pathfind simultaneously. This PR enables simply limiting the number of pathfinds per physics tick.

Notes

• Adds a project setting navigation/common/pathfinding/paths_per_tick. This can either be positive, 0 (unlimited), or negative, indicating a single pathfind every nth tick.
• Overall it is usually better (especially with large navmeshes) to distribute single pathfinds over several frames. But this would require major changes to the current design, so this PR is a reasonable compromise to get started managing performance.
• Duplicates some code in 2D and 3D versions. Could be moved out into a separate class if desired, am initially following the preference for local solutions (even if duplication occurs). See https://docs.godotengine.org/en/stable/community/contributing/best_practices_for_engine_contributors.html#prefer-local-solutions
• Also relevant to 4.x.

[akien-mga]

CC @smix8 @Scony @AndreaCatania

[Scony]

I like the overall idea, but I don't like the implementation:

• project setting navigation/common/pathfinding/paths_per_tick aggregates 2 features - one for positive values with zero and another one for negative values. IMO that should be split into 2
• code is duplicated across 2D and 3D
• the setting is static - cannot be manipulated at runtime

IMO, the much better way would be to implement the solution within the navigation server (something like async map_get_path). Moving the setting there as well would make it possible to manipulate it in the runtime. Moreover, having the entire knowledge of path calculation requests (FIFO queue) in one place would make it very easy to introduce more sophisticated algorithms on how to distribute requests among frames. Not to mention such a solution could be exposed to the user. There are plenty of use cases in games where AI must recalculate the path from time to time while it's not time-critical. It would be nice if there was an API that allows user to request path calculation "somewhere in the future when the load is not that big".

[lawnjelly]

• project setting navigation/common/pathfinding/paths_per_tick aggregates 2 features - one for positive values with zero and another one for negative values. IMO that should be split into 2
• code is duplicated across 2D and 3D
• the setting is static - cannot be manipulated at runtime

All these are easy to change.

IMO, the much better way would be to implement the solution within the navigation server (something like async map_get_path).

Yes, as I said here in the notes:

Overall it is usually better (especially with large navmeshes) to distribute single pathfinds over several frames. But this would require major changes to the current design, so this PR is a reasonable compromise to get started managing performance.

Writing async version will probably require some major rework from the navigation maintainers to get working without causing stalls (if anyone wants to volunteer for this? 😁 ). In the meantime, although 4.x is in beta, 3.x is expected to be used in production, and imo it is better to have a stop gap solution that works than nothing at all, so people can continue to ship games.

Essentially, 3.5 stable is right around the corner so if possible it would be good to clear up the major problems with the navigation asap (robustness and performance), and have a usable solution - bearing in mind we don't have time for another long series of betas for async pathfinding.

[Scony]

Writing async version will probably require some major rework from the navigation maintainers

IMO it will not. I tend to think it would be actually much easier and more readable. Also, what's the point of adding a feature we would like to get rid of in the next increment? Once users start using above implementation it will be much harder to push for the proper one.

[lawnjelly]

IMO it will not.

Then maybe you can add it? 👍

Once users start using above implementation it will be much harder to push for the proper one.

In terms of project setting we could probably just have a toggle for deferred_pathfinding, and reuse this for async at a later date. In terms of feature implementation, async pathfinding is pretty much standard for AI, I don't think anyone will be holding back the development.

[Scony]

Actually, I'm busy rewriting the entire navigation system using Recast/Detour/Crowd as a plugin/module :)

Nevertheless, having a feature switch deferred_pathfinding - off by default - would make this implementation acceptable IMO. Whoever would need it would use it and yet It would be much less harmful to the average user if we replace it later.

I'd just call to split navigation/common/pathfinding/paths_per_tick and duplicate the code in that case.

Ofc. Feel free to disagree :) Maybe @smix8 can give some thoughts as well.

[smix8]

I think the idea of this pr to slice the updates over multiple frames is good but I think we should implement this differently.

Navigation timeslice as a feature is needed, no doubt. The current NavigationAgent navigation leads to frame spikes that could be easily avoided if not all NavigationAgents node would call for updates in the same frame. I think that change is not time critical for Godot 3.5. In my tests performance is not really worse than old navigation in <3.5 before the NavigationServer backports. We can give us some time to figure things out properly for something that works in both Godot 4.0 and 3.5.1 or 3.6.

I think this should be implemented in Godot 4.0 first and later backported. With the many recent updates and backports for Godot 3.5 we have near identical navigation code in 3.5 and 4.0 now. This was a struggle to achieve. Every addition, if not very specific for Godot 3.5 codebase, should be made for Godot 4.0 first and then backported to Godot 3.5, if it works.

I think navigation features like timeslicing path queries here should not be made NavigationAgent node exclusive. Navigation features should work with pure NavigationServer API use first and then be added to any node that uses said API functions cause not everyone wants to and should use the NavigationAgent node as it is very restricting. This means this feature needs to be added on the NavigationServer and not on the NavigationAgent node, e.g. with a queue on the NavigationServer like Scony suggested.

Async/Deferred/WhateverMode path queries should never be a bool toggle for the entire project. Some actors are just more important than others and some situations need instant updates while others are more performance aware. So this should be a parameter on the actual path query. Could still add a funnel on the node like in this pr but the time slice should be in the hands of the NavigationServer and not some random node. My prefered way to add a "deferred" parameter for path queries would be with the NavigationPathQueryParameters3D objects from pr #62429. Could easily add a property like NavigationPathQueryParameters3D.path_callback = "method_name" to it. When a callback is available the NavigationServer will process the path query not immediately but only when it has time. I would prefer shifting to such Parameter objects like physics does as it makes it so much easier to add / rework features in the backend without breaking compatibility and add class documentation.

I think settings like those discussed should be part of the NavigationServer and changeable at any time from scripts not a "hardcoded" projectsetting that is only updated at the start. In general I prefer to move navigation related properties/settings to the NavigationServer for those that depend on a Godot navigation build anyway. My pr #62601 for navigation debug already tried to clean the SceneTree from as much navigation stuff as possible and move it to the NavigationServer so everything navigation related is in one place.

@Scony If you manage to integrate Detour/Crowd in Godot with the current NavigationServer that would be definitly a feature for a core module in my opinion as it would solve some annoying limitations with the current navigation system. Let me know if you make it available for testing e.g. with a branch.

[lawnjelly]

Closing as stale.