chore: clarification

modules/terrain/terrain.cpp

@@ -10,6 +10,29 @@ void Terrain::_bind_methods() {
 	BIND_HPROPERTY(Variant::ARRAY, terrain_meshes, PROPERTY_HINT_ARRAY_TYPE, vformat("%s/%s:TerrainMeshChunk", Variant::OBJECT, PROPERTY_HINT_NODE_TYPE), PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_EDITOR | PROPERTY_USAGE_READ_ONLY);
 }
 
+void Terrain::stop_threads() {
+	this->workload_lock.lock();
+	this->threads_stop = true;
+	this->workload_lock.unlock();
+	for (Thread &thread : this->threads) {
+		if (thread.is_started()) {
+			thread.wait_to_finish();
+		}
+	}
+}
+
+void Terrain::start_threads() {
+	this->workload_lock.lock();
+	print_line(vformat("Starting threads; workload: %d", this->workload.size()));
+	this->threads_stop = false;
+	for (Thread &thread : this->threads) {
+		if (!thread.is_started()) {
+			thread.start(Terrain::generate_meshes_thread, this);
+		}
+	}
+	this->workload_lock.unlock(); // don't let the threads proceed until all are started
+}
+
 void Terrain::child_order_changed() {
 	this->modifiers.clear();
 	for (Variant var : get_children()) {
@@ -21,7 +44,7 @@ void Terrain::child_order_changed() {
 }
 
 void Terrain::update_meshes() {
-	size_t num{ 1 };
+	size_t num{ max_mesh_assignments_per_frame };
 	this->dirty_meshes_lock.lock();
 	num = num > this->dirty_meshes.size() ? this->dirty_meshes.size() : num;
 	this->dirty_meshes_lock.unlock();
@@ -35,60 +58,49 @@ void Terrain::update_meshes() {
 }
 
 void Terrain::update_threads() {
-	this->workload_lock.lock();
 	if (this->workload.is_empty()) {
-		this->threads_stop = true;
-		this->workload_lock.unlock();
-		for (Thread &thread : this->threads) {
-			if (thread.is_started()) {
-				thread.wait_to_finish();
-			}
-		}
+		stop_threads();
 	} else {
-		print_line(vformat("Starting threads; workload: %d", this->workload.size()));
-		this->threads_stop = false;
-		for (Thread &thread : this->threads) {
-			if (!thread.is_started()) {
-				thread.start(Terrain::generate_meshes_thread, this);
-			}
-		}
-		this->workload_lock.unlock();
+		start_threads();
 	}
 }
 
 void Terrain::update_process() {
+	// check if there is any tasks going on that would require processing each frame
+	// any running threads?
 	for (Thread &thread : this->threads) {
 		if (thread.is_started()) {
 			return;
 		}
 	}
+	// dirty meshes?
 	this->dirty_meshes_lock.lock();
 	bool workload_empty{ this->dirty_meshes.is_empty() };
 	this->dirty_meshes_lock.unlock();
 	if (!workload_empty) {
 		return;
 	}
+	// queued mesh generation tasks?
 	this->workload_lock.lock();
 	workload_empty = this->workload.is_empty();
 	this->workload_lock.unlock();
 	if (!workload_empty) {
 		return;
 	}
+	// stop processing each frame
 	print_line("Terrain processing stopped");
 	set_process(false);
 }
 
 void Terrain::synchronous_generate_terrain() {
-	print_line("Force-regenerating entire terrain in one go.");
+	print_line("Blocking regenerate terrain");
 	this->workload_lock.lock();
 	this->threads_stop = false;
+	// queue all meshes
 	this->workload.clear();
 	this->workload.append_array(this->meshes);
-	for (Thread &thread : this->threads) {
-		if (!thread.is_started()) {
-			thread.start(&Terrain::generate_meshes_thread, this);
-		}
-	}
+	start_threads();
+	// wait for workload to empty out
 	do {
 		this->workload_lock.unlock();
 		Thread::yield();
@@ -96,9 +108,7 @@ void Terrain::synchronous_generate_terrain() {
 	} while (!this->workload.is_empty());
 	this->threads_stop = true;
 	this->workload_lock.unlock();
-	for (Thread &thread : this->threads) {
-		thread.wait_to_finish();
-	}
+	stop_threads();
 	for (TerrainChunkMesh *mesh : this->dirty_meshes) {
 		mesh->apply_new_mesh();
 	}

modules/terrain/terrain.h

@@ -12,6 +12,8 @@ class TerrainModifier;
 class Terrain : public Node {
 	GDCLASS(Terrain, Node);
 	static void _bind_methods();
+	void stop_threads();
+	void start_threads();
 	void child_order_changed();
 	void update_meshes();
 	void update_threads();
@@ -44,6 +46,7 @@ private:
 	size_t side_length{ 200 };
 	size_t chunk_size{ 50 };
 	size_t detail{ 1 };
+	size_t max_mesh_assignments_per_frame{ 1 };
 
 public:
 	void set_mesh_material(Ref<Material> material);

modules/terrain/terrain_chunk.cpp

@@ -10,27 +10,33 @@
 #include "terrain/terrain.h"
 
 void TerrainChunkMesh::_bind_methods() {
+	// bind properties so they will be saved to the scene file, don't allow direct modification
 	BIND_HPROPERTY(Variant::OBJECT, shape, PROPERTY_HINT_RESOURCE_TYPE, "HeightMapShape3D", PROPERTY_USAGE_READ_ONLY | PROPERTY_USAGE_EDITOR | PROPERTY_USAGE_STORAGE);
 	BIND_HPROPERTY(Variant::INT, size, PROPERTY_HINT_NONE, "", PROPERTY_USAGE_READ_ONLY | PROPERTY_USAGE_EDITOR | PROPERTY_USAGE_STORAGE);
 }
 
 void TerrainChunkMesh::ready() {
+	// initialise thread-safe position buffer,
+	// we're assuming that chunks only get generated on the main thread
 	this->position_buffer = get_global_position();
 	float const sizef{ (float)get_size() };
 	this->bounds.position = { this->position_buffer.x - sizef / 2.f, this->position_buffer.z - sizef / 2.f };
 	this->bounds.size = { sizef, sizef };
-
+	// add static body
 	add_child(this->body = memnew(StaticBody3D));
 	ERR_FAIL_COND_EDMSG(this->body == nullptr, "Failed to instantiate StaticBody3D");
+	// initialise collision shape
 	this->body->add_child(this->collider = memnew(CollisionShape3D));
 	this->body->set_owner(this);
 	ERR_FAIL_COND_EDMSG(this->collider == nullptr, "Failed to instantiate CollisionShape3D");
 	this->collider->set_owner(this);
+	// either instantiate the shape as cached, or initialise it
 	if (this->shape.is_null()) {
 		this->shape = memnew(HeightMapShape3D);
 		this->shape->set_map_depth(heightmap_side_length());
 		this->shape->set_map_width(heightmap_side_length());
 	} else {
+		// if cached, initialise thread-safe buffer
 		this->heightmap.append_array(this->shape->get_map_data());
 	}
 	ERR_FAIL_COND_EDMSG(!this->shape.is_valid(), "Failed to instantiate HeightMapShape3D");
@@ -41,6 +47,7 @@ void TerrainChunkMesh::generate_vertices() {
 	ERR_FAIL_COND_EDMSG(this->terrain == nullptr, "TerrainChunkMesh::generate_vertices: no terrain assigned");
 	ERR_FAIL_COND_EDMSG(this->size <= 0.f, "TerrainChunkMesh::generate_vertices: size <= 0");
 	ERR_FAIL_COND_EDMSG(points_per_side() <= 0, "TerrainChunkMesh::generate_vertices: points per side <= 0");
+	// generate mesh surface vertices
 	float const half_extent{ (float)this->size / 2.f };
 	float const point_distance{ (float)this->size / ((float)points_per_side() - 1) };
 	Vector3 origin{ this->position_buffer - Vector3{ half_extent, 0, half_extent } };
@@ -52,6 +59,7 @@ void TerrainChunkMesh::generate_vertices() {
 			this->surface->add_vertex({ coordinate.x - this->position_buffer.x, height, coordinate.y - this->position_buffer.z });
 		}
 	}
+	// generate heightmap surface points
 	this->heightmap.resize_initialized(heightmap_side_length() * heightmap_side_length());
 	for (size_t x{ 0 }; x < heightmap_side_length(); x++) {
 		for (size_t y{ 0 }; y < heightmap_side_length(); y++) {
@@ -67,6 +75,7 @@ void TerrainChunkMesh::generate_faces() {
 	size_t const faces_per_side{ points_per_side() - 1 };
 	for (size_t x{ 0 }; x < faces_per_side; ++x) {
 		for (size_t y{ 0 }; y < faces_per_side; ++y) {
+			// generate shortest diagonal edge
 			size_t const tl{ x + y * points_per_side() };
 			float tl_br{ verts[tl].vertex.distance_to(verts[tl + points_per_side() + 1].vertex) };
 			float tr_bl{ verts[tl + 1].vertex.distance_to(verts[tl + points_per_side()].vertex) };
@@ -112,6 +121,9 @@ void TerrainChunkMesh::apply_new_mesh() {
 	this->lock.unlock();
 }
 
+// NOTE: this _will not_ be called on the main thread.
+// - Don't modify scene tree
+// - Don't rely on scene tree data
 void TerrainChunkMesh::update_mesh() {
 	ERR_FAIL_COND_EDMSG(this->size <= 0.f, "TerrainChunkMesh::generate: size <= 0");
 	ERR_FAIL_COND_EDMSG(points_per_side() <= 0, "TerrainChunkMesh::generate: points per side <= 0");