# 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.` 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 (`