Use band-based task priorities, where higher means higher

constants/voxel_constants.h

@@ -2,6 +2,7 @@
 #define VOXEL_CONSTANTS_H
 
 #include <core/math/math_defs.h>
+#include <cstdint>
 
 namespace zylann::voxel::constants {
 
@@ -41,6 +42,18 @@ static const unsigned int DEFAULT_BLOCK_SIZE_PO2 = 4;
 
 static const float DEFAULT_COLLISION_MARGIN = 0.04f;
 
+// By default, tasks are sorted first by the value of band2.
+// When equal, they are sorted by band1, which usually depends on LOD.
+// When equal, they are sorted by band0, which depends on distance from viewer (when relevant).
+// band3 takes precedence over band2 but isn't used much for now.
+static const uint8_t TASK_PRIORITY_MESH_BAND2 = 10;
+static const uint8_t TASK_PRIORITY_GENERATE_BAND2 = 10;
+static const uint8_t TASK_PRIORITY_LOAD_BAND2 = 10;
+static const uint8_t TASK_PRIORITY_SAVE_BAND2 = 9;
+static const uint8_t TASK_PRIORITY_VIRTUAL_TEXTURES_BAND2 = 8; // After meshes
+
+static const uint8_t TASK_PRIORITY_BAND3_DEFAULT = 10;
+
 } // namespace zylann::voxel::constants
 
 #endif // VOXEL_CONSTANTS_H

engine/generate_block_task.cpp

@@ -74,9 +74,10 @@ void GenerateBlockTask::run(zylann::ThreadedTaskContext ctx) {
 	has_run = true;
 }
 
