docs: OP4 blast-radius review — APPROVE-WITH-FIXES (3 MUST before gate)
M1: the Character tab binds at startup, pre-session — rows seed from constructor words and nothing re-seeds on PlayerDescription; Reset (and tab-switch, which is OnHidden->Reset) then publishes the STALE baseline over server truth, and the one-shot UiLocked assignment can permanently disagree with the radar's polled read. M2: the Combat panel's three LEDs still write/read the orphaned GameplaySettings copy — decorative, wire-less, and visibly divergent from the Character tab's rows for the same PlayerOptions (the CH3 two-writable-copies mode); headless hardcodes for the same ids filed alongside. M3: ToggleUiLock's convergence guard (initialized converged, now comparing values from two different stores) can early-return past ApplyUiLock — one press flips the wire and radar but not the windows. S1: AddText is NOT acdream's transcript chokepoint — speech/tells, emotes, Turbine channels, combat text and @-replies write ChatLog directly, so the timestamp prefix reaches only ServerMessage/WeenieError lines and gate step 13's tell/say half cannot pass. S2: sky.vert never reads the fog-disable param — crisp terrain against a fogged horizon with the option on. S3/S4: missing register rows for the seven default-source changes (ViewCombatTarget's observable default flips) and the orphaned-but-written GameplaySettings; the timestamp format is culture-dependent where retail strftime is literal. S6: the two changed consumer files ship without their test files touched. Clean: scoped per-page searches structurally sound (fixture-verified), GetOptionBit thread-safe/allocation-free, the run-XOR byte-correct vs SetHoldRun @0x006b3370 with wire encoding untouched. Gate steps 3 and 13 cannot pass pre-fix — the OP4 gate must wait for the fix round. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
22b86b9ff4
commit
0acb341c29
1 changed files with 399 additions and 0 deletions
399
docs/research/2026-08-11-op4-review-blast.md
Normal file
399
docs/research/2026-08-11-op4-review-blast.md
Normal file
|
|
@ -0,0 +1,399 @@
|
|||
# OP4 review — regression / blast-radius lens
|
||||
|
||||
**Commit under review:** `22b86b9f` "feat(ui): Campaign OP slice OP4 — the Character tab"
|
||||
**Reviewer lens:** regression + blast radius (both axes on every changed surface)
|
||||
**Method:** read-only. Diff walk, pre/post consumer enumeration by `git grep` at
|
||||
`22b86b9f^` and `22b86b9f`, fixture-tree analysis of
|
||||
`options_panel_2100006E_1000018D.json`, decomp cross-check against
|
||||
`docs/research/named-retail/acclient_2013_pseudo_c.txt` and `acclient.h`.
|
||||
No build, no test run, no launch.
|
||||
|
||||
**Verdict: APPROVE-WITH-FIXES.** M1–M3 must land before the OP4 connected gate
|
||||
is run — the gate script this commit ships (`docs/research/2026-08-11-campaign-op-test-script.md`
|
||||
§OP4 steps 3 and 13) contains steps that cannot pass against this code, so
|
||||
running it now burns a user session on a known-failing check.
|
||||
|
||||
---
|
||||
|
||||
## 1. Group-C re-point: per-option old-consumer enumeration
|
||||
|
||||
Seven ids moved from the client-local `GameplaySettings` record (settings.json)
|
||||
to `RuntimeCharacterOptionsState.GetOptionBit`. For each: every read site of the
|
||||
OLD value at `22b86b9f^`, and whether it moved.
|
||||
|
||||
### AutoTarget (`0x0D`, Options1 `0x2000`, batched, ClientDefault **ON**)
|
||||
|
||||
| Old read site (`22b86b9f^`) | Moved? |
|
||||
|---|---|
|
||||
| `Composition/FrameRootComposition.cs:599` → `CombatCameraTargetSource(d.Settings, …)` | ✅ → `CharacterOptionCombatSettingsSource` |
|
||||
| `Composition/SessionPlayerComposition.cs:770` → `LiveCombatAttackOperations(… d.Settings …)` | ✅ |
|
||||
| `Composition/InteractionRetainedUiComposition.cs:286` → `LiveCombatTargetOperations(autoTarget: …)` | ✅ |
|
||||
| `UI/Layout/CombatUiController.cs:116` (LED click writes) / `:193` (LED seed reads) | ❌ **NOT moved** |
|
||||
| `Settings/RuntimeSettingsController.cs:236` `public bool AutoTarget => Gameplay.AutoTarget` | ❌ now dead (no consumer) |
|
||||
| `Settings/RuntimeSettingsController.cs:422,433` (`SetCombatGameplay` VM mirrors) | ❌ still mirrors the orphan |
|
||||
| `Headless/Hosting/HeadlessGameplayOperations.cs:127` `AutoTarget => true` (hardcoded) | ❌ never re-pointed |
|
||||
|
||||
### AutoRepeatAttack (`0x00`, Options1 `0x02`, **auto-save**, ClientDefault ON)
|
||||
|
||||
| Old read site | Moved? |
|
||||
|---|---|
|
||||
| `Composition/SessionPlayerComposition.cs:770` → `LiveCombatAttackOperations` (→ `RuntimeCombatAttackState:380,403,411`) | ✅ |
|
||||
| `UI/Layout/CombatUiController.cs:114` / `:192` | ❌ **NOT moved** |
|
||||
| `Settings/RuntimeSettingsController.cs:238` | ❌ now dead |
|
||||
| `Headless/Hosting/HeadlessGameplayOperations.cs:126` `AutoRepeatAttack => false` | ❌ never re-pointed |
|
||||
|
||||
### ViewCombatTarget (`0x07`, Options1 `0x80`, batched, ClientDefault **OFF**)
|
||||
|
||||
| Old read site | Moved? |
|
||||
|---|---|
|
||||
| `Combat/CombatCameraTargetSource.cs:44` (via injected source) | ✅ |
|
||||
| `UI/Layout/CombatUiController.cs:118` / `:194` | ❌ **NOT moved** |
|
||||
| `Settings/RuntimeSettingsController.cs:240` | ❌ now dead |
|
||||
|
||||
⚠️ **Observable-default flip.** `GameplaySettings.Default.ViewCombatTarget = true`
|
||||
(`src/AcDream.UI.Abstractions/Panels/Settings/GameplaySettings.cs:51`) but bit
|
||||
`0x80` is CLEAR in retail's `CharacterOptions1.Default = 0x50C4A54A`
|
||||
(→ `CharacterOptionTable.cs:118` `ClientDefault: false`). A fresh install's
|
||||
combat camera stops tracking the target by default. No register row filed.
|
||||
|
||||
### VividTargetingIndicator (`0x0E`, Options1 `0x8000`, batched, ClientDefault ON)
|
||||
|
||||
| Old read site | Moved? |
|
||||
|---|---|
|
||||
| `Composition/InteractionRetainedUiComposition.cs:676` → `VividTargetRuntimeBindings` | ✅ (polled lambda) |
|
||||
| `Panels/Settings/SettingsPanel.cs:322` | n/a — `SettingsPanel` has **no production construction site** (`new SettingsPanel(` appears only in `tests/AcDream.UI.Abstractions.Tests/…:40`) |
|
||||
|
||||
### CoordinatesOnRadar (`0x14`, Options1 `0x400000`, batched, ClientDefault ON)
|
||||
|
||||
| Old read site | Moved? |
|
||||
|---|---|
|
||||
| `Composition/LivePresentationComposition.cs:836` → radar snapshot | ✅ (polled lambda) |
|
||||
|
||||
### LockUI (`0x33`, Options2 `0x1000000`, **auto-save**, ClientDefault OFF)
|
||||
|
||||
| Old read site | Moved? |
|
||||
|---|---|
|
||||
| `Composition/LivePresentationComposition.cs:837` `uiLocked:` (radar) | ✅ polled — converges live |
|
||||
| `Composition/InteractionRetainedUiComposition.cs:477` `host.Root.UiLocked = …` | ⚠️ moved but **one-shot at mount**, never re-read → does NOT converge |
|
||||
| `Net/LiveSessionRuntimeFactory.cs:387` `ToggleUiLock` read | ✅ reads the bit — but see **M3** |
|
||||
| `Settings/RuntimeSettingsController.cs:355,359,362,369` (`SetUiLocked` writer) | ❌ still writes the orphan, and is still called from `ToggleUiLock` |
|
||||
|
||||
Note: `LockUI` is deliberately NOT a Character-tab row
|
||||
(`CharacterOptionsPageControllerTests.ExcludedIds_HaveNoRow`), and the OP4 gate
|
||||
script's Group-C section (items 17–18) does not list it — so this re-point ships
|
||||
ungated.
|
||||
|
||||
### AcceptLootPermits (`0x10`, Options1 `0x80000`, **auto-save**, ClientDefault OFF)
|
||||
|
||||
| Old read site | Moved? |
|
||||
|---|---|
|
||||
| `Net/LiveSessionRuntimeFactory.cs:452` (`ClientCommandController.Bindings.AcceptLootPermits`) | ✅ |
|
||||
| `Net/LiveSessionRuntimeFactory.cs:454` (`SetAcceptLootPermits`) | ✅ (now sends 0x0005 **and** writes the orphan) |
|
||||
| `UI/ClientCommandController.cs:444,450` (`/consent on|off`) | ✅ via the bindings above |
|
||||
| `Net/LiveSessionCommandRouter.cs:458,459` | ✅ pure forwarders |
|
||||
|
||||
### ToggleRun (`0x0A`, Options1 `0x400`, batched, ClientDefault ON) — Group B
|
||||
|
||||
| Old read site | Moved? |
|
||||
|---|---|
|
||||
| `Input/DispatcherMovementInputSource.cs:77` hardcoded `!walking` | ✅ → `RunAsDefaultMovement != walking` |
|
||||
| `Panels/Settings/SettingsPanel.cs:314` | n/a (test-only surface) |
|
||||
|
||||
---
|
||||
|
||||
## 2. MUST-FIX
|
||||
|
||||
### M1 — The Character tab is seeded before login and never re-seeded; Reset then writes the stale baseline over server truth
|
||||
|
||||
`RetailUiRuntime.Initialize()` (`src/AcDream.App/UI/RetailUiRuntime.cs:371`)
|
||||
calls `MountOptionsPanel()`, which calls
|
||||
`CharacterOptionsPageController.Bind` (`RetailUiRuntime.cs:2010`). That whole
|
||||
chain runs inside `lease.Mount(…)`
|
||||
(`src/AcDream.App/Composition/InteractionRetainedUiComposition.cs:876`), which
|
||||
runs inside `InteractionRetainedUiCompositionPhase` — a **startup** phase of
|
||||
`GameWindow.OnLoad` (`src/AcDream.App/Rendering/GameWindow.cs:1231`, chain at
|
||||
`:1325-1369`). No network session exists yet.
|
||||
|
||||
So at `CharacterOptionsPageController.cs:368`:
|
||||
|
||||
```csharp
|
||||
bool initial = bindings.CurrentValue(spec.Id); // == GetOptionBit(...)
|
||||
checkbox.Selected = initial;
|
||||
var row_ = new BoolOptionRow(initial, entry.ClientDefault, apply: …);
|
||||
```
|
||||
|
||||
`initial` is read from `RuntimeCharacterOptionsState`'s **constructor** words
|
||||
(`RuntimeCharacterState.cs:671-672`, `DefaultOptions1 = 0x50C4A54A` /
|
||||
`DefaultOptions2 = 0x00948700`), not the character's server options.
|
||||
`PlayerDescription` installs the real words later at
|
||||
`src/AcDream.Runtime/Session/LiveSessionEventRouter.cs:223` (`Options.Replace`),
|
||||
and the only subscriber to `OnCharacterOptionsChanged` is
|
||||
`SyncChatFromServerOptions` (`Net/LiveSessionRuntimeFactory.cs:323-324`).
|
||||
**Nothing re-seeds the 50 rows.** Consequences:
|
||||
|
||||
1. Gate-script step 3 ("Confirm the checkboxes reflect your character's actual
|
||||
state … seeded from the live server bit at panel-mount time") is false for
|
||||
every option whose server value differs from the client constructor default.
|
||||
2. Worse — the `BoolOptionRow` `_saved` baseline is also stale. Take a batched
|
||||
option the server has ON but whose constructor default is OFF (e.g.
|
||||
`SideBySideVitals`). Row seeds `_current = _saved = false`. User clicks it →
|
||||
`SetCurrentValue(true)` → `TrySetOption` sees the bit already true → no-op
|
||||
(correct). User changes their mind and clicks **Reset** (or just switches
|
||||
tab, which is `OnHidden() → Reset()`, `OptionPageModel.cs:261`) →
|
||||
`Changed == true` → `RestoreSavedValue()` → `_apply(false)` →
|
||||
`SetSingleCharacterOptionRuntimeCmd(id, false)` → **the server bit is turned
|
||||
OFF.** A visually-idempotent "toggle then cancel" mutates real character
|
||||
state in the wrong direction. This is the "contract asserting a mechanism
|
||||
that does not exist" class the C4 closeout handoff flags as binding.
|
||||
3. `host.Root.UiLocked = d.Character.Options.GetOptionBit(LockUI)`
|
||||
(`InteractionRetainedUiComposition.cs:477-486`) has the same shape but is a
|
||||
one-shot assignment, so retained-window locking never converges to the
|
||||
server bit — while the radar's `uiLocked` lambda
|
||||
(`LivePresentationComposition.cs:843-844`) polls and does converge. Two
|
||||
consumers of one option, permanently able to disagree.
|
||||
|
||||
The fix shape is a re-seed hook on `OnCharacterOptionsChanged` (the seam CH3
|
||||
already established for chat) that re-runs each row's
|
||||
`SetSeed(current)` — i.e. rewrite `_current`, `_saved`, and `checkbox.Selected`
|
||||
without invoking `_apply`. That verb does not exist on `BoolOptionRow` today.
|
||||
|
||||
### M2 — Two writable copies: the Combat panel's three LEDs still drive the orphaned `GameplaySettings`
|
||||
|
||||
`src/AcDream.App/UI/Layout/CombatUiController.cs:113-118` writes and `:192-194`
|
||||
reads `GameplaySettings.AutoRepeatAttack / AutoTarget / ViewCombatTarget`,
|
||||
wired in production at `InteractionRetainedUiComposition.cs:644-648`
|
||||
(`() => d.Settings.Gameplay`, `d.Settings.SetCombatGameplay`) and mounted at
|
||||
`RetailUiRuntime.cs:366` (`MountCombat()`). Nothing gameplay-facing reads that
|
||||
record any more — every behavioral consumer now reads the server bit. Net
|
||||
effect:
|
||||
|
||||
- The retail Combat window's **Repeat Attacks / Auto Target / Keep in View**
|
||||
checkboxes are now decorative: clicking them changes combat behavior not at
|
||||
all, and never reaches the wire.
|
||||
- They and the Character tab's rows for the *same three retail
|
||||
`PlayerOption`s* read from different stores, so they will visibly disagree.
|
||||
- `RuntimeSettingsController.SetCombatGameplay` (`:415-442`) keeps persisting
|
||||
the divergent copy to settings.json, so the disagreement survives restarts.
|
||||
|
||||
This is exactly the "two writable copies" failure mode the CH3 research warned
|
||||
about, and it is the one old read site of the three combat options that the
|
||||
re-point missed. The Combat panel's LEDs must route through
|
||||
`SetSingleCharacterOptionRuntimeCmd` and seed from `GetOptionBit`, in this
|
||||
slice or an immediately-following one.
|
||||
|
||||
### M3 — `ToggleUiLock`'s new dual write can silently drop the visual lock
|
||||
|
||||
`src/AcDream.App/Net/LiveSessionRuntimeFactory.cs:392-400`:
|
||||
|
||||
```csharp
|
||||
bool locked = !_domain.Character.Options.GetOptionBit(CharacterOptionId.LockUI);
|
||||
SendSingleCharacterOption((uint)CharacterOptionId.LockUI, locked);
|
||||
_interaction.Settings.SetUiLocked(locked);
|
||||
```
|
||||
|
||||
`RuntimeSettingsController.SetUiLocked` opens with
|
||||
(`src/AcDream.App/Settings/RuntimeSettingsController.cs:355`):
|
||||
|
||||
```csharp
|
||||
if (Gameplay.LockUI == locked && _uiLockConverged) return;
|
||||
```
|
||||
|
||||
with `_uiLockConverged = true` at construction (`:185`). **Before** this commit
|
||||
`locked` was computed as `!Gameplay.LockUI`, so the guard could never trip.
|
||||
Now `locked` is derived from a *different* store, so whenever the persisted
|
||||
`Gameplay.LockUI` already equals the newly-computed value the method
|
||||
early-returns and `_runtimeTargets?.ApplyUiLock(locked)` (`:360`) never runs —
|
||||
the wire bit flips, the radar (polled) follows, and `host.Root.UiLocked` does
|
||||
not. Concretely: a user with `"lockUI": true` in settings.json (set by
|
||||
`/lockui` in any pre-OP4 session) presses the lock toggle once after login and
|
||||
nothing happens to their windows, while the server is told the UI is locked.
|
||||
|
||||
---
|
||||
|
||||
## 3. SHOULD-FIX
|
||||
|
||||
### S1 — `DisplayTimeStamps` only reaches a small subset of transcript lines
|
||||
|
||||
`RuntimeCommunicationState.AddText` (`:219-227`) is documented as "the single
|
||||
chokepoint every producer of player-visible interface text must go through",
|
||||
and retail's `ClientSystem::AddTextToScroll @0x00563C50` genuinely is that
|
||||
chokepoint. In acdream it is not. These producers write `ChatLog` directly and
|
||||
therefore never get a timestamp:
|
||||
|
||||
| Producer | Site |
|
||||
|---|---|
|
||||
| All heard speech (say / tell / ranged) | `Runtime/Session/LiveSessionEventRouter.cs:293` `Chat.OnLocalSpeech` |
|
||||
| Emotes / soul emotes | `LiveSessionEventRouter.cs:319,321` |
|
||||
| Player-kill messages | `LiveSessionEventRouter.cs:325` |
|
||||
| Turbine channels (allegiance/general/trade/LFG/RP/society) | `LiveSessionEventRouter.cs:~535-556` via `RouteTurbineChat` |
|
||||
| All combat text | `CombatChatTranslator` (`LiveSessionEventRouter.cs:306`) writes `social.Chat` directly |
|
||||
| `@age` / `@index` / `@clist` / `@hslist` / allegiance-info replies | `Core.Net/GameEventWiring.cs:162,176,183,190,197` |
|
||||
| App-side echoes | `Net/LiveSessionRuntimeFactory.cs:165,169,418` |
|
||||
| Headless chat | `Headless/Hosting/HeadlessGameplayOperations.cs:242` |
|
||||
|
||||
Only `ServerMessage (0xF7E0)` and the WeenieError family route through `AddText`
|
||||
(`LiveSessionEventRouter.cs:310-317`, `GameEventWiring.cs:~655-668`). The gate
|
||||
script's own step 13 ("send/receive a new chat line … e.g. `/who` or any inbound
|
||||
tell/say. The NEW line should be prefixed") is therefore unsatisfiable for the
|
||||
tell/say half. Either move the remaining producers behind `AddText` or narrow
|
||||
the register/gate wording to what is actually implemented.
|
||||
|
||||
Direction check (nothing leaks where it must not): the `ClientLocal`/SpewBox
|
||||
branch returns before the prefix (`:213-217`) — correct and tested; there is no
|
||||
chat-log-file writer in the tree (`grep` for `ChatLogFile`/`WriteChatLog` is
|
||||
empty); `AddText` is display-only so nothing reaches the wire; the chat
|
||||
filters in `Panels/Chat/ChatCommandRouter.cs` / `ChatInputParser.cs` match on
|
||||
*outbound input*, not transcript lines, so a prefix cannot confuse them.
|
||||
|
||||
### S2 — `FogMode.Off` is not honored by the sky shader
|
||||
|
||||
`WeatherSystem` forces `fogMode = FogMode.Off` (`WeatherState.cs:326-327`) and
|
||||
the mesh/terrain fragment shaders respect it
|
||||
(`Rendering/Shaders/mesh_modern.frag:48-49` `if (mode == 0) return lit;`,
|
||||
`terrain_modern.frag:160`). But `Rendering/Shaders/sky.vert:159-162` computes
|
||||
`vFogFactor` from `uFogParams.x/.y` only and **never reads `uFogParams.w`**;
|
||||
`sky.frag:62` then blends the dome toward the fog color. With Disable Distance
|
||||
Fog ON the world is crisp to the horizon while the sky dome keeps its fog-color
|
||||
band — a visible seam exactly where the option is supposed to remove one. This
|
||||
is a visual-gate item (gate step 14) and needs the user's eyes either way, but
|
||||
the shader asymmetry is a code fact, not a judgement call.
|
||||
|
||||
### S3 — Observable-default changes are not registered
|
||||
|
||||
The register hunk adds/updates `AP-194`, `TS-73`, `TS-75`–`TS-80`. None of them
|
||||
records that seven options changed their *effective default source* from
|
||||
`GameplaySettings.Default` (or the user's persisted settings.json) to the
|
||||
retail constructor word, nor that `ViewCombatTarget`'s observable default flips
|
||||
`true → false`, nor that `GameplaySettings.{AutoTarget, AutoRepeatAttack,
|
||||
ViewCombatTarget, LockUI, VividTargetingIndicator, CoordinatesOnRadar,
|
||||
AcceptLootPermits, ToggleRun}` are now orphaned-but-still-written. The campaign
|
||||
contracted register rows for observable-default changes; these are the rows.
|
||||
|
||||
### S4 — The timestamp format is culture-dependent
|
||||
|
||||
`RuntimeCommunicationState.cs:225`:
|
||||
|
||||
```csharp
|
||||
text = DateTime.Now.ToString("H:mm:ss ") + text;
|
||||
```
|
||||
|
||||
No `IFormatProvider`, so `:` is `CurrentCulture.DateTimeFormat.TimeSeparator`.
|
||||
Retail's CRT `strftime("%#H:%M:%S ")` emits literal colons unconditionally. On a
|
||||
machine whose culture uses `.` (e.g. `fi-FI`) acdream renders `13.05.09 `.
|
||||
The new test `AddText_TimestampsTrue_PrefixesTranscriptLine` pins
|
||||
`^\d{1,2}:\d{2}:\d{2} …` and would fail on such a machine — i.e. the bug is
|
||||
also a latent CI/locale flake. Use `CultureInfo.InvariantCulture`.
|
||||
|
||||
### S5 — The headless host's combat-option reads were not swept
|
||||
|
||||
`src/AcDream.Headless/Hosting/HeadlessGameplayOperations.cs:126-127` still
|
||||
hardcodes `AutoRepeatAttack => false` and `AutoTarget => true`. OP7 gave
|
||||
headless bots a `characterOptions` config that writes these very bits, so a bot
|
||||
configured with `AutoRepeatAttack` now sets the server bit while its own combat
|
||||
loop still reads `false`. Same interface (`ICombatGameplaySettingsSource`
|
||||
shape), same options, opposite direction from the graphical host after this
|
||||
commit.
|
||||
|
||||
### S6 — Test coverage gaps on the two changed *consumers*
|
||||
|
||||
The Runtime-level OFF paths are well covered (fog unbound/false/true, timestamps
|
||||
unbound/false/true + SpewBox exemption, `RunAsDefaultMovement` unbound/true/false
|
||||
live-poll). What is **not** covered:
|
||||
|
||||
- `DispatcherMovementInputSource.Capture()` — the file changed
|
||||
(`:83`) but `tests/AcDream.App.Tests/Input/DispatcherMovementInputSourceTests.cs`
|
||||
was not touched. The existing
|
||||
`CapturesDispatcherHeldStateAndRetailWalkModifier` only exercises
|
||||
option-unbound (⇒ `true`) + walk-held. Nothing pins the four-way truth table
|
||||
(option × modifier), nor the `|| AutoRunActive` override with the option OFF.
|
||||
- The per-page scoped Apply/Reset/Defaults wiring —
|
||||
`OptionsPanelControllerTests.cs` was not touched, so the mechanism the commit
|
||||
message calls out (`UiElement.FindDescendant` scoping) has no test that a
|
||||
Chat-page Apply drives the Chat page's model rather than Character's.
|
||||
- Nothing pins the login-before-seed transition (M1) — unsurprising, since the
|
||||
mechanism does not exist.
|
||||
|
||||
---
|
||||
|
||||
## 4. NOTE
|
||||
|
||||
- **N1 — `FindDescendant` promotion changed semantics, and the scoping is
|
||||
sound.** The old private `UiTabPanel.FindDescendant` skipped the root; the new
|
||||
`UiElement.FindDescendant` (`src/AcDream.App/UI/UiElement.cs:312-334`) matches
|
||||
the root first. `UiTabPanel:162,163,205,208` now calls the root-inclusive
|
||||
version — harmless here because the panel's own id (`0x1000018D`) is not a tab
|
||||
button or page id, but it is a real behavior change worth knowing.
|
||||
On the OP3 re-review's duplicate-id hazard: the committed fixture
|
||||
`options_panel_2100006E_1000018D.json` has 111 nodes / 38 unique ids and **16
|
||||
duplicated ids** (`0x100002CE/CF/D0` ×16 each; `0x100001FC/FD/FE` ×3). The
|
||||
scoped searches are genuinely scoped — each page slot is a direct child of the
|
||||
root and owns its own Apply/Reset/Defaults instance:
|
||||
`/0x1000018D/0x10000211/{0x100001FC,FD,FE}`,
|
||||
`/0x1000018D/0x10000213/{…}`, `/0x1000018D/0x1000050C/{…}`. A Character-page
|
||||
search structurally cannot reach a Chat-page button. Separately,
|
||||
`0x100001FA`/`0x100001FB` (the Character ListBox + scrollbar) are **unique**,
|
||||
so `CharacterOptionsPageController`'s flat `layout.FindElement` lookups
|
||||
(`:280`, `:293`) are unambiguous — but that is a property of the data, not of
|
||||
the code, and nothing asserts it.
|
||||
- **N2 — dead surface.** `RuntimeSettingsController.AutoTarget` /
|
||||
`AutoRepeatAttack` / `ViewCombatTarget` (`:236-240`) now have zero consumers.
|
||||
`GameplaySettingsState` (`Combat/LiveCombatAttackOperations.cs:23`) survives
|
||||
only in `tests/AcDream.App.Tests/Combat/CombatCameraTargetSourceTests.cs:16`.
|
||||
- **N3 — thread-safety is consistent; the binding fields are not volatile.**
|
||||
`GetOptionBit` (`RuntimeCharacterState.cs:858-865`) reads one word through the
|
||||
existing `Volatile.Read`-backed `Options1`/`Options2` properties, one static
|
||||
`Dictionary` lookup, no lock, no allocation (`CharacterOptionTableEntry` is a
|
||||
`readonly record struct`, `TryGetValue` uses `out`). That matches every other
|
||||
read on the type. The three new `Func<bool>?` source properties are plain auto
|
||||
properties written once in the `GameWindow` constructor
|
||||
(`Rendering/GameWindow.cs:649-678`) before the render loop and the network
|
||||
thread exist, so publication is fine in practice but is not formally
|
||||
guaranteed. `WeatherSystem` is a get-only property assigned once
|
||||
(`Runtime/World/RuntimeWorldEnvironmentState.cs:144,149`), so the fog binding
|
||||
cannot be lost on session reset. Per-frame cost is ~3 dictionary lookups
|
||||
(fog + radar ×2) + 1 in `Capture()` — negligible, and none of them is on a
|
||||
Slice-I zero-allocation resolve path.
|
||||
- **N4 — `FindCheckbox` is direct-children-only.**
|
||||
`CharacterOptionsPageController.cs:385-392` searches the row root and its
|
||||
*direct* children while the commit simultaneously promotes a recursive
|
||||
`FindDescendant` for exactly this class of lookup. If a template ever nests
|
||||
the checkbox one level deeper, all 50 rows silently degrade to a log line.
|
||||
- **N5 — the timestamp weakens `ChatLog`'s text-keyed dedupe.**
|
||||
`ChatLog.OnSystemMessage` (`src/AcDream.Core/Chat/ChatLog.cs:266-278`) dedupes
|
||||
on exact text within 1 s. Timestamped and untimestamped copies of the same
|
||||
logical line (one via `AddText`, one via a direct producer from S1) can no
|
||||
longer match; two identical lines straddling a second boundary also stop
|
||||
matching. This is an acdream-only workaround, not retail behavior, so the
|
||||
consequence is a re-appearing duplicate, not a correctness break.
|
||||
- **N6 — autorun ignores the new default; retail's own seam does not.**
|
||||
`Run: (_movement.RunAsDefaultMovement != walking) || AutoRunActive`
|
||||
(`Input/DispatcherMovementInputSource.cs:83`). The `!= walking` half is
|
||||
byte-correct against retail: `CommandInterpreter::SetHoldRun @0x006b3370`
|
||||
computes `(!hold_run) != (!UITogglesRun())` — i.e. `holdRun XOR ToggleRun` —
|
||||
and passes that to `CMotionInterp::set_hold_run`, which is precisely where
|
||||
acdream's `MovementInput.Run` lands
|
||||
(`Runtime/Gameplay/PlayerMovementController.cs:1166-1168`). The wire encoding
|
||||
is untouched and stays retail-correct in both option states: `Run` →
|
||||
`IsRunning` → `HoldKey.Run`/`HoldKey.None` + `ForwardSpeed`
|
||||
(`Runtime/Gameplay/LocalPlayerOutboundController.cs:234-248`), so no
|
||||
double-inversion. There is also no collision with the `ToggleRunLock`
|
||||
keybind latch — that drives `_autoRunActive`, a separate field, and the
|
||||
property's doc comment says so explicitly. What is newly *reachable* is the
|
||||
`|| AutoRunActive` clause: `CommandInterpreter::SetAutoRun @0x006b4850` sets
|
||||
`auto_run` and calls `ApplyCurrentMovement()` — it does **not** force run —
|
||||
so with the option OFF an autorunning acdream player runs where retail would
|
||||
walk. That `||` predates this commit (2026-07-30 movement-parity audit) but
|
||||
was unobservable while the default was hardcoded.
|
||||
- **N7 — citation accuracy.** `RuntimeLocalPlayerMovementState`'s doc comment
|
||||
names "retail `PlayerOption RunAsDefaultMovement`". No such member exists —
|
||||
`acclient.h:4165` spells id `0xA` `ToggleRun_PlayerOption`, and
|
||||
`RunAsDefaultMovement` is ACE's name for the bit. The binding
|
||||
(`GameWindow.cs:677`) and the row's `RetailName` (`"ToggleRun"`, which is what
|
||||
hashes `ID_PlayerOption_ToggleRun` — confirmed at pseudo-C `:171586`) are both
|
||||
correct; only the prose is wrong.
|
||||
- **N8 — `Defaults` can emit a burst of 21 immediate `0x0005` sends.**
|
||||
`OptionPage.Defaults()` calls `RestoreDefaultValue()` on all 50 rows, each of
|
||||
which publishes `SetSingleCharacterOptionRuntimeCmd`. `TrySetOption` no-ops on
|
||||
unchanged values, so the worst case is bounded by the auto-save set (21 ids).
|
||||
Retail does the same thing; recorded here only so a future wire-rate reviewer
|
||||
does not treat it as a bug.
|
||||
Loading…
Add table
Add a link
Reference in a new issue