Refactor: Split main.zig into modular game components

[MichaelFisher1997]

Summary

This PR addresses the remaining high-priority task from the SOLID refactoring feedback: splitting the monolithic main.zig into smaller, focused modules.

Changes

• src/game/state.zig: Defines AppState and Settings structs.
• src/game/menus.zig: Contains UI rendering logic for Home, Settings, and Singleplayer screens.
• src/game/seed.zig: Utilities for seed generation and parsing.
• src/game/app.zig: Encapsulates the application lifecycle (init, run, deinit) in an App struct.
• src/main.zig: Reduced to a simple entry point that instantiates and runs App.

Verification

• Validated build with zig build.
• Validated runtime with zig build run -- --backend vulkan (verified menu navigation and world entry).

[MichaelFisher1997]

/oc can you do a indepth code review of this PR
I want it to cover vulkan and opengl parity.
SOLID.
Memory management and any issues you find

[opencode-agent]

Code Review: Refactor main.zig into Modular Game Components

Vulkan/OpenGL Parity Issues

1. Critical: Shadow Maps Not Implemented for Vulkan

src/game/app.zig:203, 458-471, 534-547

Issue: Shadow maps are entirely disabled for Vulkan:

const shadow_map = if (!is_vulkan) ShadowMap.init(rhi, settings.shadow_resolution) catch null else null;

Impact: Major feature disparity. Vulkan path completely lacks shadow rendering, cascades, and the entire shadow pipeline.

Recommendation: Implement Vulkan shadow mapping via RHI or explicitly document this as a known limitation.

2. Debug Visualization OpenGL-Only

src/game/app.zig:179-197, 579-587

Issue: Debug shadow visualization (debug_shader, debug_quad_vao/vbo) is OpenGL-exclusive:

if (self.debug_shadows and self.debug_shader != null and self.shadow_map != null) {
    self.debug_shader.?.use();
    c.glBindVertexArray().?(self.debug_quad_vao); // OpenGL calls
    // ...
}

Impact: Debug shadow toggle (u key) does nothing in Vulkan mode.

3. Atmosphere/Clouds API Divergence

src/game/app.zig:201-202, 474, 549-562, 578

Issue: Different initialization and rendering APIs:

• OpenGL: Atmosphere.init(), atmosphere.renderSky(), clouds.render()
• Vulkan: Atmosphere.initNoGL(), rhi.drawSky(struct), no cloud render call

Impact: Cloud rendering (line 578) only executes on OpenGL: if (!self.is_vulkan) cl.render(...).

4. VSync Handling Inconsistency

src/game/app.zig:169, 50-52

Issue: Different VSync configuration paths:

• OpenGL: Direct setVSync() call from renderer module
• Vulkan: RHI abstraction layer handles it

Risk: Settings change at line 349-352 calls rhi.setVSync(), but OpenGL path never calls this (only at init).

---

SOLID Principles Issues

1. Single Responsibility: App Struct is a God Object

src/game/app.zig:38-80

The App struct has ~20 responsibilities:

• Window/SDL management
• RHI/Backend selection
• Input handling
• Time management
• Camera management
• Shader management
• UI system
• World/chunk management
• Settings state
• Map system
• Debug state
• Seed input
• Full game loop + rendering logic

Violation: Clearly violates Single Responsibility Principle.

Recommendation: Extract subsystems:

• WindowManager - SDL window/context
• RenderSystem - RHI, shaders, atmosphere, clouds, shadows
• GameSystem - World, camera, input, time
• MenuSystem - UI state management

2. Open/Closed: Backend Selection Hardcoded

src/game/app.zig:82-155, 454-577

The init function and render loop have extensive if (!is_vulkan) branching throughout. This makes adding new backends difficult.

Recommendation: Use strategy pattern with backend-specific implementations behind a unified interface.

3. Interface Segregation: Menus Pass Too Many Dependencies

src/game/app.zig:673-677

switch (self.app_state) {
    .home => Menus.drawHome(u, screen_w, screen_h, &self.app_state, &self.input, &self.last_state, &self.seed_focused),
    .settings => Menus.drawSettings(u, screen_w, screen_h, &self.app_state, &self.settings, &self.input, self.last_state, self.rhi),
    .singleplayer => try Menus.drawSingleplayer(u, screen_w, screen_h, &self.app_state, &self.input, &self.seed_input, &self.seed_focused, self.allocator, &self.time, &self.pending_new_world_seed),
}

Each menu function takes 7+ parameters. This suggests tight coupling.

Recommendation: Create a MenuContext struct containing shared state.

4. Dependency Inversion: Direct Dependencies on Engine Modules

src/game/app.zig:4-21

