refactor: resolve SOLID architecture seams

[MichaelFisher1997]

Summary

• remove split-brain RenderDevice resource pools and drop the no-op bindShader command contract
• inject font atlas through UISystem and guard graphics preset access behind a mutex-backed API
• move world map GPU texture ownership out of worldgen so worldgen only produces pixels

Verification

• nix develop --command zig fmt src/ modules/engine-graphics/src/rhi_tests.zig modules/engine-graphics/src/rhi_vulkan.zig modules/engine-rhi/src/render_device.zig modules/engine-rhi/src/rhi.zig modules/engine-rhi/src/root.zig modules/engine-ui/src/font.zig modules/engine-ui/src/ui_system.zig modules/engine-ui/src/ui_system_manager.zig modules/game-core/src/map_controller.zig modules/game-core/src/session.zig modules/game-core/src/settings/json_presets.zig modules/game-core/src/settings/tests.zig modules/game-core/src/settings/ui_helpers.zig modules/game-core/src/ui/session_hud.zig modules/game-ui/src/screens/graphics.zig modules/game-ui/src/screens/settings.zig modules/game-ui/src/settings_ui.zig modules/worldgen-overworld/src/world_map.zig
• nix develop --command zig build test

Closes #802
Closes #803
Closes #804

[github-actions]

📋 Summary

This PR resolves three linked SOLID-debt issues: #802 (split-brain RenderDevice + bindShader no-op + doc lie), #803 (font-atlas global + unguarded graphics_presets), and #804 (worldgen owning a GPU texture). All three are fully addressed: RenderDevice is reduced to a stats sink, bindShader is removed from the encoder contract, the font atlas is injected through UISystem, graphics_presets is mutex-guarded behind accessor APIs, and WorldMap now produces a CPU pixel buffer while GameSession owns the GPU texture. The diff is a net deletion of ~440 lines, the test suite and shader validation pass, and zig fmt --check is clean.

📌 Review Metadata

• Reviewed Commit SHA: 61d8c70e0f817a36c3c95cc9dcb823c3db9583b3
• Reviewed PR: refactor: resolve SOLID architecture seams #838
• Linked Issues: Closes [SOLID] [M16] Fix or delete split-brain RenderDevice; honor bindShader contract (covers M14, m5) #802, Closes [SOLID] [M21] Inject font atlas; mutex-guard graphics_presets #803, Closes [SOLID] [M20] Worldgen produces pixels; upper layer owns the GPU texture #804 — all requirements are satisfied by the current changes.

🔴 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] modules/engine-rhi/src/render_device.zig:1 - RenderDevice doc says "backend-populated GPU resource statistics," but the Vulkan backend never calls setStats.
Confidence: High
Description: The new RenderDevice is documented as a backend-populated stats sink, yet modules/engine-graphics/src/rhi_vulkan.zig stores the pointer in ctx.render_device and never invokes device.setStats(...). The diagnostics UI (modules/engine-ui/src/ui_system_manager.zig:71) therefore reads all-zero GPU stats.
Impact: The timing overlay GPU counters are silently zeroed. This is not a functional crash, but it is a small observability regression and the module-level doc is misleading.
Suggested Fix: Either (a) have the Vulkan backend populate real stats from ctx.resources after creation/destruction calls, or (b) update the doc-comment to state the sink is currently unpopulated and intended for future backend integration.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	RenderDevice is now only a stats sink; WorldMap only generates pixels; json_presets only manages preset state.
Open/Closed	8	New accessor API extends preset access without exposing internals; encoder contract shrinks cleanly.
Liskov Substitution	8	Interface changes (bindShader removal) are propagated consistently across Vulkan and mock backends.
Interface Segregation	9	Removing the no-op bindShader tightens the encoder contract; measureTextWidthWithUI adds an opt-in atlas-aware variant without breaking legacy callers.
Dependency Inversion	9	Font atlas and world-map texture are injected from upper layers instead of pulled from globals or GPU modules.
Average	8.6

