163 lines
9.1 KiB
Markdown
163 lines
9.1 KiB
Markdown
# CH2 review findings — `77c8296e` (REJECT; rework list)
|
||
|
||
Opus dual-lens review, 2026-08-09. Method: all 339 `case` bodies of
|
||
`HandleFailureEvent @0x00571990` extracted, every `push imm32` string
|
||
reference in VA `0x571990–0x575480` swept from the PDB-paired binary
|
||
(`check_exe_pdb.py` → MATCH), diffed against the landed table,
|
||
cross-checked against ACE's `WeenieError.cs`/`WeenieErrorWithString.cs`
|
||
doc comments. The two oracles agreed on every discrepancy.
|
||
|
||
## BLOCKER 1 — SpewBox never renders; pending queue unbounded
|
||
|
||
`SpewBoxController.cs:75` constructs the `UiText` with `Visible = false`;
|
||
`:94` (inside `ComputeLines`, the `LinesProvider`) is the only site that
|
||
sets it true. `LinesProvider` is only invoked from `UiText.OnDraw`
|
||
(`UiText.cs:407/429/451/472`), reached only through
|
||
`UiElement.DrawSelfAndChildren`, whose first statement is
|
||
`if (!Visible) return;` (`UiElement.cs:460`; `DrawOverlays` gates
|
||
identically at `:511`). Start-invisible → provider never called →
|
||
never visible → **no SpewBox line ever draws**. Consequence 2:
|
||
`SpewBoxVM.Lines` is the sole caller of `SpewBoxState.Tick`, so
|
||
`_pending` (`SpewBoxState.cs:66`, uncapped `Queue<string>`) never drains
|
||
— an unbounded per-session leak on a hot path. The controller tests
|
||
(`SpewBoxControllerTests.cs:28,43`) invoke `text.LinesProvider!()`
|
||
directly, bypassing the `Visible` gate, so they cannot see it.
|
||
|
||
**Fix:** retail drives `gmSpewBoxUI::Update` from the UI tick (global
|
||
message 3, `UIElementManager::UseTime @0x0045CFD0`), not from drawing.
|
||
Give `SpewBoxController` an explicit per-frame `Tick(double now)` wired
|
||
from the retained-UI host loop (the existing per-UI-tick seam — see
|
||
`IUiGlobalTimeListener` / wherever retained controllers receive frame
|
||
time); it calls `_vm.Lines(now)`, caches the result, sets
|
||
`_text.Visible` there; `LinesProvider` returns the cache (exactly
|
||
`PortalWaitNoticeController`'s external-push shape). Tests: drive the
|
||
frame hook (not the provider); assert the pending queue drains without
|
||
a draw pass.
|
||
|
||
## BLOCKER 2 — table: 5 missing ids, ~22 wrong strings, 0x4F8 resolvable
|
||
|
||
Transcription by "enumerate `case` labels, read the pseudo-C's ~33-char
|
||
inline preview" is structurally unsound: it misses ids dispatched by
|
||
`else if (arg2 == N)` and merges siblings whose literals collide in the
|
||
preview. Re-derive mechanically: sweep `push imm32` operands in
|
||
`0x571990–0x575480`, read each UTF-16LE literal to NUL, attribute per
|
||
case block by disassembly (BN's line addresses shift up to ~0x20 bytes);
|
||
cross-check every row against ACE's enum comments.
|
||
|
||
### 2a. Five missing ids (else-if dispatch)
|
||
|
||
| id | retail literal | type |
|
||
|---|---|---|
|
||
| `0x04F` | `You fail to affect %s because $s cannot be harmed!` | Magic (7) |
|
||
| `0x3EE` | `The container is closed!` | ClientLocal (0x1A) |
|
||
| `0x408` | `Your spell cannot be cast inside` | ClientLocal (0x1A) |
|
||
| `0x48A` | `You must be a monarch to purchase this dwelling.` | Default (0) |
|
||
| `0x4E8` | `The %s cannot be used while on a hook, use the '@house hooks on' command to make the hook openable.` | Default (0) |
|
||
|
||
`0x43` is correctly absent (no text — only `AbortAutomaticAttack`).
|
||
`0x3EE`/`0x408` are live on local ACE (`Player_Inventory.cs:883,955`,
|
||
`Player_Magic.cs:513`). The landed test
|
||
`Format_0x004F_FallsBackToHex_NoRetailCaseExists` pins the 0x04F
|
||
regression and must be replaced.
|
||
|
||
### 2b. Wrong strings (landed line → correct retail text)
|
||
|
||
| line | id | correct retail string |
|
||
|---|---|---|
|
||
| `:166` | `0x051` | `You fail to affect %s because you are not a player killer!` |
|
||
| `:168` | `0x053` | `You fail to affect %s because you are not the same sort of player killer as %s!` |
|
||
| `:169` | `0x054` | `You fail to affect %s because you are acting across a house boundary!` |
|
||
| `:218` | `0x466` | `You must purchase Asheron's Call: Dark Majesty to interact with that portal.` |
|
||
| `:251` | `0x4A3` | `You must have linked with a portal in order to recall to it!` |
|
||
| `:268` | `0x4B5` | `You must specify a character to query.` |
|
||
| `:306` | `0x4E0` | `…certain level of skill. Your attributes cannot be transferred…` (transfer wording, not skill-lowering) |
|
||
| `:328` | `0x4F7` | `%s fails to affect you because you are not a player killer!` |
|
||
| `:401` | `0x544` | `…attempting to remove %s as an allegiance officer.` |
|
||
| `:411` | `0x54E` | `The hook does not contain a usable item. You cannot open the hook because you do not own the house to which it belongs.` |
|
||
| `:415` | `0x552` | `…Throne of Destiny to use this function.` |
|
||
| `:416` | `0x553` | `…Throne of Destiny to use this item.` |
|
||
| `:417` | `0x554` | `…Throne of Destiny to use this portal.` |
|
||
| `:418` | `0x555` | `…Throne of Destiny to access this quest.` |
|
||
| `:460` | `0x57F` | `Your allegiance chat privileges have been temporarily removed by %s. Until they are restored, you may not view or speak in the allegiance chat channel.` |
|
||
| `:463` | `0x582` | `Your allegiance chat privileges have been restored by %s.` |
|
||
|
||
(`0x581` at `:462` is correct.) For the `0x552`–`0x555` family and
|
||
`0x4E0` the reviewer gave shapes — the fixer's binary sweep provides the
|
||
exact full texts; every row must come from the swept literal, not this
|
||
table's abbreviations. All routing TYPES were verified correct — only
|
||
strings change.
|
||
|
||
### 2c. 0x4F8 resolves — delete the exclusion
|
||
|
||
`data_7d2ee8` = `" fails to affect you because you are not the same sort
|
||
of player killer as "`, `data_7d2f80` = `"!\n"`; block stages
|
||
`arg3 + literal + arg3 + "!\n"` through three `operator+` calls
|
||
(`0x57475e/0x574765/0x574776`); type 7 (Magic) at the block's
|
||
`AddTextToScroll (0x00573ba5)`. Row:
|
||
`[0x4F8u] = new("%s fails to affect you because you are not the same sort of player killer as %s!", RetailLogTextType.Magic)`
|
||
(both `%s` take the same param — replace-all already does this). Delete
|
||
`Resolve_0x4F8_IsDeliberatelyExcluded_FallsBackToHex`; the class-doc
|
||
claim about 0x4F7's "dangling" literal is a misattribution of 0x4F8's
|
||
first operand — fix it. New pinned count: **344** (338 + 5 + 0x4F8).
|
||
|
||
## SHOULD-FIX
|
||
|
||
1. **`ResetSpewBox()` is dead code** (`RuntimeCommunicationState.cs:135`
|
||
— no caller; `RuntimeGenerationReset.cs:318` only resets
|
||
ChatIdentity). Fold `SpewBox.Reset()` into the ChatIdentity stage (same
|
||
lifetime boundary) or add a stage; add a reset test.
|
||
2. **`AddText` trims the wrong end + invents an empty-drop**
|
||
(`RuntimeCommunicationState.cs:172-176`). Retail's
|
||
`AddTextToScroll` calls `trim(&str, 1, 1, ws)` at `0x00563ce3` —
|
||
BOTH ends (the trailing-only trim in research §3.1 is
|
||
`gmSpewBoxUI::Update`'s separate call). Retail also broadcasts empty
|
||
strings deliberately (`s_NullBuffer` at type 7). Fix: `text.Trim()`;
|
||
remove the empty early-return (and its pinning test) — or keep it
|
||
with a register row.
|
||
3. **`ShowWeenieError` bypasses the chokepoint** —
|
||
`LiveSessionRuntimeFactory.cs:341` still routes through
|
||
`ChatLog.OnWeenieError` (hardcoded type 0x00, comment cites retired
|
||
AP-176). `0x0561` belongs in the SpewBox and lands green in chat.
|
||
Fix: route through `Communication.AddText(Resolve(code, param))` and
|
||
delete `ChatLog.OnWeenieError` — or restore an AP row.
|
||
4. **Unmapped-id fallback is unregistered divergence**
|
||
(`WeenieErrorMessages.cs:129-134` emits `WeenieError 0xNNNN` into
|
||
chat). Retail's switch has no default — unhandled ids produce NO
|
||
text. Fix: match retail (silence toward the player), keep the hex id
|
||
as a diagnostics log line only; flip/delete the fallback tests — or
|
||
file a register row.
|
||
|
||
## NITS
|
||
|
||
1. `SpewBoxController.cs:34,42` — "AP-TBD" ×2 → AP-177 (lifetime),
|
||
AP-178 (position/colour).
|
||
2. Three stale "the split lands with CH2" comments (`ChatVM.cs:122`,
|
||
`HeadlessGameplayOperations.cs:231`, `LiveSessionRuntimeFactory.cs:336`)
|
||
+ `AddText`'s `windowId` accepted but never consumed: retail's §2.3
|
||
dual-destination (0x1A + non-zero windowId → SpewBox AND the
|
||
originating chat window, ~40 sites) is unimplemented. File a register
|
||
row for it and update the comments (implementing it is CH4/CH5 scope
|
||
at the earliest).
|
||
3. `SpewBoxLayoutDumpDiagnostic` almost certainly never consulted
|
||
`dats.Local` (`client_local_English.dat` — the file research §8.1
|
||
names); also the `0x10000012` probe treats a layout enum as a dat id.
|
||
Extend the sweep to `dats.Local` before trusting AP-178's wording; if
|
||
still negative, keep the row as-is.
|
||
4. `WeenieErrorMessages.cs:36-41` — says "19 ids", lists 18.
|
||
5. `SpewBoxState` doc overstates "exactly one frame" decoupling —
|
||
same-call `Tick`+`Snapshot` displays same-frame; retail is 0–1. Fix
|
||
wording.
|
||
6. `GameEventWiring.cs:261,276` apply `IsSilentClientControlStatus` to
|
||
0x028A/0x028B but the UseDone path (`:596-601`) does not — harmless
|
||
today; align or comment.
|
||
|
||
## CONFIRMED-OK (do not touch)
|
||
|
||
Core placement of `SpewBoxState` (canonical instance owned by
|
||
`RuntimeCommunicationState`, UI borrows); `0x47` silent (DoJump's jump
|
||
table has exactly 4 targets; `CommenceJump`'s fallback unreachable —
|
||
brief's §4.2 reading was wrong); all 338 routing TYPES verified against
|
||
the decomp with zero mismatches; all 11 `ClientTextRefusals` byte-exact;
|
||
jump-family constant sharing; AP-177/178/179 content; placeholder
|
||
honesty; architecture/layering; the `ClientLocal`-never-reaches-
|
||
transcript test.
|