Shadow System Refactoring: SOLID Violations & Monolithic VulkanContext

[MichaelFisher1997]

Shadow System Refactoring: SOLID Violations & Monolithic VulkanContext

Summary

The shadow implementation across src/engine/graphics/csm.zig, src/engine/graphics/render_graph.zig, src/engine/graphics/rhi_vulkan.zig, and src/world/world_renderer.zig has architectural issues affecting maintainability, testability, and adherence to SOLID principles. While the high-level design is sound, the Vulkan backend implementation suffers from severe SRP violations and monolithic structure.

Issues Found

🏗️ SOLID Violations

Critical: Single Responsibility Principle (SRP) Violations

1. Monolithic VulkanContext Contains Shadow Logic (src/engine/graphics/rhi_vulkan.zig)

   • 5000+ lines mixing shadow, UI, GPass, SSAO, and main pass responsibilities
   • Shadow resources scattered across lines 337-344, 211-213, 310-328
   • Shadow pipeline initialization mixed with other pipelines (~lines 2100-2300)
   • Shadow pass rendering at lines 5151-5215 mixed with other pass functions
   • Debug shadow visualization at lines 372-380, 3795-3875 embedded in production code

2. IRenderContext Interface Segregation Violation (src/engine/graphics/rhi.zig:272-352)

   • Mixes unrelated pass methods in single interface:

     beginShadowPass/endShadowPass
     beginMainPass/endMainPass
     begin2DPass/end2DPass
     drawSky/drawDebugShadowMap

   • Clients depending on shadow passes must drag in UI/2D rendering dependencies
   • Violates Interface Segregation Principle (ISP)

3. Dependency Inversion Violation (src/engine/graphics/render_graph.zig:109)

   • ShadowPass.execute() depends directly on concrete World and Camera types
   • Cannot mock for testing shadow pass logic in isolation
   • Tightly couples render graph to world implementation details

Medium Issues

4. Open/Closed Principle (OCP) Concerns

   • Adding new shadow technique requires modifying VulkanContext directly
   • No abstraction for shadow mapping algorithms (CSM vs VSM vs PCF)

5. State Duplication in beginShadowPass() (src/engine/graphics/rhi_vulkan.zig:5159-5187)

   ctx.shadow_pass_active = true;      // Line 5159
   // ... some code ...
   ctx.shadow_pass_active = true;      // Line 5185 (duplicate)
   ctx.shadow_pass_index = cascade_index; // Line 5186 (duplicate of line 5160)
   ctx.shadow_pass_matrix = light_space_matrix; // Line 5187 (duplicate of line 5161)

🧹 Code Cleanliness Problems

Major Issues

6. Shadow Resource Management Scattered

   • Resource creation: throughout rhi_vulkan.zig:2100-2300 (no single location)
   • Destruction: destroyShadowResources() at line 382 (isolated)
   • State tracking: lines 315-328 (shadow_pass_active, shadow_pass_index, etc.)
   • Debug resources: lines 372-380 mixed with production resources

7. Debug Shadow Code Embedded in Production

   • debug_shadow_pipeline, debug_shadow_pipeline_layout at lines 372-374
   • drawDebugShadowMap() function at lines 3795-3875
   • Debug descriptor pools at lines 375-377
   • Should be behind conditional compilation or separate module

