Fix potential deadlock when using data grid

doc/source/changelog.md

@@ -13,6 +13,7 @@ Semver is not yet in place, so each version can have breaking changes, although
 Primarily developped with Godot 4.3.
 
 - Fixes
+    - Fixed potential deadlock when using detail rendering and various editing features (thanks to lenesxy, issue #693)
     - `VoxelInstanceLibrary`: Editor: reworked the way items are exposed as a Blender-style list. Now removing an item while the library is open as a sub-inspector is no longer problematic
     - `VoxelInstancer`: Fixed persistent instances reloading with wrong positions (in the air, underground...) when mesh block size is set to 32
     - `VoxelLodTerrain`:

storage/voxel_data.cpp

@@ -1052,10 +1052,9 @@ void VoxelData::get_blocks_with_voxel_data(
 void VoxelData::get_blocks_grid(VoxelDataGrid &grid, Box3i box_in_voxels, unsigned int lod_index) const {
 	ZN_PROFILE_SCOPE();
 	const Lod &data_lod = _lods[lod_index];
-	RWLockRead rlock(data_lod.map_lock);
 	const int bs = data_lod.map.get_block_size() << lod_index;
 	const Box3i box_in_blocks = box_in_voxels.downscaled(bs);
-	grid.reference_area_block_coords(data_lod.map, box_in_blocks, &data_lod.spatial_lock);
+	grid.reference_area_block_coords(data_lod.map, data_lod.map_lock, box_in_blocks, data_lod.spatial_lock);
 }
 
 SpatialLock3D &VoxelData::get_spatial_lock(unsigned int lod_index) const {

storage/voxel_data.h

@@ -361,7 +361,7 @@ class VoxelData {
 		// This lock should be locked in write mode only when the map gets modified (adding or removing blocks).
 		// Otherwise it may be locked in read mode.
 		// It is possible to unlock it after we are done querying the map.
-		RWLock map_lock;
+		mutable RWLock map_lock;
 		// This should be used when reading or writing voxels/metadata in blocks. It uses block coordinates as
 		// spatial unit.
 		mutable SpatialLock3D spatial_lock;

storage/voxel_data_grid.h

@@ -2,6 +2,7 @@
 #define VOXEL_DATA_GRID_H
 
 #include "../storage/voxel_buffer.h"
+#include "../util/thread/rw_lock.h"
 #include "../util/thread/spatial_lock_3d.h"
 #include "voxel_data_map.h"
 
@@ -21,30 +22,39 @@ class VoxelDataGrid {
 	// }
 
 	// TODO This API is a bit risky, it should just be encapsulated into VoxelData maybe
-	inline void reference_area_block_coords(const VoxelDataMap &map, Box3i blocks_box, SpatialLock3D *sl) {
+	inline void reference_area_block_coords(
+			const VoxelDataMap &map,
+			RWLock &map_lock,
+			const Box3i blocks_box,
+			// Will be referenced for operations, assuming its lifetime is equal or greater than the grid
+			SpatialLock3D &spatial_lock
+	) {
 		ZN_PROFILE_SCOPE();
 		create(blocks_box.size, map.get_block_size());
+
 		_offset_in_blocks = blocks_box.position;
 		_logical_offset_in_blocks = blocks_box.position;
-		if (sl != nullptr) {
-			// Locking is needed because we access `has_voxels`
-			sl->lock_read(blocks_box);
-		}
-		// WARNING: Assumes `map` is already locked for reading
-		blocks_box.for_each_cell_zxy([&map, this](const Vector3i pos) {
-			const VoxelDataBlock *block = map.get_block(pos);
-			// TODO Might need to invoke the generator at some level for present blocks without voxels,
-			// or make sure all blocks contain voxel data
-			if (block != nullptr && block->has_voxels()) {
-				set_block(pos, block->get_voxels_shared());
-			} else {
-				set_block(pos, nullptr);
-			}
-		});
-		if (sl != nullptr) {
-			sl->unlock_read(blocks_box);
+
+		// Locking is needed because we access `has_voxels`
+		spatial_lock.lock_read(blocks_box);
+
+		{
+			RWLockRead rlock(map_lock);
+			blocks_box.for_each_cell_zxy([&map, this](const Vector3i pos) {
+				const VoxelDataBlock *block = map.get_block(pos);
+				// TODO Might need to invoke the generator at some level for present blocks without voxels,
+				// or make sure all blocks contain voxel data
+				if (block != nullptr && block->has_voxels()) {
+					set_block(pos, block->get_voxels_shared());
+				} else {
+					set_block(pos, nullptr);
+				}
+			});
 		}
-		_spatial_lock = sl;
+
+		spatial_lock.unlock_read(blocks_box);
+
+		_spatial_lock = &spatial_lock;
 	}
 
 	inline bool has_any_block() const {