docs: OP3 dual-lens review findings — two APPROVE-WITH-FIXES
Converged MUST-FIX: the six mouse-turning macro lines are typed ClientLocal 0x1A where retail's AddTextToScroll sites pass type 7 (Magic, light blue — mechanism lens decoded the stray [4]=7 writes at all six BN call sites); 0x1A additionally routes to the SpewBox where the 4-slot cap discards two of the six lines before they draw (blast lens). Fix: a typed message seam, Magic for the macro, ClientLocal retained for the mid-air refusal and AD-75; byte-verify the type argument during the fix. Mechanism: the Gameplay page is NOT an OptionPage in retail (gmGameplayOptionsUI : UIElement_Field, acclient.h:55857) — the auto-flush-on-visibility acdream gave it flushes the blob at moments retail would not, and two green tests pin the wrong shape; the OnOptionChanged enable-gating seam (@0x004F27D0 — Apply/Reset gated, Defaults never) is missing from the model; the mid-air refusal fires where retail is silent (non-player-mode/null controller). Blast: the plan-assigned docked-host (0x21000017) register ruling was not filed — deemed a divergence, row owed client-wide; Options is the only shared-geometry panel with ResizeX/four-edge/min-size, which the shared _mainPanelGeometry silently reverts when siblings show; three targeted seams lack pins (real-sibling mutual exclusion, DefaultQueueKey, dormant ActivePageChanged silence); UA/RA legibility flagged for the gate. Clean: exactly one toolbar button changed (previously ghosted), F11 same-action since K.1c, no queue key invented, settings additive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
9d26ecc623
commit
efe80d5a0d
2 changed files with 919 additions and 0 deletions
443
docs/research/2026-08-11-op3-review-blast.md
Normal file
443
docs/research/2026-08-11-op3-review-blast.md
Normal file
|
|
@ -0,0 +1,443 @@
|
||||||
|
# OP3 review — regression / blast-radius lens
|
||||||
|
|
||||||
|
**Commit under review:** `9d26ecc6` — *feat(ui): Campaign OP slice OP3 —
|
||||||
|
Options panel shell, open paths, Gameplay tab*
|
||||||
|
**Reviewer lens:** regression + blast radius (what ALREADY-SHIPPED behavior
|
||||||
|
can this change move?). Pure read-only; no build, no test run, no launch.
|
||||||
|
**Date:** 2026-08-11
|
||||||
|
**Verdict:** **APPROVE-WITH-FIXES** — 2 MUST-FIX, 4 SHOULD-FIX, 8 NOTE.
|
||||||
|
|
||||||
|
The slice's core structural choice is sound and low-blast: it reuses the
|
||||||
|
generic catalog/`RetailPanelUiController`/toolbar mechanisms rather than
|
||||||
|
adding a parallel path, and every seam it touches was already generic. The
|
||||||
|
two MUST-FIX items are both evidence/bookkeeping gaps (an unverified chat
|
||||||
|
routing decision with a demonstrable line-loss consequence, and a register
|
||||||
|
row the plan explicitly assigned to this review), not architectural breaks.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Enumeration A — the `gmPanelUI` panel catalog, before and after
|
||||||
|
|
||||||
|
`src/AcDream.App/UI/RetailPanelCatalog.cs`. `Mounted` is what
|
||||||
|
`TryGetWindowName`/`TryGetPanelId` resolve; every entry that also calls
|
||||||
|
`RetailPanelUiController.RegisterMainPanel` shares ONE parent rectangle
|
||||||
|
(`RetailPanelUiController.cs:41-65`, `:284-314`) and retail's
|
||||||
|
"one active child" exclusion (`:139-197`).
|
||||||
|
|
||||||
|
| panelId | const | window | mount site | Left/Top | ResizeX / ResizeY | resizable edges | Min W×H | authored `RestorePrevious` (`0x10000049`) |
|
||||||
|
|---|---|---|---|---|---|---|---|---|
|
||||||
|
| 3 | `CharacterInformation` | `characterInformation` | `RegisterIndicatorDetailPanel` (`RetailUiRuntime.cs:1713`) | 18 / 18 | false / true | Bottom | — | **yes** (fixture `character_info_2100006E_10000183.json`) |
|
||||||
|
| 4 | `PositiveEffects` | `positiveEffects` | `MountEffects` (`:1519`) | right-edge / 18 | false / true | Bottom | — | **yes** (`effects_positive_2100001B.json`) |
|
||||||
|
| 5 | `NegativeEffects` | `negativeEffects` | `MountEffects` (`:1519`) | right-edge / 18 | false / true | Bottom | — | **yes** |
|
||||||
|
| 7 | `Inventory` | `inventory` | `MountInventory` (`:2307`) | root.Left / root.Top | false / true | Bottom | — | not read (3-arg overload → false) |
|
||||||
|
| 8 | `LinkStatus` | `linkStatus` | `RegisterIndicatorDetailPanel` | 18 / 18 | false / true | Bottom | — | **yes** (`link_status_2100001D.json`) |
|
||||||
|
| 9 | `MiniGame` | `miniGame` | `RegisterIndicatorDetailPanel` | 18 / 18 | false / true | Bottom | — | **yes** (`mini_game_2100001E.json`) |
|
||||||
|
| 11 | `Character` | `character` | `MountCharacter` (`:2226`) | 540 / 18 | false / true | Bottom | — | not read (3-arg overload → false) |
|
||||||
|
| 13 | `Magic` | `spellbook` | `MountSpellbook` (`:1326`) | 18 / 18 | false / true | Bottom | — | not read (3-arg overload → false) |
|
||||||
|
| 15 | `Vitae` | `vitae` | `RegisterIndicatorDetailPanel` | 18 / 18 | false / true | Bottom | — | **yes** (`vitae_21000020.json`) |
|
||||||
|
| **10** | **`Options` (NEW)** | **`options`** | **`MountOptionsPanel` (`:1984`)** | **150 / 80** | **TRUE / true** | **all four** | **300 × 200** | **absent from the fixture → false** |
|
||||||
|
|
||||||
|
Authored extents (fixture roots): every one of these panels is **300 wide**;
|
||||||
|
heights are 600 (spellbook, character) or 362 (inventory, vitae, character
|
||||||
|
info, effects, **options**). So the shared-rectangle height coupling between
|
||||||
|
600-tall and 362-tall panels **already existed** pre-OP3; Options joins the
|
||||||
|
362 cohort and introduces no new height class.
|
||||||
|
|
||||||
|
**Does registering Options change any existing panel's open/close/stacking
|
||||||
|
behavior?** No, with one caveat and one new asymmetry:
|
||||||
|
|
||||||
|
- **Exclusion:** unchanged in kind. `SetPanelVisibility` hides the previous
|
||||||
|
active panel regardless of which sibling is being shown; Options is just
|
||||||
|
one more sibling. No existing pair's behavior changes.
|
||||||
|
- **Deferred-restore (`RestorePrevious`):** Options does **not** author
|
||||||
|
`0x10000049` (verified: `grep -c 268435529
|
||||||
|
tests/.../fixtures/options_panel_2100006E_1000018D.json` → 0). Per
|
||||||
|
`RetailPanelUiController.cs:164-167` a non-`RestorePrevious` panel **clears
|
||||||
|
`_deferredPanel`**. So `Character sheet → Character Info (defers sheet) →
|
||||||
|
F11` drops the pending sheet restore. This is byte-identical to what
|
||||||
|
opening Inventory/Character/Spellbook already did in that same position
|
||||||
|
(all three are `RestorePrevious: false`), so it is not a regression — but
|
||||||
|
it is a newly-reachable route to that drop.
|
||||||
|
- **Geometry asymmetry (new):** Options is the **only** main panel with
|
||||||
|
`ResizeX = true`, all-four-edge resize, and explicit `MinWidth`/`MinHeight`
|
||||||
|
(`RetailUiRuntime.cs:1970-1980`). See SHOULD-FIX 2.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Enumeration B — toolbar panel-button routing, before and after
|
||||||
|
|
||||||
|
`ToolbarController.PanelButtonIds` (`ToolbarController.cs:54-55`) has always
|
||||||
|
carried all seven ids; each binds by its OWN authored `0x10000029` value
|
||||||
|
(`:170-175`), and `BindPanelButtons` (`:374-392`) sets
|
||||||
|
`button.Enabled = isAvailable(panelId)` with `OnClick = null` when not
|
||||||
|
available. `isAvailable` is
|
||||||
|
`RetailPanelCatalog.TryGetWindowName(panelId, …) && WindowManager.TryGet(name)`
|
||||||
|
(`RetailUiRuntime.cs:679-686`).
|
||||||
|
|
||||||
|
| element | authored panelId | in catalog pre-OP3? | pre-OP3 state | post-OP3 state |
|
||||||
|
|---|---|---|---|---|
|
||||||
|
| `0x10000197` | 12 | no | ghosted, `OnClick = null` | **unchanged** |
|
||||||
|
| `0x10000198` | 13 (Magic) | yes | enabled → spellbook | **unchanged** |
|
||||||
|
| `0x10000199` | 11 (Character) | yes | enabled → character | **unchanged** |
|
||||||
|
| `0x1000055A` | 25 | no | ghosted | **unchanged** |
|
||||||
|
| `0x1000019A` | 16 | no | ghosted | **unchanged** |
|
||||||
|
| **`0x1000019B`** | **10** | **no** | **ghosted, `Enabled=false`, `OnClick=null` — clicking did NOTHING** | **enabled → `ToggleWindow("options")`** |
|
||||||
|
| `0x100001B1` | 7 (Inventory) | yes | enabled → inventory | **unchanged** |
|
||||||
|
|
||||||
|
**Answer to "what did `0x1000019B` do pre-OP3":** it was authored with panel
|
||||||
|
id 10, discovered by the generic binder, found panel 10 absent from the
|
||||||
|
catalog, and was therefore **ghosted with a null click handler**. Nothing
|
||||||
|
happened on click. No test pinned that old behavior (the fixture table at
|
||||||
|
`ToolbarControllerTests.cs:26-35` pins the *element→panelId* mapping only,
|
||||||
|
and `PanelButtons_bindByDatPanelId_andGhostUnavailablePanels` at `:221`
|
||||||
|
drives its own `isAvailable` predicate, not the real catalog), so nothing
|
||||||
|
pinning old behavior had to change. **Exactly one button's routing changed.**
|
||||||
|
`SyncToolbarWindowButtons` (`RetailUiRuntime.cs:670-675`) now also syncs
|
||||||
|
panel 10's Highlight/Normal state; `SetPanelOpen` (`ToolbarController.cs:394`)
|
||||||
|
early-returns per-id, so no other button's state is touched.
|
||||||
|
|
||||||
|
Degradation path is clean: if `MountOptionsPanel` bails (`:1954`, `:1962`),
|
||||||
|
the catalog still maps `options`→10 but `_panelUi` has no id-10 entry, so
|
||||||
|
`TogglePanel(10)` returns false (`RetailPanelUiController.cs:141`) and the
|
||||||
|
toolbar button re-ghosts (`WindowManager.TryGet` fails). F11 silently no-ops
|
||||||
|
rather than falling through to `Host.ToggleWindow`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Enumeration C — F11 history
|
||||||
|
|
||||||
|
| commit / event | `KeyBindings.RetailDefaults()` | `InputAction.ToggleOptionsPanel` routed to | net effect of pressing F11 |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `84512d3c` (Phase K.1a) | file created; retail preset not yet built | — | — |
|
||||||
|
| **`da189103`** (Phase K.1c, retail keymap cutover) | **adds `KeyChord(F11, None) → ToggleOptionsPanel`** (now `KeyBindings.cs:216`), sourced from `retail-default.keymap.txt:148` `ToggleOptionsPanel [ "" [ 0 DIK_F11 ] ]` | `_devTools.ToggleSettingsPanel()` | opened the ImGui-era Settings panel |
|
||||||
|
| Campaign V slice V11 | unchanged | `DevToolsGameplayCommands.ToggleSettingsPanel()` becomes an **empty method** (`GameplayInputCommandController.cs:70-72`, class doc `:53-59`) | **nothing** |
|
||||||
|
| **`9d26ecc6` (OP3)** | **unchanged — the row already existed** | `_retained.ToggleOptionsPanel()` → `RetailUiRuntime.ToggleWindow("options")` → `_panelUi.TogglePanel(10)` | opens/closes the retail Options panel |
|
||||||
|
|
||||||
|
**Verification of the implementer's claim** (`git log -L 216,216:…/KeyBindings.cs`):
|
||||||
|
the binding was introduced by `da189103`, not by OP3. OP3 touches
|
||||||
|
`KeyBindings.cs` **not at all** — only the new pin in
|
||||||
|
`KeyBindingsRetailTests.cs:239`. The claim is accurate.
|
||||||
|
|
||||||
|
**What stopped working:** nothing. Between V11 and OP3, F11 was a hard no-op.
|
||||||
|
|
||||||
|
**Keybind conflicts:** `grep -n "F11" KeyBindings.cs` returns exactly one row.
|
||||||
|
No chord conflict introduced. `InputScope` puts F11 at the bottom of the
|
||||||
|
stack (`InputScope.cs:19`) — unchanged.
|
||||||
|
|
||||||
|
**Dead `ToggleSettingsPanel` wiring:** untouched, as claimed — but now
|
||||||
|
**orphaned**. `GameplayInputCommandController` was its only production caller;
|
||||||
|
post-OP3 the only remaining references are the interface declaration
|
||||||
|
(`:50`), the empty implementation (`:70`), a comment (`:242`), and the test
|
||||||
|
fake (`GameplayInputCommandControllerTests.cs:141`). The commit comment says
|
||||||
|
"its no-op wiring elsewhere is untouched" — there is no *elsewhere*. Planned
|
||||||
|
for retirement in OP9; see NOTE 8.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Findings
|
||||||
|
|
||||||
|
### MUST-FIX 1 — the six mouse-turning chat lines are routed to a 4-slot transient overlay on an **unverified** retail log type, and two of them can never be seen
|
||||||
|
|
||||||
|
`MouseTurningSettingsMacro.Compute` can emit **six** lines
|
||||||
|
(`MouseTurningSettingsMacro.cs:58-104`). `RetailUiRuntime.ApplyMouseTurningSettingsMacro`
|
||||||
|
(`RetailUiRuntime.cs:1907-1908`) pushes every one of them through
|
||||||
|
`_bindings.Options.DisplaySystemMessage`, which is bound to
|
||||||
|
`d.Communication.AddText(text, RetailLogTextType.ClientLocal)`
|
||||||
|
(`InteractionRetainedUiComposition.cs:816-817`).
|
||||||
|
|
||||||
|
`RuntimeCommunicationState.AddText` (`RuntimeCommunicationState.cs:197-201`)
|
||||||
|
sends `ClientLocal (0x1A)` to `SpewBox.Enqueue` and **returns** — it never
|
||||||
|
reaches the chat transcript. `SpewBoxState.MaxConcurrentItems = 4`
|
||||||
|
(`SpewBoxState.cs:77`), and `Tick` drains the whole pending queue in one
|
||||||
|
call, dropping the oldest on overflow (`:132-149`). Six lines enqueued in one
|
||||||
|
click therefore leave **four visible**: the first two — *Camera Stiffness*
|
||||||
|
and *Camera Adjustment* — are discarded before they are ever drawn. They also
|
||||||
|
expire after 5 s (`:88`).
|
||||||
|
|
||||||
|
The routing itself is **not byte-verified**. The commit and register call the
|
||||||
|
*format strings* byte-verified (VAs `0x007A8A70` … `0x007A88D0`), and the
|
||||||
|
research doc's §4.4 table lists exactly those VAs — but neither the doc nor
|
||||||
|
the code records the `AddTextToScroll` **type argument** at
|
||||||
|
`0x0049E8F0..0x0049EB81`. This project has already been burned on precisely
|
||||||
|
this inference: `ClientTextRefusals.cs:117` documents a site where "the actual
|
||||||
|
site passes type `0` (Default), not `0x1A`". If retail passes `0` here, all
|
||||||
|
six lines belong in the scrolling chat window, all six are visible, and OP3's
|
||||||
|
routing is wrong in a way the user will see.
|
||||||
|
|
||||||
|
Fix: either byte-verify the type at the call sites and route accordingly, or —
|
||||||
|
if the sweep is out of scope this slice — file a register row recording the
|
||||||
|
assumed `ClientLocal` routing plus the 4-slot truncation, per CLAUDE.md's
|
||||||
|
"any commit that introduces a deviation adds its register row IN THE SAME
|
||||||
|
COMMIT". Right now there is neither evidence nor a row. (AD-75 covers the
|
||||||
|
UA/RA `MessageBoxA` → interface-text substitution; it says nothing about the
|
||||||
|
mouse-turning macro's six lines.)
|
||||||
|
|
||||||
|
### MUST-FIX 2 — the plan assigned this review a register ruling that OP3 did not file
|
||||||
|
|
||||||
|
`docs/plans/2026-08-10-options-panel-campaign.md:379-382`:
|
||||||
|
|
||||||
|
> The `0x21000017` docked `gmPanelUI` host variant — acdream ships the
|
||||||
|
> floating host only (**register row in OP3 if the review deems it a
|
||||||
|
> divergence**; retail exposes both).
|
||||||
|
|
||||||
|
I deem it a divergence: retail exposes both a floating and a docked
|
||||||
|
`gmPanelUI` host and acdream mounts only `0x2100006E`, so no gmPanelUI child
|
||||||
|
— Options included — can be docked. The commit files AD-74/75/76 and TS-74
|
||||||
|
but no row for this. It is inherited rather than introduced by OP3 (every
|
||||||
|
pre-existing panel already mounts floating-only), which is exactly why it has
|
||||||
|
gone unrowed until now, and why the plan routed the ruling here. One AD row,
|
||||||
|
cited at `RetailUiRuntime.MountOptionsPanel` / the shared host id, closes it.
|
||||||
|
|
||||||
|
### SHOULD-FIX 1 — `IsGrounded` returns false outside player mode, so "Exit to Character Selection" is unusable in fly mode and emits a mid-air message that is not true; the code comment asserts the opposite
|
||||||
|
|
||||||
|
`InteractionRetainedUiComposition.cs:322-324`:
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
IsGrounded: () => d.PlayerMode.IsPlayerMode
|
||||||
|
&& d.PlayerController.Controller is { IsAirborne: false },
|
||||||
|
```
|
||||||
|
|
||||||
|
with the comment (`:320-322`) claiming *"'on the ground' is meaningless
|
||||||
|
outside player mode, so the mid-air refusal never fires while flying/
|
||||||
|
spectating."* The code does the reverse: outside player mode `IsGrounded()`
|
||||||
|
is **false**, so `RequestExitToCharacterSelection`'s `else` branch
|
||||||
|
(`RetailUiRuntime.cs:1871-1873`) fires `CantLogOffMidAir`. In fly/chase-camera
|
||||||
|
mode the button confirms, refuses, and shows a message that is factually
|
||||||
|
wrong. The underlying expression is the project's established `playerOnGround`
|
||||||
|
shape (same three lines at `:322-324` feed the item-drop gate), so the shape
|
||||||
|
is fine — the comment is inverted and the not-in-player-mode case is
|
||||||
|
unconsidered. Impact is contained (Exit Game right beside it still works, and
|
||||||
|
retail has no fly mode) but the comment must not survive as-is.
|
||||||
|
|
||||||
|
### SHOULD-FIX 2 — Options is the only shared-geometry main panel that resizes horizontally and on all four edges; its width is silently discarded whenever a sibling is shown
|
||||||
|
|
||||||
|
`RetailUiRuntime.cs:1970-1980` gives Options `ResizeX = true`,
|
||||||
|
`ResizableEdges = Left|Right|Top|Bottom`, `MinWidth = 300f`,
|
||||||
|
`MinHeight = 200f`. Every one of its nine shared-rectangle siblings is
|
||||||
|
`ResizeX = false`, `ResizableEdges = Bottom`, with no explicit constraints
|
||||||
|
(Enumeration A). Consequences, all through
|
||||||
|
`RetailPanelUiController`'s one `_mainPanelGeometry`:
|
||||||
|
|
||||||
|
- A user widens Options to 450. `OnWindowResized` → `CaptureAndSynchronize…`
|
||||||
|
(`:224-247`) writes 450 into the shared rect; siblings ignore it
|
||||||
|
(`RetailWindowManager.ResizeTo:185` forces `width = frame.Width` when
|
||||||
|
`!ResizeX`). Open the character sheet, then reopen Options:
|
||||||
|
`PrepareMainPanelGeometry` → `ApplyWindowGeometry` (`:265-282`, `:306-314`)
|
||||||
|
resizes Options back to the sibling's 300. The width is lost. This
|
||||||
|
contradicts the OP3 gate script's own item 5 ("remembers its geometry
|
||||||
|
across a close/reopen").
|
||||||
|
- The 300/200 minimums are invented, not authored — `DatConstraintSource` is
|
||||||
|
not passed, so `ResolveConstraint` (`RetailWindowFrame.cs:160-163`) uses
|
||||||
|
the literals. Because all ten panels are authored 300 wide, `MinWidth=300`
|
||||||
|
currently pins Options to exactly its authored width and no clamp feedback
|
||||||
|
loop can start; that is luck, not design.
|
||||||
|
- The four-edge resize also means Options is the only panel whose *top/left*
|
||||||
|
drag moves the shared rect origin for every sibling.
|
||||||
|
|
||||||
|
Either match the sibling policy (`ResizeX = false`, bottom-only, drop the
|
||||||
|
literals) or make the whole shared group consistent — but not one panel with
|
||||||
|
its own rules inside a single-rectangle group. Needs the connected visual
|
||||||
|
gate either way; I cannot run the client.
|
||||||
|
|
||||||
|
### SHOULD-FIX 3 — no test pins the two seams the review was asked about: mutual exclusion against a *real* existing panel, and the new dialog's queue behavior
|
||||||
|
|
||||||
|
The new suites are thorough on the model (`OptionPageModelTests`, 20 facts)
|
||||||
|
and the controller's own content (`OptionsPanelControllerTests`, 15 facts),
|
||||||
|
and they do pin the F11 route on both sides
|
||||||
|
(`GameplayInputCommandControllerTests.cs:21` + `KeyBindingsRetailTests.cs:239`).
|
||||||
|
What is missing:
|
||||||
|
|
||||||
|
- **Panel mutual exclusion with an existing panel.** `RetailPanelCatalogTests`
|
||||||
|
asserts catalog membership only. No test registers, say, panel 11
|
||||||
|
(character) and panel 10 (options) in a `RetailPanelUiController` and
|
||||||
|
asserts that showing Options hides the character sheet — the exact
|
||||||
|
interaction the slice's headline claim ("shares retail's mutual exclusion
|
||||||
|
for free") rests on. The mechanism is exercised by
|
||||||
|
`RetailPanelUiControllerTests` with synthetic ids, so the risk is low, but
|
||||||
|
the claim is unpinned.
|
||||||
|
- **The confirmation dialog's queue key.** `RequestExitToCharacterSelection`
|
||||||
|
goes through `ShowConfirmation` → `MakeConfirmation` with the default
|
||||||
|
`queueKey = 2` (`RetailDialogFactory.cs:125-136`, `:10`), the same key every
|
||||||
|
existing confirmation uses — see NOTE 3. No test pins that the new dialog
|
||||||
|
shares the existing key rather than inventing one.
|
||||||
|
- No test asserts `ActivePageChanged` stays silent on a **dormant** Type-8
|
||||||
|
host (AD-73's contract). See NOTE 2.
|
||||||
|
|
||||||
|
Fixture/conformance suites are unaffected: `RetailLayoutFixtureGenerator`
|
||||||
|
gains one row (`+2` lines), `FixtureLoader` two new loaders, and no existing
|
||||||
|
fixture was regenerated (`git show --stat` shows only the new
|
||||||
|
`options_panel_2100006E_1000018D.json`). Existing catalog assertions in
|
||||||
|
`AppraisalUiControllerTests.cs:683-691` and
|
||||||
|
`EffectsUiControllerTests.cs:256-258` are negative assertions about
|
||||||
|
Examination/Effects and remain true. No count-based assertion over
|
||||||
|
`MountedPanels`/`ToolbarPanels` exists to go stale.
|
||||||
|
|
||||||
|
### SHOULD-FIX 4 — the Urgent Assistance / Report Abuse bodies are three-line, ~110-character-URL messages delivered into a 4-slot, 5-second, centered transient overlay
|
||||||
|
|
||||||
|
`OptionsPanelText.UrgentAssistanceUnavailable` / `ReportAbuseUnavailable`
|
||||||
|
(`OptionsPanelText.cs:106-118`) embed two `\n`s and the full
|
||||||
|
`support.turbine.com` URL, and go out through the same `ClientLocal` seam
|
||||||
|
(`OptionsPanelController.cs:171-174`). `AddText` only trims the ends
|
||||||
|
(`RuntimeCommunicationState.cs:195`), so the whole three-line body lands as a
|
||||||
|
**single** `SpewBoxEntry` that expires in 5 s. The *divergence* is registered
|
||||||
|
(AD-75), but its legibility is not: the user has 5 seconds to transcribe a
|
||||||
|
110-character URL from a transient overlay. Worth a look at the connected
|
||||||
|
gate; if it is unreadable, the honest fix is the chat transcript (which
|
||||||
|
scrolls back), not a longer timeout.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
### NOTE 1 — the mount-time `AfterApply` publish is harmless, but it does fire before any session exists
|
||||||
|
|
||||||
|
`MountOptionsPanel` calls `controller.ActivateTabs()` (`RetailUiRuntime.cs:1958`)
|
||||||
|
during `Initialize()`, which fires `ActivePageChanged(0, Gameplay)` →
|
||||||
|
`OptionPage.OnShown()` → `Apply()` → `AfterApply` →
|
||||||
|
`_bindings.Options.CommandBus().Publish(new SaveCharacterOptionsRuntimeCmd())`
|
||||||
|
(`:1945-1946`). `DeferredLiveSessionUiAuthority.Commands` returns
|
||||||
|
`NullCommandBus.Instance` when unbound (`InteractionUiRuntimeSources.cs:222-231`),
|
||||||
|
so there is **no NRE and no wire traffic** at mount. Post-connect, the same
|
||||||
|
publish fires on every window show and every tab switch; the router's
|
||||||
|
`SaveCharacterOptionsIfDirty` is dirty-gated (`LiveSessionRuntimeFactory.cs:359-372`),
|
||||||
|
so it is a no-op unless the batched module is actually dirty. Retail-cited
|
||||||
|
(`PlayerOptionPage::SaveCurrentValues @0x004F2710`'s `SaveToServer(0)` tail).
|
||||||
|
No action needed — recorded because "opening a UI panel sends a character-
|
||||||
|
options save" is the kind of thing a future reader will flag.
|
||||||
|
|
||||||
|
### NOTE 2 — `ActivePageChanged` cannot fire during dormant operation, but only because nobody calls `SwitchTo`
|
||||||
|
|
||||||
|
The event is raised from exactly one place: the tail of `SwitchTo`
|
||||||
|
(`UiTabPanel.cs:212-213`), after `ActivePageElementId` is updated (so handlers
|
||||||
|
read the new value — which `OptionsPanelController.OnHidden` at `:217` relies
|
||||||
|
on). `SwitchTo`'s only callers are `ActivateTabBehavior`'s default switch
|
||||||
|
(`:186`) and the click handlers *that method registers* (`:174`). Dormant
|
||||||
|
Type-8 hosts (vendor `0x100000B8`, character-sheet root `0x10000227`,
|
||||||
|
spellbook root `0x100002A8`, combat `0x100000A2` — AD-73) never run either,
|
||||||
|
so the event never fires for them. **But** `SwitchTo` is public and its own
|
||||||
|
doc (`:193-195`) invites direct calls — a future controller calling
|
||||||
|
`SwitchTo` on a dormant host WOULD raise it. Since the only subscriber is
|
||||||
|
`OptionsPanelController` on its own instance (`:131`), and it unsubscribes in
|
||||||
|
`Dispose` (`:233`), there is no existing consumer to affect today. Existing
|
||||||
|
`UiTabPanel` consumers: none subscribe; the added field is 8 bytes/instance.
|
||||||
|
|
||||||
|
### NOTE 3 — the new dialog respects the `OpenNextDialog` duplicate-key hazard because it does not introduce a key
|
||||||
|
|
||||||
|
`RequestExitToCharacterSelection` reuses `ShowConfirmation`
|
||||||
|
(`RetailUiRuntime.cs:534-541`) → `MakeConfirmation` with the default
|
||||||
|
`queueKey = DefaultQueueKey = 2` (`RetailDialogFactory.cs:10`, `:128`). Every
|
||||||
|
confirmation in the codebase uses that same key (only one `QueueKey` constant
|
||||||
|
exists outside the factory: `RetailDialogData.cs:19`, the property id). The
|
||||||
|
chat digest's DO-NOT-RETRY hazard is the `Dictionary.Add` duplicate at
|
||||||
|
`OpenNextDialog`; the existing guard at `RetailDialogFactory.cs:332-333`
|
||||||
|
(`if (_activeQueued.ContainsKey(queueKey)) return;`) already covers the
|
||||||
|
re-entrant case, and OP3 adds no new key for it to miss. Correct by
|
||||||
|
construction. Behavioral consequence: clicking Exit-to-char-select while a
|
||||||
|
server-driven confirmation is up queues behind it, same as every other
|
||||||
|
client-side confirmation.
|
||||||
|
|
||||||
|
### NOTE 4 — the Exit-Game / Escape / indicator-bar exit paths are genuinely shared and genuinely unchanged
|
||||||
|
|
||||||
|
`ExitGame: _bindings.Indicators.EndCharacterSession` (`RetailUiRuntime.cs:1942`)
|
||||||
|
resolves to `d.Window.Close` (`InteractionRetainedUiComposition.cs:688`) — the
|
||||||
|
same delegate behind (a) the indicator bar's end-session button, which wraps
|
||||||
|
it in `RequestEndCharacterSession`'s confirmation (`:1844-1851`), and (b) the
|
||||||
|
Escape key's terminal branch, via a *separate* `GameplayWindowCommands(d.Window.Close)`
|
||||||
|
instance (`SessionPlayerComposition.cs:1200` → `GameplayInputCommandController.cs:269`).
|
||||||
|
OP3 adds a third caller and modifies neither existing one. The refactor that
|
||||||
|
extracted `ResolveEndCharacterSessionConfirmMessage` (`:1832-1841`) is
|
||||||
|
behavior-preserving for `RequestEndCharacterSession` — same DAT id
|
||||||
|
`0x23000001`, same hash, same fallback string, same `DatLock` scope.
|
||||||
|
Retail-faithfulness of "Exit Game has no confirmation" is confirmed by the
|
||||||
|
research doc's `RecvNotice_EndCharacterSession(0)` branch
|
||||||
|
(`2026-08-10-keyboard-config-and-gameplay-tab.md:151-154`): param 0 sets
|
||||||
|
`m_shouldQuitOnLogout = 1` and skips both the dialog and the mid-air check —
|
||||||
|
which is also why OP3 correctly applies the mid-air refusal to
|
||||||
|
Exit-to-char-select only.
|
||||||
|
|
||||||
|
### NOTE 5 — settings-file schema and versioning are safe; the concurrent-write hazard is pre-existing and unchanged in kind
|
||||||
|
|
||||||
|
`SaveCameraTurning` goes through the existing `SaveSection`
|
||||||
|
(`SettingsStore.cs:250-252`, `:658-702`), which preserves every non-target
|
||||||
|
top-level key as raw text (`:663-683`) and re-stamps `version: 2`
|
||||||
|
unchanged (`CurrentSchemaVersion` at `:37` is untouched). `cameraTurning` is
|
||||||
|
a new top-level key, so: older acdream builds ignore it on read and preserve
|
||||||
|
it on write; newer builds fall back to `CameraTurningSettings.Default`
|
||||||
|
per-field when the section or a key is missing (`:224-241`). **No schema break
|
||||||
|
for existing `settings.json` readers.**
|
||||||
|
|
||||||
|
Concurrency: `SaveSection` is a non-atomic read-modify-`File.WriteAllText`,
|
||||||
|
and `SaveWindowLayout` is a *separate* `LoadMutableRoot`/`WriteMutableRoot`
|
||||||
|
path over the same file (`:435-462`). Interleaving either pair can clobber.
|
||||||
|
That hazard predates OP3; OP3 adds one more writer to the `SaveSection` side,
|
||||||
|
driven from a UI click handler on the same thread as the existing settings
|
||||||
|
saves. No new thread is introduced. Recorded, not charged to this slice.
|
||||||
|
|
||||||
|
### NOTE 6 — `LoadCameraTurning`/`SaveCameraTurning` correctly bypass the cached-snapshot pattern, and the interface widening is fully covered
|
||||||
|
|
||||||
|
`RuntimeSettingsController` gains two pass-throughs
|
||||||
|
(`RuntimeSettingsController.cs:444-467`) that read/write storage directly
|
||||||
|
rather than through the `RuntimeSettingsSnapshot` cache the
|
||||||
|
Display/Gameplay/Chat sections use. The implementer flagged this as
|
||||||
|
deliberate; it is correct here — a cached, change-notified copy exists to
|
||||||
|
drive live UI and there is no Camera/Input UI surface until OP6. The widened
|
||||||
|
`IRuntimeSettingsStorage` has exactly two implementers
|
||||||
|
(`JsonRuntimeSettingsStorage` at `:42`, the test fake at
|
||||||
|
`RuntimeSettingsControllerTests.cs:1280`), both updated. Save failures are
|
||||||
|
logged not thrown, matching every other `Set*` in the class.
|
||||||
|
|
||||||
|
### NOTE 7 — the five persisted camera preferences are a second, disconnected store for a value acdream already owns
|
||||||
|
|
||||||
|
`cameraTurning.mouseLookSensitivity` is persisted but never read by the
|
||||||
|
camera; the live sensitivity acdream actually uses lives in
|
||||||
|
`CameraPointerInputController` (`_chase.Sensitivity`, `_flySensitivity`,
|
||||||
|
`_orbitSensitivity` at `:110-111`, `:174-182`) and is adjusted by the
|
||||||
|
`AcdreamSensitivityUp/Down` actions. TS-74 mentions "(unwired) preferences"
|
||||||
|
in passing, but only in the context of the `UseMouseTurning` bit; the
|
||||||
|
five-preference store-without-consumer is not itself rowed. Low risk today
|
||||||
|
(nothing reads it), but OP6 will surface these in the Config tab, at which
|
||||||
|
point two stores for one concept becomes user-visible. Worth a sentence in
|
||||||
|
TS-74 rather than a new row.
|
||||||
|
|
||||||
|
Related: `SetSingleCharacterOptionRuntimeCmd` publishes directly on the bus
|
||||||
|
(`RetailUiRuntime.cs:1900-1903`), matching the established CH3 pattern at
|
||||||
|
`RuntimeSettingsTargets.cs:321-322` including its documented silent-drop-
|
||||||
|
while-disconnected behavior. The macro's own wrinkle: the six chat lines are
|
||||||
|
emitted unconditionally, so a drop while disconnected still prints "Turn to
|
||||||
|
Face Camera was changed from FALSE … to TRUE" when neither the wire send nor
|
||||||
|
the local bit write happened (`SendIfActive` skips the whole lambda, so
|
||||||
|
`TrySetOption` never runs). The Options panel is only reachable in-world, so
|
||||||
|
the window is narrow.
|
||||||
|
|
||||||
|
### NOTE 8 — `IDevToolsGameplayCommands.ToggleSettingsPanel` is now fully orphaned
|
||||||
|
|
||||||
|
Zero production callers remain (Enumeration C). The commit comment at
|
||||||
|
`GameplayInputCommandController.cs:242-244` describes it as having "no-op
|
||||||
|
wiring elsewhere"; there is none. Slated for OP9 per the same comment —
|
||||||
|
worth confirming it actually appears on OP9's list rather than relying on the
|
||||||
|
comment.
|
||||||
|
|
||||||
|
Minor inconsistency in the same area: `OptionsPanelController.Bind` logs a
|
||||||
|
loud warning when any of the seven Gameplay buttons fails to resolve
|
||||||
|
(`BindButton`, `:188-197`) but the close button (`:160-161`) fails silently
|
||||||
|
if `0x10000210` ever stops building as a `UiButton` — a panel you cannot
|
||||||
|
close by its X, with no log. One line to make it symmetric.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Verdict
|
||||||
|
|
||||||
|
**APPROVE-WITH-FIXES.**
|
||||||
|
|
||||||
|
The structural bet — register through `RetailPanelCatalog` +
|
||||||
|
`RetailPanelUiController` + the existing generic toolbar binder, rather than
|
||||||
|
building an Options-specific path — is the right one and is why the blast
|
||||||
|
radius on already-shipped panels is as small as it is. Both axes walk clean
|
||||||
|
on the mechanisms: exactly one toolbar button changed state, no other panel's
|
||||||
|
exclusion/stacking/deferred-restore behavior moved, the dialog queue key is
|
||||||
|
reused rather than invented, the settings schema is additive and
|
||||||
|
version-neutral, F11's binding predates the slice and its old target was a
|
||||||
|
proven no-op, and the new `UiTabPanel` event cannot fire on a dormant host.
|
||||||
|
The two MUST-FIX items are an unverified chat-routing decision with a
|
||||||
|
demonstrable line-loss consequence, and a register row the campaign plan
|
||||||
|
explicitly assigned to this review. Neither blocks the mechanism; both must
|
||||||
|
land before the connected gate is meaningful, since the first one changes
|
||||||
|
what the gate is supposed to observe.
|
||||||
476
docs/research/2026-08-11-op3-review-mechanism.md
Normal file
476
docs/research/2026-08-11-op3-review-mechanism.md
Normal file
|
|
@ -0,0 +1,476 @@
|
||||||
|
# Campaign OP slice OP3 — MECHANISM-FAITHFULNESS review
|
||||||
|
|
||||||
|
**Commit under review:** `9d26ecc6` ("feat(ui): Campaign OP slice OP3 — Options
|
||||||
|
panel shell, open paths, Gameplay tab")
|
||||||
|
**Contract:** `docs/plans/2026-08-10-options-panel-campaign.md` §4 OP3 + §2
|
||||||
|
D1/D5/D6/D9
|
||||||
|
**Oracles:** `docs/research/2026-08-10-options-panel-structure.md` §5/§6/§10.4;
|
||||||
|
`docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §1–§4/§7.1;
|
||||||
|
`docs/research/named-retail/acclient_2013_pseudo_c.txt` +
|
||||||
|
`docs/research/named-retail/acclient.h` (re-read directly for every decomp
|
||||||
|
claim the code's comments make).
|
||||||
|
**Lens:** mechanism faithfulness only (blast radius is the other lens).
|
||||||
|
**Method:** read-only. No build, no test run, no client launch.
|
||||||
|
|
||||||
|
## VERDICT: APPROVE-WITH-FIXES
|
||||||
|
|
||||||
|
One MUST-FIX (a byte-verifiable wrong `LogTextType` on six user-visible chat
|
||||||
|
lines this slice ships), six SHOULD-FIX, eight NOTEs. The core ports —
|
||||||
|
`OptionPage`/`BoolOptionRow`, the mount, the two open paths, the seven-button
|
||||||
|
dispatch, the mouse-turning macro — are faithful and, unusually for a slice
|
||||||
|
this size, every element id and every retail anchor I re-checked held up.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 0. What I verified as CORRECT (evidence first)
|
||||||
|
|
||||||
|
These are not filler: each one was a candidate finding that the source refuted.
|
||||||
|
|
||||||
|
### 0.1 `OptionPage`/`BoolOptionRow` vs the decomp — exact
|
||||||
|
|
||||||
|
Re-read at `acclient_2013_pseudo_c.txt:245757` (`OptionPage::SaveCurrentValues
|
||||||
|
@0x004F2C60`), `:245783` (`RestoreDefaultValues @0x004F2CB0`), `:245809`
|
||||||
|
(`RestoreSavedValues @0x004F2D00`), `:245834` (`Changed @0x004F2D60`),
|
||||||
|
`:245374` (`PlayerOptionPage::OnVisibilityChanged @0x004F26E0`), `:245390`
|
||||||
|
(`PlayerOptionPage::SaveCurrentValues @0x004F2710`), `:146926-146990`
|
||||||
|
(`UIOption_Checkbox::Changed/SaveCurrentValue/RestoreSavedValue/
|
||||||
|
RestoreDefaultValue/SetDefaultValue/SetCurrentValue`).
|
||||||
|
|
||||||
|
| Contract item | Retail | `OptionPageModel.cs` | Verdict |
|
||||||
|
|---|---|---|---|
|
||||||
|
| LED click applies immediately | `0x00486983-88`: `SetAttribute_Bool(0xE, v); vtable->Apply(1)` | `:82-86` `_current = value; _apply?.Invoke(value)` | ✅ |
|
||||||
|
| Apply commits **unconditionally** | `0x004F2C71-93`: no `Changed` test in the loop | `:178-183` plain `foreach` | ✅ |
|
||||||
|
| Apply flushes the blob | `0x004F271D` `SaveToServer(pm, 0)` then tailcall | `:182` `AfterApply?.Invoke()` | ✅ |
|
||||||
|
| Reset reverts **only Changed** | `0x004F2D1E`: `if (ecx != 0 && vtable+0x2BC() != 0)` | `:192` `.Where(row => row.Changed)` | ✅ |
|
||||||
|
| Reset applies live | `0x00486913` `Apply(0)` | `:90-94` re-invokes `_apply` | ✅ |
|
||||||
|
| Defaults applies live, no commit | `0x004F2CC1-E3` unconditional; `0x00486943` `Apply(0)`; never touches `m_saved` | `:199-203` / `:96-100` | ✅ |
|
||||||
|
| Defaults never disable-gated | `0x004F27D0` only fetches `0x100001FC`/`0x100001FD`; `0x100001FE` absent | no gate exists in the model | ✅ |
|
||||||
|
| Hide reverts uncommitted | `0x004F2701` `RestoreSavedValues` | `:213` `OnHidden() => Reset()` | ✅ |
|
||||||
|
| Show applies + commits | `0x004F26F4` `SaveCurrentValues` | `:208` `OnShown() => Apply()` | ✅ |
|
||||||
|
|
||||||
|
`Changed => _saved != _current` matches `0x004868D7` exactly, and
|
||||||
|
`OptionPage.Changed` matches `0x004F2D7B`'s any-row short-circuit.
|
||||||
|
|
||||||
|
### 0.2 The mount — both claims independently confirmed from the fixtures
|
||||||
|
|
||||||
|
- `tests/AcDream.App.Tests/UI/Layout/fixtures/options_panel_2100006E_1000018D.json:1-6`
|
||||||
|
— root `Id: 268435853` = **`0x1000018D`**, `Type: 8`, 300 × 362, i.e. the
|
||||||
|
import through host `0x2100006E` really does resolve to the base-merged
|
||||||
|
tab host at the authored slot extent.
|
||||||
|
- Same fixture `:583-587` — the slot element itself authors
|
||||||
|
`"268435497"` (= `0x10000029`, the `gmPanelUI` stack key) with
|
||||||
|
`UnsignedValue: 10`. This corroborates research §10.1's "stack key
|
||||||
|
`P0x10000029 = 10`" *from the DAT*, not just from the decomp.
|
||||||
|
- `tests/.../fixtures/toolbar_21000016.json:7869` — element `268435867` =
|
||||||
|
**`0x1000019B`** authors the same `"268435497": UnsignedValue: 10` **and**
|
||||||
|
`"18"` (= `P0x12`) `= 268435482` (= `0x1000001A`).
|
||||||
|
- `0x10000049` (`RestorePreviousPropertyId`) is **absent** from the options
|
||||||
|
slot (`grep -c 268435529` → 0), so `RegisterMainPanel(..., restorePrevious:
|
||||||
|
false)` is the correct read.
|
||||||
|
|
||||||
|
**Is `P0x10000029` the property `RetailPanelUiController` consumes, and did
|
||||||
|
`P0x12` silently win?** `ToolbarController.cs:49` `PanelIdAttribute =
|
||||||
|
0x10000029u`; `:170-175` collects `(panelId, button)` pairs from exactly that
|
||||||
|
property for the seven ids in `PanelButtonIds` (`:54-55`, which already
|
||||||
|
included `0x1000019B`); `RetailUiRuntime.BindToolbarPanelButtons` `:677-687`
|
||||||
|
maps panel id → window name → `ToggleWindow`. **`P0x12` is consumed nowhere** —
|
||||||
|
`grep -rn "0x12u\b" src/AcDream.App/UI/` returns nothing, and neither
|
||||||
|
`ElementReader`, `LayoutImporter`, `DatWidgetFactory` nor `UiButton` reads an
|
||||||
|
authored input-action id. So there is exactly ONE route, no double-toggle, and
|
||||||
|
the implementer's "no new plumbing beyond the catalog entry" claim is accurate
|
||||||
|
(`RetailPanelCatalog.cs:30,42` are the only two new lines). Retail's own
|
||||||
|
toolbar highlight path (`gmToolbarUI::RecvNotice_SetPanelVisibility
|
||||||
|
@0x004BD300`, cited at `ToolbarController.cs:395`) is panel-id-based too, so
|
||||||
|
the surviving route is the retail-consistent one.
|
||||||
|
|
||||||
|
**Mutual exclusion** is real, not asserted: `RetailUiRuntime.ToggleWindow`
|
||||||
|
`:597-600` routes any catalog-known name into `_panelUi.TogglePanel(panelId)`,
|
||||||
|
and `RetailPanelUiController.SetPanelVisibility` `:154-172` hides the previous
|
||||||
|
`_activePanel` before showing the requested one.
|
||||||
|
|
||||||
|
### 0.3 F11 — the binding really did pre-exist, in the right table
|
||||||
|
|
||||||
|
`src/AcDream.UI.Abstractions/Input/KeyBindings.cs:216` sits inside
|
||||||
|
`RetailDefaults()` (opens `:147`), binding `Key.F11 / None →
|
||||||
|
InputAction.ToggleOptionsPanel`, matching
|
||||||
|
`docs/research/named-retail/retail-default.keymap.txt:148`
|
||||||
|
(`ToggleOptionsPanel [ "" [ 0 DIK_F11 ] ]`). The commit changed only the
|
||||||
|
*landing*: `GameplayInputCommandController.cs:238-246` now calls
|
||||||
|
`_retained.ToggleOptionsPanel()` (→ `RetailUiRuntime.ToggleWindow(
|
||||||
|
WindowNames.Options)` → the panel controller) instead of the dead
|
||||||
|
`_devTools.ToggleSettingsPanel()`. `ToggleSettingsPanel` itself is untouched
|
||||||
|
and now unreachable from any action — exactly what D1/OP9 contract for.
|
||||||
|
|
||||||
|
### 0.4 The seven buttons vs research §6 / §1–§4
|
||||||
|
|
||||||
|
Element ids in `OptionsPanelController.cs:62-68` (`0x10000203`, `0x10000204`,
|
||||||
|
`0x10000205`, `0x10000206`, `0x10000207`, `0x100005CC`, `0x10000617`) and the
|
||||||
|
close button `0x10000210` (`:58`) match research §6's table and §10.1
|
||||||
|
one-for-one, and `OptionsPanelControllerTests.cs:247-268` pins all seven
|
||||||
|
against the committed fixture (so a future regeneration that drops an id fails
|
||||||
|
the build rather than degrading to a logged no-op).
|
||||||
|
|
||||||
|
- **Exit Game** → `_bindings.Indicators.EndCharacterSession` directly
|
||||||
|
(`RetailUiRuntime.cs:1942`) — **no** confirmation, **no** mid-air check.
|
||||||
|
Matches §3: `m_bLogoutConfirmed` is set by the button itself and `UseTime`
|
||||||
|
takes the `QueueUIMode(0x10000009)` epilogue branch, which never reaches the
|
||||||
|
`transient_state` test.
|
||||||
|
- **Exit to Character Selection** → confirm FIRST, mid-air check on accept
|
||||||
|
(`:1846-1860`). Matches `RecvNotice_EndCharacterSession(1)` (dialog) →
|
||||||
|
`gmGamePlayUI::UseTime @0x004EA3A0` (the `m_bLogoutConfirmed`-gated drain
|
||||||
|
that owns the `transient_state & 1` test). Ordering is right.
|
||||||
|
- The confirm text is **DAT-resolved**, not hard-coded:
|
||||||
|
`ResolveEndCharacterSessionConfirmMessage()` `:1834-1845` resolves
|
||||||
|
`ID_Client_EndCharacterSessionConfirm` through
|
||||||
|
`DatStringResolver.ComputeHash` against table `0x23000001`.
|
||||||
|
- **Mid-air refusal typing is EXACTLY right.** Retail:
|
||||||
|
`ECM_UI::SendNotice_DisplayStringInfo(0x1A, "Cannot log off while in
|
||||||
|
mid-air.")`. acdream: `RetailLogTextType.ClientLocal` =
|
||||||
|
`RetailLogTextType.cs:60` **`0x1A`**. Same constant. The string itself is
|
||||||
|
byte-anchored at VA `0x007C29C0` in `ClientTextRefusals.cs`.
|
||||||
|
- **Inert buttons are genuinely inert.** `Bind` `:163-174` never calls
|
||||||
|
`BindButton` for `0x10000204`/`0x10000205`; `BindButton` `:188-197`
|
||||||
|
early-returns on a null handler. Tests `:220-245` assert zero callbacks. No
|
||||||
|
invented text anywhere in the slice.
|
||||||
|
|
||||||
|
### 0.5 The mouse-turning macro vs `SetMouseTurningDefaults @0x0049E8F0`
|
||||||
|
|
||||||
|
Re-read at `acclient_2013_pseudo_c.txt:169512-169640`. Every structural claim
|
||||||
|
holds:
|
||||||
|
|
||||||
|
| # | Retail condition / target | `MouseTurningSettingsMacro.cs` | ✓ |
|
||||||
|
|---|---|---|---|
|
||||||
|
| 1 | `GetCurrentValue() != 0.949999988f` → `SetCurrentValue(0.95f)` `@0x0049E93C` | `:60-65` vs `MouseTurningTarget.Stiffness = 0.95f` | ✅ |
|
||||||
|
| 2 | `!= 50f` → `50f` `@0x0049E9AC` | `:67-72` | ✅ |
|
||||||
|
| 3 | `!= 0.699999988f` → `0.7f` `@0x0049EA1C` | `:74-79` | ✅ |
|
||||||
|
| 4 | `m_pSlope->GetValue() == 1` → `SetCurrentValue(0)` `@0x0049EA69-77` | `:81-86` (target false) | ✅ |
|
||||||
|
| 5 | `m_pInvert->GetValue() == 0` → `SetCurrentValue(1)` `@0x0049EABB-C9` | `:88-93` | ✅ |
|
||||||
|
| 6 | `m_pMouseTurn->GetValue() == 0` → `SetCurrentValue(1)` `@0x0049EB0D-1B` | `:95-99` | ✅ |
|
||||||
|
| tail | `this->vtable->SaveCurrentValues()` `@0x0049EB57` | `RetailUiRuntime.cs:1908` `ConfigPage.Apply()` → `AfterApply` → `SaveCharacterOptionsRuntimeCmd` | ✅ |
|
||||||
|
|
||||||
|
The tail is faithful because `gmConfigUI : PlayerOptionPage, gmNoticeHandler`
|
||||||
|
(`acclient.h:55846`), so its `SaveCurrentValues` vtable slot IS
|
||||||
|
`PlayerOptionPage::SaveCurrentValues` → `SaveToServer(pm, 0)`. acdream's
|
||||||
|
`SaveOptions` is dirty-AND-seed-gated
|
||||||
|
(`RuntimeCharacterOptionsState.TryFlush` `:922-941`), which is exactly
|
||||||
|
`SaveToServer(force: 0)`'s `if (m_bDirty != 0 || arg2 != 0)`.
|
||||||
|
|
||||||
|
The five client-local vs one server-synced split is right, and
|
||||||
|
`UseMouseTurning` really is an **auto-save** id in OP1's table
|
||||||
|
(`CharacterOptionTable.cs:160`: `Add(UseMouseTurning, false, 0x00400000u,
|
||||||
|
true, false)` → `autoSave: true`), so the `SetSingleCharacterOptionRuntimeCmd`
|
||||||
|
publication reaches `TrySetOption` and emits a `0x0005` immediately, after the
|
||||||
|
local bit write. Correct per lane C's split.
|
||||||
|
|
||||||
|
`%f` → `.ToString("F6")` is the right rendering (retail's `%f` default
|
||||||
|
precision is 6; the double promotions of `0.45f`/`0.55f`/`0.7f` all round to
|
||||||
|
the same six decimals as .NET's float `F6`).
|
||||||
|
|
||||||
|
### 0.6 TS-74's "camera mode ABSENT" verdict — spot-check holds
|
||||||
|
|
||||||
|
`src/AcDream.UI.Abstractions/Input/MouseLookState.cs:5-32` documents and
|
||||||
|
implements only retail's MMB-hold `CameraInstantMouseLook` (three transitions:
|
||||||
|
`Press`/`Release`/`OnWantCaptureMouseChanged`; `ApplyDelta` scales mouse-X into
|
||||||
|
a turn adjustment). There is no persistent "turn to face camera" mode. The only
|
||||||
|
other `UseMouseTurning` references in the tree are
|
||||||
|
`GameplaySettings.cs:42/65`, `SettingsPanel.cs:366-368` (the unrendered ImGui-era
|
||||||
|
surface OP9 retires) and `SettingsStore.cs:161/617` — i.e. storage, no consumer.
|
||||||
|
**The ABSENT verdict is correct.**
|
||||||
|
|
||||||
|
### 0.7 Register rows
|
||||||
|
|
||||||
|
`AD-74`, `AD-75`, `AD-76`, `TS-74` are accurate against the code they cite,
|
||||||
|
carry the five-column shape plus retail anchors, and the section counts are
|
||||||
|
bumped correctly (AD 53 → **56**, TS 40 → **41**) with the header narrative
|
||||||
|
updated in the same hunk. No resurrection of retired rows in the diff (the
|
||||||
|
`feedback_register_revert_resurrection` check).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. MUST-FIX
|
||||||
|
|
||||||
|
### M1 — the six mouse-turning chat lines are typed `0x1A`; retail types them `0x07`
|
||||||
|
|
||||||
|
**Where:** `src/AcDream.App/UI/RetailUiRuntime.cs:1906-1907` (`foreach (string
|
||||||
|
line in result.ChatLines) _bindings.Options.DisplaySystemMessage(line);`) →
|
||||||
|
`src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:815-816`
|
||||||
|
(`DisplaySystemMessage: text => d.Communication.AddText(text,
|
||||||
|
RetailLogTextType.ClientLocal)`).
|
||||||
|
|
||||||
|
**Retail, byte-anchored.** All six emission sites in
|
||||||
|
`gmConfigUI::SetMouseTurningDefaults` call the same chokepoint with the same
|
||||||
|
type argument (`acclient_2013_pseudo_c.txt:169512-169640`, addresses
|
||||||
|
`0x0049E97C`, `0x0049E9EC`, `0x0049EA5C`, `0x0049EAAE`, `0x0049EB00`,
|
||||||
|
`0x0049EB52`):
|
||||||
|
|
||||||
|
```
|
||||||
|
0049e972 *(uint32_t*)((char*)var_2c)[4] = 7; <- BN's rendering of the pushed type
|
||||||
|
0049e97c ClientSystem::AddTextToScroll(ClientUISystem::GetUISystem(), var_2c, 1, 0);
|
||||||
|
```
|
||||||
|
|
||||||
|
The five-parameter signature is
|
||||||
|
`ClientSystem::AddTextToScroll(this, char const* text, uint32 type, uint8
|
||||||
|
allowPluginFilter, uint32 windowId)` (`acclient_2013_pseudo_c.txt:148528`), and
|
||||||
|
the fully-decoded sibling site three functions away renders all five:
|
||||||
|
`0x004887B3` → `AddTextToScroll(GetUISystem(), <"The key for Camera Zoom In has
|
||||||
|
been…">, 7, 1, 0)` (`:148984`). The stray `= 7` line immediately preceding each
|
||||||
|
truncated call is the missing `type` argument in the same idiom. **Type = 7.**
|
||||||
|
|
||||||
|
`0x07` is `Magic` / `colorLightBlue` `#3FBFFF`
|
||||||
|
(`docs/research/2026-08-09-chat-retail-color-table.md:151`). acdream ships
|
||||||
|
`0x1A ClientLocal` — **bright red**, and per that doc's §3.2 `0x1A` also
|
||||||
|
*bypasses the timestamp prefix and the chat log file*, which `0x07` does not.
|
||||||
|
So this is a colour divergence AND a routing divergence on six lines the OP3
|
||||||
|
gate script explicitly asks the user to eyeball (step 15).
|
||||||
|
|
||||||
|
**Why this is MUST and not SHOULD:** the answer is byte-verifiable and
|
||||||
|
unambiguous, the fix is one typed argument, the divergence is user-visible at
|
||||||
|
the connected gate the user is about to run, `OptionsPanelText.cs`'s own doc
|
||||||
|
calls these "byte-verified retail string literals" (true of the text, not of
|
||||||
|
the typing), and **no register row covers it** — an undocumented deviation is,
|
||||||
|
per the register rules, "a bug twice over".
|
||||||
|
|
||||||
|
**Fix:** give the Options bindings a typed message seam (e.g.
|
||||||
|
`Action<string, RetailLogTextType>`, or a second delegate) and emit the six
|
||||||
|
macro lines as `RetailLogTextType.Magic (0x07)`. Keep `ClientLocal (0x1A)` for
|
||||||
|
the mid-air refusal (§0.4 — that one is exact) and for the UA/RA adaptation
|
||||||
|
(AD-75 already records that retail used a native `MessageBoxA` there, so there
|
||||||
|
is no retail type to match).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. SHOULD-FIX
|
||||||
|
|
||||||
|
### S1 — the Gameplay tab is given an `OptionPage`; retail's Gameplay page is not one
|
||||||
|
|
||||||
|
**Evidence.** `acclient.h:55857`:
|
||||||
|
`struct __cppobj gmGameplayOptionsUI : UIElement_Field, gmNoticeHandler` — it
|
||||||
|
derives from neither `OptionPage` (`acclient.h:54418`) nor `PlayerOptionPage`
|
||||||
|
(`:55739`). Its class id is the Gameplay page slot's authored class
|
||||||
|
(`UIElement::RegisterElementClass(0x10000029, gmGameplayOptionsUI::Create)` at
|
||||||
|
`acclient_2013_pseudo_c.txt:169244`, and research §10.1 lists page class
|
||||||
|
`0x10000029` for the Gameplay page). The three *other* pages do derive:
|
||||||
|
`gmCharacterSettingsUI` (`:55745`), `gmChatOptionsUI` (`:55817`), `gmConfigUI`
|
||||||
|
(`:55846`), all `: PlayerOptionPage`.
|
||||||
|
|
||||||
|
Consequence in retail: showing/hiding the Gameplay tab runs plain
|
||||||
|
`UIElement::OnVisibilityChanged` — **no** `SaveCurrentValues`, **no**
|
||||||
|
`SaveToServer`, no baseline traffic of any kind.
|
||||||
|
|
||||||
|
**What OP3 ships instead.** `OptionsPanelController.cs:125-129` creates an
|
||||||
|
`OptionPage` for *all four* slots including `GameplayPageId`, each with the
|
||||||
|
same `AfterApply`, and `RetailUiRuntime.cs:1944-1946` wires that `AfterApply`
|
||||||
|
to `Publish(new SaveCharacterOptionsRuntimeCmd())`. So:
|
||||||
|
|
||||||
|
1. `controller.ActivateTabs()` at mount (`RetailUiRuntime.cs:1954`) fires
|
||||||
|
`ActivePageChanged(0 → 0x10000212)` → `GameplayPage.OnShown()` → `Apply()`
|
||||||
|
→ a `SaveOptions` publication **during UI mount, before login state
|
||||||
|
exists**. It is inert today only because OP1's M1 fix makes `TryFlush`
|
||||||
|
refuse before `HasServerSeed` (`RuntimeCharacterOptionsState.cs:928`).
|
||||||
|
2. Every later entry into the Gameplay tab publishes another `SaveOptions`.
|
||||||
|
After any batched (non-auto-save) Character-tab edit — i.e. from OP4 on —
|
||||||
|
switching to the Gameplay tab flushes the `0x01A1` blob at a moment retail
|
||||||
|
would not (retail waits for Apply, logout, or the 480 s timer).
|
||||||
|
|
||||||
|
**The justification in the code is the inverse of the decomp.**
|
||||||
|
`OptionPageModel.cs:136-144` argues the empty page still flushes because
|
||||||
|
"retail's `SaveCurrentValues` flushes the blob regardless of whether THIS
|
||||||
|
page's own rows changed anything". That sentence is true — but only for a page
|
||||||
|
that *is* a `PlayerOptionPage`, which the Gameplay page is not. This is
|
||||||
|
precisely the `2026-08-05-c4-closeout-handoff.md` failure class: a contract
|
||||||
|
asserting a mechanism that does not exist.
|
||||||
|
|
||||||
|
**And two green tests pin the wrong shape** —
|
||||||
|
`OptionPageModelTests.cs:48` `EmptyPage_Apply_StillInvokesAfterApply` and
|
||||||
|
`:63` `EmptyPage_OnShown_InvokesAfterApply` — the same "green tests pinning a
|
||||||
|
wrong shape" that killed the CH3 builder (plan §4 OP1 cites it).
|
||||||
|
|
||||||
|
**Fix:** do not construct an `OptionPage` for `GameplayPageId` (or construct it
|
||||||
|
with `AfterApply = null` and document *why*), leave `OnActivePageChanged`
|
||||||
|
tolerant of a slot with no page model (it already uses `TryGetValue`), and
|
||||||
|
retarget the two tests at a *synthetic* empty `PlayerOptionPage`-shaped page so
|
||||||
|
the model-level property is still pinned without asserting it about the
|
||||||
|
Gameplay tab. Amend the `OptionPageModel` XML doc's empty-page paragraph to
|
||||||
|
say `gmGameplayOptionsUI` is not an `OptionPage` at all.
|
||||||
|
|
||||||
|
### S2 — `OptionPage` has no `OnOptionChanged` seam, so "Apply/Reset disable when clean" is only half-ported
|
||||||
|
|
||||||
|
Retail runs `this->vtable->OnOptionChanged(0)` as the **last statement of all
|
||||||
|
three verbs** — `0x004F2C95` (Apply), `0x004F2CE5` (Defaults), `0x004F2D4A`
|
||||||
|
(Reset) — and `UIOption::HandleDialogAndNotices @0x004EFB90` additionally calls
|
||||||
|
`m_pOCH->OnOptionChanged(this)` on a user click (`Apply(1)` only). The override
|
||||||
|
that matters, `PlayerOptionPage::OnOptionChanged @0x004F27D0`
|
||||||
|
(`acclient_2013_pseudo_c.txt:245433-245455`), is the ONLY thing that
|
||||||
|
enable-gates the buttons:
|
||||||
|
|
||||||
|
```
|
||||||
|
eax = GetChildRecursive(page, 0x100001FC); // Apply
|
||||||
|
eax_1 = GetChildRecursive(page, 0x100001FD); // Reset
|
||||||
|
if (OptionPage::Changed(page) == 0) { SetState(0x0D) on both } // disabled
|
||||||
|
else { SetState(0x01) on both } // enabled
|
||||||
|
```
|
||||||
|
`0x100001FE` (Defaults) is never fetched — which is exactly research §10.4's
|
||||||
|
point 3.
|
||||||
|
|
||||||
|
The contract (§4 OP3) lists "Apply/Reset disable when clean" as an
|
||||||
|
`OptionPageModel` requirement. The model ships only the `Changed` predicate
|
||||||
|
(`:173`) with **no notification and no verb-tail hook**, so nothing can react
|
||||||
|
to a value change without polling, and each of OP4/OP5/OP6 will have to invent
|
||||||
|
its own driver — three chances to get retail's call order wrong.
|
||||||
|
|
||||||
|
**Fix (small, and it belongs here rather than in OP4):** add
|
||||||
|
`Action? OnOptionChanged { get; set; }` to `OptionPage`, invoke it as the last
|
||||||
|
statement of `Apply`/`Reset`/`Defaults`, and give `BoolOptionRow` a
|
||||||
|
page-notification callback fired from `SetCurrentValue` only (retail's
|
||||||
|
`Apply(1)`-only path) — not from `RestoreSavedValue`/`RestoreDefaultValue`,
|
||||||
|
which retail calls with `Apply(0)` precisely so the page can notify once
|
||||||
|
itself.
|
||||||
|
|
||||||
|
### S3 — `IsGrounded`'s `IsPlayerMode` conjunction emits the mid-air refusal where retail emits nothing, and the comment says the opposite
|
||||||
|
|
||||||
|
`InteractionRetainedUiComposition.cs:807-810`:
|
||||||
|
```csharp
|
||||||
|
// "on the ground" is meaningless outside player mode, so the mid-air
|
||||||
|
// refusal never fires while flying/spectating.
|
||||||
|
IsGrounded: () => d.PlayerMode.IsPlayerMode
|
||||||
|
&& d.PlayerController.Controller is { IsAirborne: false },
|
||||||
|
```
|
||||||
|
`RetailUiRuntime.cs:1852-1859` takes the **else** branch — i.e. *emits*
|
||||||
|
"Cannot log off while in mid-air." — whenever `IsGrounded()` is false. With
|
||||||
|
`IsPlayerMode == false` (or a null `Controller`), `IsGrounded()` is false, so
|
||||||
|
the refusal **does** fire, which is the opposite of what the comment asserts.
|
||||||
|
|
||||||
|
Retail's drain (`gmGamePlayUI::UseTime @0x004EA3A0`, research §2.2) reaches the
|
||||||
|
`transient_state & 1` test only inside `else if (smartbox->player)`; with no
|
||||||
|
player it emits nothing at all.
|
||||||
|
|
||||||
|
The mapping itself is right where it matters —
|
||||||
|
`PlayerMovementController.cs:441` `IsAirborne => !_body.OnWalkable` is exactly
|
||||||
|
retail's `transient_state & 1` (`ON_WALKABLE`) inverted. It is only the
|
||||||
|
no-player fallback that misbehaves, and only by producing a misleading line.
|
||||||
|
|
||||||
|
**Fix:** either drop the `IsPlayerMode` conjunction from *this* binding (the
|
||||||
|
`playerOnGround` shape at `:322-324` is a different question — there it gates
|
||||||
|
a positive action), or make the binding tri-state (`bool?`) so "no player"
|
||||||
|
maps to retail's silent no-op. Either way the comment must be corrected.
|
||||||
|
|
||||||
|
### S4 — file the docked-`gmPanelUI`-host register row the plan delegated to this review
|
||||||
|
|
||||||
|
Plan §5: "The `0x21000017` docked `gmPanelUI` host variant — acdream ships the
|
||||||
|
floating host only (**register row in OP3 if the review deems it a
|
||||||
|
divergence**; retail exposes both)."
|
||||||
|
|
||||||
|
It is a divergence — retail authors both hosts for the same slot
|
||||||
|
(research §10.1) and a retail user can dock the Options panel; acdream can
|
||||||
|
only float it. `grep -n "21000017\|docked\|gmFloatyPanelUI"
|
||||||
|
docs/architecture/retail-divergence-register.md` returns **nothing**, so there
|
||||||
|
is no existing row.
|
||||||
|
|
||||||
|
Two nuances for whoever writes it: (a) OP3 did not *introduce* this — every
|
||||||
|
`gmPanelUI` sibling already ships floating-only via
|
||||||
|
`RetailWindowFrame.Mount` + `RegisterMainPanel`, so the row should be scoped
|
||||||
|
client-wide (all registered main panels), not "the Options panel"; (b) it is
|
||||||
|
an AD (adaptation), not a TS.
|
||||||
|
|
||||||
|
### S5 — TS-74's "five unwired preferences" is carried only in the Risk column
|
||||||
|
|
||||||
|
The row's What/Where is about the missing camera *mode*; the fact that all five
|
||||||
|
`CameraTurningSettings` values land in a brand-new `settings.json`
|
||||||
|
`cameraTurning` section that **no subsystem reads** appears only as a
|
||||||
|
parenthetical in the Risk column ("only the (unwired) preferences
|
||||||
|
persisting"). That has a concrete user-visible consequence the row does not
|
||||||
|
state: the chat lines quote a *from* value (`0.550000`,`0.450000`,`40.000000`
|
||||||
|
on a fresh profile — `CameraTurningSettings.Default`) that describes no live
|
||||||
|
client state, while acdream's actually-live sensitivity lives elsewhere
|
||||||
|
(`MouseLookState.SensitivityPerPixel`, F8/F9-adjustable).
|
||||||
|
|
||||||
|
**Fix:** extend TS-74's What/Where to name the five keys as store-only with no
|
||||||
|
consumer (OP6 is their contracted home), so the row can be retired precisely
|
||||||
|
rather than argued about later.
|
||||||
|
|
||||||
|
### S6 — three gate-visible behaviours missing from the OP3 test script
|
||||||
|
|
||||||
|
`docs/research/2026-08-11-campaign-op-test-script.md` is otherwise strong
|
||||||
|
(15 steps, retail-expected outcome stated for each, INERT explicitly
|
||||||
|
contracted, "what to report" and "not in scope" sections). Gaps:
|
||||||
|
|
||||||
|
1. **The toolbar Options button changed state, not just behaviour.** Before
|
||||||
|
this commit, panel id 10 was not in `RetailPanelCatalog`, so
|
||||||
|
`BindPanelButtons`'s `isAvailable(10)` returned false and the button was
|
||||||
|
**ghosted/disabled** (`ToolbarController.cs:379-381`). Step 3 should say
|
||||||
|
"the button is no longer greyed out, and highlights while the panel is
|
||||||
|
open" (`SetPanelOpen` → `Highlight`/`Normal`, `:393-405`).
|
||||||
|
2. **Nothing proves the `UseMouseTurning` `0x0005` reached ACE.** TS-74 says
|
||||||
|
the bit is sent; the only user-observable proof is a relogin (after which a
|
||||||
|
second click should emit no "Turn to Face Camera" line, because
|
||||||
|
`IsUseMouseTurningEnabled` reads the server-seeded `Options2`). Add a step.
|
||||||
|
3. **No cross-relaunch check on the five new preferences.** Step 15's "second
|
||||||
|
click in the same session sees zero lines" only covers in-session state; a
|
||||||
|
relaunch is what exercises the new `cameraTurning` `settings.json` section.
|
||||||
|
|
||||||
|
(If M1 is fixed, step 15 should also state the expected *colour* — light blue,
|
||||||
|
not the red the SpewBox uses for client-local refusals.)
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. NOTES (no action required this slice; carry into OP4–OP6)
|
||||||
|
|
||||||
|
- **N1 — flush/commit ordering.** Retail's `PlayerOptionPage::SaveCurrentValues`
|
||||||
|
calls `SaveToServer` at `0x004F271D` and *then* tailcalls
|
||||||
|
`OptionPage::SaveCurrentValues`; acdream commits baselines first, then
|
||||||
|
`AfterApply`. Content-identical (every value is already live before Apply is
|
||||||
|
pressed), so this is deliberate-looking and harmless — recording it so a
|
||||||
|
future reader does not "discover" it as a bug.
|
||||||
|
- **N2 — `SaveCurrentValue` re-reads the widget in retail.**
|
||||||
|
`0x004868E5`: `m_current = vtable->GetValue(); m_saved = m_current;`.
|
||||||
|
`BoolOptionRow.SaveCurrentValue` commits its own `_current`. Equivalent only
|
||||||
|
while *every* mutation goes through `SetCurrentValue`. OP4 must not let an LED
|
||||||
|
widget mutate its own visual state on a path that bypasses the row.
|
||||||
|
- **N3 — `Apply(0)` vs `Apply(1)` is not modelled.** Retail's Reset/Defaults
|
||||||
|
pass `0`, which skips `UIOption::HandleDialogAndNotices`'s confirm dialog and
|
||||||
|
the per-option `OnOptionChanged` notify (`0x004EFB90`). The single `_apply`
|
||||||
|
callback carries no arg. This first bites at OP6 —
|
||||||
|
`gmConfigUI::InitOptions @0x0049E5BF` is the only `SetConfirmChange` on the
|
||||||
|
four tabs (`Display_Resolution`).
|
||||||
|
- **N4 — `%f` fidelity.** `F6` on `float` matches retail's `%f` on the
|
||||||
|
double-promoted float for all six values in play. No action.
|
||||||
|
- **N5 — hard-coded English confirm fallback.**
|
||||||
|
`ResolveEndCharacterSessionConfirmMessage`'s `?? fallback` predates this
|
||||||
|
slice (it was `RequestEndCharacterSession`'s own), and the test script
|
||||||
|
correctly instructs the user to report if the fallback is what they see.
|
||||||
|
Plan §6's "never hard-coded English" is about DAT-resident strings; the
|
||||||
|
byte-verified `.rdata` literals in `OptionsPanelText`/`ClientTextRefusals`
|
||||||
|
are the Campaign CH-established pattern and are fine.
|
||||||
|
- **N6 — `P0x12` is dead data client-wide.** Not an OP3 defect (see §0.2), but
|
||||||
|
worth an issue eventually: several authored buttons carry an input-action id
|
||||||
|
acdream never reads, and OP8's Configure Keyboard button
|
||||||
|
(`P0x12 = 0x1000001F`, research §6) is one of them — OP8 will need either
|
||||||
|
that mechanism or an explicit binding.
|
||||||
|
- **N7 — UA/RA text is *near*-verbatim, and AD-75 covers the delta.** The
|
||||||
|
shipped body drops retail's `(Error code %d)` clause (documented) and its
|
||||||
|
trailing `\n` (not documented, immaterial in a chat line). One unverified
|
||||||
|
detail nobody has closed: the research renders the literal wrapped over three
|
||||||
|
lines, so whether "…is listed below." and "Please go there…" are separated by
|
||||||
|
a space (as shipped) or a newline in the binary is **not** established.
|
||||||
|
Cheap to settle with one push-imm32 read at VA `0x007A81E0` / `0x007A8128`
|
||||||
|
if OP9 wants it exact.
|
||||||
|
- **N8 — mount ordering.** `MountOptionsPanel()` runs before
|
||||||
|
`MountDialogFactory()` in `RetailUiRuntime.Mount`. Safe, because
|
||||||
|
`ShowConfirmation` resolves `DialogFactory` at click time, not at bind time
|
||||||
|
— noting it so a future reorder does not silently break Exit-to-Char-Select's
|
||||||
|
dialog.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. Summary table
|
||||||
|
|
||||||
|
| ID | Class | One-line |
|
||||||
|
|---|---|---|
|
||||||
|
| M1 | MUST-FIX | Six mouse-turning chat lines typed `ClientLocal 0x1A`; retail's `AddTextToScroll(…, 7, 1, 0)` types them `0x07` (light blue, timestamped, logged). No register row. |
|
||||||
|
| S1 | SHOULD-FIX | Gameplay tab gets an `OptionPage` + `AfterApply`; `gmGameplayOptionsUI` is not an `OptionPage` (`acclient.h:55857`) — extra `SaveOptions` at mount and on every Gameplay-tab entry, with two tests pinning the wrong shape. |
|
||||||
|
| S2 | SHOULD-FIX | No `OnOptionChanged` seam; retail runs it as the tail of all three verbs and it is the sole Apply/Reset enable-gate (`0x004F27D0`). |
|
||||||
|
| S3 | SHOULD-FIX | `IsGrounded`'s `IsPlayerMode` conjunction fires the mid-air refusal where retail is silent; the code comment asserts the opposite. |
|
||||||
|
| S4 | SHOULD-FIX | File the docked-host (`0x21000017`) register row the plan §5 delegated to this review — scoped client-wide, none exists today. |
|
||||||
|
| S5 | SHOULD-FIX | TS-74 should name the five store-only camera-turning preferences in What/Where, not only in Risk. |
|
||||||
|
| S6 | SHOULD-FIX | Test script: toolbar button no-longer-ghosted/highlight, `UseMouseTurning` bit surviving relogin, prefs surviving relaunch. |
|
||||||
|
| N1–N8 | NOTE | Ordering, `GetValue` re-read, `Apply(0)` arg, `%f`, fallback string, `P0x12`, UA/RA whitespace, mount order. |
|
||||||
Loading…
Add table
Add a link
Reference in a new issue