fix: combine terrain+mesh on worker to eliminate pipeline holes

Author: HTRMC

src/client/world/ChunkLoadThread.zig

@@ -21,6 +21,7 @@ const ChunkPos = shared.ChunkPos;
 const ChunkPosContext = shared.ChunkPosContext;
 
 const SpiralIterator = @import("ChunkManager.zig").SpiralIterator;
+const ChunkStorage = @import("ChunkStorage.zig").ChunkStorage;
 
 const tp = @import("ThreadPool.zig");
 const ThreadPool = tp.ThreadPool;
@@ -53,6 +54,9 @@ pub const ChunkLoadThread = struct {
     // Direct pool submission (thread-safe via mutex)
     pool: *ThreadPool,
 
+    // Read-only reference to chunk storage (uses getAtomic for safe cross-thread reads)
+    storage: *ChunkStorage,
+
     // Backpressure: tracks terrain tasks submitted but not yet completed
     // Incremented by this thread, decremented by worker threads after terrain generation
     in_flight_terrain: *std.atomic.Value(u32),
@@ -85,6 +89,7 @@ pub const ChunkLoadThread = struct {
         io: Io,
         pool: *ThreadPool,
         in_flight_terrain: *std.atomic.Value(u32),
+        storage: *ChunkStorage,
         view_distance: u32,
         vertical_view_distance: u32,
         unload_distance: u32,
@@ -97,6 +102,7 @@ pub const ChunkLoadThread = struct {
             .io = io,
             .running = std.atomic.Value(bool).init(false),
             .pool = pool,
+            .storage = storage,
             .in_flight_terrain = in_flight_terrain,
             .unload_requests = unload_requests,
             .player_chunk_x = std.atomic.Value(i32).init(0),
@@ -198,18 +204,14 @@ pub const ChunkLoadThread = struct {
             // Check if player position changed
             if (player.gen != self.last_gen) {
                 self.last_gen = player.gen;
-
-                const pos_changed = !player.pos.eql(self.last_player);
                 self.last_player = player.pos;
 
-                if (pos_changed) {
-                    // Reset spiral to new center
-                    self.spiral.reset(self.view_distance, self.vertical_view_distance);
-                    self.stalled_pos = null; // Stalled pos from old center is invalid
-
-                    // Scan managed set for positions outside unload range
-                    self.scanForUnloads(player.pos);
-                }
+                // Always reset spiral and scan on any gen change.
+                // Even if pos appears the same (round trip A→B→A), intermediate moves
+                // may have caused terrain results to be discarded or chunks unloaded.
+                self.spiral.reset(self.view_distance, self.vertical_view_distance);
+                self.stalled_pos = null;
+                self.scanForUnloads(player.pos);
             }
 
             // Continue spiral iteration — submit terrain tasks directly to pool
@@ -293,9 +295,13 @@ pub const ChunkLoadThread = struct {
                 .section_y = player_pos.section_y + offset.dy,
             };
 
-            // Skip if already managed
             const gop = self.managed.getOrPut(pos) catch continue;
-            if (gop.found_existing) continue;
+            if (gop.found_existing) {
+                // Validate chunk actually exists in storage (terrain may have been
+                // discarded as stale, or push to completed_terrain may have failed).
+                // If chunk is gone, fall through to re-submit terrain.
+                if (self.storage.getAtomic(pos) != null) continue;
+            }
 
             // Verify within view distance (spiral overshoots at corners)
             if (!pos.isWithinDistance(player_pos, self.view_distance)) {

src/client/world/ChunkManager.zig

@@ -66,6 +66,9 @@ const MeshSchedulerThread = mesh_scheduler_thread.MeshSchedulerThread;
 pub const TerrainResult = struct {
     pos: ChunkPos,
     chunk: Chunk,
+    /// Worker also generated and pushed an initial mesh to completed_queue.
+    /// Main thread should set state .meshing (not .generated) and skip scheduler.
+    mesh_submitted: bool = false,
 };
 
 pub const ChunkConfig = struct {
@@ -252,7 +255,9 @@ pub const ChunkManager = struct {
     ) !Self {
         logger.info("Initializing ChunkManager with view distance {}", .{config.view_distance});
 
-        const storage = try ChunkStorage.init(allocator, config.unload_distance, config.vertical_view_distance);
+        // Vertical ring buffer must be sized for the actual unload threshold (vert + 2 hysteresis),
+        // not just view distance. Otherwise positions at ±(vert+2) collide via floorMod.
+        const storage = try ChunkStorage.init(allocator, config.unload_distance, config.vertical_view_distance + 2);
 
         const self = Self{
             .allocator = allocator,
@@ -370,9 +375,6 @@ pub const ChunkManager = struct {
                 rs.getTransferFamily(), // Use transfer queue family for upload command pool
                 buf_mgr,
                 &self.completed_queue,
-                self.config.view_distance,
-                self.config.vertical_view_distance,
-                self.config.unload_distance,
                 .{}, // Use default config (0.5ms budget)
             );
             try ut.start();
@@ -411,6 +413,7 @@ pub const ChunkManager = struct {
                 self.io,
                 &self.pool,
                 &self.in_flight_terrain,
+                &self.chunk_storage,
                 self.config.view_distance,
                 self.config.vertical_view_distance,
                 self.config.unload_distance,
@@ -543,11 +546,6 @@ pub const ChunkManager = struct {
         // Ensures closest chunks are always processed first, even as player moves
         self.pool.updateCameraPos(new_chunk.x, new_chunk.z, new_chunk.section_y);
 
-        // Update upload thread for staleness checks
-        if (self.upload_thread) |ut| {
-            ut.updatePlayerPos(new_chunk.x, new_chunk.z, new_chunk.section_y);
-        }
-
         // Notify background load thread of new player position (C2ME-style)
         if (self.load_thread) |lt| {
             lt.updatePlayerPos(new_chunk.x, new_chunk.z, new_chunk.section_y);
@@ -570,25 +568,24 @@ pub const ChunkManager = struct {
         const ms = self.mesh_scheduler orelse return;
 
         var processed: u32 = 0;
-        var discarded: u32 = 0;
         while (processed < MAX_TERRAIN_PER_TICK) {
             const result = self.completed_terrain.tryPop() orelse break;
 
-            // Discard stale terrain results for positions outside unload range.
-            // This prevents orphaned chunks: if the load thread already removed this position
-            // from its managed set (via scanForUnloads), creating it here would leave it
-            // with no owner — never unloaded, never re-loaded.
+            // Discard stale terrain results far from player to prevent ring buffer collisions.
+            // Without this, put(stale_pos) can evict in-range chunks at the same ring buffer slot
+            // (two positions collide when they differ by horizontal_size=2*unload_distance+1).
             {
                 const horizontally_distant = !result.pos.isWithinDistance(self.player_chunk, self.config.unload_distance);
                 const dy = @abs(result.pos.section_y - self.player_chunk.section_y);
                 const vertically_distant = dy > self.config.vertical_view_distance + 2;
                 if (horizontally_distant or vertically_distant) {
-                    discarded += 1;
+                    processed += 1;
                     continue;
                 }
             }
 
             // If chunk already exists in storage (e.g. rapid unload/reload), update terrain data
+            // Always use scheduler for existing chunks (worker mesh gen would conflict)
             if (self.chunk_storage.get(result.pos)) |render_chunk_ptr| {
                 render_chunk_ptr.chunk = result.chunk;
                 render_chunk_ptr.setState(.generated);
@@ -604,7 +601,15 @@ pub const ChunkManager = struct {
             const render_chunk_ptr = self.allocator.create(RenderChunk) catch continue;
             render_chunk_ptr.* = RenderChunk.init(self.allocator, result.pos);
             render_chunk_ptr.chunk = result.chunk;
-            render_chunk_ptr.setState(.generated);
+
+            // If the worker already generated + pushed a mesh, set state to .meshing
+            // so processReadyUploads can apply it directly (gen 0 matches).
+            // Otherwise fall back to scheduler for mesh generation.
+            if (result.mesh_submitted) {
+                render_chunk_ptr.setState(.meshing);
+            } else {
+                render_chunk_ptr.setState(.generated);
+            }
 
             const previous = self.chunk_storage.put(result.pos, render_chunk_ptr) catch {
                 render_chunk_ptr.deinit();
@@ -632,14 +637,14 @@ pub const ChunkManager = struct {
                 self.allocator.destroy(old_chunk);
             }
 
-            ms.queueMesh(.{ .pos = result.pos, .is_remesh = false });
+            // If worker already submitted mesh, skip scheduler — mesh is in flight
+            if (!result.mesh_submitted) {
+                ms.queueMesh(.{ .pos = result.pos, .is_remesh = false });
+            }
             // Remesh neighbors that were meshed without this chunk's data
             self.queueNeighborRemeshes(result.pos);
             processed += 1;
         }
-        if (discarded > 0) {
-            profiler.plotInt("TerrainDiscarded", @intCast(discarded));
-        }
         zone.setValue(processed);
     }
 
@@ -772,11 +777,19 @@ pub const ChunkManager = struct {
                 continue;
             }
 
-            // Handle invalid/empty results (empty meshes)
             if (!result.valid) {
-                render_chunk_ptr.setState(.ready);
-                if (render_chunk_ptr.needs_remesh.swap(false, .acq_rel)) {
-                    self.queueChunkRemesh(result.pos);
+                if (result.is_empty) {
+                    // Empty mesh (no geometry) — chunk goes to .ready with no visible mesh
+                    render_chunk_ptr.setState(.ready);
+                    if (render_chunk_ptr.needs_remesh.swap(false, .acq_rel)) {
+                        self.queueChunkRemesh(result.pos);
+                    }
+                } else {
+                    // Upload failure (allocation/staging/validation) — retry mesh
+                    render_chunk_ptr.setState(.generated);
+                    if (self.mesh_scheduler) |ms| {
+                        ms.queueMesh(.{ .pos = result.pos, .is_remesh = false });
+                    }
                 }
                 continue;
             }
@@ -927,6 +940,15 @@ pub const ChunkManager = struct {
 
         while (unloaded < MAX_UNLOADS_PER_TICK) {
             const pos = lt.unload_requests.tryPop() orelse break;
+
+            // Validate: skip stale unload requests for chunks now within range.
+            // The load thread queues unloads asynchronously — by the time we drain,
+            // the player may have moved back, making this chunk in-range again.
+            const horizontally_close = pos.isWithinDistance(self.player_chunk, self.config.unload_distance);
+            const dy = @abs(pos.section_y - self.player_chunk.section_y);
+            const vertically_close = dy <= self.config.vertical_view_distance + 2;
+            if (horizontally_close and vertically_close) continue;
+
             self.unloadChunk(pos);
             unloaded += 1;
         }
@@ -1004,9 +1026,11 @@ pub const ChunkManager = struct {
         const state = neighbor_chunk.getState();
         if (state == .ready) {
             self.queueChunkRemesh(neighbor_pos);
-        } else if (state == .meshing) {
+        } else if (state == .meshing or state == .dirty) {
             // Don't cancel in-flight mesh (causes livelock in ReleaseFast).
             // Flag for remesh after current mesh completes.
+            // Also covers .dirty: the pending remesh may run before this neighbor
+            // is visible to the worker, so needs_remesh ensures a follow-up.
             neighbor_chunk.needs_remesh.store(true, .release);
         }
     }
@@ -1115,13 +1139,34 @@ pub const ChunkManager = struct {
             chunk = Chunk.generateTestChunk();
         }
 
+        // Combined terrain+mesh: generate initial mesh immediately to eliminate
+        // the round-trip through main thread → scheduler → worker pool.
+        // Saves ~2 frames of pipeline latency per chunk during initial loading.
+        var initial_mesh_result: ?CompletedMesh = null;
+        if (self.generateInitialMesh(ctx, &chunk, task.chunk_pos)) |mesh| {
+            initial_mesh_result = mesh;
+        }
+
+        // ORDERING: push terrain BEFORE mesh. The main thread processes completed_terrain
+        // before processReadyUploads in tick(). If we pushed mesh first, the upload thread
+        // could process it before the main thread creates the RenderChunk from terrain,
+        // causing the mesh result to be discarded (chunk not in storage).
         self.completed_terrain.push(TerrainResult{
             .pos = task.chunk_pos,
             .chunk = chunk,
+            .mesh_submitted = initial_mesh_result != null,
         }) catch |err| {
             logger.warn("Failed to push completed terrain: {}", .{err});
         };
 
+        // Push mesh after terrain so RenderChunk always exists when mesh result arrives
+        if (initial_mesh_result) |mesh| {
+            self.completed_queue.push(mesh) catch {
+                var m = mesh;
+                m.deinit();
+            };
+        }
+
         // Release backpressure so load thread can submit more tasks
         _ = self.in_flight_terrain.fetchSub(1, .release);
     }
@@ -1226,6 +1271,68 @@ pub const ChunkManager = struct {
             .layers = .{ CompletedLayerData.EMPTY, CompletedLayerData.EMPTY, CompletedLayerData.EMPTY },
             .allocator = self.allocator,
             .mesh_generation = gen,
-        }) catch {};
+        }) catch {
+            logger.warn("Failed to push empty mesh result for ({},{},{}), chunk stuck in .meshing", .{ pos.x, pos.z, pos.section_y });
+        };
+    }
+
+    /// Generate initial mesh on the worker thread right after terrain generation.
+    /// Captures available neighbors from storage via RCU — the chunk itself is not
+    /// yet in storage so it's passed directly. Uses mesh_generation=0 (initial).
+    fn generateInitialMesh(self: *Self, ctx: *WorkerContext, chunk: *const Chunk, pos: ChunkPos) ?CompletedMesh {
+        const worker_data: *WorkerData = @ptrCast(@alignCast(ctx.user_data orelse return null));
+        const mesh_ctx = worker_data.mesh_context;
+        const shaper = self.shared_model_shaper orelse return null;
+        const rcu_instance = self.rcu orelse return null;
+
+        // RCU read-side critical section for neighbor capture
+        _ = rcu_instance.readLock(@intCast(ctx.id));
+
+        var neighbors: [6]?Chunk = .{ null, null, null, null, null, null };
+        const offsets = [6]ChunkPos{
+            .{ .x = 0, .z = 0, .section_y = -1 }, // down
+            .{ .x = 0, .z = 0, .section_y = 1 }, // up
+            .{ .x = 0, .z = -1, .section_y = 0 }, // north
+            .{ .x = 0, .z = 1, .section_y = 0 }, // south
+            .{ .x = -1, .z = 0, .section_y = 0 }, // west
+            .{ .x = 1, .z = 0, .section_y = 0 }, // east
+        };
+
+        for (0..6) |i| {
+            const neighbor_pos = ChunkPos{
+                .x = pos.x + offsets[i].x,
+                .z = pos.z + offsets[i].z,
+                .section_y = pos.section_y + offsets[i].section_y,
+            };
+            if (self.chunk_storage.getAtomic(neighbor_pos)) |neighbor| {
+                const state = neighbor.getState();
+                if (state == .generated or state == .meshing or state == .ready or state == .dirty) {
+                    neighbors[i] = neighbor.chunk;
+                }
+            }
+        }
+
+        rcu_instance.readUnlock(@intCast(ctx.id));
+
+        // Build pointer array from stack copies
+        var neighbor_ptrs: [6]?*const Chunk = .{ null, null, null, null, null, null };
+        for (0..6) |i| {
+            if (neighbors[i] != null) {
+                neighbor_ptrs[i] = &(neighbors[i].?);
+            }
+        }
+
+        var mesh = mesh_ctx.generateMesh(
+            chunk,
+            pos,
+            neighbor_ptrs,
+            shaper,
+            self.texture_manager,
+        ) catch return null;
+
+        mesh.generated_chunk = null;
+        mesh.mesh_generation = 0; // Initial mesh always generation 0
+
+        return mesh;
     }
 };

src/client/world/RenderChunk.zig

@@ -415,9 +415,12 @@ pub const RenderChunk = struct {
         return self.future.isDirty();
     }
 
-    /// Check if this chunk is ready to render
+    /// Check if this chunk is ready to render.
+    /// Returns true whenever a valid mesh exists, even during remesh cycles
+    /// (.dirty / .meshing). The old mesh + GPU allocations stay valid until
+    /// setMesh replaces them, so rendering the stale mesh avoids holes.
     pub fn isReady(self: *const Self) bool {
-        return self.future.isReady() and self.mesh != null;
+        return self.mesh != null;
     }
 
     /// Mark chunk as dirty (needs remeshing)