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

Enabling/disabling NavigationLink3D / navigation_layer not updating paths #83707

Closed
wilfredjonathanjames opened this issue Oct 21, 2023 · 5 comments · Fixed by #83709 or #83802
Closed

Comments

@wilfredjonathanjames
Copy link

wilfredjonathanjames commented Oct 21, 2023

Godot version

4.2-beta1

System information

MacOS 12.3

Issue description

image

Using a simple NavigationLink3D setup I've noticed the following behaviour:

  • Setting enabled at runtime has no effect - in-game or with toolscripts
    • Seems to only take effect on ready or init
  • Enabling a navigation layer does have an effect on calculated paths, allowing the agent to continue
  • Disabling a navigation layer has no effect on calculated paths

Video here. This shows the simple case.

It seems that the only way to recalculate a newly blocked path is to emit some signal and ask those agents to recalculate. That will likely work and I'll be doing that next, however for the following reasons I think this is a bug or incorrect behaviour:

  • There's no point in having an enabled flag if it's ignored at run-time
  • Enabling a navigation layer has the correct effect, but disabling it does not, which is inconsistent

Steps to reproduce

  1. Load the repro project
  2. Select the Agent node and enable walk
  3. Select the Barrier node and toggle enable
  4. This toggles the Barrier/NavigationLink3D:enabled field, and the Barrier/StaticBody:collision_layer
  5. Notice the path is not blocked

To underscore the problem, disable the Barrier/StaticBody3D:collison_layer while the barrier is enabled and see that the agent walks straight through the barrier even though the link is disabled.

Additionally, notice that:

  • If you set NavigationLink3D:enabled to false and run the game, toggling with the provided button will not work. The link cannot be turned back on.
  • Toggling enabled/disabled in-editor also has no effect, because the tool script has already initialised the link

Minimal reproduction project

https://github.com/wjagodfrey/godot_4_2_beta1_navigation_link_toggle

@smix8
Copy link
Contributor

smix8 commented Oct 21, 2023

The bug was that the link connections were added regardless of enabled state when the navigation map was rebuild.

Navigation map geometry changes trigger a signal for automatic path updates on all NavigationAgents on that map.

That said changing just detail properties (layers, costs) of navigation bases like regions or links do currently not trigger automatic path updates.

Those kind of changes do not require a rebuild of the navigation map geometry, hence no signal, and are considered in the next path query. That those property changes do not trigger a navigation map rebuild is currently kept on purpose because the navigation map can only do full rebuilds, not incremental. Full rebuilds are very costly so not triggering those with a e.g. a layer change is currently the only escape for runtime performance while still dynamically changing path searches. Might be changed when the navigation map sync gets changed to not be such a runtime performance burden or having different dirty stages for the signals.

Unrelated but in the test project there are also a few setup and script problems that made it hard to test e.g. that movement does not work with a half-path due the blocked function when set target is not reachable, the patrol point target switch does not work with a blocked path due to the distance check or that apply_state() gets called twice, once by the setter and once by the button signal.

@wilfredjonathanjames
Copy link
Author

@smix8 thank you.

You say that "those property changes do not trigger a navigation map rebuild". Can you explain why enabling a navigation layer on a link causes agents to recalculate paths, but disabling the layer won't? That's the other issue I've raised here.

Video here. The point of the video is to show that enabling a navigation layer triggers a path recalculation, while disabling does not.


apply_state() gets called twice, once by the setter and once by the button signal

Good catch, that wasn't intentional.

@wilfredjonathanjames
Copy link
Author

Note that the above issue with navigation_layer is replicable with the previous version of code, which you can find on the navigation_layer_toggle branch.

I've pulled master and updated the repo to attempt some fixes now that you've fixed enabled toggling.

Two things I've noticed:

  1. The path will be recalculated on link enable/disable if the agent is mid-path, but not if they reach the end of the path without reaching the target
  • I worked around this by asking unsuccessful agents to recalculate the path every frame:
    • nav_agent.target_position = nav_agent.target_position
  • This feels hacky
  • If I were to do this in an optimised way I would have the Agent connect to a signal and wait for a NavigationLink3D:enabled state change
  • This could be a bug. If the Agent hasn't reached their target, can we really say that navigation has ended? And if it's not a bug, what is the correct way to ask the Agent to recalculate their path? nav_agent.target_position = nav_agent.target_position doesn't sit right.
  1. is_navigation_finished and is_target_reached return true optimistically, when the agent enters the final path segment but before reached the final position.
  • I found that I needed to check how far from get_final_position Agent was
  • if abs(nav_agent.get_final_position().x - global_position.x) < EPSILON:
  • This also feels hacky
  • Is this a bug? If not, it would be good to have an officially supported method to test whether the Agent has reached the true final position or target

@wilfredjonathanjames
Copy link
Author

Further observation: if a NavigationLink3D is disabled in-editor, it cannot be enabled at runtime. Video here.

@akien-mga or @smix8 can we please reopen this

@smix8
Copy link
Contributor

smix8 commented Oct 23, 2023

If not, it would be good to have an officially supported method to test whether the Agent has reached the true final position or target

That is is_navigation_finished() and the navigation_finished signal. The official documentation script examples use this. The agent's real target position is always the path end which is the closest reachable position to the target_position on navigation mesh. That is the position returned by get_final_position() or the last index of the navigation path array.

The path will be recalculated on link enable/disable if the agent is mid-path, but not if they reach the end of the path without reaching the target

If a NavigationAgent finishes its current path it will no longer auto-update the path on a map change as the path is already finished and the agent state reset.

Can you explain why enabling a navigation layer on a link causes agents to recalculate paths, but disabling the layer won't?

Changing navigation_layers on a link or region does not trigger a navigation map update or automatic update for NavigationAgent paths.

Changing a region's or link's enabled property does due to legacy compatibility as in the past disabling a region removed it from the navigation map entirely so it trigger a map update.

Changing the navigation_layers on the NavigationAgent node also triggers a path update for the agent.

Further observation: if a NavigationLink3D is disabled in-editor, it cannot be enabled at runtime.

I looked at it again and the NavigationLink nodes are missing multiple property updates for the server object in the constructor. So if the node was disabled this disabled was not send to the server object without toggling it again. Made a pr to fix that but this issue would only block the first property update or when the value has not changed, not all runtime updates.

Test project issues

That said even with the fix pr your specific test project will still not be working due to the custom movement and update logic. I recommend falling back to a more simple agent test script, e.g. the one from the official documentation.

For the ping-pong movement between the two targets set them on the navigation_finished signal. Right now the is_target_reachable() check alone is blocking the majority of the logic when the target position is not reachable by the agent.

The the distance checks on the update_target() will also cause that no new target position is set when the agent is in the middle. So when the path is finished and the agent ends up in the middle due to the blocked path/link it will just start to do nothing.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Nov 14, 2023
@akien-mga akien-mga changed the title [4.2-beta1] Enabling/disabling NavigationLink3D / navigation_layer not updating paths Enabling/disabling NavigationLink3D / navigation_layer not updating paths Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants