feat: ship high-detail live world map

[MichaelFisher1997]

Replaces the generator-only M-key map with a SeedMapper-style live overlay: actual chunk blocks, trees, foliage, water, and player edits now show in real time, backed by a 1024x1024 source texture with progressive preview and procedural fallback outside loaded chunks.

Highlights:

• Overlay captures top blocks (including water, foliage, player builds) into an immutable per-request snapshot; the map worker receives only immutable data so streaming and mutations remain race-safe.
• Per-chunk surface cache tracks content revisions, so place/break triggers immediate updates and chunk arrival invalidates correctly.
• getMapSampleReduced path uses accurate map sampling with single-point terrain blending; the full nine-sample blend stays on the chunk generation path.
• Block-by-block color table covers grass/leaves/woods/water/ice/etc. so distant trees and buildings read at a glance.
• 1024x1024 map texture, linear filtering, 256x256 preview, and resolution-independent world scaling: default zoom samples one world block per map texel.
• Coordinate grid, labels, and a scale bar drawn in world space; smooth pan/zoom with cursor-anchored wheel zoom, debounced live updates, and latest-wins coarse and full passes.

Validation:

• Pre-push formatting + full test suite passes.
• Headless visual capture confirms smooth coastlines, canopy shading, and pixel-scale building footprints (screenshots/map-live-surface.png, screenshots/map-high-resolution.png, screenshots/map-high-resolution-wide.png).
• New tests cover chunk top-surface rebuild, foliage/water capture, mutation invalidation, and worker superseded requests.

[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]

📋 Summary

Linked issues: The PR description does not reference any specific issue ("Closes #N" / "Fixes #N"). The PR is labeled documentation and build, not automated-test, so the automated-test full-coverage protocol does not apply.

This PR replaces the generator-only M-key map with a SeedMapper-style live overlay backed by a 1024×1024 texture. It introduces an immutable LoadedSurfaceOverlay snapshot mechanism so the map worker never holds chunk pointers, adds a rebuildMapSurface cache on Chunk, extends the worldgen vtable with getMapSampleReduced/getColumnInfoReduced, and wires up progressive coarse→full rendering with a single background worker plus 4-way parallel slice rendering. The implementation is well-structured, with strong test coverage for the new behavior and correct worker-thread lifecycle (join before generator destruction).

📌 Review Metadata

• Reviewed Commit SHA: c527e973d1d5532d8bbcbd09e2b8089f70383841
• Reviewed PR: feat: ship high-detail live world map #943

No linked issues to verify against. Highlights and validation claims (chunk surface rebuild, foliage/water capture, mutation invalidation, superseded requests) are all backed by tests that pass under zig build test.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (none were reported previously).

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved (none were reported previously).

None identified.

💡 Medium Priority Issues (Nice to Fix)

**[MEDIUM]** `modules/world-runtime/src/world.zig:1032-1048` + `modules/world-runtime/src/chunk_queue_coordinator.zig:590-614` - Data race on chunk.map_surface_* fields
**Confidence:** High
**Description:** `captureLoadedMapSurface` holds only `chunks_mutex.lockShared()` (line 1033) yet calls `chunk.rebuildMapSurface()` (line 1041), which writes `map_surface_blocks`, `map_surface_heights`, and `map_surface_revision`. Meanwhile, `chunk_queue_coordinator` calls `chunk_data.chunk.rebuildMapSurface()` (line 591) on the *same* chunk with NO lock held, deliberately before `self.storage.chunks_mutex.lock()` (line 595) for throughput reasons. Because an RwLock shared lock does not synchronize against an unlocked writer on the same memory, the render thread can observe a partially-written `map_surface_blocks` array while the coordinator is mid-rebuild on a chunk that is already in storage with `generated == true` (e.g., regeneration after a save-load failure path at line 543, or any future requeue path).
**Impact:** Torn reads of map color/height data copied into the overlay, producing transient visual glitches (wrong colors, mis-shaded relief) for one frame on chunks whose publication races with a capture. Self-corrects on the next residency-stable capture, but it is a formal data race that ThreadSanitizer would flag.
**Suggested Fix:** Either (a) gate the capture on `chunk.state == .generated` in addition to `chunk.generated` so chunks mid-(re)generation are skipped, or (b) move the coordinator's `rebuildMapSurface()` to after `self.storage.chunks_mutex.lock()` and accept the small serialization (the existing comment about scanning 65k blocks under the lock is exactly the trade-off to revisit), or (c) have `captureLoadedMapSurface` hold `chunks_mutex.lock()` (exclusive) for the iteration instead of `lockShared()` since this path runs at most once per debounce window.

ℹ️ Low Priority Suggestions (Optional)

