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

Deprecate functionality to copy visual Mesh data directly to a NavigationMesh #77702

Closed

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented May 31, 2023

Deprecates NavigationMesh.create_from_mesh() function and MeshInstance3D EditorPlugin Create Navigation Mesh dropdown option.

Both are a constant source of false user errors. The documentation already says what the functions do, copy vertices and indices from Mesh to NavigationMesh (and they do not more) but even what they do is wrong in most cases.

The vast majority of visual meshes have inherently invalid data for navigation mesh use. It makes little sense to support such a data copy directly with a dropdown Editor option that most new users will stumble over.

Both functions are from a time where Godot had a very simplistic concept of what a valid navigation mesh could and should be, with no navigation mesh connections and no NavigationServer to worry about.

For baking a valid navigation mesh from visual meshes either use a NavigationRegion or the NavigationMeshGenerator.
For procedural navigation meshes all the usual functions still exist to add vertices and polygons to a NavigationMesh. It is also a lot faster at runtime to use your vertices and indices arrays directly as the RenderingServer will make your performance bleed for receiving the visual mesh arrays.

@smix8 smix8 requested review from a team as code owners May 31, 2023 17:09
@smix8 smix8 added this to the 4.1 milestone May 31, 2023
@Sauermann
Copy link
Contributor

As I have just learned in #77452 (comment), the deprecated functionality should be wrapped in #ifndef DISABLE_DEPRECATED.

@Zireael07
Copy link
Contributor

So how do we make navmeshes from gridmaps and/or other procgen meshes now?

@smix8
Copy link
Contributor Author

smix8 commented May 31, 2023

For baking a valid navigation mesh from visual meshes either use a NavigationRegion or the NavigationMeshGenerator.
For procedural navigation meshes all the usual functions still exist to add vertices and polygons to a NavigationMesh.

Zireael07 I think you are referring to the GridMap issue from today. I already gave a step-by-step.
There is nothing specific to baking navigation meshes from GridMaps, it is basic NavigationRegion3D use. Depending on the NavigationMesh geometry property the GridMap cells with visual meshes or with collision shapes are parsed as source geometry for the navigation mesh baking. It works the same as with any other MeshInstance3D or StaticBody3D nodes.

…tionMesh

Deprecates NavigationMesh.create_from_mesh() function and MeshInstance3D 'Create Navigation Mesh' dropdown option.
@smix8 smix8 force-pushed the depr_copy_mesh_to_navmesh_4.x branch from aad8798 to ae86579 Compare May 31, 2023 20:31
@tavurth
Copy link
Contributor

tavurth commented Jun 1, 2023

@smix8 I would much rather see this function duplicated to NavigationMesh.create_from_plane_mesh, while deprecating the old version.

Previously you said that:

This create_from_mesh() function is a legacy problem that should be deprecated which I made a pr for #77702. This function gives users the illusion that they do something correct by copying visual Mesh data directly to a NavigationMesh while this is 95% of the times wrong, and those 5% are mostly plane meshes. All the documentation script examples that use this function assume plane, or at least flat surface, meshes without mentioning it explicit but that is another issue.

Ok great, maybe we can keep that functionality, but renamed?

Or is there a planned replacement which can handle more cases?

Currently it's relatively simple to create a Navmesh as you mentioned in this issue (#77690):

		# Add a navmesh
		var plane = PlaneMesh.new()
		plane.set_size(Vector2(0.25, 0.25))

		var nav_mesh = NavigationMesh.new()
		var nav_mesh.create_from_mesh(plane)

What would be the new proposed way to do this from code if we are deprecating this function?

@smix8
Copy link
Contributor Author

smix8 commented Jun 1, 2023

What would be the new proposed way to do this from code if we are deprecating this function?

You add a very simplistic mesh with 4 vertices and 4 indices by using the NavigationMesh.set_vertices() and NavigationMesh.add_polygon() functions.

var navmesh : NavigationMesh = NavigationMesh.new()
navmesh.vertices = PackedVector3Array([
	Vector3(-10.0, 0.0, 10.0),
	Vector3(10.0, 0.0, 10.0),
	Vector3(10.0, 0.0, -10.0),
	Vector3(-10.0, 0.0, -10.0),
])
navmesh.add_polygon(
	PackedInt32Array([0, 1, 2, 3])
)
$NavigationRegion3D.navigation_mesh = navmesh

This is an optimization over the PlaneMesh cause you do not stall the RenderingServer to create and access the PlaneMesh which is problematic at runtime especially with threads.

You can also use a convex polygon instead of the triangles of a PlaneMesh as the NavigationServer supports any flat convex polygon.

Or is there a planned replacement which can handle more cases?

The replacement is navigation mesh baking which handles all cases.

@tavurth
Copy link
Contributor

tavurth commented Jun 1, 2023

You add a very simplistic mesh with 4 vertices and 4 indices by using the NavigationMesh.set_vertices() and NavigationMesh.add_polygon() functions.

This looks easy enough. I guess it loses a little where a user can draw a plane mesh or polygon mesh and transform it in the editor window. Then one can use create_from_mesh and copy the transform using set_item_navigation_mesh_transform (if using GridMap).

I guess just documenting a lot more the NavigationMesh baking system and these problems will disappear, I still haven't had a chance to look into it yet.

(https://docs.godotengine.org/en/stable/tutorials/navigation/navigation_using_navigationmeshes.html#creating-3d-navigationmeshes)

@YuriSizov
Copy link
Contributor

It would be nice to provide some links to issues that this causes that cannot be resolved and can only be addressed by discouraging the use of the feature. It also seems to me that while the feature is flawed, there isn't a readily available replacement for it with the same level of convenience. I would like there to be a bit more discussion before we deprecate it.

So postponing to at least 4.2 to give it a chance to be discussed more.

@smix8
Copy link
Contributor Author

smix8 commented Jun 14, 2023

Examples from just the past 2 weeks why that functionality is a landmine that new users constantly run as if it is some kind of competition, and those are just official github issues, there are plenty more all over the Godot space.

#77695
#77690
#78245

@smix8 smix8 removed this from the 4.2 milestone Oct 18, 2023
@smix8 smix8 added the archived label Oct 18, 2023
@smix8 smix8 closed this Oct 18, 2023
@smix8 smix8 deleted the depr_copy_mesh_to_navmesh_4.x branch October 18, 2023 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants