From e318e8628d73164652ba09f7a90a245a531b2f87 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 07:35:32 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20OP6=20review=20=E2=80=94=20REJECT=20(by?= =?UTF-8?q?te-decoded):=20zero-fold=20captions,=20inverted=20audio=20toggl?= =?UTF-8?q?es,=20one-of-five=20fonts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/research/2026-08-11-op6-review.md | 486 +++++++++++++++++++++++++ 1 file changed, 486 insertions(+) create mode 100644 docs/research/2026-08-11-op6-review.md diff --git a/docs/research/2026-08-11-op6-review.md b/docs/research/2026-08-11-op6-review.md new file mode 100644 index 00000000..1a3cb75a --- /dev/null +++ b/docs/research/2026-08-11-op6-review.md @@ -0,0 +1,486 @@ +# 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 (`