docs: FA3 mechanism review -- APPROVE-WITH-FIXES (2 MUST-FIX, 9 SHOULD-FIX)
Campaign FA slice FA3 mechanism-faithfulness review of0a9ca2f1/74c3d85d/b6a25110/d7e1cffd. The load-bearing tab-table correction VERIFIED CORRECT three independent ways: the authored 0x2E array re-parsed straight out of the committed fixture (Allegiance 0x1000028C -> 0x10000291 is the sole IsDefault entry), each button's caption resolved from the installed DATs via the live-mount probe, and each page's own P0x57 + RegisterElementClass type re-read in the pseudo-C. A fourth corroboration the addendum missed: under the corrected pairing the default tab is also the LEFT-MOST tab (x=0). MUST-FIX (both in the user's connected-gate script, neither code): 1. The script still carries the REFUTED x-order -- step 9 states the strip as "Friends, Allegiance, Fellowship, Squelch" and step 1 primes the user to expect Friends left-most. Real geometry: Allegiance x=0, Fellowship x=72, Friends x=144, Squelch x=206. 2. Step 14 tells the user `@allegiance info` should reveal the monarch/patron blocks and to "report if they do not" -- FA2's own fix round deliberately stopped 0x027C from seeding RuntimeAllegianceState, ApplyUpdate (0x0020) is the only writer of _hasProfile, and FA3 sends no 0x001F. The script steers the user into a false defect report. SHOULD-FIX: unsupported "retail's Toggle-action semantics" claim on F3/F4 (no P0x57 read site, no OnAction handler, folded gmPanelUI global-message stub -- the rule is acdream's OpenSpellbook precedent, not retail); per-frame closure allocation in SocialAllegiancePageController.Tick; two probe findings printed but never asserted (0x10000492 count, page exclusivity); "three checkboxes" is four; AD-79 enumerates seven controls but its cited test pins six; the two page controllers do not name AD-79; the allegiance empty state is gated on HasProfile rather than retail's per-relationship rule (TryGetMonarch/TryGetPatron already exist); Tick() ignores _disposed; FindDeepest's doc overstates its guarantee. Verified clean: every §6 Campaign-OP lesson (string resolver on both Builds, scoped lookups -- I enumerated ALL duplicate ids and found three previously-uncalled-out cross-page repeats, tab activation, no 0x0-extent lazy children, cross-layout templates so the same-layout skip cannot apply, no hand-rolled viewport); U6 genuinely closed (page 0x10000292 has exactly two children); Flush/ClearContent resets ContentHeight; the J4.1 owners are borrowed by reference and clear in place; catalog id 12 byte-verified and the toolbar seam tolerates it via the same path four existing non-toolbar panels take; window-frame policy byte-identical to Options. data_794358 BYTE-VERIFIED in the PDB-paired binary as UTF-16LE " " (one space, not empty) -- lane A's L" " reading and FA3's BlankLine both correct. Live-mount probe passes against the installed DATs with no fixture drift; 22 FA3 tests and 4,871 App tests / 3 skips / 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
e9eb756480
commit
9e622f565e
1 changed files with 514 additions and 0 deletions
514
docs/research/2026-08-12-fa3-review-mechanism.md
Normal file
514
docs/research/2026-08-12-fa3-review-mechanism.md
Normal file
|
|
@ -0,0 +1,514 @@
|
||||||
|
# Campaign FA slice FA3 — MECHANISM-FAITHFULNESS review
|
||||||
|
|
||||||
|
**Target:** `0a9ca2f1` (feature), `74c3d85d` (fixture + tests), `b6a25110`
|
||||||
|
(gate script + ledger), `d7e1cffd` (research addendum), reviewed against
|
||||||
|
`docs/plans/2026-08-11-fellowship-allegiance-campaign.md` slice FA3 + D1 + §6
|
||||||
|
and `docs/research/2026-08-11-fa-panel-structure.md` (incl. §10 + the FA3
|
||||||
|
correction addendum).
|
||||||
|
|
||||||
|
**Verdict: APPROVE-WITH-FIXES** — 2 MUST-FIX (both in the user's connected-gate
|
||||||
|
script, which is the artifact the user actually reads while gating), 9
|
||||||
|
SHOULD-FIX. The load-bearing tab-table correction is **CONFIRMED CORRECT** by
|
||||||
|
re-derivation from the authored bytes AND independently from the installed
|
||||||
|
DATs. No code defect found in the mount path.
|
||||||
|
|
||||||
|
Method note: every claim below was re-derived, not taken from the commit
|
||||||
|
messages. The fixture was parsed directly; the env-gated live-mount probe was
|
||||||
|
run against the installed DATs; the retail anchors were re-read in
|
||||||
|
`acclient_2013_pseudo_c.txt`; and one BN string operand was byte-verified in
|
||||||
|
the PDB-paired binary (`check_exe_pdb.py` → `=== MATCH ===`, GUID
|
||||||
|
`9e847e2f-777c-4bd9-886c-22256bb87f32`).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. The tab-table correction — VERIFIED CORRECT (item 1)
|
||||||
|
|
||||||
|
This was the load-bearing fact. It holds, three independent ways.
|
||||||
|
|
||||||
|
**(a) The authored `0x2E` array, read straight out of the committed fixture**
|
||||||
|
(`tests/AcDream.App.Tests/UI/Layout/fixtures/social_panel_2100006E_1000018F.json`,
|
||||||
|
root `0x1000018F`, property `46` = `0x2E`, four `Kind 8` struct entries with
|
||||||
|
sub-properties `48`=`0x30` button, `49`=`0x31` page, `50`=`0x32` IsDefault):
|
||||||
|
|
||||||
|
| entry | P0x30 (button) | P0x31 (page) | P0x32 |
|
||||||
|
|---|---|---|---|
|
||||||
|
| 0 | 268436108 = `0x1000028C` | 268436113 = `0x10000291` | **true** |
|
||||||
|
| 1 | 268436110 = `0x1000028E` | 268436114 = `0x10000292` | absent |
|
||||||
|
| 2 | 268436754 = `0x10000512` | 268436755 = `0x10000513` | absent |
|
||||||
|
| 3 | 268436795 = `0x1000053B` | 268436810 = `0x1000054A` | absent |
|
||||||
|
|
||||||
|
This is **exactly** the pairing recorded in `SocialPanelController.cs:34-37`,
|
||||||
|
and Allegiance (`0x1000028C`→`0x10000291`) is the sole `IsDefault` entry.
|
||||||
|
|
||||||
|
**(b) Each button's own resolved caption** — from the live-mount probe run
|
||||||
|
against the installed DATs during this review:
|
||||||
|
|
||||||
|
```
|
||||||
|
[socialprobe] tab button 0x1000028C (UiText) caption='Allegiance'
|
||||||
|
[socialprobe] tab button 0x1000028E (UiText) caption='Fellowship'
|
||||||
|
[socialprobe] tab button 0x10000512 (UiText) caption='Friends'
|
||||||
|
[socialprobe] tab button 0x1000053B (UiText) caption='Squelch'
|
||||||
|
```
|
||||||
|
|
||||||
|
**(c) Each page's own `P0x57` + its `RegisterElementClass` type**, both from the
|
||||||
|
fixture and re-confirmed by the probe:
|
||||||
|
|
||||||
|
| page | authored `Type` | retail class (verified in pseudo-C) | `P0x57` |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `0x10000291` | `0x1000002C` | `RegisterElementClass(0x1000002c, gmAllegianceUI::Create)` @`004911fa` | `0x1000000E` = `ToggleAllegiancePanel` (F3) |
|
||||||
|
| `0x10000292` | `0x1000002D` | `RegisterElementClass(0x1000002d, gmFellowshipUI::Create)` @`0048e6aa` | `0x1000000F` = `ToggleFellowshipPanel` (F4) |
|
||||||
|
| `0x10000513` | `0x10000045` | `RegisterElementClass(0x10000045, gmFriendsUI::Create)` @`0048e21a` | `0x10000118` |
|
||||||
|
| `0x1000054A` | `0x10000047` | `RegisterElementClass(0x10000047, gmSquelchUI::Create)` @`0048d04a` | `0x10000124` |
|
||||||
|
|
||||||
|
The class doc's further claim that `0x10000118`/`0x10000124` carry **no**
|
||||||
|
retail-default keybind is also correct: neither literal appears in
|
||||||
|
`keymap-default.txt` or `retail-default.keymap.txt`.
|
||||||
|
|
||||||
|
**A fourth corroboration the addendum did not state, and should have.** With
|
||||||
|
the corrected pairing, the authored x-order becomes Allegiance (x=0),
|
||||||
|
Fellowship (x=72), Friends (x=144), Squelch (x=206) — i.e. **the authored
|
||||||
|
default tab is also the left-most tab**, which is what one would expect and
|
||||||
|
which the old (refuted) guess did not produce. Button widths track caption
|
||||||
|
lengths under the corrected mapping too (`Friends`, the shortest caption, is
|
||||||
|
the narrowest button at 62px). This same x-order is the basis of MUST-FIX 1
|
||||||
|
below.
|
||||||
|
|
||||||
|
Also verified: root `P0x10000029 = 12` (the `RetailPanelCatalog.SocialPanel`
|
||||||
|
byte-verification claim), root `P0x57 = 0x1000000D`, close button `0x10000290`
|
||||||
|
`P0x12 = 0x1000000D`, and `0x1000000D` really is an InputMap **context** id
|
||||||
|
(`ID_InputMap_ToggleChatEntry`, `src/AcDream.Core/Input/RetailActionMap.cs:161`;
|
||||||
|
it appears as `Context 0x1000000D` in `keymap-default.txt`) — so the class
|
||||||
|
doc's "authored but UNCONSUMED" reading is accurate, not a hand-wave.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## MUST-FIX
|
||||||
|
|
||||||
|
### MF-1 — the gate script carries the REFUTED x-order into the user's contract
|
||||||
|
|
||||||
|
`docs/research/2026-08-12-campaign-fa-test-script.md:59-64` (step 9):
|
||||||
|
|
||||||
|
> "left-to-right authored order is Friends, Allegiance, Fellowship, Squelch"
|
||||||
|
|
||||||
|
and `:20-25` (step 1):
|
||||||
|
|
||||||
|
> "the authored default tab — NOT Friends, despite Friends being drawn
|
||||||
|
> left-most on the tab strip in some sort orders"
|
||||||
|
|
||||||
|
Both are wrong. The authored geometry (fixture, corroborated by the probe's
|
||||||
|
`root-child` dump) is:
|
||||||
|
|
||||||
|
```
|
||||||
|
0x1000028C "Allegiance" x=0 72x25
|
||||||
|
0x1000028E "Fellowship" x=72 72x25
|
||||||
|
0x10000512 "Friends" x=144 62x25
|
||||||
|
0x1000053B "Squelch" x=206 70x25
|
||||||
|
0x10000290 (close X) x=276 24x25
|
||||||
|
```
|
||||||
|
|
||||||
|
Friends is drawn **third**, not left-most. This is the same x-order guess
|
||||||
|
`d7e1cffd` corrects in the research doc — it just never got corrected in the
|
||||||
|
script. Step 10's caption list (`:65-68`) repeats the wrong ordering.
|
||||||
|
|
||||||
|
Why this is MUST-FIX and not a nit: the script is the user's contract for the
|
||||||
|
connected gate, and step 1 explicitly primes the user to expect Friends at the
|
||||||
|
left edge next to a tab that opens by default. The user will see Allegiance at
|
||||||
|
x=0 and has been told that is the wrong-looking case. Fix step 9, step 1's
|
||||||
|
parenthetical, and step 10 to the real order — and take the opportunity to say
|
||||||
|
"the default tab is also the left-most tab", which is the most reassuring form
|
||||||
|
of the correction.
|
||||||
|
|
||||||
|
### MF-2 — the gate script's step 14 sends the user to a trigger that CANNOT fire, and tells them to report it if it doesn't
|
||||||
|
|
||||||
|
`docs/research/2026-08-12-campaign-fa-test-script.md:100-107` (step 14):
|
||||||
|
|
||||||
|
> "If some other client action causes an allegiance push to land server-side
|
||||||
|
> during this session (e.g. a `@allegiance info` chat command, which already
|
||||||
|
> works independently of this panel), the monarch/patron blocks should then
|
||||||
|
> become VISIBLE — **report if they do not**."
|
||||||
|
|
||||||
|
and the matching "What to report" bullet at `:153-155`:
|
||||||
|
|
||||||
|
> "The Allegiance page's monarch/patron blocks staying hidden after a real
|
||||||
|
> allegiance push has visibly landed (e.g. after `@allegiance info` shows
|
||||||
|
> non-empty data in chat)."
|
||||||
|
|
||||||
|
`RuntimeAllegianceState._hasProfile` is set in exactly one place —
|
||||||
|
`ApplyUpdate` (`src/AcDream.Runtime/Gameplay/RuntimeAllegianceState.cs:126-142`),
|
||||||
|
whose own doc comment reads *"`0x0020 AllegianceUpdate` — … the **ONLY**
|
||||||
|
inbound writer of this owner's profile (see the class doc's MUST-FIX 2
|
||||||
|
correction)"*. FA2's own fix round deliberately stopped `0x027C` (the
|
||||||
|
`@allegiance info` response family) from seeding this owner
|
||||||
|
(`4272ad0e`, "0x027C stops seeding"; `GameEventWiring.cs:231` documents it),
|
||||||
|
and the plan's FA5 row states the `0x001F` subscription that provokes `0x0020`
|
||||||
|
is **FA5 scope**. FA3 sends no `0x001F`.
|
||||||
|
|
||||||
|
So the script names a trigger the campaign deliberately disconnected, and then
|
||||||
|
instructs the user to file a defect when it fails to work. Either drop the
|
||||||
|
trigger entirely and state plainly that the blocks are expected to stay hidden
|
||||||
|
for the whole of FA3 (the honest shell-slice expectation), or replace it with
|
||||||
|
the only real path (a server-side allegiance change that makes ACE push
|
||||||
|
`0x0020` unsolicited) and say so. The "What to report" bullet must go with it.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## SHOULD-FIX
|
||||||
|
|
||||||
|
### SF-1 — the F3/F4 "retail Toggle-action semantics" claim has no retail anchor (item 2)
|
||||||
|
|
||||||
|
`src/AcDream.App/UI/Layout/SocialPanelController.cs:200-204`:
|
||||||
|
|
||||||
|
> "lets `RetailUiRuntime.HandleInputAction` implement **retail's** Toggle-action
|
||||||
|
> close-on-second-press semantics (same shape as `OpenSpellbook`'s own
|
||||||
|
> page-aware toggle)"
|
||||||
|
|
||||||
|
I went looking for the retail handler and could not find one. Re-derived:
|
||||||
|
|
||||||
|
- **No `P0x57` consumer exists in the binary.** The only two `0x57` reads
|
||||||
|
anywhere in `acclient_2013_pseudo_c.txt` are
|
||||||
|
`AppraisalProfile::InqInt(arg2, 0x57, …)` @`004b2954` and
|
||||||
|
`AppraisalProfile::InqFloat` @`004b3981` — unrelated. No
|
||||||
|
`GetAttribute_Enum(this, 0x57, …)` site exists. This matches lane A §6.1's
|
||||||
|
UNVERIFIED flag; the FA3 class doc correctly restates it for the *root's*
|
||||||
|
`P0x57`, then quietly relies on a different retail claim for the toggle.
|
||||||
|
- **`ClientUISystem::OnAction @0x00564B90` does not handle these actions.** For
|
||||||
|
`m_InputAction > 0x7c` it handles exactly `0x1000001E`, `0x10000025` and
|
||||||
|
`0x1000002B` and returns `0` for everything else — `0x1000000E`/`0x1000000F`
|
||||||
|
fall through.
|
||||||
|
- **`gmPanelUI` never sees the action.** `gmPanelUI::PostInit @0x004BD010` does
|
||||||
|
`RegisterForGlobalMessage(this, 1)`, but `gmPanelUI::ListenToGlobalMessage`
|
||||||
|
is COMDAT-folded onto `NoticeHandler::RecvNotice_DisplayWeenieError
|
||||||
|
@0x004F5860`, whose body is `return;`.
|
||||||
|
- No other `OnAction` implementation's range covers `0x1000000E`/`0x1000000F`
|
||||||
|
(`ChatInterface` `0x10000020..0x10000119`, magic `0x10000060..0x10000105`,
|
||||||
|
combat `0x1000005B..`, `CPlayerSystem` `0x1000002A..`).
|
||||||
|
|
||||||
|
So retail's actual F3/F4 behaviour — and in particular what a repeat press does
|
||||||
|
when the panel is already open on the *other* tab — is **not established from
|
||||||
|
this decomp**. The shipped rule ("close only when already on the target tab")
|
||||||
|
is acdream's own `OpenSpellbook` precedent, which is a perfectly defensible
|
||||||
|
choice; it just must not be labelled as retail's. Re-word the comment to name
|
||||||
|
it as precedent-following, and add the unknown to the research doc's §8 list
|
||||||
|
(next to U7, which is the same gap) so FA7's closeout can decide whether it
|
||||||
|
wants a register row. The gate script's "same toggle-closes-on-repeat-press
|
||||||
|
shape as every other `Toggle*Panel` action" (`:26-29`) is fine as-is — it
|
||||||
|
claims acdream consistency, not retail fidelity.
|
||||||
|
|
||||||
|
### SF-2 — per-frame closure allocation in the allegiance page tick
|
||||||
|
|
||||||
|
`src/AcDream.App/UI/Layout/SocialAllegiancePageController.cs:108-110`:
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
IReadOnlyList<UiText.Line> lines = hasProfile ? NoLines : BlankLine;
|
||||||
|
if (_monarchName is not null) _monarchName.LinesProvider = () => lines;
|
||||||
|
if (_patronName is not null) _patronName.LinesProvider = () => lines;
|
||||||
|
```
|
||||||
|
|
||||||
|
`lines` is captured, so this allocates a display class **plus** a delegate,
|
||||||
|
twice, on **every frame**, whether or not the panel is visible
|
||||||
|
(`RetailUiRuntime.cs:559` ticks it unconditionally). At the profile's measured
|
||||||
|
~520 FPS that is ~2,000 allocations/second of pure garbage for a value that
|
||||||
|
changes at most once per session. The Modern Runtime slices spent whole commits
|
||||||
|
driving per-frame paths to 0 B; this is a new, avoidable regression of that
|
||||||
|
discipline in the graphical hot loop.
|
||||||
|
|
||||||
|
Fix: hoist two `static readonly Func<IReadOnlyList<UiText.Line>>` fields (one
|
||||||
|
returning `BlankLine`, one returning `NoLines`) and assign the cached delegate,
|
||||||
|
and/or only write `LinesProvider` when the gate actually flips. The fellowship
|
||||||
|
page's tick is already allocation-free — this is the only offender.
|
||||||
|
|
||||||
|
### SF-3 — the live-mount probe prints two of its headline findings without asserting them (item 7)
|
||||||
|
|
||||||
|
`tests/AcDream.App.Tests/UI/Layout/SocialPanelLiveMountProbeTests.cs:117-118`
|
||||||
|
computes `passupCount` and prints it, but never asserts it. The commit message
|
||||||
|
nevertheless claims *"`0x10000492` **is confirmed** authored twice under the
|
||||||
|
allegiance page"*. A print is not a gate: this cannot fail if a future importer
|
||||||
|
change collapses or drops one of the two instances — which is exactly the
|
||||||
|
regression the scoped-lookup rule exists to survive. (For the record I did
|
||||||
|
confirm the fact independently: the fixture authors `0x10000492` twice, once
|
||||||
|
under `0x10000255/0x10000490` and once under `0x1000025A/0x10000490`, and the
|
||||||
|
probe printed `= 2`.) Add `Assert.Equal(2, passupCount)`.
|
||||||
|
|
||||||
|
Likewise, the probe activates the tab behavior (`:67`) and asserts
|
||||||
|
`UnresolvedEntries` is empty, but never asserts **page exclusivity** — that
|
||||||
|
after `ActivateTabBehavior()` exactly one of the four page elements is
|
||||||
|
`Visible`, and that it is the Allegiance page. That is the #372 class
|
||||||
|
("the panel mounts but the pages are wrong/blank") stated as an assertion
|
||||||
|
rather than a hope, and it is one line.
|
||||||
|
|
||||||
|
Everything else the probe asserts is real and does cover the classes it claims:
|
||||||
|
non-empty tab captions (the #375 resolver class — I re-ran it and it passes),
|
||||||
|
the four pages resolving, the fellowship frame pair, and the six allegiance
|
||||||
|
signature elements. All pass against the installed DATs.
|
||||||
|
|
||||||
|
### SF-4 — "all three visible option checkboxes" is four
|
||||||
|
|
||||||
|
`src/AcDream.App/UI/Layout/SocialFellowshipPageController.cs:19`. Frame
|
||||||
|
`0x1000026B` authors **four** `Type 0x10000035` children —
|
||||||
|
`0x10000270` (IgnoreFellowshipRequests), `0x10000271`
|
||||||
|
(FellowshipAutoAcceptRequests), `0x10000272` (FellowshipShareXP),
|
||||||
|
`0x10000273` (FellowshipShareLoot) — per lane A §3.1 and confirmed in the
|
||||||
|
fixture. The gate script step 11 (`:73-82`) hedges with "a fourth, Share Loot,
|
||||||
|
may also be present" when both the decomp and the committed fixture already
|
||||||
|
settle it. Fix the count in both places; a gate script that is unsure about
|
||||||
|
what the user will see invites noise.
|
||||||
|
|
||||||
|
### SF-5 — AD-79's pinning test does not cover the control AD-79 itself enumerates
|
||||||
|
|
||||||
|
The register row names seven controls, including the Friends page's "Appear
|
||||||
|
Offline"-shaped checkbox `0x1000052C`, and cites
|
||||||
|
`SocialPanelControllerTests.FriendsAndSquelchActionButtons_AreClickable_ButHaveNoHandler`
|
||||||
|
as what pins the INERT contract. That test
|
||||||
|
(`SocialPanelControllerTests.cs:272-291`) covers only the six **buttons**.
|
||||||
|
`0x1000052C` builds through `DatWidgetFactory.BuildCheckbox` as a `UiButton`
|
||||||
|
(`DatWidgetFactory.cs:785-816`), so it can be added to the existing loop
|
||||||
|
verbatim. Seven enumerated, six pinned is the kind of drift AD-78's own
|
||||||
|
"Risk" column warns about.
|
||||||
|
|
||||||
|
### SF-6 — the two page controllers do not cite AD-79 by id
|
||||||
|
|
||||||
|
`SocialFriendsPageController.cs:23-25` and `SocialSquelchPageController.cs:21-22`
|
||||||
|
both say "See this commit's single register row…". AD-79's own *Where* column
|
||||||
|
asserts "both classes' own doc comments cite this row". Literally true,
|
||||||
|
practically not: a future reader grepping `AD-79` will not find the code. Name
|
||||||
|
the row.
|
||||||
|
|
||||||
|
### SF-7 — the allegiance empty state is coarser than the contract's own source (item 4)
|
||||||
|
|
||||||
|
FA3's contract line is "all four pages' **empty states**", and lane A §4.5 is
|
||||||
|
what defines the allegiance one. I re-read and byte-verified that section's
|
||||||
|
claims because it is what the slice implements against:
|
||||||
|
|
||||||
|
- `gmAllegianceUI::UpdateMonarchData @0x00491B40` branches on
|
||||||
|
`(monarchId == player_id || monarchId == 0)` — retail hides the monarch block
|
||||||
|
when there is no monarch **or the monarch is you**, then sets **both**
|
||||||
|
`m_pMonarchName` *and* `m_pMonarchFollowers` to the literal at
|
||||||
|
`data_794358`.
|
||||||
|
- **`data_794358` byte-verified** in the PDB-paired binary
|
||||||
|
(`C:\Users\erikn\Downloads\acclient.exe`, image base `0x400000`, RVA
|
||||||
|
`0x394358` → file offset `0x394358`): bytes `20 00 00 00` = UTF-16LE `" "`,
|
||||||
|
one character. So lane A's `L" "` reading is correct and FA3's
|
||||||
|
`BlankLine = [new UiText.Line(" ", …)]`
|
||||||
|
(`SocialAllegiancePageController.cs:40-41`) matches it exactly — **a single
|
||||||
|
space, not an empty string**, as asserted at
|
||||||
|
`SocialPanelControllerTests.cs:177-178`. This half is clean.
|
||||||
|
|
||||||
|
What is coarser: FA3 gates **both** blocks on `Snapshot.HasProfile` rather than
|
||||||
|
per-relationship. The consequence is not hypothetical — the moment any
|
||||||
|
`0x0020 AllegianceUpdate` lands, a monarch character sees an **empty, visible**
|
||||||
|
monarch block that retail hides, and anyone whose patron is the monarch sees an
|
||||||
|
empty patron block instead of retail's relabelled-monarch presentation. FA2
|
||||||
|
already ships the data needed to do it properly:
|
||||||
|
`IRuntimeAllegianceView.TryGetMonarch` / `TryGetPatron`
|
||||||
|
(`GameRuntimeGameplayViews.cs:200-204`) plus `Snapshot.MonarchGuid`; the FA3
|
||||||
|
`Callbacks` record deliberately narrows the input to
|
||||||
|
`Func<RuntimeAllegianceSnapshot>` (`SocialPanelController.cs:90-96`), which is
|
||||||
|
what forecloses it.
|
||||||
|
|
||||||
|
I am **not** calling this a MUST-FIX: FA3 is a shell slice, the class doc is
|
||||||
|
honest about the simplification, and MF-2's finding means `HasProfile` is
|
||||||
|
effectively always `false` for the whole FA3 gate, so nothing wrong is visible
|
||||||
|
*during this gate*. But it is a contract gap that will be invisible by the time
|
||||||
|
FA5 lands unless it is written down. Either widen the callback to the view and
|
||||||
|
implement the real per-relationship gate now, or add an explicit FA5 acceptance
|
||||||
|
line ("the monarch block hides when the monarch is you; the patron block hides
|
||||||
|
when the patron is the monarch") to the plan's FA5 row so it cannot be lost.
|
||||||
|
|
||||||
|
Related, and worth a line in the class doc: `Tick()` reassigns
|
||||||
|
`LinesProvider` **unconditionally every frame**, so any FA5 code that sets a
|
||||||
|
real monarch/patron name on `0x10000257`/`0x1000025C` will be overwritten on
|
||||||
|
the next frame unless this method is changed at the same time.
|
||||||
|
|
||||||
|
### SF-8 — `Tick()` ignores `_disposed`
|
||||||
|
|
||||||
|
`SocialPanelController.Dispose()` (`:224-228`) sets `_disposed` and nothing
|
||||||
|
reads it. The handle passes `Controller = controller` to
|
||||||
|
`RetailWindowFrame.Mount`, and `RetailUiRuntime.SocialPanelController` is never
|
||||||
|
nulled, so `RetailUiRuntime.Tick`'s `SocialPanelController?.Tick()`
|
||||||
|
(`RetailUiRuntime.cs:559`) will keep ticking the four page controllers after
|
||||||
|
disposal. Nothing throws today (the Runtime owners outlive the panel and the
|
||||||
|
J-owner snapshot getters are disposed-guarded), so this is low severity — but
|
||||||
|
"tick a disposed controller" is precisely the class the J-slices spent commits
|
||||||
|
eliminating. One `if (_disposed) return;` closes it.
|
||||||
|
|
||||||
|
### SF-9 — `SocialPanelRowText.FindDeepest` documents a stronger guarantee than it implements
|
||||||
|
|
||||||
|
`SocialPanelRowText.cs:19-31` promises "the deepest `UiText` descendant
|
||||||
|
(pre-order, last match wins)". The implementation returns the **last match in
|
||||||
|
traversal order**, which equals the deepest only when the subtree is a single
|
||||||
|
chain. Both real templates *are* single chains — I dumped them from the
|
||||||
|
installed DATs:
|
||||||
|
|
||||||
|
```
|
||||||
|
FriendsRow 0x10000519 Type 3 (270x24) -> 0x1000051A Type 0xC -> 0x1000051F Type 0xC
|
||||||
|
SquelchRow 0x10000541 Type 3 (270x24) -> 0x10000542 Type 0xC -> 0x1000054D Type 0xC
|
||||||
|
```
|
||||||
|
|
||||||
|
so the behaviour is correct today and the class doc's honesty about
|
||||||
|
`gmFriendsUI`/`gmSquelchUI` being outside the campaign's decompiled scope is
|
||||||
|
exactly the right posture. Just make the doc describe what the code does, or
|
||||||
|
make the code track depth.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. What verified clean
|
||||||
|
|
||||||
|
Stated plainly, because most of this slice is right.
|
||||||
|
|
||||||
|
**The tab-table correction (item 1).** Confirmed three independent ways
|
||||||
|
(§1 above). The addendum in `d7e1cffd` is accurate, and correcting in place
|
||||||
|
while leaving the original text visible follows the FA1/FA2 convention.
|
||||||
|
|
||||||
|
**§6 Campaign-OP lessons compliance (item 3).** Each one re-derived:
|
||||||
|
|
||||||
|
- *String resolver on every `Build`.* `MountSocialPanel` passes
|
||||||
|
`resolver.Resolve` to the main `LayoutImporter.Build`
|
||||||
|
(`RetailUiRuntime.cs:2611-2617`) **and** its nested `TemplateResolver`
|
||||||
|
builds each row template with its own `strings.Resolve`
|
||||||
|
(`:2626-2645`). Both covered — the #375 class. The probe's non-empty-caption
|
||||||
|
assertion is the live check, and it passes.
|
||||||
|
- *Scoped `FindDescendant` for every repeating id.* I enumerated **all**
|
||||||
|
duplicate element ids in the panel subtree rather than trusting the stated
|
||||||
|
one. Duplicates exist at `0x10000439`/`0x100000E9`/`0x10000215` (×4, tab
|
||||||
|
button art), `0x100002CE`–`0x100002D0` (×16, button 3-slice art),
|
||||||
|
`0x10000328` (×6, checkbox art), `0x00000001`/`0x10000071`/`0x10000072`/
|
||||||
|
`0x10000364`–`0x10000366` (×4, scrollbar parts), `0x10000490`/`0x10000491`/
|
||||||
|
`0x10000492` (×2, the two allegiance blocks), and — not previously called
|
||||||
|
out anywhere — **`0x10000254`, `0x1000025D` and `0x1000051E` repeat ACROSS
|
||||||
|
pages** (`0x10000254` under both the Friends page and the allegiance monarch
|
||||||
|
block; `0x1000025D` under the Friends page and the patron block;
|
||||||
|
`0x1000051E` under both Friends and Squelch). Every FA3 lookup is safe:
|
||||||
|
the four page ids, the two fellowship frames, both allegiance field
|
||||||
|
containers, both name texts and both ListBox ids are each **unique**, the
|
||||||
|
per-page controllers scope to their page root anyway, and the single flat
|
||||||
|
`layout.FindElement(0x10000290)` for the close button targets a unique id.
|
||||||
|
`0x10000492` is not looked up at all in FA3, so the intra-panel duplication
|
||||||
|
cannot bite yet — but the three cross-page duplicates above are worth
|
||||||
|
carrying into FA5's notes, since FA5 will be resolving allegiance-page ids.
|
||||||
|
- *Tab activation performed.* `controller.ActivateTabs()` at
|
||||||
|
`RetailUiRuntime.cs:2662`, after `Bind` — the OP3 ordering constraint.
|
||||||
|
Verified live: `UnresolvedEntries` is empty and `IsShowingAllegiance` is true
|
||||||
|
after activation.
|
||||||
|
- *No lazily-created fill-anchored 0×0 children (the #372 class).* The new
|
||||||
|
controllers create no elements at all. The only lazy child is
|
||||||
|
`UiTemplateListBox`'s viewport, which already seeds `Width`/`Height` from the
|
||||||
|
ListBox's authored extent (`UiTemplateListBox.cs:152-182`, the #372 fix), and
|
||||||
|
both social ListBoxes carry real authored extents (Friends `270x400`, Squelch
|
||||||
|
`270x430`) at `Bind` time.
|
||||||
|
- *Same-layout template-prototype skip.* Checked for this layout specifically:
|
||||||
|
all four `P0x64` arrays point at **other** LayoutDescs — Friends
|
||||||
|
`0x2100005D/0x10000519`, Squelch `0x21000060/0x10000541`, fellows
|
||||||
|
`0x21000030/0x10000281`, vassals `0x2100002F/0x10000266`. The importer's skip
|
||||||
|
is same-LAYOUT only (`LayoutImporter.cs:233-249`), and in any case
|
||||||
|
`MountSocialPanel` uses the single-root `ImportInfos(dats, layoutId,
|
||||||
|
rootElementId)` overload, which never enumerates top-level siblings. No
|
||||||
|
parked prototypes are possible here.
|
||||||
|
- *No hand-rolled scroll viewport.* Both lists ride the existing
|
||||||
|
`UiScrollablePanel` through `UiTemplateListBox`, so the #371 straddling-row
|
||||||
|
clip comes for free. Popup/pointer-priority rules are not applicable.
|
||||||
|
|
||||||
|
**`UiTemplateListBox.Flush()`.** The doc claim "resetting `ContentHeight` to 0"
|
||||||
|
is accurate: `UiScrollablePanel.ClearContent()` (`:54-61`) removes every child,
|
||||||
|
clears `_baseTops`, sets `ContentHeight = 0` and resets the scroll offset. The
|
||||||
|
dormancy guard (`_viewport?.`) is genuine — a never-populated box still never
|
||||||
|
allocates its viewport. The shrink case the Friends/Squelch rebuild needs
|
||||||
|
therefore works.
|
||||||
|
|
||||||
|
**Fellowship empty-state frame swap (item 4).** Retail anchor re-read at
|
||||||
|
`gmFellowshipUI::Update @0x0048F440`: when
|
||||||
|
`ClientFellowshipSystem::GetFellowshipSystem()->m_pFellowship == 0` it calls
|
||||||
|
`m_pInAFellowshipFrame->SetVisible(0)` then
|
||||||
|
`m_pNotInAFellowshipFrame->SetVisible(1)` — exactly the two writes
|
||||||
|
`SocialFellowshipPageController.Tick` makes, driven off
|
||||||
|
`RuntimeFellowshipSnapshot.IsInFellowship`. Lane-A unknown **U6 is genuinely
|
||||||
|
closed**: page `0x10000292` has **exactly two children**, the two frames, so a
|
||||||
|
single `Visible` toggle per frame really is the whole swap. (Frame contents:
|
||||||
|
`0x1000026B` = name box `0x1000026F`, Create `0x10000274`, four checkboxes;
|
||||||
|
`0x10000275` = roster ListBox `0x10000279`, scrollbar, six buttons
|
||||||
|
`0x1000027B`–`0x10000280`.)
|
||||||
|
|
||||||
|
**Friends/Squelch read-only binding (item 5).** Bound to the J4.1 owners by
|
||||||
|
reference — `RuntimeCommunicationState.Friends`/`.Squelch` are get-only
|
||||||
|
properties constructed once (`RuntimeCommunicationState.cs:67-68,132-133`) and
|
||||||
|
**cleared in place** at reset (`:250-251`), so holding the instance across a
|
||||||
|
generation reset is correct, and `FriendsState.Clear()` bumps `Revision`
|
||||||
|
(`FriendsState.cs:67-74`) so the panel rebuilds to empty on its own. No new
|
||||||
|
state copies; `Snapshot()`'s array copy only runs on a revision change. The
|
||||||
|
per-frame cost when nothing changed is one `Interlocked.Read` per list. The
|
||||||
|
D1 INERT contract holds: checkboxes build as `UiButton` with a null `OnClick`
|
||||||
|
(no latching visual state without a handler), and the six buttons are pinned
|
||||||
|
null by test.
|
||||||
|
|
||||||
|
**Panel exclusivity + catalog (item 6).** `SocialPanel = 12` is byte-verified
|
||||||
|
(root `P0x10000029 = 12`). Registered in `Mounted` only. The toolbar seam
|
||||||
|
genuinely tolerates it: `SyncToolbarWindowButtons` iterates `ToolbarPanels`
|
||||||
|
only (`RetailUiRuntime.cs:809-814`), so no phantom button; `BindPanelButtons`
|
||||||
|
binds by authored toolbar element, and the toolbar authors no `0x10000029 = 12`
|
||||||
|
button; and the one path that *does* reach the toolbar with a non-toolbar id —
|
||||||
|
`OnWindowVisibilityChanged` → `ToolbarController.SetPanelOpen(12, …)`
|
||||||
|
(`:831-832`) — walks `_panelButtons`, finds nothing, and returns
|
||||||
|
(`ToolbarController.cs:394-406`). This is the same path `PositiveEffects` (4),
|
||||||
|
`LinkStatus` (8), `MiniGame` (9) and `Vitae` (15) already take. No crash, no
|
||||||
|
greyed phantom. Both open paths keep `RetailPanelUiController` state coherent:
|
||||||
|
`ToggleWindow` and `CloseWindow` both route through `TryGetPanelId` →
|
||||||
|
`_panelUi` (`:708-711`, `:801-807`).
|
||||||
|
|
||||||
|
**Window-frame policy.** `RetailWindowFrame.Options` for the social panel is
|
||||||
|
byte-for-byte the Options panel's post-review policy (ResizeX=false,
|
||||||
|
bottom-edge-only, both constrain flags, full ContentAnchors,
|
||||||
|
`DrawChromeCenter = !AuthorsFullPanelCenter(rootInfo)`), differing only in
|
||||||
|
`Left`/`Top`. No invented divergence in the shared main-panel cohort. The
|
||||||
|
root/page height mismatch (root `300x362`, pages `300x575`) is **identical** to
|
||||||
|
the already-gated Options panel's own authored shape, so it is not an FA3
|
||||||
|
property.
|
||||||
|
|
||||||
|
**The live-mount probe run (item 7).** Executed with
|
||||||
|
`ACDREAM_PROBE_LIVE_MOUNT=1` against the installed DATs: **1 passed, 0
|
||||||
|
failed**. Every assertion holds. Given #383 (installed-DAT vs committed-fixture
|
||||||
|
drift), I also cross-checked the committed social-panel fixture against the
|
||||||
|
probe's live dump on root extent, child count and order, the tab table, all
|
||||||
|
four page `P0x57` values, and both allegiance blocks' geometry (including the
|
||||||
|
two *differently sized* `0x10000490` instances, monarch `(210,0 90x36)` vs
|
||||||
|
patron `(220,0 80x36)`). **No drift** — this fixture is faithful to the
|
||||||
|
installed DAT.
|
||||||
|
|
||||||
|
**Tests.** `SocialPanelControllerTests` + `RetailPanelCatalogTests`: 22 passed.
|
||||||
|
Full `AcDream.App.Tests` on the committed Release binaries: **4,871 passed / 3
|
||||||
|
skipped / 0 failed** — no collateral regressions. The ledger's `+18` arithmetic
|
||||||
|
checks out exactly (13 `SocialPanelControllerTests` + 4 `RetailPanelCatalogTests`
|
||||||
|
+ 1 probe). I did not re-run the full 13,233-test solution suite (no builds
|
||||||
|
permitted in this review), so that figure is unverified but arithmetically
|
||||||
|
consistent.
|
||||||
|
|
||||||
|
**Register bookkeeping.** AD-79's header count is right — the table contains
|
||||||
|
exactly 59 `AD-` rows, matching "59 active rows", up one from 58. The row's
|
||||||
|
element enumeration is accurate against the fixture (Friends `0x10000514`/
|
||||||
|
`0x10000515`/`0x10000516` + `0x1000052C`; Squelch `0x10000547`/`0x1000054B`/
|
||||||
|
`0x1000054C`), and its honesty about `gmFriendsUI`/`gmSquelchUI` being outside
|
||||||
|
the campaign's researched scope is the right framing. Only SF-5/SF-6 above.
|
||||||
|
|
||||||
|
**Gate-script spot checks (item 8).** Steps 1–3 (F3 opens on Allegiance; repeat
|
||||||
|
F3 closes; F4 opens directly on Fellowship), step 4 (F3 while open on
|
||||||
|
Fellowship switches rather than closes) and step 5 (the authored X at
|
||||||
|
`0x10000290`, a direct root child at `(276,0) 24x25`, always visible because it
|
||||||
|
is not inside any page, wired to `ToggleWindow`) all match
|
||||||
|
`RetailUiRuntime.OpenSocialPanel` (`:603-620`) and
|
||||||
|
`SocialPanelController.Bind`'s close wiring (`:145-150`) exactly. Steps 7–8's
|
||||||
|
exclusivity claims match `RetailPanelUiController.SetPanelVisibility`'s
|
||||||
|
one-active-panel logic. Step 6's geometry claims match the frame options. The
|
||||||
|
two claims that do **not** hold are MF-1 and MF-2. One wording nit for whoever
|
||||||
|
edits the script: step 5 calls the X "top-right of the window chrome" when it
|
||||||
|
is an authored panel element, not chrome — harmless, but a user hunting the
|
||||||
|
chrome's own close affordance may be briefly confused.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. Disposition
|
||||||
|
|
||||||
|
APPROVE-WITH-FIXES. MF-1 and MF-2 must land before the user runs the gate —
|
||||||
|
both are in `docs/research/2026-08-12-campaign-fa-test-script.md` and neither
|
||||||
|
touches code. SF-1 through SF-9 are code/doc/test corrections that can ride the
|
||||||
|
same fix round. Nothing here warrants a re-review of the mount path itself: the
|
||||||
|
tab table, the frame swap, the string resolver, the scoped lookups, the tab
|
||||||
|
activation, the catalog registration and the toolbar seam are all correct, and
|
||||||
|
the live-mount probe passes against the installed DATs.
|
||||||
Loading…
Add table
Add a link
Reference in a new issue