test(engine-math): pin intersectsChunkRelative camera-relative semantics (#741)

[MichaelFisher1997]

Summary

Closes #741.

Investigation result: the reported bug does not exist. The audit concluded that intersectsChunkRelative (libs/zig-math/frustum.zig:118) miscalculates the bounding-sphere center (world_y = -cam_y) and proposed anchoring world_y = 0. That analysis conflated world space with camera-relative space.

Why the current code is correct

The renderer uses camera-relative (floating-origin) rendering:

• The terrain view-projection is built from Camera.getViewMatrixOriginCentered — Mat4.lookAt(Vec3.zero, forward, up), rotation only (modules/engine-graphics/src/camera.zig:203, consumed at modules/engine-graphics/src/render_graph.zig:464).
• Chunk model matrices translate by chunk_world − camera_pos (modules/world-runtime/src/world_renderer.zig:334, :456, :660).

So the frustum handed to intersectsChunkRelative lives in camera-relative space (apex at origin). In that space the chunk spans y ∈ [−cam_y, 256 − cam_y], and its vertical center is (CHUNK_SIZE_Y * 0.5) − cam_y = 128 − cam_y — exactly what the function computes (-cam_y + 128).

The audit's cam_y = 80 example: the sphere covers y ∈ [−96, 192] in camera-relative space, which is y ∈ [−16, 272] in world space — fully containing the chunk's y ∈ [0, 256]. The camera-y offset cancels between the sphere center and the chunk corners, so the sphere contains every chunk corner for any camera height. The radius = 144 is a deliberate conservative bound (true circumscribed radius is √(8² + 128² + 8²) ≈ 128.5).

Why the proposed fix is harmful

Setting world_y = 0 would anchor the sphere's center at camera-relative y = 128 regardless of camera height. When the camera is high above terrain, the chunk's true camera-relative center is far below (e.g. −472 at cam_y = 600), but the "fixed" sphere would sit ~cam_y units above the view axis and cull terrain that is actually in front of the camera. It also makes cam_y an unused parameter (compile error) and breaks the existing Frustum forward view at y=80 sees all nearby chunks test.

Changes

No production change. Added regression tests in src/math_tests.zig (the root-module test file that is actually collected by zig build test; note the inline tests in libs/zig-math/frustum.zig and modules/engine-math/src/frustum_tests.zig are not collected by the build):

• Frustum forward view sees chunk ahead across camera heights — the chunk straight ahead is visible for cam_y ∈ {0, 80, 128, 200, 256} (issue acceptance criterion Roadmapv3 #1).
• Frustum looking down at chunk from high camera still sees it — a camera at y = 600 (and y = 200) aimed straight at a chunk still sees it. This directly refutes the "missing terrain when camera is high" claim and fails if the sphere is detached from the camera (verified: fails under the proposed world_y = 0 change).
• Frustum intersectsChunkRelative matches independent circumscribed sphere — for a battery of forward/down/up/angled frustums, multiple camera heights, and chunk/camera offsets, the function reports visible whenever an independently-derived circumscribed sphere (radius ≈ 128.5) at the camera-relative chunk center does. This verifies the sphere contains the full chunk (all 256 Y levels) and would fail if the center drifted out of camera-relative space.

Verification

`zig build test` — 275/275 pass (was 272; +3 new tests).

Confirmed the new tests are effective guards by temporarily applying the proposed buggy change (world_y = 0): the suite then fails with 2 failures — the pre-existing y=80 test plus the new looking down at chunk from high camera test. Reverted afterward; final tree is pristine production code + tests only.

$ git diff --stat
 src/math_tests.zig | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

Acceptance criteria

• intersectsChunkRelative correctly culls chunks when camera is at various heights (y=0, 80, 200, 256)
• The bounding sphere actually contains the full chunk (all 256 Y levels) — verified by the independent circumscribed-sphere property test
• Existing frustum culling tests pass: `nix develop --command zig build test`
• New unit test verifies edge cases (camera at chunk bottom, middle, and top, plus high above)

[github-actions]

📋 Summary

This PR addresses issue #741, which claimed intersectsChunkRelative miscalculates the bounding-sphere center. The PR investigates and concludes the audit conflated world space with the renderer's camera-relative space, where the current -cam_y + 128 center is correct. No production code is changed; instead, three regression tests are added to src/math_tests.zig to pin the camera-relative semantics and prove the proposed world_y = 0 "fix" would incorrectly cull terrain when the camera is high.

The implementation fully satisfies the issue's acceptance criteria: it verifies correct culling across camera heights, confirms the sphere contains the full chunk column, and all tests pass.

📌 Review Metadata

• Reviewed Commit SHA: 4b7113196f089854bc1a2c80e3a6299a689d888f
• Reviewed PR: test(engine-math): pin intersectsChunkRelative camera-relative semantics (#741) #781
• Linked Issue: [Audit][High] Frustum.intersectsChunkRelative bounding sphere center is miscalculated #741 — addressed via test-only regression coverage demonstrating the reported bug does not exist.

🔴 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)

None identified. (The PR is labeled documentation, though the change is clearly a test addition; consider relabeling as test or automated-test for tracking.)

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	Tests are focused solely on frustum/chunk intersection semantics.
Open/Closed	8	Extends test coverage without modifying production behavior.
Liskov Substitution	N/A	No inheritance hierarchies involved.
Interface Segregation	N/A	No interfaces introduced.
Dependency Reversal	8	Uses existing public Frustum/Mat4/Vec3 APIs.
Average	8.3

🎯 Final Assessment

Overall Confidence Score: 95%

Confidence Breakdown:

• Code Quality: 95% — Clean, well-commented tests with clear intent and correct math.
• Completeness: 95% — Covers acceptance criteria and refutes the audit claim with a property test.
• Risk Level: 5% — Test-only change; no production behavior altered.
• Test Coverage: 100% — Adds three targeted regression tests; all pass.

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 — Test-only regression coverage is correct, well-reasoned, and all tests pass.

{
  "reviewed_sha": "4b7113196f089854bc1a2c80e3a6299a689d888f",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 95,
  "recommendation": "MERGE"
}

opencode session  |  github run