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

Fix for threaded NavigationMesh baking under new thread guards #77412

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented May 23, 2023

Fixes threaded NavigationMesh baking under new SceneTree thread guards that blocked the process.

Fixes #77180.

As mentioned in the linked issue. The new SceneTree thread guards created the sudden problem that the navigation mesh baking could no longer parse the SceneTree for nodes when running on a thread, which is the default. Now, that the SceneTree is not thread-safe is not something that will change in the near future but single-threaded navmesh baking is also not really an option.

To solve this the bake functionality is now split between the SceneTree parsing step (not thread-safe) and the navigation mesh baking step (thread-safe).

This pr also ports a few fixes and smaller features like the NavigationMeshSourceGeometryData from pr #70724. The NavigationMeshSourceGeometryData adds the option to reuse already parsed source geometry or to add it procedurally. This can help to mitigate performance loss that could be caused by the now forced main thread parsing. To quote the description from the rework pr.

NavigationMeshSourceGeometryData

NavigationMeshSourceGeometryData is the resulting data of a parsing operation done with the NavigationMeshGenerator used for navigation mesh baking.

The advantage of having this data available in a resources is that it can be stored and loaded from disk. This helps to avoid runtime performance issues with parsing operations on larger scenes. NavigationMeshSourceGeometryData makes it possibility to split the source geometry parsing process from the navigation mesh baking process. It also makes it possible to reuse the same source data to bake multiple meshes with different parameters.

Previously both parsing and baking were forced into the same function and frame which resulted in unavoidable runtime stutter.
Since parsing requires reading from the SceneTree (which is not thread-safe) this always caused problems when baking with threads. With already parsed source geometry the baking can now happen safely in a background thread.

Since the NavigationMeshGenerator singleton has a problematic dependency and init chain in current Godot new functions were added to both the NavigationMeshGenerator and The NavigationServer3D (not bound for scripting). Expect them to change / disappear again when the dependency issues are resolved and / or the larger rework prs merged.

Parsing Performance Tips:

With the source geometry parsing now enforced on the main thread problems may show up for projects with heavy scenes and nodes but there are also some new options to solve them.

  • Use CollisionShapes instead of Meshes as the source geometry. The RenderingServer stalls when the parsing process needs to request mesh data from nodes. The PhysicsServer does not have the same problem. Also physics shapes are usually simpler and more optimized so in general faster to parse than visual meshes.
  • If the scene is too big for a single parse split the nodes into node groups on the NavigationMesh and do two or more parse operations over multiple frames. Then combine the vertices and polygons from all the NavigationMeshSourceGeometryData3D resources together in a single one for the baking that can happen on a background thread.
  • If nothing else works add your data procedurally with the NavigationMeshSourceGeometryData3D helper function at your own pace over multiple frames.

Fixes threaded NavigationMesh baking under new SceneTree thread guards that blocked the process.
@smix8 smix8 force-pushed the fix_threaded_navmesh_baking_4.x branch from cbc9b6d to ee14b01 Compare June 13, 2023 23:54
@akien-mga
Copy link
Member

CC @RandomShaper to review the threading assumptions.

@RandomShaper
Copy link
Member

As mentioned in the linked issue. The new SceneTree thread guards created the sudden problem that the navigation mesh baking could no longer parse the SceneTree for nodes when running on a thread, which is the default. Now, that the SceneTree is not thread-safe is not something that will change in the near future but single-threaded navmesh baking is also not really an option.

The scene tree was already non-thread-safe. That means that if the baking process was already playing safely with it (knowing what it is doing), it's just a matter of disabling the thread guards for the baking thread. However, I have the impression that this PR acknowledges that it wasn't safe earlier either and brings in a new, perfectly safe approach. Is that right?

Regarding guards, since the generic thread guards are still in the scope of Node we still have to add code like this:
ERR_FAIL_COND_MSG(!Thread::is_main_thread(), "The SceneTree can only be parsed on the main thread. Call this function from the main thread or use call_deferred().");
Sometime in the future we may want to move them to a more central location so that non-node code doesn't need to introduce their own bespoke guards, like the ones already present in some other places.

@smix8
Copy link
Contributor Author

smix8 commented Jun 14, 2023

I have the impression that this PR acknowledges that it wasn't safe earlier either and brings in a new, perfectly safe approach. Is that right?

@RandomShaper
It was never safe to access the SceneTree like that for the geometry parsing and plenty of issues originated from it in the past years since multi-threaded navigation mesh baking is a thing. Like random crashes or lockups because both nodes and servers had a problem with it, e.g. RenderingServer freezing while single-threaded or low performance while multi-threaded due to locks or crash when one of the nodes was changed / deleted in the middle of something.

This pr not only acknowledges all that but splits the navigation mesh baking steps between the must-be-main-thread geometry parsing part and the can-be-background-thread navigation mesh baking part.

When I made this issue / pr the "exception options" for the thread guards were not a thing and it wouldn't solve the core issues and crashes anyway for the geometry parsing. One part of me is actually glad that the SceneTree and thread changes now forces a hard reset for how Godot does navigation mesh baking. The old implementation never really was designed or worked for threads but no one had time or wanted to change the status quo.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go ahead and take the liberty to give an approval based on the excellent rationale on threading. Regarding navigation itself, I can't really give an informed opinion, but can't spot anything fishy.

@akien-mga akien-mga merged commit 0da20d0 into godotengine:master Jun 15, 2023
@akien-mga
Copy link
Member

Thanks!

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.

Runtime NavigationMesh baking on thread broken due to new thread guards
3 participants