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

Navigation mesh baking #253

Merged
merged 4 commits into from
Dec 1, 2023
Merged

Conversation

tcoxon
Copy link
Contributor

@tcoxon tcoxon commented Nov 24, 2023

The first two commits are not strictly related to navigation mesh baking, but since they clean up a couple of things about my previous contribution I thought you'd want them.

  • First commit fixes the weird button styling issue you noticed in Add terrain occlusion baking to the editor #242. I took a look at what the editor does for its own bake buttons, and it simply makes them flat, so that's what I've done here.

image

  • Second commit carves holes out of baked occluders and array meshes. There's a minor caveat regarding holes and occlusion baking noted in the commit message.

So on navigation (#46):

Unlike occlusion, adding geometry to a scene doesn't necessarily result in an additive change to the nav mesh. Think about the example of adding walls around an area: this results in portions of the nav mesh being removed. So to generate a nav mesh for terrain, you need to know not just about the terrain, but also everything else in the scene too. We can't upload meshes based solely on the terrain, so I don't think doing the baking in the Terrain3D node is the right approach.

The NavigationRegion3D node doesn't have any way to register custom geometry yet (godotengine/godot-proposals#5138), so the way I've chosen to do this is to create a new node. TerrainNavigationRegion3D extends NavigationRegion3D and implements a couple of functions that replicate the standard nav mesh baking functions that users of NavigationRegion3D are used to, but that work with terrain. I've done this with GDScript because it's not performance-critical code, but if you prefer I'll rewrite this in C++. It's not much code at all.

To try it out you'll need to:

  1. Add a TerrainNavigationRegion3D node to the Demo scene.
  2. Set its terrain property and give it a template NavigationMesh resource to use. This determines all the settings used to generate geometry and bake meshes, and the settings of your navigation agents need to match.
  3. Paint some navigable areas onto the terrain.
  4. Select the TerrainNavigationRegion3D node and click "Bake with Terrain":

image

The "Bake NavMesh" button is provided by Godot's built-in NavigationMeshEditor plugin. That button will bake a nav mesh for everything except the terrain, because it doesn't know about it. I could do some tricks in the nav_ui.gd script to detect it and hijack its signal or hide it if you want, but it would be brittle code.

In the final commit I've added an example script for runtime navmesh baking using the terrain, which I've used in the CodeGenerated example. The script is based on what I am using in my own project, but users will likely need to heavily customise it for their needs. If you enable Debug > Visible Navigation in the editor, you'll be able to see when the nav mesh is updated and how far it extends. It seems trivial to get outside the nav mesh, but then the player speed is set very high currently.

Props to SlashScreen for the intel and the makeshift baker tool which were immensely helpful to me.

@TokisanGames
Copy link
Owner

Great job! Thanks for submitting this.

  • Regarding the menus, we're going to consolidate them in editor: move baking tools into its own menu #254. I'll probably merge that before this is finalized so you can incorporate it.

  • Default Player speed is dropping to ~20 in the updated demo

  • Baking: how about we have a Terrain3D bake nav mesh button that upon click it instantiates and adds a TerrainNavigationRegion3D child, with terrain assigned, a navigation mesh assigned, and starts baking? And if one already exists, then frees it first before recreating.

  • Then maybe you could change the Bake with Terrain3D button to a NOP button labeled Use Terrain3D to bake or <-- Don't use this. What do you think?

  • It should print to the console when baking is finished successfully via button push. LOG(MESG... in C++.

  • Roope wanted everything in C++, eventually even all the UI stuff. I'm ok with UI and demos in gdscript, but otherwise we should do all other classes in C++.

  • I get this error when running. Can it be fixed?
    ERROR: Navigation map synchronization error. Attempted to merge a navigation mesh polygon edge with another already-merged edge. This is usually caused by crossing edges, overlapping polygons, or a mismatch of the NavigationMesh / NavigationPolygon baked 'cell_size' and navigation map 'cell_size'.

  • I see it generates properly. Adding the enemy to the demo worked great, and the code generated demo is fantastic. Really amazing work @tcoxon .

image

@TokisanGames
Copy link
Owner

#254 has been merged and cherry-picked into 0.9. Please rebase and add the new baker button to it. Feel free to improve the menu updates. This menu will house other tools in the future.

@TokisanGames TokisanGames mentioned this pull request Nov 27, 2023
13 tasks
@tcoxon
Copy link
Contributor Author

tcoxon commented Nov 27, 2023

Thanks for the feedback!

Baking: how about we have a Terrain3D bake nav mesh button that upon click it instantiates and adds a TerrainNavigationRegion3D child, with terrain assigned, a navigation mesh assigned, and starts baking? And if one already exists, then frees it first before recreating.

The annoying thing about NavigationMesh in Godot is that not only is it the output from baking, but it's also a very important input. It contains settings like where to find the source geometry (children of the NavigationRegion3D, or nodes with a particular group name?), what form the source geometry takes (collisions, or MeshInstance nodes?), how fine-grained to make the mesh, the sizes of agents, etc. Users need to be able to set up the NavigationMesh resource before they click any "Bake NavMesh" buttons. Deleting and recreating the NavigationRegion3D node would destroy any settings and signals the user has set up on that node too.

If all you want is to be able to bake a nav mesh while the Terrain3D node is selected I think I can do that another way. And it could let us get rid of the TerrainNavigationRegion3D class altogether. Here's what I'd do:

  1. Move bake_navigation_mesh_with_terrain from TerrainNavigationRegion3D to the editor baker script. Users lose the ability to use this at runtime but that's probably fine in most cases, and it can be easily copied by a user anyway. (The example runtime nav baker script would be unaffected by this.)
  2. If a Terrain3D node is selected when Terrain3D Tools > Bake NavMesh is pressed, search the edited scene tree for a NavigationRegion3D whose NavigationMesh settings would have it read geometry from the selected Terrain3D node (if it knew how). If there are 1 or more matching NavigationRegion3D nodes, rebake them using the baker script function. Otherwise, display an error message and prompt the user to set up a NavigationRegion3D node.
  3. If a NavigationRegion3D node is selected, display a reduced Terrain3D Tools menu (with only Bake NavMesh available), so that users can continue to rebake navmeshes with the NavigationRegion3D selected, the way they're used to.

What do you think?

@TokisanGames
Copy link
Owner

Baking: how about we have a Terrain3D bake nav mesh button that upon click it instantiates and adds a TerrainNavigationRegion3D child, with terrain assigned, a navigation mesh assigned, and starts baking? And if one already exists, then frees it first before recreating.

The annoying thing about NavigationMesh in Godot is that not only is it the output from baking, but it's also a very important input. It contains settings like where to find the source geometry (children of the NavigationRegion3D, or nodes with a particular group name?), what form the source geometry takes (collisions, or MeshInstance nodes?), how fine-grained to make the mesh, the sizes of agents, etc. Users need to be able to set up the NavigationMesh resource before they click any "Bake NavMesh" buttons. Deleting and recreating the NavigationRegion3D node would destroy any settings and signals the user has set up on that node too.

My goal is to have an easy one click option, like bake occlusion. I've never used navigation in Godot until reviewing your PR. It took me a minute even to follow your directions and get it working in the demo with the enemy. If it took me a minute, it will take others an hour or never.

What if we have the Terrain3D bake nave mesh button that adds the child and bakes with default settings (which is all I did with your instructions). Then they can adjust and rebake using either the terrain3d button (no destroy) or the navregion's Bake w/ Terrain3D button. Or they can add it manually, change settings and bake. Both buttons call the same function, and if the child is already there it just reuses it. If not, it creates a new one.

If all you want is to be able to bake a nav mesh while the Terrain3D node is selected I think I can do that another way. And it could let us get rid of the TerrainNavigationRegion3D class altogether. Here's what I'd do:

Move bake_navigation_mesh_with_terrain from TerrainNavigationRegion3D to the editor baker script. Users lose the ability to use this at runtime but that's probably fine in most cases, and it can be easily copied by a user anyway. (The example runtime nav baker script would be unaffected by this.)

I haven't looked closely enough to determine the differences between TerrainNavigationRegion3D and the runtime baker. If they essentially do the same thing, I suppose it's your call if we need the former or not. I think what you have is 95% of the way there. If we can just add the Terrain3D menu button to instantiate the class and bake automatically it's a simple change.

If a Terrain3D node is selected when Terrain3D Tools > Bake NavMesh is pressed, search the edited scene tree for a NavigationRegion3D whose NavigationMesh settings would have it read geometry from the selected Terrain3D node (if it knew how).

Is there any issue just requiring it remain a child of Terrain3D? As a gamedev I'd be happy to keep occluders and nav meshes specific to terrain3d organized under the node. If the child is there, it rebakes using it. If not, it creates a new one.

@tcoxon
Copy link
Contributor Author

tcoxon commented Nov 28, 2023

I've never used navigation in Godot until reviewing your PR. It took me a minute even to follow your directions and get it working in the demo with the enemy. If it took me a minute, it will take others an hour or never.

Navmeshes are kind of a complex feature, at least in Godot. But I think we can at least make it so that devs who know what to do with navmeshes find it straightforward to bake a navmesh with their terrain.

The #1 most tricky thing is making sure collisions, nav agent settings, and navmesh settings are all in agreement. If something is even slightly off, agents can fail to find obvious routes, get stuck in loops, or get stuck on collisions. The default navmesh settings are basically useless for anything other than demos.

Is there any issue just requiring it remain a child of Terrain3D? As a gamedev I'd be happy to keep occluders and nav meshes specific to terrain3d organized under the node. If the child is there, it rebakes using it. If not, it creates a new one.

Just to be clear, the navmesh is not specific to Terrain3D. The navmesh is for all of the scene's geometry, and has to be in order for agents to navigate over and around other objects that users place in the scene. This is different from occluders. The demo scene might have terrain as its only geometry, but real projects will have things like bridges and buildings that agents need to be able to navigate over and around.

There is a particular reason for wanting the nav region to be high up in the scene hierarchy, and the nav region should probably be parent to the terrain in most cases.

In the NavigationMesh resource you have to tell it where the NavigationRegion3D node can find geometry (geometry_source_geometry_mode). The three options are:

  • "Root Node Children", meaning all child and descendant nodes of the NavigationRegion3D node. (The choice of the word 'root' is unfortunate, since it doesn't use the scene root, instead it uses the nav region as the root of its search.) This is the default option, and the most commonly used, since it makes the relationship between the nav region and the geometry explicit in the scene tree.
  • "Groups With Children", which means all nodes in the scene that have a particular group assigned to them (and all their children and descendants). Groups are unfortunately kinda hidden in the editor, which means using this can lead to mistakes.
  • "Groups Explicit", which means all the nodes with a particular group assigned to them, but not their children.

Since "Root Node Children" is a preferred option in a lot of cases, a typical 3D scene has a hierarchy that looks something like this:
image

The nav region is near the top of the hierarchy, with all the geometry providers under it. But if the nav region is under the terrain, then the usual hierarchy is inverted. Instead of all the geometry providers being under the nav region, one of them is above it. And aside from that, it gives an inelegant special status to the Terrain3D node. Instead of simply being another object in the scene, it now has to be grandparent to all of the geometry in the scene.

I'm finding it difficult to explain everything with the right level of detail. Apologies for that. If you prefer, we can switch over to Discord for a bit.

Note that baked occluders sometimes still occlude slightly more than they
strictly speaking should, around the mouths of holes, due to being baked at a
lower LOD. Users may have cover the edge of the hole to disguise this, but
they probably have to do that to hide the jagged edge anyway.
@tcoxon tcoxon force-pushed the feature/navigation branch from a1dd9eb to 3e107ff Compare November 30, 2023 17:23
@tcoxon
Copy link
Contributor Author

tcoxon commented Nov 30, 2023

I've updated my branch in response to your feedback and our call on discord the other day.

Draft documentation for the wiki / readthedocs: https://gist.github.com/tcoxon/1971616c1352b0a9cb7088c80516686c

The return type of Terrain3DStorage::get_control changed but the utility functions still expect float. Not sure what you want to do here, but I've added overloads for the utility functions that take uint32_t to match the new get_control return type. Let me know if that's not what you want.

As we discussed, I've added two buttons to the Terrain3D tools menu: one for baking, one for the first-time setup. It performs a search for nav regions and terrains as the menu pops up to determine which buttons are relevant. I've kept them as separate buttons because they're different functions, and users won't want to accidental activate 'setup' if they're expecting 'bake' but have set up their scene incorrectly.

It should print to the console when baking is finished successfully via button push. LOG(MESG... in C++.

The C++ code in terrain_3d.cpp is only involved as far as generating source geometry. It doesn't know when navmesh baking has finished. I've added a print call to baker.gd after baking finishes that does the same thing. Is that OK?

I get this error when running. Can it be fixed?

The error is an engine bug. I searched the godot issue tracker and couldn't find it, so I've reported it here. It seems relatively harmless since the navmeshes still appear to work.

@TokisanGames
Copy link
Owner

The return type of Terrain3DStorage::get_control changed but the utility functions still expect float. Not sure what you want to do here, but I've added overloads for the utility functions that take uint32_t to match the new get_control return type. Let me know if that's not what you want.

My mistake. Yes, that's perfect, thanks.

Looks good, I'll review it soon.

@TokisanGames
Copy link
Owner

Excellent work. Thank you.

The runtimenav baker doesn't generate if the slope angle is too high. Where in the code or settings is that angle set?

@TokisanGames TokisanGames merged commit daeb1db into TokisanGames:0.9 Dec 1, 2023
This was referenced Dec 1, 2023
@tcoxon
Copy link
Contributor Author

tcoxon commented Dec 1, 2023

The max angle setting is on the template NavigationMesh on the runtime baker script.

@TokisanGames
Copy link
Owner

TokisanGames commented Dec 1, 2023

This is weird. Normally it's fine, but I saw this one odd area.

navRecording.2023-12-01.173953.mp4

@tcoxon
Copy link
Contributor Author

tcoxon commented Dec 1, 2023

Weird! I wonder if it has something to do with smix8's comment on the error issue last night: godotengine/godot#85548 (comment)

@tcoxon
Copy link
Contributor Author

tcoxon commented Dec 1, 2023

Just confirmed that it is that issue. smix8 has provided some advice in the comments.

Here is something we can put in the Terrain3D navigation documentation:

Navigation map synchronization error. Attempted to merge a navigation mesh polygon edge with another already-merged edge. This is usually caused by crossing edges, overlapping polygons, or a mismatch of the NavigationMesh / NavigationPolygon baked 'cell_size' and navigation map 'cell_size'

There are several possible causes for this. If the cell_size of your nav mesh matches the cell_size in your project settings, it's currently believed to be caused by the baking process creating lots of tiny edges in the nav mesh. This can cause strange pathfinding bugs, including agents taking indirect routes to their targets.

To fix your nav mesh, you can increase the detail_sample_distance and detail_sample_max_error settings on your NavigationMesh resource until the error goes away.

There might be something we can do in Terrain3D to work around this, but it sounds like it will be difficult. The fix would be to simplify the source geometry before feeding it to the engine nav mesh bake function - however I'm not sure how it would be possible to verify that this fixes all possible cases.

Another possible way we could solve this in Terrain3D (I'm speculating here) would be to directly modify the navmesh after it has been baked, but before it enters the map. Like maybe all we need to do is round vertex positions to the nearest multiple of cell_size/cell_height, and then remove any polygons that have 0 area. I think that would satisfy the navigation map edge merging constraints/limitations? It would require experimentation.

@TokisanGames
Copy link
Owner

TokisanGames commented Dec 2, 2023

simplify the source geometry before feeding it to the engine nav mesh bake function

You do have a lod baker.

Like maybe all we need to do is round vertex positions to the nearest multiple of cell_size/cell_height, and then remove any polygons that have 0 area.

Have you looked at the engine source for the exact conditions that causes it to print this error?

It would require experimentation.

Ideally we hit the next release without errors, and accurate navigation. But if we can't, I'm fine releasing and documenting the error until we figure it out.

@tcoxon
Copy link
Contributor Author

tcoxon commented Dec 3, 2023

I've moved this to #264.

You do have a lod baker.

Lod won't help, see #264.

Have you looked at the engine source for the exact conditions that causes it to print this error?

Yes, but there's a lot of code and I don't understand it all.

@tcoxon tcoxon deleted the feature/navigation branch July 12, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants