Skip to content

fix: polish LOD seam transitions#693

Merged
github-actions[bot] merged 1 commit intodevfrom
feature/issue-687-lod-polish
May 3, 2026
Merged

fix: polish LOD seam transitions#693
github-actions[bot] merged 1 commit intodevfrom
feature/issue-687-lod-polish

Conversation

@MichaelFisher1997
Copy link
Copy Markdown
Collaborator

Summary

  • Blend LOD boundary heights toward a coarse-aligned edge sample to reduce cracks at region seams while keeping the existing heightfield path.
  • Use representative surface/water sampling for LOD quad materials and add seam diagnostics behind ZIGCRAFT_LOD_DIAG.
  • Widen shader dither handoff and tune LOD mask/fog defaults for softer near/far transitions.

Fixes #687

Verification

  • nix develop --command zig fmt modules/world-lod/src/lod_mesh.zig modules/world-lod/src/lod_chunk.zig
  • nix develop --command zig build test
  • nix develop --command zig build -Dskip-present
  • nix develop --command zig build run -Dskip-present -Dauto-world=normal -Dstartup-diagnostic-seconds=5 timed out after 90s during startup/build output

@github-actions github-actions Bot added documentation Improvements or additions to documentation shaders labels May 3, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 3, 2026

📋 Summary

Linked Issue: Fixes #687

This PR fully implements issue #687's requirements:

  1. Adjacency-aware edge stitching via stitchedHeight() blending boundary samples toward coarse-aligned grid points
  2. Dithered/faded LOD handoff in terrain.frag replacing chunk-based hard discard with distance-based dither fade (aligned with g_pass.frag); transition width widened from 24→32
  3. Representative material sampling via representativeSurfaceBlock() (majority-vote over 4 cells) and representativeWaterDepth() (coverage-weighted average)
  4. Tuned water/fog/mask — water threshold raised to 0.35 with depth exception, fog start percentages adjusted per level, overlap reduced from -1 to -2 chunks
  5. Debug diagnostics via ZIGCRAFT_LOD_DIAG env flag logging max edge adjustment
  6. Tests updated and addedcalculateMaskRadius expectations updated, new tests for blockForLODQuad and stitchedHeight

Overall Quality: Well-scoped, focused PR with clean separation of concerns. No previous automated reviews to reconcile.


📌 Review Metadata


🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.
None identified.


⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved.
None identified.


💡 Medium Priority Issues (Nice to Fix)

None identified.


ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/world-lod/src/lod_mesh.zig:723representativeSurfaceBlock tie-breaking undocumented
Confidence: High
Description: When two or more block types tie for majority count (e.g., 2 grass + 2 stone), the first one encountered in the nested loop wins. This is deterministic but arbitrary.
Impact: Minor — could produce inconsistent material selection at biome boundaries depending on loop order.
Suggested Fix: Add a brief comment explaining the tie-breaking behavior, or break ties using a secondary heuristic (e.g., prefer the block at the quad center).

[LOW] modules/world-lod/src/lod_mesh.zig:861stitchedHeight magic blend factor undocumented
Confidence: High
Description: The 0.35 multiplier in const blend = edge_weight * 0.35 controls maximum edge blend strength but has no explanatory comment.
Impact: Minor — future maintainers won't know why 0.35 was chosen or whether it's safe to tune.
Suggested Fix: Add a doc comment explaining that 0.35 caps the blend so the heightfield doesn't fully snap to coarse grid, preserving local detail while reducing seams.


📊 SOLID Principles Score

Principle Score Notes
Single Responsibility 8 Each new function (stitchedHeight, representativeSurfaceBlock, representativeWaterDepth, maxStitchedHeightAdjustment) has a single clear purpose.
Open/Closed 8 Extends LOD behavior without modifying core interfaces; diagnostics are opt-in via env flag.
Liskov Substitution 7 No inheritance used; changes are additive within existing struct behavior.
Interface Segregation 7 No new interfaces introduced; changes fit within existing ILODConfig and meshing patterns.
Dependency Inversion 7 Reuses existing abstractions (engine-core env flag, RHI types, TextureAtlas).
Average 7.4

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

  • Code Quality: 90% — Clean, idiomatic Zig; proper allocator hygiene (defer vertices.deinit), consistent naming.
  • Completeness: 90% — All issue Phase 1: polish LOD seams, fades, and representative sampling #687 acceptance criteria met; shaders, meshing, and config tuned cohesively.
  • Risk Level: 15% — Changes are localized to LOD system; no RHI/Vulkan interface changes; fallback paths preserved.
  • Test Coverage: 85% — Existing test updated, two new unit tests added. Could benefit from a visual/regression test but unit coverage is adequate.

Merge Readiness:

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

Verdict:

MERGE

Low-priority documentation nits do not block merge; PR is clean, focused, and fully addresses linked issue #687.

{
  "reviewed_sha": "07006cd1ae4dac4fc6afd8cbb2a3b4971573c473",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

New%20session%20-%202026-05-03T18%3A55%3A02.205Z
opencode session  |  github run

@github-actions github-actions Bot merged commit b39dfef into dev May 3, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation shaders

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phase 1: polish LOD seams, fades, and representative sampling

1 participant