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

Backport NavigationServer to 3.x #48395

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Conversation

Duroxxigar
Copy link
Contributor

@Duroxxigar Duroxxigar commented May 3, 2021

This is the nav server backported as much as I could. There are also a few additional changes that I made that I will be making a PR for in master - such as improved error messaging and fixing a race condition.

Did not implement #47024 because it included some potentially breaking features, such as removing the navigation node and implementing navigation layers. I can backport it - just need the okay to do so. This also means other PRs that fixed things due to this PR, was not implemented either.

Did not implement #48028 because it kept crashing the engine in the 3.x branch. Even when I tried to do the 3.x equivalent. There may be something that I have overlooked about it.

Other than these, I'm fairly certain I got everything. I may have missed a PR or two, but all the major ones were implemented. I have tested the new workflow, as well as the old (using get_simple_path()) and both work. So this means that people who are relying on the old workflow should not notice a change or anything breaking.

I also have a Windows build (x64) people should be able to download and try out. I wasn't able to build for any other platforms, because I lack the tooling to do so. All the documentation should be updated as well.

There is a known issue (which exists on master as well) in one instance that I've found. If you are trying to command an agent to move to a certain location by mouse click (and do the raycast and all that jazz), when it gets to the final location, the agent oscillates as if they can't actually get to the position. This can be scripted around by slightly increasing the target_desired_distance property in the agent. According to Andreas, this kind of jitter is expected. I did not observe this when utilizing the nav server with AI chasing the player or any other thing of the sort.

If the checks fail for formatting - I'll address that when it is pretty much time to merge.

Bugsquad edit: Backport of #34776 and follow-up PRs.

@Duroxxigar Duroxxigar requested review from a team as code owners May 3, 2021 00:24
@aaronfranke aaronfranke added this to the 3.4 milestone May 3, 2021
@aaronfranke aaronfranke changed the base branch from 3.3 to 3.x May 3, 2021 00:36
@aaronfranke aaronfranke force-pushed the nav-backport branch 3 times, most recently from 7ec0450 to 8b11724 Compare May 3, 2021 01:43
@aaronfranke
Copy link
Member

Some more pre-compiled builds for easy testing (all 64-bit x86): Linux Editor, Linux Template, Mac Editor, Mac Template, and folder.

@phelioz
Copy link

phelioz commented May 4, 2021

Really nice work with porting this to 3.x :)

Tried quickly to use my 3.3 2D project with the new navigation.
My code that uses Navigation2D.get_simple_path now only returns a empty PoolVector2Array, doesn't seem to find the path.
I am using a TileMap with navigation tiles defined but the TileMap is not a direct child under the Navigation2D node, don't know if that has anything to do with it.

Tried the NavigationAgent2D quickly to and of course a get the same problem there. The get_next_location() only returns the position of the agent's parent as it does when no path is found.

Also don't get a callback from the velocity_computed when using set_velocity.

Don't know it this problems only exist in this port to 3.x or in 4.0 also, because haven't tried navigation with the 4.0 brach.

@Duroxxigar
Copy link
Contributor Author

Duroxxigar commented May 4, 2021

@phelioz Thanks for testing this - I am unable to repro your issues. The TileMap should be a child of the nav node, yes. This is so the navigation node has navigation information that you can query.

For the nav agent - it needs to be under a nav node as well, or you can tell the agent what the nav node is by using agent.set_navigation(navigation). This may be why the signal and the get_next_location() function is not working for you.

@phelioz
Copy link

phelioz commented May 4, 2021

@Duroxxigar I have the TileMap as a child to the Navigation2D node, it is just not a direct child.
Like this:

  • Navigation2D
    • RandomNode
      • TileMap

But the don't think the problem is that, because is walks up when finding the Navigation2D node.
Have also tried to use agent.set_navigation(navigation) but dosen't work either.

Could be something crazy I am doing on my side.
But my navigation broke when I updated to this branch.
Will try to test this more to find out what's causing it on my side.

@aaronfranke
Copy link
Member

Debugging would be much easier if y'all post minimal reproduction projects. @Duroxxigar can post one that shows a working example, and @phelioz and others can post projects that don't work so that we can look for for regressions.

@phelioz
Copy link

phelioz commented May 4, 2021

Yeah I know, will try to recreate the problem in a minimal reproduction project on the weekend. Should have time then

@aaronfranke aaronfranke force-pushed the nav-backport branch 2 times, most recently from a6711be to 0e9616b Compare May 4, 2021 20:23
@phelioz
Copy link

phelioz commented May 5, 2021

Found my problem. The navigation stops working when you reparent the Navigation2D or the Navigation2DAgent node.
It happens because needed initialization when under a new parent only happens in NOTIFICATION_READY when it should happen in NOTIFICATION_ENTER_TREE notification.

Made a minimal reproduction project where you can test how it breaks when changing parent for Navigation2D.
project.zip

Made a pull-request against the Duroxxigar:nav-backport fixing this. Don't know if there was any reason to only do this in NOTIFICATION_READY.

@Duroxxigar
Copy link
Contributor Author

I don't think changing that is the solution. If you put it in _enter_tree, you can't guarantee everything that is needed is in place already.

@phelioz
Copy link

phelioz commented May 5, 2021

The NOTIFICATION_ENTER_TREE notification cascades down the tree so thought the needed things would be in place?
For example NavigationPolygonInstance uses the NOTIFICATION_ENTER_TREE notification for setup.