**[LOW]** `modules/worldgen-overworld/src/world_map.zig:70-87` - Dead branch in sampleRepresentative radius
**Confidence:** High
**Description:** `sampleRepresentative` computes `radius = if (scale <= 8.0) @max(scale * 0.5, 0.5) else 0.0`. The only caller (`renderRows` line 379) passes `live_footprint = @min(request.scale * sample_step, 8.0)`, so `scale` is always ≤ 8.0 and the `else 0.0` branch is unreachable. Additionally, if it were ever reached with an integer `wx`, `max_x = @intFromFloat(@ceil(wx + 0) - 1.0)` would be `wx - 1`, making the loop not execute and silently falling back to procedural sampling.
**Impact:** No functional impact today; just misleading dead code that could bite a future caller.
**Suggested Fix:** Drop the `else 0.0` branch and always use `@max(scale * 0.5, 0.5)`, or document that the function expects `scale <= 8.0`.

**[LOW]** `modules/worldgen-overworld/src/world_map.zig:16,326-355` - FULL_RENDER_WORKERS is hardcoded
**Confidence:** Medium
**Description:** `FULL_RENDER_WORKERS = 4` is hardcoded regardless of `std.Thread.getCpuCount()`. On 2-core CI/headless machines this oversubscribes; on high-end desktops it leaves cores idle. Thread-spawn failure is already handled gracefully (lines 349-353), so this is purely a tuning issue.
**Impact:** Suboptimal map refinement latency on some hosts.
**Suggested Fix:** `const FULL_RENDER_WORKERS: u32 = @min(@max(std.Thread.getCpuCount() catch 4, 2), 8);` (or expose as a build option).

**[LOW]** `modules/worldgen-overworld/src/world_map.zig:473-477` - shouldCancel acquires the mutex per row
**Confidence:** Medium
**Description:** Inside `renderRows`, `shouldCancel(request.generation)` is called once per output row (line 371). Each call takes `self.mutex` exclusively. With `FULL_RENDER_WORKERS=4` slices all polling the same mutex, this creates noticeable contention on large (1024×1024) textures.
**Impact:** Minor throughput reduction during refinement; no correctness issue.
**Suggested Fix:** Throttle the check (e.g., every 16 rows), or read `latest_generation` via a relaxed atomic load instead of going through the mutex.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	LoadedSurfaceOverlay cleanly isolates immutable snapshot data; MapController owns pan/zoom, WorldMap owns rendering. captureLoadedMapSurface arguably mixes "viewport culling" with "snapshot copy" but stays focused.
Open/Closed	8	New vtable entries (getColumnInfoReduced, getMapSampleReduced, column_info_thread_safe) are optional with documented fallbacks, so existing generators keep working without code changes.
Liskov Substitution	9	All four generators (overworld, overworld-v2, flat, test) set column_info_thread_safe=true consistently, and the fallback MapSample builder ensures any generator without a reduced sampler still conforms.
Interface Segregation	8	The Generator.VTable grew by two optional function pointers plus a bool rather than a separate MapSampler interface. Acceptable for the size, but the vtable is becoming a kitchen sink.
Dependency Inversion	7	WorldMap depends on the Generator abstraction (good), but MapController now depends directly on the World concrete struct (line 14 of map_controller.zig) rather than IWorld, slightly tightening the coupling.
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 82%

Confidence Breakdown:

• Code Quality: 88% (clean structure, good comments, proper errdefer/defer, comprehensive tests)
• Completeness: 90% (delivers all highlights listed in the PR body; tests cover the claimed behaviors)
• Risk Level: 70% (introduces a real but cosmetic data race; adds significant threading complexity to a previously synchronous module)
• Test Coverage: 85% (new tests for surface rebuild, foliage/water, superseded requests, parallel render slices; missing a test that exercises the coordinator-publishes-while-capture-runs race window)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (verified zig build test, zig fmt --check, and targeted filter runs all green)

Verdict:

MERGE WITH FIXES — Solid, well-tested feature work; the shared-lock write in captureLoadedMapSurface racing against the coordinator's unlocked rebuildMapSurface should be tightened (e.g., gating capture on state == .generated) before merge, but the issue is cosmetic and self-correcting if you choose to follow up post-merge.

