fix(ui): OP8 round-2 residuals — inert-row conflict exclusion, DAT-default display, injectivity pin
R1 (code half): store-only rows (MappedAction null) are excluded from the conflict universe — they never reach the InputDispatcher, so a chord they display cannot collide; counting them made the ten Camera Alternate arrow-key defaults trip a false N-way confirm on any arrow rebind. Mapped cross-context sharing (retail's ConflictingMaps — the combat cluster) remains deferred as ISSUES #373 with the OP8 gate script now carrying the explicit do-not-file warning. SHOULD: unmapped rows with no persisted chords display their DAT defaults (retail shows the arrow keys; blank read as 'unbound') — display-only, the store is untouched until the row itself is edited; the independence test updated to pin the new display semantics while keeping its storage-isolation asserts. Injectivity of RetailActionIdentityTable is now test-enforced (load-bearing for both M1's per-row activation capture and M2's de-alias). R2: AP-203 addendum names the ten same-verb-sibling-live rows and the conflict exclusion. Full Release suite in this worktree: 13,155 passed / 4 skips / 0 failed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
b1968ce980
commit
f1d502072e
4 changed files with 66 additions and 6 deletions
|
|
@ -185,7 +185,7 @@ AP-94..AP-112 for the confirmed retail-UI completion gaps.
|
|||
|---|---|---|---|---|---|
|
||||
| AP-201 | **Filed 2026-08-11 at the OP5 review-fix round (S2), predates OP5 but was made user-visible by it.** `UiTemplateListBox`'s internal row viewport (`UiScrollablePanel.LayoutScrollableChildren`) culls a child WHOLE — `child.Visible = top >= -0.5f && top + child.Height <= Height + 0.5f` — rather than clipping the visible portion of a row that straddles the viewport edge, because the UI renderer has no scissor stack. Retail's own `UIElement_ListBox`/scroll-region rendering clips partially-visible rows at the pixel boundary, same as any native scroll view. Every row in this viewport was 8-36px until Campaign OP slice OP5 added five self-sized filter blocks (12x20=240px / 13x20=260px, AP-195) to the Chat tab's ~560px viewport; a 240-260px block straddling the viewport edge at a given scroll offset now disappears ENTIRELY (a visible "pop") instead of clipping, where the pre-OP5 8-36px rows made the same all-or-nothing cull read as ordinary row-granular scrolling. | `src/AcDream.App/UI/UiScrollablePanel.cs:69` (the cull predicate); consumed by `src/AcDream.App/UI/UiTemplateListBox.cs` (`Viewport`) — the Character/Chat/Config Options-panel tabs and any other controller-built row list sharing this viewport | A scissor stack does not exist anywhere in the retained-UI renderer yet (class's own doc comment, `UiScrollablePanel.cs:8-12`, predates this row); whole-row culling is a correct, cheap stand-in for every list whose rows are small relative to the viewport, which was true for every consumer before OP5. | A tall block (any future row taller than roughly the viewport's own height, not just OP5's filter blocks) can vanish completely for a range of scroll offsets instead of showing a partial view — the OP5 gate script's own step 2 documents the exact symptom so it is not mistaken for a self-sizing regression (`docs/research/2026-08-11-campaign-op-test-script.md`). Scrolling further always restores the block whole; no data or state is lost, only the presentation pops. | No scissor-stack retail oracle needed — this is a stand-in for ordinary native clip-rect rendering every GUI toolkit (including retail's own) provides; issue #371 tracks adding a real per-row clip rect to `UiScrollablePanel` |
|
||||
| AP-202 | **Filed 2026-08-11 at Campaign OP slice OP8 (D4).** Configure Keyboard persists every rebind to `keybinds.json` only. Retail's own storage is a `<Documents>\Asheron's Call\<name>.keymap` text file (`CInputManager_WIN32::SaveKeyMap @0x00686C20`, `PFileParser`), with Load-File/Save-As buttons for NAMED keymap profiles and a `keymap` key in `UserPreferences.ini` selecting which one loads at startup (research doc §5.7). D4 chose the existing, tested `keybinds.json` schema over building a second `PFileParser`-compatible text codec + named-profile management; this row's the Load File/Save As buttons on the Configure Keyboard screen (`0x10000027`/`0x10000029`) are wired but INERT. | `src/AcDream.App/UI/Layout/KeyboardConfigController.cs` (`WireScreenButtons`'s Load/Save-As no-op); `src/AcDream.UI.Abstractions/Input/KeyBindings.cs` (`SaveToFile`/`LoadOrDefault`) | `keybinds.json` already round-trips every retail action this screen can bind (identity table + the DAT-defaults conformance test), so the ONLY capability lost is exchanging `.keymap` files with a real retail client or another acdream install by named profile — a real feature gap, not a correctness gap. | A user who expects to export/import a named `.keymap` profile (e.g. to share a control layout with a retail-client friend) cannot; every rebind still works and persists locally. | `docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §5.7-§5.8; `CInputManager_WIN32::SaveKeyMap @0x00686C20`; `gmKeyboardUI::SaveKeymap @0x004DCF90` |
|
||||
| AP-203 | **Filed 2026-08-11 at Campaign OP slice OP8.** Of the DAT ActionMap's 306 user-bindable rows, `RetailActionIdentityTable` (`src/AcDream.UI.Abstractions/Input/RetailActionIdentityTable.cs`) resolves roughly half to a live acdream `InputAction`; the rest render, bind, conflict-check, and persist (via `RetailUnmappedKeyBindings`, a sibling `*-unmapped.json` file) exactly like any other row, but have no live gameplay consumer to dispatch through. The two largest classes: 82 of 87 Emote rows (only Cry/Laugh/Cheer/Wave/PointState dispatch an animation today — acdream has no general emote-animation player), and all 48 CharacterSettings hotkey rows (ctx `0x10000008` — these are hotkeys for the SAME `PlayerOption`/`CharacterOptions` preference bits OP1's `CharacterOptionTable` and OP4's Character-tab checkboxes already model; wiring "press this key, flip that same server-synced bit" is a real feature, a hotkey-to-option-toggle dispatcher, that does not exist anywhere in acdream yet). Smaller residuals: Spell Slot 10-12, Quickslot 10-13 (both hit a PRE-EXISTING `InputAction` enum gap this slice did not introduce), and roughly twenty UI-panel-toggle rows for panels acdream has no analog for (Vitae/Link Status/House/Map/Character Info/the two Magic panels/...). | `src/AcDream.UI.Abstractions/Input/RetailActionIdentityTable.cs` (class doc has the full accounting); `src/AcDream.App/UI/Layout/KeyboardConfigController.cs` (`CurrentForUnmapped`/`SetForUnmapped`) | Guessing a mapping for an ambiguous row risks silently misrouting a rebind to the wrong gameplay action (worse than an honest "not wired yet" — the identity table's own class doc states this directly); every mapping that WAS added was cross-verified two ways (label match + DAT-default-vs-`KeyBindings.RetailDefaults()` byte match, see `RetailActionIdentityRoundTripTests`). | A user rebinds e.g. an emote or a CharacterSettings hotkey on the Configure Keyboard screen and the binding persists but has no observable in-game effect — matches retail's OWN screen shape (the row exists, is bindable) while honestly lacking retail's gameplay behavior behind it. | `docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §5.1-§5.3; live-DAT probe 2026-08-11 (306-row/six-ActionClass accounting, `RetailActionMapReaderTests`) |
|
||||
| AP-203 | **Filed 2026-08-11 at Campaign OP slice OP8.** Of the DAT ActionMap's 306 user-bindable rows, `RetailActionIdentityTable` (`src/AcDream.UI.Abstractions/Input/RetailActionIdentityTable.cs`) resolves roughly half to a live acdream `InputAction`; the rest render, bind, conflict-check, and persist (via `RetailUnmappedKeyBindings`, a sibling `*-unmapped.json` file) exactly like any other row, but have no live gameplay consumer to dispatch through. The two largest classes: 82 of 87 Emote rows (only Cry/Laugh/Cheer/Wave/PointState dispatch an animation today — acdream has no general emote-animation player), and all 48 CharacterSettings hotkey rows (ctx `0x10000008` — these are hotkeys for the SAME `PlayerOption`/`CharacterOptions` preference bits OP1's `CharacterOptionTable` and OP4's Character-tab checkboxes already model; wiring "press this key, flip that same server-synced bit" is a real feature, a hotkey-to-option-toggle dispatcher, that does not exist anywhere in acdream yet). Smaller residuals: Spell Slot 10-12, Quickslot 10-13 (both hit a PRE-EXISTING `InputAction` enum gap this slice did not introduce), and roughly twenty UI-panel-toggle rows for panels acdream has no analog for (Vitae/Link Status/House/Map/Character Info/the two Magic panels/...). | `src/AcDream.UI.Abstractions/Input/RetailActionIdentityTable.cs` (class doc has the full accounting); `src/AcDream.App/UI/Layout/KeyboardConfigController.cs` (`CurrentForUnmapped`/`SetForUnmapped`) | Guessing a mapping for an ambiguous row risks silently misrouting a rebind to the wrong gameplay action (worse than an honest "not wired yet" — the identity table's own class doc states this directly); every mapping that WAS added was cross-verified two ways (label match + DAT-default-vs-`KeyBindings.RetailDefaults()` byte match, see `RetailActionIdentityRoundTripTests`). | A user rebinds e.g. an emote or a CharacterSettings hotkey on the Configure Keyboard screen and the binding persists but has no observable in-game effect — matches retail's OWN screen shape (the row exists, is bindable) while honestly lacking retail's gameplay behavior behind it. ADDENDUM (2026-08-11, OP8 re-review round 2): this row's scope EXPLICITLY includes the ten CameraAlternateControls (InputMap 0x6) rows the M2 de-alias narrowed to store-only — a case the generic wording understated because their SIBLING rows (InputMap 0x5, the same verbs) ARE live on the same screen: the 0x6 rows display their DAT-default arrow keys (display-only seeding), persist user edits, and drive nothing; only the 0x5 scheme reaches the InputDispatcher. Store-only rows are also EXCLUDED from the conflict universe (they cannot actually collide) — mapped cross-context sharing remains ISSUES #373. | `docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §5.1-§5.3; live-DAT probe 2026-08-11 (306-row/six-ActionClass accounting, `RetailActionMapReaderTests`) |
|
||||
| ~~AP-204~~ | **RETIRED 2026-08-11 at the OP8 rework (M3, combined review).** Originally filed for two narrowings: (1) silent auto-reassign on a cross-row conflict instead of retail's modal `OpenOverwriteBindingDialog`, and (2) OK/Cancel wired as left-click instead of retail's right-click-release gesture. (1) is FIXED — `KeyboardConfigController.BeginSlotCapture` now opens a real confirm dialog through `RetailDialogFactory.MakeConfirmation` (the SAME seam `GameplayConfirmationController` uses) BEFORE reassigning, listing every conflicting row (N-way), and only applies on accept; decline leaves every row untouched. (2) is NOT fixed and does not warrant its own row: it is authored-input-only with zero observable difference to a user (retail's own right-click-release on just this pair of buttons carries no distinguishing visual cue either, and every other Campaign OP button already uses left-click) — noted as a code comment at the OK/Cancel wiring site instead of a register row, matching this register's convention of reserving rows for divergences that could produce an observable symptom. | `src/AcDream.App/UI/Layout/KeyboardConfigController.cs` (`FindConflicts`/`BeginSlotCapture`; `WireScreenButtons`'s OK/Cancel `OnClick` comment); `src/AcDream.App/UI/RetailUiRuntime.cs` (`MountKeyboardConfig`'s `ConfirmOverwrite` wiring) | — | — | `docs/research/2026-08-10-keyboard-config-and-gameplay-tab.md` §5.4 (`UIOption_ActionKeyMap::KeyHitHandler @0x00489570`, `OpenOverwriteBindingDialog @0x00488BF0`, `OpenCantOverwriteBindingDialog @0x00489300`) and §5.5 (OK/Cancel `idMessage 0x19` gesture) |
|
||||
| AP-194 | `CharacterOptionTable`'s `ClientDefault` column (what the Character tab's Defaults button restores) disagrees with the raw constructor default word for three ids: `ConfirmVolatileRareUse` (`0x2D`), `ShowHelm` (`0x2F`), and `ShowCloak` (`0x32`) are all ON in retail's constructor default `CharacterOptions2 = 0x00948700` (`PlayerModule::PlayerModule @0x005D51F0`, byte-verified literal write) but report default-OFF via `PlayerModule::GetDefaultOptionValue @0x005D2A30`, whose own per-option table stops at id `0x2A` and returns `false` for everything past it. This is retail's OWN behavior, reproduced deliberately — the Defaults button does not reproduce a fresh `PlayerModule`. **CONFIRMED 2026-08-11 at Campaign OP slice OP4**: `CharacterOptionsPageController` seeds every `BoolOptionRow`'s default directly from this column (`EveryRow_DefaultValue_MatchesCharacterOptionTableClientDefault`, `tests/AcDream.App.Tests/UI/Layout/CharacterOptionsPageControllerTests.cs`); the directive below was followed, not re-litigated. OP4 also independently traced retail's OWN mechanism for the Character tab specifically — `UIOption_Checkbox::SetPlayerOption @0x00486e80` (pseudo-C line 147375) sets `m_default` directly from `GetDefaultOptionValue`, confirming this column (not the separate `DBPropertyCollection`/`InqDefaultGameplayOptionProperty` mechanism that governs the Chat/Config tabs' `m_propName`-bound rows) is the correct and ONLY source for this tab. | `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` (`ClientDefault` column; see the type's XML doc); `src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs` | Byte-verified at both addresses (wire research §2.5 for the constructor literals, §8.2 for `GetDefaultOptionValue`'s own table and bounds check) — this is not a guess, it is retail's documented quirk. "Fixing" it to match the constructor default would make acdream's Defaults button MORE correct than retail's own, which is the opposite of this project's goal. | A future OP-campaign slice (OP4, the Character tab's Defaults button) must consult THIS column, not the constructor default word, or a future reader may "fix" this back and silently diverge from retail. | `PlayerModule::GetDefaultOptionValue @0x005D2A30`; `UIOption_Checkbox::SetPlayerOption @0x00486e80` (N-4 anchor-column correction, OP4 review-fix round 2026-08-11 — was mislabeled `PlayerModule::SetPlayerOption`, same address, wrong class); `PlayerModule::PlayerModule @0x005D51F0`; `docs/research/2026-08-10-set-character-options-wire.md` §8.2 |
|
||||
| AP-193 | Character option id `0x34` (`ListenToPKDeathMessages` / "Listen to PK death messages") is mapped to `CharacterOptions2` bit `0x02000000` and modeled as a batched (non-auto-save) option purely on ACE's own enum — the id does not exist in the 2013 EoR PDB (`PlayerOption` there terminates at `TotalNumberOfPlayerOptions_PlayerOption = 0x34`), so neither the mask nor its `IsAutoSaveOption`/`GetDefaultOptionValue` classification is byte-verifiable against our binary. | `src/AcDream.Runtime/Gameplay/CharacterOptionTable.cs` (`HearPkDeathMessages` row) | The user's retail memory (and ACE's own `CharacterOption` enum) both carry this option; shipping wire+store coverage for it is strictly better than omitting the row the Character tab's screenshots show, and ACE never actually reads the bit server-side (`PlayerFactory.cs:659-660` — "possibly was added to Defaults post PDB we have"), so a wrong id/mask/auto-save guess here has zero server-observable consequence either way. | If the final EoR client's real id/mask/auto-save classification ever surfaces (a later PDB, or a byte-level trace against a 2015+ binary), this row's values may be wrong and need correcting — until then treat them as ACE-sourced, not retail-verified. | ACE `PlayerFactory.cs:659-660`, `CharacterOptions2.cs` (`ListenToPKDeathMessages = 0x02000000`); `named-retail/acclient.h:4162-4218` (2013 `PlayerOption` terminates at `0x34`); `docs/research/2026-08-10-set-character-options-wire.md` §8.1 |
|
||||
|
|
|
|||
|
|
@ -855,6 +855,22 @@ Three live runs against local ACE (`127.0.0.1:9000`, `+Acdream`,
|
|||
|
||||
## OP8 — Configure Keyboard
|
||||
|
||||
**Known, tracked behaviors — do NOT file as defects (read before testing):**
|
||||
|
||||
- **Shared combat keys prompt a false conflict (ISSUES #373).** Retail
|
||||
legitimately shares Insert/Delete/End/PageUp/PageDown across the three
|
||||
combat contexts (Melee/Missile/Magic) via the DAT's `ConflictingMaps`
|
||||
table, which acdream does not read yet. Rebinding any of those keys — or
|
||||
any chord one combat context already holds — will show a conflict
|
||||
confirmation retail would not. DECLINE the prompt and move on; the
|
||||
binding you declined stays untouched. Report only if declining CHANGES a
|
||||
binding anyway.
|
||||
- **Camera Alternate Controls rows are display/store-only (AP-203).** They
|
||||
show retail's authored arrow-key defaults and accept edits, but do not
|
||||
drive the camera — only the primary Camera scheme is live. Verify they
|
||||
RENDER (arrow keys visible); do not expect camera behavior from them.
|
||||
|
||||
|
||||
The screen is retail's own separate full-screen window (`gmKeyboardUI`,
|
||||
LayoutDesc `0x21000009`), NOT a fifth tab of the Options panel. It opens
|
||||
from the Gameplay tab's Configure Keyboard button — **the OP3 INERT
|
||||
|
|
|
|||
|
|
@ -366,10 +366,19 @@ public sealed class KeyboardConfigController
|
|||
? (liveBindings[0].Activation, liveBindings[0].Scope)
|
||||
: (ActivationType.Press, InputScope.Game);
|
||||
|
||||
IReadOnlyList<KeyChord> defaults = DatDefaultsToChords(row.DefaultBindings);
|
||||
IReadOnlyList<KeyChord> storedUnmapped = mapped
|
||||
? Array.Empty<KeyChord>()
|
||||
: bindings.CurrentForUnmapped((row.InputMapId, row.ActionId));
|
||||
// OP8 re-review round 2 (SHOULD-FIX): an unmapped/store-only row with
|
||||
// no persisted chords displays its DAT DEFAULTS — retail shows the
|
||||
// authored bindings (the Camera Alternate rows' arrow keys) and a
|
||||
// blank row misreads as "unbound". Display-only: nothing here feeds
|
||||
// the InputDispatcher, and the store only gains the defaults if the
|
||||
// user actually edits the row (the apply closure below).
|
||||
IReadOnlyList<KeyChord> initial = mapped
|
||||
? liveBindings.Select(b => b.Chord).ToArray()
|
||||
: bindings.CurrentForUnmapped((row.InputMapId, row.ActionId));
|
||||
IReadOnlyList<KeyChord> defaults = DatDefaultsToChords(row.DefaultBindings);
|
||||
: storedUnmapped.Count > 0 ? storedUnmapped : defaults;
|
||||
|
||||
var model = new ActionKeyMapOptionRow(initial, defaults, apply: value =>
|
||||
{
|
||||
|
|
@ -538,6 +547,15 @@ public sealed class KeyboardConfigController
|
|||
foreach (RowView other in _rows)
|
||||
{
|
||||
if (ReferenceEquals(other, exclude)) continue;
|
||||
// OP8 re-review round 2 R1: store-only rows (MappedAction null —
|
||||
// the Camera Alternate scheme, Emote/CharacterSettings hotkeys)
|
||||
// never reach the InputDispatcher, so a chord they display cannot
|
||||
// actually collide with anything; counting them made the ten
|
||||
// arrow-key defaults trip a false N-way confirm on any arrow
|
||||
// rebind. Retail-mapped cross-context sharing (ConflictingMaps —
|
||||
// the Insert/Delete/End/PageUp/PageDown combat cluster) remains
|
||||
// deferred as ISSUES #373; only INERT rows are excluded here.
|
||||
if (other.MappedAction is null) continue;
|
||||
if (other.Model.Current.Contains(chord))
|
||||
rows.Add(other);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -557,12 +557,19 @@ public sealed class KeyboardConfigControllerTests
|
|||
Assert.Equal(InputAction.CameraRotateLeft, ctx5.MappedAction);
|
||||
Assert.Null(ctx6.MappedAction); // unmapped — no live dual-binding infrastructure (M2)
|
||||
|
||||
// Rebinding ctx5's row must not touch ctx6's storage, and vice versa.
|
||||
// Round-2 SHOULD-FIX: an unmapped row with no persisted chords now
|
||||
// DISPLAYS its DAT defaults (retail shows the arrow keys; blank read
|
||||
// as "unbound"). Display-only — storage stays untouched until the
|
||||
// user edits THIS row.
|
||||
Assert.NotEmpty(ctx6.Model.Current);
|
||||
var ctx6InitialDisplay = ctx6.Model.Current.ToArray();
|
||||
|
||||
// Rebinding ctx5's row must not touch ctx6's display or storage.
|
||||
ctx5.KeyButtons[0].OnClick!.Invoke();
|
||||
fake.Capture(ChordW);
|
||||
Assert.Contains(ChordW, ctx5.Model.Current);
|
||||
Assert.Empty(ctx6.Model.Current); // ctx6 never seeded/written by ctx5's edit
|
||||
Assert.Empty(fake.Unmapped); // ctx6 untouched — only ctx5's mapped write happened
|
||||
Assert.Equal(ctx6InitialDisplay, ctx6.Model.Current); // unchanged by ctx5's edit
|
||||
Assert.Empty(fake.Unmapped); // ctx6's STORE untouched — display seeding writes nothing
|
||||
|
||||
ctx6.KeyButtons[0].OnClick!.Invoke();
|
||||
fake.Capture(ChordA);
|
||||
|
|
@ -584,4 +591,23 @@ public sealed class KeyboardConfigControllerTests
|
|||
|
||||
Assert.Null(controller);
|
||||
}
|
||||
|
||||
// ── OP8 re-review round 2: identity-map injectivity is load-bearing for
|
||||
// BOTH M1 (per-row activation capture) and M2 (no dual rows editing one
|
||||
// action) — pin it so a future mapping addition cannot silently alias. ──
|
||||
|
||||
[Fact]
|
||||
public void IdentityMap_IsInjective_NoTwoRowsShareOneInputAction()
|
||||
{
|
||||
var seen = new Dictionary<InputAction, (uint MapId, uint ActionId)>();
|
||||
foreach (((uint mapId, uint actionId), InputAction action) in RetailActionIdentityTable.Map)
|
||||
{
|
||||
Assert.False(
|
||||
seen.TryGetValue(action, out (uint MapId, uint ActionId) prior),
|
||||
$"InputAction.{action} is mapped by BOTH (0x{prior.MapId:X}, 0x{prior.ActionId:X}) "
|
||||
+ $"and (0x{mapId:X}, 0x{actionId:X}) — aliasing reintroduces the M2 twin-row clobber.");
|
||||
seen[action] = (mapId, actionId);
|
||||
}
|
||||
Assert.True(seen.Count > 100, $"sanity: only {seen.Count} mapped actions seen");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue