perf: accelerate world and LOD startup

[MichaelFisher1997]

Summary

• prioritize azimuthally distributed coarse LOD horizon seeds and retain their priority through cache, generation, and meshing transitions
• reduce Medium LOD4 source density, rebalance streaming workers, coalesce remesh work, and add robust GPU-meshing fallback/retry behavior
• reveal the menu after drawable terrain while continuing background streaming, including safe expanded distant LODs in the home preview
• keep runtime preset changes synchronized with the full-detail chunk-radius cap

Performance

• cached Debug first outer LOD4: 18.153s -> 6.778s
• ReleaseFast Medium traversal: 720.8 average FPS, 1.387ms average CPU, 0.847ms average GPU

Validation

• nix develop --command zig fmt --check src/ modules/
• nix develop --command zig build test
• nix develop --command zig build -Doptimize=ReleaseFast
• nix develop --command zig build test-integration -Dskip-present=true
• headless home preview LOD4 renderability and screenshot verification
• pre-push formatting and full test hooks

[github-actions]

kcov coverage

Line coverage ran for this PR and uploaded a non-blocking report artifact named kcov-report. Codecov upload is configured as non-blocking while the project captures a stable baseline.

[blacksmith-sh]

Found 1 test failure on Blacksmith runners:

Failure

Test	View Logs
(TestExpectedEqual)/(TestExpectedEqual)	View Logs

[github-actions]

Now I have a comprehensive understanding of the PR. Let me write the review.

📋 Summary

No linked issues found in the PR description (no "Closes #", "Fixes #", or "Resolves #" references).

📌 Review Metadata

• Reviewed Commit SHA: 76def407067931ae4e5a16539260f173a55db964
• Reviewed PR: perf: accelerate world and LOD startup #940

This PR accelerates world/LOD startup through several coordinated changes: azimuthally distributed coarse LOD horizon seed selection with preserved priorities across the lifecycle pipeline, reduced Medium LOD4 source density, rebalanced worker pools, coalesced remesh work via neighborhood-readiness checks, robust GPU-meshing fallback/retry behavior, earlier menu reveal, and preset-capped full-detail chunk radius. The changes are well-structured with good test coverage and include a legitimate concurrency fix in finalizeChunkMesh.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (no previous reviews to check).

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] modules/world-runtime/src/gpu_mesher.zig:230 - GPU mesh dirty path doesn't enqueue re-mesh
Confidence: High
Description: When the GPU mesher completes and data.chunk.dirty is true, the chunk transitions to .generated state with valid (but stale) mesh allocations, but enqueuePendingMesh is never called. This is inconsistent with the CPU upload path in chunk_queue_coordinator.zig:460-462, which calls enqueuePendingMesh immediately for the same scenario. The GPU path runs under storage.chunks_mutex.lock() (line 167), so it cannot call enqueuePendingMesh directly due to lock ordering (pending_mesh_mutex must be acquired before chunks_mutex per the comment at line 822). The chunk must wait for the 60-frame recovery scan (RECOVERY_SCAN_PERIOD) or an enqueueReadyNeighborhood notification from a finishing neighbor.
Impact: Chunks that become dirty during GPU meshing will be invisible for up to ~1 second (60 frames at 60fps) before the recovery scan re-queues them. During active streaming with rapid neighbor generation, this could cause visible flickering/holes at chunk boundaries.
Suggested Fix: Collect dirty chunk coordinates into a local buffer during the finalizeCompletedMeshes loop, then after storage.chunks_mutex.unlock() (after the defer), iterate and call enqueuePendingMesh for each:

var dirty_refs: [MAX_GPU_MESH_BATCH]PendingMeshRef = undefined;
var dirty_count: usize = 0;
// ... in the loop, when dirty:
//   dirty_refs[dirty_count] = .{ .x = req.cx, .z = req.cz, .job_token = ... };
//   dirty_count += 1;
// after unlock:
//   for (dirty_refs[0..dirty_count]) |ref| enqueuePendingMesh(ref.x, ref.z, ref.job_token);

Note: This requires access to the ChunkQueueCoordinator from GpuMesher, or a callback. If that's architecturally complex, the current recovery-scan fallback is acceptable but introduces latency.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/world-runtime/src/world_streamer.zig:247 - i32 overflow in distance check
Confidence: Low
Description: dx * dx + dz * dz > radius * radius uses i32 arithmetic. For radii above ~46340 (sqrt of i32 max), radius * radius overflows. The same pattern appears in enqueueReadyNeighborhood at line 803. While render distances are currently bounded to reasonable values (<256), this is technically unsafe.
Impact: None in practice with current presets, but could surface if future presets allow very large radii.
Suggested Fix: Use i64 arithmetic as done in hasRenderableCoarsestNear (lod_manager.zig:310) or updatePlayerPos (job_system.zig:250) which already use i64 intermediates.

[LOW] modules/world-lod/src/lod_scheduler.zig:197-264 - Shared lock held across long horizon scan
Confidence: Low
Description: ctx.mutex.lockShared() is held across the entire horizon candidate scan (nested loops over region_radius), including candidates.insert() calls that allocate memory. For coarse LOD4 with a large horizon (512 chunks, region_radius ~17), this is ~1189 iterations under the shared lock. Writers (exclusive lock holders) are blocked during this time.
Impact: Minimal in practice since the scan is bounded and runs during startup, but could cause writer contention on systems with many concurrent LOD operations.
Suggested Fix: Consider snapshotting the candidate_storage state (count + existing keys) under the lock, releasing it, then doing the scan lock-free. This is a non-trivial refactor and the current approach is acceptable for the performance goals.

[LOW] modules/game-ui/src/screens/home.zig:107-108 - Very early menu reveal
Confidence: Low
Description: isReadyForPresentation now returns true when chunks_rendered > 0, removing the !isStartupBusy() gate. This means the menu can appear with just a single rendered chunk, potentially showing a mostly empty world behind the menu.
Impact: UX concern only — the user might see sky/void behind the menu briefly. The PR description states this is intentional ("reveal the menu after drawable terrain while continuing background streaming"), so this is by design.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	Each module change is focused: scheduler handles priorities, coordinator handles state, mesher handles GPU completion
Open/Closed	7/10	New QueueResult enum and compact_tiles_enabled flag extend behavior without breaking existing callers; ILODConfig vtable extended cleanly
Liskov Substitution	8/10	QueueResult enum is a clean replacement for the bool return; all call sites updated consistently
Interface Segregation	7/10	ILODConfig gained getCompactTilesEnabled — small focused addition to the interface
Dependency Inversion	7/10	Scheduler depends on abstract config interface; coordinator depends on GPU abstraction via QueueResult
Average	7.4

🎯 Final Assessment

Overall Confidence Score: 82%

Confidence Breakdown:

• Code Quality: 85% (well-structured, consistent patterns, proper lock usage, good comments)
• Completeness: 85% (thorough implementation of startup acceleration with good test coverage)
• Risk Level: 80% (concurrency changes are carefully reasoned; the finalizeChunkMesh lock fix is a genuine improvement; the GPU dirty-path gap is bounded)
• Test Coverage: 78% (4 new tests + 1 updated test covering the main behaviors; the GPU dirty-path gap is not tested)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (per PR description)

Verdict:

MERGE WITH FIXES — The GPU mesher dirty-path missing enqueuePendingMesh should be addressed to avoid visible chunk gaps during streaming, but the PR is otherwise high quality with solid concurrency reasoning and good test coverage.

Machine Readable Verdict

