acdream/docs/research/2026-08-11-op3-review-mechanism.md
Erik efe80d5a0d 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>
2026-08-11 02:32:57 +02:00

27 KiB
Raw Permalink Blame History

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 nowheregrep -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.949999988fSetCurrentValue(0.95f) @0x0049E93C :60-65 vs MouseTurningTarget.Stiffness = 0.95f
2 != 50f50f @0x0049E9AC :67-72
3 != 0.699999988f0.7f @0x0049EA1C :74-79
4 m_pSlope->GetValue() == 1SetCurrentValue(0) @0x0049EA69-77 :81-86 (target false)
5 m_pInvert->GetValue() == 0SetCurrentValue(1) @0x0049EABB-C9 :88-93
6 m_pMouseTurn->GetValue() == 0SetCurrentValue(1) @0x0049EB0D-1B :95-99
tail this->vtable->SaveCurrentValues() @0x0049EB57 RetailUiRuntime.cs:1908 ConfigPage.Apply()AfterApplySaveCharacterOptionsRuntimeCmd

The tail is faithful because gmConfigUI : PlayerOptionPage, gmNoticeHandler (acclient.h:55846), so its SaveCurrentValues vtable slot IS PlayerOptionPage::SaveCurrentValuesSaveToServer(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: 0x004887B3AddTextToScroll(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 ClientLocalbright 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::OnVisibilityChangedno 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 shapeOptionPageModelTests.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 verbs0x004F2C95 (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:

// "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" (SetPanelOpenHighlight/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 OP4OP6)

  • 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.
N1N8 NOTE Ordering, GetValue re-read, Apply(0) arg, %f, fallback string, P0x12, UA/RA whitespace, mount order.