diff --git a/docs/plans/2026-08-10-options-panel-campaign.md b/docs/plans/2026-08-10-options-panel-campaign.md index 9ee32e8b..b6497a83 100644 --- a/docs/plans/2026-08-10-options-panel-campaign.md +++ b/docs/plans/2026-08-10-options-panel-campaign.md @@ -422,7 +422,7 @@ before anything builds on them. | OP3 | CODE-COMPLETE, gate READY | `9d26ecc6` → fixes `386076af` → residuals `cb334690` | dual APPROVE-WITH-FIXES (`2026-08-11-op3-review-{mechanism,blast}.md`) → re-review REOPEN-narrow (`2026-08-11-op3-rereview.md`) → coordinator residuals landed | connected gate OWED (script §OP3) | | OP4 | CODE-COMPLETE, gate READY | `22b86b9f` → fixes `bc43fb1d` → residuals `ac0304dc` | dual APPROVE-WITH-FIXES (`2026-08-11-op4-review-{mechanism,blast}.md`) → re-review REOPEN-narrow (`2026-08-11-op4-rereview.md`) → coordinator residuals landed | connected gate OWED (script §OP4) | | OP5 | CODE-COMPLETE, gate READY | `e71e5a96` (AP-195 retired) → fixes `6d0b0f92` → residuals `67b0815c` | combined APPROVE-WITH-FIXES (`2026-08-11-op5-review.md`) → re-check CLOSED (`2026-08-11-op5-recheck.md`) → coordinator drag residuals landed | connected gate OWED (script §OP5) | -| OP6 | rework landed, re-review pending | `f5ac1742` (REJECTED) → rework `472525b9` | REJECT (`2026-08-11-op6-review.md`) → focused re-review in flight | | +| OP6 | CODE-COMPLETE, gate READY | `f5ac1742` (REJECTED) → rework `472525b9` → doc residuals (coordinator) | REJECT (`2026-08-11-op6-review.md`) → re-review CLOSED, all six caption sites byte-decoded (`2026-08-11-op6-rereview.md`) | connected gate OWED (script §OP6) | | OP7 | CLOSED | `09cb548a` → fixes in `7b60e71b` (shared commit, see its message) | combined-lens APPROVE-WITH-FIXES (`2026-08-11-op7-review.md`); all nine findings closed | live bot-vs-ACE gate PASSED 2026-08-11 (coordinator; evidence in script §OP7) | | OP8 | — | | | | | OP9 | — | | | | diff --git a/docs/research/2026-08-10-options-panel-structure.md b/docs/research/2026-08-10-options-panel-structure.md index 002dec78..4d3d46b3 100644 --- a/docs/research/2026-08-10-options-panel-structure.md +++ b/docs/research/2026-08-10-options-panel-structure.md @@ -484,7 +484,7 @@ Chat tab's `SetUserData` argument *is* the retail window id. ## 4. The **Config** tab — complete row map (`gmConfigUI::InitOptions @0x0049E400`) -Six sections, **6 headers / 5 interior separators + 1 trailing / 24 option rows**. Bindings +Six sections, **6 headers / 5 interior separators + 1 trailing / 27 option rows** (corrected 2026-08-11 at the OP6 re-review — the original 24 was a miscount; the decomp has 27 `Add*Option` calls and the OP6 slice's 39-item count pin (6+6+27) is the authoritative tally). Bindings are `PStringBase` preference-key globals whose names are `Section_Key`; they map one-to-one onto `UserPreferences.ini` sections (verified against the user's live retail `UserPreferences.ini`). All headers byte-verified from `mov eax,[global]; push eax` operands. diff --git a/docs/research/2026-08-11-campaign-op-test-script.md b/docs/research/2026-08-11-campaign-op-test-script.md index b19ac5ce..5b751ef7 100644 --- a/docs/research/2026-08-11-campaign-op-test-script.md +++ b/docs/research/2026-08-11-campaign-op-test-script.md @@ -567,13 +567,16 @@ should show no caption. 4. **CORRECTED at the OP6 rework round (2026-08-11, review M2) — the checkbox's stored value is ENABLED-sense, not "Disabled" as the rejected slice's own gate script previously described.** With the - "Disable Sound Effects" checkbox CHECKED (its default state — see step + first Sound-trio checkbox CHECKED (its default state — see step 6), sound effects are AUDIBLE. **Uncheck it** — sound effects go SILENT immediately, regardless of where the slider is set. Check it again — - sound effects resume at the slider's current level. (The checkbox's own - retail LABEL reads as if checked should mean muted; byte evidence says - the opposite — see `AudioSettings`' class doc for the full citation. - This gate is about the OBSERVED behaviour, not the label's English.) + sound effects resume at the slider's current level. **Also report the + checkbox's DAT-resolved caption VERBATIM** (OP6 re-review SF-2: the + authored `ID_Sound_DisableSound` string was never read during research, + so this script does not know retail's exact English — if the caption + reads as a "Disable ..." phrasing while CHECKED means audible, that + label-vs-behavior tension is retail's own, byte-verified; report the + text so the script can record it, and gate on the BEHAVIOUR). 5. **Repeat step 4 for the "Ambient Sound" trio** (the second row) using an ambient loop (wind, water, torches) as your audible test — same live drag + mute-on-uncheck behaviour. diff --git a/docs/research/2026-08-11-op6-rereview.md b/docs/research/2026-08-11-op6-rereview.md new file mode 100644 index 00000000..203f40f3 --- /dev/null +++ b/docs/research/2026-08-11-op6-rereview.md @@ -0,0 +1,479 @@ +# OP6 rework — focused re-review + +**Commit under re-review:** `472525b9` "fix(ui): OP6 rework — six range +captions, un-invert Sound enabled flags, five font faces" +**Closes:** `docs/research/2026-08-11-op6-review.md` (REJECT — M1/M2/M3, +S1–S5, N1–N4) +**Reviewer:** focused re-reviewer, Opus, 2026-08-11. Read-only, no +subagents, no build/test/launch. + +## Verdict: **CLOSED** + +All three MUST-FIX findings, all five SHOULD-FIXes, and both actionable +NOTEs are closed. **Every one of the three MUST-FIX closures was +independently re-derived from primary sources in this session, not +accepted from the review's own dumps** — including a fresh PE byte-decode +of all six `SetSliderLabel` call sites (the review decoded them; the +implementer byte-read only two and pattern-matched four; this pass decoded +all six plus their push order, and added a third, independent line of +evidence the earlier two rounds did not have). + +Residuals are two documentation-truthfulness items (SHOULD-FIX) and six +NOTEs. None is a behaviour defect; none blocks the connected gate. + +--- + +## Evidence discipline for this pass + +Byte quotes below were decoded in this session from +`C:\Users\erikn\Downloads\acclient.exe` (4,837,376 bytes), image base +`0x400000`, `.text` VA→file offset = `VA - 0x400000`. Symbol names came +from `docs/research/named-retail/acclient_2013_pseudo_c.txt`. Layout facts +came from the committed fixture +`tests/AcDream.App.Tests/UI/Layout/fixtures/options_2100002B.json`. + +--- + +# Per-finding closure + +## M1 — six range-caption pairs — **CLOSED (independently re-verified, no transposition)** + +`ConfigOptionsPageController.BuildSliderRow` gained +`rangeLowKey`/`rangeHighKey` (`:777-778`), applied at `:792-795` through +`SetRangeLabel` (`:834-848`) onto `SliderRangeMinElementId = 0x1000021E` / +`SliderRangeMaxElementId = 0x1000021F` (`:187-188`). + +**Full six-site byte decode (this session, all six — not two):** + +| Slider | pref pushed | `SetSliderLabel` VA | arg3 (low) | arg4 (high) | +|---|---|---|---|---| +| `Camera_Stiffness` | `0x0083E5A0` | `0x0049E4E0` | `[0x0083E790]` | `[0x0083E794]` | +| `Camera_AdjustmentSpeed` | `0x0083E5A4` | `0x0049E51D` | `[0x0083E788]` | `[0x0083E78C]` | +| `Render_FieldOfView` | `0x0083E5F4` | `0x0049E556` | `[0x0083E780]` | `[0x0083E784]` | +| `Render_ScreenBrightness` | `0x0083E608` | `0x0049E614` | `[0x0083E768]` | `[0x0083E76C]` | +| `Render_GraphicsPerformance` | `0x0083E61C` | `0x0049E65D` | `[0x0083E770]` | `[0x0083E774]` | +| `Render_DegradeDistance` | `0x0083E620` | `0x0049E68E` | `[0x0083E778]` | `[0x0083E77C]` | + +Arg order is recovered from the push sequence, not assumed. Two encodings +appear and both resolve the same way. Stiffness (`0x0049E4C9`): +`8b 0d 94 e7 83 00` / `8b 15 90 e7 83 00` / `51 52 ... 50` — pushed +ecx, edx, eax, so eax (slot) is the last push = first stack arg, edx = +arg3, ecx = arg4. Screen Brightness (`0x0049E602`): +`8b 0d 68 e7 83 00` / `8b f8` / `a1 6c e7 83 00` / `50 51 57` — pushed +eax, ecx, edi, so edi (slot) = first arg, ecx = arg3, eax = arg4. Both +give **arg3 = the numerically lower global = the low caption**. + +PDB names confirmed at `acclient_2013_pseudo_c.txt:1148623-1148634` +(`0x0083E768` `ID_Graphics_Value_Dark` … `0x0083E794` +`ID_Graphics_Value_Hard`), and the same names are built by +`compute_str_hash` at `:769790-769878` with the literals present in +`.rdata` at `:975858-975896`. + +**Pairing in the shipped code is correct, with zero transposition:** + +| Row | `rangeLowKey` | `rangeHighKey` | file:line | +|---|---|---|---| +| Camera Stiffness | `Soft` | `Hard` | `ConfigOptionsPageController.cs:396` | +| Camera Adjustment Speed | `Slow` | `Fast` | `:403` | +| Field Of View | `Narrow` | `Wide` | `:414` | +| Screen Brightness | `Dark` | `Bright` | `:482` | +| Graphics Performance | `Speed` | `Detail` | `:494` | +| Degrade Distance | `Close` | `Far` | `:501` | + +**Third, independent evidence line the earlier rounds did not have.** The +low/high → element assignment could still have been transposed even with +the right pairs. The fixture settles it geometrically: in template +`0x1000021D`, child `0x1000021E` is at `X=146, W=60, HJustify=0` +(left-aligned, left half of the slider track at `X=146, W=120`) and +`0x1000021F` is at `X=206, W=60, HJustify=2` (right-aligned, right half). +Low = left = `0x1000021E`. This matches OP5's already-shipped Chat +precedent (`ChatOptionsPageController.cs:343-344`: arg3 `Transparent` → +Min, arg4 `Opaque` → Max). + +**Assessment of the implementer's 2-of-6 chain: sound, and now moot.** The +four pattern-matched sites are individually byte-confirmed above. The +chain was safe to begin with because the six globals are a contiguous, +PDB-named `.data` block in a fixed declaration order and each site reads +an adjacent `(n, n+4)` pair — but "safe reasoning" is now replaced by +measurement for all six. + +**Keys resolve by name, no hard-coded English.** `SetRangeLabel` goes +through `resolveString(StringTableId, DatStringResolver.ComputeHash(key))` +and logs + leaves the widget blank on failure (`:839-846`) — the same rule +as every other string on the page. + +**Doc + gate corrected.** Class doc U4 note rewritten (`:46-73`), gate +script §OP6 intro (`2026-08-11-campaign-op-test-script.md:531-547`) and +"What to report" (`:670-675`) both now name the six pairs and the one +genuine exception. Mouse Look Sensitivity correctly stays uncaptioned +(`ConfigOptionsPageController.cs:594-598`, no range keys) — confirmed +against `AddSliderOption(this, &Input_MouseLookSensitivity, 0)` at +`0x0049E76B` with no following `SetSliderLabel`. + +**The `SetSliderLabel` call count is exactly 6**, verified in the decomp +(`acclient_2013_pseudo_c.txt:169415, 169419, 169422, 169435, 169439`, plus +`0x0049E68E`) — six labelled sliders, seven sliders total. + +--- + +## M2 — Sound toggles un-inverted — **CLOSED (chain re-verified end to end)** + +`AudioSettings.SfxEnabled`/`AmbientEnabled`/`InterfaceEnabled`, all +defaulting `true` (`AudioSettings.cs:90-95`). + +**Retail chain, re-verified independently:** + +- `UserPreferences::RegisterPreference(&SoundManager::effect_sounds_enabled, &Sound_SoundDisabled, …)` + at `:346727`; siblings at `:346734` / `:346741`. The backing variable is + enabled-sense; the preference id's *name* is the legacy INI key + ("Sound Disabled"). +- `.data` statics all compiled `= 0x1`: `:1102289` / `:1102294` / + `:1102299`. +- Every consumer read is enabled-sense: `:346813`, `:346838`, `:346863`, + `:346896`, `:346930`, `:346957`, `:347014`, `:347041`, `:347816`, + `:347960`, `:348116`. +- **The one place an inversion could still have hidden — the checkbox + widget — was checked and is clean.** `UIOption_Checkbox::Refresh` + (`0x004868A0`) is `SetAttribute_Bool(0xe, this->m_current)`; + `SetCurrentValue` (`0x00486970`) is `SetAttribute_Bool(0xe, arg2)`; and + the UIPreference branch of `GetValue` (`0x00486F60`) returns + `UIPreferences::InqPreferenceValue`'s out-param directly + (`0x00487027`). No negation anywhere. Checked = stored 1 = audible. +- `SetDefaultValue(1, 0x3f800000)` on all three trios, byte-verified at + `0x0049E435` / `0x0049E457` / `0x0049E479` (`68 00 00 80 3f 6a 01`). + +**Truth table.** `ComputeEffectiveCategoryVolumes` +(`RuntimeSettingsTargets.cs:161-167`) is `enabled ? slider : 0f` +per-category. Pinned by +`RuntimeSettingsControllerTests.cs:325-350` (Theory: `(T,T)` +pass-through, `(F,F)` → `0f/0f`, `(T,F)` → independent gating) and +`:352-364` (default profile → `1.0f/1.0f`). + +**`ApplyAudio` pushes the right values.** `RuntimeSettingsTargets.cs:131-142`: +`engine.MasterVolume = audio.Master`, then `engine.SfxVolume = sfx` / +`engine.AmbientVolume = ambient` from the helper. All three live call +sites now share it: startup (`RuntimeSettingsController.cs:263`), save +(`:534` → `RuntimeSettingsTargets.cs:334-335`), and the per-frame draft +preview (`WorldRenderFrameBuilder.cs:372`). + +**Existing settings.json loads with sound ON.** `SettingsStore.cs:131-133` +reads the three fresh keys with `d.` (= `true`) as the +missing-key fallback. Pinned end-to-end by +`ConfigOptionsPageControllerTests.cs:134-172`, which writes a real +legacy-shaped file containing only a `display` section, loads it, and +asserts both the record flags AND that +`ComputeEffectiveCategoryVolumes` returns `1.0f/1.0f`. + +**No old-key remnants.** A repo-wide grep for +`SfxDisabled|AmbientDisabled|InterfaceDisabled|sfxDisabled|ambientDisabled|interfaceDisabled` +returns three hits, all inside explanatory comments +(`AudioSettings.cs:88`, `SettingsStore.cs:125-126`). No reader, no +writer, no test. `WriteAudio` (`SettingsStore.cs:680-690`) emits only the +new keys. + +**AP-199 and gate step 6 are now truthful.** +`retail-divergence-register.md:192` opens with the explicit correction and +states "a fresh profile is audible, not muted"; gate step 6 +(`2026-08-11-campaign-op-test-script.md:578-587`) now says sound starts ON +and that silence on a fresh character IS a defect to report. §"What to +report" adds the same as an explicit reportable (`:678-680`). + +--- + +## M3 — five font faces — **CLOSED** + +`ChatFontFaceChoices` (`ConfigOptionsPageController.cs:636-640`) ships +Arial, CourierNew, PalatinoLinotype, Tahoma, TimesNewRoman — matching +`gmClient::InitUIPreferences`'s fixed five-`compute_str_hash` array at +`0x00403891`, `0x004038C8`, `0x00403910`, `0x00403958`, `0x004039A0` +(`acclient_2013_pseudo_c.txt:2798, 2806, 2820, 2834, 2848`), terminated by +one `SetEnumChoices(&UI_ChatFontFace, …)` at `0x004039ED` (`:2860`). All +five literals present in `.rdata` (`:948252-948268`). + +Default index 2 = PalatinoLinotype, in range. Byte-verified: `6a 02` at +`0x0049E7E4` inside the `AddMenuOption(&UI_ChatFontFace, 1)` at +`0x0049E7D4`. Wired at `ConfigOptionsPageController.cs:665` and pinned by +`ConfigOptionsPageControllerTests.cs:606` (row 28 → `2`). + +The wrong "per-machine runtime enumeration / SmartArray has no fixed upper +bound" comment is gone, replaced by the corrected doc at `:619-635` which +names the same `SmartArray::grow` push idiom `ChatFontSizeChoices` uses. + +--- + +## S1 — trailing separator — **CLOSED** + +`Bind` emits a sixth `BuildSeparatorRow` after the UI section +(`ConfigOptionsPageController.cs:288-294`). + +Byte-verified six `AddSeperator` calls, all targeting `0x004F2730`: +`0x0049E4A9`, `0x0049E58A`, `0x0049E6A4`, `0x0049E750`, `0x0049E7C3`, and +the tailcall `e9 1e 3f 05 00` at `0x0049E80D`. Six `AddHeader` calls +(`0x004F28B0`) at `0x0049E40A`, `0x0049E4B6`, `0x0049E597`, `0x0049E6B1`, +`0x0049E75D`, `0x0049E7D0`, reading the six header globals +`0x0083E750`–`0x0083E764`. Twenty-seven option `Add*` calls, counted +site-by-site in the same dump: Sound 5, Camera 4, Graphics 7, Rendering +Quality 6, Input 3, UI 2. **6 + 6 + 27 = 39.** + +Pinned by +`ConfigOptionsPageControllerTests.cs:381-413` (`Assert.Equal(39, viewport.Children.Count)`). + +Lane A's stale "24 option rows" line was not corrected in +`2026-08-10-options-panel-structure.md` — the review flagged that as an +aside, not a required fix, and the doc's separator count ("5 interior + 1 +trailing") was already right. Recorded as NOTE **N-g** below. + +--- + +## S2 — Screen Brightness gets its own field — **CLOSED** + +`DisplaySettings.ScreenBrightness = 0f` (`DisplaySettings.cs:65`), range +`[-1,1]` at the build site (`ConfigOptionsPageController.cs:477-482`), +matching `SetPreferenceRange(&Render_ScreenBrightness, -1f, 1f)` +(`0x004043F3`) and the byte-verified `SetDefaultValue` immediate `6a 00` +at `0x0049E61A`. + +**`Gamma` is untouched.** Still `float Gamma` at `DisplaySettings.cs:44`, +`Default.Gamma = 1.0f` at `:91`, its existing pin +`DisplaySettingsTests.cs:26` (`Assert.Equal(1.0f, d.Gamma)`) unmodified, +and its legacy live control `renderer.SliderFloat("Gamma", ref gamma, +0.5f, 2.0f)` unmodified at `SettingsPanel.cs:219-221`. No Config row +writes `Gamma`. + +Store: `SettingsStore.cs:74` (read, fallback `d.ScreenBrightness`) and +`:662` (write). Additive only — an existing `settings.json` without the +key loads `0f`. Round-trip pinned at +`ConfigOptionsPageControllerTests.cs:174-204`; default pinned at `:83` +and at the per-row table `:590`. + +--- + +## S3 — tooltips on sliders and menus — **CLOSED** + +`UiScrollbar.TooltipText` + `GetTooltipText` override +(`UiScrollbar.cs:88-104`) and `UiMenu.TooltipText` + override +(`UiMenu.cs:37-48`), both following `UiButton.cs:92-100` and the existing +`UiElement.cs:480` virtual hook. + +Coverage is now complete for all 30 registered rows: +- 9 plain toggles + 3 trio toggles → `ApplyLabelAndTooltip` + (`:735`, `:894`, `:1081-1095`). +- 7 sliders → `slider.TooltipText` (`:805-809`). +- 3 trio slider halves → `slider.TooltipText` from an explicit + `sliderTooltipKey` (`:911-913`). +- 7 int menus + Resolution → `menu.TooltipText` (`:965-969`, `:1047-1051`). + +**`_Help` suffix convention spot-checked at 3+ rows against +`gmClient::InitUIPreferences`** (and, in fact, at every site in the +function): +`ID_Camera_Stiffness_Help` @ `0x00404624` → `AttachPreference(&Camera_Stiffness, …)` @ `0x00404647`; +`ID_Graphics_ScreenBrightness_Help` @ `0x004043BC` → `@0x004043DF`; +`ID_UI_ChatFontFace_Help` @ `0x00403858` → `@0x0040387B`. +The trio slider halves' keys are also right: +`ID_Sound_EffectVolume_Help` @ `0x004035E7` (pref `Sound_SoundVolume`), +`ID_Sound_AmbientVolume_Help` @ `0x00403653`, +`ID_Sound_InterfaceVolume_Help` @ `0x004036BD` — exactly the three +`sliderTooltipKey` values at `ConfigOptionsPageController.cs:335, 345, 359`. +No `AttachPreference` site in this function deviates from the suffix. + +The class doc's "applied uniformly" claim (`:33-42`) is now TRUE. The +narrower scope it states — "every toggle/trio/slider/menu row's **own +interactive widget**", not the sibling name text — is retail-shaped; see +NOTE **N-e**. + +--- + +## S4 — `800x600` selectable — **CLOSED** + +`DisplaySettings.AvailableResolutions` now leads with `"800x600"` +(`DisplaySettings.cs:113`). Row default `"800x600"` at +`ConfigOptionsPageController.cs:458`, byte-verified `68 58 02 20 03` +(`0x03200258`) at `0x0049E5AC`. + +**Blast checked and clean.** The only other consumer, +`SettingsPanel.cs:200-206`, resolves by VALUE (`Array.IndexOf`) and writes +back the string, never a persisted index — so prepending an entry shifts +no stored data. `SettingsPanelTests.cs:334-345` asserts membership only. +`DisplaySettingsTests.cs:31-45` asserts ascending width; `800 ≤ 1280` +holds. See NOTE **N-b** for the now-stale test name. + +--- + +## S5 — the four missing tests — **CLOSED** + +1. **Effective-engine-volume mapping** — + `RuntimeSettingsControllerTests.cs:325-364`, Theory + default-audible + Fact. Closes the exact gap M2 slipped through. +2. **Label/choice-key conformance table** — + `ConfigOptionsPageControllerTests.cs:627-733`. **Spot-checked: the + table is 97 keys with zero duplicates, and I traced every one of the + controller's query sites by hand against it — no invented key, no + dropped key.** Ten verified against the decomp: + `ID_Sound_SoundFeatures`(+`_Help`) `@0x0040371D`/`@0x004036FA`; + `ID_Sound_DisableSound`(+`_Help`) `@0x004035DD`/`@0x004035BA`; + `ID_Sound_NoFocusNoSound`(+`_Help`) `@0x004037E4`/`@0x004037C1`; + `ID_Camera_AdjustmentSpeed`(+`_Help`) `@0x00404688`; + `ID_Camera_AlignToSlope`(+`_Help`) `@0x0040461A`; + `ID_Graphics_AdaptiveDegradeBias`(+`_Help`) `@0x0040444D`; + `ID_Rendering_SyncToDisplayRefresh`(+`_Help`) `@0x004044F8`; + `ID_Rendering_DisplayResolution`(+`_Help`) `@0x00404525`; + `ID_Input_UseMouseTurning`(+`_Help`) `@0x004045ED`; + `ID_Graphics_LandscapeDrawDistance` + its six choices + (VeryLow..Extreme) `@0x004041B7`/`@0x00404371`. Also confirmed the + asymmetric-but-correct entries: the three volume sliders contribute + only their `_Help` key (the trio row has no slider name text), and + Resolution's menu items are literal strings, not string-table keys — + both match the code paths exactly. + The test is load-bearing beyond keys: `SetRangeLabel` returns early + without querying when the caption child is absent, so a missing + `0x1000021E`/`0x1000021F` would surface as a dropped + `ID_Graphics_Value_*` key. It is therefore a real structural pin on + M1's fix. +3. **Per-row retail-default pin** — `ConfigOptionsPageControllerTests.cs:557-625`, + all 30 rows against the literals. I re-derived every one of the 27 + `SetDefaultValue` immediates from the binary; the table matches + (`0x3EE66666`=0.45, `0x42200000`=40, `0x42B40000`=90, `0x03200258`, + `0x42480000`=50, `0x3F0CCCCD`=0.55, and the menu immediates + `6a 02`/`6a 01`/`6a 01`/`6a 08`/`6a 02`/`6a 01`). +4. **Separator/list-item count pin** — `:381-413` (39). + +--- + +## N1 — AP-198 row count — **CLOSED** + +Section-3 header (`retail-divergence-register.md:178`) now reads "the +Config tab's TEN Graphics/Rendering-Quality-family rows (including Screen +Brightness, its own field as of the S2 fix)"; the row body (`:191`) states +the correction and enumerates the same ten. Internally consistent. + +## N2 — authored scrollbar id — **CLOSED** + +`Bind` reads `listBox.ScrollbarElementId` +(`ConfigOptionsPageController.cs:259`), fed from dat property `0x72` via +`ElementReader.cs:524` → `DatWidgetFactory.cs:106` → +`UiTemplateListBox.cs:102/141`. The fixture's Config ListBox carries +property `114` (`0x72`) = `268435969` = `0x10000201`, so the authored read +returns the same value the constant documented. The `scrollbarElementId == 0` +guard (`:261`) degrades to the existing log path. + +## N3 / N4 — recorded, out of scope + +Left as the review dispositioned them. N4 needs no action. + +--- + +# Residuals + +## SHOULD-FIX + +**SF-1 — `AudioSettings.cs:106` misattributes `SetDefaultValue` to the +wrong retail function, contradicting line 50 of the same file.** +`Default`'s doc says the six trailing fields come from "retail's OWN +byte-verified `gmClient::InitUIPreferences` literals: `SetDefaultValue(1, +0x3f800000)`". `SetDefaultValue` does not exist anywhere in +`gmClient::InitUIPreferences` — that function contains only +`AttachPreference` / `SetPreferenceRange` / `SetEnumChoices` (verified +across its whole body). The `SetDefaultValue(1, 0x3f800000)` calls are in +`gmConfigUI::InitOptions` at `0x0049E435` / `0x0049E457` / `0x0049E479`, +which is exactly what the *class* doc says two paragraphs earlier at +`AudioSettings.cs:50`. The rework edited this sentence (it changed +"toggle=checked/'Disabled'=true" to "toggle=checked=enabled") and left the +wrong function name in place. One-word fix; matters because this project +treats decomp citations as load-bearing. + +**SF-2 — the gate script asserts an unread DAT caption and tells the +tester a checked "Disable Sound Effects" box means audible.** +`2026-08-11-campaign-op-test-script.md:568-575` and `:580-587` name the +control by the English "Disable Sound Effects" and instruct that CHECKED = +audible. The behaviour is correct (verified above, including the +un-inverted checkbox widget). But the review's own M2 fix direction asked +to "read the DAT caption for `ID_Sound_DisableSound` at the gate to +confirm which way the LED reads", and the rework did not — it asserted +instead that the caption is a legacy misnomer +(`AudioSettings.cs:41-46`). Two failure modes: if the DAT English really +does read "Disable Sound Effects", a tester reading step 4 will see a +self-contradictory instruction and may report a false defect; if it reads +something else, step 4 names a control that is not on screen. Cheapest +honest fix: have the step ask the tester to **report the checkbox's +caption verbatim** and describe the observed behaviour, rather than +asserting the caption. (Register/AP-199 wording is fine — it describes the +stored semantic, not the English.) + +## NOTE + +**N-a — `ScreenBrightness` was inserted mid-record, not appended.** +`DisplaySettings.cs:65` sits between `ParticleRange` and +`AutomaticDegrades`, shifting every subsequent positional parameter. Safe +today: both `new DisplaySettings(` sites use named arguments +(`SettingsStore.cs:64`, `SettingsStoreTests.cs:41`) and a positional call +would fail to compile on the `float`-vs-`bool` mismatch anyway. Appending +to the all-defaulted tail is the lower-risk convention for this record +family. + +**N-b — `DisplaySettingsTests.AvailableResolutions_includes_common_16_9_options` +is now misnamed.** `DisplaySettingsTests.cs:31` — the list it validates +now contains a 4:3 mode. The assertions still pass (membership + +ascending width); only the name and the summary comment are stale. + +**N-c — `ConfigOptionsPageController.ScrollbarElementId` is no longer read +by production code.** After N2, `Bind` uses the authored field +(`:259`); the constant (`:163`) survives as documentation and is used only +by the two scrollbar tests (`ConfigOptionsPageControllerTests.cs:767`, +`:812`). Those tests therefore assert the constant, not the authored +value — a DAT change would fail them loudly rather than silently, which is +the acceptable direction. + +**N-d — the engine assignment itself is still untested.** +`ComputeEffectiveCategoryVolumes` is pinned three ways, but +`engine.SfxVolume = sfx; engine.AmbientVolume = ambient` +(`RuntimeSettingsTargets.cs:140-141`) has no fake-engine test — swapping +those two lines would pass every suite. The `FakeRuntimeTargets` +"target-audio" event assertion (`RuntimeSettingsControllerTests.cs:301`) +still only proves the call happened. Low risk (two adjacent lines, no +branch), recorded so it is not rediscovered. + +**N-e — tooltips are on the interactive widget only, and that is +retail-shaped. Do not reopen.** `SetLabelText` (`:1109-1120`) still sets +no tooltip on the sibling name `UiText`. Retail behaves the same: +`PlayerOptionPage::AddSliderOption`/`AddMenuOption` return the option +element and `UIOption_*::SetTooltip` (`0x00487053` → +`UIElement::SetTooltip`) targets it; `SetSliderLabel` only writes +`StringInfo` onto the text children. acdream matches. Recorded so a later +reviewer does not re-file S3. + +**N-f — the "counterintuitive default" framing is fully gone from the gate.** +The old step-6 text that told the user muted-by-default was "not a bug" +has been replaced by its inverse, including an explicit "if you hear +silence… that IS a defect — report it" (`:587`). No surviving gate step +blesses the M2 behaviour. + +**N-g — lane A's "24 option rows" is still stale.** +`2026-08-10-options-panel-structure.md:487` still says 24; the decomp has +27, which is what the slice implements and what the new 39-item pin +encodes. The review listed this as a parenthetical aside inside S1, not a +required fix, and it was not corrected. Harmless (nothing reads it), but +it is the kind of stale research line that costs a future session an hour. + +--- + +# Blast radius (new surfaces this rework introduced) + +| Surface | Consumers found | Verdict | +|---|---|---| +| `AudioSettings` field renames | `RuntimeSettingsTargets.cs:164-165`; `ConfigOptionsPageController.cs:339/340/349/350/363/364`; `SettingsStore.cs:131-133, 683-689`; 3 test files | Complete. Zero readers of the old names; renames are compile-breaking, so nothing can be silently missed. | +| Startup path | `RuntimeSettingsController.cs:263` → `RuntimeSettingsStartupTargets.ApplyAudio` | Correct values via the shared helper. | +| Dev-tools / legacy panel | `SettingsPanel.cs` has no audio-enable surface; `SettingsVM` mutates by `with` | Untouched; draft preview (`WorldRenderFrameBuilder.cs:372`) uses the same corrected helper. | +| OP7 / headless bots | `src/AcDream.Headless/` — zero `Audio` references | Confirmed: bots declare no audio; OP7's `characterOptions` block is unaffected. | +| `UiScrollbar`/`UiMenu.TooltipText` | Default `null` → `GetTooltipText()` returns `null` (`UiScrollbar.cs:103`, `UiMenu.cs:48`) | Contained. Every pre-existing scrollbar/menu (Chat opacity sliders, inventory, etc.) is behaviourally unchanged. | +| `DisplaySettings.ScreenBrightness` | `SettingsStore.cs:74, 662`; `ConfigOptionsPageController.cs:480-481` | Additive-only; `Gamma` and its legacy `[0.5,2.0]` control untouched, its existing default pin unmodified. | +| `AvailableResolutions` prepend | `SettingsPanel.cs:200-206` (value-based `IndexOf`) | No persisted index anywhere; no shift hazard. | + +--- + +# Fix checklist + +| # | Severity | Item | +|---|---|---| +| SF-1 | SHOULD | `AudioSettings.cs:106` — cite `gmConfigUI::InitOptions` (`@0x0049E435`/`E457`/`E479`) for `SetDefaultValue`, not `gmClient::InitUIPreferences`; line 50 of the same file already has it right | +| SF-2 | SHOULD | Gate script `:568-587` — ask the tester to report the Sound-trio checkbox caption verbatim instead of asserting "Disable Sound Effects"; the DAT string for `ID_Sound_DisableSound` was never read | +| N-a…N-g | NOTE | Mid-record insert; stale `16_9` test name; now-unused `ScrollbarElementId` constant; untested engine assignment; tooltip scope is retail-shaped (do not reopen); gate no longer blesses M2; lane A's stale "24 option rows" | diff --git a/src/AcDream.UI.Abstractions/Panels/Settings/AudioSettings.cs b/src/AcDream.UI.Abstractions/Panels/Settings/AudioSettings.cs index cae61c05..fc5e97d8 100644 --- a/src/AcDream.UI.Abstractions/Panels/Settings/AudioSettings.cs +++ b/src/AcDream.UI.Abstractions/Panels/Settings/AudioSettings.cs @@ -103,7 +103,10 @@ public sealed record AudioSettings( /// sound preference (SoundManager::InitPrefs @ 0x005503F0), /// so ambient starts at unity rather than the invented 0.8. The six OP6 /// trailing fields default to retail's OWN byte-verified - /// gmClient::InitUIPreferences literals: SetDefaultValue(1, + /// gmConfigUI::InitOptions literals (@0x0049E435/E457/E479 — + /// OP6 re-review SF-1: an earlier revision mis-attributed these to + /// gmClient::InitUIPreferences, which contains no + /// SetDefaultValue at all): SetDefaultValue(1, /// 0x3f800000) on every trio (toggle=checked=enabled per the class /// doc's byte evidence, slider=1.0) — retail's 2013 EoR build genuinely /// ships every Sound category ENABLED and audible by default; ported