acdream/docs/research/2026-07-02-ui-architecture-review.md

# UI System Architecture Review — 2026-07-02

**Scope:** the D.2b retail UI engine (`src/AcDream.App/UI/` widgets + `UI/Layout/`
importer + controllers), `AcDream.UI.Abstractions`, the ImGui backend, the new
uncommitted item-interaction + cursor slice, Studio/MockupDesktop, and the
GameWindow composition root — reviewed against the CLAUDE.md Code Structure Rules
and the three-layer UI strategy (`docs/plans/2026-04-24-ui-framework.md`).

**Method:** 6 parallel dimension reviewers (layering / widget-tree / importer /
controllers / cursor / abstractions-fit), every medium+high finding adversarially
verified by an independent agent reading the cited code (40 agents total).
Result: **34 findings confirmed, 0 refuted** (2 high, 32 medium), plus 11 low.
Five load-bearing findings were additionally spot-checked by hand in this session
(GameWindow +490 diff, raw property writes, dead fall-through seam, dual ChatVM,
dual useability truth) — all held.

**Worktree state reviewed:** branch `claude/peaceful-visvesvaraya-e0a196` at
`e3fc7ac5` + the uncommitted D.2b item-interaction/cursor diff (51 modified files,
19 new files) copied from the codex worktree.

---

## Verdict

**The middle of the UI system is architecturally sound. The edges are not.**

The widget toolkit's core abstractions, the importer's two-layer split, the
controller binding pattern, the cursor semantic/presentation split, and the
ViewModel reuse are all genuinely good — GL-free, testable, single-responsibility,
with the Studio proving the bindings run headless. The structural rot concentrates
in four places:

1. **GameWindow as composition root keeps absorbing feature bodies** — the
   uncommitted work alone added +490 lines (now 14,214), most of it a character-sheet
   data provider that belongs in its own class. Growth is linear in window count.
2. **State ownership around the character sheet is violated** — the UI writes raw
   property dictionaries, bypassing both designed change-event chokepoints, with no
   rollback contract (item moves have one; raises don't).
3. **Three input stacks coexist without an arbiter** — InputDispatcher (Phase K),
   UiRoot (retail), ImGui — glued by order-dependent boolean gates; the designed
   fall-through seam is dead code.
4. **The design SSOT has silently drifted** — the ui-framework plan and architecture
   doc still promise an `IPanelRenderer` backend swap that the shipped D.2b pivoted
   away from; the plugin UI contract fractured along the same line.

None of this is load-bearing-broken *today* — the app works — but every finding is
a compounding tax on the next window, the next input consumer, the next plugin.

---

## What is sound (keep doing this)

- **Widget layer is GL-free and testable**: sprites/fonts arrive via injected
  `Func<uint,(tex,w,h)>` resolve delegates; headless tests exist at the right layer.
- **Draw vs game-state mutation separated**: widget draw paths only render; wire
  actions flow through injected controller delegates.
- **Drag policy split is right**: UiRoot owns only the pointer state machine
  (item-agnostic via `GetDragPayload`/`GetDragGhost` virtuals); semantics live in
  controllers.
- **Two-pass overlay draw** reproduces retail's root-level MakePopup semantics
  without reparenting.
- **Importer two-layer split is clean**: pure layer (`Build`/`BuildFromInfos` over
  `ElementInfo` POCOs) is GL-free and dat-free; `DatReaderWriter` types never leak
  past the importer — which is exactly why the same path runs in Studio.
- **`DatWidgetFactory.Create` is a single localized type-registration switch** —
  adding a retail widget type is one case + one widget class.
- **SSOT largely holds for items**: controllers read `ClientObjectTable` live via
  its events and keep only view state; controller-to-controller coupling is
  mediated (events + injected services), never direct.
- **ItemInteractionController is a well-shaped seam**: constructor-injected
  delegates (player guid, clock, wire sends, toast) — unit-testable without GL or
  session.
- **Cursor split is real**: `CursorFeedbackController.ResolveKind` is the single
  priority chain for cursor MEANING (pure over a snapshot record, tested);
  EnumIDMap resolution is dat-faithful with a decomp citation
  (`ClientUISystem::UpdateCursorState` 0x00564630), cached, lock-disciplined;
  `RetailCursorManager` dedupes OS writes. Its GameWindow wiring is exemplary
  rule-1 compliance (2 fields + 7-line helper).
- **The ViewModel layer survived the D.2b pivot**: ChatWindowController consumes
  `ChatVM`/`ChatCommandRouter`/`ChatInputParser`; VitalsVM reused via thin Func
  adapters; VitalsController is pure data binding by dat element id.
- **D.2a boundary discipline held for two years**: no Abstractions panel imports a
  backend namespace; exactly one IPanelRenderer implementation, isolated in
  AcDream.UI.ImGui.

---

## Findings by theme

Severities are post-verification. Every row was confirmed by an independent
adversarial reader at the cited location.

### T1 — GameWindow god-object growth (the dominant theme; both HIGHs)

| Sev | Finding | Where |
|---|---|---|
| **HIGH** | ~430 lines of character-sheet feature body added directly to GameWindow (uncommitted): `CurrentCharacterSheet`, `ComputeLevelXp`, `BuildAttributeRaiseCosts`, `BuildLiveCharacterSkills`, `RaiseCostFromXpCurve`, raise send/apply flow. Pure dat-table domain math trapped untestable in a GL-bound 14K-line class. Two bare `catch {}` swallows in `LoadCharacterExperienceTable`; `IsUsableUntrained` hardcodes skill ids 18/37–40. File grew 13,719 → 14,214 in this stream; committed-only delta was +13 — all growth is this feature. | [GameWindow.cs:11766](src/AcDream.App/Rendering/GameWindow.cs:11766) |
| **HIGH** | The whole retail-UI composition block in `OnLoad` is ~590 lines of real layout logic (per-window mount recipes: nine-slice frame construction, anchor rewrites, layout constants `Left=540`, `MaxHeight=760`). Each new retail window adds another mount block — growth is linear in the D.2b window count. | [GameWindow.cs:1834](src/AcDream.App/Rendering/GameWindow.cs:1834) |
| MED | Per-window mount recipes duplicated verbatim between GameWindow and Studio's MockupDesktop (same element-id anchor rewrites in two files). | [MockupDesktop.cs:184](src/AcDream.App/Studio/MockupDesktop.cs:184), [GameWindow.cs:2161](src/AcDream.App/Rendering/GameWindow.cs:2161) |
| MED | GameWindow reaches back into raw LayoutDesc structures after import, duplicating the importer's property parsing inline. | [GameWindow.cs:2013](src/AcDream.App/Rendering/GameWindow.cs:2013) |
| MED | Studio's `RenderBootstrap` is a hand-maintained copy of GameWindow.OnLoad wiring — already drifting. | [RenderBootstrap.cs:147](src/AcDream.App/Rendering/RenderBootstrap.cs:147) |

### T2 — State ownership / event flow

| Sev | Finding | Where |
|---|---|---|
| MED | Single-owner-state violation: GameWindow writes raw `PropertyBundle` dictionaries (`props.Int64s[2u]`, `props.Ints[...]` with a 0x18→0xC0→0xB5 guess-chain), bypassing BOTH designed change-event chokepoints (`ClientObjectTable.UpdateIntProperty` fires ObjectUpdated; `LocalPlayerState` mutators fire CharacterChanged). The only raw writes into live bundles outside ClientObjectTable, repo-wide. Same bug class as `feedback_single_owner_state_per_entity_write`. | [GameWindow.cs:12106](src/AcDream.App/Rendering/GameWindow.cs:12106) |
| MED | Optimistic-mutation contract inconsistent: item moves have the `RecordPending`/`ConfirmMove`/`RollbackMove` ledger (reconciled on 0x0022/0x0023/0x00A0); character raises apply immediately with no pending snapshot and no rollback — a rejected raise leaves the sheet permanently wrong until an unrelated full refresh. | [GameWindow.cs:12080](src/AcDream.App/Rendering/GameWindow.cs:12080) |
| MED | Two parallel "use an item" stacks with two useability truths: world double-click keys off GameWindow-private `_lastSpawnByGuid` (24 refs); the retail UI path keys off `ClientObjectTable`. | [GameWindow.cs:13324](src/AcDream.App/Rendering/GameWindow.cs:13324) |
| MED | `LocalPlayerState.CharacterChanged` has zero subscribers while the character sheet polls a full sheet rebuild via ~30 per-widget closures per frame. `Properties` returns the internal mutable reference despite its "snapshot" doc. | [LocalPlayerState.cs:134](src/AcDream.Core/Player/LocalPlayerState.cs:134) |
| MED | Window open/close state has no notification channel to controllers: inventory close button routes straight to GameWindow's `CloseRetailWindow`, so `NoLongerViewingContents` is never sent for the viewed container and `_openContainer` goes stale. Toolbar-button sync is manual push at every mutation site. | [InventoryController.cs:404](src/AcDream.App/UI/Layout/InventoryController.cs:404) |
| LOW | Window-visibility/toolbar sync manual push, no event seam on UiHost/UiRoot. | [GameWindow.cs:12023](src/AcDream.App/Rendering/GameWindow.cs:12023) |
| LOW | `ShortcutStore` (session-scoped state) trapped as a private field of ToolbarController. | [ToolbarController.cs:70](src/AcDream.App/UI/Layout/ToolbarController.cs:70) |

### T3 — Input arbitration (three stacks, no arbiter)

| Sev | Finding | Where |
|---|---|---|
| MED | Retail UI is a parallel Silk input subscription arbitrated by order-dependent boolean gates; the designed `WorldMouseFallThrough`/`WorldKeyFallThrough` seam exists only in a doc comment — dead code. | [UiHost.cs:78](src/AcDream.App/UI/UiHost.cs:78) |
| MED | UiRoot bypasses the Phase-K InputDispatcher scope stack and hardcodes its own chords. | [UiRoot.cs:463](src/AcDream.App/UI/UiRoot.cs:463) |
| MED | No arbitration between ImGui and UiRoot input stacks — both subscribe raw Silk devices; neither yields to the other's capture. | [UiHost.cs:78](src/AcDream.App/UI/UiHost.cs:78) |
| MED | Target-mode click interception is per-call-site opt-in: Toolbar/Paperdoll/main-pack consult ItemInteractionController first; InventoryController grid cells don't (`Clicked = () => SelectItem(guid)`), while the cursor shows TargetValid over those same cells — the first observable drift. | [InventoryController.cs:308](src/AcDream.App/UI/Layout/InventoryController.cs:308) |

### T4 — Widget engine internals

| Sev | Finding | Where |
|---|---|---|
| MED | Layout runs inside the draw traversal with no invalidation model; hit-testing/input consume draw-produced geometry (stale rects between frames; hidden subtrees' layout frozen). Already forced the `Anchors=None` exemption hack, repeated in UiScrollablePanel. | [UiElement.cs:323](src/AcDream.App/UI/UiElement.cs:323) |
| MED | Hit-testing allocates + sorts per node per call (`_children.ToArray()` + `Array.Sort` at every level), hidden behind property getters composed multiplicatively — the cursor controller added ~5 full walks/frame without touching UiRoot. | [UiRoot.cs:66](src/AcDream.App/UI/UiRoot.cs:66) |
| MED | Pointer-intent disambiguation is an accreting five-flag boolean lattice, hand-duplicated between press-time (`OnMouseDown`) and hover-time (`HoverWindowMove`) — drift shows as "cursor says move but press does drag". | [UiRoot.cs:288](src/AcDream.App/UI/UiRoot.cs:288) |
| LOW | Dead Device seams: `RegisterTimerEvent` accumulates timers that never fire; `FireEvent` has no callers. | [UiRoot.cs:675](src/AcDream.App/UI/UiRoot.cs:675) |
| LOW | `ItemDragPayload` carries a live widget back-ref no consumer reads, while grids rebuild mid-drag. | [ItemDragPayload.cs:24](src/AcDream.App/UI/ItemDragPayload.cs:24) |

### T5 — Importer policy scatter

| Sev | Finding | Where |
|---|---|---|
| MED | "Which dat children become live widgets" policy has three owners: class-level `ConsumesDatChildren` flag, an importer carve-out naming `UiMeter` concretely, and controllers re-creating swallowed children from hardcoded sprite ids (CharacterStatController tab sprites 0x06005D92 etc.) — a dat-fidelity seam regression. | [LayoutImporter.cs:133](src/AcDream.App/UI/Layout/LayoutImporter.cs:133) |
| MED | `FindElement`'s last-wins `byId` table breaks on duplicate dat ids; the workaround is a second, type-incomplete lookup (per-widget `ElementId` switch returning 0 for four widget types) duplicating the base `UiElement.DatElementId` that already exists. Three parallel id-lookup mechanisms coexist. | [LayoutImporter.cs:116](src/AcDream.App/UI/Layout/LayoutImporter.cs:116) |
| MED | Scrollbar/menu chrome sprites hardcoded in three controllers although the dat carries them and the importer already resolves the inheritance chain that delivers them. | [DatWidgetFactory.cs:82](src/AcDream.App/UI/Layout/DatWidgetFactory.cs:82) |
| MED | Sub-window mount + slot-size cascade are structural heuristics tuned to the windows shipped so far, accreting inline patches. | [LayoutImporter.cs:300](src/AcDream.App/UI/Layout/LayoutImporter.cs:300) |
| LOW | Synthetic multi-top-level root leaks a hidden contract (consumers pluck the real window; rely on AddChild re-parenting to orphan stray siblings). | [LayoutImporter.cs:206](src/AcDream.App/UI/Layout/LayoutImporter.cs:206) |
| LOW | WindowChromeController accumulates per-window button ids in a shared static, binds by whole-tree sweep, and binds ALL ids (incl. Max/Min) to close — while ChatWindowController binds the same MaxMin id with a different meaning. | [WindowChromeController.cs:13](src/AcDream.App/UI/Layout/WindowChromeController.cs:13) |

### T6 — Controllers

| Sev | Finding | Where |
|---|---|---|
| MED | CharacterStatController is five controllers welded into one 1,959-line **static** `Bind` with closure-boxed state (`new[] { -1 }` mutable-array boxes threaded through ~33 static helpers). No instance → no teardown/rebind path, tab/selection state untestable. The god-object pattern relocated from GameWindow into a controller. | [CharacterStatController.cs:247](src/AcDream.App/UI/Layout/CharacterStatController.cs:247) |
| MED | Controller lifecycle contract inconsistent: ToolbarController subscribes with inline lambdas + no Dispose (unremovable by construction); Inventory/Paperdoll declare unsubscribing `Dispose()` but don't implement `IDisposable` and nothing ever calls them. Any relog/rebind or Studio hot-reload double-subscribes. | [ToolbarController.cs:152](src/AcDream.App/UI/Layout/ToolbarController.cs:152) |
| MED | Equip-rule logic split across two App-layer files (`ItemEquipRules` + ItemInteractionController's coverage scans) and sits below its natural Core home. | [ItemEquipRules.cs:5](src/AcDream.App/UI/ItemEquipRules.cs:5) |
| LOW | ItemInteractionController is pure game-rule logic living in the App.UI namespace (Core candidate). | [ItemInteractionController.cs:11](src/AcDream.App/UI/ItemInteractionController.cs:11) |

### T7 — Cursor pipeline (new slice)

| Sev | Finding | Where |
|---|---|---|
| MED | Cursor-art precedence is an implicit contract split across two layers; generic widget hover art shadows target-mode global cursors. | [CursorFeedbackController.cs:154](src/AcDream.App/UI/CursorFeedbackController.cs:154) |
| MED | OS cursor has no single arbiter: RetailCursorManager writes Type/Image/Hotspot across ALL mice; GameWindow independently writes `CursorMode` from three sites on the FIRST mouse only. Works only because the writers touch disjoint properties — an undocumented contract. | [RetailCursorManager.cs:28](src/AcDream.App/Rendering/RetailCursorManager.cs:28) |
| MED | Target-mode cursor cannot see world hover: `HoverTargetGuid` is built exclusively from the UI tree, so hovering a drudge with an active health kit shows pending crosshair instead of valid/invalid — retail keys 0x28/0x29 off the SmartBox target (a world object) per the catalog's own citation. | [CursorFeedbackController.cs:67](src/AcDream.App/UI/CursorFeedbackController.cs:67) |
| MED | Semantic controller hardcodes dat LayoutDesc state-name priority lists as string literals. | [CursorFeedbackController.cs:165](src/AcDream.App/UI/CursorFeedbackController.cs:165) |
| LOW | Silent permanent negative caches + OS-cursor fallback mask dat resolution failures; the fallback deviation has **no divergence-register row** (register rule breach). | [RetailCursorResolver.cs:47](src/AcDream.App/Rendering/RetailCursorResolver.cs:47) |
| LOW | Per-frame cursor update runs ~4 full UI hit-tests (property-getter recomputation). | [CursorFeedbackController.cs:57](src/AcDream.App/UI/CursorFeedbackController.cs:57) |
| LOW | OS-application half of the seam untested; the golden dat test silently passes when dats are absent. | [RetailCursorCatalogTests.cs:34](tests/AcDream.App.Tests/UI/RetailCursorCatalogTests.cs:34) |

### T8 — Strategy drift (three-layer plan vs reality)

| Sev | Finding | Where |
|---|---|---|
| MED | Shipped D.2b is a parallel fourth stack, not the Abstractions backend the design SSOT promises: ui-framework.md still says D.2b "remains design-only" / "AcDream.UI.Retail implements IPanelRenderer"; the architecture doc still shows the swappable-backend diagram. The pivot is acknowledged only in the 2026-06-14 panel-frame spec §0. The real stable seam is now the **ViewModels**, not IPanel/IPanelRenderer. | [ui-framework.md:8](docs/plans/2026-04-24-ui-framework.md) |
| MED | Plugin UI contract fractured: plugin surface exposes only `AddMarkupPanel`; markup panels render only under `ACDREAM_RETAIL_UI=1`, IPanel panels only under devtools. Plugin.Abstractions has zero project references — no backend-agnostic IPanel registration exists despite both docs promising it. | [IUiRegistry.cs:13](src/AcDream.Plugin.Abstractions/IUiRegistry.cs:13) |
| MED | Two live ChatVM instances with asymmetric wiring: the retail ChatVM lacks Fps/Position providers (init-only, can't be wired later) → `/framerate` and `/loc` silently degrade in the retail chat window — the surface meant to replace the ImGui one. | [GameWindow.cs:1937](src/AcDream.App/Rendering/GameWindow.cs:1937) |
| MED | Vivid Target Indicator (retail gameplay UI, decomp 0x004d6165) is implemented on the ImGui backend inside the devtools gate — in the end-state (RETAIL_UI=1, DEVTOOLS=0), selection has no visual indicator at all. | [TargetIndicatorPanel.cs:4](src/AcDream.App/UI/TargetIndicatorPanel.cs:4) |
| LOW | WindowChromeController hardwires a global close-button id list; binds Max/Min to close for every window. | [WindowChromeController.cs:52](src/AcDream.App/UI/Layout/WindowChromeController.cs:52) |

---

## Recommended remediation sequence (awaiting approval — report only)

Ordered by leverage; R1 is time-sensitive because the breach is still uncommitted.

- **R1 — Extract the character-sheet provider BEFORE committing this work.** Move
  `CurrentCharacterSheet` + XP-curve/raise-cost math + raise send/apply into a
  `CharacterSheetProvider` (App/UI/Layout; the pure curve math is Core-eligible),
  and the repeated window-mount block into a small `RetailWindowMounter` shared
  with MockupDesktop. This addresses both HIGHs + the Studio duplication at once,
  and avoids cementing a rule-1 breach into history.
- **R2 — State-write chokepoints.** Route XP/credit debits through eventful APIs
  (`ClientObjectTable.UpdateIntProperty` + an Int64 twin + a `LocalPlayerState`
  spend API firing CharacterChanged); adopt the pending/confirm/rollback ledger
  for raises; make `LocalPlayerState.Properties` read-only.
- **R3 — Input arbitration.** Decide the ownership model (single Silk owner
  fanning out, or explicit documented gate order); delete or wire the dead
  fall-through seam; route target-mode click interception through one chokepoint
  (UiItemSlot/UiRoot offers the click to ItemInteractionController first).
- **R4 — Doc + register reconciliation.** One commit: ui-framework.md status
  header + architecture-doc UI section rewritten to the real shape (two coexisting
  stacks, VMs shared, retail UI = UiRoot tree); divergence-register row for the
  cursor OS-fallback; decide the plugin markup-vs-IPanel contract question.
- **R5 — Structural debt (phase-scale, schedule separately).** CharacterStatController
  → instance controller; importer policy consolidation (per-child consume predicate,
  duplicate-aware FindElement); layout pass out of draw; single ChatVM; Vivid Target
  Indicator onto the retail path; controller IDisposable standardization.

## What this is NOT

This is NOT an assessment of the reported **terrain regression** — none of the 70
copied files touch terrain/streaming/rendering-world code (the diff is confined to
UI, Core.Net messages, and Core item/player state), so that remains a separate
investigation on its own evidence. It is also NOT a retail-fidelity gap list — the
cursor EnumIDMap chain, equip-mask semantics, and useability gates reviewed here
are dat/decomp-faithful in approach; the issues are structural, not behavioral.