feat: integrate GPU frustum culling into WorldRenderer (#379)#409
feat: integrate GPU frustum culling into WorldRenderer (#379)#409MichaelFisher1997 merged 6 commits intodevfrom
Conversation
Replace the CPU-side frustum culling loop in WorldRenderer.render() with GPU compute shader dispatch via CullingSystem. The culling shader now writes visible chunk indices instead of placeholder DrawIndirectCommands. CPU reads back visible indices and builds proper MDI draw commands with real mesh data. - Updated culling.comp to write compact visible index list - CullingSystem uses host-visible index buffers for direct readback - WorldRenderer.render() uses GPU culling with CPU fallback - Shadow pass retains CPU culling (per-chunk model matrix needed) - Fixes import paths in culling_system.zig for proper module resolution - Passes RHI to WorldRenderer.init() for CullingSystem creation - Runtime detection: falls back to CPU if GPU culling init fails
github-actions
bot
added
documentation
|
Now I have a complete picture of the changes. Let me provide my review: 📋 SummaryLinked Issue: Closes #379 - This PR implements GPU frustum culling integration as requested. PR Purpose: Integrates the existing GPU compute frustum culling system (CullingSystem) into WorldRenderer.render(), replacing the CPU-side frustum culling loop with a GPU dispatch pipeline while preserving CPU fallback. Overall Quality: Good architecture with proper fallback handling and clean separation between GPU/CPU paths. However, there's a critical buffer offset bug that will cause incorrect rendering. 🔴 Critical Issues (Must Fix - Blocks Merge)[CRITICAL] // Change line 223 from:
@memcpy(out[0..copy_count], src[2 .. 2 + copy_count]);
// To:
@memcpy(out[0..copy_count], src[4 .. 4 + copy_count]);
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8/10 | Clean separation: CullingSystem handles GPU compute, WorldRenderer orchestrates |
| Open/Closed | 7/10 | Good fallback preserves existing CPU path; GPU path is additive |
| Liskov Substitution | 9/10 | CPU and GPU culling produce equivalent results (once offset bug fixed) |
| Interface Segregation | 8/10 | CullingSystem has focused API: init, updateAABBData, dispatch, read results |
| Dependency Inversion | 7/10 | Depends on concrete RHI but uses proper abstraction for buffer management |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 35%
How to interpret: The critical buffer offset bug would cause severe rendering issues. The synchronization issue compounds the problem.
Confidence Breakdown:
- Code Quality: 70% (Good structure but critical bugs present)
- Completeness: 60% (Implementation present but has functional bugs)
- Risk Level: 85% (High risk due to GPU/CPU synchronization and memory offset issues)
- Test Coverage: 30% (Build passes but runtime behavior would be wrong)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- Tests present and passing (build tests pass)
Verdict:
DO NOT MERGE - Critical buffer offset bug at line 223 will cause incorrect chunk indices to be read, leading to wrong geometry being rendered. Must fix before merge.
- Fix incorrect buffer offset in readVisibleIndices (src[2] -> src[4]) The shader has 4 u32 header (count + 3 padding), not 2 - Fix GPU/CPU sync: read back previous frame results before dispatching current frame. beginFrame waits on fence so previous frame's data is ready when render() is called - Log specific error when GPU culling init fails - Break long AABB construction lines for readability
📋 SummaryLinked Issue: Closes #379 - Integrates GPU compute frustum culling into WorldRenderer. PR Purpose: Replaces CPU-side frustum culling with GPU compute dispatch using CullingSystem, while preserving CPU fallback for robustness. The shader now writes compact visible chunk indices instead of placeholder DrawIndirectCommands. Overall Quality: Good architecture with proper double-buffering and fallback handling. However, a critical synchronization bug was introduced that will cause wrong chunks to be rendered. Previous Review Issues Status:
🔴 Critical Issues (Must Fix - Blocks Merge)[CRITICAL] fn renderGpuCull(self: *WorldRenderer, view_proj: Mat4, camera_pos: Vec3, pc_x: i64, pc_z: i64, r_dist: i64) void {
const cs = self.culling_system orelse unreachable;
const fi = self.query.getFrameIndex();
// FIRST: Read back previous frame's results while lookup is still valid
const prev_visible_count = cs.readVisibleCount(fi);
self.gpu_visible_indices.clearRetainingCapacity();
if (prev_visible_count > 0) {
self.gpu_visible_indices.resize(self.allocator, prev_visible_count) catch return;
cs.readVisibleIndices(fi, prev_visible_count, self.gpu_visible_indices.items);
const limit = @min(@as(usize, @intCast(prev_visible_count)), self.gpu_visible_indices.items.len);
for (self.gpu_visible_indices.items[0..limit]) |idx| {
if (idx < self.chunk_lookup.items.len) {
self.visible_chunks.append(self.allocator, self.chunk_lookup.items[idx]) catch continue;
}
}
}
// THEN: Clear and populate for current frame
self.aabb_data.clearRetainingCapacity();
self.chunk_lookup.clearRetainingCapacity();
// ... rest of chunk population logic ...
}
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8/10 | Clean separation: CullingSystem handles GPU compute, WorldRenderer orchestrates |
| Open/Closed | 7/10 | Good fallback preserves existing CPU path; GPU path is additive |
| Liskov Substitution | 9/10 | CPU and GPU culling produce equivalent results (once sync bug fixed) |
| Interface Segregation | 8/10 | CullingSystem has focused API: init, updateAABBData, dispatch, read results |
| Dependency Inversion | 7/10 | Depends on concrete RHI but uses proper abstraction for buffer management |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 25%
How to interpret: The critical chunk_lookup synchronization bug would cause severe rendering issues with wrong chunks being displayed.
Confidence Breakdown:
- Code Quality: 75% (Good structure but critical sync bug present)
- Completeness: 80% (Implementation complete but has functional bug)
- Risk Level: 90% (High risk due to synchronization bug)
- Test Coverage: 40% (Build passes but runtime behavior would be wrong)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- Tests present and passing (build tests pass)
Verdict:
DO NOT MERGE - Critical chunk_lookup synchronization bug at lines 301-346 will cause visible indices from the previous frame to reference the wrong chunks, leading to incorrect geometry being rendered. The readback must happen before clearing chunk_lookup.
|
Now I have a complete picture of the changes. Let me provide my review: 📋 SummaryLinked Issue: Closes #379 - This PR implements GPU frustum culling integration as requested. PR Purpose: Integrates the existing GPU compute frustum culling system (CullingSystem) into WorldRenderer.render(), replacing the CPU-side frustum culling loop with a GPU dispatch pipeline while preserving CPU fallback. Overall Quality: Good architecture with proper fallback handling and clean separation between GPU/CPU paths. However, there's a critical buffer offset bug that will cause incorrect rendering. 🔴 Critical Issues (Must Fix - Blocks Merge)[CRITICAL] // Change line 223 from:
@memcpy(out[0..copy_count], src[2 .. 2 + copy_count]);
// To:
@memcpy(out[0..copy_count], src[4 .. 4 + copy_count]);
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8/10 | Clean separation: CullingSystem handles GPU compute, WorldRenderer orchestrates |
| Open/Closed | 7/10 | Good fallback preserves existing CPU path; GPU path is additive |
| Liskov Substitution | 9/10 | CPU and GPU culling produce equivalent results (once offset bug fixed) |
| Interface Segregation | 8/10 | CullingSystem has focused API: init, updateAABBData, dispatch, read results |
| Dependency Inversion | 7/10 | Depends on concrete RHI but uses proper abstraction for buffer management |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 35%
How to interpret: The critical buffer offset bug would cause severe rendering issues. The synchronization issue compounds the problem.
Confidence Breakdown:
- Code Quality: 70% (Good structure but critical bugs present)
- Completeness: 60% (Implementation present but has functional bugs)
- Risk Level: 85% (High risk due to GPU/CPU synchronization and memory offset issues)
- Test Coverage: 30% (Build passes but runtime behavior would be wrong)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- Tests present and passing (build tests pass)
Verdict:
DO NOT MERGE - Critical buffer offset bug at line 223 will cause incorrect chunk indices to be read, leading to wrong geometry being rendered. Must fix before merge.
📋 SummaryLinked Issue: Closes #379 - GPU frustum culling integration. Previous Review Issues Status:
PR Purpose: Integrates GPU compute frustum culling into WorldRenderer.render(), replacing CPU-side frustum culling with GPU dispatch while preserving CPU fallback. Overall Quality: Good architecture with proper double-buffering for GPU resources, but the chunk_lookup synchronization bug is a critical flaw that would cause incorrect rendering. 🔴 Critical Issues (Must Fix - Blocks Merge)[CRITICAL] // In WorldRenderer struct:
chunk_lookup: [MAX_FRAMES_IN_FLIGHT]std.ArrayListUnmanaged(*ChunkData),
// In renderGpuCull:
const fi = self.query.getFrameIndex();
const prev_fi = (fi + MAX_FRAMES_IN_FLIGHT - 1) % MAX_FRAMES_IN_FLIGHT;
// Read back using PREVIOUS frame's lookup
const prev_visible_count = cs.readVisibleCount(prev_fi);
// ... read indices ...
for (indices) |idx| {
if (idx < self.chunk_lookup[prev_fi].items.len) {
self.visible_chunks.append(self.allocator, self.chunk_lookup[prev_fi].items[idx]) catch continue;
}
}
// Clear CURRENT frame's lookup and populate for next frame
self.chunk_lookup[fi].clearRetainingCapacity();
// ... populate self.chunk_lookup[fi] ...
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8/10 | Clean separation: CullingSystem handles GPU compute, WorldRenderer orchestrates |
| Open/Closed | 7/10 | Good fallback preserves CPU path; GPU path is additive |
| Liskov Substitution | 6/10 | CPU and GPU paths produce equivalent results in theory, but chunk_lookup bug breaks this |
| Interface Segregation | 8/10 | CullingSystem has focused API: init, updateAABBData, dispatch, read results |
| Dependency Inversion | 7/10 | Depends on concrete RHI but uses proper abstraction for buffer management |
| Average | 7.2 |
🎯 Final Assessment
Overall Confidence Score: 25%
How to interpret: The critical chunk_lookup synchronization bug would cause severe rendering issues with wrong chunks being displayed. This is a fundamental architectural issue with the double-buffering scheme.
Confidence Breakdown:
- Code Quality: 75% (Good structure with helper functions and error handling)
- Completeness: 80% (Implementation is complete but has critical functional bug)
- Risk Level: 95% (Very high risk due to synchronization bug)
- Test Coverage: 40% (Build and shader tests pass, but no runtime behavior tests)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- Tests present and passing (build tests pass)
Verdict:
DO NOT MERGE - The critical chunk_lookup synchronization bug at lines 312-360 will cause visible indices from the previous frame to reference the wrong chunks. The chunk_lookup array must be double-buffered to match the GPU buffer indexing scheme, or the architecture must be changed to not rely on persistent lookup tables across frames.
|
Now let me analyze the current code against previous reviews to identify what's fixed and what remains. 📋 SummaryLinked Issue: Closes #379 - GPU frustum culling integration into WorldRenderer. Previous Review Issues Status:
PR Purpose: Integrates GPU compute frustum culling into WorldRenderer.render() with proper double-buffering and CPU fallback. Overall Quality: Good architecture with clean separation between GPU/CPU paths. Critical synchronization issue from previous reviews appears to be addressed through proper double-buffering of chunk_lookup. 🔴 Critical Issues (Must Fix - Blocks Merge)None identified - all previously reported critical issues have been resolved.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9/10 | Clean separation: CullingSystem handles GPU compute, WorldRenderer orchestrates, chunkAABB is pure function |
| Open/Closed | 8/10 | Excellent fallback preserves CPU path; GPU path is additive and optional |
| Liskov Substitution | 9/10 | CPU and GPU culling produce equivalent results with same output format |
| Interface Segregation | 9/10 | CullingSystem has focused API: init, updateAABBData, dispatch, read results |
| Dependency Inversion | 8/10 | Uses RHI abstraction properly, depends on interfaces not implementations |
| Average | 8.6 |
🎯 Final Assessment
Overall Confidence Score: 75%
How to interpret: Implementation is solid with good architecture. Minor synchronization concern that depends on external fence management.
Confidence Breakdown:
- Code Quality: 85% (Excellent structure, proper double-buffering, clean helper functions)
- Completeness: 90% (Full implementation with fallback, all previous issues fixed)
- Risk Level: 65% (Moderate risk - synchronization depends on calling code)
- Test Coverage: 60% (Build tests pass, runtime behavior needs verification)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0 (8.6)
- Overall confidence >= 60% (75%)
- No security concerns
- Tests present and passing
Verdict:
MERGE WITH FIXES - The implementation is well-architected and all previously identified critical bugs have been fixed. The high-priority synchronization issue should be verified before merge (check that frame fences are waited on before calling render). If external fence management is confirmed, this is ready to merge.
📋 SummaryLinked Issue: Closes #379 - GPU frustum culling integration. Previous Review Issues Status:
PR Purpose: Integrates GPU compute frustum culling into WorldRenderer.render() with proper double-buffering and CPU fallback. Overall Quality: Well-architected implementation with clean separation between GPU/CPU paths. All previously identified critical issues have been resolved. The code follows Zig conventions and properly handles memory management. 🔴 Critical Issues (Must Fix - Blocks Merge)None identified - all previously reported critical issues have been resolved.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9/10 | Clean separation: CullingSystem handles GPU compute, WorldRenderer orchestrates, chunkAABB is pure function |
| Open/Closed | 8/10 | Excellent fallback preserves CPU path; GPU path is additive and optional |
| Liskov Substitution | 9/10 | CPU and GPU culling produce equivalent results with same output format |
| Interface Segregation | 9/10 | CullingSystem has focused API: init, updateAABBData, dispatch, read results |
| Dependency Inversion | 8/10 | Uses RHI abstraction properly, depends on interfaces not implementations |
| Average | 8.6 |
🎯 Final Assessment
Overall Confidence Score: 75%
How to interpret: Implementation is solid with good architecture. Minor synchronization concern that depends on external fence management.
Confidence Breakdown:
- Code Quality: 85% (Excellent structure, proper double-buffering, clean helper functions)
- Completeness: 90% (Full implementation with fallback, all previous issues fixed)
- Risk Level: 65% (Moderate risk - synchronization depends on calling code)
- Test Coverage: 60% (Build tests pass, runtime behavior needs verification)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0 (8.6)
- Overall confidence >= 60% (75%)
- No security concerns
- Tests present and passing
Verdict:
MERGE WITH FIXES - The implementation is well-architected and all previously identified critical bugs have been fixed. The high-priority synchronization issue should be verified before merge (check that frame fences are waited on before calling render). If external fence management is confirmed, this is ready to merge.
1 participant
Summary
Closes #379
Integrates the existing GPU compute frustum culling (CullingSystem from #403) into WorldRenderer.render(), replacing the CPU-side frustum culling loop with a GPU dispatch pipeline.
Changes
assets/shaders/vulkan/culling.comp
src/engine/graphics/vulkan/culling_system.zig
src/world/world_renderer.zig
src/world/world.zig
Testing
Performance Impact
At 128 chunk render distance (~65K positions), the CPU culling loop touches every position. With GPU culling only renderable chunks are uploaded as AABB data (~10K), GPU frustum test runs in parallel across workgroups of 64, and CPU only processes the visible subset (~2-4K) to build draw commands.