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

Add NavigationPathQuery objects and NavigationServer query_path() #62429

Merged
merged 1 commit into from
Sep 21, 2022

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Jun 26, 2022

Adds NavigationPathQueryParameters and NavigationPathQueryResult objects in 2D and 3D similar to e.g. physics query parameters and results.

Can be used with 2D or 3D NavigationServer query_path() to get a customized navigation path considering additional options.

Does not affect the normal pathfinding with NavigationAgent nodes or NavigationServer.map_get_path() at all. Without customized options returns the same path as map_get_path() for now.

Removed for this pr most other options that are still developed and not finished yet (like #2527 or #3812). If there is only a single function or an enum has only a single value that is why. This is basically only the core left as many other pr's and features will depend on this. This query can be used as a base to fix issues #19011 and #60277 in the future by adding a property that controls how the pathfinding adds points or a simplifying method in path postprocessing.

This pr is also a prerequisite for #62115 or any other issue that is related to the currently used pathfinding algorithm and forced path post-processing. Since Godot seems that it wants to support both 2D and 3D pathfinding as free-form as well as tile / grid based with TileMap and GridMap (#61293) it is impossible to have just one pathfinding algorithm and one out of two forced path post-processing for everything like it is rightnow.

Usage is to create both objects first and store them into a var for reuse.
Then use query_path() for updating the content of the result.

Example:

var query_parameters : NavigationPathQueryParameters3D = NavigationPathQueryParameters3D.new()
var query_result  : NavigationPathQueryResult3D = NavigationPathQueryResult3D.new()

# fill in parameters...

NavigationServer3D.query_path(query_parameters, query_result)
var path = query_result.get_path()

@smix8 smix8 requested review from a team as code owners June 26, 2022 14:22
@smix8 smix8 force-pushed the navigation_path_query_4.x branch 7 times, most recently from 0e7eaca to 4682c8e Compare June 26, 2022 15:27
@Calinou Calinou added bug enhancement cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:navigation labels Jun 26, 2022
@Calinou Calinou added this to the 4.0 milestone Jun 26, 2022
@smix8 smix8 force-pushed the navigation_path_query_4.x branch from 4682c8e to 4176445 Compare July 6, 2022 13:46
@akien-mga
Copy link
Member

CC @AndreaCatania @Scony @lawnjelly

@lawnjelly
Copy link
Member

Have written some comments. But really before adding this PR I think the navigation team should decide whether they are going to add async, and if so add this first because the API and implementation of all this stuff may need to be different if supporting async pathfinding.

Copy link
Contributor

@AndreaCatania AndreaCatania left a comment

Choose a reason for hiding this comment

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

It looks good, just few nits!

modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.h Outdated Show resolved Hide resolved
doc/classes/NavigationPathQueryParameters2D.xml Outdated Show resolved Hide resolved
doc/classes/NavigationPathQueryParameters3D.xml Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
servers/navigation_server_3d.cpp Outdated Show resolved Hide resolved
servers/navigation_server_3d.cpp Outdated Show resolved Hide resolved
@smix8 smix8 force-pushed the navigation_path_query_4.x branch 2 times, most recently from fde2528 to 0d5c1ec Compare August 5, 2022 22:59
@smix8 smix8 force-pushed the navigation_path_query_4.x branch 2 times, most recently from c0effbc to d075df1 Compare September 13, 2022 20:41
@smix8
Copy link
Contributor Author

smix8 commented Sep 13, 2022

Removed the callback and processqueue from the pr, basically only the navquery core left now.

@DarkKilauea
Copy link
Contributor

I believe we should move the query parameter and result objects next to the navigation server itself, instead of putting them into the module.

  1. I want to be able to use them inside NavigationAgent without having to disable functionality is the module is disabled. This is particularly needed to make working with navigation links easier and more intuitive. Right now, there isn't a way to trigger when an agent enters a link and I'd like that capability to always be there.
  2. We should define a minimum set of query features that is required for other navigation nodes to work. It's ok if future server implementations add features, but they should all implement the base set. Moving the types next to the server would allow other implementations to derive from our base parameters and result objects and then add their new features, with a cast used on the consumption side to gain access.

@smix8
Copy link
Contributor Author

smix8 commented Sep 14, 2022

I agree but that module issue is not simply only for the navquery.

The current situation is that nearly every navigation pr that adds something new requires a lot of ifdef guards to even pass the recently added git check for Godot minimum build which is very annoying because the navigation module is in no state to be disabled as shown by issues like #36091 or the recent discussions on rocketchat (with no real solution) how to even properly disable such an intertwined module like navigation. Just look at this filechange mess currently required for nonavigation builds to not break in Godot https://github.com/godotengine/godot/pull/65562/files.

If the navigation module folder is intended to be only for the godot_navigation_server backend implementation that basically means we a) need a dummy server for navigation similar to physics and b) the entire stuff that has general use like the navigationmeshgenerator or the navquery stuff needs to be moved from the module navigation to a server navigation folder.

