feat: add GPU LOD culling and indirect compaction

[MichaelFisher1997]

Summary

• add a dedicated pre-render Vulkan compute pass for LOD distance/frustum culling
• compact terrain and water instances and indirect commands in one GPU visibility pass
• add bounded indirect-count submission with capability, workload-threshold, and overflow CPU fallbacks
• add delayed CPU/GPU visibility validation, diagnostics, and compute/barrier timing
• synchronize host-written candidate data before compute and exercise GPU ABI/backend tests from the main test aggregator

Optional depth-pyramid occlusion remains deferred until the compute + MDI path is benchmarked and stable.

Closes #918

Verification

• nix develop --command zig build test
• nix develop --command zig build
• nix develop --command zig build -Doptimize=ReleaseFast
• ZIGCRAFT_LOD_GPU_CULLING=1 ZIGCRAFT_LOD_GPU_CULLING_THRESHOLD=1 ZIGCRAFT_LOD_GPU_CULLING_VALIDATE=1 nix develop --command zig build run -Dskip-present -Dauto-world=normal -Dstartup-diagnostic-seconds=5
• ReleaseFast headless benchmark produced zig-out/lod-gpu-culling-smoke.json (1,383 frames, 276.59 average FPS); benchmark reported the medium-preset GPU-memory SLO breach at 2647.64 MB > 2400 MB

[github-actions]

Summary

This PR implements Phase 3 of the LOD roadmap: a Vulkan compute pass that frustum/distance culls LOD candidates and compacts visible terrain/water instances into indirect draw streams. It wires the new ILODCullingSystem through the RHI, adds a CPU fallback threshold, and includes an opt-in CPU/GPU validation mode. The architecture is clean, but I found a real synchronization bug and a stale/inaccessible ABI test that need to be fixed before merging.

Metadata

• PR: feat: add GPU LOD culling and indirect compaction #927 — feat: add GPU LOD culling and indirect compaction
• Author: MichaelFisher1997
• Branch: feature/lod-gpu-culling
• Base: origin/dev
• Head SHA: 44d2472d1e592988498dee5b59da14fe80adddd1
• Issue: Closes LOD GPU roadmap Phase 3: GPU culling and indirect command generation #918 (LOD GPU roadmap Phase 3)
• Automated test writer: No (no automated-test label)
• Previous automated reviews: None

PR Description & Scope

The change adds:

• modules/engine-rhi/src/culling.zig: public LODCullCandidate, LODCullDispatch, LODCullDiagnostics, and ILODCullingSystem RHI interface.
• modules/engine-graphics/src/vulkan/lod_culling_system.zig: Vulkan compute backend, descriptor/pipeline setup, and CPU reference validation.
• assets/shaders/vulkan/lod_culling.comp: compute shader that frustum/distance culls, atomically counts per-LOD visible entries, and writes instance/indirect/visible-id buffers.
• modules/engine-graphics/src/vulkan/rhi_draw_submission.zig: drawIndirectCount path using the GPU-produced count buffer.
• modules/world-lod/src/lod_renderer.zig + modules/world-runtime/src/world_renderer.zig: CPU/GPU path selection and render-graph integration.

This matches the issue acceptance criteria: GPU culling, CPU fallback, validation mode, separate terrain/water streams, and compact MDI output.

Issues

High

1. Host writes to the candidate buffer are not synchronized before the compute shader reads them.
   LODCullingSystem.dispatch copies candidate data into host-visible candidates[frame_index] via @memcpy at modules/engine-graphics/src/vulkan/lod_culling_system.zig:105, then inserts a host-to-compute memory barrier at modules/engine-graphics/src/vulkan/lod_culling_system.zig:118-122. That barrier uses srcAccessMask = VK_ACCESS_TRANSFER_WRITE_BIT only, omitting VK_ACCESS_HOST_WRITE_BIT. Because candidates is written by the CPU, the compute dispatch may read stale data, producing incorrect culling.
   Fix: Add c.VK_ACCESS_HOST_WRITE_BIT to transfer_to_compute.srcAccessMask.

2. The CPU fallback path is not taken when the device lacks drawIndirectCount support.
   drawIndirectCount returns false if !ctx.vulkan_device.draw_indirect_count (modules/engine-graphics/src/vulkan/rhi_draw_submission.zig:187), but the renderer appears to call the GPU path unconditionally once candidates exceed the threshold. On devices without VK_KHR_draw_indirect_count, this would silently fail to draw LODs rather than falling back to the CPU cull/draw path.
   Fix: Ensure LODRenderer.useGpuCulling also checks the device capability and falls back to CPU when drawIndirectCount is unavailable.

Medium

3. The LODCullCandidate ABI test is wrong and not exercised.
   modules/engine-rhi/src/culling.zig:123-126 expects @sizeOf(LODCullCandidate) == 144, but the actual size is 160 bytes (verified by compile-time introspection: 16 + 16 + 64 + 16 + 16 + 16 + 16). This is because the struct includes two DrawIndirectCommand fields and a [4]u32 pad, which the test did not account for. Furthermore, the test is not reached by src/tests.zig because the aggregator never imports engine-rhi’s culling module, so CI does not compile it.
   Fix: Update the expected size to 160 and add an import in src/tests.zig (e.g., _ = @import("engine-rhi").culling;) so the test runs.

4. New tests in lod_culling_system.zig are also not reached.
   The same lazy-import behavior means the tests at modules/engine-graphics/src/vulkan/lod_culling_system.zig:405-424 are not compiled into the unit suite unless src/tests.zig forces engine-graphics.rhi_vulkan or the culling system file to be analyzed.

Low

5. SPIR-V size baseline is stale.
   docs/shaders/spirv-sizes.json:15 records lod_culling.comp as 9544 bytes, but zig build test reports 10312 bytes (≈8% growth, within the 10% tolerance). The baseline should be refreshed with scripts/update_spirv_baseline.sh.

6. Release build and headless verification not demonstrated.
   Per the project checklist, a performance-critical rendering change should also be verified with nix develop --command zig build -Doptimize=ReleaseFast and a headless screenshot/benchmark. I did not see evidence of this in the PR description.

SOLID Score

6.5 / 10

• Single Responsibility: Good. The culling backend is isolated in its own file and exposed only through the RHI interface.
• Open/Closed: Good. The new ILODCullingSystem extends the RHI without modifying existing draw paths.
• Liskov / Interface Segregation: Good. The VTable is narrow and the CPU fallback preserves the existing interface contract.
• Dependency Inversion: Good. World code depends on the RHI interface, not Vulkan directly.
• Testability / Correctness: Poor. The new ABI test is wrong and not compiled, and the host-write barrier is missing, which means the feature is not actually validated by CI and may produce incorrect results.

Final Assessment

The PR is architecturally sound and matches the feature scope, but the missing HOST_WRITE barrier is a correctness bug that must be fixed before merge. The ABI test also needs to be corrected and wired into the test aggregator so the CPU/GPU struct contract is actually enforced. With those fixes, the change is mergeable.

Machine Readable Verdict

request_changes

opencode session  |  github run

[github-actions]

📋 Summary

This PR implements Phase 3 of the LOD GPU culling roadmap: a Vulkan compute pass that frustum/distance-culls LOD candidates and compacts visible terrain/water instances into indirect draw streams. It wires the new ILODCullingSystem through the RHI, adds CPU fallbacks, and includes CPU/GPU validation diagnostics.

Linked issue: Closes #918.

The previous automated review identified five code issues (two High, two Medium, one Low). All of them have been fixed in the current head. However, a new Critical Vulkan buffer-usage bug was introduced in the latest revision that must be fixed before merge.

---

📌 Review Metadata

• Reviewed Commit SHA: 5c5765008960434c14f56aacd1852bca7e1f0a1d
• Reviewed PR: feat: add GPU LOD culling and indirect compaction #927
• Linked Issue: LOD GPU roadmap Phase 3: GPU culling and indirect command generation #918 (LOD GPU roadmap Phase 3)
• Automated test writer: No (no automated-test label)

---

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved. (No critical issues were reported in the previous review.)

New Critical Issue

[CRITICAL] modules/engine-graphics/src/vulkan/lod_culling_system.zig:69 - Count buffer lacks VK_BUFFER_USAGE_INDIRECT_BUFFER_BIT
Confidence: High
Description: The atomic counter buffer is created with .storage usage:

self.counters[i] = try ctx.resources.createBuffer(MAX_LOD_LEVELS * 2 * @sizeOf(u32), .storage);

In resource_manager.zig, .storage maps to STORAGE_BUFFER | TRANSFER_SRC | TRANSFER_DST | VERTEX_BUFFER, but not INDIRECT_BUFFER. This same buffer is then passed to vkCmdDrawIndirectCountKHR as the count buffer in rhi_draw_submission.zig:220. Vulkan requires a count buffer used by drawIndirectCount to have VK_BUFFER_USAGE_INDIRECT_BUFFER_BIT.
Impact: Invalid Vulkan usage; likely validation-layer errors, potential driver crash or undefined behavior, and the GPU culling path will not work correctly on strict drivers.
Suggested Fix: Change the usage to .indirect, which already maps to INDIRECT_BUFFER | TRANSFER_DST | STORAGE_BUFFER:

self.counters[i] = try ctx.resources.createBuffer(MAX_LOD_LEVELS * 2 * @sizeOf(u32), .indirect);

---

⚠️ High Priority Issues (Should Fix)

None identified.

---

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] modules/world-lod/src/lod_renderer.zig:264 - CPU fallback uses a non-culled projection
Confidence: Medium
Description: prepareFrame builds the visibility projection with use_frustum=false and max_distance_chunks=null:

self.buildVisibilityProjection(meshes, regions, config, view_proj, camera_pos, chunk_checker, checker_ctx, false, null, null, null) catch ...

This is correct when the GPU shader performs the culling, but if renderGpuCulledLayer fails at draw time (e.g., a mesh becomes non-pooled, drawIndirectCount returns false), renderProjectedLayer falls back to the CPU path and renders the same broad projection without frustum or distance culling.
Impact: A GPU-path failure can cause a large, non-culled CPU draw list, producing a severe frame-time spike.
Suggested Fix: If the GPU path fails at render time, invalidate projection_frame so renderFrame rebuilds the projection with use_frustum=true and the correct max_distance_chunks, or build a separate frustum-culled projection for the CPU fallback.

---

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/world-lod/src/lod_renderer.zig:153 - GPU culling system allocated unconditionally
Confidence: Low
Description: The LODCullingSystem is created in LODRenderer.init even when gpu_culling_requested is false, allocating descriptor pools, pipelines, and GPU buffers that may never be used.
Suggested Fix: Only create the system when gpu_culling_requested is true, or defer creation until the first frame that requests it.

[LOW] assets/shaders/vulkan/lod_culling.comp:42 - Shader hardcodes max LOD level
Confidence: Low
Description: The shader uses min(c.lod_and_padding.x, 4u) which is only correct while MAX_LOD_LEVELS == 5.
Suggested Fix: Pass MAX_LOD_LEVELS - 1 via a specialization constant or push constant, or define a named constant shared with the host code.

---

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Culling backend is isolated; render graph prepass is cleanly separated.
Open/Closed	8	New ILODCullingSystem extends RHI without modifying existing draw paths.
Liskov Substitution	8	CPU fallback preserves the existing renderer interface contract.
Interface Segregation	8	The ILODCullingSystem VTable is narrow and focused.
Dependency Inversion	8	World code depends on the RHI interface, not Vulkan directly.
Average	8.0	Strong architecture; minor tightness around GPU/CPU fallback sharing one projection.

---

🎯 Final Assessment

Overall Confidence Score: 75%

Confidence Breakdown:

• Code Quality: 80%
• Completeness: 85%
• Risk Level: 60% (the missing INDIRECT_BUFFER_BIT is a concrete correctness risk)
• Test Coverage: 70% (unit tests cover CPU validation and ABI; no GPU-driven integration test for the actual count-buffer submission)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 80% (blocked by critical issue)
• No security concerns
• Tests present and passing (nix develop --command zig build test passed)

Verdict:

MERGE WITH FIXES

The count-buffer usage bug is a one-line fix; once resolved, the PR is architecturally sound and ready to merge.

---