8. Manual Mutex Locking Without RAII (src/engine/graphics/rhi_vulkan.zig:3821)

   ctx.mutex.lock();
   const tex_entry = ctx.textures.get(depth_map_handle);
   ctx.mutex.unlock();

   • No exception safety (though Zig doesn't have exceptions, this pattern is error-prone)
   • Should use Zig's defer pattern for guaranteed unlock

Medium Issues

9. Missing Documentation

   • CSM cascade split algorithm (lambda=0.92) not explained
   • No comments on shadow bias values (line 5183: 1.25, 0.0, 1.75)
   • No explanation of reverse-Z shadow mapping (line 5175 comment is helpful but sparse)

10. Hardcoded Shadow Constants

    • Shadow cascade count: SHADOW_CASCADE_COUNT = 3 (line 32 of rhi.zig)
    • Shadow distance hardcoded in ShadowPass.execute() via ctx.shadow_distance
    • Shadow resolution passed as parameter but not validated

11. No Unit Tests

    • csm.zig:computeCascades() has no tests for matrix calculations
    • Cascade split distance calculations not validated
    • Texel snapping logic not tested

✅ Positive Findings

12. Well-Structured High-Level Design

    • csm.zig is clean and focused (110 lines)
    • ShadowPass in render_graph.zig is well-scoped
    • world_renderer.zig:renderShadowPass() has clear responsibility

13. Good Use of Interface Pattern

    • IRenderPass enables pass extension without modification
    • RenderGraph follows open/closed principle

14. Stable CSM Implementation

    • Bounding sphere approach prevents shadow edge artifacts
    • Texel snapping reduces shimmering
    • Reverse-Z depth improves precision

Proposed Solution

Phase 1: Extract ShadowSystem (Critical)

// New file: src/engine/graphics/shadow_system.zig
pub const ShadowSystem = struct {
    allocator: std.mem.Allocator,
    rhi: *RHI,

    // Resources
    shadow_image: c.VkImage,
    shadow_image_memory: c.VkDeviceMemory,
    shadow_image_views: [rhi.SHADOW_CASCADE_COUNT]c.VkImageView,
    shadow_framebuffers: [rhi.SHADOW_CASCADE_COUNT]c.VkFramebuffer,
    shadow_sampler: c.VkSampler,
    shadow_render_pass: c.VkRenderPass,
    shadow_pipeline: c.VkPipeline,

    // State
    pass_active: bool = false,
    pass_index: u32 = 0,
    pass_matrix: Mat4,

    pub fn init(allocator: std.mem.Allocator, rhi: *RHI, resolution: u32) !ShadowSystem { /* ... */ }
    pub fn deinit(self: *ShadowSystem) void { /* ... */ }
    pub fn beginPass(self: *ShadowSystem, cascade_index: u32, light_space_matrix: Mat4) void { /* ... */ }
    pub fn endPass(self: *ShadowSystem) void { /* ... */ }
    pub fn updateUniforms(self: *ShadowSystem, params: rhi.ShadowParams) void { /* ... */ }
};

Phase 2: Interface Segregation

// Split IRenderContext into focused interfaces
pub const IShadowContext = struct {
    ptr: *anyopaque,
    vtable: *const VTable,

    pub const VTable = struct {
        beginPass: *const fn (ptr: *anyopaque, cascade_index: u32, light_space_matrix: Mat4) void,
        endPass: *const fn (ptr: *anyopaque) void,
        updateUniforms: *const fn (ptr: *anyopaque, params: ShadowParams) void,
    };
    // ... methods
};

pub const IMainPassContext = struct {
    ptr: *anyopaque,
    vtable: *const VTable,

    pub const VTable = struct {
        beginPass: *const fn (ptr: *anyopaque) void,
        endPass: *const fn (ptr: *anyopaque) void,
        // ... main pass only methods
    };
    // ... methods
};

// Compose RHI from interfaces
pub const RHI = struct {
    shadow: IShadowContext,
    main_pass: IMainPassContext,
    // ... other contexts
};

Phase 3: Dependency Inversion

// Refactor ShadowPass to depend on abstractions
pub const IShadowScene = struct {
    ptr: *anyopaque,
    vtable: *const VTable,

    pub const VTable = struct {
        renderShadowPass: *const fn (ptr: *anyopaque, light_space_matrix: Mat4, camera_pos: Vec3) void,
    };

    pub fn renderShadowPass(self: IShadowScene, light_space_matrix: Mat4, camera_pos: Vec3) void {
        self.vtable.renderShadowPass(self.ptr, light_space_matrix, camera_pos);
    }
};

// World implements IShadowScene
pub fn shadowScene(self: *World) IShadowScene {
    return .{
        .ptr = self,
        .vtable = &.{
            .renderPass = renderShadowPassWrapper,
        },
    };
}

Phase 4: Debug Code Separation

// Use conditional compilation or separate module
const debug_shadows = @import("build_options").debug_shadows;

if (debug_shadows) {
    // Debug shadow pipeline, resources, rendering
}

Phase 5: Code Quality Improvements

1. Extract ShadowResources struct for lifecycle management
2. Add RAII-style mutex handling:

   ctx.mutex.lock();
   defer ctx.mutex.unlock();
   const tex_entry = ctx.textures.get(depth_map_handle);

3. Add unit tests for CSM calculations
4. Document magic constants (lambda=0.92, bias values, etc.)
5. Create ShadowConfig struct for extensible configuration

Phase 6: Documentation & Testing

// src/engine/graphics/csm.zig
/// Computes cascaded shadow map matrices using stable CSM approach.
/// 
/// Parameters:
/// - resolution: Shadow map resolution in pixels
/// - camera_fov: Camera vertical field of view in radians
/// - aspect: Camera aspect ratio (width / height)
/// - near: Camera near plane distance
/// - far: Camera far plane distance
/// - sun_dir: Direction to sun (normalized)
/// - cam_view: Camera view matrix
/// - z_range_01: If true, maps depth to [0,1] (for reverse-Z)
///
/// Algorithm:
/// 1. Computes cascade splits using logarithmic/linear blend (lambda=0.92)
/// 2. For each cascade, computes bounding sphere of frustum slice
/// 3. Builds orthographic projection snapped to texel grid
/// 4. Returns light-space matrices and cascade metadata
pub fn computeCascades(resolution: u32, camera_fov: f32, aspect: f32, near: f32, far: f32, sun_dir: Vec3, cam_view: Mat4, z_range_01: bool) ShadowCascades { /* ... */ }

test "computeCascades generates valid matrices" {
    // Test cascade splits are monotonic
    // Test matrices are invertible
    // Test texel snapping preserves alignment
}

Expected Impact

• Maintainability: ~60% reduction in VulkanContext size (remove ~500 shadow-related lines)
• Testability: Enable isolated unit testing of shadow system and CSM calculations
• Extensibility: Add new shadow techniques without modifying monolithic context
• Code Quality: Clearer responsibilities, better documentation, reduced coupling

Related Files

• src/engine/graphics/rhi_vulkan.zig (lines 337-380: shadow resources, 5151-5215: shadow pass, 3795-3875: debug)
• src/engine/graphics/rhi.zig (lines 272-352: IRenderContext interface)
• src/engine/graphics/csm.zig (entire file - well-structured)
• src/engine/graphics/render_graph.zig (lines 91-141: ShadowPass)
• src/world/world_renderer.zig (lines 165-205: renderShadowPass)
• assets/shaders/vulkan/shadow.vert
• assets/shaders/vulkan/shadow.frag

Labels

enhancement, shaders, engine

Checklist

• Extract ShadowSystem struct from VulkanContext
• Split IRenderContext into focused interfaces
• Implement IShadowScene abstraction for DIP
• Remove debug shadow code from production (conditional compilation)
• Add unit tests for computeCascades()
• Document CSM algorithm and constants
• Create ShadowConfig for extensible configuration
• Replace manual mutex locking with defer pattern
• Fix state duplication in beginShadowPass()

Questions

1. Should we support multiple shadow mapping techniques (VSM, PCF, PCSS)?
2. Do we want runtime-configurable cascade count/split scheme?
3. Should debug shadow visualization be toggleable at runtime or build-time?
4. Is the current 3-cascade CSM configuration sufficient, or should we make it configurable?
5. Should we add a shadow map cache to reduce per-frame reallocations?

[opencode-agent]

Based on the issue's proposed refactoring phases, here are relevant documentation links and error handling guidance:

Documentation Links

Zig Error Handling Fundamentals:

• https://ziglang.org/documentation/master/#error-handling
• https://ziglang.org/documentation/master/#Errors
• https://ziglang.org/documentation/master/#defer

Testing in Zig:

• https://ziglang.org/documentation/master/#Zig-Test
• https://ziglang.org/documentation/master/#The-Testing-Namespace

Memory Management:

• https://ziglang.org/documentation/master/#Memory

Doc Comments:

• https://ziglang.org/documentation/master/#Doc-Comments
• https://ziglang.org/documentation/master/#Top-Level-Doc-Comments

Error Handling Guidance

Phase 1: ShadowSystem Extraction

Issue: Missing error handling in resource allocation paths.

// Add error set and propagation
pub const ShadowSystemError = error{
    ImageCreationFailed,
    MemoryAllocationFailed,
    ViewCreationFailed,
    SamplerCreationFailed,
    FramebufferCreationFailed,
};

pub fn init(allocator: std.mem.Allocator, rhi: *RHI, resolution: u32) ShadowSystemError!ShadowSystem {
    // Use try for Vulkan calls that can fail
    const image = try ctx.vkCreateImage(...);
    errdefer ctx.vkDestroyImage(...);  // Cleanup on error

    const mem_requirements = try ctx.vkGetImageMemoryRequirements(...);
    const memory = try ctx.allocateMemory(mem_requirements);
    errdefer ctx.freeMemory(memory);

    try ctx.vkBindImageMemory(...);
    // ... rest of initialization
}

Phase 2: Interface Segregation

Issue: Interface methods should return errors where appropriate.

pub const IShadowContext = struct {
    ptr: *anyopaque,
    vtable: *const VTable,

    pub const VTable = struct {
        beginPass: *const fn (ptr: *anyopaque, cascade_index: u32, light_space_matrix: Mat4) ShadowSystemError!void,
        endPass: *const fn (ptr: *anyopaque) void,
        updateUniforms: *const fn (ptr: *anyopaque, params: ShadowParams) void,
    };

    pub fn beginPass(self: IShadowContext, cascade_index: u32, light_space_matrix: Mat4) ShadowSystemError!void {
        return self.vtable.beginPass(self.ptr, cascade_index, light_space_matrix);
    }
};

Phase 3: Dependency Inversion

Issue: ShadowPass.execute() should handle errors from scene rendering.

pub const IShadowScene = struct {
    ptr: *anyopaque,
    vtable: *const VTable,

    pub const VTable = struct {
        renderShadowPass: *const fn (ptr: *anyopaque, light_space_matrix: Mat4, camera_pos: Vec3) ShadowSystemError!void,
    };

    pub fn renderShadowPass(self: IShadowScene, light_space_matrix: Mat4, camera_pos: Vec3) ShadowSystemError!void {
        return self.vtable.renderShadowPass(self.ptr, light_space_matrix, camera_pos);
    }
};

// Usage with proper error propagation
try shadow_scene.renderShadowPass(light_space_matrix, camera_pos);

Phase 5: Code Quality - Mutex Handling

Issue: Manual mutex locking without RAII (from issue: line 3821).

// Before (error-prone):
ctx.mutex.lock();
const tex_entry = ctx.textures.get(depth_map_handle);
ctx.mutex.unlock();

// After (using defer):
ctx.mutex.lock();
defer ctx.mutex.unlock();
const tex_entry = ctx.textures.get(depth_map_handle) orelse return error.TextureNotFound;

Phase 6: Testing

Issue: No unit tests for computeCascades().

// Add tests with proper error handling
test "computeCascades generates valid matrices" {
    const cascades = computeCascades(
        2048,  // resolution
        1.2,   // camera_fov
        16.0/9.0,  // aspect
        0.1,   // near
        100.0, // far
        Vec3.normalize(.{1.0, -1.0, -0.5}),  // sun_dir
        Mat4.lookAt(...),  // cam_view
        true,   // z_range_01
    ) catch |err| {
        return error.CascadeComputationFailed;  // Propagate test failure
    };

    // Validate cascade splits are monotonic
    for (0..cascades.count - 1) |i| {
        try std.testing.expect(cascades.splits[i] < cascades.splits[i + 1]);
    }

    // Test matrices are invertible (det != 0)
    for (cascades.matrices) |matrix| {
        const det = matrix.determinant();
        try std.testing.expect(det != 0.0);
    }
}

Additional Patterns

Init/Deinit Pattern (standard in codebase):

pub fn deinit(self: *ShadowSystem) void {
    if (self.shadow_sampler != null) ctx.vkDestroySampler(...);
    // ... cleanup all resources
}

Error Logging (from codebase conventions):

const log = @import("../engine/core/log.zig");

// In init functions
log.log.err("Failed to create shadow image: {}", .{err});
return error.ImageCreationFailed;

Optional Returns with Error Context:

const entry = ctx.textures.get(depth_map_handle) orelse {
    log.log.err("Shadow depth map not found: {d}", .{depth_map_handle});
    return error.TextureNotFound;
};

opencode session  |  github run