docs: OP4 mechanism review — APPROVE-WITH-FIXES (converges with blast on the seed bug)
MF-1 converges with blast M1: rows seed once at startup pre-login and never re-read; retail's own schedule is the fix (UIOption_Checkbox:: SaveCurrentValue @0x004868E0 = m_current = GetValue() live, m_saved = m_current, on every OnShown — predicted by OP3 review note N2). MF-2: the OnOptionChanged Apply/Reset ghosting seam (Ghosted 0x0D / Normal, PostInit runs it once so both start disabled) is wired by NOTHING in production. The Defaults correction is independently VERIFIED from the decomp (SetPlayerOption @0x00486e80 sets m_default from GetDefaultOptionValue; InqDefaultGameplayOptionProperty's only caller is the Chat tab's slider path — the plan's U1 directive would have made Defaults restore nothing). Cross-page button collision confirmed from the fixture (0x100001FC/FD/FE x3). 18 rows traced end-to-end; six header hashes + 49 label globals hand-verified. SHOULD: culture-dependent timestamp separator + overclaimed byte-verification; no OnClick/ToggleBehavior-driven LED test; gate item 8's logout-flush false failure. NOTEs incl. sky-dome fog and no mid-session echo repaint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
0acb341c29
commit
85afaae5fd
1 changed files with 545 additions and 0 deletions
545
docs/research/2026-08-11-op4-review-mechanism.md
Normal file
545
docs/research/2026-08-11-op4-review-mechanism.md
Normal file
|
|
@ -0,0 +1,545 @@
|
||||||
|
# Campaign OP slice OP4 — MECHANISM-FAITHFULNESS review
|
||||||
|
|
||||||
|
**Commit under review:** `22b86b9f` "Campaign OP slice OP4 — the Character tab"
|
||||||
|
**Lens:** mechanism faithfulness (retail behaviour vs. the port). The
|
||||||
|
regression/blast-radius lens is a separate document.
|
||||||
|
**Contract:** `docs/plans/2026-08-10-options-panel-campaign.md` §4 OP4 + §2 D3/D7.
|
||||||
|
**Oracles:** `docs/research/2026-08-10-character-options-map.md` §2/§7.1;
|
||||||
|
`docs/research/2026-08-10-options-panel-structure.md` §3/§6/§7/§10.4;
|
||||||
|
`docs/research/2026-08-10-set-character-options-wire.md`;
|
||||||
|
`docs/research/named-retail/acclient.h`;
|
||||||
|
`docs/research/named-retail/acclient_2013_pseudo_c.txt`.
|
||||||
|
**Mode:** read-only. No build, no test run, no client launch. The only file
|
||||||
|
written is this one.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Verdict
|
||||||
|
|
||||||
|
**APPROVE-WITH-FIXES** — with one hard sequencing condition:
|
||||||
|
|
||||||
|
> **The connected gate must NOT be scheduled until MUST-FIX 1 lands.** The
|
||||||
|
> Character tab's 50 LEDs are seeded once, at *window startup*, before any
|
||||||
|
> network session exists, from `RuntimeCharacterOptionsState`'s
|
||||||
|
> constructor default word — so the panel cannot show the logged-in
|
||||||
|
> character's real options. Test-script items 3, 6, 7 and 8 all assert the
|
||||||
|
> opposite and will fail on the first run.
|
||||||
|
|
||||||
|
The *substance* of the slice is right and is the best-evidenced work in the
|
||||||
|
campaign so far. The 50-row binding is exact, the DEFAULTS CORRECTION is
|
||||||
|
independently verified and the plan's own §1 note was genuinely wrong, the
|
||||||
|
cross-page button-id collision is real and the scoped fix is correct, the
|
||||||
|
Group-B/C consumers are correctly anchored, and the register bookkeeping is
|
||||||
|
honest — including two corrections (TS-75, the U1 defaults source) where the
|
||||||
|
implementer refused to wire a plausible-but-wrong seam. Both MUST-FIX items
|
||||||
|
are contained, retail-anchored, and touch three files.
|
||||||
|
|
||||||
|
| Class | Count | Items |
|
||||||
|
|---|---|---|
|
||||||
|
| MUST-FIX | 2 | MF-1 stale row seed (never reads live option word); MF-2 Apply/Reset enable-gating not wired |
|
||||||
|
| SHOULD-FIX | 3 | SF-1 timestamp culture + over-claimed "byte-verified"; SF-2 test gaps (widget click path, header hashes); SF-3 test-script false-failure in item 8 + the enable-gate items |
|
||||||
|
| NOTE | 5 | N-1 `FindCheckbox` ignores the retail id; N-2 `FindDescendant` semantics changed on promotion; N-3 sky dome still fogs; N-4 AP-194 anchor mislabel; N-5 no refresh on server-side option echo |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. What I verified as CORRECT (evidence first)
|
||||||
|
|
||||||
|
These are recorded because several are load-bearing for later slices and
|
||||||
|
because a future reader must not "rediscover" them as bugs.
|
||||||
|
|
||||||
|
### 1.1 The 50-row binding — 18 rows traced end-to-end
|
||||||
|
|
||||||
|
`src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs:101-186`
|
||||||
|
(`Groups`) reproduces `docs/research/2026-08-10-options-panel-structure.md`
|
||||||
|
§7's authored sequence **exactly**: group sizes 3 / 15 / 6 / 11 / 7 / 7+1,
|
||||||
|
row-for-row in authored order, six header keys in authored order. Every
|
||||||
|
`RetailName` matches `acclient.h:4162-4218`'s literal enumerator spelling
|
||||||
|
(I diffed all 49 against the header).
|
||||||
|
|
||||||
|
I traced **18** rows (the brief asked for 15) row → `PlayerOption` id →
|
||||||
|
word/mask → auto-save flag → client default → wire route, against
|
||||||
|
`CharacterOptionTable` (`src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs`)
|
||||||
|
and, independently, against the two byte-verified jump tables transcribed in
|
||||||
|
character-options-map.md §1.3 (`IsAutoSaveOption`, VA `0x0059A62C`) and §1.4
|
||||||
|
(`GetDefaultOptionValue`, VA `0x005D2A5C`). Note both tables are *inverted*
|
||||||
|
(`0x00` → `jump_table[0]` → `return 1`); the table's columns decode
|
||||||
|
correctly under that inversion in all 18 cases.
|
||||||
|
|
||||||
|
| Row | id | word · mask | auto-save | ClientDefault | wire route |
|
||||||
|
|---|---|---|---|---|---|
|
||||||
|
| Keep Combat Targets in View | 0x07 | O1 `0x80` | no (idx 7 = 01) | false | dirty → `0x01A1` |
|
||||||
|
| Salvage Multiple | 0x22 | O2 `0x80` | no | false | dirty |
|
||||||
|
| Use Main Pack as Default | 0x29 | O2 `0x4000` | no | false | dirty |
|
||||||
|
| Vivid Targeting Indicator | 0x0E | O1 `0x8000` | no | **true** (idx 0x0E = 00) | dirty |
|
||||||
|
| Display 3D Tooltips | 0x08 | O1 `0x100` | no | true | dirty |
|
||||||
|
| Show Coordinates By the Radar | 0x14 | O1 `0x400000` | no | true | dirty |
|
||||||
|
| Disable Distance Fog | 0x30 | O2 `0x200000` | no (idx 0x30 = 01) | false (past table end) | dirty |
|
||||||
|
| Show Your Helm | 0x2F | O2 `0x100000` | **yes** (idx 0x2F = 00) | false (past end) | `0x0005` |
|
||||||
|
| Show Your Cloak | 0x32 | O2 `0x800000` | **yes** | false (past end) | `0x0005` |
|
||||||
|
| Ignore Allegiance Requests | 0x01 | O1 `0x04` | yes | false | `0x0005` |
|
||||||
|
| Ignore Fellowship Requests | 0x02 | O1 `0x08` | yes | **true** | `0x0005` |
|
||||||
|
| Share Fellowship XP | 0x0F | O1 `0x40000` | yes | true | `0x0005` |
|
||||||
|
| Accept Corpse Looting Permissions | 0x10 | O1 `0x80000` | yes | false | `0x0005` |
|
||||||
|
| Automatically Repeat Attacks | 0x00 | O1 `0x02` | yes | true | `0x0005` |
|
||||||
|
| Listen to Society Chat | 0x2E | O2 `0x80000` | yes | false (past end) | `0x0005` |
|
||||||
|
| Display Timestamps | 0x21 | O2 `0x40` | no | false | dirty |
|
||||||
|
| Run as Default Movement | 0x0A | O1 `0x400` | no | true | dirty |
|
||||||
|
| Listen to PK death messages | 0x34 | O2 `0x02000000` | no (ACE-sourced) | false | dirty — D3 wire+store only, AP-193 |
|
||||||
|
|
||||||
|
The click path is `checkbox.OnClick` (`:381`) → `BoolOptionRow.SetCurrentValue`
|
||||||
|
→ the row's `apply` closure (`:371-380`) → `Bindings.SetOption` →
|
||||||
|
`SetSingleCharacterOptionRuntimeCmd` (`RetailUiRuntime.cs:2029-2031`) →
|
||||||
|
`LiveSessionCommandRouter.cs:173-177` → `LiveSessionRuntimeFactory.cs:347-355`
|
||||||
|
`SendSingleCharacterOption` → `RuntimeCharacterOptionsState.TrySetOption`
|
||||||
|
(`RuntimeCharacterState.cs:806-842`), which writes the local bit first and then
|
||||||
|
*either* `sendAutoSave` (`0x0005`) *or* `MarkDirty()`. That is retail's
|
||||||
|
`CPlayerModule::OnChanged @0x0059A8E0` step order exactly. The batched half
|
||||||
|
flushes through `OptionPage.AfterApply` → `SaveCharacterOptionsRuntimeCmd`
|
||||||
|
(`RetailUiRuntime.cs:1987-1988`) → `TryFlush` → `SendSetCharacterOptions`. ✅
|
||||||
|
|
||||||
|
D3's 50th row is wire+store only: no consumer, `HearPkDeathMessages` appears
|
||||||
|
in no Group-B/C binding, and AP-193 carries the ACE-sourced mapping. ✅
|
||||||
|
|
||||||
|
The 50th row's label key is right: I computed
|
||||||
|
`compute_str_hash("ID_PlayerOption_HearPKDeaths")` with the exact algorithm at
|
||||||
|
`DatStringResolver.cs:68-82` and got **`0x0D16E9A3`**, matching structure doc
|
||||||
|
§7's byte-verified string id. ✅
|
||||||
|
|
||||||
|
### 1.2 The DEFAULTS CORRECTION is correct — the plan's §1 "U1 closed" note was wrong
|
||||||
|
|
||||||
|
This is the highest-stakes claim in the commit and it holds. Read directly
|
||||||
|
from the pseudo-C rather than from the implementer's summary:
|
||||||
|
|
||||||
|
`acclient_2013_pseudo_c.txt:147331` —
|
||||||
|
`UIOption_Checkbox::SetPlayerOption @0x00486e80`, guarded at `00486ea3` by
|
||||||
|
`if (this->m_propName == 0 && …)`, contains at `00486f2d`:
|
||||||
|
|
||||||
|
```
|
||||||
|
this->m_default = PlayerModule::GetDefaultOptionValue((__return_1 + 4), this->m_playerOption);
|
||||||
|
```
|
||||||
|
|
||||||
|
and `PlayerOptionPage::AddToggleOption(enum PlayerOption) @0x004F2A00`
|
||||||
|
(`:245616`) calls `SetPlayerOption` then `OptionPage::RegisterOption` for
|
||||||
|
*every* row on this tab. So the Character tab's `m_default` **is**
|
||||||
|
`GetDefaultOptionValue` — i.e. `CharacterOptionTable.ClientDefault`, the table
|
||||||
|
OP1 already ported. Structure doc §7's "biggest open unknown" is genuinely
|
||||||
|
closed by this.
|
||||||
|
|
||||||
|
The plan's §1 note ("U1 closed: `UIOption::InqDefaultGameplayOptionProperty
|
||||||
|
@0x004ef8d0` … the Defaults button restores DAT-authored values") is
|
||||||
|
**mis-scoped, and the implementer's rejection of it is right**. The pseudo-C
|
||||||
|
has exactly one caller of `InqDefaultGameplayOptionProperty` — line `:145204`,
|
||||||
|
inside `UIOption_Slider::SetGameplayOptionProperty @0x00485030`, the
|
||||||
|
`m_propName`-bound path. The only three `SetGameplayOptionProperty` call sites
|
||||||
|
in the whole binary are `gmChatOptionsUI::InitOptions`' two sliders
|
||||||
|
(`0x10000080`, `0x10000081` at `:170766`, `:170797`) and its filter bitfields
|
||||||
|
(`0x1000007F` at `:170826`, `:170882`). So the claim that DAT collection
|
||||||
|
`0x78000001` holds only those three Chat-tab entries is *consistent with the
|
||||||
|
decomp by construction*, and — more importantly — it is **not load-bearing**:
|
||||||
|
every Character-tab row takes the `m_propName == 0` PlayerOption branch, so
|
||||||
|
the DBProperty mechanism cannot reach this tab regardless of what the
|
||||||
|
collection contains. Had the implementer followed the plan's note, the
|
||||||
|
Defaults button would have restored nothing at all (no PlayerOption-range keys
|
||||||
|
exist in that collection).
|
||||||
|
|
||||||
|
AP-194's amendment (`docs/architecture/retail-divergence-register.md:186`)
|
||||||
|
states this truthfully: it confirms the directive (use the `ClientDefault`
|
||||||
|
column) was followed, cites the new independent trace, and names the
|
||||||
|
Chat/Config `m_propName` mechanism as the thing that does *not* apply here. ✅
|
||||||
|
|
||||||
|
### 1.3 The cross-page button-id collision is real — verified from the fixture
|
||||||
|
|
||||||
|
Counted occurrences of each element id in the committed host fixture
|
||||||
|
`tests/AcDream.App.Tests/UI/Layout/fixtures/options_panel_2100006E_1000018D.json`:
|
||||||
|
|
||||||
|
| Element | Occurrences |
|
||||||
|
|---|---|
|
||||||
|
| `0x100001FA` Character ListBox | **1** |
|
||||||
|
| `0x100001FB` Character scrollbar | **1** |
|
||||||
|
| `0x100001FC` Apply | **3** |
|
||||||
|
| `0x100001FD` Reset | **3** |
|
||||||
|
| `0x100001FE` Defaults | **3** |
|
||||||
|
| `0x10000200` Config ListBox | 1 |
|
||||||
|
|
||||||
|
Three physical instances each of Apply/Reset/Defaults (Character, Chat,
|
||||||
|
Config — matching structure doc §3.1's "identical geometry AND identical
|
||||||
|
element ids", all base `0x21000040/0x100002CD`). A flat
|
||||||
|
`ImportedLayout.FindElement` would have wired all three pages' buttons to
|
||||||
|
whichever instance the flat index happened to hold. The per-page scoped
|
||||||
|
`UiElement.FindDescendant(pageRoot, id)` in
|
||||||
|
`OptionsPanelController.cs:214-236` is the correct fix and matches retail's
|
||||||
|
own `GetChildRecursive(<the gm*UI element>, 0x100001FC)` scoping in
|
||||||
|
`PlayerOptionPage::OnOptionChanged @0x004F27D0`. The Character ListBox and
|
||||||
|
scrollbar ids are unique, so the flat lookups at
|
||||||
|
`CharacterOptionsPageController.cs:246,256` are safe. ✅
|
||||||
|
|
||||||
|
### 1.4 All six group-header keys hash to §7's byte-verified string ids
|
||||||
|
|
||||||
|
I ran `DatStringResolver.ComputeHash`'s algorithm by hand over the six
|
||||||
|
`HeaderKey` literals:
|
||||||
|
|
||||||
|
| Key | Computed | structure doc §7 |
|
||||||
|
|---|---|---|
|
||||||
|
| `ID_CharacterOption_UIBehavior_Section` | `0x06489B6E` | `0x06489B6E` ✅ |
|
||||||
|
| `ID_CharacterOption_UIDisplay_Section` | `0x0A9BC99E` | `0x0A9BC99E` ✅ |
|
||||||
|
| `ID_CharacterOption_Grouping_Section` | `0x0CBAAFAE` | `0x0CBAAFAE` ✅ |
|
||||||
|
| `ID_CharacterOption_OtherPlayers_Section` | `0x0872DFFE` | `0x0872DFFE` ✅ |
|
||||||
|
| `ID_CharacterOption_CharacterBehavior_Section` | `0x08674D5E` | `0x08674D5E` ✅ |
|
||||||
|
| `ID_CharacterOption_Chat_Section` | `0x0987FE8E` | `0x0987FE8E` ✅ |
|
||||||
|
|
||||||
|
And all 49 `ID_PlayerOption_<Name>` / `_Help` label keys the controller
|
||||||
|
constructs at `:343,352` exist verbatim as globals in the retail binary
|
||||||
|
(grepped out of `acclient_2013_pseudo_c.txt`; the truncated 32-char variants
|
||||||
|
alongside them are the known BN preview artifact). **No invented user-visible
|
||||||
|
English anywhere** — every label, tooltip and header resolves by name against
|
||||||
|
table `0x23000003`, and an unresolved string degrades to *no text*
|
||||||
|
(`:328-333`, `:346-351`) rather than a fabricated caption. ✅
|
||||||
|
|
||||||
|
### 1.5 Separator cadence, template indices, and the LED click contract
|
||||||
|
|
||||||
|
`Bind` emits header → rows → separator per group (`:264-277`), i.e. 6 headers
|
||||||
|
and 6 separators = structure doc §7's "5 interior separators + 1 trailing"
|
||||||
|
(§7's PK-death note independently confirms the trailing `AddSeperator` after
|
||||||
|
`HearSocietyChat`). Template indices 0/1/2 match §1.5's authored arrays, and
|
||||||
|
index 2 is literally the constant in `AddToggleOption`'s own
|
||||||
|
`AddItemFromTemplateList(m_pOptionBox, 2, nullptr)` at `:245616`. ✅
|
||||||
|
|
||||||
|
The LED click contract works because the toggle template's checkbox
|
||||||
|
`0x10000219` authors DAT property `0x0B` = true (fixture
|
||||||
|
`options_2100002B.json:406,428-431` → `UiButton.ToggleBehavior`), so
|
||||||
|
`UiButton` self-flips `_selected` at MouseUp (`UiButton.cs:486`) before the
|
||||||
|
`Click` event invokes `OnClick`, and `checkbox.OnClick = () =>
|
||||||
|
row_.SetCurrentValue(checkbox.Selected)` (`:381`) therefore reads the *new*
|
||||||
|
value. ✅ (See SF-2 — nothing pins this.)
|
||||||
|
|
||||||
|
### 1.6 Consumers
|
||||||
|
|
||||||
|
- **Display Timestamps.** Prefixed at
|
||||||
|
`RuntimeCommunicationState.AddText` (`:224-225`) — retail's
|
||||||
|
`ClientSystem::AddTextToScroll @0x00563C50` chokepoint — *after* the
|
||||||
|
both-ends trim and *after* the `ClientLocal (0x1A)` early return, which is
|
||||||
|
exactly retail's order (the CH2 doc comment on the same method already
|
||||||
|
records that retail "skips the timestamp prefix … for this type"). Format
|
||||||
|
`"H:mm:ss "` is the correct .NET spelling of `%#H:%M:%S ` (non-padded 24h
|
||||||
|
hour, padded minute/second, trailing space). Prefix applies only when the
|
||||||
|
bit is on and only to *new* lines. ✅ (See SF-1 for two caveats.)
|
||||||
|
- **Disable Distance Fog.** `WeatherSystem.Snapshot` forces
|
||||||
|
`FogMode.Off` (`WeatherState.cs:326-327`), `FogMode.Off == 0`
|
||||||
|
(`SkyState.cs:15`), the UBO carries it as `FogParams.w`
|
||||||
|
(`SceneLightingUbo.cs:130`), and both world shaders gate on it —
|
||||||
|
`mesh_modern.frag:48-49` and `terrain_modern.frag:160-161` both
|
||||||
|
`if (mode == 0) return lit;`. The shader-gate claim is real. ✅ (See N-3.)
|
||||||
|
- **Run as Default Movement.** `DispatcherMovementInputSource.cs:84`
|
||||||
|
computes `Run: (_movement.RunAsDefaultMovement != walking) || AutoRunActive`.
|
||||||
|
This is byte-correct *and better-evidenced than the commit claims*: retail's
|
||||||
|
`CommandInterpreter::SetHoldRun @0x006B3370` (`:699110-699118`) computes
|
||||||
|
`ecx = (hold_run == 0); edx_3 = (UITogglesRun() == 0); eax_3 = (ecx != edx_3)`
|
||||||
|
— an XOR of the modifier against the option, which is precisely the port's
|
||||||
|
`!=`. `ACCmdInterp::UITogglesRun @0x0058B180` (`:405377`) is
|
||||||
|
`PlayerModule::ToggleRun`, i.e. option `0x0A`, which is the id GameWindow
|
||||||
|
binds (`GameWindow.cs:676-678`). With the option ON the expression collapses
|
||||||
|
to the pre-OP4 `!walking`, so the default path is bit-identical. ✅
|
||||||
|
- **Group C re-point.** `CharacterOptionCombatSettingsSource`
|
||||||
|
(`LiveCombatAttackOperations.cs:31-63`) reads `GetOptionBit` live for
|
||||||
|
AutoTarget/AutoRepeatAttack/ViewCombatTarget and is substituted at both
|
||||||
|
construction sites (`SessionPlayerComposition.cs:770`,
|
||||||
|
`FrameRootComposition.cs:599-603`). `VividTargetingIndicator`,
|
||||||
|
`CoordinatesOnRadar`, `LockUI` and `AcceptLootPermits` likewise
|
||||||
|
(`InteractionRetainedUiComposition.cs:286-288,485-487,687-694`,
|
||||||
|
`LivePresentationComposition.cs:840-847`). Toggling writes through
|
||||||
|
`TrySetOption` (local bit first, then wire) per CH3 — verified for the two
|
||||||
|
non-panel entrances too: `ToggleUiLock` and `SetAcceptLootPermits` now both
|
||||||
|
call `SendSingleCharacterOption` before the legacy settings mirror
|
||||||
|
(`LiveSessionRuntimeFactory.cs:394-401,476-481`), which closes the two
|
||||||
|
divergences character-options-map.md §0 named. ✅
|
||||||
|
|
||||||
|
### 1.7 Register bookkeeping
|
||||||
|
|
||||||
|
TS-73's narrowing is **truthful and appropriately scoped** — it explicitly
|
||||||
|
says neither closed case lives inside `TrySetOption`'s own switch, so the
|
||||||
|
row's literal title claim stays accurate while the user-observable symptom is
|
||||||
|
recorded as fixed for `0x07`/`0x30` only.
|
||||||
|
|
||||||
|
TS-75's `ForcedDayGroupIndex` correction is **verified**:
|
||||||
|
`RuntimeWorldDayGroupDefinition(Name, ChanceOfOccur, SkyObjectCount, Sky)`
|
||||||
|
(`src/AcDream.Runtime/World/RuntimeWorldEnvironmentState.cs:5-14`) is a
|
||||||
|
probability-weighted *weather-variety* group from Region `0x13000000`; the
|
||||||
|
field's only consumers are `RuntimeOptions.cs:55,122` /
|
||||||
|
`GameWindow.cs:639` env-var plumbing. Wiring `PersistentAtDay` to it would
|
||||||
|
have silently changed the character's weather odds while leaving night
|
||||||
|
falling — the row is right to call that "an incorrect fix masquerading as a
|
||||||
|
correct one". TS-76..TS-80 are each honest no-consumer rows with the correct
|
||||||
|
retail anchors; TS-80 correctly identifies `FellowshipShareXP` as the one
|
||||||
|
client-sourced option whose *packet field*, not the stored bit, is what ACE
|
||||||
|
reads. Counts are consistent. ✅
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. MUST-FIX
|
||||||
|
|
||||||
|
### MF-1 — The 50 rows are seeded once at *window startup*, before login, and never re-read the live option word
|
||||||
|
|
||||||
|
**The panel cannot display the logged-in character's options.**
|
||||||
|
|
||||||
|
Chain of evidence:
|
||||||
|
|
||||||
|
1. `CharacterOptionsPageController.Bind` reads each row's initial value once,
|
||||||
|
at build time: `bool initial = bindings.CurrentValue(spec.Id);` →
|
||||||
|
`checkbox.Selected = initial;` → `new BoolOptionRow(initial, …)`
|
||||||
|
(`CharacterOptionsPageController.cs:368-380`). Nothing re-reads it after
|
||||||
|
that.
|
||||||
|
2. `Bind` is called from `RetailUiRuntime.MountOptionsPanel`
|
||||||
|
(`RetailUiRuntime.cs:2010`), which runs inside `Initialize()`
|
||||||
|
(`:371`) — i.e. once, when the retained UI is composed.
|
||||||
|
3. The retained UI is composed by the **startup** graph:
|
||||||
|
`GameWindow.OnLoad()` (`GameWindow.cs:1231`) →
|
||||||
|
`GameWindowCompositionPipeline.Run(… InteractionRetainedUiCompositionPhase …)`
|
||||||
|
(`:1326`). This runs at window load, before any `WorldSession` exists.
|
||||||
|
4. Pre-login, `RuntimeCharacterOptionsState` holds its constructor defaults —
|
||||||
|
`_options1 = DefaultOptions1 (0x50C4A54A)`, `_options2 = DefaultOptions2
|
||||||
|
(0x00948700)` (`RuntimeCharacterState.cs:645-647, 671-672`). The class even
|
||||||
|
carries a `_hasServerSeed` flag (`:676`) that the panel never consults.
|
||||||
|
5. So every LED shows ACE's *character-creation* default word — which is
|
||||||
|
neither the character's real state nor even retail's own Defaults column
|
||||||
|
(AP-194: the two disagree on `0x2D`/`0x2F`/`0x32`).
|
||||||
|
|
||||||
|
Consequences on the connected gate as scripted:
|
||||||
|
|
||||||
|
- Item 3 ("confirm the checkboxes reflect your character's actual state …
|
||||||
|
should show checked on open") — fails for every option the character has
|
||||||
|
ever changed.
|
||||||
|
- Item 6 ("relog … rows should still read whatever you left them at (server
|
||||||
|
echo)") — fails; the rows never see the echo.
|
||||||
|
- Items 7/8 (batched persist-vs-revert, judged by re-reading the row) — the
|
||||||
|
row is not a valid readout of persisted state.
|
||||||
|
|
||||||
|
There is no wire corruption (`TrySetOption` no-ops on an unchanged bit,
|
||||||
|
`RuntimeCharacterState.cs:812-814`, and `Reset` only touches `Changed` rows),
|
||||||
|
and the *consumers* are all correct because they poll `GetOptionBit` live.
|
||||||
|
The damage is confined to the panel's own display and baseline — but that is
|
||||||
|
the entire deliverable of this slice.
|
||||||
|
|
||||||
|
**Retail's own mechanism already fixes this, and the OP3 mechanism review
|
||||||
|
predicted it.** `docs/research/2026-08-11-op3-review-mechanism.md:424-427`
|
||||||
|
(note N2) reads: *"`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 is the slice that
|
||||||
|
violates that precondition — the option word has many other writers (the
|
||||||
|
PlayerDescription seed, `/consent`, `@join`, `ToggleUiLock`, headless
|
||||||
|
commands, the fellowship mutual-exclusion recursion inside `TrySetOption`
|
||||||
|
itself).
|
||||||
|
|
||||||
|
And `UIOption_Checkbox::GetValue @0x00486f60` is not a widget-local read:
|
||||||
|
character-options-map.md §1.2 establishes it branches
|
||||||
|
`m_playerOption != Invalid → PlayerModule::GetOption`, i.e. **the live
|
||||||
|
server-synced option word**. Since `OptionPage.OnShown() => Apply()`
|
||||||
|
(`OptionPageModel.cs:256`) runs on the initial default-tab activation, on
|
||||||
|
every tab switch in, and on every window (re)open, porting `SaveCurrentValue`
|
||||||
|
faithfully makes the panel self-correct on exactly retail's schedule.
|
||||||
|
|
||||||
|
**Fix.** Give `BoolOptionRow` the `GetValue` reader it is missing and use it
|
||||||
|
in `SaveCurrentValue`:
|
||||||
|
|
||||||
|
- add a `Func<bool>? read` ctor parameter (OP4 already has the exact reader:
|
||||||
|
`bindings.CurrentValue(spec.Id)`);
|
||||||
|
- `SaveCurrentValue()` becomes `_current = _read?.Invoke() ?? _current;
|
||||||
|
_saved = _current;` — retail `@0x004868E0` verbatim;
|
||||||
|
- push the re-read value onto the widget (retail's `Refresh()`), i.e. the
|
||||||
|
row's existing `apply`-side `checkbox.Selected = value` needs an
|
||||||
|
equivalent on the save path, or `BoolOptionRow` gains a `refresh` callback.
|
||||||
|
Do **not** route the re-read through `apply` — that would send the value
|
||||||
|
back on the wire.
|
||||||
|
|
||||||
|
Pin it with a test that changes the binding's underlying value *behind* the
|
||||||
|
row and asserts the row and the checkbox both converge on the next
|
||||||
|
`OnShown()`. Add a second test for the pre-login case (bind with the
|
||||||
|
constructor default word, seed the state, show the page, assert all 50 rows
|
||||||
|
match).
|
||||||
|
|
||||||
|
### MF-2 — Apply/Reset enable-gating is never wired in production
|
||||||
|
|
||||||
|
Structure doc §3.4 (`PlayerOptionPage::OnOptionChanged @0x004F27D0`):
|
||||||
|
|
||||||
|
```
|
||||||
|
eax = GetChildRecursive(<the gm*UI element>, 0x100001FC); // Apply
|
||||||
|
eax_1 = GetChildRecursive(<the gm*UI element>, 0x100001FD); // Reset
|
||||||
|
if (OptionPage::Changed(...) == 0) { SetState(0x0D) on both } // disabled
|
||||||
|
else { SetState(0x01) on both } // enabled
|
||||||
|
```
|
||||||
|
|
||||||
|
plus "`PostInit` calls `InitOptions()` then `OnOptionChanged(0)` so the
|
||||||
|
buttons start disabled", and "**Defaults (`0x100001FE`) is never
|
||||||
|
enable-gated**". §10.4's *"Behaviours that MUST be ported, not improved"*
|
||||||
|
lists this as item 3.
|
||||||
|
|
||||||
|
`OptionPage.OnOptionChanged` exists (`OptionPageModel.cs:204`), fires as the
|
||||||
|
last statement of `Apply`/`Reset`/`Defaults` (`:227,240,250`) and on every
|
||||||
|
live edit via `AttachPageNotify` (`:211`) — but **nothing in `src/` ever
|
||||||
|
assigns it**. Grep across `src/` and `tests/`: the only assignments are in
|
||||||
|
`OptionPageModelTests.cs`. OP4 is the slice that wired the three buttons
|
||||||
|
(`OptionsPanelController.cs:214-236`) and owns the first page that has them;
|
||||||
|
`OptionPageModel.cs:201-202` says in so many words "OP4-6 bind buttons to this
|
||||||
|
seam".
|
||||||
|
|
||||||
|
Everything needed is already present: `UiButtonStateMachine.Ghosted == 13`
|
||||||
|
(= retail `0x0D`) and `Normal == 1` (= `0x01`)
|
||||||
|
(`UiButtonStateMachine.cs:17,23`), and `UiButton.TrySetRetailState`
|
||||||
|
(`UiButton.cs:209-223`) already implements `Ghosted → Enabled = false` and the
|
||||||
|
inverse. `UiButton.OnEvent` already refuses clicks when `!Enabled`
|
||||||
|
(`:494`).
|
||||||
|
|
||||||
|
**Fix.** In `OptionsPanelController.Bind`'s per-page loop, capture the
|
||||||
|
resolved Apply and Reset `UiButton`s and set
|
||||||
|
`page.OnOptionChanged = () => { apply.TrySetRetailState(page.Changed ?
|
||||||
|
UiButtonStateMachine.Normal : UiButtonStateMachine.Ghosted); reset.Try…; }`,
|
||||||
|
then invoke it once at bind time so the pair starts disabled (retail's
|
||||||
|
`PostInit` tail). Leave Defaults untouched. Pin: (a) both start disabled on a
|
||||||
|
freshly-bound page, (b) one LED click enables both, (c) Apply disables both,
|
||||||
|
(d) Defaults leaves them enabled when it actually changed something and
|
||||||
|
(e) the Defaults button is never disabled.
|
||||||
|
|
||||||
|
Without this the connected gate silently accepts a missing MUST-PORT
|
||||||
|
behaviour — and the script's own "what to report" bullet ("Any Reset/Defaults
|
||||||
|
button that stays permanently disabled") reads as though the author believed
|
||||||
|
the gating shipped.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. SHOULD-FIX
|
||||||
|
|
||||||
|
### SF-1 — Timestamp: culture-dependent separator, an over-claimed "byte-verified", and the ignored per-character format string
|
||||||
|
|
||||||
|
Three separate problems in one two-line consumer
|
||||||
|
(`RuntimeCommunicationState.cs:224-225`).
|
||||||
|
|
||||||
|
1. **Culture.** `DateTime.Now.ToString("H:mm:ss ")` uses the current culture,
|
||||||
|
and `:` in a .NET custom date/time format string is the *time-separator
|
||||||
|
placeholder*, replaced by `DateTimeFormatInfo.TimeSeparator`. On a culture
|
||||||
|
that uses `.` (fi-FI and friends) both production output and the test's
|
||||||
|
`Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} …")`
|
||||||
|
(`tests/AcDream.Runtime.Tests/Gameplay/RuntimeCommunicationStateTests.cs`,
|
||||||
|
the OP4 block) change. Retail's CRT `strftime` emits a literal `:`. Use
|
||||||
|
`CultureInfo.InvariantCulture` (or escape as `H\:mm\:ss `) and make the
|
||||||
|
test culture-explicit.
|
||||||
|
2. **The "byte-verified" claim is wrong.** The doc comment at
|
||||||
|
`RuntimeCommunicationState.cs:100-104` calls `"%#H:%M:%S "` "the
|
||||||
|
byte-verified CRT strftime string". The wire oracle says the opposite:
|
||||||
|
`docs/research/2026-08-10-set-character-options-wire.md:941` open-unknown
|
||||||
|
**U6** — *"The default timestamp format string `"%#H:%M:%S "` is
|
||||||
|
BN-sourced, not byte-verified"* (and `:314-316` repeats it). The campaign's
|
||||||
|
§8 process rule is that decomp claims are byte-verified against the
|
||||||
|
PDB-paired binary; this one asserts verification the source explicitly
|
||||||
|
withholds. Either verify it (the operand decode recipe is in structure
|
||||||
|
doc's appendix) or downgrade the comment to "BN-sourced (wire doc U6)".
|
||||||
|
Note U6's own "only matters when the Chat tab implements timestamp
|
||||||
|
formatting" caveat is now stale — OP4 shipped the consumer.
|
||||||
|
3. **The per-character format is ignored.** Retail reads the format from
|
||||||
|
`GenericQualitiesData::InqString(m_pPlayerOptionsData, 1, &m_TimeStampFormat)`
|
||||||
|
(wire doc `:311-313`), and acdream's parser reads-and-discards it
|
||||||
|
(`:647`, "timestamp string (`0x80`) | read, discarded | ❌ | never sent").
|
||||||
|
Hardcoding the constructor default is defensible, but it is a divergence
|
||||||
|
with no register row. File one (or fold it into TS-77's neighbourhood).
|
||||||
|
|
||||||
|
### SF-2 — Two conformance gaps in an otherwise strong test suite
|
||||||
|
|
||||||
|
The 52 tests are good — both-directions row conformance, the independently
|
||||||
|
transcribed authored order, the `ClientDefault` pin, the tab-hide revert
|
||||||
|
against real fixtures. Two things a mechanism reviewer would want pinned:
|
||||||
|
|
||||||
|
- **The widget click path is never exercised.**
|
||||||
|
`ClickingARow_PublishesSetOption_WithTheAuthoredId`
|
||||||
|
(`CharacterOptionsPageControllerTests.cs`, "Behavioral" section) calls
|
||||||
|
`boolRow.SetCurrentValue(true)` directly. Nothing drives
|
||||||
|
`checkbox.OnClick`, so nothing pins the `ToggleBehavior` dependency the
|
||||||
|
whole LED interaction rests on (§1.5 above). If DAT property `0x0B` on
|
||||||
|
`0x10000219` ever fails to import, all 50 rows go inert and the suite stays
|
||||||
|
green. Add one test that dispatches MouseDown/MouseUp/Click at the built
|
||||||
|
checkbox and asserts both `Selected` and the published `SetOption`.
|
||||||
|
- **Only one of the seven string keys is hash-pinned.**
|
||||||
|
`HearPkDeathMessages_RetailNameHash_MatchesByteVerifiedStringId` pins
|
||||||
|
`0x0D16E9A3`. The six header keys are not pinned, even though structure doc
|
||||||
|
§7 gives all six byte-verified string ids. I verified all six by hand (§1.4)
|
||||||
|
— they are correct today — but a typo in a `HeaderKey` literal would
|
||||||
|
produce a silently blank header that only the user's eye catches. A
|
||||||
|
six-row `[Theory]` is three minutes of work.
|
||||||
|
|
||||||
|
### SF-3 — Test-script items that can generate a false failure, and items that test nothing
|
||||||
|
|
||||||
|
- **Item 8** ("Toggle a batched row WITHOUT clicking Apply, then relog. The
|
||||||
|
row should revert to its PRE-toggle value") is only true if the page is
|
||||||
|
hidden first. Structure doc §3.5: `CPlayerSystem::LogOffCharacter
|
||||||
|
@0x00563520` calls `SaveToServer(pm, 0)` — logout flushes the dirty blob.
|
||||||
|
In acdream the same holds (`GameRuntime.cs:333`, the shared flush body). A
|
||||||
|
batched toggle left un-reverted therefore *does* persist. It happens to
|
||||||
|
revert in the common path because reaching Exit Game requires switching to
|
||||||
|
the Gameplay tab (→ `OnHidden` → `Reset`), but a user who closes the client
|
||||||
|
another way will see the opposite of what the script promises. Add "close
|
||||||
|
the Options panel (F11) first" as an explicit step, or state both outcomes.
|
||||||
|
- **Items 10 and the "what to report" enable/disable bullets** ask the user
|
||||||
|
to judge Apply/Reset enable-gating that does not exist (MF-2). Once MF-2
|
||||||
|
lands, add an explicit step: "on first open of the Character tab, Apply and
|
||||||
|
Reset are greyed out; click any LED and they light up; click Apply and they
|
||||||
|
grey out again; Defaults is never greyed out."
|
||||||
|
- **Item 3** as written is a good check and will *correctly* fail today —
|
||||||
|
that is the MF-1 detector. Keep it exactly as-is.
|
||||||
|
|
||||||
|
Otherwise the section is thorough, states retail-expected outcomes per step,
|
||||||
|
and its "Explicitly NOT in scope" block correctly pre-empts the OP3 R1 lesson
|
||||||
|
for the nine no-consumer rows and the two weather/daylight rows.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. NOTE
|
||||||
|
|
||||||
|
- **N-1 — `FindCheckbox` ignores the retail element id.**
|
||||||
|
`CharacterOptionsPageController.cs:384-391` returns the first `UiButton`
|
||||||
|
among the row root's direct children. Retail is
|
||||||
|
`GetChildRecursive(row, 0x10000219)` (`:245616`). Correct for today's
|
||||||
|
single-child template, but the commit *just promoted*
|
||||||
|
`UiElement.FindDescendant` for exactly this purpose — use it with the
|
||||||
|
retail id and delete the bespoke helper.
|
||||||
|
- **N-2 — `FindDescendant`'s semantics changed during promotion.**
|
||||||
|
`UiTabPanel`'s private version skipped the root; `UiElement.FindDescendant`
|
||||||
|
(`UiElement.cs:325-337`) matches the root itself first. `UiTabPanel` calls
|
||||||
|
it as `FindDescendant(this, entry.ButtonElementId/PageElementId)`
|
||||||
|
(`UiTabPanel.cs:162-163,205-208`), so a tab host whose own id collided with
|
||||||
|
a button/page id would now resolve to itself. No collision exists today
|
||||||
|
(host `0x10000208` vs buttons `0x1000020D…` and pages `0x10000211…`), but
|
||||||
|
the behaviour change rode along uncommented in a commit whose message
|
||||||
|
describes the promotion as a pure move.
|
||||||
|
- **N-3 — the sky dome still fogs when Disable Distance Fog is on.**
|
||||||
|
`sky.vert:159-162` computes `vFogFactor` from `uFogParams.xy` with no
|
||||||
|
`mode == 0` guard, unlike the two world shaders. So distant terrain stops
|
||||||
|
fading but the dome's horizon band still blends to fog colour. Retail's
|
||||||
|
`LScape::m_fFogEnabled = !value` may or may not reach its sky path — not
|
||||||
|
established either way. TS-73's "0x30 DisableDistanceFog … now behave
|
||||||
|
correctly" is therefore slightly overstated; the gate script's item 14
|
||||||
|
wisely limits the expectation to "distant terrain/objects", so this is not
|
||||||
|
a false-failure risk, only a register-precision one.
|
||||||
|
- **N-4 — AP-194's anchor column mislabels the symbol.** The prose correctly
|
||||||
|
says `UIOption_Checkbox::SetPlayerOption @0x00486e80`; the "Retail anchor"
|
||||||
|
column at the end of the row says `PlayerModule::SetPlayerOption
|
||||||
|
@0x00486e80`. Same address, wrong class — worth fixing in a register that
|
||||||
|
exists to be grepped by symbol name.
|
||||||
|
- **N-5 — no refresh on the server's own option echo.** Even after MF-1's
|
||||||
|
`GetValue` re-read lands, a change made *while the Character tab is
|
||||||
|
visible* (typing `/consent on`, a mid-session PlayerDescription, a plugin)
|
||||||
|
will not repaint the LED until the next hide/show. Retail has
|
||||||
|
`CM_UI::SendNotice_PlayerOptionChanged` / `SendNotice_RefreshOptionsPanel`
|
||||||
|
(structure doc §3.7) and acdream already has the hook —
|
||||||
|
`LiveSessionRuntimeFactory`'s `OnCharacterOptionsChanged`
|
||||||
|
(character-options-map.md §4.1). Wiring it is small; if it is deferred,
|
||||||
|
file a register row rather than leaving it implicit.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. Fix-order recommendation
|
||||||
|
|
||||||
|
1. MF-1 (`BoolOptionRow` `GetValue` re-read + widget refresh) — blocks the gate.
|
||||||
|
2. MF-2 (enable-gating) — blocks the gate's own acceptance criteria.
|
||||||
|
3. SF-3 (script items 8/10 + the new enable-gate step) — do this *with* MF-2
|
||||||
|
so the script and the code land consistent.
|
||||||
|
4. SF-1, SF-2 — same round, cheap.
|
||||||
|
5. N-1..N-5 — coordinator's call; N-4 is a one-word register edit and N-5
|
||||||
|
wants either a wire-up or a row before OP9's reconciliation.
|
||||||
Loading…
Add table
Add a link
Reference in a new issue