@smix8 smix8 force-pushed the navigation_path_query_4.x branch 2 times, most recently from eb9132c to 489922a Compare September 15, 2022 08:17
@smix8
Copy link
Contributor Author

smix8 commented Sep 15, 2022

Changed the folder structure and moved the parameters / results from the godot navigation module folder to "servers/navigation" as they are considered a part of the NavigationServer that needs to be available for other classes or server implementations that replace the godot server module.

@DarkKilauea
Copy link
Contributor

I did some bench-marking to try to understand the performance difference between this PR and #65680.

Code:

var nav_map_rid := get_world_2d().navigation_map;
var start_location := NavigationServer2D.map_get_closest_point(nav_map_rid, start.global_position);
var target_location := NavigationServer2D.map_get_closest_point(nav_map_rid, end.global_position);

var result := NavigationPathQueryResult2D.new();
var query := NavigationPathQueryParameters2D.new();
query.map = nav_map_rid;
query.start_position = start_location;
query.target_position = target_location;

for i in 10:
	var average_buffer := PackedInt32Array();
	
	var current_run_time = run_time_seconds;
	while(current_run_time >= 0.0):
		var cur_time := Time.get_ticks_usec();
		
		NavigationServer2D.query_path(query, result);
		
		var end_time := Time.get_ticks_usec();
		var duration := end_time - cur_time;
		var duration_in_sec := duration * 0.000001;
		
		current_run_time -= duration_in_sec;
		average_buffer.push_back(duration);
	
	var average_duration := 0;
	for dur in average_buffer:
		average_duration += dur;
	
	average_duration /= average_buffer.size();
	print("Average query time: %s usec" % average_duration);

Build Info:

> scons target=release_debug
scons: Reading SConscript files ...
Automatically detected platform: windows
Auto-detected 16 CPU cores available for build parallelism. Using 15 cores by default. You can override it with the -j argument.
Found MSVC version 14.3, arch x86_64
Building for platform "windows", architecture "x86_64", editor, target "release_debug".

Results:

Godot Engine v4.0.alpha.custom_build.489922a0e - https://godotengine.org
Vulkan API 1.2.0 - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce RTX 2080

Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec
Average query time: 20 usec

See #65680 for details about the testing done there, but the short version is that this PR is about 13.04% (3 usec) faster.

servers/navigation_server_3d.h Outdated Show resolved Hide resolved
modules/navigation/nav_utils.h Outdated Show resolved Hide resolved
modules/navigation/godot_navigation_server.cpp Outdated Show resolved Hide resolved
modules/navigation/navigation_path_query_result_2d.cpp Outdated Show resolved Hide resolved
servers/navigation/navigation_path_query_parameters_2d.h Outdated Show resolved Hide resolved
servers/navigation_server_3d.h Outdated Show resolved Hide resolved
modules/navigation/nav_utils.h Outdated Show resolved Hide resolved
@DarkKilauea
Copy link
Contributor

Sorry for the sudden comment spam, didn't notice that GitHub does not post your comments until you "finish your review". After playing with this PR for a while, I'm happy with its overall shape. I left a few comments for improvement, but these can be completed later.

@smix8
Copy link
Contributor Author

smix8 commented Sep 20, 2022

Removed the path len calculation and the "optimize" bool parameter that switched between the two available postprocessing options in map_get_path(). Added path postprocessing enum as a replacement for the outdated "optimize" parameter.

Adds NavigationPathQueryParameters objects that can be used with NavigationServer.query_path() to query a customized navigation path.
@DarkKilauea
Copy link
Contributor

I'm happy with this PR. It should be good to merge.

@akien-mga akien-mga merged commit 9521849 into godotengine:master Sep 21, 2022
@akien-mga
Copy link
Member

Thanks!

@smix8 smix8 deleted the navigation_path_query_4.x branch September 29, 2022 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:navigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants