feat: migrate player menus to RmlUi

[MichaelFisher1997]

Summary

• integrate RmlUi with the existing Vulkan UI pipeline and SDL input path
• migrate Home, World Library, Create World, Settings, Resource Packs, Environment, Pause, and modals to a shared RML/RCSS design system
• retain and refine immediate-mode fallbacks for -Drmlui=false, including the guided world wizard and corrected single-column settings scrolling
• add safe DOM bridge helpers, escaped dynamic markup, save metadata hardening, and transactional resource/environment switching

Validation

• nix build .#rmlui-bridge
• nix develop --command zig build test -Drmlui=true
• nix develop --command zig build test -Drmlui=false
• nix develop --command zig build -Dskip-present -Drmlui=true
• nix develop --command zig build -Dskip-present -Drmlui=false
• bounded headless RmlUi startup/document-load smoke test
• git diff --check

Known limitation

• windowed visual review is still required because the current headless menu golden/capture path is unreliable.

[github-actions]

APIError: You've reached your usage limit for this billing cycle. Your quota will be refreshed in the next cycle. To continue now, purchase extra usage or upgrade your plan: https://www.kimi.com/membership/subscription?tab=quota

opencode session  |  github run

[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

No linked issues ("Closes #123", "Fixes #456", etc.) were found in the PR description.

📌 Review Metadata

• Reviewed Commit SHA: 1959a8ac1a9cc0a4378c2b0e11e543c8fc79d793
• Reviewed PR: feat: migrate player menus to RmlUi #936

This PR integrates RmlUi 6.2 with the existing Vulkan UI pipeline and SDL input path, migrating all player-facing menus (Home, World Library, Create World wizard, Settings, Resource Packs, Environment, Pause, and modals) to a shared RML/RCSS design system. The immediate-mode fallback path (-Drmlui=false) is retained and refined. Implementation quality is high: GPU resource lifecycle is symmetric, premultiplied-alpha blending is correctly separated from legacy UI, and dual-mode (RmlUi/legacy) descriptor pool usage is bounded per frame. Both -Drmlui=true and -Drmlui=false build targets pass zig build test, SPIR-V validation succeeds for the three new ui_rml shaders, and zig fmt --check is clean.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (no prior reviews exist; no critical issues identified in current code).

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved (no prior reviews exist; no high issues identified in current code).

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ All previously reported medium priority issues have been resolved.

None identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] modules/game-ui/src/screens/rml_environment.zig:105-115 - Environment map setting can drift from active texture on partial failure
Confidence: Low
Description: activateEnvironment calls setEnvironmentMap (updating the persisted settings JSON) before getEnvMapPtr()/waitIdle()/texture swap. If a later step in the same function fails, the new texture is rolled back via errdefer replacement.deinit(), but the settings field already reflects the new name. A subsequent settings save would persist a name that does not match the in-memory env map. The same pattern in rml_resource_packs.zig:93-105 does include an errdefer that restores the previous pack setting; consider mirroring that rollback here.
Impact: Settings file could record an environment pack that is not actually loaded; user-restart would load the requested pack rather than silently keeping the old one (arguably the better behavior, but inconsistent with the resource-pack screen).
Suggested Fix: Add an errdefer settings_pkg.persistence.setEnvironmentMap(self.context.settings, self.context.allocator, previous_name) mirroring the pattern used in rml_resource_packs.zig:97-99.

[LOW] modules/game-ui/src/screens/world_save.zig:6 - Hardcoded Linux path ignores XDG_DATA_HOME
Confidence: Medium
Description: pub const SAVE_DIR = ".local/share/zigcraft/saves"; is relative only to $HOME. On systems that set XDG_DATA_HOME (or on macOS/Windows), saves will land in a non-standard location.
Impact: Pre-existing pattern (unchanged from dev); this PR only relocates the constant. Portability gap is unchanged.
Suggested Fix: Out of scope for this PR; consider a follow-up that resolves XDG_DATA_HOME (or per-OS equivalent) when set.

[LOW] modules/engine-ui/src/rmlui.zig:91-105 - Defensive cleanup iterates after C runtime shutdown
Confidence: Low
Description: deinit destroys the C-side runtime/context first, then iterates geometries and generated_textures to free Zig-owned CPU memory. RmlUi's context destructor already invokes the release_geometry/release_texture callbacks for live documents, so by the time the loops run most entries will already be gone. The leftover iteration is correct (idempotent) but slightly misleading.
Impact: None functionally; the loops safely handle the empty case.
Suggested Fix: Optional: add a one-line comment noting that the iteration is defensive for any assets not released by RmlUi during shutdown.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	Each screen owns its own DOM lifecycle; C bridge isolates RmlUi C++ from Zig; Page wrapper centralizes shared document lifecycle.
Open/Closed	8	New IUIContext.drawIndexedGeometry/setScissorRegion extend the RHI without breaking legacy callers; default no-op VTable entries keep existing backends working.
Liskov Substitution	9	MockContext in rhi_tests.zig updated with the new optional VTable entries; UIRenderer thin wrapper preserves IUIContext semantics.
Interface Segregation	9	RmlUi adapter consumes only UIRenderer + ResourceManager; screens only depend on the focused EngineContext subsets (EnvironmentContext, ResourcePacksContext).
Dependency Inversion	8	UI screens depend on *rmlui.RmlUi via the manager accessor; C ABI bridge keeps high-level Zig decoupled from RmlUi C++ headers.
Average	8.6

🎯 Final Assessment

Overall Confidence Score: 88%

How to interpret:

• 0-30%: Major concerns, do not merge without significant rework
• 31-60%: Moderate concerns, several issues need addressing
• 61-80%: Minor concerns, mostly ready with some fixes
• 81-100%: High confidence, ready to merge or with trivial fixes

Confidence Breakdown:

• Code Quality: 92% (Clean conventions, well-documented, dual-build tested, formatting clean.)
• Completeness: 85% (Full menu migration plus retained fallback; windowed visual review still pending per PR description.)
• Risk Level: 82% (Large surface area touching Vulkan pipelines and screenshot path, but careful errdefer/rollback discipline and existing test coverage mitigate risk.)
• Test Coverage: 90% (New contract tests, input processor test, world-list edge cases, hotbar layout tests, RmlUi geometry/scissor validation tests.)

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

Large, well-architected feature addition that compiles cleanly under both -Drmlui=true and -Drmlui=false, passes all unit tests, and respects existing RHI/Vulkan conventions; address the optional low-priority environment-map rollback consistency before merge if convenient.

Machine Readable Verdict

{
  "reviewed_sha": "1959a8ac1a9cc0a4378c2b0e11e543c8fc79d793",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

opencode session  |  github run