acdream/docs/research/2026-08-11-op6-rereview.md

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"