Navigation design does not align with the rest of the engine

[0x0ACB]

Tested versions

4.x

System information

Issue description

The current design of the navigation system exhibits significant inconsistencies compared to the rest of the engine. These inconsistencies hinder flexibility, extensibility, and alignment with established patterns in the engine's architecture. Below are the key issues and proposed resolutions.

1. NavigationServer

The NavigationServer is implemented in a way that discourages modularity and alternative implementations. The core implementation resides in a module, but it doesn't allow for easy substitution or extensibility. Unlike other servers, which permit registration of alternatives, the NavigationServerXDManager only supports a default navigation server. Custom implementations require deactivating the default module during the build process and can not be implemented via GDExtension. This should be brought in line with other servers such as the PhysicsServer.

2. NavigationMesh

TheNavigationMesh API forces the navigation system to conform strictly to the recast paradigm. Custom implementations (e.g., navigation volumes, voxel octrees, or simple navigation graphs) are impractical due to the hard dependency on the NavigationMesh. Even replacing the NavigationServer with a custom implementation still requires inheriting from the default NavigationMesh. In contrast the PhysicsServer for example just uses a generic Shape RID and does not rely on any specific Shape in its API.

3. NavigationRegion

In it's current form this is really just a NavigationMeshInstance seeing as a region can not have more than one mesh assigned in any case, based on point 2 this should just be a generic resource like Shape that does not rely on any specific underlying implementation.

4. NavigationObstacle and NavigationLink

These nodes deviate from the engine's resource management conventions. The rest of the engine always keeps the data in a Resource class and then adds it to the scene tree with an instance class that holds the resource. See MeshInstanceXD and Mesh, CollisionShapeXD and ShapeXD, PathXD and CurveXD etc.

5. source_geometry_parser_create and source_geometry_parser_set_callback

The newly implemented geometry parsing mechanism is overly complicated and diverges from established systems. Functions like source_geometry_parser_create and source_geometry_parser_set_callback require static functions and resource management and are inconsistent with the design of similar systems in the engine. This could be much simpler by adding a virtual function _parse_source_geometry to Node2D and Node3D similar to

godot/scene/3d/visual_instance_3d.h

Line 205 in 9ee1873

virtual Ref<TriangleMesh> generate_triangle_mesh() const;

or

godot/scene/3d/visual_instance_3d.h

Line 65 in 9ee1873

virtual AABB get_aabb() const;

and additionally a navigation_layer property similar to

godot/scene/3d/physics/collision_object_3d.h

Line 48 in 9ee1873

uint32_t collision_layer = 1;

to decide in which navigation meshes/regions the geometry of the node should actually be included in. This way a custom node just needs to override/implement _parse_source_geometry and be done with it.

Conclusion

The navigation system would greatly benefit from adopting the engine's established design principles. Refactoring these components to align with the rest of the engine will improve modularity, flexibility, and developer experience.

Steps to reproduce

Minimal reproduction project (MRP)

[AThousandShips]

For point 4, what would be in the resource for NavigationLink? It is entirely "local" in the sense that all data (except for the costs, and arguably layers) is only relevant for the node, the comparison would be to have a Label store the text property in a resource, it doesn't make sense to me to put anything from NavigationLink into a resource. Nodes like PhysicsBody doesn't store any similar properties like mass etc. in a resource

[0x0ACB]

True, the navigation link in its current from probably should not have a resource attached. The point still stands for the obstacle though. Personally I would like to see more complex options for links though where the entry and exit points could be generic shapes/areas to allow marking cliffs with jump links for example.

[AThousandShips]

For NavigationObstacle I'd say it only makes sense to put the vertices in a resource, but that's an experimental feature, but in that sense it'd be the same as Polygon2D which also doesn't store the vertices in a resource

[0x0ACB]

I would say it's more similar to an Area or Collider though. Given that an obstacle would usually also have some form of collision and both of those use Shapes. Didn't actually know about Polygon2D as I have never used that before.

[AThousandShips]

Both Line2D and Polygon2D are like this, they store their points in themselves and not any resource, so this is arguably the standard, as not the opposite, as NavigationObstacle, like Polygon2D, doesn't allow arbitrary Shapes, but a set of vertices, so I'd say this is following the norm of the engine rather than breaking any standard

[0x0ACB]

The underlying issue though is that this blows up the scene files quite a bit. Especially during development when they are all stored as Text. With obstacles you will probably have quite a few that use the same shape so not being able to store them in a common resource can be quite an issue. I had to implement my own resource for this because I had scene files approaching 100mb and take way too long to load.

[AThousandShips]

That's a valid point, but I'd say very much outside the scope of a discussion about aligning the navigation code with other parts, as this is not a discrepancy, I think that idea would be best as a feature proposal on its own, or it'll drown in other topics

[AThousandShips]

This could be much simpler by adding a virtual function _parse_source_geometry to Node2D and Node3D similar to

Imo this breaks locality of relevance and makes things pretty convoluted, what if you want a whole bunch of different nodes to share one behavior? How would yo make them all use the same code easily without a lot of boilerplate code? You'd also have to add it to a bunch of nodes that otherwise wouldn't have any script attached

Meanwhile you can make it do that by calling such a method on your node, by just making a dedicated callable for that

[0x0ACB]

