From eba3acadaca8527554a0cf30ba86a9939e1d7758 Mon Sep 17 00:00:00 2001 From: smix8 <52464204+smix8@users.noreply.github.com> Date: Fri, 21 Jun 2024 10:49:46 +0200 Subject: [PATCH] Fix thread-use causing navigation polygon data corruption Fixes navigation polygon data corruption caused by thread-use that changed vertices or polygons while the navigation polygon was processed, e.g. by server region sync, navmesh baking or user thread updates. --- .../navigation/2d/nav_mesh_generator_2d.cpp | 12 ++--- modules/navigation/nav_region.cpp | 4 +- scene/resources/2d/navigation_polygon.cpp | 45 ++++++++++++++++++- scene/resources/2d/navigation_polygon.h | 4 ++ 4 files changed, 52 insertions(+), 13 deletions(-) diff --git a/modules/navigation/2d/nav_mesh_generator_2d.cpp b/modules/navigation/2d/nav_mesh_generator_2d.cpp index aa4c7977237..8c2fb424635 100644 --- a/modules/navigation/2d/nav_mesh_generator_2d.cpp +++ b/modules/navigation/2d/nav_mesh_generator_2d.cpp @@ -1010,8 +1010,7 @@ void NavMeshGenerator2D::generator_bake_from_source_geometry_data(Refset_vertices(Vector()); - p_navigation_mesh->clear_polygons(); + p_navigation_mesh->clear(); return; } @@ -1045,8 +1044,7 @@ void NavMeshGenerator2D::generator_bake_from_source_geometry_data(Refset_vertices(Vector()); - p_navigation_mesh->clear_polygons(); + p_navigation_mesh->clear(); return; } @@ -1071,11 +1069,7 @@ void NavMeshGenerator2D::generator_bake_from_source_geometry_data(Refset_vertices(new_vertices); - p_navigation_mesh->clear_polygons(); - for (int i = 0; i < new_polygons.size(); i++) { - p_navigation_mesh->add_polygon(new_polygons[i]); - } + p_navigation_mesh->set_data(new_vertices, new_polygons); } #endif // CLIPPER2_ENABLED diff --git a/modules/navigation/nav_region.cpp b/modules/navigation/nav_region.cpp index fc1db391ae0..c91071a3abe 100644 --- a/modules/navigation/nav_region.cpp +++ b/modules/navigation/nav_region.cpp @@ -84,11 +84,11 @@ void NavRegion::set_transform(Transform3D p_transform) { void NavRegion::set_navigation_mesh(Ref p_navigation_mesh) { #ifdef DEBUG_ENABLED - if (map && !Math::is_equal_approx(double(map->get_cell_size()), double(p_navigation_mesh->get_cell_size()))) { + if (map && p_navigation_mesh.is_valid() && !Math::is_equal_approx(double(map->get_cell_size()), double(p_navigation_mesh->get_cell_size()))) { ERR_PRINT_ONCE(vformat("Attempted to update a navigation region with a navigation mesh that uses a `cell_size` of %s while assigned to a navigation map set to a `cell_size` of %s. The cell size for navigation maps can be changed by using the NavigationServer map_set_cell_size() function. The cell size for default navigation maps can also be changed in the ProjectSettings.", double(p_navigation_mesh->get_cell_size()), double(map->get_cell_size()))); } - if (map && !Math::is_equal_approx(double(map->get_cell_height()), double(p_navigation_mesh->get_cell_height()))) { + if (map && p_navigation_mesh.is_valid() && !Math::is_equal_approx(double(map->get_cell_height()), double(p_navigation_mesh->get_cell_height()))) { ERR_PRINT_ONCE(vformat("Attempted to update a navigation region with a navigation mesh that uses a `cell_height` of %s while assigned to a navigation map set to a `cell_height` of %s. The cell height for navigation maps can be changed by using the NavigationServer map_set_cell_height() function. The cell height for default navigation maps can also be changed in the ProjectSettings.", double(p_navigation_mesh->get_cell_height()), double(map->get_cell_height()))); } #endif // DEBUG_ENABLED diff --git a/scene/resources/2d/navigation_polygon.cpp b/scene/resources/2d/navigation_polygon.cpp index 274b13a487e..a845809bf2f 100644 --- a/scene/resources/2d/navigation_polygon.cpp +++ b/scene/resources/2d/navigation_polygon.cpp @@ -38,6 +38,7 @@ #ifdef TOOLS_ENABLED Rect2 NavigationPolygon::_edit_get_rect() const { + RWLockRead read_lock(rwlock); if (rect_cache_dirty) { item_rect = Rect2(); bool first = true; @@ -65,6 +66,7 @@ Rect2 NavigationPolygon::_edit_get_rect() const { } bool NavigationPolygon::_edit_is_selected_on_click(const Point2 &p_point, double p_tolerance) const { + RWLockRead read_lock(rwlock); for (int i = 0; i < outlines.size(); i++) { const Vector &outline = outlines[i]; const int outline_size = outline.size(); @@ -80,6 +82,7 @@ bool NavigationPolygon::_edit_is_selected_on_click(const Point2 &p_point, double #endif void NavigationPolygon::set_vertices(const Vector &p_vertices) { + RWLockWrite write_lock(rwlock); { MutexLock lock(navigation_mesh_generation); navigation_mesh.unref(); @@ -89,10 +92,12 @@ void NavigationPolygon::set_vertices(const Vector &p_vertices) { } Vector NavigationPolygon::get_vertices() const { + RWLockRead read_lock(rwlock); return vertices; } void NavigationPolygon::_set_polygons(const TypedArray> &p_array) { + RWLockWrite write_lock(rwlock); { MutexLock lock(navigation_mesh_generation); navigation_mesh.unref(); @@ -104,6 +109,7 @@ void NavigationPolygon::_set_polygons(const TypedArray> &p_array } TypedArray> NavigationPolygon::_get_polygons() const { + RWLockRead read_lock(rwlock); TypedArray> ret; ret.resize(polygons.size()); for (int i = 0; i < ret.size(); i++) { @@ -114,6 +120,7 @@ TypedArray> NavigationPolygon::_get_polygons() const { } void NavigationPolygon::_set_outlines(const TypedArray> &p_array) { + RWLockWrite write_lock(rwlock); outlines.resize(p_array.size()); for (int i = 0; i < p_array.size(); i++) { outlines.write[i] = p_array[i]; @@ -122,6 +129,7 @@ void NavigationPolygon::_set_outlines(const TypedArray> &p_array } TypedArray> NavigationPolygon::_get_outlines() const { + RWLockRead read_lock(rwlock); TypedArray> ret; ret.resize(outlines.size()); for (int i = 0; i < ret.size(); i++) { @@ -132,6 +140,7 @@ TypedArray> NavigationPolygon::_get_outlines() const { } void NavigationPolygon::add_polygon(const Vector &p_polygon) { + RWLockWrite write_lock(rwlock); Polygon polygon; polygon.indices = p_polygon; polygons.push_back(polygon); @@ -142,20 +151,24 @@ void NavigationPolygon::add_polygon(const Vector &p_polygon) { } void NavigationPolygon::add_outline_at_index(const Vector &p_outline, int p_index) { + RWLockWrite write_lock(rwlock); outlines.insert(p_index, p_outline); rect_cache_dirty = true; } int NavigationPolygon::get_polygon_count() const { + RWLockRead read_lock(rwlock); return polygons.size(); } Vector NavigationPolygon::get_polygon(int p_idx) { + RWLockRead read_lock(rwlock); ERR_FAIL_INDEX_V(p_idx, polygons.size(), Vector()); return polygons[p_idx].indices; } void NavigationPolygon::clear_polygons() { + RWLockWrite write_lock(rwlock); polygons.clear(); { MutexLock lock(navigation_mesh_generation); @@ -164,6 +177,7 @@ void NavigationPolygon::clear_polygons() { } void NavigationPolygon::clear() { + RWLockWrite write_lock(rwlock); polygons.clear(); vertices.clear(); { @@ -172,12 +186,31 @@ void NavigationPolygon::clear() { } } +void NavigationPolygon::set_data(const Vector &p_vertices, const Vector> &p_polygons) { + RWLockWrite write_lock(rwlock); + vertices = p_vertices; + polygons.resize(p_polygons.size()); + for (int i = 0; i < p_polygons.size(); i++) { + polygons.write[i].indices = p_polygons[i]; + } +} + +void NavigationPolygon::get_data(Vector &r_vertices, Vector> &r_polygons) { + RWLockRead read_lock(rwlock); + r_vertices = vertices; + r_polygons.resize(polygons.size()); + for (int i = 0; i < polygons.size(); i++) { + r_polygons.write[i] = polygons[i].indices; + } +} + Ref NavigationPolygon::get_navigation_mesh() { MutexLock lock(navigation_mesh_generation); if (navigation_mesh.is_null()) { navigation_mesh.instantiate(); Vector verts; + Vector> polys; { verts.resize(get_vertices().size()); Vector3 *w = verts.ptrw(); @@ -188,11 +221,12 @@ Ref NavigationPolygon::get_navigation_mesh() { w[i] = Vector3(r[i].x, 0.0, r[i].y); } } - navigation_mesh->set_vertices(verts); for (int i(0); i < get_polygon_count(); i++) { - navigation_mesh->add_polygon(get_polygon(i)); + polys.push_back(get_polygon(i)); } + + navigation_mesh->set_data(verts, polys); navigation_mesh->set_cell_size(cell_size); // Needed to not fail the cell size check on the server } @@ -200,38 +234,45 @@ Ref NavigationPolygon::get_navigation_mesh() { } void NavigationPolygon::add_outline(const Vector &p_outline) { + RWLockWrite write_lock(rwlock); outlines.push_back(p_outline); rect_cache_dirty = true; } int NavigationPolygon::get_outline_count() const { + RWLockRead read_lock(rwlock); return outlines.size(); } void NavigationPolygon::set_outline(int p_idx, const Vector &p_outline) { + RWLockWrite write_lock(rwlock); ERR_FAIL_INDEX(p_idx, outlines.size()); outlines.write[p_idx] = p_outline; rect_cache_dirty = true; } void NavigationPolygon::remove_outline(int p_idx) { + RWLockWrite write_lock(rwlock); ERR_FAIL_INDEX(p_idx, outlines.size()); outlines.remove_at(p_idx); rect_cache_dirty = true; } Vector NavigationPolygon::get_outline(int p_idx) const { + RWLockRead read_lock(rwlock); ERR_FAIL_INDEX_V(p_idx, outlines.size(), Vector()); return outlines[p_idx]; } void NavigationPolygon::clear_outlines() { + RWLockWrite write_lock(rwlock); outlines.clear(); rect_cache_dirty = true; } #ifndef DISABLE_DEPRECATED void NavigationPolygon::make_polygons_from_outlines() { + RWLockWrite write_lock(rwlock); WARN_PRINT("Function make_polygons_from_outlines() is deprecated." "\nUse NavigationServer2D.parse_source_geometry_data() and NavigationServer2D.bake_from_source_geometry_data() instead."); diff --git a/scene/resources/2d/navigation_polygon.h b/scene/resources/2d/navigation_polygon.h index b9816f900c6..4e99660b0ee 100644 --- a/scene/resources/2d/navigation_polygon.h +++ b/scene/resources/2d/navigation_polygon.h @@ -36,6 +36,7 @@ class NavigationPolygon : public Resource { GDCLASS(NavigationPolygon, Resource); + RWLock rwlock; Vector vertices; struct Polygon { @@ -153,6 +154,9 @@ public: void clear(); + void set_data(const Vector &p_vertices, const Vector> &p_polygons); + void get_data(Vector &r_vertices, Vector> &r_polygons); + NavigationPolygon() {} ~NavigationPolygon() {} };