-int GenerateBlockTask::get_priority() {
+TaskPriority GenerateBlockTask::get_priority() {
 	float closest_viewer_distance_sq;
-	const int p = priority_dependency.evaluate(lod, &closest_viewer_distance_sq);
+	const TaskPriority p =
+			priority_dependency.evaluate(lod, constants::TASK_PRIORITY_GENERATE_BAND2, &closest_viewer_distance_sq);
 	too_far = drop_beyond_max_distance && closest_viewer_distance_sq > priority_dependency.drop_distance_squared;
 	return p;
 }

engine/generate_block_task.h

@@ -16,7 +16,7 @@ public:
 	~GenerateBlockTask();
 
 	void run(ThreadedTaskContext ctx) override;
-	int get_priority() override;
+	TaskPriority get_priority() override;
 	bool is_cancelled() override;
 	void apply_result() override;
 

engine/generate_distance_normalmap_task.cpp

@@ -2,6 +2,7 @@
 #include "../util/profiling.h"
 #include "voxel_engine.h"
 //#include "../util/string_funcs.h" // Debug
+#include "../constants/voxel_constants.h"
 
 namespace zylann::voxel {
 
@@ -55,9 +56,10 @@ void GenerateDistanceNormalmapTask::apply_result() {
 	callbacks.virtual_texture_output_callback(callbacks.data, o);
 }
 
-int GenerateDistanceNormalmapTask::get_priority() {
-	// TODO Give a proper priority, we just want this task to be done last relative to meshing
-	return 99999999;
+TaskPriority GenerateDistanceNormalmapTask::get_priority() {
+	// Priority by distance, but after meshes
+	TaskPriority p = priority_dependency.evaluate(lod_index, constants::TASK_PRIORITY_VIRTUAL_TEXTURES_BAND2, nullptr);
+	return p;
 }
 
 bool GenerateDistanceNormalmapTask::is_cancelled() {

engine/generate_distance_normalmap_task.h

@@ -6,6 +6,7 @@
 #include "../util/memory.h"
 #include "../util/tasks/threaded_task.h"
 #include "distance_normalmaps.h"
+#include "priority_dependency.h"
 
 namespace zylann::voxel {
 
@@ -35,10 +36,11 @@ public:
 	// Identification
 	Vector3i block_position;
 	uint32_t volume_id;
+	PriorityDependency priority_dependency;
 
 	void run(ThreadedTaskContext ctx) override;
 	void apply_result() override;
-	int get_priority() override;
+	TaskPriority get_priority() override;
 	bool is_cancelled() override;
 };
 

engine/load_all_blocks_data_task.cpp

@@ -18,8 +18,8 @@ void LoadAllBlocksDataTask::run(zylann::ThreadedTaskContext ctx) {
 	ZN_PRINT_VERBOSE(format("Loaded {} blocks for volume {}", _result.blocks.size(), volume_id));
 }
 
-int LoadAllBlocksDataTask::get_priority() {
-	return 0;
+TaskPriority LoadAllBlocksDataTask::get_priority() {
+	return TaskPriority();
 }
 
 bool LoadAllBlocksDataTask::is_cancelled() {

engine/load_all_blocks_data_task.h

@@ -11,7 +11,7 @@ namespace zylann::voxel {
 class LoadAllBlocksDataTask : public IThreadedTask {
 public:
 	void run(ThreadedTaskContext ctx) override;
-	int get_priority() override;
+	TaskPriority get_priority() override;
 	bool is_cancelled() override;
 	void apply_result() override;
 

engine/load_block_data_task.cpp

@@ -112,9 +112,10 @@ void LoadBlockDataTask::run(zylann::ThreadedTaskContext ctx) {
 	_has_run = true;
 }
 
-int LoadBlockDataTask::get_priority() {
+TaskPriority LoadBlockDataTask::get_priority() {
 	float closest_viewer_distance_sq;
-	const int p = _priority_dependency.evaluate(_lod, &closest_viewer_distance_sq);
+	const TaskPriority p =
+			_priority_dependency.evaluate(_lod, constants::TASK_PRIORITY_LOAD_BAND2, &closest_viewer_distance_sq);
 	_too_far = closest_viewer_distance_sq > _priority_dependency.drop_distance_squared;
 	return p;
 }

engine/load_block_data_task.h

@@ -17,7 +17,7 @@ public:
 	~LoadBlockDataTask();
 
 	void run(ThreadedTaskContext ctx) override;
-	int get_priority() override;
+	TaskPriority get_priority() override;
 	bool is_cancelled() override;
 	void apply_result() override;
 

engine/mesh_block_task.cpp

@@ -346,6 +346,7 @@ void MeshBlockTask::run(zylann::ThreadedTaskContext ctx) {
 		nm_task->volume_id = volume_id;
 		nm_task->virtual_textures = virtual_textures;
 		nm_task->virtual_texture_settings = virtual_texture_settings;
+		nm_task->priority_dependency = priority_dependency;
 
 		VoxelEngine::get_singleton().push_async_task(nm_task);
 	}
@@ -363,9 +364,10 @@ void MeshBlockTask::run(zylann::ThreadedTaskContext ctx) {
 	_has_run = true;
 }
 
-int MeshBlockTask::get_priority() {
+TaskPriority MeshBlockTask::get_priority() {
 	float closest_viewer_distance_sq;
-	const int p = priority_dependency.evaluate(lod_index, &closest_viewer_distance_sq);
+	const TaskPriority p =
+			priority_dependency.evaluate(lod_index, constants::TASK_PRIORITY_MESH_BAND2, &closest_viewer_distance_sq);
 	_too_far = closest_viewer_distance_sq > priority_dependency.drop_distance_squared;
 	return p;
 }

engine/mesh_block_task.h

@@ -17,7 +17,7 @@ public:
 	~MeshBlockTask();
 
 	void run(ThreadedTaskContext ctx) override;
-	int get_priority() override;
+	TaskPriority get_priority() override;
 	bool is_cancelled() override;
 	void apply_result() override;
 

engine/priority_dependency.cpp

@@ -1,10 +1,12 @@
 #include "priority_dependency.h"
 #include "../constants/voxel_constants.h"
+#include "../util/math/funcs.h"
 
 namespace zylann::voxel {
 
-int PriorityDependency::evaluate(uint8_t lod_index, float *out_closest_distance_sq) {
-	ERR_FAIL_COND_V(shared == nullptr, 0);
+TaskPriority PriorityDependency::evaluate(uint8_t lod_index, uint8_t band2_priority, float *out_closest_distance_sq) {
+	TaskPriority priority;
+	ERR_FAIL_COND_V(shared == nullptr, priority);
 
 	const std::vector<Vector3> &viewer_positions = shared->viewers;
 	const Vector3 block_position = world_position;
@@ -26,17 +28,27 @@ int PriorityDependency::evaluate(uint8_t lod_index, float *out_closest_distance_
 		*out_closest_distance_sq = closest_distance_sq;
 	}
 
-	// TODO Any way to optimize out the sqrt?
+	// TODO Any way to optimize out the sqrt? Maybe with a fast integer version?
 	// I added it because the LOD modifier was not working with squared distances,
 	// which led blocks to subdivide too much compared to their neighbors, making cracks more likely to happen
-	int priority = static_cast<int>(Math::sqrt(closest_distance_sq));
+	const int distance = static_cast<int>(Math::sqrt(closest_distance_sq));
 
 	// TODO Prioritizing LOD makes generation slower... but not prioritizing makes cracks more likely to appear...
 	// This could be fixed by allowing the volume to preemptively request blocks of the next LOD?
 	//
 	// Higher lod indexes come first to allow the octree to subdivide.
 	// Then comes distance, which is modified by how much in view the block is
-	priority += (constants::MAX_LOD - lod_index) * 10000;
+	//priority += (constants::MAX_LOD - lod_index) * 10000;
+
+	// Closer is higher priority. Decreases over distance.
+	// Scaled by LOD because we segment priority by LOD too in band 1.
+	priority.band0 = math::max(TaskPriority::BAND_MAX - (distance >> (4 + lod_index)), 0);
+	// Note: in the past, making lower LOD indices (aka closer detailed ones) have higher priority made cracks between
+	// meshes more likely to appear somehow, so for a while I had it inverted. But that priority makes sense so I
+	// changed it back. Will see later if that really causes any issue.
+	priority.band1 = constants::MAX_LOD - lod_index;
+	priority.band2 = band2_priority;
+	priority.band3 = constants::TASK_PRIORITY_BAND3_DEFAULT;
 
 	return priority;
 }

engine/priority_dependency.h

@@ -1,6 +1,7 @@
 #ifndef PRIORITY_DEPENDENCY_H
 #define PRIORITY_DEPENDENCY_H
 
+#include "../util/tasks/task_priority.h"
 #include <core/math/vector3.h>
 #include <memory>
 #include <vector>
@@ -23,12 +24,13 @@ struct PriorityDependency {
 	std::shared_ptr<ViewersData> shared;
 	// Position relative to the same space as viewers.
 	// TODO Won't update while in queue. Can it be bad?
+	// TODO May not be worth storing with double precision
 	Vector3 world_position;
 
 	// If the closest viewer is further away than this distance, the request can be cancelled as not worth it
 	float drop_distance_squared;
 
-	int evaluate(uint8_t lod_index, float *out_closest_distance_sq);
+	TaskPriority evaluate(uint8_t lod_index, uint8_t band2_priority, float *out_closest_distance_sq);
 };
 
 } // namespace zylann::voxel

engine/save_block_data_task.cpp

@@ -85,8 +85,11 @@ void SaveBlockDataTask::run(zylann::ThreadedTaskContext ctx) {
 	_has_run = true;
 }
 
-int SaveBlockDataTask::get_priority() {
-	return 0;
+TaskPriority SaveBlockDataTask::get_priority() {
+	TaskPriority p;
+	p.band2 = constants::TASK_PRIORITY_SAVE_BAND2;
+	p.band3 = constants::TASK_PRIORITY_BAND3_DEFAULT;
+	return p;
 }
 
 bool SaveBlockDataTask::is_cancelled() {

engine/save_block_data_task.h

@@ -20,7 +20,7 @@ public:
 	~SaveBlockDataTask();
 
 	void run(ThreadedTaskContext ctx) override;
-	int get_priority() override;
+	TaskPriority get_priority() override;
 	bool is_cancelled() override;
 	void apply_result() override;
 

tests/tests.cpp

@@ -2306,6 +2306,13 @@ void test_threaded_task_runner() {
 	ZYLANN_TEST_ASSERT(serial_counter->current_count == 0);
 }
 
+void test_task_priority_values() {
+	ZYLANN_TEST_ASSERT(TaskPriority(0, 0, 0, 0) < TaskPriority(1, 0, 0, 0));
+	ZYLANN_TEST_ASSERT(TaskPriority(0, 0, 0, 0) < TaskPriority(0, 0, 0, 1));
+	ZYLANN_TEST_ASSERT(TaskPriority(10, 0, 0, 0) < TaskPriority(0, 10, 0, 0));
+	ZYLANN_TEST_ASSERT(TaskPriority(10, 10, 0, 0) < TaskPriority(10, 10, 10, 0));
+}
+
 ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
 
 #define VOXEL_TEST(fname)                                                                                              \
@@ -2349,6 +2356,7 @@ void run_voxel_tests() {
 	VOXEL_TEST(test_voxel_buffer_metadata_gd);
 	VOXEL_TEST(test_voxel_mesher_cubes);
 	VOXEL_TEST(test_threaded_task_runner);
+	VOXEL_TEST(test_task_priority_values);
 
 	print_line("------------ Voxel tests end -------------");
 }

util/tasks/godot/threaded_task_gd.cpp

@@ -16,8 +16,10 @@ public:
 		ref->mark_completed();
 	}
 
-	int get_priority() override {
-		return ref->get_priority();
+	TaskPriority get_priority() override {
+		TaskPriority priority;
+		priority.whole = ref->get_priority();
+		return priority;
 	}
 
 	bool is_cancelled() override {

util/tasks/task_priority.h

@@ -0,0 +1,58 @@
+#ifndef ZN_TASK_PRIORITY_H
+#define ZN_TASK_PRIORITY_H
+
+#include <cstdint>
+
+namespace zylann {
+
+// Represents the priorirty of a task, which can be compared quickly to another.
+struct TaskPriority {
+	static const uint8_t BAND_MAX = 255;
+
+	union {
+		struct {
+			uint8_t band0; // Higher means higher priority (to state the obvious)
+			uint8_t band1; // Takes precedence over band0
+			uint8_t band2; // Takes precedence over band1
+			uint8_t band3; // Takes precedence over band2
+		};
+		uint32_t whole;
+	};
+
+	TaskPriority() : whole(0) {}
+
+	TaskPriority(uint8_t p_band0, uint8_t p_band1, uint8_t p_band2, uint8_t p_band3) :
+			band0(p_band0), band1(p_band1), band2(p_band2), band3(p_band3) {}
+
+	// Returns `true` if the left-hand priority is lower than the right-hand one.
+	// Means the right-hand task should run first.
+	inline bool operator<(const TaskPriority &other) const {
+		return whole < other.whole;
+	}
+
+	// Returns `true` if the left-hand priority is higher than the right-hand one.
+	// Means the left-hand task should run first.
+	inline bool operator>(const TaskPriority &other) const {
+		return whole > other.whole;
+	}
+
+	inline bool operator==(const TaskPriority &other) const {
+		return whole == other.whole;
+	}
+
+	static inline TaskPriority min() {
+		TaskPriority p;
+		p.whole = 0;
+		return p;
+	}
+
+	static inline TaskPriority max() {
+		TaskPriority p;
+		p.whole = 0xffffffff;
+		return p;
+	}
+};
+
+} // namespace zylann
+
+#endif // ZN_TASK_PRIORITY_H

util/tasks/threaded_task.h

@@ -1,6 +1,7 @@
 #ifndef THREADED_TASK_H
 #define THREADED_TASK_H
 
+#include "task_priority.h"
 #include <cstdint>
 
 namespace zylann {
@@ -25,9 +26,9 @@ public:
 	// Hints how soon this task will be executed after being scheduled. This is relevant when there are a lot of tasks.
 	// Lower values means higher priority.
 	// Can change between two calls. The thread pool will poll this value regularly over some time interval.
-	// TODO Should we disallow negative values? I can't think of a use for it.
-	virtual int get_priority() {
-		return 0;
+	virtual TaskPriority get_priority() {
+		// Defaulting to maximum priority as it's the most common expectation.
+		return TaskPriority::max();
 	}
 
 	// May return `true` in order for the thread pool to skip the task

util/tasks/threaded_task_runner.cpp

@@ -155,7 +155,7 @@ void ThreadedTaskRunner::thread_func(ThreadData &data) {
 				// Pick best tasks
 				for (uint32_t bi = 0; bi < _batch_count && _tasks.size() != 0; ++bi) {
 					size_t best_index = 0; // Take first by default, this is a valid index
-					int best_priority = std::numeric_limits<int>::max();
+					TaskPriority highest_priority = TaskPriority::min();
 					bool picked_task = false;
 
 					// Find best task to pick
@@ -182,8 +182,8 @@ void ThreadedTaskRunner::thread_func(ThreadData &data) {
 
 						// Pick item if it has better priority.
 						// If the item is serial, there must not be a serial task already running.
-						if (item.cached_priority < best_priority && !(item.is_serial && _is_serial_task_running)) {
-							best_priority = item.cached_priority;
+						if (item.cached_priority > highest_priority && !(item.is_serial && _is_serial_task_running)) {
+							highest_priority = item.cached_priority;
 							// This index should remain valid even if some tasks are removed because the "remove and
 							// swap back" technique only affects items coming after
 							best_index = i;

util/tasks/threaded_task_runner.h

@@ -83,7 +83,7 @@ public:
 private:
 	struct TaskItem {
 		IThreadedTask *task = nullptr;
-		int cached_priority = 99999;
+		TaskPriority cached_priority;
 		bool is_serial = false;
 		uint64_t last_priority_update_time_ms = 0;
 	};