docs: FA4 mechanism review -- APPROVE-WITH-FIXES (5 MUST-FIX, 8 SHOULD-FIX)
Findings persisted before any fixer dispatch, per the campaign's §7 review protocol. MUST-FIX, in the order they were derived: 1. D5's percentage conversion rounds where retail truncates. Byte-decoded 0x0048ECC9..0x0048ECD8 (fld pct; fmul [0x007A5170]=100.0f; call _ftol2 @0x005DE394 -- the fld/fst/fistp/fild truncation dance), so a 6-fellow roster displays 45% where retail shows 44%, and 8 fellows shows 35% vs 34%. The table itself IS byte-exact; only the ->int conversion diverges, and it is not fixable by a plain cast because 0.45f*100f already rounds up to 45.0f in single precision. 2. D6's client-side invite intercept has no retail anchor. Read in full: Handle_Character__ConfirmationRequest @0x005640A0 (bare jump table), RecvNotice_FellowshipRequest @0x00490880, MakeFellowRequestDialog @0x00490620 (only guard is m_fellowRequestContext), plus a whole-file sweep of both option accessors -- zero reads on any confirmation path. The code comment cites ACE's Fellowship.cs as "retail". ACE filters both bits server-side, so the intercept is dead against a correct server and harmful against a drifting one -- and IgnoreFellowshipRequests defaults to TRUE client-side. 3. D4 never re-declares 0x00A6 after a generation reset: the edge- triggered _pageVisible latch survives reconnect, so fellow vitals stay frozen for the whole new session. ResetSessionTransientUi is the seam. 4. gmFellowshipUI::UpdateFellowSelection @0x0048F0F0 is not ported -- selecting a fellow in the WORLD leaves Dismiss/Leader disabled and no row ever shows selected; the plan's contracted UiTemplateListBox selection model + 0x1000000D row instance-id were not added. 5. Three shipped deviations have no register row (invite intercept, gold leader tint, name-text-only row selection); the Recruit is-a-player gate's "inline comment, not a register row" call is also wrong. Re-derived rather than trusted: the live-mount probe was re-run against the installed DATs (every ledger element/string claim CONFIRMED, Bind() warning-free), the GetEvenSplitXPPctg table was byte-read from the PDB-paired binary, FlushPreservingScroll's shrink semantics were traced through UiScrollablePanel/UiScrollable (sound), and the five touched test classes pass 93/93. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
6849b45771
commit
913e35cdb5
1 changed files with 472 additions and 231 deletions
|
|
@ -1,263 +1,504 @@
|
||||||
# FA4 — mechanism-faithfulness review
|
# Campaign FA slice FA4 — MECHANISM-FAITHFULNESS review
|
||||||
|
|
||||||
**Slice:** Campaign FA slice FA4 — the Fellowship page fully live.
|
**Reviewer lens:** mechanism faithfulness (does the shipped code do what
|
||||||
**Targets:** `357d2032` (Runtime), `5bdd0528` (App+tests), `38f08314` (docs).
|
retail does, for the reasons retail does it). Sibling lens: regression /
|
||||||
**Reviewer lens:** mechanism-faithfulness (does the code do what retail does,
|
blast radius (separate doc).
|
||||||
byte-for-byte where a byte exists, and is every divergence honestly filed).
|
|
||||||
**Method:** re-derived from the code, the lane docs
|
**Target:** `357d2032` (Runtime), `5bdd0528` (App + tests), `38f08314`
|
||||||
(`2026-08-11-fa-fellowship-wire.md`, `-fa-panel-structure.md`), the
|
(docs) against `docs/plans/2026-08-11-fellowship-allegiance-campaign.md`
|
||||||
PDB symbol table (`docs/research/named-retail/symbols.json`), and a LIVE run
|
slice FA4 + D4/D5/D6/D7 + the four FA3 re-review carry-forwards, with
|
||||||
of the env-gated mount probe against the real installed DATs.
|
`docs/research/2026-08-11-fa-fellowship-wire.md` (lane B) and
|
||||||
|
`-fa-panel-structure.md` (lane A) as ground truth.
|
||||||
|
|
||||||
|
**Verdict: APPROVE-WITH-FIXES — 5 MUST-FIX, 8 SHOULD-FIX, 3 NIT.**
|
||||||
|
|
||||||
|
The slice is real work and most of it is faithful: the button-enable
|
||||||
|
table is a verbatim port, the even-split *table* is byte-exact, the
|
||||||
|
roster diff genuinely never rebuilds on a vitals tick, the create-flow
|
||||||
|
gating matches retail's "the button IS the refusal mechanism", and the
|
||||||
|
live-DAT probe (re-run by this review, below) substantiates every
|
||||||
|
element-resolution claim in the ledger. The MUST-FIXes are not polish:
|
||||||
|
two of them are behaviors retail's binary demonstrably does *not* have
|
||||||
|
(D6's invite intercept, D5's rounding), one is a mechanism retail *does*
|
||||||
|
have and this slice did not port (world→panel selection sync), one is a
|
||||||
|
lifecycle hole that silently disables the very stream D4 exists to turn
|
||||||
|
on, and one is the register rule.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## VERDICT: APPROVE-WITH-FIXES
|
## 0. What this review re-derived (not trusted)
|
||||||
|
|
||||||
One MUST-FIX (a doc-only register omission), three SHOULD-FIX, two NITs. No
|
| Claim | How it was checked | Result |
|
||||||
code-behavior REJECT: every wire-touching mechanism this slice ships is
|
|---|---|---|
|
||||||
retail-faithful and the divergences it makes are (with the one exception
|
| Live-DAT mount/element/string claims in the FA4 ledger row | Re-ran `ACDREAM_PROBE_LIVE_MOUNT=1 dotnet test --no-build -c Release --filter SocialPanelLiveMountProbeTests` against the installed DATs | **CONFIRMED.** `0x1000026F` builds as `UiField`; all 11 buttons/checkboxes as `UiButton`; ListBox `0x10000279` templates=1 `0x21000030/0x10000281`, scrollbar `0x1000027A`; all 5 row fields at the right widget types; labels resolve ("Ignore Fellowship Requests", "Automatically Accept Fellowship Requests", "Share Fellowship Experience and Luminance", "Share Fellowship Loot"); captions resolve `'Open'`/`'Close'`; production `Bind()` console output **empty** |
|
||||||
below) honestly registered.
|
| The frame-containment "structural finding" | Same probe dump | **CONFIRMED and EXTENDED** — `0x1000026B` children are `0x10000272,0x10000273,0x10000274,0x1000026C..0x10000271`: the Create button, the name field **and all four checkboxes** live in the NOT-in-fellowship frame. See SHOULD-FIX 8 for the consequence the ledger missed |
|
||||||
|
| `GetEvenSplitXPPctg` table | Byte-read `.rdata` of the PDB-paired `acclient.exe` at the ten VAs lane B §7.2 names | **CONFIRMED verbatim**, including `0x007E72B8 = 0.31111109256744385` and default `0.0` |
|
||||||
**Strongest single piece of evidence:** the live-mount probe
|
| How retail converts that float to the displayed integer | Byte-decoded `0x0048ECC9..0x0048ECD8` + the callee at `0x005DE394` | **REFUTES the port** — see MUST-FIX 1 |
|
||||||
(`ACDREAM_PROBE_LIVE_MOUNT=1`, real installed DATs, not a fixture) **PASSES**
|
| Whether retail's client reads the two fellowship option bits on the invite path | Read `gmFellowshipUI::RecvNotice_FellowshipRequest @0x00490880`, `MakeFellowRequestDialog @0x00490620`, `ClientUISystem::Handle_Character__ConfirmationRequest @0x005640A0` in full + swept every `IgnoreFellowshipRequests`/`FellowshipAutoAcceptRequests` occurrence in the 2013 pseudo-C | **REFUTES D6's invite half** — see MUST-FIX 2 |
|
||||||
(1/1). It confirms, end-to-end against the production mount, that the name
|
| The 93 tests in the five touched test classes | `dotnet test --no-build -c Release --filter` | 93 passed / 0 failed |
|
||||||
field builds as `UiField`, all 11 buttons/checkboxes resolve as `UiButton`,
|
|
||||||
the sole row template `0x21000030/0x10000281` resolves through the production
|
|
||||||
`RowTemplateResolver` with all five checked fields at the right widget types
|
|
||||||
(`UiText`/`UiText`/`UiMeter`/`UiMeter`/`UiMeter`), the four checkbox labels
|
|
||||||
resolve to real retail English, the Open/Close captions resolve to
|
|
||||||
`"Open"`/`"Close"`, and a full production-path `Bind()` emits zero
|
|
||||||
"not found" warnings. FA4 test suites also pass under `--no-build`:
|
|
||||||
`SocialFellowshipPageController`/`RowTemplateResolver`/`UiTemplateListBoxFlushPreservingScroll`/`CharacterOptions`
|
|
||||||
= 75/75; `RuntimeFellowshipStateTests` = 28/28; probe = 1/1.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## What verified CLEAN (re-derived, not trusted)
|
## MUST-FIX 1 — D5's percentage conversion rounds where retail truncates: 6- and 8-fellow rows display the wrong number
|
||||||
|
|
||||||
### D4 — the `0x00A6` gate (the highest-stakes mechanism)
|
`SocialFellowshipPageController.cs:650`
|
||||||
The five contracted transitions all resolve correctly. The conjunction is
|
|
||||||
`_fellowship?.SetPageVisible(_visible && IsShowingFellowship)`
|
|
||||||
(`SocialPanelController.cs:267-268`), driven from three sources:
|
|
||||||
`ActivePageChanged` (ctor `:140`), `OnShown` (`:250-254`), `OnHidden`
|
|
||||||
(`:258-262`). `SetPageVisible` sends `0x00A6` only on an actual flip
|
|
||||||
(`SocialFellowshipPageController.cs:453-458`, `if (_pageVisible == visible) return;`).
|
|
||||||
|
|
||||||
- **open-on-fellowship:** `OnShown` → `_visible=true` ∧ `IsShowingFellowship=true`
|
```csharp
|
||||||
→ `SetPageVisible(true)` → one `true` send. ✓
|
return $"{member.Level} {(int)MathF.Round(pct * 100f)}%";
|
||||||
- **tab-away:** `ActivePageChanged` → `_visible=true` ∧ `false`
|
```
|
||||||
→ `SetPageVisible(false)` (flip from true) → one `false` send. ✓
|
|
||||||
- **tab-back:** → `SetPageVisible(true)` → one `true` send. ✓
|
|
||||||
- **close-while-on-fellowship:** `OnHidden` → `_visible=false`
|
|
||||||
→ `SetPageVisible(false)` → one `false` send. ✓
|
|
||||||
- **close-while-on-other-tab:** `_pageVisible` already `false` (we left the
|
|
||||||
Fellowship tab earlier), `OnHidden` → `SetPageVisible(false)` → no-op, **no
|
|
||||||
redundant send**. ✓
|
|
||||||
- **switch between two non-Fellowship tabs while shown:**
|
|
||||||
`SetPageVisible(false)` while already `false` → no spurious send. ✓
|
|
||||||
|
|
||||||
The default authored tab is Allegiance (probe: `button=0x1000028C
|
The class doc and the commit message both claim "The percentage itself IS
|
||||||
page=0x10000291 default=True`), so `ActivateTabs()` at mount time
|
retail's exact number". The *table* is exact (byte-verified this review).
|
||||||
(`RetailUiRuntime.cs:2775`, before the window is shown with `Visible=false`)
|
The **conversion is not**.
|
||||||
fires `ActivePageChanged` for Allegiance while `_visible=false` → no spurious
|
|
||||||
`0x00A6` at mount. ✓
|
|
||||||
|
|
||||||
**No-send-while-disconnected** verified at the command layer: `Invoke`
|
Retail, byte-decoded from the PDB-paired binary at
|
||||||
returns `RuntimeCommandStatus.Inactive` without dispatching when
|
`gmFellowshipUI::UpdateFellowStats`:
|
||||||
`_deactivated || _view is null || _commands is null`
|
|
||||||
(`InteractionUiRuntimeSources.cs:181-186`); `FellowshipSetPanelOpen` just
|
|
||||||
forwards through it (`:157-159`). **Idempotence** verified twice (the flip
|
|
||||||
guard + the pinning test `SetPageVisible_SendsPanelOpen_OnlyOnATransition`,
|
|
||||||
`SocialFellowshipPageControllerTests.cs:537-554`).
|
|
||||||
|
|
||||||
### Leader-quit rule (FA2 hand-off not bypassed)
|
```
|
||||||
FA4's Quit routes `_bindings.Quit(disband)` →
|
0048ECC9 d9 44 24 10 fld dword [esp+0x10] ; pct (float)
|
||||||
`late.GameRuntime.FellowshipQuit` → `commands.Fellowship.Quit`
|
0048ECCD d8 0d 70 51 7a 00 fmul dword [0x007A5170] ; = 100.0f (byte-read)
|
||||||
(`InteractionUiRuntimeSources.cs:138-140`), whose adapter body sends
|
0048ECD3 e8 bc f6 14 00 call 0x005DE394 ; _ftol2
|
||||||
`0x0290 SendFellowshipAssignNewLeader` BEFORE `0x00A3 SendFellowshipQuit`
|
```
|
||||||
when `RequiresLeaderHandoffBeforeQuit` returns true
|
|
||||||
(`DirectGameRuntimeCommandAdapter.cs:888-895`, mirrored
|
|
||||||
`CurrentGameRuntimeCommandAdapter.cs:883`). No raw `WorldSession.Send*`
|
|
||||||
bypass exists in the UI layer. The FA2 rule itself is correct: fires only
|
|
||||||
when `!disband && _isInFellowship && _leaderGuid == selfGuid`, picks the
|
|
||||||
first non-self member (`RuntimeFellowshipState.cs:292-313`). ✓
|
|
||||||
|
|
||||||
### D5 — the XP-share table
|
`0x005DE394` is MSVC's `_ftol2` (`55 8b ec 83 ec 20 83 e4 f0 d9 c0 d9 54
|
||||||
`EvenSplitPercentTable = [1.0, 0.75, 0.6, 0.55, 0.5, 0.45, 0.4, 0.35,
|
24 18 df 7c 24 10 df 6c 24 10 …` — the fld/fst/fistp/fild + correction
|
||||||
0.3111111, 0.28]` (`SocialFellowshipPageController.cs:103-104`) reproduces
|
dance whose whole purpose is to turn the FPU's round-to-nearest `fistp`
|
||||||
lane B §7.2's byte-decoded `.rdata` floats **exactly**, including the
|
into **C truncation toward zero**). The product is formed on the x87
|
||||||
out-of-range default `0.0` (`EvenSplitPercent`, `:655-656`, returns `0f` for
|
stack from two floats, so it is exact; `_ftol2` then chops it.
|
||||||
`memberCount` outside `[1,10]`). Branch structure matches
|
|
||||||
`gmFellowshipUI::UpdateFellowStats` §2.9 verbatim (`FormatStatsText`,
|
|
||||||
`:643-653`): `!ShareXp` → `0%` (retail's literal `pct=0.0f` branch),
|
|
||||||
even-split → table, proportional → **omitted** (level only) rather than a
|
|
||||||
guessed formula — the honest, CLAUDE.md-compliant disposition. AD-80
|
|
||||||
accurately states the ACE display divergence (`.3` vs `.3111111` at 9, no
|
|
||||||
10-fellow row, wrong `1.0` out-of-range default) as ACE-vs-retail, not
|
|
||||||
acdream-vs-retail. ✓
|
|
||||||
|
|
||||||
### D6 — invite auto-response
|
Per-entry comparison (float bits read from the binary; acdream column is
|
||||||
`HandleConfirmationRequest` intercepts type-4 (`ConfirmationType.Fellowship=4`,
|
the shipped expression):
|
||||||
`GameEvents.cs:580`) BEFORE the generic `GameplayConfirmationController`
|
|
||||||
(`RetailUiRuntime.cs:677-683`). `TryAutoRespondToFellowshipInvite` (`:685-693`)
|
|
||||||
reads both bits and sends `SendConfirmationResponse(type, ctx, autoAccept)`
|
|
||||||
(the shared confirmation-response primitive,
|
|
||||||
`InteractionRetainedUiComposition.cs:807-812`) — decline when only Ignore is
|
|
||||||
set, accept when AutoAccept is set. The mutual exclusion IS Runtime's:
|
|
||||||
`RuntimeCharacterState.TrySetOption` clears the opposite bit when either is
|
|
||||||
turned on (`:821-834`), so both-set is unreachable through the write path.
|
|
||||||
Neither bit set → `return false` → falls through unchanged. ✓
|
|
||||||
|
|
||||||
### D7 — un-dims
|
| n | retail float | retail `trunc(pct×100)` | acdream `(int)MathF.Round(pct*100f)` |
|
||||||
Exactly four rows flip `StoreOnly`→`Live`
|
|--:|---|--:|--:|
|
||||||
(`CharacterOptionsPageController.cs:206-208`): `IgnoreFellowshipRequests`,
|
| 1 | 1.0 | 100 | 100 |
|
||||||
`FellowshipAutoAcceptRequests`, `FellowshipShareXP`, `FellowshipShareLoot`.
|
| 2 | 0.75 | 75 | 75 |
|
||||||
Each has a live read site: the first two in
|
| 3 | 0.6000000238418579 | 60 | 60 |
|
||||||
`TryAutoRespondToFellowshipInvite`; `ShareXP` at the Create-button click
|
| 4 | 0.550000011920929 | 55 | 55 |
|
||||||
(`SocialFellowshipPageController.cs:325`) and its own checkbox;
|
| 5 | 0.5 | 50 | 50 |
|
||||||
`ShareLoot` via its own live checkbox surface (`RefreshCheckboxSelections`
|
| **6** | **0.44999998807907104** | **44** | **45** ❌ |
|
||||||
`:484-486`). `IgnoreAllegianceRequests`/`DisplayAllegianceLogonNotifications`
|
| 7 | 0.4000000059604645 | 40 | 40 |
|
||||||
stay `StoreOnly` (`:206`/`:208`). Conformance test updated 35→31 store-only /
|
| **8** | **0.3499999940395355** | **34** | **35** ❌ |
|
||||||
15→19 live with the exact four ids removed from `ExpectedStoreOnlyIds`
|
| 9 | 0.31111109256744385 | 31 | 31 |
|
||||||
(`CharacterOptionsPageControllerTests.cs:729-733`, `:762-765`). ✓
|
| 10 | 0.2800000011920929 | 28 | 28 |
|
||||||
|
|
||||||
### Roster scroll preservation
|
Two of the ten roster sizes are wrong. Note the failure is not fixed by
|
||||||
`FlushPreservingScroll` (`UiTemplateListBox.cs:290-296`) saves `ScrollY`,
|
swapping `MathF.Round` for a cast: `0.45f * 100f` **already rounds up to
|
||||||
`ClearContent()`s, restores `ScrollY`. It genuinely preserves across a
|
exactly `45.0f`** in single precision, so `(int)(pct * 100f)` still
|
||||||
membership-change rebuild because `ClearContent` zeroes only the *panel's*
|
yields 45. The product must be formed in a wider type, exactly as
|
||||||
`ContentHeight` and never syncs `Scroll.ContentHeight`
|
retail's x87 does:
|
||||||
(`UiScrollablePanel.cs:54-61`), so the restore's `SetScrollY` clamps against
|
|
||||||
the STALE (pre-rebuild) `MaxScroll` (`UiScrollable.cs:56`) — exactly as the
|
|
||||||
method's own doc claims. The next `LayoutScrollableChildren` re-clamps
|
|
||||||
against the fresh height. Tests exercise the real cases: preserve across
|
|
||||||
same-count rebuild, clamp-down on shorter content, dormant no-op
|
|
||||||
(`UiTemplateListBoxFlushPreservingScrollTests.cs:50-98`). The no-change path
|
|
||||||
(`UpdateRow`) only sets `LinesProvider`/meter delegates — no ListBox
|
|
||||||
mutation (`SocialFellowshipPageController.cs:607-627`). ✓
|
|
||||||
|
|
||||||
### Create-flow refusal-by-enabled-state
|
```csharp
|
||||||
`RefreshCreateButtonState` sets `Enabled = !IsNullOrWhiteSpace(name)`
|
return $"{member.Level} {(int)((double)pct * 100.0)}%";
|
||||||
(`:460-465`); the click handler re-guards (`:322-327`). Matches lane B §2.2
|
```
|
||||||
(`gmFellowshipUI::CreateFellowship` @0x0048F730): the button IS the guard,
|
|
||||||
no separate error text. `ShareXP` read from the character option at click
|
|
||||||
time (lane B row 67: "shareXP comes from option 0x0F"), not a dialog field. ✓
|
|
||||||
|
|
||||||
### Button-enable rules (§2.8 verbatim)
|
(`(double)0.44999998807907104 * 100.0 = 44.999998807907104` → 44 ✓;
|
||||||
`RefreshButtonStates` (`:510-537`) ports the `gmFellowshipUI::UpdateButtons`
|
`0.3499999940395355 * 100.0 = 34.99999940395355` → 34 ✓; every other row
|
||||||
@0x0048E6C0 table exactly: Quit always; Disband/Open `= isLeader`;
|
unchanged.)
|
||||||
Leader/Dismiss `= isLeader && hasSelection && !selectedIsSelf`; Recruit
|
|
||||||
`= targetValid && notFull && (isLeader || IsOpen)`. The one gap (Recruit does
|
|
||||||
not gate on "target is a player") is honestly disclosed as a
|
|
||||||
superset-of-retail rule in an inline comment (`:529-534`) with the server
|
|
||||||
refusing a non-player target the same way retail's click-handler no-ops.
|
|
||||||
|
|
||||||
### §6 compliance / anchors
|
**Also fix in the same commit:**
|
||||||
String resolver threaded to every `LayoutImporter.Build`
|
- `SocialFellowshipPageControllerTests.FormatStatsText_MatchesD5Rules`
|
||||||
(`RetailUiRuntime.cs:2714-2720`, the `RowTemplateResolver` build closure);
|
exercises only n=1/n=9/proportional — neither failing size is pinned.
|
||||||
scoped `FindDescendant(pageRoot,…)`/`FindDescendant(row,…)` throughout;
|
Add `[InlineData(true, true, 6, "12 44%")]` and
|
||||||
scrollbar id-0 guard (`:247-256`); `RowTemplateResolver` caches null misses
|
`[InlineData(true, true, 8, "12 34%")]`.
|
||||||
so a permanently-unresolvable template imports ONCE (`RowTemplateResolver.cs:63-72`),
|
- The gate script's §FA4 step 21 currently presents the 9-member ACE gap
|
||||||
and the revision latch advances before `RefreshRoster`
|
(AD-80) as the *only* percentage divergence a tester should tolerate,
|
||||||
(`SocialFellowshipPageController.cs:433-437`) so a partial resolver failure
|
which would prime them to accept the wrong 45% at six fellows as
|
||||||
never becomes a per-frame retry. Five retail anchors byte-verified in the
|
correct. Same class as FA3's own mechanism MF-1/MF-2 script
|
||||||
PDB: `0x0048E6C0`→`gmFellowshipUI::UpdateButtons`,
|
corrections.
|
||||||
`0x0048F730`→`::CreateFellowship`, `0x005B9070`→`Fellowship::IsFull`,
|
- AD-80's row text says acdream "renders retail's own byte-decoded XP-share
|
||||||
`0x005B9BA0`→`FellowshipSystem::GetEvenSplitXPPctg`,
|
table verbatim" — true of the table, false of the rendered integer until
|
||||||
`0x004901C0`→`gmFellowshipUI::ListenToElementMessage`,
|
this lands.
|
||||||
`0x0048EB20`→`::UpdateFellowStats`, `0x006A6380`→`CM_Fellowship::Event_UpdateRequest`
|
|
||||||
— all match their citations. `SelectionChangeSource.Social`
|
|
||||||
(`SelectionState.cs:21-26`) is anchored to `::ListenToElementMessage`'s
|
|
||||||
`SetSelectedObject` call and matches the shared selection primitive. Gate
|
|
||||||
script §FA4 claims all trace to the code (create gating, caption swap, button
|
|
||||||
states, D5 display, invite bits, leader hand-off). ✓
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Findings
|
## MUST-FIX 2 — D6's invite intercept is a client-side mechanism retail does not have, and it can silently swallow real invites
|
||||||
|
|
||||||
### MUST-FIX 1 — the leader-gold-tint adaptation has no divergence-register row
|
`RetailUiRuntime.cs:665-693`
|
||||||
`SocialFellowshipPageController` tints the fellowship LEADER's roster name a
|
|
||||||
distinct gold (`LeaderNameColor = (1, 0.84, 0, 1)`, applied in `UpdateRow`,
|
|
||||||
`SocialFellowshipPageController.cs:110`/`:614`). Its own class doc
|
|
||||||
(`:57-65`) is honest that lane A's row template has **no** dedicated leader
|
|
||||||
marker and retail "may simply not mark leadership in the row at all" — i.e.
|
|
||||||
this is an *invented, user-visible visual cue retail (probably) does not
|
|
||||||
have*. That is precisely the class of deviation the divergence register
|
|
||||||
exists to hold, and CLAUDE.md's binding rule is unambiguous: "Any commit that
|
|
||||||
introduces a deviation … adds its register row IN THE SAME COMMIT" / "A
|
|
||||||
deviation found without a row is a bug twice over." The FA4 docs commit filed
|
|
||||||
AD-80 (XP-share display) and AD-81 (the numeric-composite text gap) but
|
|
||||||
**neither covers the gold tint** — a register grep for `gold`/`tint` returns
|
|
||||||
only the unrelated AP-178 (SpewBox). This is exactly the same reasoning that
|
|
||||||
earned AD-78 its row ("caption colors retail never has → register row"). Fix:
|
|
||||||
add a one-line AD row (or fold a clause into AD-81) recording the leader
|
|
||||||
gold-tint adaptation and citing `SocialFellowshipPageController.cs:110`/`:614`.
|
|
||||||
*(Doc-only; the code behavior is a reasonable, honestly-code-documented
|
|
||||||
adaptation — the defect is purely the missing auditable row.)*
|
|
||||||
|
|
||||||
### SHOULD-FIX 2 — AD-78's "35 of 50 rows dimmed" parenthetical is now stale
|
```csharp
|
||||||
`38f08314`'s message claims "AD-78's derivation table gains its D7 addendum,"
|
if (request.Type == (uint)GameEvents.ConfirmationType.Fellowship
|
||||||
but the addendum landed in the CODE class-doc (in `5bdd0528`), while the
|
&& TryAutoRespondToFellowshipInvite(request))
|
||||||
**register row itself** was not touched: AD-78's "Where" column still reads
|
return true;
|
||||||
`CharacterOptionsPageController.cs (35 of 50 rows dimmed …)`
|
```
|
||||||
(`retail-divergence-register.md:175`) when the post-FA4 count is **31 of 50**.
|
|
||||||
The register is "the single auditable list"; a stale count in an active row
|
|
||||||
is a bookkeeping drift. Fix: update `35`→`31` in AD-78's Where column.
|
|
||||||
|
|
||||||
### SHOULD-FIX 3 — D4 is not re-armed across a generation reset (reconnect)
|
**(a) No retail anchor exists.** Read in full this review:
|
||||||
`0x00A6` is a per-session, server-latched declaration; `_pageVisible`
|
|
||||||
(App-side UI state) is NOT reset by a generation reset, and
|
|
||||||
`ResetSessionTransientUi` (`RetailUiRuntime.cs:731-736`) does not hide/close
|
|
||||||
the social panel. So if the Fellowship tab is open across a reconnect,
|
|
||||||
`_pageVisible` stays `true`, no `OnShown`/`OnHidden`/`ActivePageChanged`
|
|
||||||
transition fires, `SetPageVisible` is never re-called, and the fresh session
|
|
||||||
never receives `0x00A6`. Since `RuntimeFellowshipState.ResetSession` clears
|
|
||||||
the roster and ACE only sends `0x02BE`/`0x02C0` while `FellowshipPanelOpen`
|
|
||||||
(gated by `0x00A6`, lane B §4.5/row 78), the roster can present empty/frozen
|
|
||||||
after reconnect until the user manually toggles the tab. This is the exact
|
|
||||||
"missed true-send freezes the roster" failure the D4 contract warns about,
|
|
||||||
just on the reconnect edge (outside the five in-session transitions, which
|
|
||||||
are all correct). Recommend re-declaring on session reset (drive
|
|
||||||
`SetPageVisible` from the reset hook, or clear `_pageVisible` so the next
|
|
||||||
`Tick`/transition re-arms) — and confirm ACE's world-enter re-send behavior
|
|
||||||
before the connected gate closes. *(I could not establish ACE's exact
|
|
||||||
world-enter `0x02BE` behavior from the client decomp; flag it for the gate.)*
|
|
||||||
|
|
||||||
### SHOULD-FIX 4 — no panel-level test pins the D4 conjunction
|
- `ClientUISystem::Handle_Character__ConfirmationRequest @0x005640A0` —
|
||||||
The controller-level idempotence IS tested
|
a bare 7-way jump table; `case 4` is one call to
|
||||||
(`SetPageVisible_SendsPanelOpen_OnlyOnATransition`), but the panel-level
|
`CM_Fellowship::SendNotice_FellowshipRequest`. No option read.
|
||||||
wiring in `SocialPanelController` — `OnShown`/`OnHidden`/`SwitchTo` ×
|
- `gmFellowshipUI::RecvNotice_FellowshipRequest @0x00490880` — copies the
|
||||||
active-tab → the correct `SetPanelOpen` calls, including the "close on other
|
string, tail-calls `MakeFellowRequestDialog`. No option read.
|
||||||
tab sends nothing" and "tab-away sends false" cases — has **no** direct
|
- `gmFellowshipUI::MakeFellowRequestDialog @0x00490620` — its **only**
|
||||||
assertion. The callbacks record `fellowship-set-panel-open:{bool}`
|
guard is `if (this->m_fellowRequestContext == 0)` (`@0x00490635`), the
|
||||||
(`SocialPanelControllerTests.cs:51`) but nothing asserts on it. For the
|
one-dialog-at-a-time rule lane B §2.4 already documents. No option read.
|
||||||
highest-stakes mechanism, add a transition test that opens on Fellowship,
|
- A whole-file sweep of `IgnoreFellowshipRequests` /
|
||||||
tabs away/back, and closes on each tab, asserting the exact
|
`FellowshipAutoAcceptRequests` in `acclient_2013_pseudo_c.txt` returns
|
||||||
`set-panel-open` call sequence.
|
**only**: the two `PlayerOptionPage` label registrations
|
||||||
|
(`@0x004a0b56`/`@0x004a0d16`), an input-action toggle pair
|
||||||
|
(`@0x00561fbf`/`@0x005621ef`), `CPlayerModule::OnChanged`'s mutual
|
||||||
|
exclusion (`@0x0059a971`/`@0x0059a987`), and the `PlayerModule`
|
||||||
|
get/set switch arms (`@0x005d3aca`/`@0x005d3f11`/`@0x005d3c06`/
|
||||||
|
`@0x005d4031`). **Zero reads on any confirmation path.**
|
||||||
|
|
||||||
### NIT 5 — Open/Close caption does not optimistically pre-toggle
|
Retail's client shows the dialog unconditionally. This is the same shape
|
||||||
Lane B row 77 notes retail "pre-toggles its own `_open_fellow`" on the Open
|
as FA1's D9 and FA2's D2: the plan asserted a mechanism, primary source
|
||||||
button click, i.e. the caption flips immediately client-side, then the server
|
refutes it.
|
||||||
confirms. acdream's `RefreshOpenCaption` keys the caption off the
|
|
||||||
server-authoritative `snapshot.IsOpen` (`:495-508`), so the caption flips
|
|
||||||
only when the `0x02BE` echo lands. Functionally convergent (and instant on
|
|
||||||
localhost), but not byte-faithful to retail's optimistic local pre-toggle.
|
|
||||||
|
|
||||||
### NIT 6 — `MathF.Round` vs retail `_ftol2` truncation (non-issue, documented)
|
**(b) The code comment cites ACE server source as a retail anchor.**
|
||||||
`FormatStatsText` uses `(int)MathF.Round(pct*100f)` (`:650`) where retail's
|
`RetailUiRuntime.cs:669` reads "retail's `Fellowship.cs:121`-equivalent
|
||||||
`UpdateFellowStats` truncates via `_ftol2`. For every value in the even-split
|
client-side mirror". `Fellowship.cs` is `ACE.Server`. Lane B's own rows
|
||||||
table (×100 = 100/75/60/55/50/45/40/35/31.11/28) round and truncation agree,
|
for these two options say the opposite of what the comment implies —
|
||||||
so there is no observable difference on any reachable input. Worth a one-word
|
feature 5: "**Store only.** ACE is the consumer; acdream has nothing to
|
||||||
note that the choice is deliberate/equivalent; no change required.
|
do beyond sending the bit (already correct)"; feature 6: "**Store
|
||||||
|
only**".
|
||||||
|
|
||||||
|
**(c) Against a correct ACE it is dead code; against a drifting one it is
|
||||||
|
harmful.** Per lane B, ACE refuses the recruit outright when the target
|
||||||
|
has `IgnoreFellowshipRequests` (`Player_Fellowship.cs:98-102` →
|
||||||
|
`FellowshipIgnoringRequests 0x0417` to the *recruiter*) and auto-joins
|
||||||
|
without a confirmation when the target has `FellowshipAutoAcceptRequests`
|
||||||
|
(`Fellowship.cs:121` → `AddConfirmedMember(…, true)`). In both cases the
|
||||||
|
target's client never receives a type-4 request, so the intercept never
|
||||||
|
fires. It only fires when the client's copy of the bit disagrees with the
|
||||||
|
server's — and then it does the wrong thing.
|
||||||
|
|
||||||
|
**(d) That disagreement is the default state, not an edge case.**
|
||||||
|
`CharacterOptionTable.cs:113` gives `IgnoreFellowshipRequests` a client
|
||||||
|
default of **`true`**. A player who has never touched the option, on any
|
||||||
|
server whose own default differs or that does not implement the
|
||||||
|
server-side filter, will have every fellowship invite auto-declined with
|
||||||
|
**no dialog and no chat line** — indistinguishable from a broken client.
|
||||||
|
The FA3 gate script's §FA4 steps 18/19 cannot detect this: they pass
|
||||||
|
identically whether the intercept exists or not, because ACE filters
|
||||||
|
first.
|
||||||
|
|
||||||
|
**Recommended disposition (matching the FA1-D9 / FA2-D2 precedent):**
|
||||||
|
delete `TryAutoRespondToFellowshipInvite` and its call site, revert the
|
||||||
|
`IgnoreFellowshipRequests` / `FellowshipAutoAcceptRequests` rows in
|
||||||
|
`CharacterOptionsPageController.Groups` to `StoreOnly` (they return to
|
||||||
|
OP1's classification, which was right), and record the finding as a D6
|
||||||
|
addendum in the plan: *the invite-receive half of D6 is refuted; both
|
||||||
|
bits are server-consumed, acdream's job is to send them, and retail's own
|
||||||
|
client shows the dialog unconditionally.* If instead the user wants the
|
||||||
|
auto-respond behavior kept as a deliberate quality-of-life divergence,
|
||||||
|
it needs (i) an explicit register row, (ii) the ACE-as-retail citation
|
||||||
|
corrected, and (iii) an answer for the default-on `Ignore` bit — but that
|
||||||
|
is a user decision, not an implementer one.
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## Coverage confirmation
|
## MUST-FIX 3 — D4 never re-declares `0x00A6` after a generation reset, so a reconnect leaves the vitals stream dark for the whole new session
|
||||||
- Live-mount probe (real DATs): **PASS 1/1** — the FA4 fellowship assertions
|
|
||||||
(name field `UiField`, 11 controls `UiButton`, row template + 5 fields,
|
`SocialFellowshipPageController.cs:453-458` +
|
||||||
labels/captions, zero-warning `Bind()`) all hold against the production mount.
|
`SocialPanelController.cs:250-268`
|
||||||
- `--no-build` targeted suites: App FA4 = 75/75, Runtime fellowship = 28/28.
|
|
||||||
- The structural finding in the ledger (Create-flow controls are children of
|
`SetPageVisible` is edge-triggered on `_pageVisible`, and
|
||||||
the not-in-fellowship frame `0x1000026B`) is confirmed by the probe dump:
|
`FellowshipSetPanelOpen` is a no-op while disconnected (returns
|
||||||
`0x1000026B` children include `0x10000270/71/72/73` (all four checkboxes),
|
`Inactive` without sending). Enumerating every transition:
|
||||||
`0x1000026F` (name field), `0x10000274` (Create).
|
|
||||||
|
| Transition | Behavior | Verdict |
|
||||||
|
|---|---|---|
|
||||||
|
| Panel opens with Fellowship active | `OnShown` → `_visible=true` → `SetPageVisible(true)` → sends `1` | ✓ |
|
||||||
|
| Panel opens on another tab | conjunction false, `_pageVisible` already false, no send | ✓ (retail's widget isn't visible either) |
|
||||||
|
| Switch **to** Fellowship while open | `ActivePageChanged` → sends `1` | ✓ |
|
||||||
|
| Switch **away** | sends `0` | ✓ |
|
||||||
|
| Close window while on Fellowship | `OnHidden` → sends `0` | ✓ |
|
||||||
|
| `ActivateTabs()` at mount (default = Allegiance, `Visible=false`) | conjunction false, no spurious send; ordering is safe because `ActivateTabs` runs before `RetailWindowFrame.Mount` (`RetailUiRuntime.cs:2775`) | ✓ |
|
||||||
|
| Logout | nothing sent; ACE's `LogOut_Inner` quits the fellowship anyway (lane B feature 21) and the flag is per-session server-side | ✓ — no close is owed |
|
||||||
|
| **Disconnect mid-open, then reconnect** | `_pageVisible` stays `true` across the generation reset; no visibility edge occurs on the new generation; **`0x00A6` is never sent again** | ❌ |
|
||||||
|
|
||||||
|
`RetailUiRuntime` is process-lifetime (`GameWindow.cs:1020`, published
|
||||||
|
once behind a "already owns interaction/UI state" guard), and
|
||||||
|
`ResetSessionTransientUi` (`RetailUiRuntime.cs:731-736`) only resets the
|
||||||
|
dialog controller, the appraisal controller and the Examination window —
|
||||||
|
it does not touch the social panel. `RetailWindowLayoutPersistence`'s
|
||||||
|
`RestoreAll()` on the new `EnteredWorld` will not produce an edge either,
|
||||||
|
because `RetailWindowHandle.NotifyVisibility` early-returns when
|
||||||
|
`_notifiedVisible == visible`.
|
||||||
|
|
||||||
|
Observable result: after any in-process reconnect with the panel left
|
||||||
|
open on Fellowship, every fellow's health/stamina/mana freezes for the
|
||||||
|
rest of the session (lane B §4.5: `Fellowship.OnVitalUpdate` sends
|
||||||
|
`0x02C0` only to fellows whose `FellowshipPanelOpen` is true, and that
|
||||||
|
flag is set only by `0x00A6`).
|
||||||
|
|
||||||
|
**Fix:** clear the declaration latch at the existing session-reset seam
|
||||||
|
so the next `Tick`/visibility evaluation re-declares — e.g. a
|
||||||
|
`SocialPanelController.ResetSessionDeclaration()` that resets
|
||||||
|
`_fellowship`'s `_pageVisible` to `false` and then re-runs
|
||||||
|
`UpdateFellowshipPageVisibility()`, called from
|
||||||
|
`ResetSessionTransientUi`. That is a one-line addition at a seam that
|
||||||
|
already runs on every generation reset (`LiveSessionRuntimeFactory.cs:182`
|
||||||
|
→ `LiveSessionResetBindings.SessionDialogs`) — not a timer, not a poll.
|
||||||
|
|
||||||
|
**Honesty note on the retail comparison:** `gmFellowshipUI` has no
|
||||||
|
`OnEndCharacterSession` (verified — its method list is
|
||||||
|
Create/PostInit/Update/UpdateButtons/UpdateFellow*/RecvNotice_*/
|
||||||
|
ListenToElementMessage/OnVisibilityChanged/MakeFellowRequestDialog and
|
||||||
|
nothing else), and its `PostInit` does **not** send `Event_UpdateRequest`
|
||||||
|
(lane A §6.3 — only the allegiance panel does). Retail's re-declaration
|
||||||
|
therefore comes from `OnVisibilityChanged` firing when the in-game UI
|
||||||
|
state is torn down/restored around character select; that inference is
|
||||||
|
not byte-established here. The *defect* stands regardless of how retail
|
||||||
|
gets it right: acdream reaches a state where the stream it just turned on
|
||||||
|
is off and nothing will ever turn it back on.
|
||||||
|
|
||||||
|
**Also:** the gate script §FA4 has no reconnect step, so the owed
|
||||||
|
connected gate cannot catch this. Add one after step 8.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## MUST-FIX 4 — `gmFellowshipUI::UpdateFellowSelection` is not ported: selecting a fellow in the WORLD leaves Dismiss/Leader disabled, and no row ever shows as selected
|
||||||
|
|
||||||
|
The slice ports one direction of retail's two-directional selection
|
||||||
|
coupling. `SelectFellow` (`SocialFellowshipPageController.cs:676-680`)
|
||||||
|
correctly does panel → world (`SelectionChangeSource.Social`, the FA4
|
||||||
|
Runtime commit). The reverse arm is missing:
|
||||||
|
|
||||||
|
`gmFellowshipUI::UpdateFellowSelection @0x0048F0F0`, reached from
|
||||||
|
`RecvNotice_SelectionChanged @0x0048F1C0` (and from `Update`
|
||||||
|
`@0x0048F6E9`), walks the list box on every world-selection change:
|
||||||
|
|
||||||
|
```
|
||||||
|
selectedID = ACCWeenieObject::selectedID
|
||||||
|
for each row:
|
||||||
|
if row.GetAttribute_InstanceID(0x1000000D) == selectedID:
|
||||||
|
m_iidSelectedFellow = selectedID
|
||||||
|
SetSelectedItem(listBox, row, 0) // and return
|
||||||
|
else if that id == m_iidSelectedFellow:
|
||||||
|
remember row as the fallback
|
||||||
|
SetSelectedItem(listBox, fallbackOrNull, 0)
|
||||||
|
UpdateButtons(this)
|
||||||
|
```
|
||||||
|
|
||||||
|
So in retail, clicking a fellow **in the 3D world** (or reaching them via
|
||||||
|
any other selection origin) selects their panel row and enables
|
||||||
|
Dismiss/Assign-Leader. In acdream `_selectedFellowGuid` is written only
|
||||||
|
by the row name-text `OnClick`
|
||||||
|
(`SocialFellowshipPageController.cs:599,676`), so the world→panel arm
|
||||||
|
does nothing and those two buttons stay greyed.
|
||||||
|
|
||||||
|
Two further consequences of the same gap:
|
||||||
|
|
||||||
|
- **No visual selection at all.** There is no `SetSelectedItem`
|
||||||
|
equivalent — nothing in the roster ever indicates which fellow is
|
||||||
|
selected. The user clicks a name and gets no feedback until they notice
|
||||||
|
two buttons un-greying.
|
||||||
|
- **The plan's FA4 contract line is not met.** §3's FA4 row reads
|
||||||
|
"roster rows (adds `UiTemplateListBox` Flush/selection/row-instance-id
|
||||||
|
— lane A sized this)". `Flush` pre-dated FA4 (FA3); this slice added
|
||||||
|
`FlushPreservingScroll` only. **No selection model and no per-row
|
||||||
|
instance-id were added** — retail's row identity key is
|
||||||
|
`SetAttribute_InstanceID(row, 0x1000000D, fellowIid)` (lane A §3.1 row
|
||||||
|
table, `ghidra@0x0048F440:95`), the exact primitive both
|
||||||
|
`ListenToElementMessage` and `UpdateFellowSelection` read. The ledger's
|
||||||
|
four-item "Contradictions/deferrals" list does not mention this.
|
||||||
|
|
||||||
|
Minimum fix: give `UiTemplateListBox` a row-instance-id + selected-row
|
||||||
|
concept (or, if that is deferred, say so explicitly in the ledger and
|
||||||
|
file the register row), and subscribe the controller to
|
||||||
|
`SelectionState.Changed` — or simply re-derive `_selectedFellowGuid` from
|
||||||
|
`Selection.SelectedObjectId` in `RefreshButtonStates` when the selected
|
||||||
|
world object is a member — so the world→panel arm exists.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## MUST-FIX 5 — three shipped deviations have no register row (CLAUDE.md's register rule, plan §5)
|
||||||
|
|
||||||
|
The register rule is binding and explicit: *"Any commit that introduces a
|
||||||
|
deviation adds its register row IN THE SAME COMMIT… A deviation found
|
||||||
|
without a row is a bug twice over."* `38f08314` files AD-80 (D5 display
|
||||||
|
vs ACE) and AD-81 (StringInfo/FormatName) and amends AD-78. Missing:
|
||||||
|
|
||||||
|
1. **The D6 client-side invite intercept** — a client behavior retail
|
||||||
|
does not have (MUST-FIX 2). If it survives review at all it needs its
|
||||||
|
own row; if it is deleted, the row is not owed.
|
||||||
|
2. **The gold leader-name tint** (`LeaderNameColor` `(1, 0.84, 0, 1)`,
|
||||||
|
`SocialFellowshipPageController.cs:110,614`). This is an invented
|
||||||
|
user-visible color with no DAT or decomp anchor — the class doc says
|
||||||
|
so in as many words ("a minimal, clearly-adaptive visual cue rather
|
||||||
|
than inventing a DAT mechanism that was not found"). That is precisely
|
||||||
|
an AD row: a reviewer comparing a retail screenshot will see a color
|
||||||
|
retail never paints. Same class as AD-78's own rationale.
|
||||||
|
3. **Row selection restricted to the name text.** Retail's list box
|
||||||
|
selects on the *row* (`ListenToElementMessage` message `3`/`0x42`,
|
||||||
|
lane B §2.8). acdream binds `OnClick` to the row's name `UiText` only,
|
||||||
|
so clicking the stats text, any meter, or row whitespace does nothing.
|
||||||
|
Documented in `SelectFellow`'s doc comment; not registered.
|
||||||
|
|
||||||
|
Separately, the ledger's decision to keep the **Recruit "is the target a
|
||||||
|
player" gate** as "an inline comment, not a register row" is the wrong
|
||||||
|
call by the same rule. Retail *disables* the button
|
||||||
|
(`UpdateButtons`, lane B §2.8: "selected world object **is a player**,
|
||||||
|
not already a fellow, and `!IsFull`"); acdream enables it and lets the
|
||||||
|
server refuse. That is a divergence with a user-visible symptom (a lit
|
||||||
|
Recruit button that does nothing when a chest is selected), and the
|
||||||
|
justifying comment — "exactly like retail's own silent no-op" — is
|
||||||
|
inexact, because retail's click handler is unreachable when the button is
|
||||||
|
disabled.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## SHOULD-FIX
|
||||||
|
|
||||||
|
**SF-1 — `RefreshFellowshipName` reintroduces the per-frame `LinesProvider`
|
||||||
|
closure allocation that FA3's fix round removed from the sibling page one
|
||||||
|
commit earlier.**
|
||||||
|
`SocialFellowshipPageController.cs:489-493` assigns
|
||||||
|
`() => [new UiText.Line(name, MemberNameColor)]` on **every** `Tick`
|
||||||
|
while in a fellowship — a display-class + delegate allocation per frame,
|
||||||
|
and `SocialPanelController.Tick` calls the fellowship page
|
||||||
|
unconditionally, so it allocates while the panel is hidden too.
|
||||||
|
`35c40a9b` fixed exactly this in `SocialAllegiancePageController`
|
||||||
|
(hoisted `BlankLineProvider`/`NoLinesProvider` statics, doc: "zero
|
||||||
|
allocation while idle") in response to FA3 mechanism SF-2 / blast SF-4.
|
||||||
|
Cache the provider and only reassign when `name` actually changes.
|
||||||
|
|
||||||
|
**SF-2 — the commit message's carry-forward-4 claim does not describe the
|
||||||
|
code.** The message says "The Fellowship roster path never advances its
|
||||||
|
revision latch on a partial resolver failure until the NEXT real
|
||||||
|
membership change — never a per-frame retry loop."
|
||||||
|
`SocialFellowshipPageController.cs:433-437` advances `_lastRosterRevision`
|
||||||
|
**unconditionally**, before `RefreshRoster` runs. What actually happens
|
||||||
|
on a permanently-unbuildable template: `RebuildRoster` `continue`s past
|
||||||
|
the failed row (`:580-586`), leaving `_rows.Count < members.Count`, so
|
||||||
|
`membershipChanged` is true on **every** subsequent revision bump — i.e.
|
||||||
|
a full `FlushPreservingScroll` + N `LayoutImporter.Build` calls under the
|
||||||
|
shared DAT lock on every incoming `0x02C0` vitals tick, not "until the
|
||||||
|
next real membership change". `RowTemplateResolver` bounds the *import*
|
||||||
|
(a null import is cached, `RowTemplateResolverTests` pins it) but not the
|
||||||
|
*build*. Either make the claim accurate or bound the rebuild (e.g. latch
|
||||||
|
a per-generation "row template is unbuildable" flag).
|
||||||
|
|
||||||
|
**SF-3 — Recruit's "already a fellow" test reads render rows, not
|
||||||
|
membership.** `SocialFellowshipPageController.cs:527` uses
|
||||||
|
`!_rows.ContainsKey(id)`. Retail uses `Fellowship::IsFellow`. If a row
|
||||||
|
failed to build (SF-2's path), an existing fellow passes `targetValid`
|
||||||
|
and Recruit lights up for someone already in the fellowship. Use the
|
||||||
|
member set.
|
||||||
|
|
||||||
|
**SF-4 — `SocialPanelController.Dispose` does not unsubscribe
|
||||||
|
`ActivePageChanged`.** `:140` subscribes a `this`-capturing lambda;
|
||||||
|
`:305-309` only sets `_disposed`. `Tick()` guards on `_disposed`
|
||||||
|
(`:295`); the event handler does not, so a tab switch after disposal
|
||||||
|
still reaches `SetPageVisible` → a Runtime command. Add the unsubscribe
|
||||||
|
(or a `_disposed` check in `UpdateFellowshipPageVisibility`).
|
||||||
|
|
||||||
|
**SF-5 — test naming/coverage gaps on the slice's headline mechanisms.**
|
||||||
|
- `Tick_MembershipChange_RebuildsRoster_ButPreservesScrollPosition`
|
||||||
|
(`SocialFellowshipPageControllerTests.cs:244`) asserts only
|
||||||
|
`Children.Count == 2`; its own comment admits it cannot observe the
|
||||||
|
preserved offset. Rename it or build a roster tall enough to scroll.
|
||||||
|
(The widget-level `UiTemplateListBoxFlushPreservingScrollTests` **do**
|
||||||
|
cover the shrink case correctly — re-derived: `ClearContent` zeroes
|
||||||
|
`UiScrollablePanel.ContentHeight` but not `UiScrollable.ContentHeight`,
|
||||||
|
so the restore clamps against the stale height, and
|
||||||
|
`LayoutScrollableChildren` re-clamps against the fresh height at the
|
||||||
|
head of the next `OnDraw`, before anything paints. That is sound.)
|
||||||
|
- **D4's actual conjunction is untested.** The only D4 test is
|
||||||
|
`SetPageVisible_SendsPanelOpen_OnlyOnATransition` on the *page*
|
||||||
|
controller. Nothing tests `SocialPanelController`'s
|
||||||
|
`_visible && IsShowingFellowship` logic, the `ActivePageChanged`
|
||||||
|
subscription, or `OnShown`/`OnHidden` — the mechanism the slice is
|
||||||
|
named for. `SocialPanelControllerTests` gained a bindings helper and
|
||||||
|
zero new tests.
|
||||||
|
- No controller-level test for a member **leaving** (the shrink rebuild),
|
||||||
|
and none for `_selectedFellowGuid` being cleared when the selected
|
||||||
|
fellow departs (`:568-569`).
|
||||||
|
|
||||||
|
**SF-6 — the live-mount probe prints the strings it is cited for but does
|
||||||
|
not assert them.** `SocialPanelLiveMountProbeTests.cs:185-201` writes the
|
||||||
|
four checkbox labels and the two captions to the console with no
|
||||||
|
assertion, yet the ledger's live-DAT paragraph cites them as verified.
|
||||||
|
This is the same finding FA3's own mechanism SF-3 raised ("printed but
|
||||||
|
never asserted — deserves a real assertion, not just a hope") for
|
||||||
|
`0x10000492` and page exclusivity. Assert non-null (and, for the two
|
||||||
|
captions, the exact `"Open"`/`"Close"`).
|
||||||
|
|
||||||
|
**SF-7 — gate-script §FA4 defects (the FA3 MF-1/MF-2 class).**
|
||||||
|
- Step 3 contradicts itself: "Only the Quit button should be enabled …
|
||||||
|
Disband and Open should ALSO be enabled." A tester following the first
|
||||||
|
clause reports correct behavior as a defect.
|
||||||
|
- Steps 18/19 claim to gate D6, but per lane B ACE filters both bits
|
||||||
|
server-side and never sends the confirmation, so both steps pass
|
||||||
|
identically with or without the client intercept. They cannot fail.
|
||||||
|
- No reconnect step (MUST-FIX 3) and no six/eight-fellow percentage step
|
||||||
|
(MUST-FIX 1) — the two defects most likely to reach a user are both
|
||||||
|
outside the script's reach.
|
||||||
|
|
||||||
|
**SF-8 — D7's `FellowshipShareLoot` un-dim does not meet AD-78's own
|
||||||
|
definition of "Live".** AD-78 defines the dimmed set as rows that
|
||||||
|
"persist and, where auto-save, send the wire bit, but drive nothing
|
||||||
|
observable client-side". `FellowshipShareLoot`'s stated new consumer is
|
||||||
|
"a SECOND live checkbox surface on the fellowship page itself" — but a
|
||||||
|
second editor of the same stored value is not a consumer, nothing in
|
||||||
|
acdream reads the bit (`FormatStatsText` uses `snapshot.ShareXp` only,
|
||||||
|
and the `0x00A2` Create builder carries `shareXP` alone), and the live
|
||||||
|
DAT dump confirms that checkbox (`0x10000273`) is a child of the
|
||||||
|
NOT-in-fellowship frame `0x1000026B`, i.e. **invisible whenever you are
|
||||||
|
in a fellowship**. Re-dim it, or state the widened definition explicitly
|
||||||
|
in AD-78. (`FellowshipShareXP` is defensible: the Create click genuinely
|
||||||
|
reads it, `:325-326`.) The same "second surface" argument in
|
||||||
|
`CharacterOptionsPageController`'s doc should be corrected either way.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## NIT
|
||||||
|
|
||||||
|
**N-1 — meter text vs authored child.** Retail's `UpdateFellowVitals
|
||||||
|
@0x0048ED60` sets meter attribute `0x69` to `cur/max` and writes the
|
||||||
|
cur/max ints into the meter's authored **child** text elements
|
||||||
|
(`0x10000286` under health, `0x10000288` under stamina). acdream uses
|
||||||
|
`UiMeter.Fill`/`UiMeter.Label` because `UiMeter.ConsumesDatChildren` is
|
||||||
|
true — a pre-existing widget decision, not FA4's, but worth one line in
|
||||||
|
`SetVitals`'s doc so the next reader does not go hunting for the
|
||||||
|
unbound `0x10000286`/`0x10000288`/`0x1000028A`.
|
||||||
|
|
||||||
|
**N-2 — `max > 0` guard.** `SetVitals` (`:661`) returns `0f` when
|
||||||
|
`max == 0`; retail divides unconditionally. The guard is the right
|
||||||
|
engineering call; just say so, since "verbatim port" is claimed nearby.
|
||||||
|
|
||||||
|
**N-3 — snapshot/roster read atomicity.** `Tick` takes
|
||||||
|
`_bindings.Snapshot()` and `_bindings.Members()` under two separate
|
||||||
|
acquisitions of `RuntimeFellowshipState._gate`, so `Revision` /
|
||||||
|
`MemberCount` can momentarily disagree with the returned roster. It is
|
||||||
|
self-correcting on the next revision bump and harmless in practice
|
||||||
|
(single-threaded tick today), but a one-line comment would stop a future
|
||||||
|
reader from assuming atomicity.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## What is faithful (checked, no action)
|
||||||
|
|
||||||
|
- **Button-enable table** (`RefreshButtonStates`, `:510-537`) matches lane
|
||||||
|
B §2.8 exactly, including the non-obvious Recruit rule
|
||||||
|
(`isLeader || snapshot.IsOpen`) and Quit-always-enabled.
|
||||||
|
- **Open/Close caption reads as the ACTION, not the state** (`:495-508`),
|
||||||
|
resolved once at `Bind` and swapped from cache — correct, and correctly
|
||||||
|
refuses to invent English when the strings do not resolve.
|
||||||
|
- **Create gating** — empty/whitespace name disables the button and the
|
||||||
|
click double-checks (`:322-327`, `:460-465`); no invented error text,
|
||||||
|
matching lane B §2.2's "the button IS the refusal mechanism".
|
||||||
|
- **Quit/Disband share `0x00A3` with the flag flipped**, and both route
|
||||||
|
through `IRuntimeFellowshipCommands.Quit` so the retail pre-quit
|
||||||
|
`0x0290` leader hand-off in `RequiresLeaderHandoffBeforeQuit` is not
|
||||||
|
bypassed — the reason the commit gives for not calling `WorldSession`
|
||||||
|
directly is correct and load-bearing.
|
||||||
|
- **The roster diff never rebuilds on a vitals tick** — the FA3
|
||||||
|
carry-forward's exact hazard. Verified by construction (set comparison
|
||||||
|
at `:551-560`, in-place `UpdateRow` otherwise) and by
|
||||||
|
`Tick_SameMemberSet_UpdatesRowsInPlace_NoRebuild`'s `Assert.Same` on
|
||||||
|
the row instance.
|
||||||
|
- **`FlushPreservingScroll` shrink semantics** — re-derived against
|
||||||
|
`UiScrollablePanel`/`UiScrollable`; correct, and pinned by
|
||||||
|
`FlushPreservingScroll_ClampsToTheNewShorterContent`.
|
||||||
|
- **`EvenSplitPercentTable` and the `>= 9` full check** — byte-verified
|
||||||
|
against the binary, including the out-of-range `0.0` default matching
|
||||||
|
retail's unsigned `ja` fallthrough for n=0.
|
||||||
|
- **`RowTemplateResolver`** — clean extraction, caches the import (miss
|
||||||
|
included) and rebuilds per row; carry-forward 2 satisfied.
|
||||||
|
- **`IRuntimeFellowshipView.GetMembers`** — materialized under the same
|
||||||
|
lock as every other read, honest ordering caveat in its doc.
|
||||||
|
- **`SelectionChangeSource.Social`** — additive; no exhaustive switch on
|
||||||
|
the enum exists in production, so no dispatch site was missed.
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue