Make target_desired_distance affect the navigation of NavigationAgent2D/3D

[ershn]

Make target_desired_distance affect the navigation of NavigationAgent2D/3D.

• If the target is reachable, end the navigation only when the target is reached
• If the target is unreachable, end the navigation when the last waypoint is reached (same as before)

I created a test project to compare the old and new behavior.
Navigation agent tests.zip

Below are screenshots of running the test scenes on 4.1 and this branch.

Test scene screenshots

NavigationAgent2D

4.1

This branch

NavigationAgent3D

Legend

• red sphere -> waypoint area
• red cylinder -> position where a waypoint_reached signal was emitted
• blue sphere -> target area
• blue cylinder -> position where the target_reached signal was emitted
• white cylinder -> position where the navigation_finished signal was emitted

4.1

This branch

Notes

• The diff is bigger than I would have liked but a good chunk of it is because I extracted some code from update_navigation to individual functions (either out of necessity or to be able to express the algorithm with higher level methods). There isn't actually that much new code.

• I also updated the documentation of the signals/methods/properties that were affected by this change. Some of the changes are because the behavior changed, others are because the documentation wasn't actually 100% correct from the start.

• Fixes NavigationAgents' is_target_reached returns false even within target_desired_distance to the target_position #82560
  Bugsquad edit:

• Fixes The "Target Reached" signal on the NavigationAgent2D node does not reliably trigger when the path desired distance is set above the target desired distance #85247

[ershn]

This is unrelated to the PR but I have some questions about the implementation of NavigationAgents.

• Is there a reason NavigationAgent2D/3D are Nodes and not Node2Ds/Node3Ds ? IMHO making them Node2D/3Ds should remove the need to set a parent and allow to get the position directly in the node. It should then simplify the implementation / remove the possibility for usage errors.
• Currently the state update happens through calls to get_next_path_position/is_navigation_finished/get_final_position and to some degree when a NOTIFICATION_INTERNAL_PHYSICS_PROCESS is received. This way you only pay for what you use but this can lead to some finicky behavior. The same update function can also end up running several times a frame. What about updating everything on a NOTIFICATION_INTERNAL_PHYSICS_PROCESS and making the above methods const ?

[smix8]

I think what should change is to make sure that the target_desired_distance is never allowed to be lower than the path_desired_distance. The path following is stopped and the state reset when the path end is reached, there shouldn't be random checks running after this point. The NavigationAgent defaults should be updated to match this. Maybe print a warning if a user tries to set the value lower but clamp it anyway.

Users can set a target_position that is impossible because it is never reachable, e.g. because there is no connected navigation mesh in reach of that position. The current changes in this pr would make the distance check run forever every physics tick for an impossible check or for a path that already has ended and reset state. That should imo not be applied like that.

