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 thread-use causing navigation source geometry data corruption #93407

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Jun 20, 2024

Fixes navigation source geometry data corruption caused by thread-use that changed vertices or indices while the source geometry data was used in a parsing process or read from by the navmesh baking.

resolves #90934

Pairs with #93392

  • Extends the existing NavigationMeshSourceGeometryData RW locks to make data thread-safe where relevant.
  • Adds C++ functions to set_data() and get_data() in one call to avoid desync.
  • Updates navigation mesh generator baking to use the new, more thread-safe functions.

The issue was the same as with the navigation mesh. If users kept a source geometry object around to add procedual geometry for baking, e.g. with a chunk system, they did run into conflict when using a thread for the procedual "parsing" or baking step by corrupting the source data.

I tested this and the other PR together by baking various large godot 3D demos while also adding custom geometry without a lock issue or crash, still more testing is welcome.

TODO would be to extend to the navigation mesh generator parsing as well but that requires also changing the nodes so postponed for now as that part needs to run single-threaded anyway due to the SceneTree.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

I am not able to evaluate / test this, but navigation is important.

Fixes navigation source geometry data corruption caused by thread-use that changed vertices or indices while the source geometry data was used in a parsing process or read from by the navmesh baking.
@smix8 smix8 force-pushed the lock_this_geometry_up branch from 5b3187d to d4722b9 Compare June 21, 2024 06:06
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.

Looks good and makes sense at first glance.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jun 21, 2024
@akien-mga akien-mga merged commit 04a530f into godotengine:master Jun 21, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@smix8 smix8 deleted the lock_this_geometry_up branch June 21, 2024 08:50
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.

NavigationMeshSourceGeometryData is not thread-safe
4 participants