docs: OP2 dual-lens review findings — double REJECT (re-classing blast radius)
Blast lens: the unguarded Type-8 arm re-classes five pre-existing elements in four shipped panels (vendor backdrop sprite stops drawing; character sheet + spellbook swallow clicks; IUiDatStateful propagation severed), the Type-5 guard's premise is factually wrong (ten shipped elements author non-empty 0x64), and the 27 stale fixtures make the harness structurally blind to both — the green suite was true but meaningless. Merge semantics, fixture provenance, hermeticity, and the children-attached hook all verified clean. Mechanism lens: readers decode correctly through the canonical effective-state path; Type 8's real retail class is UIElement_Panel (Update @0x0046BD00 — switching behavior confirmed faithful); both claimed structural identities (UIOption_Slider = horizontal scrollbar, UIOption_Menu = UiMenu shape) CONFIRMED from fixture fingerprints; U10 closed (0x10000521 is the bitfield row template, consumed by CreateChildren @0x00485DF0 via AddItemFromTemplateList(0)); TS-72 is backwards — the toggle math is decomp-confirmed right, IsSet's all-bits predicate is confirmed wrong (retail Refresh checks ANY mask bit). Missing: GetChildRecursive resolution, no-0x32 means NO default switch, SetupTabPageHash's malformed-entry filters. Rework round 1 follows: UiTabControl/UiTemplateListBox become UiDatElement subclasses with dormant behavior (controller-activated), bitfield rows build from the authored template, IsSet goes any-bit, all 32 fixtures regenerate as the acceptance gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Erik
parent
df9c7a35eb
commit
26b119354d
2 changed files with 956 additions and 0 deletions
376
docs/research/2026-08-11-op2-review-blast.md
Normal file
376
docs/research/2026-08-11-op2-review-blast.md
Normal file
|
|
@ -0,0 +1,376 @@
|
||||||
|
# OP2 review — regression / blast-radius lens
|
||||||
|
|
||||||
|
**Commit under review:** `df9c7a35` "feat(ui): Campaign OP slice OP2 — tab control,
|
||||||
|
template ListBox, UIOption widget mappings"
|
||||||
|
**Reviewer lens:** what OP2 breaks OUTSIDE its own scope (Options panel).
|
||||||
|
**Date:** 2026-08-11
|
||||||
|
**Verdict: REJECT** — two independent MUST-FIX regressions on shipped panels, and a
|
||||||
|
test harness structurally incapable of observing either.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 0. Executive summary
|
||||||
|
|
||||||
|
OP2's own acceptance criterion is written into the code it added:
|
||||||
|
|
||||||
|
> `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:103` —
|
||||||
|
> "*(OP2 acceptance: no behavior change to any existing widget)*"
|
||||||
|
|
||||||
|
That criterion is **falsified on two independent axes**, and the justification for one
|
||||||
|
of them is a factual claim about the codebase that is demonstrably wrong:
|
||||||
|
|
||||||
|
> `DatWidgetFactory.cs:99-101` — "*an ORDINARY Type-5 ListBox that authors no template
|
||||||
|
> array (**none currently reach this factory** …) keeps falling to the generic
|
||||||
|
> UiDatElement fallback unchanged*"
|
||||||
|
|
||||||
|
**Ten** Type-5 elements reach that factory today, in five shipped panels, and **every
|
||||||
|
one of them authors a non-empty template array**. Separately, **five** Type-8 elements
|
||||||
|
in four shipped panels change widget class.
|
||||||
|
|
||||||
|
Total blast radius: **15 elements across 7 shipped, user-gated panels** — character
|
||||||
|
sheet, combat, spellbook/component book, vendor, examination, effects (positive +
|
||||||
|
negative), mini-game — silently change widget class in production.
|
||||||
|
|
||||||
|
The 12,770-test green suite is a **structural false negative**: the fixture path never
|
||||||
|
calls the code that populates the new fields, so every fixture-driven test takes the
|
||||||
|
pre-OP2 branch while production takes the new one.
|
||||||
|
|
||||||
|
This is the failure mode `docs/research/2026-08-05-c4-closeout-handoff.md` names as
|
||||||
|
binding: *a contract asserting a mechanism that does not exist*, plus *inferring a fact
|
||||||
|
you can observe*.
|
||||||
|
|
||||||
|
The new Options-panel work itself (fixtures, tab table read, template list read, the
|
||||||
|
four `UIOption_*` mappings, the 25 tests) is sound. The problem is entirely in the
|
||||||
|
un-gated widening of two `switch` arms onto pre-existing authored data.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Fixture-wide enumeration (the requested sweep)
|
||||||
|
|
||||||
|
Method: parsed all **32** committed fixtures under
|
||||||
|
`tests/AcDream.App.Tests/UI/Layout/fixtures/` (27 pre-existing + 5 new), walked every
|
||||||
|
`ElementInfo` node, and recorded (a) the node `Type`, (b) whether any state authors dat
|
||||||
|
property `0x2E` / `0x64` / `0x72`, and (c) whether the JSON carries the new serialized
|
||||||
|
fields.
|
||||||
|
|
||||||
|
### 1.1 Type-8 (`UIElement_TabControl`) — 6 occurrences, 5 of them pre-existing
|
||||||
|
|
||||||
|
| Fixture | Element | Children | Authors `0x2E`? | Tab entries | Pre-OP2 widget | Post-OP2 widget |
|
||||||
|
|---|---|---|---|---|---|---|
|
||||||
|
| `character_2100002E.json` | `0x10000227` (**panel ROOT**) | 7 | yes | **3** | `UiDatElement` | `UiTabControl` |
|
||||||
|
| `combat_21000073.json` | `0x100000A2` | 16 | yes | **8** | `UiDatElement` | `UiTabControl` |
|
||||||
|
| `spellbook_21000034.json` | `0x100002A8` (**panel ROOT**) | 5 | yes | **2** | `UiDatElement` | `UiTabControl` |
|
||||||
|
| `vendor_21000012_100000B7.json` | `0x100000B8` | 6 | yes | **3** | `UiDatElement` | `UiTabControl` |
|
||||||
|
| `vendor_21000012_100000B7.json` | `0x1000008D` | 0 | **no** | 0 | `UiDatElement` | `UiTabControl` |
|
||||||
|
| `options_2100002B.json` | `0x10000208` | 10 | yes | 4 | *(new)* | `UiTabControl` ✅ |
|
||||||
|
|
||||||
|
Authored tab tables actually present in the pre-existing panels (verbatim from the
|
||||||
|
fixtures, `{0x30 button, 0x31 page, 0x32 isDefault}`):
|
||||||
|
|
||||||
|
- character `0x10000227`: `(0x10000228→0x1000022B, default)`, `(0x10000229→0x1000022C)`,
|
||||||
|
`(0x100003F8→0x100003F9)`
|
||||||
|
- combat `0x100000A2`: 8 rows, `(0x10000063→0x1000006A, default)` … `(0x10000382→0x10000383)`
|
||||||
|
- spellbook `0x100002A8`: `(0x100002A9→0x100002AC, default)`, `(0x100002AA→0x100002AD)`
|
||||||
|
- vendor `0x100000B8`: `(0x100000B9→0x1000007C, default)`, `(0x100000BA→0x10000084)`,
|
||||||
|
`(0x100000BB→0x1000008D)`
|
||||||
|
|
||||||
|
### 1.2 Type-5 (`UIElement_ListBox`) — 16 occurrences, 10 of them pre-existing
|
||||||
|
|
||||||
|
**Every pre-existing Type-5 element authors a NON-EMPTY property `0x64`.**
|
||||||
|
|
||||||
|
| Fixture | Element | `0x64` rows | `0x72` | Controller constant | Post-OP2 (production) |
|
||||||
|
|---|---|---|---|---|---|
|
||||||
|
| `character_2100002E.json` | `0x1000023D` ×2 | **5** | `0x1000023E` | `CharacterStatController.ListBoxId` | `UiTemplateListBox` |
|
||||||
|
| `character_2100002E.json` | `0x10000532` | **1** | `0x10000533` | — | `UiTemplateListBox` |
|
||||||
|
| `effects_positive_2100001B.json` | `0x10000123` | **1** | `0x10000124` | `EffectsUiController.ListId` | `UiTemplateListBox` |
|
||||||
|
| `effects_negative_2100001B.json` | `0x10000123` | **1** | `0x10000124` | `EffectsUiController.ListId` | `UiTemplateListBox` |
|
||||||
|
| `examine_2100006B_100005F2.json` | `0x10000149` | **1** | — | `AppraisalUiController.CreatureStatsListId` | `UiTemplateListBox` |
|
||||||
|
| `examine_2100006B_100005F2.json` | `0x1000032D` | **1** | — | `AppraisalUiController.SpellFormulaListId` | `UiTemplateListBox` |
|
||||||
|
| `examine_2100006B_100005F2.json` | `0x10000335` | **1** | — | `AppraisalUiController.CreatureExtraListId` | `UiTemplateListBox` |
|
||||||
|
| `mini_game_2100001E.json` | `0x10000174` | **1** | — | — | `UiTemplateListBox` |
|
||||||
|
| `spellbook_21000034.json` | `0x10000464` | **2** | `0x10000465` | `SpellbookWindowController.ComponentListId` | `UiTemplateListBox` |
|
||||||
|
| `options_*` (3 new) | `0x100001FA` / `0x10000200` / `0x1000050D` | 3 / 8 / 9 | yes | *(new)* | `UiTemplateListBox` ✅ |
|
||||||
|
|
||||||
|
### 1.3 The four `0x100000xx` UIOption class ids — **zero** pre-existing occurrences
|
||||||
|
|
||||||
|
| Class id | Occurrences | Files |
|
||||||
|
|---|---|---|
|
||||||
|
| `0x10000036` CheckboxSlider | 2 | `options_2100002B.json` only |
|
||||||
|
| `0x10000037` Slider | 4 | `options_2100002B.json` only |
|
||||||
|
| `0x10000038` Menu | 1 | `options_2100002B.json` only |
|
||||||
|
| `0x10000044` CheckboxBitfield64 | 1 | `options_2100002B.json` only |
|
||||||
|
|
||||||
|
**Clean.** These four arms are the only genuinely additive part of the switch change.
|
||||||
|
No pre-existing layout carries any of them.
|
||||||
|
|
||||||
|
### 1.4 New-field presence per fixture (fixture-provenance check)
|
||||||
|
|
||||||
|
`System.Text.Json` with `IncludeFields = true` serializes *every* public field, so a
|
||||||
|
fixture generated by the current generator necessarily contains `Outline`,
|
||||||
|
`OutlineColor`, `TabTable`, `TemplateList`, and `ScrollbarElementId` on every node.
|
||||||
|
|
||||||
|
| Group | Files | `Outline`/`OutlineColor` | `TabTable`/`TemplateList`/`ScrollbarElementId` |
|
||||||
|
|---|---|---|---|
|
||||||
|
| The 5 new Options fixtures | `options_2100002B`, `options_gameplay_2100002A`, `options_character_21000028`, `options_chat_2100005C`, `options_config_21000029` | **present** | **present** |
|
||||||
|
| `chat_2100006f.json` | regenerated at CH6a | present | absent |
|
||||||
|
| The other 26 pre-existing | all remaining | **absent** | **absent** |
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. MUST-FIX findings
|
||||||
|
|
||||||
|
### MUST-FIX 1 — Type-8 arm silently re-classes 5 elements in 4 shipped panels
|
||||||
|
|
||||||
|
**Site:** `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:110`
|
||||||
|
```csharp
|
||||||
|
8 => new UiTabControl(info.TabTable),
|
||||||
|
```
|
||||||
|
Unlike the Type-5 arm, this one carries **no guard at all** — every Type-8 element in
|
||||||
|
every layout now becomes a `UiTabControl`.
|
||||||
|
|
||||||
|
`UiTabControl` (`src/AcDream.App/UI/UiTabControl.cs:42`) derives from bare `UiElement`.
|
||||||
|
`UiDatElement` (`src/AcDream.App/UI/Layout/UiDatElement.cs:31`) is
|
||||||
|
`UiElement, IUiDatStateful` and its constructor sets `ClickThrough = true`
|
||||||
|
(`UiDatElement.cs:111`). Four independent consequences:
|
||||||
|
|
||||||
|
**(a) Lost background art — a straight rendering regression.**
|
||||||
|
Vendor `0x1000008D` is 800×20 and authors a DirectState sprite:
|
||||||
|
`StateMedia[""] = (100687632, DrawMode 1)`. `RetailUiRuntime.cs:2287` documents it by
|
||||||
|
name: *"backdrop element `0x1000008D` provides its own fill"*. It has **zero children**
|
||||||
|
and **no `0x2E` property** — it is pure chrome that happens to carry retail Type 8.
|
||||||
|
`UiTabControl` has no media plumbing whatsoever, so **the vendor panel's backdrop fill
|
||||||
|
stops drawing.** This is the clearest single defect in the commit.
|
||||||
|
|
||||||
|
**(b) `ClickThrough` inverts `true → false` on all five.**
|
||||||
|
`UiElement.ClickThrough` is an auto-property defaulting to `false`
|
||||||
|
(`UiElement.cs:159`); `UiElement.HitTest` gates on it at `UiElement.cs:582-583`
|
||||||
|
(`if (ClickThrough) return null; return OnHitTest(...) ? this : null;`), and
|
||||||
|
`OnHitTest` defaults to a plain bounds test (`UiElement.cs:421-422`). Two of the five
|
||||||
|
are **panel roots** (character `0x10000227`, spellbook `0x100002A8`), so the entire
|
||||||
|
character sheet and the entire spellbook now claim every pointer hit inside their
|
||||||
|
bounds instead of passing unclaimed clicks through to the window shell behind them.
|
||||||
|
|
||||||
|
**(c) Lost `IUiDatStateful` state propagation.**
|
||||||
|
`LayoutImporter.cs:168-169` runs `stateful.TrySetRetailState(...)` only for
|
||||||
|
`IUiDatStateful` widgets; `UiDatElement.TrySetRetailState` recurses into children
|
||||||
|
(`UiDatElement.cs:98-99`). `UiTabControl` does not implement the interface, so the
|
||||||
|
authored default-state propagation into the 7 / 16 / 5 / 6 children of these four tab
|
||||||
|
hosts silently stops. This is the exact mechanism the comment at
|
||||||
|
`LayoutImporter.cs:164-167` says the hook exists to preserve.
|
||||||
|
|
||||||
|
**(d) Import-time visibility + click rebinding on controller-owned tabs.**
|
||||||
|
`UiTabControl.OnChildrenAttached` (`UiTabControl.cs:65-81`) runs *during*
|
||||||
|
`LayoutImporter.Build`, before any controller wiring, and calls
|
||||||
|
`RetailTabBinding.SetClick` (which also sets `ClickThrough = false` on each tab button,
|
||||||
|
`RetailTabBinding.cs:16`) plus `SwitchTo(default)` → `page.Visible = active` and
|
||||||
|
`RetailTabBinding.SetOpen`. All four panels already own these tabs
|
||||||
|
(`CharacterStatController.cs:428-432`, `SpellbookWindowController.cs:138-139`,
|
||||||
|
`VendorUiController.cs:587-589`).
|
||||||
|
|
||||||
|
Mitigating detail, and it is *luck*, not design: on 3 of the 4 panels the tab table's
|
||||||
|
ids do **not** match the resolved child ids, so `FindDirectChild` misses and `SwitchTo`
|
||||||
|
is inert — vendor's table names pages `0x1000007C/0x10000084/0x1000008D` while the
|
||||||
|
controller and the actual children use `0x100000BC/0x100000C4/0x100000CD`; combat's
|
||||||
|
table names `0x1000006A..0x10000383` while the children are `0x100000AA..0x100005C3`.
|
||||||
|
Only spellbook and character resolve, and both controllers happen to re-assert
|
||||||
|
visibility afterwards. **Nothing in the commit establishes or tests this ordering.**
|
||||||
|
|
||||||
|
Worth flagging on its own: `CharacterStatController.cs:439-449` carries an explicit
|
||||||
|
comment — *"WHY this cannot be done in the importer … The controller is the correct and
|
||||||
|
only place for initial tab-page selection"* — that OP2 now contradicts for four panels
|
||||||
|
without amending it.
|
||||||
|
|
||||||
|
### MUST-FIX 2 — Type-5 guard rests on a false premise; 10 elements flip in production
|
||||||
|
|
||||||
|
**Site:** `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:104-105`
|
||||||
|
```csharp
|
||||||
|
5 when info.TemplateList.Count > 0 => new UiTemplateListBox(
|
||||||
|
info.TemplateList, info.ScrollbarElementId),
|
||||||
|
```
|
||||||
|
The guard is correct *in the fixture path* and useless *in production*, because
|
||||||
|
`info.TemplateList` is populated by different code on the two paths:
|
||||||
|
|
||||||
|
- **Fixture path:** `FixtureLoader.LoadInfos` (`FixtureLoader.cs:253-265`) deserializes
|
||||||
|
straight into `ElementInfo` and hands it to `LayoutImporter.Build`.
|
||||||
|
`ApplyCanonicalLegacyProjection` is **never called**. Stale fixtures carry no
|
||||||
|
`TemplateList` key → the field initializer's empty list survives → guard is `false`
|
||||||
|
→ old `UiDatElement` behavior. Tests are green.
|
||||||
|
- **Production path:** `LayoutImporter.ToInfo` calls
|
||||||
|
`ElementReader.ApplyCanonicalLegacyProjection(info)` (`LayoutImporter.cs:465`), which
|
||||||
|
runs `info.TemplateList = ReadTemplateList(info)` from property `0x64`. Guard is
|
||||||
|
`true` for all ten → `UiTemplateListBox`.
|
||||||
|
|
||||||
|
Section 1.2 shows all ten pre-existing Type-5 elements author 1–5 valid
|
||||||
|
`{0x63 layoutDid, 0x62 elementId}` rows. Six of them are named controller constants
|
||||||
|
(`AppraisalUiController.CreatureStatsListId` / `CreatureExtraListId` /
|
||||||
|
`SpellFormulaListId`, `CharacterStatController.ListBoxId`, `EffectsUiController.ListId`,
|
||||||
|
`SpellbookWindowController.ComponentListId`).
|
||||||
|
|
||||||
|
Consequences: same `IUiDatStateful` loss and same media loss as MUST-FIX 1, plus a
|
||||||
|
**spurious child injection** — `UiTemplateListBox`'s constructor unconditionally does
|
||||||
|
`base.AddChild(_viewport)` (`UiTemplateListBox.cs:94`), so every one of these ten list
|
||||||
|
boxes gains a full-size anchored `UiScrollablePanel` as its first child. Since
|
||||||
|
`UiPanel` defaults `ClickThrough = false` (confirmed by the comment at
|
||||||
|
`UiPanel.cs:157-159`), that transparent viewport participates in hit-testing across the
|
||||||
|
whole list-box rect, competing with the `UiItemList` each controller adds afterwards on
|
||||||
|
an untested Z-order tie-break.
|
||||||
|
|
||||||
|
Mitigating: the consumers all use `FindElement(...)` returning `UiElement?` with an
|
||||||
|
`is UiItemList` test plus an else-branch (`EffectsUiController.cs:90-94`,
|
||||||
|
`SpellbookWindowController.cs:196-199`, `AppraisalUiController.cs:169`,
|
||||||
|
`:241-243`, `:313-315`), so no controller *fails to mount*. The damage is confined to
|
||||||
|
rendering, hit-testing, and state propagation — but it is unreviewed and unmeasured.
|
||||||
|
|
||||||
|
### MUST-FIX 3 — the harness cannot observe either change (the root enabler)
|
||||||
|
|
||||||
|
`ApplyCanonicalLegacyProjection` has exactly two call sites, both DAT-only:
|
||||||
|
`LayoutImporter.cs:465` (`ToInfo`) and `ElementReader.cs:410` (`Merge`). The fixture
|
||||||
|
path reaches neither. Therefore:
|
||||||
|
|
||||||
|
- Every fixture-driven test exercises the **pre-OP2** branch for all 15 affected
|
||||||
|
elements while production exercises the **post-OP2** branch. The "12,770 passed /
|
||||||
|
0 failed, zero regressions" claim in the commit message is true and meaningless for
|
||||||
|
this change.
|
||||||
|
- The one existing test that touches an affected element,
|
||||||
|
`RetailFixtureConformanceTests.cs:100`
|
||||||
|
(`{ FixtureLoader.LoadCharacterInfos, 0x10000227u, 8u, 300f, 600f, 60 }`), asserts the
|
||||||
|
**`ElementInfo.Type`**, not the built widget class — it already knew `0x10000227` is
|
||||||
|
Type 8 and still could not catch this.
|
||||||
|
- `RetailLayoutFixtureGenerator.cs` now regenerates **all 32** layouts. The moment the
|
||||||
|
coordinator runs `ACDREAM_REGENERATE_UI_FIXTURES=1`, the 27 stale fixtures gain
|
||||||
|
populated `TabTable`/`TemplateList`, and the suite flips to production shapes for all
|
||||||
|
15 elements in one commit, with no test written to explain the diff.
|
||||||
|
|
||||||
|
Answering the task's forward-looking question directly: **no committed test depends on
|
||||||
|
fields present only in new-shape fixtures.** The exposure is the inverse and worse —
|
||||||
|
tests currently depend on those fields being *absent*.
|
||||||
|
|
||||||
|
**Minimum fix shape (recommendation, not a mandate):** narrow both arms so they cannot
|
||||||
|
reach a pre-existing element — e.g. gate on the Options LayoutDesc ids, or on the
|
||||||
|
authored-tab-table ids actually resolving to direct children, or (cleanest) require
|
||||||
|
`info.TabTable.Count > 0 && every ButtonElementId/PageElementId resolves` before
|
||||||
|
choosing `UiTabControl`, and make `UiTabControl`/`UiTemplateListBox` carry the same
|
||||||
|
media + `IUiDatStateful` behavior `UiDatElement` provides. Whatever the shape, land a
|
||||||
|
test that builds the four affected pre-existing layouts **through the DAT-path
|
||||||
|
projection** (call `ApplyCanonicalLegacyProjection` over the fixture tree before
|
||||||
|
`Build`) and asserts the resolved widget class for all 15 ids.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. SHOULD-FIX
|
||||||
|
|
||||||
|
### SHOULD-FIX 1 — `UiTemplateListBox` injects a hit-testable viewport unconditionally
|
||||||
|
|
||||||
|
`UiTemplateListBox.cs:94` (`base.AddChild(_viewport)`) runs in the constructor, before
|
||||||
|
`LayoutImporter.BuildWidget` appends the element's own dat children. The widget does not
|
||||||
|
set `ConsumesDatChildren`, so authored children become **siblings of the viewport rather
|
||||||
|
than rows inside it** — silently correct today only because all three Options ListBoxes
|
||||||
|
author zero children, and a latent trap for OP3+ the moment one doesn't. Combined with
|
||||||
|
`UiPanel`'s `ClickThrough = false` default this is the mechanism behind MUST-FIX 2's
|
||||||
|
hit-testing exposure.
|
||||||
|
|
||||||
|
### SHOULD-FIX 2 — the tab table's ids are base-layout ids on inherited layouts
|
||||||
|
|
||||||
|
`UiTabControl.FindDirectChild` (`UiTabControl.cs:105-111`) matches
|
||||||
|
`child.DatElementId` against the raw `0x2E` values. On three of the four pre-existing
|
||||||
|
panels those values do not correspond to any child in the *incorporated* tree (§2,
|
||||||
|
MUST-FIX 1(d)) — strongly suggesting property `0x2E` carries **base-layout** ids that
|
||||||
|
`LayoutImporter.IncorporateChildren` remaps. The Options panel happens to author its
|
||||||
|
tab table with post-incorporation ids, so OP2's own tests pass. This will bite OP3+
|
||||||
|
directly, because the Options pages mount cross-layout via `BaseLayoutId`/`BaseElement`
|
||||||
|
— exactly the case where the remap applies. Worth resolving now rather than
|
||||||
|
rediscovering it as an OP4 mystery.
|
||||||
|
|
||||||
|
### SHOULD-FIX 3 — silent no-resolve is indistinguishable from correct behavior
|
||||||
|
|
||||||
|
`OnChildrenAttached` and `SwitchTo` both treat a `FindDirectChild` miss as a no-op
|
||||||
|
(`UiTabControl.cs:70-72`, `:95-99`). A tab control whose entire table fails to resolve
|
||||||
|
(combat, today) is behaviorally identical to one with no table at all. Given
|
||||||
|
SHOULD-FIX 2, a diagnostic line on unresolved entries would have surfaced this during
|
||||||
|
implementation.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. NOTEs (verified clean, or informational)
|
||||||
|
|
||||||
|
**NOTE 1 — `ElementReader.Merge` handles the three new fields correctly.** Verified.
|
||||||
|
`Merge` (`ElementReader.cs:344-412`) does *not* add scalar merge rules for `TabTable`,
|
||||||
|
`TemplateList`, or `ScrollbarElementId`, and does not need to: it merges the **state
|
||||||
|
property tables** first (`:400-408`, `UiStateInfo.Merge` giving derived-wins per
|
||||||
|
property) and then calls `ApplyCanonicalLegacyProjection(m)` at `:410`, which recomputes
|
||||||
|
all three from the merged effective state. Base-only `0x64` survives; derived `0x64`
|
||||||
|
replaces base wholesale (correct — arrays are whole-value properties, not concatenated);
|
||||||
|
neither is doubled. **No existing `Merge` behavior changed** — the new field reads are
|
||||||
|
appended after the existing `Outline`/`OutlineColor` block in
|
||||||
|
`ApplyCanonicalLegacyProjection` and touch no prior line. This addresses focus point 2
|
||||||
|
in full: nothing here can drop or double a template list for the OP3+ cross-layout
|
||||||
|
mounts.
|
||||||
|
|
||||||
|
**NOTE 2 — fixture-drift handling is exactly as described.** `git show --stat df9c7a35`
|
||||||
|
lists 5 fixture files, all `A` (added), zero `M`. The 27 pre-existing fixtures are
|
||||||
|
byte-untouched by this commit. And per §1.4 the 5 new fixtures **do** carry
|
||||||
|
`Outline`/`OutlineColor` (plus the three new fields), proving they came from the current
|
||||||
|
generator rather than being hand-built or stale-sourced. This focus-3 MUST-FIX condition
|
||||||
|
is **satisfied**.
|
||||||
|
|
||||||
|
**NOTE 3 — the 25 new tests are hermetic.**
|
||||||
|
`OptionsPanelLayoutConformanceTests.cs` contains exactly 25 `[Fact]`s (matching the
|
||||||
|
commit message) and zero `IDatReaderWriter` / `DatCollection` references; every test
|
||||||
|
loads a committed fixture through `FixtureLoader`. The generator is env-gated
|
||||||
|
(`RetailLayoutFixtureGenerator.cs:42-50`, early-returns unless
|
||||||
|
`ACDREAM_REGENERATE_UI_FIXTURES == "1"`), so it is inert on a machine without DATs. The
|
||||||
|
csproj copies fixtures by wildcard (`AcDream.App.Tests.csproj:28`,
|
||||||
|
`UI\Layout\fixtures\*.json`), so the five new files need no registration. **The suite
|
||||||
|
remains green without the machine DAT.** Focus point 5: clean.
|
||||||
|
|
||||||
|
**NOTE 4 — `IUiChildrenAttachedListener` cannot double-fire.** The hook is invoked
|
||||||
|
exactly once per widget instance, at `LayoutImporter.cs:174-175`, immediately after that
|
||||||
|
widget's own child loop and before `BuildWidget` returns. Depth-first ordering means a
|
||||||
|
nested listener fires before its ancestor, which is the correct order for
|
||||||
|
cross-reference resolution. Nested `LayoutImporter.Build` calls from
|
||||||
|
`UiTemplateListBox.TemplateResolver` create fresh widget instances, so no re-entry
|
||||||
|
hazard. Focus point 4's "fire twice / wrong subtree" concern: **not present.**
|
||||||
|
|
||||||
|
**NOTE 5 — wheel / drag / clipping forwarding is structurally fine.** Because
|
||||||
|
`_viewport` is anchored `Left|Top|Right|Bottom` and `UiElement.HitTest` walks children
|
||||||
|
before gating on the parent (`UiElement.cs:571-583`), pointer and wheel events land on
|
||||||
|
the `UiScrollablePanel` directly — no forwarding shim is needed, and the scroll model,
|
||||||
|
per-row clipping, and `LineHeight` quantum are the same `UiScrollablePanel` code the
|
||||||
|
character sheet and CH6 already exercise. The anchor-capture behavior for a viewport
|
||||||
|
constructed at 0×0 inside a sized host is *not* covered by any of the 25 tests; low risk,
|
||||||
|
but unmeasured.
|
||||||
|
|
||||||
|
**NOTE 6 — `UiOptionToggleSlider` first-match binding.**
|
||||||
|
`UiOptionToggleSlider.OnChildrenAttached` (`UiOptionToggleSlider.cs:33-42`) binds the
|
||||||
|
first `UiButton` and first `UiScrollbar` child. The `else if` is harmless (a child cannot
|
||||||
|
be both types). Zero pre-existing layouts carry class `0x10000036`, so no blast radius —
|
||||||
|
noted only because the pattern silently picks the first match rather than the authored
|
||||||
|
`0x10000219` / `0x1000021C` ids the class doc cites.
|
||||||
|
|
||||||
|
**NOTE 7 — per-element allocation in the hot projection path.**
|
||||||
|
`ApplyCanonicalLegacyProjection` now allocates two fresh `List<>` instances
|
||||||
|
(`ReadTabTable`, `ReadTemplateList`, `ElementReader.cs:490-497`) for **every element on
|
||||||
|
every call**, including the ~99% that author neither property, and `Merge` calls it once
|
||||||
|
per inheritance level. Import is startup-only and not on a frame path, so this is
|
||||||
|
cosmetic — but an early `TryGetEffectiveProperty` bail before allocating would be free.
|
||||||
|
|
||||||
|
**NOTE 8 — TS-72 is correctly filed.** The register hunk
|
||||||
|
(`docs/architecture/retail-divergence-register.md`) adds TS-72 for
|
||||||
|
`UiCheckboxBitfield64`'s click-toggle bit math and bumps the count 40 → 41. Correct per
|
||||||
|
the register rules; the widget is genuinely unreachable today. No comment on the bit
|
||||||
|
math itself — outside this lens.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. Verdict
|
||||||
|
|
||||||
|
**REJECT.**
|
||||||
|
|
||||||
|
The additive half of OP2 (five fixtures, the three `ElementReader` property readers, the
|
||||||
|
`Merge` interaction, the `IUiChildrenAttachedListener` hook, the four `UIOption_*` class
|
||||||
|
ids, the 25 hermetic tests) is well-built and clean under this lens. But two `switch`
|
||||||
|
arms were widened onto authored data the commit believed was absent, changing the widget
|
||||||
|
class of 15 elements across 7 shipped, user-gated panels — including a documented
|
||||||
|
rendering backdrop that now draws nothing — and the harness that reported "zero
|
||||||
|
regressions" is structurally incapable of seeing any of it. The re-review gate should be
|
||||||
|
the DAT-path projection test described at the end of §2.
|
||||||
580
docs/research/2026-08-11-op2-review-mechanism.md
Normal file
580
docs/research/2026-08-11-op2-review-mechanism.md
Normal file
|
|
@ -0,0 +1,580 @@
|
||||||
|
# Campaign OP slice OP2 — MECHANISM-FAITHFULNESS review
|
||||||
|
|
||||||
|
**Date:** 2026-08-11
|
||||||
|
**Commit under review:** `df9c7a35` — "feat(ui): Campaign OP slice OP2 — tab control,
|
||||||
|
template ListBox, UIOption widget mappings"
|
||||||
|
**Lens:** retail-mechanism fidelity + contract fidelity against
|
||||||
|
`docs/plans/2026-08-10-options-panel-campaign.md` §4 OP2 and
|
||||||
|
`docs/research/2026-08-10-options-panel-structure.md`.
|
||||||
|
**Mode:** READ-ONLY. No build, no test run, no launch. Every claim below is either a
|
||||||
|
quote from committed source, a decode of a committed fixture, or an extract from
|
||||||
|
`docs/research/named-retail/acclient_2013_pseudo_c.txt`.
|
||||||
|
|
||||||
|
## VERDICT: **REJECT**
|
||||||
|
|
||||||
|
Two independent MUST-FIX classes, each with hard evidence:
|
||||||
|
|
||||||
|
1. **The unconditional `Type 8` mapping (and the `Type 5` mapping) silently rewires four
|
||||||
|
already-shipped windows and nine already-shipped list boxes at runtime.** The committed
|
||||||
|
test suite cannot see it because the 27 pre-existing fixtures were deliberately NOT
|
||||||
|
regenerated, so they deserialize with `TabTable`/`TemplateList` empty. One
|
||||||
|
already-committed test (`EffectsUiControllerTests.cs:135-137`) encodes the pre-OP2
|
||||||
|
contract and would fail against real DAT data. The slice's own acceptance criterion
|
||||||
|
("no change to any existing widget's behaviour") is false as landed.
|
||||||
|
2. **The retail mechanism for both new widgets is fully present in the named decomp and
|
||||||
|
was not consulted.** `UIElement_Panel::Update/OpenTab/SetupTabPageHash` (Type 8 is
|
||||||
|
`UIElement_Panel`, **not** a class called `UIElement_TabControl` — that class does not
|
||||||
|
exist in the PDB) and `UIOption_CheckboxBitfield64::CreateChildren/ListenToElementMessage/
|
||||||
|
Refresh` are all decompiled. Reading them confirms most of the port, refutes
|
||||||
|
`UiCheckboxBitfield64`'s checked-state predicate, refutes its "the dat authors nothing"
|
||||||
|
premise, and makes TS-72's stated unknown a non-unknown.
|
||||||
|
|
||||||
|
The parts the prompt asked me to independently verify — the two structural identities, the
|
||||||
|
tab-table decode, the template-list decode, the effective-state discipline, the conformance
|
||||||
|
pins — are **all correct**. This is a REJECT on blast radius and on two mechanism defects,
|
||||||
|
not on the Options-panel data work, which is clean.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 1. Property readers (0x2E / 0x64 / 0x72)
|
||||||
|
|
||||||
|
### 1.1 Tab table `0x2E` — decode CORRECT, entry filtering DIVERGES
|
||||||
|
|
||||||
|
`ElementReader.ReadTabTable` (`src/AcDream.App/UI/Layout/ElementReader.cs:508-527`) decodes
|
||||||
|
`{0x30 button, 0x31 page, 0x32 isDefault}` and preserves authored array order.
|
||||||
|
|
||||||
|
Independently decoded from the committed fixture
|
||||||
|
`tests/AcDream.App.Tests/UI/Layout/fixtures/options_2100002B.json`, element `0x10000208`
|
||||||
|
DirectState:
|
||||||
|
|
||||||
|
```
|
||||||
|
P0x58 = Enum(0x1)
|
||||||
|
P0x2E = Array[4]
|
||||||
|
Struct{0x30=Enum(0x1000020D), 0x31=Enum(0x10000212), 0x32=Bool(True)}
|
||||||
|
Struct{0x30=Enum(0x1000020E), 0x31=Enum(0x10000211), 0x32=Bool(False)}
|
||||||
|
Struct{0x30=Enum(0x1000050B), 0x31=Enum(0x1000050C), 0x32=Bool(False)}
|
||||||
|
Struct{0x30=Enum(0x1000020F), 0x31=Enum(0x10000213), 0x32=Bool(False)}
|
||||||
|
P0x57 = Enum(0x1000001A)
|
||||||
|
```
|
||||||
|
|
||||||
|
This matches research §1.3 / §10.1 exactly (Gameplay default, entry 0; Chat third at
|
||||||
|
`0x1000050B`/`0x1000050C`). The `Enum`/`DataId`/`Integer` coercion in `ReadStructMemberId`
|
||||||
|
(`:554-564`) is the right tolerance — the authored kind here is `Enum`.
|
||||||
|
|
||||||
|
Retail's own reader is `UIElement_Panel::SetupTabPageHash @0x0046C2E0`. Three filters
|
||||||
|
acdream does not implement:
|
||||||
|
|
||||||
|
| Retail | acdream | Impact |
|
||||||
|
|---|---|---|
|
||||||
|
| `BaseProperty::GetPropertyName(entry) != 0x2F` → **skip entry** (`0x0046C3A6`) | any `Struct` accepted | NOTE — every authored entry is `0x2F` |
|
||||||
|
| `InqProperty(0x30)` or `InqProperty(0x31)` missing → **skip entry** (`0x0046C3E5`, `0x0046C3FC`) | entry added with `0` for the missing member | **SHOULD-FIX** — a partial entry yields `UiTabTableEntry(0, pageId, …)` that still participates in `SwitchTo` and can hide a page |
|
||||||
|
| `tab != page` required (`0x0046C46B`) | not checked | NOTE |
|
||||||
|
|
||||||
|
### 1.2 Template list `0x64` — decode EXACT
|
||||||
|
|
||||||
|
`ReadTemplateList` (`:529-545`) decodes `{0x63 layout DID, 0x62 element id}`, in that order.
|
||||||
|
Retail: `UIElement_ListBox::AddItem(BaseProperty*) @0x0046F170` reads
|
||||||
|
`InqStructElement(0x63)` then `InqStructElement(0x62)` and calls
|
||||||
|
`AddItem(did, elementId, parent)`. **Byte-for-byte the same pair, same order.** ✔
|
||||||
|
|
||||||
|
Decoded from the committed fixtures (all three arrays, verbatim):
|
||||||
|
|
||||||
|
```
|
||||||
|
0x100001FA (Character) P0x64 = 3 × {0x2100002B, 0x10000216 / 0x10000217 / 0x10000218}
|
||||||
|
0x10000200 (Config) P0x64 = 8 × {0x2100002B, 216,217,218,21A,222,220,21D,221}
|
||||||
|
0x1000050D (Chat) P0x64 = 9 × {…same 8…, 0x10000520}
|
||||||
|
```
|
||||||
|
|
||||||
|
Identical to research §1.5's table. ✔
|
||||||
|
|
||||||
|
### 1.3 Scrollbar `0x72` — correct value, imprecise semantics, missing sibling
|
||||||
|
|
||||||
|
`ReadReferencedElementId(info, 0x72)` (`:569-579`) is correct: the authored kind is `Enum`
|
||||||
|
and retail reads it with `InqEnum`.
|
||||||
|
|
||||||
|
Exact retail semantics, from `UIElement_Scrollable::OnSetAttribute @0x00474630` (not cited
|
||||||
|
anywhere in the commit):
|
||||||
|
|
||||||
|
```
|
||||||
|
0x71 -> m_eHorizonalScrollbarID (+ UpdateScrollbarSize_(1))
|
||||||
|
0x72 -> m_eVerticalScrollbarID (+ UpdateScrollbarSize_(0))
|
||||||
|
```
|
||||||
|
resolution is `UIElement::FindRelative(this, id)` validated by `DynamicCast(0xB)` and a
|
||||||
|
mutual-relative check (`GetScrollbarPointer_ @0x00473EC0`).
|
||||||
|
|
||||||
|
- **NOTE:** `ElementInfo.ScrollbarElementId` / `UiTemplateListBox.ScrollbarElementId` should
|
||||||
|
be named *vertical*; `0x71` (horizontal) is unread. `0x71` IS authored in already-committed
|
||||||
|
fixtures (`vendor_…` `0x100000BD`/`0x100000C5`/`0x100000CE`, `combat_…` `0x100000B6`) — not
|
||||||
|
on the Options ListBoxes, so no OP2-observable gap, but the asymmetry deserves a comment.
|
||||||
|
|
||||||
|
### 1.4 The round-5 N1 lesson — CORRECTLY OBSERVED
|
||||||
|
|
||||||
|
All three readers resolve through `ElementInfo.TryGetEffectiveProperty` (`:215-235`), which
|
||||||
|
is DirectState-as-base then effective-default override — **not** a per-state first-wins scan.
|
||||||
|
`ApplyCanonicalLegacyProjection` is invoked from both entry points
|
||||||
|
(`LayoutImporter.ToInfo` and `ElementReader.Merge:410`), and the three new fields are
|
||||||
|
*recomputed* from the merged state rather than scalar-merged, which is strictly correct.
|
||||||
|
No N1 reintroduction. ✔
|
||||||
|
|
||||||
|
- **SHOULD-FIX (coverage):** the new conformance tests read `TabTable`/`TemplateList`/
|
||||||
|
`ScrollbarElementId` off the **serialized** fixture fields — `LayoutImporter.Build` does not
|
||||||
|
re-run the projection at load. So `ReadTabTable`/`ReadTemplateList` are exercised only at
|
||||||
|
fixture-generation time and a future regression in either reader would not fail a single
|
||||||
|
test. One synthetic `ElementInfo` + `States[Direct].Properties[0x2E]` unit test per reader
|
||||||
|
closes this for ~20 lines.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 2. `UiTabControl` — right behaviour, wrong class, wrong lookup, invented fallback
|
||||||
|
|
||||||
|
**The core behaviour is confirmed against retail.** `UIElement_Panel::Update @0x0046BD00`:
|
||||||
|
|
||||||
|
```
|
||||||
|
if (arg2 != m_OpenPageToken && arg3 != m_OpenTabToken) {
|
||||||
|
m_OpenTabToken = arg3; m_OpenPageToken = arg2;
|
||||||
|
foreach (tabId -> pageId) in m_TabPageHash {
|
||||||
|
visible = 0; state = 0xB; // Closed
|
||||||
|
if (m_OpenTabToken == tabId && m_OpenPageToken == pageId) { visible = 1; state = 0xC; } // Open
|
||||||
|
page = GetChildRecursive(this, pageId); if (page) page->SetVisible(visible);
|
||||||
|
tab = GetChildRecursive(this, tabId); if (tab) tab->SetState(state);
|
||||||
|
}
|
||||||
|
UIElement::BroadcastElementMessage(this, 0x2C, 0, 0);
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
`UiTabControl.SwitchTo` (`src/AcDream.App/UI/UiTabControl.cs:88-103`) does exactly this:
|
||||||
|
exactly one page `Visible`, every tab button driven to `Open`/`Closed` via
|
||||||
|
`RetailTabBinding.SetOpen` → `RetailUiStateIds.Open = 12` / `Closed = 11`
|
||||||
|
(`src/AcDream.App/UI/IUiDatStateful.cs:15-16`) = retail's `0xC`/`0xB`. ✔ The authored data
|
||||||
|
corroborates independently: the four tab buttons are `Type 0xC` text elements with
|
||||||
|
`DefaultStateId = 11 (Closed)` and authored state-11 grey `ARGB(255,127,127,127)` vs state-12
|
||||||
|
`ARGB(255,204,204,204)` font colours.
|
||||||
|
|
||||||
|
**The authored default IS honoured on mount** — `OnChildrenAttached` (`:65-81`) picks the
|
||||||
|
`IsDefault` entry and `SwitchTo`s it, and the fixture proves entry 0 (Gameplay,
|
||||||
|
`0x1000020D`/`0x10000212`) is the sole default. Retail takes the last `0x32==true` entry;
|
||||||
|
acdream's loop does too. ✔
|
||||||
|
|
||||||
|
Four defects:
|
||||||
|
|
||||||
|
- **MUST-FIX (citation).** The class is documented as retail `UIElement_TabControl`
|
||||||
|
(`UiTabControl.cs:7`, `DatWidgetFactory.cs:108-109`). **No such class exists** —
|
||||||
|
`grep UIElement_TabControl docs/research/named-retail/symbols.json` returns nothing. Type 8
|
||||||
|
is `UIElement_Panel` (`UIElement::RegisterElementClass(8, UIElement_Panel::Create)` at
|
||||||
|
`0x0046C6B7`). CLAUDE.md's phase checklist requires a named symbol + address; citing a
|
||||||
|
fictional class is what let items 2–4 below through. Correct anchors:
|
||||||
|
`UIElement_Panel::SetupTabPageHash @0x0046C2E0`, `::Update @0x0046BD00`,
|
||||||
|
`::OpenTab @0x0046BE20`, `::InqTabFromPage @0x0046BEB0`,
|
||||||
|
`::ListenToElementMessage @0x0046BF90`.
|
||||||
|
- **SHOULD-FIX.** `FindDirectChild` (`:105-111`) scans immediate children only; retail uses
|
||||||
|
`GetChildRecursive` for **both** the tab and the page. Fine for `0x2100002B` (all direct),
|
||||||
|
wrong for the Configure Keyboard tab host `0x1000049B` (research §8) this widget is
|
||||||
|
explicitly meant to also serve.
|
||||||
|
- **SHOULD-FIX.** `defaultEntry ??= _tabs[0]` (`:78`) is invented. Retail leaves
|
||||||
|
`m_OpenPageToken/m_OpenTabToken` at 0 when no entry authors `0x32`, so `Update(0,0)` hits the
|
||||||
|
guard and **no switch happens at all**. Either match retail or file a register row.
|
||||||
|
- **NOTE.** Retail's `BroadcastElementMessage(this, 0x2C, …)` after each switch, its
|
||||||
|
`SetMouseVisible(1)` per tab button during setup (`0x0046C4FB`), and its
|
||||||
|
`ListenToElementMessage` handling of `idMessage 0x18` (a page becoming visible re-opens its
|
||||||
|
tab; the panel follows its open page's visibility) are unported. Retail also drives
|
||||||
|
switching from a child element message (`0x19`/`0x29` → `OpenTab`) rather than a per-button
|
||||||
|
click delegate — acdream's `RetailTabBinding.SetClick` adaptation is consistent with the
|
||||||
|
existing character/vendor/spellbook tabs, but it is an adaptation and OP3 will need the
|
||||||
|
`0x2C` notification for the `OnVisibilityChanged` apply/revert semantics (research §3.6).
|
||||||
|
|
||||||
|
### 2.1 **MUST-FIX — the Type-8 arm is unconditional and rewires four shipped windows**
|
||||||
|
|
||||||
|
`DatWidgetFactory.cs:110` maps **every** Type-8 element to `UiTabControl`. Type 8 =
|
||||||
|
`UIElement_Panel`; a Panel without a `0x2E` array is just a panel. Two consequences, both
|
||||||
|
regressions relative to the previous `_ => new UiDatElement(info, resolve)` fallback:
|
||||||
|
|
||||||
|
* `UiTabControl : UiElement` has **no `OnDraw`** — the authored DirectState media is not drawn
|
||||||
|
(`UiDatElement.OnDraw`, `src/AcDream.App/UI/Layout/UiDatElement.cs:193-216`).
|
||||||
|
* `UiElement.ClickThrough` defaults **false**; `UiDatElement` sets it **true**
|
||||||
|
(`UiDatElement.cs:111`). Every converted panel now swallows clicks that miss a child.
|
||||||
|
|
||||||
|
I decoded the raw `States` bag of every committed fixture (the raw property survives even
|
||||||
|
though the projected field does not). Pre-existing Type-8 elements:
|
||||||
|
|
||||||
|
| Fixture | Element | Size | Authored `P0x2E` | Media | Consequence |
|
||||||
|
|---|---|---|---|---|---|
|
||||||
|
| `vendor_21000012_100000B7.json` | `0x1000008D` | 800×20 | **none** | **yes** (`0x06004…`, DirectState) | loses its fill; `RetailUiRuntime.cs:2287` comments that this element "provides its own fill" |
|
||||||
|
| `vendor_…` | `0x100000B8` | 800×110 | 3 entries (`B9/BC`, `BA/C4`, `BB/CD`) | no | live tab control fights `VendorUiController` |
|
||||||
|
| `character_2100002E.json` | `0x10000227` (window root) | 300×600 | 3 entries (`228/22B`, `229/22C`, `538/539`) | no | live tab control; entry 3 is the **Titles** tab `CharacterStatController.cs:432` deliberately keeps inert |
|
||||||
|
| `spellbook_21000034.json` | `0x100002A8` (window root) | 300×600 | 2 entries | no | live tab control vs `SpellbookWindowController` |
|
||||||
|
| `combat_21000073.json` | `0x100000A2` | 800×60 | **8 entries** (`A3/AA` … `5C2/5C3`) | no | **no controller rebinds these** — `UiTabControl` takes ownership outright and hides 7 of the 8 stance pages |
|
||||||
|
|
||||||
|
`VendorUiController`/`SpellbookWindowController`/`CharacterStatController` re-bind their tabs
|
||||||
|
after import, so their click hijack is overwritten; but `UiTabControl.OnChildrenAttached`
|
||||||
|
still runs first and mutates `Visible`/state, and **combat has no re-binder at all**.
|
||||||
|
|
||||||
|
The minimum correct fix is (a) gate the arm exactly like the Type-5 arm
|
||||||
|
(`8 when info.TabTable.Count > 0`) so media-bearing plain panels such as `0x1000008D` keep
|
||||||
|
`UiDatElement`, (b) give `UiTabControl` `UiDatElement`'s media draw + `ClickThrough`
|
||||||
|
semantics, and (c) explicitly reconcile the four pre-existing hosts — either `UiTabControl`
|
||||||
|
owns their switching and the duplicated controller code goes, or Type 8 is not mapped
|
||||||
|
outside `0x2100002B` in this slice.
|
||||||
|
|
||||||
|
### 2.2 **MUST-FIX — the same class of regression on the Type-5 arm**
|
||||||
|
|
||||||
|
`DatWidgetFactory.cs:99-105` justifies the gate with "none currently reach this factory". That
|
||||||
|
is refuted by the same raw-property decode. Pre-existing Type-5 elements that **do** author
|
||||||
|
`P0x64`, and therefore flip from `UiDatElement` to `UiTemplateListBox` at runtime:
|
||||||
|
|
||||||
|
| Fixture | Element | Media | Referenced by |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `effects_positive/negative_2100001B.json` | `0x10000123` | no | `EffectsUiController.ListId` |
|
||||||
|
| `examine_2100006B_100005F2.json` | `0x10000149`, `0x10000335`, `0x1000032D` | no | `AppraisalUiController.CreatureStatsListId` / `CreatureExtraListId` / `SpellFormulaListId` |
|
||||||
|
| `character_2100002E.json` | `0x1000023D` | no | `CharacterStatController.ListBoxId` |
|
||||||
|
| `character_2100002E.json` | `0x10000532` | **yes** | Titles page list |
|
||||||
|
| `spellbook_21000034.json` | `0x10000464` | no | `SpellbookWindowController.ComponentListId` |
|
||||||
|
| `mini_game_2100001E.json` | `0x10000174` | **yes** | mini-game board |
|
||||||
|
|
||||||
|
`UiTemplateListBox : UiPanel` zeroes `BackgroundColor`/`BorderColor` (`UiTemplateListBox.cs:92-93`)
|
||||||
|
and draws no authored media, is not `ClickThrough`, and inserts an anchored `UiScrollablePanel`
|
||||||
|
as child 0.
|
||||||
|
|
||||||
|
**Smoking gun:** `tests/AcDream.App.Tests/UI/Layout/EffectsUiControllerTests.cs:135-137`
|
||||||
|
already asserts
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
UiElement listHost = Assert.IsType<UiDatElement>(layout.FindElement(EffectsUiController.ListId));
|
||||||
|
UiItemList list = Assert.IsType<UiItemList>(Assert.Single(listHost.Children));
|
||||||
|
```
|
||||||
|
|
||||||
|
Against real DAT data both assertions now fail (wrong type; two children). It passes only
|
||||||
|
because `effects_positive_2100001B.json` predates the `TemplateList` field.
|
||||||
|
|
||||||
|
**This is also why the commit message's fixture-drift paragraph is wrong.** It states the
|
||||||
|
regenerated drift was "an Outline/OutlineColor field pair added by an earlier commit,
|
||||||
|
bcc34ee3". The pre-existing fixtures contain **neither** `Outline`/`OutlineColor` **nor**
|
||||||
|
`TabTable`/`TemplateList`/`ScrollbarElementId` (grep count 0 for all five in
|
||||||
|
`character_2100002E`, `vendor_…`, `spellbook_…`, `combat_…`). Regeneration would have
|
||||||
|
populated the three new fields too — i.e. the decision to revert the drift is exactly what
|
||||||
|
suppressed the evidence of §2.1/§2.2. Regenerating those 27 fixtures is the acceptance gate
|
||||||
|
for any fix here.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 3. `UiTemplateListBox` — mechanism faithful
|
||||||
|
|
||||||
|
`AddItemFromTemplateList(index)` (`src/AcDream.App/UI/UiTemplateListBox.cs:104-118`)
|
||||||
|
resolves `Templates[index]` through `TemplateResolver(layoutId, elementId)` and appends the
|
||||||
|
built subtree. Retail (`UIElement_ListBox::AddItemFromTemplateList @0x0046F2B0` →
|
||||||
|
`AddItem(BaseProperty*) @0x0046F170` → `AddItem(did, elementId, parent) @0x0046EDC0` →
|
||||||
|
`UIElementManager::CreateChildElementByDid(this, did, elementId)`) does the same thing: build
|
||||||
|
the named element out of the named LayoutDesc as a child of the list box, then append.
|
||||||
|
Instantiating through `LayoutImporter.ImportInfos(dats, layoutId, elementId)` + `Build` is the
|
||||||
|
correct acdream analogue of `CreateChildElementByDid`, and it is the same overload
|
||||||
|
`RetailDialogFactory` already uses. ✔ Out-of-range and no-resolver both return null, matching
|
||||||
|
retail's null returns. ✔
|
||||||
|
|
||||||
|
Scrollbar linkage is carried as data (`ScrollbarElementId`) for a page controller to bind to
|
||||||
|
`Scroll`; that mirrors `ChatWindowController`'s existing pattern. Acceptable for a
|
||||||
|
"mechanism-only" slice — see §1.3 for the naming/`0x71` notes.
|
||||||
|
|
||||||
|
- **NOTE.** Row stacking uses `_viewport.ContentHeight` as the next row's `Top`
|
||||||
|
(`:114-116`). Retail sizes the box from `CalculatePaperSize` and the ListBox owns row
|
||||||
|
placement; the acdream model is equivalent for uniform top-anchored rows and is the same
|
||||||
|
model `CharacterStatController.RebuildActiveList` already uses. Fine.
|
||||||
|
- **NOTE.** Authored dat children of a `UiTemplateListBox` land as siblings of the internal
|
||||||
|
viewport (the class does not override `AddChild`), while template rows go inside it. Benign
|
||||||
|
for the Options ListBoxes (no authored children) but relevant to §2.2's converted lists.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 4. The two structural identities — **BOTH CONFIRMED** (independent check)
|
||||||
|
|
||||||
|
I decoded the fixture myself rather than taking the commit message's word.
|
||||||
|
|
||||||
|
### 4.1 `UIOption_Slider 0x10000037` ≡ horizontal `UIElement_Scrollbar` — **CONFIRMED**
|
||||||
|
|
||||||
|
`options_2100002B.json` element `0x1000021C`:
|
||||||
|
|
||||||
|
```
|
||||||
|
0x1000021C T=0x10000037 (146,4) 120x12 DirectState image 0x060012C5
|
||||||
|
P0x7B=Bool(True) P0x86=Float(0.5) P0x7C=Bool(True) P0x7E=Bool(False)
|
||||||
|
P0x82=Bool(False) P0x83=Bool(True) [+ P0x47/P0x48/P0x4B, see below]
|
||||||
|
child id=1 T=0x1 7x12 DirectState image 0x060012C6
|
||||||
|
```
|
||||||
|
|
||||||
|
Compare the real `Type 0xB` scalar slider in `combat_21000073.json`, element `0x1000004F`
|
||||||
|
(gmCombatUI desired-power):
|
||||||
|
|
||||||
|
```
|
||||||
|
0x1000004F T=0xB 707x14
|
||||||
|
P0x7B=Bool(True) P0x86=Float(0.5) P0x7C=Bool(True) P0x7E=Bool(False)
|
||||||
|
P0x82=Bool(False) P0x83=Bool(False)
|
||||||
|
child id=1 T=0x1 12x14 child id=4 T=0x3 child T=0x7 (meter)
|
||||||
|
```
|
||||||
|
|
||||||
|
**Identical property fingerprint, identical `child id = 1` thumb convention, `W > H`.**
|
||||||
|
`BuildScrollbar`'s horizontal branch (`DatWidgetFactory.cs:223-279`) reads exactly
|
||||||
|
`DefaultImage(info)` for the track and `child id 1` for the thumb. The identity holds; the
|
||||||
|
mapping is correct and the conformance test (`OptionsPanelFixture_SliderControl_BuildsAs…`)
|
||||||
|
pins `Horizontal == true` and a non-zero thumb sprite. ✔
|
||||||
|
|
||||||
|
### 4.2 `UIOption_Menu 0x10000038` ≡ `UiMenu`'s shape — **CONFIRMED**
|
||||||
|
|
||||||
|
```
|
||||||
|
options_2100002B 0x10000224 T=0x10000038 120x18
|
||||||
|
P0x2 P0x6 P0x7=DID(0x21000043) P0x8 P0x9 P0xA=DID(0x21000043) P0x13 [+0x47/0x48/0x4B]
|
||||||
|
children: 0x10000355 T=0xC 100x18 (label) 0x10000356 T=0x3 17x19 (arrow cap)
|
||||||
|
|
||||||
|
vendor_… 0x100000BF T=0x6 (UIElement_Menu) 117x18
|
||||||
|
P0x2 P0x6 P0x7=DID(0x21000043) P0x8 P0x9 P0xA=DID(0x21000043) P0x13
|
||||||
|
children: 0x1000034D T=0xC 100x18 (label) 0x1000034E T=0x3 17x19 (arrow cap)
|
||||||
|
```
|
||||||
|
|
||||||
|
Same seven properties, the same popup-template DID `0x21000043`, the same 100×18 label +
|
||||||
|
17×19 arrow-cap child pair. `new UiMenu()` is right, and it matches how the existing Type-6
|
||||||
|
arm is built. ✔
|
||||||
|
|
||||||
|
- **NOTE (both identities).** The `UIOption_*` family carries an extra authored triple
|
||||||
|
`P0x47=Enum(0x10000397)`, `P0x48=DID(0x21000041)`, `P0x4B=Bool(True)` that the plain
|
||||||
|
Type-6/Type-11 counterparts do not. It is on the slider, the menu, **and** the already-mapped
|
||||||
|
`UIOption_Checkbox 0x10000219`, so it is a pre-existing uniform gap rather than something
|
||||||
|
OP2 introduced — but somebody should establish what `0x21000041/0x10000397` is (it looks like
|
||||||
|
a shared tooltip/option-chrome base pair) before OP4–OP6 rely on those rows rendering
|
||||||
|
completely.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 5. `UiOptionToggleSlider` / `UiCheckboxBitfield64` and TS-72
|
||||||
|
|
||||||
|
### 5.1 `UiOptionToggleSlider` — consistent with the authored templates ✔
|
||||||
|
|
||||||
|
Fixture-verified: `0x10000220` (272×20) and `0x10000221` (272×36) are both
|
||||||
|
`T=0x10000036` and both contain exactly one `0x10000219` (`T=0x10000035`, checkbox) and one
|
||||||
|
`0x1000021C` (`T=0x10000037`, slider); `0x10000221` additionally carries the two range-label
|
||||||
|
`Type 0xC` children `0x1000021E`/`0x1000021F`. Composing rather than consuming children is
|
||||||
|
right, and `OnChildrenAttached` (`src/AcDream.App/UI/UiOptionToggleSlider.cs:33-42`) picks the
|
||||||
|
first `UiButton` and first `UiScrollbar`, which is unambiguous for both templates. The
|
||||||
|
"labelled slider row `0x1000021D` is a plain `Type 3`, not `0x10000036`" test is a genuinely
|
||||||
|
good data-driven discriminator. ✔
|
||||||
|
|
||||||
|
### 5.2 `UiCheckboxBitfield64` — **MUST-FIX ×2**
|
||||||
|
|
||||||
|
**(a) The checked-state predicate is wrong.** `IsSet` (`src/AcDream.App/UI/UiCheckboxBitfield64.cs:137-138`)
|
||||||
|
requires **all** mask bits set. Retail `UIOption_CheckboxBitfield64::Refresh @0x004859C0`:
|
||||||
|
|
||||||
|
```
|
||||||
|
lowHit = m_current.low & mask.low;
|
||||||
|
highHit = m_current.high & mask.high;
|
||||||
|
allSet = (lowHit == mask.low && highHit == mask.high); // computed…
|
||||||
|
ebx = ((lowHit | highHit) != 0) ? 0 : 1; // …but the VISIBLE flag is ANY-set
|
||||||
|
row = UIElement_ListBox::GetItem(this, i);
|
||||||
|
cb = UIElement::GetChild(row, 0x10000219);
|
||||||
|
UIElement::SetAttribute_Bool(cb, 0xE, ebx == 0); // checked <=> ANY mask bit set
|
||||||
|
```
|
||||||
|
|
||||||
|
Retail checks the box when **any** bit of the mask is set; acdream only when **all** are. This
|
||||||
|
matters immediately: 8 of the 13 Chat filter masks are multi-bit
|
||||||
|
(Gameplay `0x83912021`, Combat `0x00600040`, Magic `0x00020080`, AreaSpeech `0x00001004`,
|
||||||
|
Tells `0x00000018`, Allegiance `0x00040C00`, …). The click direction is derived from the
|
||||||
|
displayed state on both sides, so a wrong predicate inverts the next click too.
|
||||||
|
|
||||||
|
**(b) The toggle math is NOT unknown — TS-72's premise is false.**
|
||||||
|
`UIOption_CheckboxBitfield64::ListenToElementMessage @0x00485AE0`:
|
||||||
|
|
||||||
|
```
|
||||||
|
if (idMessage == 1
|
||||||
|
&& GetAttribute_Int (pElement, 0x10000084, &index) // row index stamped at build time
|
||||||
|
&& GetAttribute_Bool(pElement, 0xE, &onOff) // the row checkbox's new state
|
||||||
|
&& index < childCount)
|
||||||
|
{
|
||||||
|
BitUtils::SetBitsOnOrOff(&m_current, m_rgChildValues[index].mask, onOff);
|
||||||
|
vtable+0x2D4 (1); // Apply(1)
|
||||||
|
}
|
||||||
|
return UIElement_ListBox::ListenToElementMessage(this, edi);
|
||||||
|
```
|
||||||
|
|
||||||
|
`SetBitsOnOrOff(value, mask, on)` **is** "on ⇒ OR mask, off ⇒ AND-NOT mask" — so
|
||||||
|
`ToggleRow` (`:140-155`) is *correct*, and TS-72's claim that "the decompiled excerpt this
|
||||||
|
campaign pulled covers `Apply`'s WRITE side, not its own click-handler's bit math" describes
|
||||||
|
what the campaign grepped, not what exists. Every method is in
|
||||||
|
`acclient_2013_pseudo_c.txt` and `symbols.json`: `CreateChildren @0x00485DF0`,
|
||||||
|
`AddChild @0x004862A0`, `Refresh @0x004859C0`, `ListenToElementMessage @0x00485AE0`,
|
||||||
|
`GetValue @0x00486150`, `Changed/SaveCurrentValue/RestoreSavedValue/RestoreDefaultValue`
|
||||||
|
`@0x004856A0…0x00485780`. **TS-72 is therefore not an accurate statement of what is and is
|
||||||
|
not verified.** It should be rewritten (or retired) to say: the OR/AND-NOT half is
|
||||||
|
byte-confirmed correct; the checked-state predicate is byte-confirmed **wrong**.
|
||||||
|
|
||||||
|
**(c) "The template authors zero children" is refuted by the committed fixture.**
|
||||||
|
`BuildCheckboxBitfield64` (`DatWidgetFactory.cs:810-815`) reads nothing from `ElementInfo`,
|
||||||
|
and the class doc (`UiCheckboxBitfield64.cs:11-14`) plus the commit message assert the
|
||||||
|
template "carries NO children and NO media". The actual authored element:
|
||||||
|
|
||||||
|
```
|
||||||
|
0x10000520 T=0x10000044 272x100
|
||||||
|
P0x64 = Array[1] -> Struct{0x63=DID(0x2100002B), 0x62=Enum(0x10000521)}
|
||||||
|
P0x10000082 = DID(0x06004D17)
|
||||||
|
P0x10000083 = DID(0x06004D19)
|
||||||
|
```
|
||||||
|
|
||||||
|
It authors **its own row-template list** — the very `0x64` property this commit taught the
|
||||||
|
reader to decode — naming `0x2100002B/0x10000521`, plus two RenderSurface DIDs (which are the
|
||||||
|
LED sprite's `Highlight`/`Ghosted` images, `0x06004D17`/`0x06004D19`). `0x10000521` is a
|
||||||
|
272×20 container holding `0x10000219` (`Type 1`, base `0x21000044/0x10000329`) with the 13×13
|
||||||
|
five-state LED child `0x10000328` — i.e. exactly the row art. Research §1.5 flagged
|
||||||
|
`0x10000521` as "UNKNOWN consumer"; **this fixture closes that unknown** (research U10 too),
|
||||||
|
and the decomp confirms it:
|
||||||
|
|
||||||
|
```
|
||||||
|
UIOption_CheckboxBitfield64::CreateChildren @0x00485DF0
|
||||||
|
for i in 0 .. m_rgChildValues.m_num-1:
|
||||||
|
row = UIElement_ListBox::AddItemFromTemplateList(this, 0, nullptr); // template idx 0
|
||||||
|
cb = GetChildRecursive(row, 0x10000219)->DynamicCast(1);
|
||||||
|
SetAttribute_Int(cb, 0x10000084, i);
|
||||||
|
UIElement_Text::SetStringInfo(cb, childValues[i].siLabel);
|
||||||
|
UIElement::SetTooltip (cb, childValues[i].siTooltip);
|
||||||
|
ResizeTo(GetWidth(), CalculatePaperSize(0, -1));
|
||||||
|
Refresh();
|
||||||
|
```
|
||||||
|
|
||||||
|
`UIOption_CheckboxBitfield64` **is a `UIElement_ListBox`** — it calls
|
||||||
|
`UIElement_ListBox::AddItemFromTemplateList`/`GetItem`/`CalculatePaperSize`/
|
||||||
|
`ListenToElementMessage` on itself. So the faithful acdream port is
|
||||||
|
`UiCheckboxBitfield64` composing the `UiTemplateListBox` mechanism **this same commit
|
||||||
|
shipped**, not a hand-rolled `UiPanel` of synthesized `UiButton`s. As written, `AddChild`
|
||||||
|
(`:108-135`) fabricates an `ElementInfo{Type=1, Height=RowHeight}` with a hand-injected
|
||||||
|
`0x0B` property and an invented `RowHeight = 14f` whose comment claims "No authored row
|
||||||
|
height exists for this block (it is unauthored in the dat)" — the dat authors 272×20 with a
|
||||||
|
260×14 checkbox inset at (6,2). Consequences as landed: no LED art, no authored font
|
||||||
|
(`0x40000002`), no authored colour (`0x1B`), no authored label insets (`0x23`/`0x25`), no
|
||||||
|
justification (`0x14=Left`), no tooltip plumbing, and no outline seeding.
|
||||||
|
|
||||||
|
`AddChild(uint64 mask, uint32 labelId, uint32 tooltipId) @0x004862A0` is also confirmed to be
|
||||||
|
pure bookkeeping (append a `ChildInfo{mask, StringInfo(label, tableEnum **0x10000006**),
|
||||||
|
StringInfo(tooltip, 0x10000006)}`) — note the table enum is `0x10000006`, not the `0x10000003`
|
||||||
|
used for headers/labels elsewhere. Worth pinning for OP5.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 6. Outline seams (`aa6635ae` / AP-192) — **PRESERVED**
|
||||||
|
|
||||||
|
I scanned all five new fixtures for authored `0x21`/`0x22`. Every occurrence (32 total) is
|
||||||
|
`0x21 = Bool(True)` **in state 3 only**, on `Type 1` buttons: the three
|
||||||
|
Apply/Reset/Defaults buttons on each of Character/Chat/Config, and the seven Gameplay-tab
|
||||||
|
buttons. Those build through the untouched `BuildButton`, whose effective-default resolution
|
||||||
|
correctly yields outline-off — precisely the AP-192 deferral, unchanged. No authored `0x22`
|
||||||
|
anywhere; no authored `0x21` on any element reaching a *new* build path. ✔
|
||||||
|
|
||||||
|
- **NOTE.** `new UiMenu()` for `0x10000038` does not seed `Outline`/`OutlineColor` — but
|
||||||
|
neither does the pre-existing Type-6 arm, and `UiMenu`'s own doc records "No authored menu
|
||||||
|
element carries it today", which the Options menu `0x10000224` confirms (no `0x21`/`0x22`
|
||||||
|
on it or its label child). Unchanged, not regressed.
|
||||||
|
- **NOTE (folds into §5.2c).** `UiCheckboxBitfield64`'s synthesized rows have no outline
|
||||||
|
seeding either — but that is a symptom of bypassing the authored template, not a separate
|
||||||
|
defect. Building the rows through `AddItemFromTemplateList` gets the seam for free via
|
||||||
|
`BuildCheckbox`.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 7. Conformance tests vs research §10.1 — **ACCURATE**, coverage thin in two places
|
||||||
|
|
||||||
|
I spot-checked every pinned constant against the research doc and the fixtures. All correct:
|
||||||
|
|
||||||
|
| Test | Pins | Research §10.1 / §1.3 / §1.5 | Verdict |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `…TabControl_HasFourEntriesInAuthoredOrder_GameplayDefault` | `020D/0212 default`, `020E/0211`, `050B/050C`, `020F/0213`, `Assert.Single(IsDefault)` | identical | ✔ |
|
||||||
|
| `CharacterListBoxFixture_TemplateList_…` | 3 × `{0x2100002B, 216/217/218}` | identical | ✔ |
|
||||||
|
| `ConfigListBoxFixture_TemplateList_…` | 8, order `216,217,218,21A,222,220,21D,221` | identical | ✔ |
|
||||||
|
| `ChatListBoxFixture_TemplateList_…` | 9, + `0x10000520` | identical | ✔ |
|
||||||
|
| scrollbar linkage ×3 | `0x100001FB`, `0x10000201`, `0x10000201` | identical | ✔ |
|
||||||
|
| `CharacterListBox_TemplateMechanism_ReachesSixHeadersAndFortyNineToggles` | `{3,15,6,11,7,7}` = 49 under 6 headers; `6*22 + 49*20` content height | §7 counts; template heights 292×22 / 272×20 confirmed in fixture | ✔ |
|
||||||
|
| `UiTabControl_*` ×5 | default mount, exactly-one-visible, Open/Closed flip, click-through, no-op | matches `UIElement_Panel::Update` | ✔ |
|
||||||
|
|
||||||
|
Two gaps:
|
||||||
|
|
||||||
|
- **SHOULD-FIX.** No test pins the reader itself (see §1.4) — every "reader" test actually
|
||||||
|
reads a serialized field.
|
||||||
|
- **SHOULD-FIX.** `UiCheckboxBitfield64_AddChild_TracksMaskAndTogglesOnClick` deliberately
|
||||||
|
picks the single-bit `Error` mask (`0x04000000`) because it "round-trips cleanly" — which is
|
||||||
|
exactly the case that cannot distinguish acdream's all-set predicate from retail's any-set
|
||||||
|
predicate. A multi-bit mask (Combat `0x00600040`) against a partially-set current value is
|
||||||
|
the test that would have caught §5.2a.
|
||||||
|
- **NOTE.** The plan's OP2 contract cites "tab-button ↔ page-slot switching per lane A §5".
|
||||||
|
§5 is the Chat-tab row map; the tab control is §1.3 and its host is §1.4. The implementer
|
||||||
|
cited the right sections; the plan's cross-reference should be corrected so OP8 does not
|
||||||
|
chase §5.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Summary of classifications
|
||||||
|
|
||||||
|
**MUST-FIX**
|
||||||
|
1. `DatWidgetFactory.cs:110` — unconditional `Type 8 → UiTabControl` converts four shipped
|
||||||
|
Type-8 hosts (vendor `0x100000B8`, character `0x10000227`, spellbook `0x100002A8`, combat
|
||||||
|
`0x100000A2` — the last with **no** controller re-binder) and strips `UiDatElement` media
|
||||||
|
drawing + `ClickThrough` from the media-bearing vendor backdrop `0x1000008D`.
|
||||||
|
2. `DatWidgetFactory.cs:104` — the Type-5 gate's "none currently reach this factory" is false;
|
||||||
|
nine shipped ListBoxes author `P0x64` (effects, examine ×3, character ×2, spellbook,
|
||||||
|
mini-game). `EffectsUiControllerTests.cs:135-137` already encodes the pre-OP2 contract and
|
||||||
|
fails against real DAT data. Fixture staleness — not correctness — is what keeps the suite
|
||||||
|
green; regenerating the 27 fixtures is the acceptance gate for the fix.
|
||||||
|
3. `UiCheckboxBitfield64.cs:137-138` — checked-state predicate is all-bits-set; retail
|
||||||
|
(`Refresh @0x004859C0`) is any-bit-set.
|
||||||
|
4. `UiCheckboxBitfield64` / `DatWidgetFactory.cs:810` — the template `0x10000520` **does**
|
||||||
|
author content (`P0x64 → 0x2100002B/0x10000521`, plus `P0x10000082`/`P0x10000083`); retail
|
||||||
|
`CreateChildren @0x00485DF0` builds every row through `AddItemFromTemplateList(0)`. The
|
||||||
|
synthesized `ElementInfo` + invented `RowHeight = 14f` must be replaced by the authored
|
||||||
|
template path, and the false "unauthored in the dat" comments removed.
|
||||||
|
5. `UiTabControl.cs:7` / `DatWidgetFactory.cs:108` — cite `UIElement_Panel`
|
||||||
|
(`SetupTabPageHash @0x0046C2E0`, `Update @0x0046BD00`, `OpenTab @0x0046BE20`); the named
|
||||||
|
class `UIElement_TabControl` does not exist in the PDB.
|
||||||
|
6. `docs/architecture/retail-divergence-register.md` TS-72 — inaccurate as written; the click
|
||||||
|
math is in the decomp, the OR/AND-NOT half is confirmed right and the predicate half is
|
||||||
|
confirmed wrong. Also missing rows for the invented bitfield row geometry and the dropped
|
||||||
|
`0x64`/`0x10000082`/`0x10000083` authored data.
|
||||||
|
|
||||||
|
**SHOULD-FIX**
|
||||||
|
7. `ReadTabTable` must skip entries missing `0x30`/`0x31` (retail `0x0046C3E5`/`0x0046C3FC`).
|
||||||
|
8. `UiTabControl.FindDirectChild` → recursive descendant lookup (retail `GetChildRecursive`).
|
||||||
|
9. `defaultEntry ??= _tabs[0]` is invented; retail performs no switch when nothing authors
|
||||||
|
`0x32`.
|
||||||
|
10. Add reader-level unit tests for `0x2E`/`0x64`/`0x72` (fixture tests only pin serialized
|
||||||
|
values).
|
||||||
|
11. Add a multi-bit-mask `UiCheckboxBitfield64` test.
|
||||||
|
|
||||||
|
**NOTE**
|
||||||
|
12. `0x2F` MasterPropertyId and `tab != page` filters unimplemented (harmless today).
|
||||||
|
13. `0x72` is specifically the **vertical** scrollbar (`UIElement_Scrollable::OnSetAttribute
|
||||||
|
@0x00474630`); `0x71` (horizontal) is unread though authored in vendor/combat layouts.
|
||||||
|
14. `BroadcastElementMessage(0x2C)`, `SetMouseVisible(1)`, and the `idMessage 0x18`
|
||||||
|
visibility-follow path are unported — OP3 needs the first for §3.6 semantics.
|
||||||
|
15. The `UIOption_*` `P0x47/P0x48/P0x4B` triple (`0x21000041/0x10000397`) is unread across the
|
||||||
|
whole family, including the pre-existing checkbox mapping.
|
||||||
|
16. `UIOption_CheckboxBitfield64::AddChild` uses string table enum `0x10000006`, not
|
||||||
|
`0x10000003` — pin for OP5.
|
||||||
|
17. Research §1.5's "`0x10000521` … UNKNOWN consumer" and §10.5 U10 are now closed by the
|
||||||
|
committed fixture; the research doc should be updated.
|
||||||
|
18. The plan's OP2 contract cites "lane A §5" for the tab host; the correct sections are §1.3
|
||||||
|
(tab table) and §1.4 (host).
|
||||||
|
19. The commit message's fixture-drift paragraph understates the drift (three new fields, not
|
||||||
|
one pair).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Appendix — retail extracts used (all from `docs/research/named-retail/`)
|
||||||
|
|
||||||
|
| Symbol | Address | Used for |
|
||||||
|
|---|---|---|
|
||||||
|
| `UIElement::RegisterElementClass(8, UIElement_Panel::Create)` | `0x0046C6B7` | Type 8 identity |
|
||||||
|
| `UIElement_Panel::SetupTabPageHash` | `0x0046C2E0` | `0x2E` entry filters, default selection, `Update(page, tab)` tail |
|
||||||
|
| `UIElement_Panel::Update` | `0x0046BD00` | one-visible-page + `SetState(0xB/0xC)` + `Broadcast(0x2C)` |
|
||||||
|
| `UIElement_Panel::OpenTab` / `InqTabFromPage` / `ListenToElementMessage` | `0x0046BE20` / `0x0046BEB0` / `0x0046BF90` | switching entry points, `0x18`/`0x19`/`0x29` messages |
|
||||||
|
| `UIElement_ListBox::AddItemFromTemplateList` | `0x0046F2B0` | `0x64` array indexing |
|
||||||
|
| `UIElement_ListBox::AddItem(BaseProperty*)` | `0x0046F170` | `0x63`/`0x62` struct member order |
|
||||||
|
| `UIElement_ListBox::AddItem(did, elementId, parent)` | `0x0046EDC0` | `CreateChildElementByDid` seam |
|
||||||
|
| `UIElement_Scrollable::OnSetAttribute` | `0x00474630` | `0x71` horizontal / `0x72` vertical |
|
||||||
|
| `UIElement_Scrollable::GetScrollbarPointer_` | `0x00473EC0` | `FindRelative` + `DynamicCast(0xB)` validation |
|
||||||
|
| `UIOption_CheckboxBitfield64::CreateChildren` | `0x00485DF0` | rows built from template index 0 |
|
||||||
|
| `UIOption_CheckboxBitfield64::ListenToElementMessage` | `0x00485AE0` | `SetBitsOnOrOff` click math |
|
||||||
|
| `UIOption_CheckboxBitfield64::Refresh` | `0x004859C0` | any-bit-set checked predicate |
|
||||||
|
| `UIOption_CheckboxBitfield64::AddChild` | `0x004862A0` | `ChildInfo{mask,label,tooltip}`, table enum `0x10000006` |
|
||||||
|
| `UIElement::RegisterElementClass(0x10000037/38/36/44, …)` | `0x0048522A` / `0x0048436A` / `0x00484A3A` / `0x00485EDA` | the four UIOption class ids |
|
||||||
|
|
||||||
|
Fixture decodes were performed with a throwaway JSON reader over the committed
|
||||||
|
`tests/AcDream.App.Tests/UI/Layout/fixtures/*.json` (scratchpad only, not committed). Nothing
|
||||||
|
in the repository was modified except this document.
|
||||||
Loading…
Add table
Add a link
Reference in a new issue