{
  "reviewed_sha": "5c5765008960434c14f56aacd1852bca7e1f0a1d",
  "critical_issues": 1,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 75,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[github-actions]

📋 Summary

PR #927 closes #918 (LOD GPU roadmap Phase 3). The implementation adds a dedicated Vulkan compute prepass for LOD distance/frustum culling, compacts visible terrain and water instances into indirect command streams, and adds CPU fallback thresholds plus an opt-in CPU/GPU validation mode.

I verified the issue acceptance criteria are met:

• Real GPU culling is gated by ZIGCRAFT_LOD_GPU_CULLING and dispatches a compute pass.
• ZIGCRAFT_LOD_GPU_CULLING_VALIDATE compares GPU output against a CPU reference.
• Separate terrain and water command/instance streams are generated.
• Small workloads fall back to the CPU path via gpu_culling_threshold.
• Overflow, unsupported-device, and pooled-buffer-unavailable paths fall back to CPU rendering.

All issues reported in the two prior automated reviews have been resolved. nix develop --command zig build test and nix develop --command zig build -Doptimize=ReleaseFast both pass.

📌 Review Metadata

• Reviewed Commit SHA: 726ed63d6c9ed8a720153a9ded5b374d91dbefab
• Reviewed PR: feat: add GPU LOD culling and indirect compaction #927
• Linked Issue: LOD GPU roadmap Phase 3: GPU culling and indirect command generation #918 (LOD GPU roadmap Phase 3)
• Automated test writer: No

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

No new critical issues identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved.

• ✅ [FIXED] Host-write barrier on the candidate buffer now includes VK_ACCESS_HOST_WRITE_BIT at modules/engine-graphics/src/vulkan/lod_culling_system.zig:120.
• ✅ [FIXED] CPU fallback now checks device capabilities: prepareFrame requires supportsIndirectFirstInstance() and supportsIndirectCount() at modules/world-lod/src/lod_renderer.zig:321, and renderGpuCulledLayer repeats the check at line 656.

No new high priority issues identified.

💡 Medium Priority Issues (Nice to Fix)

✅ All previously reported medium priority issues have been resolved.

• ✅ [FIXED] LODCullCandidate ABI test now expects the correct 160-byte size at modules/engine-rhi/src/culling.zig:124.
• ✅ [FIXED] New GPU culling tests are now reached from the test aggregator via _ = @import("engine-graphics").lod_culling_system; and _ = @import("engine-rhi").culling; at src/tests.zig:50-52.
• ✅ [FIXED] CPU fallback no longer uses a non-culled projection; renderFrame invalidates the GPU projection and rebuilds with use_frustum/max_distance_chunks at modules/world-lod/src/lod_renderer.zig:246-260.

No new medium priority issues identified.

ℹ️ Low Priority Suggestions (Optional)

✅ All previously reported low priority issues have been resolved.

• ✅ [FIXED] SPIR-V baseline updated to 10360 bytes for lod_culling.comp in docs/shaders/spirv-sizes.json.
• ✅ [FIXED] ReleaseFast build verification included and passing.
• ✅ [FIXED] Shader no longer hardcodes min(..., 4u); it uses a named specialization constant MAX_LOD_LEVELS at assets/shaders/vulkan/lod_culling.comp:3.
• ✅ [FIXED] LODCullingSystem is only created when ZIGCRAFT_LOD_GPU_CULLING is enabled at modules/world-lod/src/lod_renderer.zig:153-160.

[LOW] modules/engine-rhi/src/rhi_contract_tests.zig - Missing contract assertions for new RHI surface
Confidence: Low
Description: The new drawIndirectCount method on IGraphicsCommandEncoder/RenderContext and the new ILODCullingSystem interface are not declared in rhi_contract_tests.zig. The functionality is covered by culling.zig ABI tests and rhi_tests.zig mock tests, but the contract suite would be more complete with explicit declarations.
Impact: No runtime risk; minor test-suite completeness gap.
Suggested Fix: Add @hasDecl assertions for IGraphicsCommandEncoder.drawIndirectCount, RenderContext.drawIndirectCount, and rhi.ILODCullingSystem.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Culling backend is isolated in lod_culling_system.zig; render-graph prepass is separate; CPU fallback remains in lod_renderer.zig.
Open/Closed	8	New ILODCullingSystem extends the RHI without modifying existing draw paths.
Liskov Substitution	8	CPU fallback preserves the existing renderer interface contract.
Interface Segregation	8	ILODCullingSystem VTable is narrow and focused.
Dependency Inversion	8	World code depends on the RHI interface, not Vulkan directly.
Average	8.0	Strong architecture; minor test-suite completeness gap noted.

🎯 Final Assessment

Overall Confidence Score: 90%

Confidence Breakdown:

• Code Quality: 90%
• Completeness: 90%
• Risk Level: 80%
• Test Coverage: 85%

Merge Readiness:

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

Verdict:

MERGE

All previously reported issues are fixed, the new GPU culling path is cleanly isolated behind the RHI, tests and ReleaseFast build pass, and the implementation satisfies the #918 acceptance criteria.

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

opencode session  |  github run