fix: unify terrain lighting model

[MichaelFisher1997]

Summary\n- remove the forced simple-lighting override and restore the shared feature settings\n- render textured terrain, fallback terrain, and LOD terrain through one Lambert plus dielectric GGX lighting equation\n- restrict AO/SSAO to indirect diffuse, preserve RGB block light, fix camera-relative view vectors/TBN handedness, and remove mandatory lush grading\n\n## Verification\n- nix develop --command zig build test\n- nix develop --command zig build test-integration\n- nix develop --command zig build run -Dskip-present -Dauto-world=normal -Dstartup-diagnostic-seconds=5\n\nCloses #901

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

[github-actions]

📋 Summary

PR #908 closes #901 by removing the forced simple_lighting_mode override, collapsing the terrain simple/legacy/partial-PBR branches into a single computeTerrainLighting function, and applying the fixes to view vectors, TBN handedness, and AO/SSAO usage described in the issue. The default LOW preset is now aligned with the other presets (pbr_enabled: true). I verified that nix develop --command zig build test passes (exit 0), including shader compilation for the modified terrain.frag, water.frag, and post_process.frag.

📌 Review Metadata

• Reviewed Commit SHA: 54a3e6d6e4356359a9df8a703d08e9e6c86e44d6
• Reviewed PR: fix: unify terrain lighting model #908
• Linked Issue: [Lighting Overhaul][Phase 3] Unify terrain lighting, atmosphere, and output #901
• Issue Verification: The PR implements the [Lighting Overhaul][Phase 3] Unify terrain lighting, atmosphere, and output #901 scope items: removes the hard-coded simple_lighting_mode, unifies terrain/LOD/fallback lighting into one Lambert+dielectric GGGX equation, preserves RGB block light, applies AO/SSAO only to indirect diffuse, fixes camera-relative view vectors and TBN handedness, and removes applyLushGrade. The change is net-positive and matches the issue requirements.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

None identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] assets/shaders/vulkan/post_process.frag:181 - Dead code gate now trivially true
Confidence: High
Description: computeScreenSunShafts is defined but never called, and its global.render_flags.w < 0.5 guard is now always true because render_flags.w is hard-coded to 0.0 in rhi_render_state.zig:43. In sky.frag:208 the same guard is intentionally always-open now that simple lighting is gone.
Impact: No runtime impact today (function is dead), but the vestigial uniform field and dead function are technical debt.
Suggested Fix: Either remove computeScreenSunShafts and stop populating render_flags.w, or repurpose the bit for a future feature gate with a clear comment.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	One lighting function replaces four; clear separation of direct/indirect terms.
Open/Closed	7	New model is feature-gated by existing settings, but preset defaults changed.
Liskov Substitution	8	No inheritance hierarchy affected; shader inputs remain compatible.
Interface Segregation	8	FrameRenderParams is simplified by removing simple_lighting_enabled.
Dependency Inversion	7	RHI still consumes concrete settings; reasonable for this layer.
Average	7.6

🎯 Final Assessment

Overall Confidence Score: 85%

Confidence Breakdown:

• Code Quality: 90% (clean, focused refactor; removes more code than it adds)
• Completeness: 85% (addresses the main [Lighting Overhaul][Phase 3] Unify terrain lighting, atmosphere, and output #901 scope; some visual acceptance criteria like windowed/headless equivalence are not independently verified)
• Risk Level: 30% (shader changes always carry visual/regression risk, but tests pass and the change is well-scoped)
• Test Coverage: 80% (existing tests compile and pass; no new unit tests added for the lighting function)

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 - The PR correctly unifies the terrain lighting model, removes the forced simple-lighting override, and passes the build/test pipeline with no critical or high-priority issues.

Machine Readable Verdict

{
  "reviewed_sha": "54a3e6d6e4356359a9df8a703d08e9e6c86e44d6",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 85,
  "recommendation": "MERGE"
}

opencode session  |  github run