Fix late mesh updates were applied to blocks that no longer need them.

For example, blocks can still be present if they need only collision,
but when a visual mesh update arrives late, it needs to be ignored,
instead of just be applied.
Moving viewer A through viewer B's region very fast could make this happen.

Author: Zylann

terrain/variable_lod/voxel_lod_terrain.cpp

@@ -1366,6 +1366,11 @@ void VoxelLodTerrain::apply_main_thread_update_tasks() {
 			}
 			block->drop_visuals();
 			remove_shader_material_from_block(*block, _shader_material_pool);
+			// Also update the state in the threaded representation
+			auto it = lod.mesh_map_state.map.find(bpos);
+			if (it != lod.mesh_map_state.map.end()) {
+				it->second.visual_loaded = false;
+			}
 		}
 		lod.mesh_blocks_to_drop_visual.clear();
 
@@ -1678,6 +1683,8 @@ void VoxelLodTerrain::apply_mesh_update(VoxelEngine::BlockMeshOutput &ob) {
 	bool collision_active;
 	bool first_collision_load = false;
 	bool first_visual_load = false;
+	bool visual_expected = false;
+	bool collision_expected = false;
 	{
 		VoxelLodTerrainUpdateData::Lod &lod = update_data.state.lods[ob.lod];
 		RWLockRead rlock(lod.mesh_map_state.map_lock);
@@ -1707,21 +1714,23 @@ void VoxelLodTerrain::apply_mesh_update(VoxelEngine::BlockMeshOutput &ob) {
 		visual_active = mesh_block_state.visual_active;
 		collision_active = mesh_block_state.collision_active;
 
-		if (ob.visual_was_required) {
-			if (!mesh_block_state.visual_loaded) {
-				// First mesh load (note, no mesh being present counts as load too. Before that we would not know)
-				mesh_block_state.visual_loaded = true;
-				first_visual_load = true;
-			}
+		visual_expected = mesh_block_state.mesh_viewers.get() > 0;
+		collision_expected = mesh_block_state.collision_viewers.get() > 0;
+
+		if (visual_expected && ob.visual_was_required) {
+			// Mark visuals loaded for the streaming system to subdivide LODs.
+			// First mesh load? (note, no mesh being present counts as load too. Before that we would not know)
+			first_visual_load = (mesh_block_state.visual_loaded.exchange(true) == false);
 		}
-		if (!mesh_block_state.collision_loaded) {
-			// First mesh load (note, no mesh being present counts as load too. Before that we would not know)
-			mesh_block_state.collision_loaded = true;
-			first_collision_load = true;
+		if (collision_expected) {
+			// Mark collisions loaded for the streaming system to subdivide LODs.
+			// First mesh load? (note, no mesh being present counts as load too. Before that we would not know)
+			first_collision_load = (mesh_block_state.collision_loaded.exchange(true) == false);
 		}
 	}
 	if ((first_visual_load || first_collision_load) &&
 			_update_data->settings.streaming_system == VoxelLodTerrainUpdateData::STREAMING_SYSTEM_CLIPBOX) {
+		// Notify streaming system so it can subdivide LODs as they load
 		VoxelLodTerrainUpdateData::ClipboxStreamingState &cs = _update_data->state.clipbox_streaming;
 		MutexLock mlock(cs.loaded_mesh_blocks_mutex);
 		cs.loaded_mesh_blocks.push_back(VoxelLodTerrainUpdateData::LoadedMeshBlockEvent{
@@ -1737,7 +1746,7 @@ void VoxelLodTerrain::apply_mesh_update(VoxelEngine::BlockMeshOutput &ob) {
 	VoxelMesher::Output &mesh_data = ob.surfaces;
 
 	Ref<ArrayMesh> mesh;
-	if (ob.visual_was_required) {
+	if (ob.visual_was_required && visual_expected) {
 		if (ob.has_mesh_resource) {
 			// The mesh was already built as part of the threaded task
 			mesh = ob.mesh;
@@ -1797,7 +1806,7 @@ void VoxelLodTerrain::apply_mesh_update(VoxelEngine::BlockMeshOutput &ob) {
 	}
 #endif
 
-	if (ob.visual_was_required) {
+	if (ob.visual_was_required && visual_expected) {
 		// We consider a block having a "rendering" mesh as having loaded visuals.
 		if (!block->has_mesh()) {
 			// Setup visuals
@@ -1864,7 +1873,7 @@ void VoxelLodTerrain::apply_mesh_update(VoxelEngine::BlockMeshOutput &ob) {
 		has_collision = ob.lod < _collision_lod_count;
 	}
 
-	if (has_collision) {
+	if (has_collision && collision_expected) {
 		const uint64_t now = get_ticks_msec();
 
 		if (_collision_update_delay == 0 ||
@@ -1889,9 +1898,11 @@ void VoxelLodTerrain::apply_mesh_update(VoxelEngine::BlockMeshOutput &ob) {
 		}
 	}
 
+	// This is done regardless in case a MeshInstance or collision body is created, because it will then set its
+	// position
 	block->set_parent_transform(get_global_transform());
 
-	if (ob.detail_textures != nullptr) {
+	if (ob.detail_textures != nullptr && visual_expected) {
 		if (ob.detail_textures->valid) {
 			apply_detail_texture_update_to_block(*block, *ob.detail_textures, ob.lod);
 		} else {

terrain/variable_lod/voxel_lod_terrain_update_clipbox_streaming.cpp

@@ -783,6 +783,7 @@ void unview_mesh_box(const Box3i out_of_range_box, VoxelLodTerrainUpdateData::Lo
 			} else if (mesh_block.mesh_viewers.get() == 0) {
 				// Unload graphics to save memory
 				lod.mesh_blocks_to_drop_visual.push_back(bpos);
+				// Note, `visuals_loaded` will remain true until they are actually unloaded.
 			}
 		}
 	});

terrain/variable_lod/voxel_lod_terrain_update_data.h

@@ -6,7 +6,7 @@
 #include "../../generators/voxel_generator.h"
 #include "../../streams/voxel_stream.h"
 #include "../../util/containers/fixed_array.h"
-#include "../../util/ref_count.h"
+#include "../../util/safe_ref_count.h"
 #include "../../util/tasks/cancellation_token.h"
 #include "../voxel_mesh_map.h"
 #include "lod_octree.h"
@@ -102,27 +102,32 @@ struct VoxelLodTerrainUpdateData {
 		std::atomic<DetailTextureState> detail_texture_state;
 
 		// Refcount here to support multiple viewers, we can't do it on the main thread's mesh map since the
-		// streaming logic is in the update task
-		RefCount mesh_viewers;
-		RefCount collision_viewers;
+		// streaming logic is in the update task.
+		// TODO Optimize: this could almost not need to be atomic.
+		// This is an atomic refcount only because the main thread needs to read it when receiving mesh updates. It
+		// could be made non-atomic if mesh updates were handled in the threaded update, but that has a more
+		// implications than making this atomic for now.
+		SafeRefCount mesh_viewers;
+		SafeRefCount collision_viewers;
 
 		// Cancelled when this mesh block is removed, so if tasks are still queued to do work for that block, they
 		// will be cancelled
 		TaskCancellationToken cancellation_token;
 
 		// Index within the list of meshes to update during one update of the terrain. Used to avoid putting the same
-		// mesh more than once in the list, while allowing to change options after it's been added to the list. Not
-		// relevant outside of the update.
+		// mesh more than once in the list, while allowing to change options after it's been added to the list. Should
+		// reset to -1 after each update (since the list is consumed)
 		int update_list_index;
 
 		uint8_t transition_mask;
 		bool visual_active;
 		bool collision_active;
 
 		// Tells whether the first meshing was done since this block was added.
-		// Written by the main thread only, since the main thread receives mesh updates
-		bool visual_loaded;
-		bool collision_loaded;
+		// Written by the main thread only, when it receives mesh updates or when it unloads resources.
+		// Read by threaded update to decide when to subdivide LODs.
+		std::atomic_bool visual_loaded;
+		std::atomic_bool collision_loaded;
 
 		// bool pending_update_has_visuals;
 		// bool pending_update_has_collision;
@@ -183,6 +188,7 @@ struct VoxelLodTerrainUpdateData {
 		int last_view_distance_mesh_blocks = 0;
 
 		// Deferred outputs to main thread. Should only be read once the task is finished, so no need to lock.
+		// TODO These output actions are not particularly serialized, that might cause issues (havent so far).
 		std::vector<Vector3i> mesh_blocks_to_unload;
 		std::vector<TransitionUpdate> mesh_blocks_to_update_transitions;
 		std::vector<Vector3i> mesh_blocks_to_activate_visuals;

util/safe_ref_count.h

@@ -0,0 +1,36 @@
+#ifndef ZN_SAFE_REF_COUNT_H
+#define ZN_SAFE_REF_COUNT_H
+
+#include "errors.h"
+#include <atomic>
+
+namespace zylann {
+
+// Thread-safe reference counter.
+class SafeRefCount {
+public:
+	SafeRefCount() {}
+	SafeRefCount(int initial_count) : _count(initial_count) {}
+
+	inline int add() {
+		return _count.fetch_add(1, std::memory_order_acq_rel);
+	}
+
+	inline int remove() {
+		const int previous_count = _count.fetch_sub(1, std::memory_order_acq_rel);
+		ZN_ASSERT_RETURN_V_MSG(
+				previous_count != 0, previous_count, "Trying to decrease refcount when it's already zero");
+		return previous_count;
+	}
+
+	inline int get() const {
+		return _count;
+	}
+
+private:
+	std::atomic_int32_t _count = { 0 };
+};
+
+} // namespace zylann
+
+#endif // ZN_SAFE_REF_COUNT_H