Imo this breaks locality of relevance and makes things pretty convoluted, what if you want a whole bunch of different nodes to share one behavior? How would yo make them all use the same code easily without a lot of boilerplate code?

Not really seeing the issue here to be honest. The precedent has already been set with how collision data is generated and nothing is stopping you from implementing some generic parsing logic and calling that from the proposed _parse_source_geometry function.

func _parse_source_geometry(...) -> void:
 super(...)
 my_custom_parser(...)
 ...

(static) func my_custom_parser(...) -> void:
 ...

Is not really a lot of boilerplate. It's actually even less boilerplate than the current implementation which requires you to first allocate a parser, then set the callback and finally also take care to free that parser. Also not like the current implementation doesn't already use a similar solution as well:

godot/scene/3d/physics/static_body_3d.cpp

Lines 153 to 154 in 9ee1873

	CylinderMesh::create_mesh_array(arr, cylinder->get_radius(), cylinder->get_radius(), cylinder->get_height());
	p_source_geometry_data->add_mesh_array(arr, transform);

godot/scene/3d/physics/static_body_3d.cpp

Lines 161 to 162 in 9ee1873

	SphereMesh::create_mesh_array(arr, sphere->get_radius(), sphere->get_radius() * 2.0);
	p_source_geometry_data->add_mesh_array(arr, transform);

godot/scene/3d/physics/static_body_3d.cpp

Lines 145 to 146 in 9ee1873

	CapsuleMesh::create_mesh_array(arr, capsule->get_radius(), capsule->get_height());
	p_source_geometry_data->add_mesh_array(arr, transform);

godot/scene/3d/physics/static_body_3d.cpp

Lines 137 to 138 in 9ee1873

	BoxMesh::create_mesh_array(arr, box->get_size());
	p_source_geometry_data->add_mesh_array(arr, transform);

If anything the current implementation encourages breaking separation of concerns by allowing parsing of all kinds of different nodes in a single function no matter if they use the same logic or not. Also the current implementation does not allow you to reuse any of the existing parsing logic as it is all C++ only. I already had this issue with the previous implementation where I had to copy the code for parsing convex shapes since the code for that was private inside the parse_source_geometry_data function of the NavMeshGenerator in the navigation module. With the virtual method you would have at least access to the parent implementation. I would also argue that if your nodes have that similar parsing logic they should have a common parent.

Additionally the current implementation has one big issue and these two functions where actually what triggered me to write this issue in the first place. If I inherit from MeshInstance3D I can not change how my node is parsed since every parser is called for every node and this check here

godot/scene/3d/mesh_instance_3d.cpp

Line 864 in 9ee1873

if (mesh_instance == nullptr) {

will be valid for any node that inherits from MeshInstance3D. So if I wanted to return a simplified mesh for navmesh baking I could do that but I wouldn't gain anything from that because the default parser would still be called and add the complex geometry. This is both unintuitive and inflexible.

You'd also have to add it to a bunch of nodes that otherwise wouldn't have any script attached

Why? How would your custom nodes have any relevant parsing logic if they do not even have any custom properties? I mean sure you could contrive some esoteric example where your source geometry parser generates something based on the node name. But why would you do that externally and not in the nodes script? To throw your own words back at you "Imo this breaks locality"

[AThousandShips]

If we're comparing with the physics engine it does use callbacks, not virtual methods, for callbacks that is, complex behavior that requires data to be fed in and out

But I can see various contexts where not calling a method on a node would make sense for the geometry parser, for example just doing something like adding extra margins or some other filtering or parsing

But a callback is the most balanced position IMO, where you can choose to call a method on the node itself, or do something else, having a virtual method on the class is IMO really limiting the flexibility

[smix8]

Please move to a proposal discussions. This has no qualifiers for being in the issue tracker.

[0x0ACB]

Please move to a proposal discussions. This has no qualifiers for being in the issue tracker.

This is discussing defects with the current implementation though. Not proposing any new features, I mean I guess adding the ability to replace the NavigationServer would count as a new feature. I would however say this very much is an issue and not a proposal even though it is indeed proposing solutions to the issues with the current implementation.

If we're comparing with the physics engine it does use callbacks, not virtual methods, for callbacks that is, complex behavior that requires data to be fed in and out

Where exactly can I register a callback with the physics system that gets called for every node it needs a collision shape for? Cause the editor functionality to create a collision shape from a mesh is using virtual functions here:

godot/editor/plugins/mesh_instance_3d_editor_plugin.cpp

Lines 65 to 69 in 9ee1873

	case SHAPE_TYPE_TRIMESH: {
	shapes.push_back(p_mesh->create_trimesh_shape());
	if (p_verbose && shapes.is_empty()) {
	err_dialog->set_text(TTR("Couldn't create a Trimesh collision shape."));

I would say baking a navigation mesh is pretty similar in functionality. The only callbacks in the physics server are acting on specific instances and are not generically called for every physics object registered with the server.

But a callback is the most balanced position IMO, where you can choose to call a method on the node itself, or do something else, having a virtual method on the class is IMO really limiting the flexibility

Please give an example of where a global callback getting called for every node without any way of preventing that is somehow more flexible than a virtual function you can customize for all your custom node types. Same as the global method choosing to call a method on the node itself or not, the method on the node can choose to call a global method or not and additionally can choose to extend on the functionality already present or replace it entirely.