🎯 Final Assessment

Overall Confidence Score: 90%

Confidence Breakdown:

• Code Quality: 90% (clean deletion, consistent patterns, good errdefers)
• Completeness: 95% (all three linked issues fully addressed)
• Risk Level: 15% (mostly deletions and re-routing; no RHI behavior changes)
• Test Coverage: 90% (existing tests updated, shader validation passes, build green)

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 refactor is focused, well-scoped, and resolves the stated architecture debt with passing tests and clean formatting.

{
  "reviewed_sha": "61d8c70e0f817a36c3c95cc9dcb823c3db9583b3",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 90,
  "recommendation": "MERGE"
}

opencode session  |  github run

[github-actions]

📋 Summary

This PR closes #802, #803, and #804. It removes the split-brain RenderDevice resource pools and reduces it to a stats sink, drops the no-op bindShader encoder contract, injects the font atlas through UISystem, wraps graphics_presets behind a mutex-guarded accessor API, and moves world-map GPU texture ownership from worldgen-overworld into GameSession. The changes are well-scoped, delete ~440 net lines, and the test suite plus shader validation pass.

📌 Review Metadata

• Reviewed Commit SHA: ad584cf9bae6907ba4ce44a20769ecad3b8ff145
• Reviewed PR: refactor: resolve SOLID architecture seams #838
• Linked Issues: Closes [SOLID] [M16] Fix or delete split-brain RenderDevice; honor bindShader contract (covers M14, m5) #802, Closes [SOLID] [M21] Inject font atlas; mutex-guard graphics_presets #803, Closes [SOLID] [M20] Worldgen produces pixels; upper layer owns the GPU texture #804 — all three are fully addressed.

🔴 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] modules/engine-rhi/src/render_device.zig:1 - RenderDevice documented as backend-populated stats sink, but no backend calls setStats.
Confidence: High
Description: The module doc states RenderDevice is a "backend-populated GPU resource statistics" sink, and modules/engine-ui/src/ui_system_manager.zig:71 reads device.getStats() for the timing overlay. However, modules/engine-graphics/src/rhi_vulkan.zig stores the pointer in ctx.render_device and never invokes device.setStats(...), so the GPU counters remain zeroed. This is the same issue noted in the previous automated review.
Impact: The diagnostics overlay silently reports zero GPU resource counts, making the new stats sink misleading.
Suggested Fix: Either (a) have the Vulkan backend populate real stats after resource creation/destruction calls, or (b) update the doc-comment to note the sink is currently unpopulated and intended for future backend integration.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	RenderDevice is now only a stats sink; WorldMap only generates pixels; preset management is isolated.
Open/Closed	8	New accessor APIs extend preset access without exposing internals; encoder contract shrinks cleanly.
Liskov Substitution	8	bindShader removal is propagated consistently across Vulkan and mock backends.
Interface Segregation	9	Removing the no-op bindShader tightens the encoder contract; measureTextWidthWithUI adds an opt-in atlas-aware variant.
Dependency Inversion	9	Font atlas and world-map texture are injected from upper layers instead of pulled from globals or GPU modules.
Average	8.6

🎯 Final Assessment

Overall Confidence Score: 90%

Confidence Breakdown:

• Code Quality: 90% (clean deletion, consistent patterns, good errdefers)
• Completeness: 95% (all three linked issues fully addressed)
• Risk Level: 15% (mostly deletions and re-routing; no RHI behavior changes)
• Test Coverage: 90% (existing tests updated, shader validation passes, build green)

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 refactor is focused, well-scoped, and resolves the stated architecture debt with passing tests and clean formatting. The only remaining note is a low-priority observability/doc gap in RenderDevice stats population.

{
  "reviewed_sha": "ad584cf9bae6907ba4ce44a20769ecad3b8ff145",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 90,
  "recommendation": "MERGE"
}

opencode session  |  github run