fix: quarantine disabled LPV resources

[MichaelFisher1997]

Summary

• defer LPV grid textures, buffers, descriptor sets, and compute pipelines until LPV is enabled
• release all LPV resources when it is disabled and skip the LPV update timing/pass
• keep shader descriptor bindings stable by supplying zero handles for the existing dummy 3D texture fallback
• default omitted preset LPV settings to disabled and cover that preset behavior

Implements the disabled-LPV allocation/update acceptance criterion from #902.

Validation

• nix develop --command zig fmt modules/engine-graphics/src/vulkan/lpv_system.zig modules/game-ui/src/screens/world.zig modules/game-ui/src/screens/world_frame_params.zig modules/game-core/src/settings/json_presets.zig modules/game-core/src/settings/tests.zig
• nix develop --command zig build test
• nix develop --command zig build run -Dskip-present -Dauto-world=normal -Dstartup-diagnostic-seconds=5
• nix develop --command zig build test-robustness blocked by pre-existing Zig 0.16 incompatibility: src/integration_test_robustness.zig references removed process.Child.Term.Exited

[github-actions]

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]

📋 Summary

PR #909 implements the disabled-LPV quarantine acceptance criterion from #902 (specifically: "Disabled LPV allocates no textures/buffers and runs no update pass"). It defers LPV grid textures, buffers, descriptor sets, and compute pipelines until LPV is enabled; releases them when disabled; skips the update pass and timing; defaults omitted preset lpv_enabled fields to false; and adds a test for that preset behavior. zig fmt produced no changes, and nix develop --command zig build test passed (exit code 0). The test-robustness failure is a pre-existing Zig 0.16 incompatibility unrelated to this change.

📌 Review Metadata

• Reviewed Commit SHA: 610f5edd6908b5578a107f22c498785bb484995d
• Reviewed PR: fix: quarantine disabled LPV resources #909
• Linked Issue: [Lighting Overhaul][Phase 4] Bring AO, materials, water, LOD, and volumetrics to parity #902 (implements one acceptance criterion; does not fully close the broader phase-4 issue)

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] modules/engine-graphics/src/vulkan/lpv_system.zig:111 - Resource leak on initResources error path
Confidence: High
Description: initResources creates grid textures with try self.createGridTextures() before registering errdefer self.destroyGridTextures(); on the following line. If createGridTextures fails (e.g., uploadDebugOverlay returns an error after applyGridResources has already stored the new handles in self), the newly created grid texture handles are stored in self but never destroyed. Since resources_initialized remains false, subsequent deinitResources calls return early, and a re-init attempt overwrites the handles without freeing the old textures.
Impact: Memory leak of LPV 3D grid textures and debug overlay texture on the (rare) error path; violates the repo's strict errdefer-after-allocation convention.
Suggested Fix: Add an errdefer self.destroyGridTextures(); inside createGridTextures immediately after self.applyGridResources(resources); so that any subsequent failure in that function destroys the textures it just applied to self.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/engine-graphics/src/vulkan/lpv_system.zig - Missing unit tests for quarantine behavior
Confidence: Medium
Description: The only new test is in modules/game-core/src/settings/tests.zig and verifies that a preset missing lpv_enabled defaults to false. There are no tests verifying that LPVSystem.init(enabled=false) leaves resources_initialized == false, that setSettings(enabled=false) releases resources, or that setSettings(enabled=true) allocates them.
Impact: The quarantine logic is only covered by manual runtime validation; regressions in resource lifecycle could be missed.
Suggested Fix: Add focused unit tests in lpv_system.zig (or a new test file) that assert isEnabled() and resources_initialized state across init/enable/disable cycles, and validate that texture/buffer handles are zero while disabled.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	LPV resource lifecycle is now separated into initResources/deinitResources, and the enable gate is consolidated in isEnabled.
Open/Closed	7	Existing structs are modified rather than extended, but the change is a focused bug fix.
Liskov Substitution	8	No inheritance or interface subtypes introduced.
Interface Segregation	8	No new interfaces; isEnabled() is a simple, focused query.
Dependency Inversion	8	No new concrete dependencies; still relies on existing RHI/Vulkan abstractions.
Average	7.8

🎯 Final Assessment

Overall Confidence Score: 75%

Confidence Breakdown:

• Code Quality: 75% (clean refactor, but the errdefer ordering misses the rare error path)
• Completeness: 80% (implements the stated acceptance criterion, but could use targeted lifecycle tests)
• Risk Level: 60% (low in normal operation; runtime resource destruction is safe via the resource-manager deletion queue, but the error-path leak is a latent risk)
• Test Coverage: 70% (preset default test added, but quarantine behavior is not unit-tested)

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 correctly implements the disabled-LPV quarantine and passes tests; address the createGridTextures errdefer leak before merging.

{
  "reviewed_sha": "610f5edd6908b5578a107f22c498785bb484995d",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 75,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[github-actions]

📋 Summary

PR #909 implements the disabled-LPV quarantine acceptance criterion from #902 (specifically: "Disabled LPV allocates no textures/buffers and runs no update pass"). It defers LPV resource allocation until enabled, releases resources when disabled, skips the update pass when disabled, defaults omitted preset lpv_enabled fields to false, and adds a test for that preset default. The createGridTextures error-path leak from the previous review has been fixed.

📌 Review Metadata

• Reviewed Commit SHA: 7dc333bc2df60f7fe4dd4fad32ddeaeb9778adee
• Reviewed PR: fix: quarantine disabled LPV resources #909
• Linked Issue: [Lighting Overhaul][Phase 4] Bring AO, materials, water, LOD, and volumetrics to parity #902 (implements one acceptance criterion; does not fully close the broader phase-4 issue)

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: Resource leak on initResources error path in createGridTextures. The errdefer self.destroyGridTextures(); is now correctly placed inside createGridTextures at modules/engine-graphics/src/vulkan/lpv_system.zig:582 immediately after applyGridResources, so any failure in buildDebugOverlay or uploadDebugOverlay destroys the textures that were just applied.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/engine-graphics/src/vulkan/lpv_system.zig - Missing unit tests for quarantine behavior
Confidence: Medium
Description: The only new test is in modules/game-core/src/settings/tests.zig and verifies that a preset missing lpv_enabled defaults to false. There are no tests verifying that LPVSystem.init(enabled=false) leaves resources_initialized == false, that setSettings(enabled=false) releases resources, or that setSettings(enabled=true) allocates them.
Impact: The quarantine logic is only covered by manual runtime validation; regressions in resource lifecycle could be missed.
Suggested Fix: Add focused unit tests in lpv_system.zig (or a new test file) that assert isEnabled() and resources_initialized state across init/enable/disable cycles, and validate that texture/buffer handles are zero while disabled.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	LPV resource lifecycle is cleanly separated into initResources/deinitResources, and the enable gate is consolidated in isEnabled.
Open/Closed	7	Existing structs are modified rather than extended, but the change is a focused bug fix.
Liskov Substitution	8	No inheritance or interface subtypes introduced.
Interface Segregation	8	No new interfaces; isEnabled() is a simple, focused query.
Dependency Inversion	8	No new concrete dependencies; still relies on existing RHI/Vulkan abstractions.
Average	7.8

🎯 Final Assessment

Overall Confidence Score: 85%

Confidence Breakdown:

• Code Quality: 85% (clean refactor, errdefer error path is now handled)
• Completeness: 85% (implements the stated acceptance criterion, but could use targeted lifecycle tests)
• Risk Level: 75% (low in normal operation; runtime resource destruction relies on existing RHI/Vulkan cleanup paths)
• Test Coverage: 75% (preset default test added, but quarantine behavior is not unit-tested)

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 — The PR correctly implements the disabled-LPV quarantine; consider adding lifecycle unit tests as a follow-up.

{
  "reviewed_sha": "7dc333bc2df60f7fe4dd4fad32ddeaeb9778adee",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 85,
  "recommendation": "MERGE"
}

opencode session  |  github run