feat: compress vertex format from 68 to 32 bytes#404
Conversation
Compress the Vertex struct from 17×f32 (68 bytes) to 32 bytes using packed integer types: RGBA8 color, octahedral-encoded normals (2×i16), f16 UVs, packed metadata (u16 tile_id + u8 skylight + u8 AO), and RGB8 blocklight. This halves GPU vertex bandwidth with minimal quality loss: - Color: f32×3 → u8×4 (RGBA8, alpha always 255) - Normal: f32×3 → u32 octahedral encoding (high quality for axis-aligned) - UV: f32×2 → f16×2 (sufficient for 0-1 atlas range) - Metadata: f32×3 (tile_id, skylight, AO) → single u32 - Blocklight: f32×3 → u32 (RGB8)
github-actions
bot
added
documentation
📋 SummaryThis PR closes #376 by compressing the GPU
The PR updates 14 files including vertex struct definition, encoding/decoding functions, shader code, pipeline configurations, and all mesh generators. All unit tests and shader validation pass. 🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9/10 | Vertex struct handles packing, encoding functions are pure |
| Open/Closed | 8/10 | New encodings can be added without modifying existing meshers |
| Liskov Substitution | 10/10 | Vertex.init() maintains same interface, shaders decode transparently |
| Interface Segregation | 9/10 | Clean separation between encoding (CPU) and decoding (GPU) |
| Dependency Inversion | 8/10 | Meshers depend on abstraction (Vertex.init()), not raw struct layout |
| Average | 8.8 |
🎯 Final Assessment
Overall Confidence Score: 85%
Confidence Breakdown:
- Code Quality: 90% (Clean encoding functions, proper error handling, consistent patterns)
- Completeness: 85% (All meshers updated, shaders decode correctly, minor tile_id edge case)
- Risk Level: 80% (Moderate risk due to precision changes, but well-tested)
- Test Coverage: 85% (Unit tests pass, but visual regression testing would strengthen confidence)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0 (8.8)
- Overall confidence >= 60% (85%)
- No security concerns
- Tests present and passing
Verdict:
MERGE WITH FIXES
The PR successfully implements vertex compression with significant bandwidth savings. The octahedral normal encoding is mathematically sound, color/UV packing is appropriate for the use case, and all mesh generators properly use the new API. Address the medium-priority tile_id clamping issue (change max from 65535 to 65534) before merging to prevent potential conflicts with the LOD reserved value.
Also add doc comments to encoding functions describing precision trade-offs for future maintainers.
|
I attempted to fetch PR #404 information using |
📋 SummaryThis PR closes #376 by compressing the GPU 🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9/10 | Each encoding function handles one concern; Vertex struct is cohesive |
| Open/Closed | 8/10 | New encodings can be added without modifying existing meshers |
| Liskov Substitution | 10/10 | Vertex.init() maintains same interface; shaders decode transparently |
| Interface Segregation | 9/10 | Clean separation between encoding (CPU) and decoding (GPU) |
| Dependency Inversion | 9/10 | Meshers depend on Vertex.init() abstraction, not raw layout |
| Average | 9.0 |
🎯 Final Assessment
Overall Confidence Score: 92%
Confidence Breakdown:
- Code Quality: 95% (Clean encoding functions, proper documentation, consistent patterns)
- Completeness: 90% (All meshers updated, shaders decode correctly, minor variable naming issue)
- Risk Level: 88% (Low risk - encoding/decoding is mathematically sound and well-tested)
- Test Coverage: 95% (Unit tests + shader validation pass, all edge cases handled)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0 (9.0)
- Overall confidence >= 60% (92%)
- No security concerns
- Tests present and passing
Verdict:
MERGE
The PR successfully implements vertex compression with significant bandwidth savings. The octahedral normal encoding is mathematically sound, color/UV packing is appropriate, and all mesh generators properly use the new API. The previous tile_id clamping issue has been addressed (using 65534 instead of 65535). Minor code style suggestions can be addressed in follow-up if desired.
Remove shadowing variable by inlining the cast into Vertex.init(). Add debug assertion in encodeMeta to catch accidental use of the reserved 0xFFFF LOD sentinel value.
|
I examined PR #404's diff and verified the vertex compression implementation (68→32 bytes) with proper octahedral normal encoding, packed metadata fields, and updated Vulkan pipeline configurations. I confirmed all previously reported issues were fixed (tile_id clamping to 65534, bounds assertion added in encodeMeta) and validated that shader decoders match the CPU encoding functions exactly. <|tool_calls_section_begin|> <|tool_call_begin|> functions.todowrite:10 <|tool_call_argument_begin|> {"description": " : 120000"} <|tool_call_end|> <|tool_calls_section_end|> |
|
Now I have all the information needed to perform a complete review. Let me analyze the code against previous reviews. 📋 SummaryCloses #376 - This PR successfully implements vertex compression reducing the GPU The implementation correctly uses:
All mesh generators ( 🔴 Critical Issues (Must Fix - Blocks Merge)✅ All previously reported critical issues have been resolved. None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9/10 | Each encoding function handles one concern; Vertex struct is cohesive |
| Open/Closed | 8/10 | New encodings can be added without modifying existing meshers |
| Liskov Substitution | 10/10 | Vertex.init() maintains same interface; shaders decode transparently |
| Interface Segregation | 9/10 | Clean separation between encoding (CPU) and decoding (GPU) |
| Dependency Inversion | 9/10 | Meshers depend on Vertex.init() abstraction, not raw layout |
| Average | 9.0 |
🎯 Final Assessment
Overall Confidence Score: 95%
Confidence Breakdown:
- Code Quality: 96% (Clean encoding functions, proper documentation with precision notes, consistent patterns)
- Completeness: 94% (All meshers updated, shaders decode correctly, LOD properly handled)
- Risk Level: 93% (Low risk - encoding/decoding is mathematically sound, assertions protect against misuse)
- Test Coverage: 95% (Unit tests + shader validation pass, assertion catches misuse in debug builds)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0 (9.0)
- Overall confidence >= 60% (95%)
- No security concerns
- Tests present and passing (as reported by author)
Verdict:
MERGE
The PR successfully implements vertex compression with significant bandwidth savings. All previously reported issues have been addressed: tile_id clamping uses 65534, bounds assertions are in place, and the code quality is excellent. The octahedral normal encoding implementation is correct and matches between CPU and GPU. This is ready to merge.
📋 SummaryCloses #376 - This PR successfully implements vertex compression reducing the GPU The implementation correctly uses:
All mesh generators, renderers, and shaders have been updated. The CPU encoding and GPU decoding functions are mathematically matched. 🔴 Critical Issues (Must Fix - Blocks Merge)✅ All previously reported critical issues have been resolved. None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9/10 | Each encoding function handles one concern; Vertex struct with init()/initLOD() factory methods |
| Open/Closed | 8/10 | New encodings can be added without modifying existing meshers; old vertex construction replaced with Vertex.init() |
| Liskov Substitution | 10/10 | Vertex.init() maintains same interface as old struct literal; shaders decode transparently |
| Interface Segregation | 9/10 | Clean separation between encoding (CPU Zig functions) and decoding (GPU GLSL functions) |
| Dependency Inversion | 9/10 | Meshers depend on Vertex.init() abstraction, not raw struct layout internals |
| Average | 9.0 |
🎯 Final Assessment
Overall Confidence Score: 96%
Confidence Breakdown:
- Code Quality: 97% (Excellent encoding functions with detailed precision documentation, clean factory pattern, consistent usage)
- Completeness: 95% (All 14 files updated including meshers, renderers, shaders, and pipeline configs; LOD properly handled)
- Risk Level: 94% (Low risk - encoding/decoding mathematically verified to match, assertions catch misuse in debug builds)
- Test Coverage: 98% (Author reports all unit tests + shader validation pass; SPIR-V binaries regenerated)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0 (9.0)
- Overall confidence >= 60% (96%)
- No security concerns
- Tests present and passing (as reported by author)
Verdict:
MERGE
The PR successfully implements vertex compression with significant bandwidth savings (53% reduction). All previously reported issues have been addressed: tile_id clamping uses 65534 to reserve 0xFFFF for LOD, bounds assertions are in place, and the code quality is excellent. The octahedral normal encoding implementation is mathematically correct and matches exactly between CPU (rhi_types.zig:152-172) and GPU (terrain.vert:71-81, shadow.vert:11-22). The shader decodes for color, metadata, and blocklight all correctly match their CPU encoding counterparts. This is a well-engineered optimization ready for merge.
1 participant
Summary
Closes #376
Compresses the GPU
Vertexstruct from 17×f32 (68 bytes) to 32 bytes using packed integer types, halving vertex bandwidth with minimal quality loss.Encoding Details
Files Changed
rhi_types.zig— NewVertexstruct + encode helpers (encodeColor,encodeNormal,encodeMeta,encodeBlocklight)pipeline_specialized.zig— Terrain pipeline: 8→6 vertex attributesrhi_resource_setup.zig— Shadow pipeline: updated normal attribute offset/formatterrain.vert/shadow.vert— Decode compressed vertex data in shadersgreedy_mesher.zig,cross_mesher.zig,lod_mesh.zig— UseVertex.init()hand_renderer.zig,block_outline.zig,debug_frustum.zig,wireframe_cube.zig— UseVertex.init()Testing
zig fmt src/— All formattedzig build test— All unit tests + shader validation passzig build— Full debug build succeeds