Skip to content

fix: establish lighting phase zero baselines - #905

Merged
github-actions[bot] merged 1 commit into
devfrom
feature/lighting-phase0-contracts
Jul 9, 2026
Merged

fix: establish lighting phase zero baselines#905
github-actions[bot] merged 1 commit into
devfrom
feature/lighting-phase0-contracts

Conversation

@MichaelFisher1997

@MichaelFisher1997 MichaelFisher1997 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • align the four-cascade shadow UBO across CPU, GLSL, and checked-in SPIR-V with a reflection-based ABI gate
  • transition every shadow array layer with complete early/late depth dependencies and use the physically allocated resolution for fitting and sampling
  • recreate shadow resources transactionally at frame boundaries when quality changes
  • add deterministic lighting fixtures, settled capture orchestration, canonical debug channels, and UNORM screenshot sRGB encoding/metadata
  • document the repeatable capture matrix and commit the 60-second high-preset baseline

Verification

  • nix develop --command zig build test
  • nix develop --command zig build -Dskip-present
  • pre-push formatting and full test hooks
  • deterministic shadow-world startup: 49/49 chunks renderable, generation/mesh/upload queues empty
  • 60-second high benchmark generated docs/benchmarks/lighting-phase0-high.json; it records existing draw-call and GPU-memory SLO breaches

Known Existing Failure

  • nix develop --command zig build test-robustness does not compile on the repository Zig 0.16 toolchain because src/integration_test_robustness.zig still switches on the removed .Exited process term tag

Closes #900

@github-actions github-actions Bot added documentation Improvements or additions to documentation engine game build shaders labels Jul 9, 2026
Signed-off-by: MichaelFisher1997 <contact@michaelfisher.tech>
@MichaelFisher1997
MichaelFisher1997 force-pushed the feature/lighting-phase0-contracts branch from e62d532 to 08787e0 Compare July 9, 2026 20:18
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

kcov coverage

Line coverage ran for this PR and uploaded a non-blocking report artifact named kcov-report. Codecov upload is configured as non-blocking while the project captures a stable baseline.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

📋 Summary

PR #905 closes #900 and establishes the phase-zero lighting baseline: it aligns the four-cascade shadow UBO across CPU, GLSL, and checked-in SPIR-V via a compile-time ABI gate plus a SPIR-V reflection script; adds transactional shadow-map recreation at frame boundaries; switches cascade fitting/sampling to the backend-allocated physical resolution; extends the deterministic zigcraft:shadow-test world with canonical fixtures; adds screenshot settle logic and UNORM-to-sRGB PNG encoding; and commits the 60-second high-preset baseline.

The stated issue requirements are satisfied: deterministic fixtures, a repeatable capture matrix, and documented baselines are present and verified.

nix develop --command zig build test passes, including the new scripts/check_shadow_abi.sh gate. The PR body correctly notes that the pre-existing test-robustness compile failure is not addressed here.

📌 Review Metadata

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

None identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] assets/shaders/vulkan/terrain.frag:117 and assets/shaders/vulkan/sky.frag:38 - Outdated ShadowUniforms comment
Confidence: High
Description: Both GLSL blocks comment shadow_params as // x = light_size (PCSS), y/z/w reserved, but the CPU now writes y = 1.0 / shadow_resolution and terrain.frag reads shadows.shadow_params.y for inverse resolution and texel-size margins.
Impact: ABI comment drift; future maintainers may assume the field is free and overwrite it.
Suggested Fix: Update the comment to // x = light_size (PCSS), y = 1 / shadow resolution, z/w reserved in both shaders.

[LOW] modules/engine-graphics/src/vulkan/rhi_state_control.zig:188 - setShadowResolution is not synchronized with ctx.mutex
Confidence: Low
Description: setShadowResolution writes pending_shadow_resolution while beginFrame reads it under ctx.mutex. This is consistent with existing options setters like setMSAA, but it remains a latent data race if the settings/UI path ever runs off the render thread.
Impact: Rare stale or torn resolution change on weakly-ordered memory.
Suggested Fix: Either document the render-thread-only contract in the function or acquire ctx.mutex before writing, matching the convention used by beginFrame.

[LOW] scripts/check_shadow_abi.sh:20 - Brittle ABI verification via exact substring matching
Confidence: Low
Description: The script asserts exact glslang reflection text such as "ShadowUniforms: offset -1, type ffffffff, size 304". A future glslang version that changes formatting will fail the gate even when the ABI is correct.
Impact: Fragile CI that can produce false negatives on toolchain updates.
Suggested Fix: Add a small normalizer or switch to a structured reflection tool (e.g., spirv-reflect) if the toolchain allows.

📊 SOLID Principles Score

Principle Score Notes
Single Responsibility 8 Each module has a clear role; shadow recreation is isolated in rhi_frame_orchestration.
Open/Closed 7 Compile-time ABI guards make the shadow layout hard to break; new debug channels still touch enum and shader.
Liskov Substitution 8 VTable default for getResolution keeps mock implementations compiling.
Interface Segregation 7 setShadowResolution appears in both IRenderSettings and IRenderQualityOptions; duplication is minor but could be consolidated.
Dependency Inversion 8 RHI interface abstracts Vulkan; shadow code uses IShadowContext.
Average 7.6

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

  • Code Quality: 85%
  • 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

The PR lands the phase-zero baseline cleanly with passing tests and no safety blockers; only minor documentation and low-risk polish items remain.

{
  "reviewed_sha": "08787e0a445c564d5a15c4d112a5cd12545b0bf6",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

New%20session%20-%202026-07-09T20%3A19%3A25.892Z
opencode session  |  github run

@github-actions
github-actions Bot merged commit 66947d5 into dev Jul 9, 2026
24 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build documentation Improvements or additions to documentation engine game shaders

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Lighting Overhaul][Phase 0] Establish visual baselines and repair pipeline contracts

1 participant