e318e8628d
M1: the 'U4 resolved: zero range captions' claim is the SAME BN zero-fold artifact lane A caught on AddHeader — the bytes at 0x0049E4C6 load two string-id globals (ID_Graphics_Value_Hard/_Soft) and all six labelled sliders decode cleanly (Stiffness Soft/Hard, Adjustment Slow/Fast, FOV Narrow/Wide, Brightness Dark/Bright, Performance Speed/Detail, Degrade Close/Far) — U4 closes the OPPOSITE way; OP5's Chat sliders already ported the same mechanism. M2: the three Sound 'Disabled' toggles are sense-INVERTED (the INI key names the string; the registered static is effect_sounds_enabled = 1) — SFX/ambient ship muted for every user including existing settings.json installs, undoing Campaign A; a test, AP-199's wording, and gate step 6 all enshrine it. M3: UI_ChatFontFace ships 1 of retail's 5 authored choices (all five font strings verbatim in the binary) with default index 2 out of range — a self-inflicted copy of the genuine LandscapeDrawDistance oddity. S1: the sixth trailing separator missing (retail builds 39 items); lane A's '24 rows' is itself stale (27 is right). S2: Screen Brightness overloads Gamma ([-1,1] default 0 into a [0.5,2.0] multiplier defaulting 1.0). S3: tooltips reach 12/30 rows (TooltipText exists only on UiButton). S4: the 800x600 default is absent from the hardcoded resolution list. S5: the tests pin safe seams and miss every risky one (no ApplyAudio value assertion, no label/choice conformance, no per-row default pin, no count pin). A large verified-clean catalog is recorded so the rework does not re-litigate. Rework round 1 queues behind the OP5 fixer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
486 lines
28 KiB
Markdown
486 lines
28 KiB
Markdown
# OP6 (Config tab) — combined dual-lens review
|
||
|
||
**Commit under review:** `f5ac1742` "feat(ui): Campaign OP slice OP6 — the Config tab"
|
||
**Contract:** `docs/plans/2026-08-10-options-panel-campaign.md` §4 OP6 + §2 D1
|
||
**Research base:** `2026-08-10-options-panel-structure.md` §4/§9;
|
||
`2026-08-10-keyboard-config-and-gameplay-tab.md` §7.3
|
||
**Reviewer:** combined mechanism × blast lens, Opus, 2026-08-11. Read-only.
|
||
|
||
## Verdict: **REJECT**
|
||
|
||
Three MUST-FIX findings, two of them contradicting decompiler claims the slice
|
||
asserts as *resolved fact* in code comments, the commit message, AND the
|
||
connected-gate script. One of the three (M2) mutes sound effects and ambient
|
||
audio for **every** profile — fresh and existing — on the next launch, which is
|
||
a direct regression of Campaign A (closed 2026-08-08). Because the gate script
|
||
instructs the user to treat both wrong behaviours as expected (§OP6 steps 6 and
|
||
"What to report"), a connected gate as written would *certify* the defects
|
||
rather than catch them. That combination — false primary-source claims + a
|
||
user-visible regression + an acceptance script that blesses them — is past
|
||
APPROVE-WITH-FIXES.
|
||
|
||
Everything else in the slice is unusually good: the template-index mapping, all
|
||
eight authored template ids, every real-unit slider range, every truncated label
|
||
key, the `AddSliderOption` BN-artifact decode, and the scoped-scrollbar fix all
|
||
verify clean against the binary and the committed fixture (see §"Verified
|
||
correct" below). The failures are concentrated and fixable.
|
||
|
||
---
|
||
|
||
## Evidence discipline for this review
|
||
|
||
`check_exe_pdb.py C:\Users\erikn\Downloads\acclient.exe` → `MATCH` (linker
|
||
`2013-09-06T00:17:56Z`, CodeView GUID `9e847e2f-777c-4bd9-886c-22256bb87f32`).
|
||
All byte quotes below were decoded from that binary via a PE VA→file-offset
|
||
reader; image base `0x400000`, `.text` VA `0x401000` / raw `0x1000`.
|
||
|
||
---
|
||
|
||
# MUST-FIX
|
||
|
||
## M1 — the "U4 resolved: retail ships ZERO range captions" claim is FALSE (byte-disproved)
|
||
|
||
**Claim under review** (`src/AcDream.App/UI/Layout/ConfigOptionsPageController.cs:44-58`):
|
||
|
||
> **U4 resolved: retail ships ZERO slider range captions on this tab.** Every
|
||
> one of the seven `PlayerOptionPage::SetSliderLabel` calls this tab makes
|
||
> passes literal `(0, 0)` … This controller therefore never touches the
|
||
> range-caption text children (`0x1000021E`/`0x1000021F`) at all — omitting
|
||
> them is the faithful port, not a shortcut.
|
||
|
||
Repeated in the commit message ("retail ships ZERO range captions on this tab
|
||
(every SetSliderLabel call passes literal string id 0)") and in the gate script
|
||
(`docs/research/2026-08-11-campaign-op-test-script.md:535`, and :653 which tells
|
||
the user to **report a caption as a defect**).
|
||
|
||
**The claim rests entirely on the Binary Ninja pseudo-C rendering**
|
||
`PlayerOptionPage::SetSliderLabel(this, eax_5, 0, 0)`. That rendering is the
|
||
*same* zero-fold artifact the lane-A researcher already caught and worked
|
||
around: in the identical function BN also renders `PlayerOptionPage::AddHeader(this, 0)`
|
||
six times, yet lane A byte-decoded six *different* header string-id globals
|
||
(`2026-08-10-options-panel-structure.md:492-499`) — and this slice itself uses
|
||
those decoded header keys. The implementer applied the correct skepticism to
|
||
`AddHeader` and not to `SetSliderLabel`, in the same 60 lines of pseudo-C.
|
||
|
||
**Byte evidence — the operands are globals, not immediates.** Camera Stiffness,
|
||
`0x0049E4C4`:
|
||
|
||
```
|
||
0049E4B8 6a 01 push 1 ; arg3 = 1 (labelled)
|
||
0049E4BA 68 a0 e5 83 00 push offset Camera_Stiffness
|
||
0049E4BF 8b ce mov ecx, esi
|
||
0049E4C1 e8 a7 45 05 00 call PlayerOptionPage::AddSliderOption
|
||
0049E4C6 8b 0d 94 e7 83 00 mov ecx, [0083E794] ; <- arg4, NOT 0
|
||
0049E4CC 8b 15 90 e7 83 00 mov edx, [0083E790] ; <- arg3, NOT 0
|
||
0049E4D2 51 52 50 push ecx / push edx / push eax
|
||
0049E4DB 8b ce mov ecx, esi
|
||
0049E4E0 e8 9b 46 05 00 call PlayerOptionPage::SetSliderLabel
|
||
```
|
||
|
||
The PDB names those globals (`acclient_2013_pseudo_c.txt:1148623-1148634`):
|
||
|
||
| VA | Symbol |
|
||
|---|---|
|
||
| `0x0083E768` / `0x0083E76C` | `ID_Graphics_Value_Dark` / `_Bright` |
|
||
| `0x0083E770` / `0x0083E774` | `ID_Graphics_Value_Speed` / `_Detail` |
|
||
| `0x0083E778` / `0x0083E77C` | `ID_Graphics_Value_Close` / `_Far` |
|
||
| `0x0083E780` / `0x0083E784` | `ID_Graphics_Value_Narrow` / `_Wide` |
|
||
| `0x0083E788` / `0x0083E78C` | `ID_Graphics_Value_Slow` / `_Fast` |
|
||
| `0x0083E790` / `0x0083E794` | `ID_Graphics_Value_Soft` / `_Hard` |
|
||
|
||
They are `uint32_t const … = 0x0` in `.data` (runtime-filled by string-table
|
||
registration) — which is exactly *why* BN constant-folded them to `0`.
|
||
|
||
**All six call sites decoded, giving the complete U4 answer:**
|
||
|
||
| Slider | `SetSliderLabel` VA | arg3 (low, child `0x1000021E`) | arg4 (high, child `0x1000021F`) |
|
||
|---|---|---|---|
|
||
| `Camera_Stiffness` | `0x0049E4E0` | `ID_Graphics_Value_Soft` | `ID_Graphics_Value_Hard` |
|
||
| `Camera_AdjustmentSpeed` | `0x0049E51D` | `..._Slow` | `..._Fast` |
|
||
| `Render_FieldOfView` | `0x0049E556` | `..._Narrow` | `..._Wide` |
|
||
| `Render_ScreenBrightness` | `0x0049E614` | `..._Dark` | `..._Bright` |
|
||
| `Render_GraphicsPerformance` | `0x0049E65D` | `..._Speed` | `..._Detail` |
|
||
| `Render_DegradeDistance` | `0x0049E68E` | `..._Close` | `..._Far` |
|
||
|
||
Six labelled sliders ↔ six pairs ↔ lane A's six observed pairs, in declaration
|
||
order, each pairing semantically obvious. `Input_MouseLookSensitivity` (`arg3=0`,
|
||
template idx3) correctly gets no `SetSliderLabel` — and idx3 has no caption
|
||
children. **U4 is now closed in the opposite direction from the commit's claim.**
|
||
|
||
**Cross-check that removes all doubt:** OP5 already ported this exact mechanism.
|
||
`gmChatOptionsUI::InitOptions` renders as `SetSliderLabel(this, esi, 0, 0)`
|
||
(`acclient_2013_pseudo_c.txt:170779`), yet at `0x0049FD37` the bytes are
|
||
`a1 2c e8 83 00 / 8b 0d 28 e8 83 00 / 50 51 56` — `[0x0083E82C]` /
|
||
`[0x0083E828]`, i.e. `ID_UI_Value_Opaque` / `ID_UI_Value_Transparent` — and
|
||
`ChatOptionsPageController.cs:313-315` renders them. The campaign's own gate
|
||
script contradicts itself between line 381 (Chat: captions present) and line 535
|
||
(Config: "retail ships zero range captions").
|
||
|
||
**Blast:** the authored idx6 template *has* both caption children
|
||
(`Fixtures/options_2100002B.json`, element `0x1000021D` → children `0x1000021B`,
|
||
`0x1000021C`, `0x1000021E`, `0x1000021F`), so six rows currently render two
|
||
empty caption slots each. The fix is a straight reuse of
|
||
`ChatOptionsPageController.SetRangeLabel`'s shape at
|
||
`ConfigOptionsPageController.cs:681-729` (pass the two keys per row), plus
|
||
correcting the class doc, the commit-message claim in the ledger, and gate-script
|
||
lines 535 and 653.
|
||
|
||
---
|
||
|
||
## M2 — the three Sound "Disabled" toggles are semantically inverted; SFX + ambient ship MUTED
|
||
|
||
**What the code does.** `AudioSettings` gained
|
||
`SfxDisabled = true` / `AmbientDisabled = true` / `InterfaceDisabled = true`
|
||
(`src/AcDream.UI.Abstractions/Panels/Settings/AudioSettings.cs:51-55`), and the
|
||
engine write became
|
||
(`src/AcDream.App/Settings/RuntimeSettingsTargets.cs:147-148`):
|
||
|
||
```csharp
|
||
engine.SfxVolume = audio.SfxDisabled ? 0f : audio.Sfx;
|
||
engine.AmbientVolume = audio.AmbientDisabled ? 0f : audio.Ambient;
|
||
```
|
||
|
||
**What retail actually stores.** `gmConfigUI::InitOptions @0x0049E435` is
|
||
`SetDefaultValue(1, 0x3f800000)`; the implementer read the `1` as
|
||
"Disabled = true". But `Sound_SoundDisabled` is only the **INI key string**
|
||
(`"Sound.SoundDisabled"`, `acclient_2013_pseudo_c.txt:718239`). The preference is
|
||
registered onto an **enabled-sense** variable
|
||
(`SoundManager::InitPrefs @0x005503F0`, `:346727`):
|
||
|
||
```c
|
||
UserPreferences::RegisterPreference(&SoundManager::effect_sounds_enabled, &Sound_SoundDisabled, …);
|
||
UserPreferences::RegisterPreference(&SoundManager::ambient_sounds_enabled, &Sound_AmbientSoundDisabled, …);
|
||
```
|
||
|
||
and every read is enabled-sense (`:346813`, `:346838`, `:347014`, `:347041`):
|
||
|
||
```c
|
||
if (SoundManager::effect_sounds_enabled != 0 && (s_bPlaySoundOnlyWhenActive == 0 || Device::m_bIsActiveApp != 0))
|
||
```
|
||
|
||
with `.data` statics `SoundManager::effect_sounds_enabled = 0x1`,
|
||
`ambient_sounds_enabled = 0x1`, `interface_sounds_enabled = 0x1`
|
||
(`:1102289`, `:1102294`, `:1102299`), and `SoundManager::Init` forcing
|
||
`effect_sounds_enabled = 0` only when `SoundOK()` fails (`:346769`). **Retail's
|
||
`SetDefaultValue(1)` means sound ENABLED.** The slice mapped it to
|
||
`Disabled = true`, then wired that to a hard zero.
|
||
|
||
*(Contrast `Sound_PlaySoundOnlyWhenActive`: positive-sense name, static `= 0x1`
|
||
at `:1102304`, and the implementer's `PlaySoundOnlyWhenActive = true` is
|
||
**correct** — which shows the three "Disabled" fields are the isolated error.)*
|
||
|
||
**Blast radius — this is not confined to the new tab.**
|
||
|
||
1. `AudioSettings.Default` (`AudioSettings.cs:72-75`) does not override the
|
||
trailing parameters, so `Default.SfxDisabled == Default.AmbientDisabled == true`.
|
||
2. `SettingsStore.LoadAudio` uses `d.<field>` as the missing-key fallback
|
||
(`SettingsStore.cs:123-124`), so **every existing `settings.json`** — which
|
||
has no `sfxDisabled` key — loads as disabled. Not just fresh profiles.
|
||
3. `RuntimeSettingsController.ApplyStartup` (`:250-263`) runs unconditionally in
|
||
the graphical host (`SettingsDevToolsComposition.cs:49`) and calls the same
|
||
static `ApplyAudio`, so the client comes up with `SfxVolume = 0` and
|
||
`AmbientVolume = 0` against engine defaults of `1f` / `0.8f`
|
||
(`OpenAlAudioEngine.cs:160-161`).
|
||
4. The per-frame draft-preview path (`WorldRenderFrameBuilder.cs:372`) shares the
|
||
same static helper, so the legacy Settings-panel audio sliders are muted too.
|
||
|
||
Net effect: **silent sound effects and silent ambient audio for all users on the
|
||
next launch**, undoing Campaign A's A2/A5 work.
|
||
|
||
**The defect is enshrined, not merely present.** A test asserts it
|
||
(`tests/AcDream.App.Tests/UI/Layout/ConfigOptionsPageControllerTests.cs:41-58`,
|
||
`Assert.True(d.SfxDisabled)` under a comment calling it byte-verified), the
|
||
register says so (AP-199: "gating the effective volume to zero when disabled"),
|
||
and the gate script tells the user this is "retail's own byte-verified 2013 EoR
|
||
default … **not a bug**, however counterintuitive"
|
||
(`2026-08-11-campaign-op-test-script.md:567-572`).
|
||
|
||
**Fix direction:** store retail's own enabled-sense value. Rename to
|
||
`SfxEnabled`/`AmbientEnabled`/`InterfaceEnabled` (default `true`), make
|
||
`ApplyAudio` `enabled ? volume : 0f`, and read the DAT caption for
|
||
`ID_Sound_DisableSound` at the gate to confirm which way the LED reads — the key
|
||
name is legacy; the *stored* semantic is unambiguous. `SettingsStore` must keep
|
||
reading the old `sfxDisabled` key only if it inverts it (or, cleaner, use fresh
|
||
key names since no shipped profile can contain the one-commit-old key).
|
||
|
||
---
|
||
|
||
## M3 — `UI_ChatFontFace` ships 1 of retail's 5 authored choices, and the row's default index falls outside it
|
||
|
||
`ConfigOptionsPageController.cs:551-554`:
|
||
|
||
```csharp
|
||
private static readonly string[] ChatFontFaceChoices = { "ID_UI_Value_Arial", };
|
||
```
|
||
|
||
justified at `:571-578` with "the face-choice array is authored with only ONE
|
||
literal decoded so far … the remaining installed system fonts are a per-machine
|
||
list retail enumerates at runtime (`UIPreferences::AttachPreference`'s own
|
||
SmartArray grow loop has no fixed upper bound in the decompile)".
|
||
|
||
That is contradicted by the decompile the slice cites. `gmClient::InitUIPreferences`
|
||
builds a fixed, compile-time five-entry array at `0x00403885`–`0x004039ED`
|
||
(`acclient_2013_pseudo_c.txt:2765-2802`): `ID_UI_Value_Arial`,
|
||
`ID_UI_Value_CourierNew`, `ID_UI_Value_PalatinoLinotype`, `ID_UI_Value_Tahoma`,
|
||
`ID_UI_Value_TimesNewRoman`, then one `SetEnumChoices`. All five strings are
|
||
present verbatim in the binary (`.rdata` string scan). The SmartArray growth is
|
||
just the container's normal push idiom — the same idiom used for `ChatFontSize`,
|
||
which the slice *did* transcribe as five entries.
|
||
|
||
Consequence: the menu offers one item, and the row's retail default `2`
|
||
(`:583`, matching `SetDefaultValue(2) @0x0049E7DE` = PalatinoLinotype) indexes
|
||
past a 1-entry array — a self-inflicted copy of the genuine
|
||
`LandscapeDrawDistance` oddity, which makes the real oddity indistinguishable
|
||
from this bug at the gate.
|
||
|
||
---
|
||
|
||
# SHOULD-FIX
|
||
|
||
## S1 — the trailing separator is missing (5 emitted, retail emits 6)
|
||
|
||
`gmConfigUI::InitOptions` ends `/* tailcall */ return PlayerOptionPage::AddSeperator(this);`
|
||
(`0x0049E80D`), for six `AddSeperator` calls: `0x0049E4A9`, `0x0049E58A`,
|
||
`0x0049E6A4`, `0x0049E750`, `0x0049E7C3`, `0x0049E80D`. `Bind`
|
||
(`ConfigOptionsPageController.cs:241-251`) emits five interior separators and no
|
||
trailing one, so the list is 38 items where retail builds 39. Lane A called this
|
||
out explicitly ("5 interior separators + 1 trailing",
|
||
`2026-08-10-options-panel-structure.md:487`). Visible as a missing bottom spacer.
|
||
No test pins the separator count.
|
||
|
||
*(Aside: lane A's "24 option rows" in that same line is wrong — the decomp has 27
|
||
option `Add*` calls, which is what the slice implements. The slice is right and
|
||
the doc is stale; worth correcting the doc while fixing the separator.)*
|
||
|
||
## S2 — Screen Brightness overloads `DisplaySettings.Gamma`, which already has a different unit system
|
||
|
||
`ConfigOptionsPageController.cs:410-416` binds `Render_ScreenBrightness`
|
||
(retail range `[-1, 1]`, default `0`, `AttachPreference @0x004043DF` +
|
||
`SetPreferenceRange @0x004043F3`) onto the pre-existing `Gamma` field. But
|
||
`Gamma` is a multiplier: `DisplaySettings.Default.Gamma == 1.0f`
|
||
(`DisplaySettings.cs:79`, pinned by `DisplaySettingsTests.cs:26`) and its live
|
||
control is `renderer.SliderFloat("Gamma", ref gamma, 0.5f, 2.0f)`
|
||
(`SettingsPanel.cs:220-222`, still present until OP9). Two incompatible unit
|
||
systems on one field:
|
||
|
||
- Fresh profile: `Gamma = 1.0` normalizes to `(1-(-1))/2 = 1.0` → the Config
|
||
Screen Brightness slider sits pinned at **maximum**, where retail sits centred.
|
||
- Clicking Defaults writes `Gamma = 0` — outside the legacy slider's `[0.5, 2.0]`
|
||
range and meaningless as a gamma multiplier.
|
||
- Moving the legacy slider to `2.0` makes the Config row read `1.5` normalized
|
||
→ clamped.
|
||
|
||
This also contradicts the slice's own stated principle: AP-200 refuses exactly
|
||
this kind of overload for `ChatSettings.FontSize` ("Inventing a … mapping without
|
||
retail evidence would risk silently overwriting … already-live … behaviour"),
|
||
while AP-198 does it for Gamma. A new `ScreenBrightness` field (defaulting to
|
||
retail's `0f`) matches every other OP6 row and costs one more `SettingsStore`
|
||
line.
|
||
|
||
## S3 — tooltips reach only 12 of 30 rows, though the doc claims the full table was applied uniformly
|
||
|
||
The class doc (`ConfigOptionsPageController.cs:33-36`) claims the complete
|
||
`AttachPreference` tooltip table (`<label>_Help`) was transcribed and "applied
|
||
uniformly". In fact only `ApplyLabelAndTooltip` (`:940-955`) sets a tooltip, and
|
||
it is called only from `BuildToggleRow` (`:653`) and `BuildTrioRow` (`:769`).
|
||
`SetLabelText` (`:957-968`), used by every slider and menu row, sets no tooltip.
|
||
So the 9 plain toggles + 3 trio toggles get tooltips and the 10 sliders + 8 menus
|
||
do not. Retail attaches a `_Help` string to **every** preference on this tab —
|
||
verified at all 27 `AttachPreference` sites.
|
||
|
||
Root cause is a capability gap, not just a missed call: `TooltipText` exists only
|
||
on `UiButton` (`UiButton.cs:96-100`); `UiScrollbar`/`UiMenu`/`UiText` have no
|
||
tooltip surface. Either add one (`GetTooltipText` override, the `UiElement.cs:480`
|
||
hook already exists) or correct the doc and file a register row. Do not leave the
|
||
doc overclaiming.
|
||
|
||
## S4 — Resolution's retail default `"800x600"` is not a selectable menu item
|
||
|
||
`BuildStringMenuRow` populates from `DisplaySettings.AvailableResolutions`
|
||
(`DisplaySettings.cs:85-93`: 1280x720 … 3840x2160) but passes
|
||
`defaultValue: "800x600"` (`ConfigOptionsPageController.cs:396`, correctly
|
||
decoded from `SetDefaultValue(0x3200258) @0x0049E5AC`). Clicking Defaults
|
||
therefore resizes the window to 800×600 (live, via `ApplyDisplayWindowState`) and
|
||
leaves the dropdown showing an entry that cannot be re-selected. The comment at
|
||
`:383-390` frames this as faithful, but retail's `Display_Resolution` menu is
|
||
populated from *enumerated display modes* — 800×600 is a real retail mode
|
||
(`gmClient::Init @0x004047AF` literally calls
|
||
`Device::ForceDisplayResolution(1, 0x320, 0x258)`). The divergence is acdream's
|
||
hardcoded modern preset list, not retail's default. Cheapest honest fix: add
|
||
`"800x600"` to `AvailableResolutions`.
|
||
|
||
## S5 — the tests pin the safe seams and miss every risky one
|
||
|
||
Genuinely good: per-record store round-trips (`:97-191`), the 30-row registration
|
||
count and full row-type sequence (`:270-327`), the scoped-scrollbar tests
|
||
(`:481-554`), the missing-ListBox and null-resolver degradation paths
|
||
(`:572-595`).
|
||
|
||
Missing exactly where the three MUST-FIXes live:
|
||
|
||
- **No test on `ApplyAudio`'s effective-volume mapping.** The one changed
|
||
pre-existing test (`RuntimeSettingsControllerTests.cs:300-304`) only adds
|
||
`"target-audio"` to an ordered event list — legitimate and honest, but it
|
||
asserts *that* the target was called, never *what* reached
|
||
`engine.SfxVolume`. A three-line test asserting a default-profile startup
|
||
leaves SFX audible would have caught M2.
|
||
- **No label/choice-key conformance table.** `LabelResolutionFailure…` (`:588`)
|
||
passes a null resolver, so every string key — including the 1-vs-5 font-face
|
||
array — is invisible to tests. The Character tab's registry-conformance pattern
|
||
(plan §4 OP4: "an invented row or a dropped row fails the build") is the
|
||
precedent to copy here.
|
||
- **No per-row default-value conformance.** `Defaults_Restores…` (`:442-469`)
|
||
only asserts self-consistency (`Current == DefaultValue`), never that
|
||
`DefaultValue` equals retail's literal. Row defaults are inline literals with
|
||
no pin.
|
||
- **No separator/list-item-count pin** (S1 would have failed one).
|
||
- `AudioSettings_Default_…` (`:41-58`) actively pins M2's inverted semantics.
|
||
|
||
---
|
||
|
||
# NOTE
|
||
|
||
- **N1 — AP-198 row count is internally inconsistent.** The register row
|
||
enumerates ten rows ("plus Screen Brightness"); the commit message says "the
|
||
nine Graphics/Rendering-Quality rows". Reconcile. (The `137 → 140` active-row
|
||
header count itself is **correct**: AP-198/199/200 are three new rows, and the
|
||
TS-74 extension is an edit, not a new row.)
|
||
- **N2 — the scrollbar id is hardcoded where the DAT supplies it.** The Config
|
||
ListBox node carries the authored `P0x72` value (`ScrollbarElementId: 268435969`
|
||
= `0x10000201` in `Fixtures/options_2100002B.json`), but the controller
|
||
hardcodes the constant (`:140`). It matches today; reading the authored field
|
||
is the standing project preference (`feedback_prefer_dat_field_over_geometry`).
|
||
- **N3 — `Defaults()` performs one full `settings.json` write per row.** Each
|
||
row's `apply` is `Save<Section>(Load<Section>() with { … })`, so a Defaults
|
||
click issues up to 30 sequential load-modify-save round-trips (plus 30
|
||
`ApplyAudio`/`ApplyDisplayWindowState` pushes, including repeated window
|
||
resizes). Correctness is fine; it is worth a batched-write follow-up before
|
||
OP9. Retail's preference system writes the INI on its own cadence, not per row.
|
||
- **N4 — `AfterApply` fires on Config-tab entry.** `OptionsPanelController.cs:150-154`
|
||
gives the Config page the same `SaveCharacterOptionsRuntimeCmd` publisher as
|
||
Character/Chat. This is retail-shaped — `gmConfigUI : PlayerOptionPage`, and
|
||
`PlayerOptionPage::SaveCurrentValues @0x004F2710` does call
|
||
`CPlayerModule::SaveToServer(module, 0)` — and acdream's flush is dirty-gated
|
||
("a clean module sends nothing", `RuntimeCharacterState.cs:930`), so this is
|
||
correct, not a finding. Recorded so a later reviewer does not re-open it.
|
||
|
||
---
|
||
|
||
# Verified correct (spot-checked against primary sources, no action needed)
|
||
|
||
These were checked end-to-end and are right; they are the reason this REJECT is
|
||
narrow rather than a rework.
|
||
|
||
**Template mechanism — all eight indices verified against both the decomp and
|
||
the committed fixture** (`Fixtures/options_2100002B.json`, ListBox `0x10000200`
|
||
`TemplateList`):
|
||
|
||
| idx | Fixture element | Retail call | Slice constant |
|
||
|---|---|---|---|
|
||
| 0 | `0x10000216` (Type 12) | `AddHeader` → `AddItemFromTemplateList(…, 0)` `@0x004F28C5` | `HeaderTemplateIndex` ✓ |
|
||
| 1 | `0x10000217` | `AddSeperator` → `(…, 1)` `@0x004F273E` | `SeparatorTemplateIndex` ✓ |
|
||
| 2 | `0x10000218` + `0x10000219` | `AddToggleOption` → `(…, 2)`, child `0x10000219`, cast `0x10000035` `@0x004F29A3` | `ToggleTemplateIndex` / `ToggleCheckboxElementId` ✓ |
|
||
| 3 | `0x1000021A` + `0x1000021B`/`0x1000021C` | `AddSliderOption(arg3=0)` → `(…, 3)` | `SimpleSliderTemplateIndex` ✓ |
|
||
| 4 | `0x10000222` + `0x10000223`/`0x10000224` | `AddMenuOption` → `(…, 4)`, child `0x10000224`, cast `0x10000038` `@0x004F2B03` | `MenuTemplateIndex` ✓ |
|
||
| 5 | `0x10000220` (Type `0x10000036`) | `AddToggleWithSliderOption` → `(…, 5)` `@0x004F2943` | `TrioTemplateIndex` ✓ |
|
||
| 6 | `0x1000021D` (+ caption children) | `AddSliderOption(arg3=1)` → `(…, 6)` | `RangedSliderTemplateIndex` ✓ |
|
||
| 7 | `0x10000221` | never invoked by `InitOptions` | correctly unused ✓ |
|
||
|
||
- **The `AddSliderOption` arg3 → template decode is a correct BN-artifact read.**
|
||
BN renders `AddItemFromTemplateList(m_pOptionBox, (((eax - eax) & 3) + 3))`
|
||
(`@0x004F2A90`) — a mangled `neg/sbb/and 3/add 3`, i.e. `arg3 != 0 → 6`,
|
||
`arg3 == 0 → 3`. The slice's 3-vs-6 split, and its "idx3 is used exactly once"
|
||
structural cross-check, are both right.
|
||
- **`AddMenuOption`'s `arg3` does NOT change the template** (always 4); it selects
|
||
`SetUIPreference` vs `SetUserPreference` (`@0x004F2B3C`/`@0x004F2B67`). The
|
||
slice's decision to use one template with two payload types (int / string for
|
||
Resolution) matches.
|
||
- **Every slider range matches `UIPreferences::SetPreferenceRange` exactly:**
|
||
volumes `[0,1]`; `Camera_Stiffness [0.285714298, 1]` `@0x0040465B`;
|
||
`Camera_AdjustmentSpeed [5, 80]` `@0x0040469C`; `Render_FieldOfView [10, 160]`
|
||
`@0x004043B2`; `Render_ScreenBrightness [-1, 1]` `@0x004043F3`;
|
||
`Render_GraphicsPerformance [-1, 1]` `@0x00404461`;
|
||
`Render_DegradeDistance [0, 100]` `@0x0040449E`;
|
||
`Input_MouseLookSensitivity [0.00999999978, 1]` `@0x00404593`.
|
||
- **Every label key is right, including all six the pseudo-C truncates.**
|
||
Verified by scanning the binary's `ID_*` strings: `…TextureFiltering_Sharp`,
|
||
`…BuildingDetailTextures`, `…LandscapeTextureDetail`,
|
||
`…EnvironmentTextureDetail`, `…LandscapeDrawDistance`,
|
||
`ID_Rendering_SyncToDisplayRefresh`. The `_Help` tooltip-suffix convention is
|
||
correct at all 27 `AttachPreference` sites (cf. Chat's `_Desc`).
|
||
- **Every numeric default matches**, incl. `0x3EE66666 = 0.45`,
|
||
`0x42200000 = 40`, `0x42B40000 = 90`, `0x42480000 = 50`, `0x3F0CCCCD = 0.55`,
|
||
`0x03200258 = 800x600`, and the four menu immediates
|
||
(`6a 02 / 6a 01 / 6a 01 / 6a 08` at `0x0049E6C6`…`0x0049E714`).
|
||
- **The `LandscapeDrawDistance` oddity is real and reproduced faithfully.**
|
||
`SetDefaultValue(8)` (`6a 08 @0x0049E714`) against a 6-entry
|
||
`SetEnumChoices` (VeryLow…Extreme, `@0x00404371`). Stored as an opaque `int`;
|
||
`UiMenu` handles the out-of-range value safely (`Equals(Payload, 8)` matches
|
||
nothing → no highlight, and `ButtonLabelProvider` bounds-checks at
|
||
`:853-857`). AP-198's sub-note is accurate. **This is the one place where the
|
||
"reproduce, don't fix" instinct was applied correctly** — M1/M3 are the same
|
||
instinct applied to non-facts.
|
||
- **Row order matches `InitOptions` statement-for-statement** across all six
|
||
sections (only the trailing separator, S1, differs).
|
||
- **The scoped-lookup fix is correct and its test genuinely discriminates.**
|
||
Scoping from `PageSlotElementId = 0x10000213` (`:224-227`) is exactly what OP5's
|
||
review N11 demanded, and `SharedScrollbarId_ChatAndConfigBoundTogether_…`
|
||
(`:508-554`) asserts through *scoped* lookups on both pages plus
|
||
`NotSame(chatListBox.Scroll, configListBox.Scroll)` — under a flat lookup
|
||
Config's `Bind` would overwrite the single shared instance's `Model` and the
|
||
Chat assertion fails. This is a strictly better test than the OP5 one N11
|
||
criticised.
|
||
- **`IntOptionRow`/`StringOptionRow` match the `Bool`/`Float` contract exactly**
|
||
(`OptionPageModel.cs:282-434`): same `(current, saved, default)` triple, same
|
||
`SetCurrentValue → apply + notify` / `SaveCurrentValue → read + refresh` /
|
||
`RestoreSaved` / `RestoreDefault` split, and `AttachPageNotify` so both
|
||
participate in Apply/Reset ghosting (`OptionsPanelController.cs:229-243`). Every
|
||
Config row supplies `read`/`refresh`, so the OP4 re-seed (`OnShown → Apply`) and
|
||
the R2 `ReloadFromLive` path both work. `StringOptionRow.Changed` uses C# string
|
||
value equality — correct.
|
||
- **The `ApplyAudio` seam reaches the real engine and does not double-apply at
|
||
startup.** `RuntimeSettingsTargets._audio` ← `content.Audio?.Engine`
|
||
(`SessionPlayerComposition.cs:347`) is the same `OpenAlAudioEngine` instance as
|
||
the startup targets (`:364`) and the frame preview
|
||
(`FrameRootComposition.cs:355`). `ApplyStartup` runs once via the startup
|
||
target; `SaveAudio` only fires on an actual save; `Bind` only reads. The
|
||
optional-trailing ctor parameter keeps every pre-existing construction site
|
||
compiling. *(The mechanism is right — only the value it pushes is wrong, per
|
||
M2.)*
|
||
- **Interface addition blast is contained.** `IRuntimeSettingsTargets` has exactly
|
||
one production implementor (`RuntimeSettingsTargets`, `internal sealed`) and one
|
||
test fake, both updated; the startup interface's separate fake is untouched.
|
||
- **Store schema is additive-only with correct fallbacks.** All new keys go
|
||
through `ReadBool`/`ReadFloat`/the new `ReadInt` with the record default as
|
||
fallback (`SettingsStore.cs:73-81, 121-126, 217-218, 255`), writers extend the
|
||
existing sorted dictionaries, no version field, no removals — an existing
|
||
`settings.json` parses unchanged. *(The only behavioural break is M2's default,
|
||
not the schema.)*
|
||
- **No invented user-visible text.** Every caption resolves from string table
|
||
`0x23000003` by `compute_str_hash` name; failures log and leave the widget
|
||
blank (`:607-615`, `:840-848`, `:943-955`, `:957-968`), matching the campaign's
|
||
§6 rule and the Character/Chat controllers' convention.
|
||
- **TS-74's extension is accurate and correctly scoped** (the sixth,
|
||
previously-unmodeled `Input_UseMouseTurning` client-local preference, correctly
|
||
distinguished from the server-synced `PlayerOption.UseMouseTurning` bit), and
|
||
the **AP-174 citation on the Interface trio is correct** — retail registers
|
||
`Sound_InterfaceSoundVolume`/`…Disabled` (`@0x004036E0`/`@0x004036B3`) and
|
||
never reads them; AP-174 already documents "interface sounds are scaled by the
|
||
EFFECT knob". Matching retail's dead knob rather than building a working one is
|
||
the right call.
|
||
|
||
---
|
||
|
||
# Fix checklist
|
||
|
||
| # | Severity | Item |
|
||
|---|---|---|
|
||
| M1 | MUST | Wire the six range-caption pairs (Soft/Hard, Slow/Fast, Narrow/Wide, Dark/Bright, Speed/Detail, Close/Far) onto idx6's `0x1000021E`/`0x1000021F`; correct the class doc, ledger/commit claim, and gate-script lines 535 + 653 |
|
||
| M2 | MUST | Invert the three Sound toggles to retail's enabled-sense (`effect_sounds_enabled`); fix `AudioSettings` defaults, `ApplyAudio`, the store keys, the pinning test, AP-199's wording, and gate-script step 6 |
|
||
| M3 | MUST | Ship all five `UI_ChatFontFace` choices; delete the "runtime-enumerated" justification |
|
||
| S1 | SHOULD | Emit the trailing separator (6 total); pin the count; correct lane A's "24 option rows" |
|
||
| S2 | SHOULD | Give Screen Brightness its own field instead of overloading `Gamma` |
|
||
| S3 | SHOULD | Tooltips on slider/menu rows, or correct the doc + file a register row |
|
||
| S4 | SHOULD | Add `"800x600"` to `AvailableResolutions` so Defaults lands on a selectable item |
|
||
| S5 | SHOULD | Add: effective-engine-volume test, label/choice-key conformance table, per-row retail-default pin, separator-count pin |
|
||
| N1–N4 | NOTE | AP-198 nine-vs-ten; read the authored scrollbar id; batch the Defaults writes; (N4 needs no action) |
|