feat: rewrite cascaded shadow pipeline#907
Conversation
github-actions
Bot
added
documentation
kcov coverageLine coverage ran for this PR and uploaded a non-blocking report artifact named |
📋 SummaryPR #907 rewrites the cascaded shadow pipeline for ZigCraft. It implements practical logarithmic cascade splits, receiver-volume extrusion for caster bounds, texel-snapped stable projections, atlas-aware alpha-tested cutout shadows, and coherent PCF tiers (hard/4/9/16 taps). It expands the The PR closes issue #903. Most of #903's scope is addressed (splits, fitting, view-depth usage, stability, overlap, margins, bias, caster extrusion, cutout/entity-ish chunk shadows, PCF tiers, MDI, debug channels). The acceptance item "Include supported entities in the shadow scene" is not visibly implemented for non-chunk entities. The PR notes the unrelated
📌 Review Metadata
The PR implements the majority of #903's requested scope and required tests. However, it does not visibly add non-chunk entity rendering to the shadow pass, and some acceptance criteria (visual quality, performance target) require runtime/visual verification that cannot be done from this environment. 🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8 | CSM math, GPU upload, and scene submission are cleanly separated. |
| Open/Closed | 7 | New fields extend structs; test aggregation could be more open. |
| Liskov Substitution | 8 | IShadowScene VTable change is propagated across all implementations. |
| Interface Segregation | 8 | ShadowParams fields are all consumed by shader/RHI. |
| Dependency Inversion | 8 | World renders via IShadowScene; no direct RHI calls from world code. |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 76%
Confidence Breakdown:
- Code Quality: 80% - Clean, idiomatic Zig, follows existing patterns.
- Completeness: 72% - Most [Lighting Overhaul][Phase 2] Rewrite cascaded shadows and caster submission #903 scope implemented; entity shadow gap and un-aggregated tests.
- Risk Level: 75% - Shadow pipeline changes are high-risk, but ABI checks and unit tests pass.
- Test Coverage: 75% - New inline CSM tests run and pass;
shadow_tests.zigtests are not exercised.
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 WITH FIXES
The PR is a solid, well-structured rewrite of the shadow system and is close to ready. The main fixes needed are the stale/outdated tests in modules/engine-graphics/src/shadow_tests.zig, extending ShadowCascades.isValid() to cover the new fields, and clarifying the entity-shadow scope. Addressing those would make this ready to merge with high confidence.
{
"reviewed_sha": "ce878cc793a4b5c6bcc9dcd3fda8f12acdb25bfa",
"critical_issues": 0,
"high_priority_issues": 0,
"medium_priority_issues": 3,
"overall_confidence_score": 76,
"recommendation": "MERGE WITH FIXES"
}
📋 SummaryPR #907 rewrites ZigCraft's cascaded shadow pipeline, replacing fixed splits with practical logarithmic splits, adding receiver-volume extrusion for caster bounds, stable texel-snapped projections, alpha-tested cutout shadows, and coherent PCF tiers (hard/4/9/16 taps). It expands the
📌 Review Metadata
The PR implements the majority of #903's requested scope and includes the required tests. The non-chunk entity shadow acceptance criterion remains unaddressed. 🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8 | CSM math, GPU upload, and scene submission are cleanly separated. |
| Open/Closed | 7 | New fields extend structs; test isolation could be more robust. |
| Liskov Substitution | 8 | IShadowScene VTable change is propagated across all implementations. |
| Interface Segregation | 8 | ShadowParams fields are all consumed by shader/RHI. |
| Dependency Inversion | 8 | World renders via IShadowScene; no direct RHI calls from world code. |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 80%
Confidence Breakdown:
- Code Quality: 85% - Clean, idiomatic Zig; previous issues addressed.
- Completeness: 75% - Most [Lighting Overhaul][Phase 2] Rewrite cascaded shadows and caster submission #903 scope implemented; entity shadow gap remains.
- Risk Level: 75% - Shadow pipeline changes are high-risk, but ABI checks and unit tests pass.
- Test Coverage: 78% - New inline CSM tests pass; some test isolation gaps remain.
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 WITH FIXES
The shadow rewrite is solid and well-structured. The previous review's medium issues were largely addressed in the follow-up commit. The remaining blockers are the isValid test isolation gap and the unresolved entity-shadow scope item; fixing those would make this ready to merge with high confidence.
{
"reviewed_sha": "99b6c5bf5308a47fc1ab19c6df54b5ddd528766a",
"critical_issues": 0,
"high_priority_issues": 0,
"medium_priority_issues": 2,
"overall_confidence_score": 80,
"recommendation": "MERGE WITH FIXES"
}
📋 SummaryPR #907 rewrites ZigCraft's cascaded shadow pipeline, replacing fixed cascade ratios with practical logarithmic splits, adding stable texel-snapped projections, receiver-volume extrusion for caster bounds, explicit overlap bands, per-cascade bias scaling, alpha-tested cutout shadows, and coherent hard/4/9/16-tap PCF tiers. It expands the
📌 Review Metadata
The PR implements the majority of #903's requested scope, including the required behavioral tests. The non-chunk entity shadow acceptance criterion remains unaddressed. 🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8 | CSM math, GPU upload, and scene submission are cleanly separated. |
| Open/Closed | 7 | New fields extend structs; test isolation is now robust. |
| Liskov Substitution | 8 | IShadowScene VTable change is propagated across all implementations. |
| Interface Segregation | 8 | ShadowParams fields are all consumed by shader/RHI. |
| Dependency Inversion | 8 | World renders via IShadowScene; no direct RHI calls from world code. |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 85%
Confidence Breakdown:
- Code Quality: 90% - Clean, idiomatic Zig; previous test issues addressed.
- Completeness: 80% - Most [Lighting Overhaul][Phase 2] Rewrite cascaded shadows and caster submission #903 scope implemented; entity shadow gap remains.
- Risk Level: 75% - Shadow pipeline changes are high-risk, but ABI checks and unit tests pass.
- Test Coverage: 88% - New inline CSM tests and hardened isolation tests 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 WITH FIXES
The shadow rewrite is solid and well-structured. The previous review's test issues were fully addressed in the latest commit. The remaining item is the #903 entity shadow scope gap; clarifying or implementing that would make this ready to merge with high confidence.
{
"reviewed_sha": "65b4089b59869fef318120019113605df76d3c5b",
"critical_issues": 0,
"high_priority_issues": 0,
"medium_priority_issues": 1,
"overall_confidence_score": 85,
"recommendation": "MERGE WITH FIXES"
}
📋 SummaryPR #907 rewrites ZigCraft's cascaded shadow pipeline and closes issue #903. The current commit (
📌 Review Metadata
The PR fully implements #903's requested scope, including the entity shadow acceptance criterion that was outstanding in previous reviews. 🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8 | CSM math, GPU upload, scene submission, and entity shadow casting are cleanly separated. |
| Open/Closed | 7 | New fields extend structs; entity shadow hook is optional and doesn't break existing code. |
| Liskov Substitution | 8 | IShadowScene VTable change is propagated across all implementations. |
| Interface Segregation | 8 | ShadowParams and SceneContext optional fields are all consumed. |
| Dependency Inversion | 8 | World and ECS render via IShadowScene / RHI abstractions; no direct RHI calls from world code. |
| Average | 7.8 |
🎯 Final Assessment
Overall Confidence Score: 88%
Confidence Breakdown:
- Code Quality: 90% - Clean, idiomatic Zig; follows existing patterns; previous review issues addressed.
- Completeness: 90% - [Lighting Overhaul][Phase 2] Rewrite cascaded shadows and caster submission #903 scope is fully implemented, including entity shadows.
- Risk Level: 80% - Shadow pipeline changes are inherently high-risk, but ABI checks and unit tests pass.
- Test Coverage: 85% - New CSM/ABI/entity shadow tests pass;
shadow_tests.zigis aggregated.
Merge Readiness:
- All critical issues resolved
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- Tests present and passing
Verdict:
MERGE
The shadow pipeline rewrite is complete, well-structured, and addresses all outstanding review items including entity shadow rendering.
{
"reviewed_sha": "0e2fbe9e5fdfe4d60ba7f896d887409f85d759ec",
"critical_issues": 0,
"high_priority_issues": 0,
"medium_priority_issues": 0,
"overall_confidence_score": 88,
"recommendation": "MERGE"
}
1 participant
Summary
Verification
nix develop --command zig build testnix develop --command zig build -Dskip-presentnix develop --command zig build run -Dskip-present -Dauto-world=normal -Dstartup-diagnostic-seconds=5Known verification limitation
nix develop --command zig build test-robustnesscurrently fails to compile in unchangedsrc/integration_test_robustness.zig:34because Zig 0.16 renamed thestd.process.Child.Term.Exitedtag.Closes #903