[NavigationAgent2D/3D]: target_reached signal is emitted before internal state is updated

[sambriels]

When using a NavigationAgent2D or NavigationAgent3D the signal that gets emitted once the agent has reached its target (target_reached) is sent before the internal state is properly updated.

Namely, the target_reached property of the agent will be overridden once the signal has been processed. When listereners to the signal try to issue a new target, as in:

# agent is a NavigationAgent2D or NavigationAgent3D
...
func on_target_reached() -> void:
    agent.set_target_location(some_new_location)

its internal state is (correctly) reset, but directly thereafter the target_reached property is set to true again. This true is based on the old target, not the new one, and thus the agent enters a corrupt state where it thinks it reached it's target but it actually hasn't.

The solution is to delay the emission of the signal until the internal state has been changed. Which also makes sense since the actual reaching of the target should only complete once the internal representation is reflecting that.

I don't see any downsides to delaying the emission of the signal, and since all the tests still pass I assume this change can be safely made. I've attached a minimal reproduction project and two gifs from its execution before and after the change

minimal_repro_project

current_behaviour.webm

expected_behaviour.webm

[kleonc]

Needs a dedicated backport to 3.x, only the 2D part:

godot/scene/2d/navigation_agent_2d.cpp

Lines 481 to 488 in 33b80bd

	void NavigationAgent2D::_check_distance_to_target() {
	if (!target_reached) {
	if (distance_to_target() < target_desired_distance) {
	emit_signal("target_reached");
	target_reached = true;
	}
	}
	}

3D is fine in there (for whatever reason 😆):

godot/scene/3d/navigation_agent.cpp

Lines 492 to 499 in 33b80bd

	void NavigationAgent::_check_distance_to_target() {
	if (!target_reached) {
	if (distance_to_target() < target_desired_distance) {
	target_reached = true;
	emit_signal("target_reached");
	}
	}
	}

[clayjohn]

Makes sense to me. Thanks and congratulations on your first merged contribution!

[sambriels]

@kleonc about the backport, is this something that will be done at some point or is it expected I create a new PR targetting 3.x branch? In the pr_workflow document it says

The stable branches are named after their version, e.g. 3.1 and 2.1. They are used to backport bugfixes and enhancements from the master branch to the currently maintained stable release (e.g. 3.1.2 or 2.1.6). As a rule of thumb, the last stable branch is maintained until the next minor version (e.g. the 3.0 branch was maintained until the release of Godot 3.1). If you want to make PRs against a maintained stable branch, please check first if your changes are also relevant for the master branch, and if so make the PR for the master branch in priority. Release managers can then cherry-pick the fix to a stable branch if relevant.

For me it's fine either way, whatever is easiest for you 👍

[kleonc]

about the backport, is this something that will be done at some point or is it expected I create a new PR targetting 3.x branch?

@sambriels Actually I haven't added any cherry-pick label to this PR before so at this point it could be forgotten (if no one would backport it manually). AFAIK currently all cherry-picks are done by @akien-mga and if the changes between the master and 3.x branches are not one-to-one then he'd request a separate PR for 3.x. Hence I auto assumed a separate PR is needed here. But the changes needed in 3.x are actually a subset of the changes done in this PR so it's probably indeed cherry-pickable. So I'll mark it as such now. If it's not ok @akien-mga will let us know. 🙂
(but if you want feel free to open a dedicated PR against 3.x, would be fine as well)