But on the other hand NavigationObstacle2D also uses NOTIFICATION_READY as Navigation2D and NavigationAgent2D right now.
So probably there is some problem with reparenting NavigationObstacle2D also right now.

But I am probably missing something here as you pointed out.

[Edit]
Yes, still getting some problems when moving the navigation nodes when using NOTIFICATION_ENTER_TREE

@vini-guerrero
Copy link

I'm not sure how updated this is against current 3.x branch, but I found this cherry pick backport of navigation server, maybe it could be helpful to validate some points, https://github.com/Scony/godot/tree/cherry-pick-new-navigation
This is a key feature for most of my projects, hope I could help testing and that this works out 👏👏

@Duroxxigar
Copy link
Contributor Author

I'm not sure how updated this is against current 3.x branch, but I found this cherry pick backport of navigation server, maybe it could be helpful to validate some points, https://github.com/Scony/godot/tree/cherry-pick-new-navigation
This is a key feature for most of my projects, hope I could help testing and that this works out 👏👏

That is an old and out of date one. This current branch is the most up-to-date that I'm aware of. I haven't had time to return to this to double check on it however. That said, as long as you're not doing what they stated (reparenting things) it should work fine.

The soonest I can look at this is this weekend most likely. I've just been incredibly busy this past month with work and projects.

@phelioz
Copy link

phelioz commented Jun 8, 2021

I also reckon that the "reparenting" issues also exist in the master branch, so probably not something done wrong with this port.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks pretty good overall, some remaining nitpicks for the docs. I'm actually going to amend the PR myself as it's probably the easiest (should have done this from the start instead of doing review comments but well...).

I'll also tweak the modules/navigation/SCsub to take #44457 into account.

doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
doc/classes/@GlobalScope.xml Outdated Show resolved Hide resolved
doc/classes/Navigation2DServer.xml Outdated Show resolved Hide resolved
doc/classes/Navigation2DServer.xml Outdated Show resolved Hide resolved
doc/classes/Navigation2DServer.xml Outdated Show resolved Hide resolved
doc/classes/NavigationServer.xml Outdated Show resolved Hide resolved
doc/classes/NavigationServer.xml Outdated Show resolved Hide resolved
doc/classes/NavigationServer.xml Outdated Show resolved Hide resolved
doc/classes/NavigationServer.xml Outdated Show resolved Hide resolved
doc/classes/NavigationServer.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga force-pushed the nav-backport branch 2 times, most recently from 2e30364 to 5b9bf70 Compare January 5, 2022 14:53
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Amended the docs and SCsub to address my remaining feedback, and fixed the remaining issue found by @mysterycoconut. Time to merge and get wider testing started :)

Change the entire navigation system.
Remove editor prefix from nav mesh generator class. It is now used for baking
at runtime as well.
Navigation supports obstacle avoidance now with the RVO2 library.
Nav system will also automatically link all nav meshes together to form one
overall complete nav map.
@akien-mga akien-mga merged commit 0b54b3d into godotengine:3.x Jan 5, 2022
@akien-mga
Copy link
Member

Thanks hugely for this backport!

Comment on lines +87 to +93
case NOTIFICATION_PARENTED: {
parent_node2d = Object::cast_to<Node2D>(get_parent());
reevaluate_agent_radius();
}
case NOTIFICATION_UNPARENTED: {
parent_node2d = nullptr;
}
Copy link
Member

Choose a reason for hiding this comment

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

These two cases are missing a break 😯

Copy link
Member

Choose a reason for hiding this comment

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

How come the compiler didn't warn about this? It's fallthrough but there's no fallthrough statement.

@BigZaphod
Copy link

I tried throwing together a very simple test of the navigation with an obstacle in the 3.5b1 build and it seems to behave oddly with the agent sometimes jumping or bumping as if it hit something. I can't tell if I've set this up wrong or am missing something or not, but I assume this isn't expected behavior? I used the blog post as a guide to configuring the agent and obstacle, but I can't rule out that I did something stupid here.

Godot3.5NavTest.mov

The scene tree looks like this:

image

And this is the only script (attached to the player):

extends RigidBody2D

var speed = 100

func _ready():
	$NavigationAgent2D.set_target_location(Vector2(0,0))


func _physics_process(delta):
	if $NavigationAgent2D.is_navigation_finished():
		linear_velocity = Vector2.ZERO
	else:
		var target = $NavigationAgent2D.get_next_location()
		var vel = global_position.direction_to(target) * speed
		$NavigationAgent2D.set_velocity(vel)


func _on_NavigationAgent2D_velocity_computed(safe_velocity):
	linear_velocity = safe_velocity

Full project:
Godot3.5NavTest.zip

@RonanZe
Copy link

RonanZe commented Feb 4, 2022

https://github.com/godotengine/godot/files/4021788/Navigation2.zip
I just tried this exemple (available here --> #34776) and it's seems to be really laggy. Is it a known issue?
I also tested it on an 3d projet and it's also really slow

@Zireael07
Copy link
Contributor

@RonanZe: Your issue sounds like #56838

@BlenderIsaac
Copy link

Is there export templates?

@Calinou
Copy link
Member

Calinou commented Jun 14, 2022

Is there export templates?

Yes, in 3.5.rc3 for example.

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.