refactor: remove legacy lighting paths#910
Conversation
kcov coverageLine coverage ran for this PR and uploaded a non-blocking report artifact named |
📋 SummaryPR #910 references #904 ([Lighting Overhaul][Phase 5] Remove legacy paths and enforce regression gates). This PR implements a subset of that issue: it removes the legacy entrance-bounce lighting storage, propagation, mesh attributes, shader varyings, and debug controls; drops unused PCSS and sun-shaft code paths; locks GPU meshing to the CPU fallback; adds a terrain vertex ABI test; refreshes SPIR-V artifacts; and documents the production lighting architecture. It does not fully close #904: acceptance items like deterministic visual regression captures, benchmark thresholds, and preset tuning are not included in this change. The PR scope is accurately described in the title/body, so leaving #904 open is appropriate.
📌 Review Metadata
This is a focused cleanup/refactor: 120 insertions, 926 deletions across 50 files. The shader/CPU vertex contract stays aligned, and the new 🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9 | Removes orthogonal responsibilities (entrance bounce, PCSS, sun shafts) from lighting, meshing, and settings modules. |
| Open/Closed | 7 | Mostly deletion; the vertex ABI test freezes the CPU-side contract, which is appropriate for a stabilization phase. |
| Liskov Substitution | 8 | Interface changes (ShadowParams, Vertex, DebugLightInfo) are propagated consistently across all implementers and tests. |
| Interface Segregation | 8 | ShadowParams and FrameRenderParams are slimmed; settings no longer carry dead toggles. |
| Dependency Inversion | 7 | No new abstractions introduced; existing RHI contracts remain the boundary. |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 85%
Confidence Breakdown:
- Code Quality: 85% (clean deletions, consistent renames, shader/CPU ABI aligned, tests updated)
- Completeness: 70% (fulfills the stated PR scope, but only a partial step toward [Lighting Overhaul][Phase 5] Remove legacy paths and enforce regression gates #904's full acceptance criteria)
- Risk Level: 30% (removes code paths, but GPU meshing is explicitly disabled and the fallback is production-tested)
- Test Coverage: 80% (relevant unit tests updated, new ABI test added, shader validation passes)
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- Tests present and passing
Verdict:
MERGE — The PR is a clean, well-scoped refactor that removes dead lighting paths, keeps the production vertex/shader contract consistent, and passes the full test suite including shader validation.
{
"reviewed_sha": "724310c47bcacfaf33c0d14ab60acd735977ee4f",
"critical_issues": 0,
"high_priority_issues": 0,
"medium_priority_issues": 0,
"overall_confidence_score": 85,
"recommendation": "MERGE"
}
Summary
Verification
Compiler: 0.16.0
Compiler: 0.16.0
SPIR-V size: assets/shaders/vulkan/bloom_downsample.frag 9764 bytes (73ms compile)
SPIR-V size: assets/shaders/vulkan/bloom_downsample.vert 1160 bytes (69ms compile)
SPIR-V size: assets/shaders/vulkan/bloom_upsample.frag 3956 bytes (65ms compile)
SPIR-V size: assets/shaders/vulkan/bloom_upsample.vert 1160 bytes (69ms compile)
SPIR-V size: assets/shaders/vulkan/culling.comp 11352 bytes (66ms compile)
SPIR-V size: assets/shaders/vulkan/debug_shadow.frag 836 bytes (67ms compile)
SPIR-V size: assets/shaders/vulkan/debug_shadow.vert 1344 bytes (70ms compile)
SPIR-V size: assets/shaders/vulkan/depth_pyramid.comp 2936 bytes (66ms compile)
SPIR-V size: assets/shaders/vulkan/fxaa.frag 5916 bytes (69ms compile)
SPIR-V size: assets/shaders/vulkan/fxaa.vert 1160 bytes (68ms compile)
SPIR-V size: assets/shaders/vulkan/g_pass.frag 6912 bytes (67ms compile)
SPIR-V size: assets/shaders/vulkan/lpv_inject.comp 4844 bytes (67ms compile)
SPIR-V size: assets/shaders/vulkan/lpv_propagate.comp 8316 bytes (69ms compile)
SPIR-V size: assets/shaders/vulkan/mesh.comp 45244 bytes (70ms compile)
SPIR-V size: assets/shaders/vulkan/post_process.frag 10856 bytes (71ms compile)
SPIR-V size: assets/shaders/vulkan/post_process.vert 1160 bytes (85ms compile)
SPIR-V size: assets/shaders/vulkan/shadow.frag 1592 bytes (78ms compile)
SPIR-V size: assets/shaders/vulkan/shadow.vert 4596 bytes (69ms compile)
SPIR-V size: assets/shaders/vulkan/sky.frag 18980 bytes (74ms compile)
SPIR-V size: assets/shaders/vulkan/sky.vert 2696 bytes (67ms compile)
SPIR-V size: assets/shaders/vulkan/ssao_blur.frag 1760 bytes (72ms compile)
SPIR-V size: assets/shaders/vulkan/ssao.frag 6588 bytes (70ms compile)
SPIR-V size: assets/shaders/vulkan/ssao.vert 1160 bytes (67ms compile)
SPIR-V size: assets/shaders/vulkan/taa.frag 9760 bytes (73ms compile)
SPIR-V size: assets/shaders/vulkan/taa.vert 1160 bytes (73ms compile)
SPIR-V size: assets/shaders/vulkan/terrain_debug.frag 1336 bytes (73ms compile)
SPIR-V size: assets/shaders/vulkan/terrain.frag 63108 bytes (73ms compile)
SPIR-V size: assets/shaders/vulkan/terrain.vert 10500 bytes (64ms compile)
SPIR-V size: assets/shaders/vulkan/ui.frag 744 bytes (71ms compile)
SPIR-V size: assets/shaders/vulkan/ui_tex.frag 1408 bytes (70ms compile)
SPIR-V size: assets/shaders/vulkan/ui_tex.vert 1344 bytes (69ms compile)
SPIR-V size: assets/shaders/vulkan/ui.vert 1336 bytes (68ms compile)
SPIR-V size: assets/shaders/vulkan/water.frag 22164 bytes (69ms compile)
SPIR-V size: assets/shaders/vulkan/water.vert 8796 bytes (66ms compile)
assets/shaders/vulkan/bloom_downsample.vert
assets/shaders/vulkan/bloom_upsample.vert
assets/shaders/vulkan/debug_shadow.vert
assets/shaders/vulkan/fxaa.vert
assets/shaders/vulkan/post_process.vert
assets/shaders/vulkan/shadow.vert
assets/shaders/vulkan/sky.vert
assets/shaders/vulkan/ssao.vert
assets/shaders/vulkan/taa.vert
assets/shaders/vulkan/terrain.vert
assets/shaders/vulkan/ui_tex.vert
assets/shaders/vulkan/ui.vert
assets/shaders/vulkan/water.vert
assets/shaders/vulkan/bloom_downsample.frag
assets/shaders/vulkan/bloom_upsample.frag
assets/shaders/vulkan/debug_shadow.frag
assets/shaders/vulkan/fxaa.frag
assets/shaders/vulkan/g_pass.frag
assets/shaders/vulkan/post_process.frag
assets/shaders/vulkan/shadow.frag
assets/shaders/vulkan/sky.frag
assets/shaders/vulkan/ssao_blur.frag
assets/shaders/vulkan/ssao.frag
assets/shaders/vulkan/taa.frag
assets/shaders/vulkan/terrain_debug.frag
assets/shaders/vulkan/terrain.frag
assets/shaders/vulkan/ui.frag
assets/shaders/vulkan/ui_tex.frag
assets/shaders/vulkan/water.frag
assets/shaders/vulkan/culling.comp
assets/shaders/vulkan/depth_pyramid.comp
assets/shaders/vulkan/lpv_inject.comp
assets/shaders/vulkan/lpv_propagate.comp
assets/shaders/vulkan/mesh.comp
Shadow ABI reflection: 4 matrices, 352-byte block, offsets 0/256/272/288/304/320/336
Compiler: 0.16.0
Compiler: 0.16.0
Compiler: 0.16.0
assets/shaders/vulkan/bloom_downsample.vert
assets/shaders/vulkan/bloom_upsample.vert
assets/shaders/vulkan/debug_shadow.vert
assets/shaders/vulkan/fxaa.vert
assets/shaders/vulkan/post_process.vert
assets/shaders/vulkan/shadow.vert
assets/shaders/vulkan/sky.vert
assets/shaders/vulkan/ssao.vert
assets/shaders/vulkan/taa.vert
assets/shaders/vulkan/terrain.vert
assets/shaders/vulkan/ui_tex.vert
assets/shaders/vulkan/ui.vert
assets/shaders/vulkan/water.vert
assets/shaders/vulkan/bloom_downsample.frag
assets/shaders/vulkan/bloom_upsample.frag
assets/shaders/vulkan/debug_shadow.frag
assets/shaders/vulkan/fxaa.frag
assets/shaders/vulkan/g_pass.frag
assets/shaders/vulkan/post_process.frag
assets/shaders/vulkan/shadow.frag
assets/shaders/vulkan/sky.frag
assets/shaders/vulkan/ssao_blur.frag
assets/shaders/vulkan/ssao.frag
assets/shaders/vulkan/taa.frag
assets/shaders/vulkan/terrain_debug.frag
assets/shaders/vulkan/terrain.frag
assets/shaders/vulkan/ui.frag
assets/shaders/vulkan/ui_tex.frag
assets/shaders/vulkan/water.frag
assets/shaders/vulkan/culling.comp
assets/shaders/vulkan/depth_pyramid.comp
assets/shaders/vulkan/lpv_inject.comp
assets/shaders/vulkan/lpv_propagate.comp
assets/shaders/vulkan/mesh.comp
Refs #904