@ershn
You could join the navigation channel in the Godot Contributors Chat (https://chat.godotengine.org/) if you have general questions about the navigation system so we don't derail this pr. The short answer to your questions is physics / node / legacy jank.

[ershn]

I think what should change is to make sure that the target_desired_distance is never allowed to be lower than the path_desired_distance. The path following is stopped and the state reset when the path end is reached, there shouldn't be random checks running after this point. The NavigationAgent defaults should be updated to match this. Maybe print a warning if a user tries to set the value lower but clamp it anyway.

Yes, that's a reasonable fix. But aren't there genuine use cases where you would want the agent to loosely follow the path but still end up close to the target ? I honestly don't need that in my project right now but that doesn't seem to be an unreasonable requirement.
I just thought of another way to fix the problem. In case the target is reachable, for the last waypoint distance check use target_desired_distance instead of path_desired_distance. This way we can have target_desired_distance to be lower than path_desired_distance without having to run a check after the navigation is finished. It also doesn't seem to contradict the class documentation. What do you think ?

You could join the navigation channel in the Godot Contributors Chat (https://chat.godotengine.org/) if you have general questions about the navigation system so we don't derail this pr.

Sure ! I didn't know about that chat. I'll ask my questions there then. Thank you !

[smix8]

But aren't there genuine use cases where you would want the agent to loosely follow the path but still end up close to the target ?

Yes there could. To support this the pathfollowing would need to be changed so that it does not reset immediately on the last path array point. So this would need to change the check in a way similar like you already outlined. E.g. use the target_desired_distance if feasible but use the path_desired_distance still if the target is not right or reachable.

[ershn]

Ok, I'll update the PR to do that then.

[ershn]

I updated the PR to implement the behavior as described in a previous comment.

In case the target is reachable, for the last waypoint distance check use target_desired_distance instead of path_desired_distance.

Please note that this will slightly change the timing at which the waypoint_reached and navigation_finished signals are fired (maybe link_reached too ?). Until now, in case the target was reachable, for the last waypoint these signals were fired when the agent's position changed to be within path_desired_distance to target_position. Now they will be fired when the agent's position changes to be within target_desired_distance to target_position.
I think that change is ok because the documentation said nothing about this and the new behavior makes sense.

Additional notes:

• I also updated the class reference to add more details and make it read a bit better. Also changed though to but to make it sound more professional.
• I had to do some refactoring to avoid code duplication / having update_navigation call itself.
• I took the opportunity to rename update_navigation to _update_navigation to stick with private methods' naming convention.

Also, I'm not sure if we still need to call _check_distance_to_target in NOTIFICATION_INTERNAL_PHYSICS_PROCESS since we are now using target_desired_distance for distance checks directly. It's probably necessary if the agent teleports to the target_position without following the path but do we have to account for that ?

[ershn]

I pushed a small fix to the class reference. I was confusing e.g. with i.e.

[ershn]

Thinking about the problem again, I think the fix in its current form would lead to surprising behavior in some specific cases.

Until now, target_desired_distance didn't affect the navigation. It just controlled when is_target_reached changes to true, when target_reached is fired, and the value of is_target_reachable. This fix would make it affect the navigation too, and therefore make reasoning about NavigationAgents more difficult. IMO the effect target_desired_distance has on the agent shouldn't change with this PR (target distance related methods/properties are actually pretty independent from the rest of navigation agents, they are just convenience methods you could easily implement in user scripts).

Also, in case target_desired_distance is large enough that it encompasses a waypoint with its path_desired_distance radius, the timing at which navigation_finished is fired would be a bit surprising.

The diagram below shows how the navigation agent behaves currently.

• red circle: path desired distance
• blue circle: target desired distance
• orange line: path taken by the agent
• grey line: path taken by the agent if it doesn't stop on navigation_finished
• blue dot: target_reached signal + is_target_reached turns to true
• green dot: waypoint_reached signal
• purple dot: navigation_finished signal + is_navigation_finished turns to true + last waypoint_reached signal

Below is how the behavior would change with this fix.

Conceptually the current fix would make it so that navigation_finished signals when we reached the target. However as you can see in the diagram above, this wouldn't always hold true. In the above scenario, I also don't think that stopping the navigation when the blue circle is reached is the correct solution.

@smix8 I think now that we should just make it so that the target distance check runs until necessary, i.e. when the target is reachable, keep running the target distance check on NOTIFICATION_INTERNAL_PHYSICS_PROCESS until the target is reached. I cannot think of other easy ways to fix the problem. What do you think ?

---

For reference, the diagram below illustrates the problem I'm trying to fix with this PR.

As you can see there is no blue dot. The target_reached signal is never fired and is_target_reached never turns to true.

[smix8]

I also don't think that stopping the navigation when the blue circle is reached is the correct solution.

I think it would be. The target_desired_distance was designed to control when the pathfollowing should end compared to the normal pathfollowing distance controlled with path_desired_distance. The target_desired_distance also should override the path_desired_distance check for the last path position. At one point there was only a single desired_distance property but to have different behavior for the path end is the entire reason why this was separated into two properties.

[ershn]

I see ! If that's the intended effect of target_desired_distance, the current behavior definitely needs a fix then.

Before making changes, I'd like your opinion on the following scenario. What do you think should happen here ?

If we end the navigation when the blue circle is reached, the last returned waypoint will actually be outside of the target's radius. But maybe there isn't much we can do here ? (and we should let the user be responsible for stopping the movement when the target is reached ?)

[smix8]

If we end the navigation when the blue circle is reached, the last returned waypoint will actually be outside of the target's radius. But maybe there isn't much we can do here ?

For that to happen users would need to set the target_desired_distance willingly to such a large value so the result is on them. I think important is that the default values work without much issues. If path_desired_distance and target_desired_distance are the same value the user choice exist but people that just use the defaults wouldn't be negatively affected by the change.

[ershn]

I agree that users are responsible for the settings they use and that common cases should be prioritized.
But what I was trying to make sure with that example is that the algorithm is sound, i.e. it makes sense in all cases. IMO testing an algorithm at its limits is a good way to check if it's actually correct (that's how I found the bug above).

We seem to agree that there is no problem here, so I'll proceed with the changes 👍

[ershn]

The diff doesn't make it clear but _trigger_waypoint_reached and _transition_to_navigation_finished only contain code that was previously in update_navigation. The processing itself is unchanged.

[ershn]

Since reaching the target now ends the navigation, it's not enough anymore to just try to update target_reached and emit the corresponding signal on each frame. We need to also update the navigation state and I think this should be left to _update_navigation.

[ershn]

As far as I understand undershooting is not a problem because the agent will simply move a little bit closer to the waypoint each frame.

[ershn]

This proved to be more work than I expected but the PR should be ready for review now.
I also updated the PR description with test scene screenshots.

[TitanShadow12]

I see ! If that's the intended effect of target_desired_distance, the current behavior definitely needs a fix then.

Before making changes, I'd like your opinion on the following scenario. What do you think should happen here ?

If we end the navigation when the blue circle is reached, the last returned waypoint will actually be outside of the target's radius. But maybe there isn't much we can do here ? (and we should let the user be responsible for stopping the movement when the target is reached ?)

I originally thought navigation should finish once the destination waypoint is reached:

• path to all waypoints using path_desired_distance
• if the next waypoint is the last one, then consider it reached if within target_desired_distance instead of path_desired_distance

I like this implementation though, since it allows you to preempt the pathfinding if you get within target_desired_distance (if that is greater than path_desired_distance) - possibly allowing an agent to (maybe only incidentally) avoid pathing around a wall if unnecessary.

I'm fine with this fix because it solves my particular problem - I don't want my agents to be picky about hitting their path waypoints, but I want them to stop almost exactly where they're told (target_desired_distance < path_desired_distance). The 4.1 implementation doesn't take this difference into account, and I'm excited to see this change.

[ershn]

Rebased and re-tested.

[smix8]

I tested it for the discussed changes and it works in general fine for the target desired distance as I would expect but what worked unexpectedly is that target desired distance seem to not reliably or at all overrule the path desired distance for the last path position.

The target_desired_distance also should override the path_desired_distance check for the last path position.

E.g. if I set a NavigationAgent target_desired_distance to 0.5 and the path_desired_distance to 5.0 the agent does never move in to 0.5 distance of the target. It stops the path at 5.0 distance to the last path position. Since in my test I immediately query a new path to the next patrol position when the path ends my agent basically never even comes close to the target position.

[ershn]

@smix8 I can't reproduce the problem you're seeing on my side. Could you provide a test project to reproduce the behavior you are seeing ? Or maybe the source code you are using for the agent ?

if I set a NavigationAgent target_desired_distance to 0.5 and the path_desired_distance to 5.0 the agent does never move in to 0.5 distance of the target. It stops the path at 5.0 distance to the last path position.

The only reason I can see this happening is because the target is unreachable. If the target is unreachable the target_desired_distance doesn't override path_desired_distance.

Or maybe it's because you are requesting a new path directly when reaching the last waypoint ? E.g. even after this change and even if target_desired_distance < path_desired_distance, waypoint_reached will be sent when the agent moves within path_desired_distance of the last waypoint. If you want the agent to reach the target you need to wait for the target_reached or navigation_finished signal.
Conceptually the target doesn't replace the last waypoint, rather it acts as an alternate waypoint that takes precedence over other waypoints.

The test project I uploaded in the description tests for the scenario you are describing but everything seems to be working as expected. In the images below, the white cross/cylinder is also the final position of the agent.

[smix8]

I tested the 2D scene and it works as expected.

I tested the 3D scene with path_desired_distance = 2.0 and target_desired_distance = 0.1.

The agent stops at the path desired distance circle.

I noticed that the problem happens with any target desired distance value below 0.5 in that scene.
If the value is >= 0.5 it works as expected.

I had the same low distance values in my own test project so I ran in that same issue.

[ershn]

Ah I see ! In that scene 0.5 is the y distance between the scene geometry and the computed navigation mesh.
The problem was that get_final_position didn't subtract path_height_offset from the position it returned. This position was then used by is_target_reachable which resulted in the target being considered unreachable when target_desired_distance < 0.5. This bug should be fixed now.
It seems this problem has been there for a while but it became more apparent with this PR, since is_target_reachable now affects navigation.

On an unrelated note, I took this opportunity to also change the order in which the target_reached and navigation_finished signals are sent (target_reached first now). This will allow using is_target_reached in navigation_finished signal callbacks. Re-tested everything after this change.

[smix8]

I tested the change with the 3D demo scene but with any target_desired_distance below 0.5 is enough to get the agent stuck on the last path_desired_distance red circle instead of moving closer to the target_desired_distance blue circle.

[ershn]

Sorry but I really can't reproduce the problem this time. Could you provide more info ?

I did a fresh build on this branch, downloaded the zip in the description, and run every 3D scene with values of 0.1 (the minimum), 0.4, 0.5 for target_desired_distance. I also tested a bunch of different combinations with path_desired_distance but I couldn't find a case where it doesn't work as expected.

It also doesn't seem like I forgot to push a commit.

Please note that you need to have a path_height_offset of 0.5 (the default) in the 3D test scenes, otherwise the target won't be considered reachable and the agent will never reach it.

[smix8]

Sorry but I really can't reproduce the problem this time.

It is fine and works. I noticed I made a mistake. I set some properties in script from testing. When I started to print those values I noticed that they did not match what I set on the NavigationAgent node.

[oxeron]

When can we hope a release ? I think I'm affected by this bug

[YuriSizov]

Looks great to me! Documentation changes alone are very much appreciated, and improved logic and code organization are just a cherry on top 🙂

[YuriSizov]

Thanks!

[PythonCircuit]

When can we hope a release ? I think I'm affected by this bug

I am trying to create an agent that utilizes Path Desired Distance with .1 and Target Desired Distance with .1 but it just seems to get stuck in a re-loop 99.99% of the time, the goal is to have 99.99% precision in my case . Hopefully this fixes that issue if I understand this thread correctly. I have not tested or reviewed the code being implemented here.
I have read documentation and example code possible for setting up a simple navigation region 3d on simple geometry with basic settings with character controlled agent that simply clicks a position on a map and uses the navigation system to get there. I also cannot get the navigation mesh to properly work with default settings on very smooth geometry for hills inclines/slopes without setting the cell height to .01 to .1f which seems to cause other issues I cannot fathom. I personally am using Terrain3D for terrain but had exact same issues on a default plane/block with 45 degree slopes

[oxeron]

Any news ?

For the moment I still use a hack to avoid actor bouncing around the final destination (Godot 4.3-stable) :

# HACK
if navigation_agent.target_position.distance_to(actor.global_position) < 5:
	navigation_agent.target_reached.emit()