-
-
Notifications
You must be signed in to change notification settings - Fork 21.2k
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
Integrated the new NavigationServer
and NavigationServer2D
#34776
Conversation
Awesome work! Will throughly review the coming week |
@AndreaCatania I noticed in the previous PR that you had, you did document everything in the .xml files, but I'm not seeing it in this PR. Perhaps I overlooked it? |
Looking forward to this. Though I didn't make it too far in testing yet, the engine crashes. (Win10 64-bit 118aaa5) In the 2D (Level3) example I copied the Rigidbody2Ds, since the interesting thing to me about RVO would be how lots of entities navigate and with different sizes and speeds.
|
@avencherus I've fixed a problem, I'm quite sure it was related. Can you please check it now? If it will continue to not work I'll test it in windows. However, I've updated the sample project by adding more characters and other changes to allow it: |
@Duroxxigar No, you are correct. That PR was based on the RVO only feature and that docs was written by @nathanmerrill; that documentation is no more valid, for this reason you can't find it here. However, if someone want to document it, I'm available to help him. |
AT LAST! |
I'm more happy with the merge navigation meshes/nav polygons part - I was trying to achieve merging in GDScript and had to ultimately scrap that in favor of using A* for pathing. |
@AndreaCatania Good to know. I've been eagerly looking forward to something like this for quite some time. Once I got some downtime this weekend, I was going to start working on the documentation myself. Most likely starting this evening (so in about 5-6 hours hopefully). I want this thing to be well understood. I also want to point out that the engine crashes when I try to add a navigation agent node. I have the issue reported on your fork of Godot (I figured that would be the right way to handle that). This still happens with your most recent change as well. |
Great work! |
/// | ||
/// Note: All the `set` functions are commands executed during the `sync` phase, | ||
/// don't expect that a change is immediately propagated. | ||
class NavigationServer : public Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a change slated for Godot 4.0, and 3D nodes will be renamed to have 3D suffixes, shouldn't this new system be called NavigationServer3D
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that, but this is not yet a decision made nor we are in a transition state. So I thought that it's not yet optimal a name like: NavigationServer3D
.
We could change idea in any moment and use another kind of naming strategy.
@AndreaCatania Thanks. Gave it a retest this morning. The changes appear to have resolved the crash, and your example project works well. Though the agents are a bit jittery and sometimes become sluggish, is this expected, and to be resolved by the end implementation of the game? Also I reran the project where I fiddle about and created the crash, and oddly the agents ignore the walls in many cases. After some time, a few agents at the end of goal will restart their pathing. Here is my project so you can inspect what I did. (This is from Level3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is to be merged, maybe update the copyright headers to 2020. :)
servers/navigation_2d_server.cpp
Outdated
ClassDB::bind_method(D_METHOD("agent_set_radius", "agent", "radius"), &Navigation2DServer::agent_set_radius); | ||
ClassDB::bind_method(D_METHOD("agent_set_max_speed", "agent", "max_speed"), &Navigation2DServer::agent_set_max_speed); | ||
ClassDB::bind_method(D_METHOD("agent_set_velocity", "agent", "velocity"), &Navigation2DServer::agent_set_velocity); | ||
ClassDB::bind_method(D_METHOD("agent_set_velocity_target", "agent", "target_velocity"), &Navigation2DServer::agent_set_target_velocity); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be either target_velocity
or velocity_target
for consistency. Same for the 3D variant. I like the latter more because it'd show up next to agent_set_velocity
in the docs (although I'm not sure there is any naming conventions in use.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "velocity target" is the least confusing. "Target velocity" sounds similar to "target's velocity" (velocity of some other guy besides the agent), while "velocity target" is more obviously the level of velocity to be reached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, shouldn't it be set_agent_radius
instead of agent_set_radius
? I think there's a discussion about this specifically somewhere, and I can't find it, but #33026 is related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I see such naming I think of the convention used in servers in general, as they are close to a C-like API, so because they use to take a RID as "self", the type of "self" would be prepended as the first word of the function's name. In that sense it's consistent with servers
@starry-abyss It's already possible. Each |
@Naryosha Updated |
@avencherus
Yes this is expected. You have to set the agent properties (like
This is expected since the walls doesn't have any collider.
This is a feature. The |
b84f613
to
8f8edaa
Compare
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "enabled"), "set_enabled", "is_enabled"); | ||
|
||
ADD_SIGNAL(MethodInfo("navigation_mesh_changed")); | ||
ADD_SIGNAL(MethodInfo("bake_done")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this signal be named bake_finished
? I believe other signals throughout the engine have it in this way. I don't recall a signal with "done" for the name. (Only suggesting for constancy sake)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other signals with the _done
suffix, like:
_thumbnail_done
_tree_thumbnail_done
_file_list_thumbnail_done
_preview_done
_thread_done
_request_done
I didn't found any event with _finish
suffix.
Despite this, do you think it would make more sense: bake_finish
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreaCatania We have signals that end with completed
and finished
, so we should use past tense instead of present tense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AndreaCatania The first one that pops in my mind is with animations. I believe audio is "finished" as well. I wasn't aware of the other signals you listed. Other than the _request_done
one, but that one slipped my mind.
@Calinou If this is the case - does this call in the question some of the ones that they mentioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fault, in the fast check that I did this morning I searched for: _finish"
instead of _finished"
.
I've already changed it, it's bake_finished
now.
Is there any plan for this to be backported to 3.2 in one of the LTS releases? I'm asking since I'm almost done with version 0.1 of my own navigation module (focused on procedural generation, so no editor integration) for Godot 3.2, and now I am wondering if I should continue working on it beyond that. https://github.com/TheSHEEEP/godotdetour Though from what I've seen so far, both projects have very different foci and features. Eg. godotdetour supports multiple navmeshes at the same time from the get-go, as well as marking areas as water, grass, etc. and I might add other things as I need them. |
@TheSHEEEP This would be very unlikely to be backported to 3.2, since it's a major change and has already broken compatibility (here). Generally, for something to be backported it would either be a useful or important bug fix, or an important feature that's very unlikely to cause regressions. In a way, the "breaks compat" label is also a "backporting this would probably be bad" label. |
I thought so, just better to ask than to continue working on it if there's a significant risk it might just get obsolete the moment it is "done" ;) Though I might have continued anyway due to the different foci of the libraries. |
This PR breaks MinGW/clang cross build form macOS (I have not tested it on Linux or with gcc) with following error:
|
i get this Error when Compiling on Windows with MinGW :
Sorry , i copy it directly from CMD |
And from Linux with MinGW/GCC :)
|
Why does Rvo2 rely on Windows SDK headers in the first place? That's bogus and should be fixed, the library should be cross-platform. |
Nothing in the |
Does this change solve #28235? |
This has been backported to the 3.x branch in #48395 and will be in Godot 3.5 when it's released. |
This is the integration of the
NavigationServer
proposed here: godotengine/godot-proposals#332What's new
Retro compatibility
This new
NavigationServer
, that is used for both 3D and 2D part, even if it offers many new features it's fully retro compatible.Map & Regions welding
The
Navigation
node now creates a newMap
in the server, and is possible to add as manyRegion
s you desire by simply addingNavigationMeshInstance
under theNavigation
node (perfectly equal as before). The difference is that theRegions
now are welded all together to form one single fully navigable map.This operation is cheap because it doesn't regenerate the
NavigationMesh
, rather it just connects the many (probably pre-baked)Regions
. You can remove and addRegions
as you like at runtime.NavigationMesh
bakingThe
NavigationMesh
can be pre baked in the editor (as before), or can be baked during the game without stopping it. This operation is not cheap and for this reason it's done in a separate thread, once it's done, it automatically sets the newNavigationMesh
without any stop. (See the below 3D gif)NavigationAgent
Even if the old workflow is still valid, now it's possible to use the
NavigationAgent
node that simplifies a lot the map navigation.The
NavigationAgent
just needs to know the desired final location; it internally takes care to compute the optimal path. This path may or may not change during the game, but what you have to only care about is to just reach the next location returned by:$NavigationAgent.get_next_location()
, let the agent do the work for you.Collision Avoidance
Since the
NavigationMesh
baking is an heavy operation, an algorithm for collision avoidance that take cares ofDynamic
obstacles got introduced.As mention before, you can compute the player velocity using the position returned by the
get_next_location()
function but this function doesn't take care of the dynamic obstacles.To avoid collisions you have to set the calculated velocity to the agent, using:
$NavigationAgent.set_velocity(velocity)
, then the event:_velocity_computed(safe_velocity):
is emitted with the parametersafe_velocity
that is a version of your velocity vector that avoids dynamic obstacles.Obstacles
The collision avoidance takes care to avoid collision with other agents and obstacles. Agents and Obstacles are different only for the fact that Obstacles can't be directly controlled. Similar to the Agents, is possible to create an Obstacle by adding the node
NavigationObstacle
.The
NavigationObstacle
doesn't have any parameter because it's automatically initialized and updated, so you have just to add it to the scene.2D
The
Navigation2DServer
is a bridge to theNavigationServer
that allow to use the latter using 2D classes, likeVector2D
orTransform2D
.The nodes
NavigationAgent2D
andNavigationObstacle2D
works exactly like the 3D version described above.Multi thread safe
The
NavigationServer
keeps tracks of any call and executes these during the sync phase. This mean that you can request any change to your map using any thread without any worrying.3D version
In the below GIF you can see a character that navigates the map, composed by two different regions, while avoiding collisions with dynamic objects. Most important, you can see that after a bit the right side region is modified and a call to
bake
allows me toRegenerate
the navigation mesh and so reach the above part before not reachable.Also note that the pink box is a kinematic object, while the black balls are rigid bodies.
In this GIF it's possible to see that moving a region, automatically executes the map re-link and immediately allows the navigation. This step is done in real time.
2D version
Similarly to the above 3D version, the character is navigating two regions while avoiding the dynamic obstacles collisions.
Example project
Navigation2.zip
This work has been kindly sponsored by IMVU.