ff5776415b
The re-reviewer byte-decoded ALL six caption sites independently (no
transposition; a third evidence line from the fixture's authored
left/right label geometry), re-verified the audio chain through the one
place an inversion could still hide (the checkbox pass-throughs), and
hand-traced the 97-key conformance table (exactly 97, none invented or
dropped). Residuals applied here: SF-1 the AudioSettings doc comment's
wrong function attribution (the SetDefaultValue literals live in
gmConfigUI::InitOptions @0x0049E435/E457/E479, not InitUIPreferences);
SF-2 the gate script no longer asserts an unread DAT caption — it gates
on behaviour and asks the tester to report the authored English
verbatim; lane A's stale '24 option rows' corrected to 27 (the 39-item
pin is the authoritative tally). AudioSettings.cs change is
comment-only (no executable-code delta; suite state carries from
67b0815c, re-verified at the next code commit).
Campaign state: OP1-OP7 ALL CODE-COMPLETE; OP3/OP4/OP5/OP6 gates ready;
OP8 (Configure Keyboard) is the sole remaining implementation slice.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
479 lines
24 KiB
Markdown
479 lines
24 KiB
Markdown
# 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.<field>` (= `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" |
|