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 mesh data corruption #93392

Conversation

smix8
Copy link
Contributor

@smix8 smix8 commented Jun 20, 2024

Fixes navigation mesh data corruption caused by thread-use that changed vertices or polygons while the navigation mesh was processed, e.g. by server region sync, navmesh baking or user thread updates.

  • Adds NavigationMesh RW locks to make data thread-safe where relevant.
  • Adds NavigationMesh C++ functions to set_data() and get_data() in one call to avoid desync.
  • Moves NavigationMesh property warning checks from server region sync() to region navigation mesh setter.
  • Copies vertices and polygon indices for a pending update immediately from resource to server internal.
  • Removes server region mesh property as it was mostly useless and prevented a practically unused resource from being freed.

Pairs with #93407

The NavigationMesh has a problematic legacy API where it is possible to set vertices and polygon indices that are not in sync due to both using different functions with no real validation.

When user-threads were manipulating the resource there was opportunity to read the resource while another thread changed the data in the middle. This was very common in large projects when users would reuse the same navigation mesh resource and started the next update as soon as they set the navigation mesh on a region, not knowing that it takes some time for the sync to happen.

@smix8 smix8 added this to the 4.x milestone Jun 20, 2024
@smix8 smix8 changed the title Fix thread use causing navigation mesh data corruption Fix thread-use causing navigation mesh data corruption Jun 20, 2024
@smix8 smix8 requested review from RandomShaper and a team June 20, 2024 14:32
@smix8 smix8 force-pushed the if_you_cant_behave_responsible_you_get_locked branch from 7eddad0 to 236a40a Compare June 21, 2024 06:05
Fixes navigation mesh data corruption caused by thread use that changed vertices or polygons while the navigation mesh was processed, e.g. by server sync or baking.
@smix8 smix8 force-pushed the if_you_cant_behave_responsible_you_get_locked branch from 236a40a to fd727ab Compare June 21, 2024 07:39
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 2e1b651 into godotengine:master Jun 21, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@smix8 smix8 deleted the if_you_cant_behave_responsible_you_get_locked 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.

3 participants