diff --git a/constants/voxel_constants.h b/constants/voxel_constants.h index 73125289..b30548d9 100644 --- a/constants/voxel_constants.h +++ b/constants/voxel_constants.h @@ -2,6 +2,7 @@ #define VOXEL_CONSTANTS_H #include +#include 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 diff --git a/engine/generate_block_task.cpp b/engine/generate_block_task.cpp index cc17756e..7d6b3adb 100644 --- a/engine/generate_block_task.cpp +++ b/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; } diff --git a/engine/generate_block_task.h b/engine/generate_block_task.h index 8b934408..13afc0c8 100644 --- a/engine/generate_block_task.h +++ b/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; diff --git a/engine/generate_distance_normalmap_task.cpp b/engine/generate_distance_normalmap_task.cpp index 88b3e456..76cf6711 100644 --- a/engine/generate_distance_normalmap_task.cpp +++ b/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() { diff --git a/engine/generate_distance_normalmap_task.h b/engine/generate_distance_normalmap_task.h index 267c2980..e71ce624 100644 --- a/engine/generate_distance_normalmap_task.h +++ b/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; }; diff --git a/engine/load_all_blocks_data_task.cpp b/engine/load_all_blocks_data_task.cpp index f3b7c9ee..baa4d4b6 100644 --- a/engine/load_all_blocks_data_task.cpp +++ b/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() { diff --git a/engine/load_all_blocks_data_task.h b/engine/load_all_blocks_data_task.h index 3c9a9775..f8023fdc 100644 --- a/engine/load_all_blocks_data_task.h +++ b/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; diff --git a/engine/load_block_data_task.cpp b/engine/load_block_data_task.cpp index 86bb7d69..64dd8be7 100644 --- a/engine/load_block_data_task.cpp +++ b/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; } diff --git a/engine/load_block_data_task.h b/engine/load_block_data_task.h index 442355fe..528805ba 100644 --- a/engine/load_block_data_task.h +++ b/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; diff --git a/engine/mesh_block_task.cpp b/engine/mesh_block_task.cpp index bcfe9ea4..b2753890 100644 --- a/engine/mesh_block_task.cpp +++ b/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; } diff --git a/engine/mesh_block_task.h b/engine/mesh_block_task.h index 917719f5..c68478c5 100644 --- a/engine/mesh_block_task.h +++ b/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; diff --git a/engine/priority_dependency.cpp b/engine/priority_dependency.cpp index cbcb400b..4187d29f 100644 --- a/engine/priority_dependency.cpp +++ b/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 &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(Math::sqrt(closest_distance_sq)); + const int distance = static_cast(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; } diff --git a/engine/priority_dependency.h b/engine/priority_dependency.h index ae7b6d80..af7f5ba5 100644 --- a/engine/priority_dependency.h +++ b/engine/priority_dependency.h @@ -1,6 +1,7 @@ #ifndef PRIORITY_DEPENDENCY_H #define PRIORITY_DEPENDENCY_H +#include "../util/tasks/task_priority.h" #include #include #include @@ -23,12 +24,13 @@ struct PriorityDependency { std::shared_ptr 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 diff --git a/engine/save_block_data_task.cpp b/engine/save_block_data_task.cpp index 6a6d3da8..7c7c9c07 100644 --- a/engine/save_block_data_task.cpp +++ b/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() { diff --git a/engine/save_block_data_task.h b/engine/save_block_data_task.h index ea997134..fd9a6b90 100644 --- a/engine/save_block_data_task.h +++ b/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; diff --git a/tests/tests.cpp b/tests/tests.cpp index d16e1a74..0d3f065f 100644 --- a/tests/tests.cpp +++ b/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 -------------"); } diff --git a/util/tasks/godot/threaded_task_gd.cpp b/util/tasks/godot/threaded_task_gd.cpp index 9374891b..781bec48 100644 --- a/util/tasks/godot/threaded_task_gd.cpp +++ b/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 { diff --git a/util/tasks/task_priority.h b/util/tasks/task_priority.h new file mode 100644 index 00000000..d30e85d5 --- /dev/null +++ b/util/tasks/task_priority.h @@ -0,0 +1,58 @@ +#ifndef ZN_TASK_PRIORITY_H +#define ZN_TASK_PRIORITY_H + +#include + +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 diff --git a/util/tasks/threaded_task.h b/util/tasks/threaded_task.h index 783c7490..16dca769 100644 --- a/util/tasks/threaded_task.h +++ b/util/tasks/threaded_task.h @@ -1,6 +1,7 @@ #ifndef THREADED_TASK_H #define THREADED_TASK_H +#include "task_priority.h" #include 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 diff --git a/util/tasks/threaded_task_runner.cpp b/util/tasks/threaded_task_runner.cpp index 3da217f6..84faec14 100644 --- a/util/tasks/threaded_task_runner.cpp +++ b/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::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; diff --git a/util/tasks/threaded_task_runner.h b/util/tasks/threaded_task_runner.h index 35b4388f..68a856c2 100644 --- a/util/tasks/threaded_task_runner.h +++ b/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; };