App directly imports and knows about all engine internals (Camera, Shader, UISystem, Atmosphere, Clouds, ShadowMap, TextureAtlas, etc.).

Violation: High-level module (App) depends on low-level details.

Recommendation: App should depend on abstractions (e.g., IRenderer, IInput, IWorldManager).

---

Memory Management Issues

1. Leak: OpenGL Debug Quad Resources Never Freed

src/game/app.zig:183-197, 248-269

Issue: debug_quad_vao and debug_quad_vbo are allocated via c.glGenVertexArrays/Buffers() but never freed in deinit().

pub fn deinit(self: *App) void {
    // ... other cleanup ...
    // Missing: c.glDeleteVertexArrays(1, &self.debug_quad_vao);
    // Missing: c.glDeleteBuffers(1, &self.debug_quad_vbo);
}

Severity: Minor leak (only 2 objects), but should be fixed.

2. Unsafe: App Returned as Raw Pointer

src/game/app.zig:81, 268

pub fn init(allocator: std.mem.Allocator) !*App {  // Returns raw pointer
    const app = try allocator.create(App);
    // ...
    return app;
}

pub fn deinit(self: *App) void {
    // ...
    self.allocator.destroy(self);  // Self-destruction!
}

Issue: App.deinit() destroys itself, but the caller also holds the pointer. This is fragile - if caller forgets to call deinit or calls it twice, undefined behavior.

Recommendation: Return !App by value, or use std.heap.ArenaAllocator for easier cleanup.

3. Risk: pending_new_world_seed Never Cleared on Error

src/game/app.zig:86-93

When transitioning from .singleplayer to .world:

const seed = try seed_gen.resolveSeed(seed_input, allocator);
pending_new_world_seed.* = seed;  // Set here
app_state.* = .world;  // Then set state

If World.init() fails (line 287), pending_new_world_seed remains set, causing the frame loop to try again endlessly.

Mitigation: This is somewhat handled by frame loop structure, but could be clearer with error recovery.

4. Memory Order: World Cleanup Uses waitIdle() but May Race

src/game/app.zig:277-293

if (self.pending_world_cleanup or self.pending_new_world_seed != null) {
    self.rhi.waitIdle();
    if (self.world) |w| {
        w.deinit();
        self.world = null;
    }
    self.pending_world_cleanup = false;
}

if (self.pending_new_world_seed) |seed| {
    self.world = try World.init(self.allocator, self.settings.render_distance, seed, self.rhi);
    // ...
}

Issue: If World creation fails after waitIdle(), we've lost the old world but don't have a new one. The app may crash or be in undefined state.

5. Inconsistent Deinit Order

src/game/app.zig:248-268

Deinit order:

1. world_map
2. world
3. seed_input
4. shadow_map
5. clouds
6. atmosphere
7. ui
8. atlas
9. debug_shader
10. shader
11. input
12. rhi

Issue: No explicit dependency tracking. If ui depends on rhi, or world depends on atlas, this order could use-after-free if dependencies are reordered.

Recommendation: Use RAII-like defer statements or document dependency graph.

---

Other Issues

1. Menus: Quit Button Does Nothing

src/game/menus.zig:37-40

if (Widgets.drawButton(u, ..., "QUIT", 2.2, mouse_x, mouse_y, mouse_clicked)) {
    // We can't set input.should_quit directly as input is const here.
    // We'll handle quit return value in the caller or pass a mutable bool pointer.
}

Issue: The quit button is non-functional. This comment suggests it was intentionally left incomplete.

Recommendation: Return an enum/action type from menu functions instead of void.

2. Seed Input Buffer Has No Size Limit Enforcement

src/game/menus.zig:133

if (seed_focused.*) try handleSeedTyping(seed_input, allocator, input, 32);

The max_len is 32, but drawSingleplayer calls handleSeedTyping with this limit. However, the UI rendering doesn't enforce this visually - user could type more than 32 characters before the buffer is truncated.

3. Type Inconsistency: Using std.ArrayListUnmanaged in App Struct

src/game/app.zig:67

seed_input: std.ArrayListUnmanaged(u8),

Issue: Using Unmanaged requires explicit allocator passing everywhere. For a simple string buffer, ArrayList(u8) would be simpler.

---

Summary

Category	Issues	Severity
Vulkan Parity	Shadow maps missing, debug visualization OpenGL-only, clouds not rendering on Vulkan	HIGH
SOLID	App is a god object, tight coupling, no abstraction layer	MEDIUM
Memory	Debug quad leak, unsafe self-destruction pattern, potential race in world cleanup	MEDIUM

Overall: This is a good first step toward modularity, but the App struct still has too many responsibilities and the Vulkan backend is feature-incomplete compared to OpenGL. The memory management issues are real but not catastrophic.

opencode session  |  github run