{
  "reviewed_sha": "76def407067931ae4e5a16539260f173a55db964",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 82,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[github-actions]

Based on my analysis of the PR (including the second commit 689e738 which addressed the prior review's concern), here is my review:

📋 Summary

No linked issues found in the PR description (no "Closes #", "Fixes #", or "Resolves #" references).

📌 Review Metadata

• Reviewed Commit SHA: 689e73861c4541c8fbf12f06ff591848164c0b9e
• Reviewed PR: perf: accelerate world and LOD startup #940

This PR accelerates world/LOD startup through azimuthally distributed coarse LOD horizon seed selection with priority preservation across cache/generation/meshing transitions, rebalanced worker pools, coalesced remesh via neighborhood-readiness checks, robust GPU-meshing fallback/retry behavior, earlier menu reveal, and preset-capped full-detail chunk radius. The second commit (689e738) specifically addresses the medium-priority concern raised in the prior automated review by adding a RemeshCallback to GpuMesher.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (no previous critical issues to check).

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: GPU mesh dirty path didn't enqueue re-mesh in finalizeCompletedMeshes — commit 689e738 adds a RemeshCallback (gpu_mesher.zig:152, setRemeshCallback) wired from WorldStreamer.init (world_streamer.zig:191) to ChunkQueueCoordinator.enqueuePendingMesh. The implementation correctly batches dirty requests into a stack-local remesh_requests array (gpu_mesher.zig:176), releases chunks_mutex (gpu_mesher.zig:259), and only then invokes the callback (gpu_mesher.zig:261-264) — preserving the documented pending_mesh_mutex-before-chunks_mutex lock ordering. The CPU upload path (chunk_queue_coordinator.zig:460-462) now also transitions .dirty chunks through .generated and re-queues them.

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: world_streamer.zig:247 i32 overflow — the new isStartupBusy distance check at world_streamer.zig:252-255 now uses i64 intermediates.

✅ [FIXED] Previous issue: chunk_queue_coordinator.zig:803 i32 overflow in enqueueReadyNeighborhood — commit 689e738 updated this to use i64 intermediates at chunk_queue_coordinator.zig:801-804.

[LOW] modules/world-lod/src/lod_scheduler.zig:197-264 - Shared lock held across horizon candidate scan
Confidence: Low
Description: ctx.mutex.lockShared() (line 197) is still held across the entire nested candidate scan loop, including candidates.insert() calls that allocate memory. For coarse LOD4 with a 512-chunk horizon (~1189 region iterations), writers remain blocked during the scan. This was flagged in the prior review and the implementation choice was deemed acceptable for the startup performance goals; it remains acceptable here.
Impact: Minimal in practice — scan is bounded and runs during startup.
Suggested Fix: Optional future refactor: snapshot candidate_storage.count() under the lock, release it, then scan lock-free. Non-blocking.

[LOW] modules/game-ui/src/screens/home.zig:107 / modules/game-ui/src/screens/rml_home.zig:106 - Very early menu reveal
Confidence: Low
Description: isReadyForPresentation returns true with chunks_rendered > 0 only. The previous !isStartupBusy() gate was removed. Per the PR description this is intentional ("reveal the menu after drawable terrain while continuing background streaming"), so it is by-design.
Impact: UX-only; users may briefly see sky/void behind the menu during the first frame a single chunk is drawable. No functional issue.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8/10	Each module change is focused: scheduler handles priorities, coordinator handles state transitions, mesher handles GPU completion
Open/Closed	8/10	QueueResult enum and compact_tiles_enabled flag extend behavior without breaking existing callers; ILODConfig vtable extended cleanly
Liskov Substitution	8/10	QueueResult enum is a clean replacement for the bool return; all call sites updated consistently
Interface Segregation	8/10	ILODConfig gained getCompactTilesEnabled — small, focused addition; RemeshCallback is opt-in via setter
Dependency Inversion	8/10	Scheduler depends on abstract config interface; mesher exposes callback abstraction rather than reaching into the coordinator
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

• Code Quality: 90% (well-structured, consistent patterns, careful lock ordering, clear comments explaining non-obvious choices)
• Completeness: 88% (thorough implementation; prior review's medium concern explicitly fixed in follow-up commit)
• Risk Level: 85% (broad scope touching 24 files including concurrency-sensitive streaming paths, but reasoning is sound and existing invariants are preserved)
• Test Coverage: 88% (4 new focused tests + 1 updated assertion; verified locally — zig build test passes all suites, zig fmt --check clean, ReleaseFast build succeeds)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (verified locally: all tests pass, format clean, ReleaseFast builds)

Verdict:

MERGE — The previously reported medium-priority GPU-mesh remesh concern is fully resolved by commit 689e738, both prior low-priority i32 overflow concerns are fixed, and all tests/format/ReleaseFast verification succeeds locally.

Machine Readable Verdict

{
  "reviewed_sha": "689e73861c4541c8fbf12f06ff591848164c0b9e",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

opencode session  |  github run

[MichaelFisher1997]

Addressed the GPU dirty-remesh review finding in 689e738:

• GPU completions now collect dirty/fallback chunks while holding chunks_mutex, then enqueue remeshes through the streaming coordinator after releasing it, preserving lock order.
• Overflow and vertex-allocation fallback paths are also requeued immediately instead of waiting for the recovery scan.
• Distance-square checks in startup readiness and neighborhood checks now use i64 intermediates.

Validation: format check, full unit suite, ReleaseFast build, integration tests, and pre-push checks passed. test-robustness could not run in this session because robust-demo was signaled in the headless environment.