{
  "reviewed_sha": "c527e973d1d5532d8bbcbd09e2b8089f70383841",
  "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]

All checks pass. The fix commit resolves all 4 previously reported issues, introduces no new problems, and all tests + ReleaseFast build succeed. Here is my review:

📋 Summary

Linked issues: None. The PR description does not reference any specific issue ("Closes #N" / "Fixes #N"), and the PR is labeled documentation + build (not automated-test), so the automated-test full-coverage protocol does not apply.

The PR (2 commits) replaces the generator-only M-key map with a SeedMapper-style live overlay backed by a 1024×1024 texture, capturing actual chunk blocks/trees/foliage/water/player edits into an immutable per-request snapshot. The second commit (531cbc6, "fix: synchronize map surface capture") directly addresses the data-race and tuning concerns raised in the prior review of commit c527e97, and all tests now pass on the current HEAD.

📌 Review Metadata

• Reviewed Commit SHA: 531cbc635448cc72dfd319b41d3c5cb2a93a8d9f
• Reviewed PR: feat: ship high-detail live world map #943

No linked issues to verify against.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (none were reported previously).

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved (none were reported previously).

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: Data race on chunk.map_surface_* fields between captureLoadedMapSurface (shared lock) and chunk_queue_coordinator.rebuildMapSurface (unlocked).

The fix at modules/world-runtime/src/world.zig:1043 adds or chunk.state != .generated to the skip condition. This is correct: the coordinator performs the unlocked rebuildMapSurface() at chunk_queue_coordinator.zig:591 while the chunk's state is still .generating, and only transitions to .generated under the exclusive chunks_mutex.lock() at chunk_queue_coordinator.zig:613 (after the rebuild completes). The capture path's shared lock therefore never observes a chunk mid-rebuild. Mutations remain synchronized via lighting_mutex (held by both applyBlockMutation at world_mutation.zig:56 and captureLoadedMapSurface at world.zig:1032). Verified: ThreadSanitizer-safe invariant documented in the comment at world.zig:1040-1042.

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: Dead branch in sampleRepresentative radius — the else 0.0 branch was removed at world_map.zig:71; radius is now always @max(scale * 0.5, 0.5).

✅ [FIXED] Previous issue: FULL_RENDER_WORKERS hardcoded — replaced with MAX_FULL_RENDER_WORKERS = 8 and dynamic worker_count = @min(@max(getCpuCount() catch 4, 2), 8) at world_map.zig:328. The RenderSlice.slice_stride field (line 128) correctly propagates the runtime count to each worker, and the fallback path at lines 352-355 re-renders failed spawns with the same stride.

✅ [FIXED] Previous issue: shouldCancel per-row mutex contention — now throttled to every 16 rows via rows_since_cancel_check at world_map.zig:373-379.

**[LOW]** Blacksmith CI failure `(TestExpectedEqual)/(TestExpectedEqual)` on commit c527e97
**Confidence:** Low
**Description:** Blacksmith reported one expectEqual failure on the first commit (before the fix commit). The generic test name gives no context. All WorldMap/MapController/chunk-surface/world-facade tests pass locally on the current HEAD (531cbc6), and the fix commit was pushed after the failure.
**Impact:** If the failure was a real regression from this PR (rather than pre-existing flakiness), it may already be resolved by the synchronization fix; otherwise it could recur on Blacksmith runners.
**Suggested Fix:** Confirm the Blacksmith re-run on 531cbc6 is green before merge; if it still fails, capture the specific test name and stack trace.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	LoadedSurfaceOverlay isolates immutable snapshot data cleanly; captureLoadedMapSurface stays focused on viewport-culled copy.
Open/Closed	8	New vtable entries (getColumnInfoReduced, getMapSampleReduced, column_info_thread_safe) are optional with documented fallbacks; existing generators work unchanged.
Liskov Substitution	9	All four generators set column_info_thread_safe=true consistently; fallback MapSample builder ensures conformance.
Interface Segregation	8	VTable grew by two optional pointers + a bool rather than a separate interface. Acceptable for size, but the vtable is becoming a kitchen sink.
Dependency Inversion	7	WorldMap depends on Generator abstraction; MapController now depends on concrete World (map_controller.zig:14) rather than IWorld, tightening coupling slightly.
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

• Code Quality: 90% (clean structure, good comments explaining the synchronization invariant, proper errdefer/defer, comprehensive tests)
• Completeness: 92% (delivers all highlights; fix commit addresses every prior review concern)
• Risk Level: 80% (threading complexity is now correctly guarded by the state-machine invariant; the coupling between coordinator publish order and capture skip condition is documented)
• Test Coverage: 88% (new tests for surface rebuild, foliage/water, superseded requests, parallel slices, dynamic worker count; the race-window skip is implicitly covered by the generation flow)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (verified zig build test, zig fmt --check, ReleaseFast build, and targeted filter runs all green)

Verdict:

MERGE — All previously reported issues are resolved by the synchronization fix commit, no new issues are introduced, and the full test suite + ReleaseFast build pass; confirm the Blacksmith re-run is green since the original failure predated the fix.

{
  "reviewed_sha": "531cbc635448cc72dfd319b41d3c5cb2a93a8d9f",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

opencode session  |  github run