feat: define compact far-terrain tile foundation

[MichaelFisher1997]

Summary

• add the Phase 4 decision record comparing reusable indexed grids, compute expansion, and mesh shaders
• add a versioned 16-byte compact LOD3/4 sample format for terrain, materials, water, lighting, vegetation summaries, and provenance
• add neighbor-apron patching for stable boundary normals, strict serialization validation, CPU fallback rejection for unsupported span topology, and upload-byte accounting
• aggregate comprehensive compact-tile tests into the project suite

Scope

This is the design-approval and representation foundation required by #917 before production GPU renderer work. It intentionally does not claim that the reusable-grid renderer, far-water shader, or default enablement is complete; the ADR defines the measured rollout gates and maintained CPU fallback contract.

Validation

• nix develop --command zig fmt --check modules/world-lod/src/lod_tile.zig modules/world-lod/src/root.zig src/tests.zig
• nix develop --command zig build test --summary all (328/328 tests passed)
• git diff --check

Relates to #917

[github-actions]

📋 Summary

PR #928 relates to #917 (does not claim to close it). It lands the Phase 4 decision record and the CPU-side representation foundation for compact GPU far-terrain tiles: a versioned 16-byte CompactLODSample, allocator-owned CompactLODTile with one-sample apron patching, versioned serialization with CRC validation, CPU fallback rejection for unsupported vertical spans, byte-accounting metrics, and a full test suite.

The implementation matches the scope stated in the PR body: it is the foundation required by #917 before production renderer work, not the renderer, far-water shader, or default enablement. No previous automated issues were reported; this is the first review.

📌 Review Metadata

• Reviewed Commit SHA: fe4f9275fa7ed8405e3b9304a571a974b44529f3
• Reviewed PR: feat: define compact far-terrain tile foundation #928
• Linked Issue: LOD GPU roadmap Phase 4: compact GPU-rendered far-terrain tiles #917 — Relates to, not Fixes/Closes. The PR satisfies the foundation/ABI requirement described in LOD GPU roadmap Phase 4: compact GPU-rendered far-terrain tiles #917; the remaining GPU renderer, metrics, and visual-validation gates are explicitly deferred.

🔴 Critical Issues

None identified.

⚠️ High Priority Issues

None identified.

💡 Medium Priority Issues

None identified.

ℹ️ Low Priority Suggestions

**[LOW]** `modules/world-lod/src/lod_tile.zig:152` - `ByteMetrics.savedBytes` can underflow
**Confidence:** Medium
**Description:** `savedBytes` subtracts `compact_upload_bytes` from `conservative_expanded_top_grid_bytes` without saturation. For valid LOD3/4 tiles the result is positive, but a hand-rolled `CompactLODTile` with a very small `width` would produce a wrap-around `usize`.
**Impact:** Metrics consumers could see implausibly large "savings" for malformed tiles.
**Suggested Fix:** Use `std.math.sub` or return `0` when `compact_upload_bytes >= conservative_expanded_top_grid_bytes`.

**[LOW]** `modules/world-lod/src/lod_tile.zig:124` - Material validation is fragile against future block IDs
**Confidence:** Medium
**Description:** `isValid` compares the 7-bit material field against `@as(u7, @intCast(MAX_KNOWN_BLOCK_ID))`. Today `MAX_KNOWN_BLOCK_ID = 78`, so this is correct, but if `BlockType` ever grows beyond `0x7f`, the cast truncates and the check rejects valid samples.
**Impact:** ABI version 1 would silently reject encoded tiles when the block enum passes 127.
**Suggested Fix:** Compare against `@min(MAX_ENCODED_BLOCK_ID, MAX_KNOWN_BLOCK_ID)` or `MAX_ENCODED_BLOCK_ID` directly, since the wire field is only 7 bits wide.

**[LOW]** `modules/world-lod/src/lod_tile.zig:300` - CRC only covers the sample payload, not the header
**Confidence:** Low
**Description:** The checksum is computed over `std.mem.sliceAsBytes(self.samples)` only. Magic, version, LOD, apron mask, width, and sample count are unprotected by the CRC.
**Impact:** A header byte flip (e.g., width or LOD) is only caught by downstream length/count checks, not by the checksum itself.
**Suggested Fix:** Compute the CRC over the entire serialized buffer (header + payload) or document that the header is considered separately validated.

**[LOW]** `modules/world-lod/src/lod_tile.zig:281` - `byteMetrics` couples renderer-independent tile to `engine-rhi.Vertex` size
**Confidence:** Low
**Description:** The compact-tile module claims to be renderer-independent, but `byteMetrics` imports `@import("engine-rhi").Vertex` to compute the CPU-expanded comparison.
**Impact:** The metric is tied to a specific RHI vertex layout; changing `Vertex` changes the savings number even if the compact ABI is unchanged.
**Suggested Fix:** Accept the vertex size as a parameter or define a `pub const CompactLODVertexSize` in the module, making the comparison explicit.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	lod_tile.zig is cohesive: encode, decode, apron, serialization, and metrics all serve one compact-tile abstraction.
Open/Closed	6	The ABI is versioned, but version 1 handling is hardcoded; future versions will require direct edits.
Liskov Substitution	9	No subtyping; pure data structs and free functions.
Interface Segregation	8	Public surface is focused on tile/sample/metrics types.
Dependency Inversion	6	byteMetrics directly depends on engine-rhi.Vertex instead of receiving a size parameter.
Average	7.4

🎯 Final Assessment

Overall Confidence Score: 90%

Confidence Breakdown:

• Code Quality: 90% (well-structured, idiomatic Zig, thorough tests)
• Completeness: 85% (foundation is complete; renderer integration intentionally deferred)
• Risk Level: 20% (pure CPU additions, isolated module, no RHI/GPU threading changes)
• Test Coverage: 90% (round-trip, clamping, aprons, span rejection, corruption, and metrics are covered)

Merge Readiness:

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

Verdict:

MERGE — The foundation code is clean, well-tested, and correctly scoped; the remaining #917 work is explicitly deferred in both the ADR and the PR description.

{
  "reviewed_sha": "fe4f9275fa7ed8405e3b9304a571a974b44529f3",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 90,
  "recommendation": "MERGE"
}

opencode session  |  github run