From ac0304dcf0d41a4728711495458057dbf13d6887 Mon Sep 17 00:00:00 2001 From: Erik Date: Tue, 11 Aug 2026 06:36:14 +0200 Subject: [PATCH] =?UTF-8?q?fix(ui,runtime):=20OP4=20re-review=20residuals?= =?UTF-8?q?=20R1-R4=20(coordinator=20pass)=20=E2=80=94=20OP4=20CLOSED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R1: the timestamp prefix moves from ChatLog.Append (which stamped the stored BODY, rendering 'Alice says, "13:05:09 hi"') to ChatVM's display composition — FormatTimestampPrefix(entry.Received) prepends the COMPOSED line, matching retail's separate-leading-string model (fprintf("%ls%ls", ts, text) @0x00563e5b; AddTextToScroll receives composed lines). The prefix renders entry.Received in LOCAL time (retail strftime), invariant literal colons. The ten defect-pinning test cases across ChatLogTests/RuntimeCommunicationStateTests are rewritten to pin the corrected contract (stored bodies stay clean; the composed line carries the stamp outside the quotes — ChatVMTests). R2: open option-bearing panels converge on every PlayerDescription seed: OptionPage.ReloadFromLive (per-row live re-read + gating re-eval, NO AfterApply flush — the seed just cleared the dirty module), OptionsPanelController.OnServerOptionsSeeded (active page), CombatUiController.OnServerOptionsSeeded (SyncControls), wired through RuntimeSettingsController.ServerOptionsSeeded from the same factory hook LockUI already uses. Retail cannot reach this state (its panels close across login); the adaptation exists because retained panels survive the session boundary — documented at the seam. R3: tests drive the refresh widget push (model AND checkbox converge) and ReloadFromLive's no-flush contract. R4: AP-196 addendum names the headless AutoRepeatAttack false->true effective-default flip and the characterOptions escape hatch. Full Release suite: 13,083 passed / 4 skipped / 0 failed. Co-Authored-By: Claude Fable 5 --- .../retail-divergence-register.md | 2 +- .../InteractionRetainedUiComposition.cs | 9 +++ .../Net/LiveSessionRuntimeFactory.cs | 4 ++ .../Settings/RuntimeSettingsController.cs | 15 ++++ .../UI/Layout/CombatUiController.cs | 6 ++ src/AcDream.App/UI/Layout/OptionPageModel.cs | 19 +++++ .../UI/Layout/OptionsPanelController.cs | 10 +++ src/AcDream.Core/Chat/ChatLog.cs | 21 +++--- .../Panels/Chat/ChatVM.cs | 18 ++++- .../UI/Layout/OptionPageModelTests.cs | 59 ++++++++++++++++ tests/AcDream.Core.Tests/Chat/ChatLogTests.cs | 69 ++++--------------- .../RuntimeCommunicationStateTests.cs | 65 +++++------------ .../ChatVMTests.cs | 33 +++++++++ 13 files changed, 216 insertions(+), 114 deletions(-) diff --git a/docs/architecture/retail-divergence-register.md b/docs/architecture/retail-divergence-register.md index 9257058d..37147479 100644 --- a/docs/architecture/retail-divergence-register.md +++ b/docs/architecture/retail-divergence-register.md @@ -185,7 +185,7 @@ AP-94..AP-112 for the confirmed retail-UI completion gaps. |---|---|---|---|---|---| | 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 | -| AP-196 | **Filed 2026-08-11 at the OP4 review-fix round (MUST-FIX 3 / blast M2).** OP4's Group-C re-point (`AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget`/`VividTargetingIndicator`/`CoordinatesOnRadar`/`LockUI`/`AcceptLootPermits`/`ToggleRun`) moved these options' EFFECTIVE default source from the client-local `GameplaySettings.Default` record (or a persisted `settings.json`) to the retail constructor word, read live through `RuntimeCharacterOptionsState`/`CharacterOptionTable.ClientDefault` — a fresh install's observable default for `ViewCombatTarget` FLIPS from `true` (the old `GameplaySettings.Default`) to `false` (retail's real `CharacterOptions1` default — bit `0x80` is clear in `0x50C4A54A`). This fix round additionally DELETED the three now-fully-orphaned `GameplaySettings` fields (`AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget`) and their `RuntimeSettingsController` mirror properties/`SetCombatGameplay` method outright — the Combat panel's own three LEDs (`CombatUiController`) now read/write the SAME `RuntimeCharacterOptionsState` seam the Character tab's rows use, closing the "two writable copies" divergence the blast review found. The other five re-pointed options remain present in `GameplaySettings` as WRITE-BEHIND persistence/draft mirrors (`settings.json` still records the last-known value for restart continuity and Settings-panel draft consistency) — but the AUTHORITATIVE read for gameplay behavior is always the live server bit. | `src/AcDream.UI.Abstractions/Panels/Settings/GameplaySettings.cs`; `src/AcDream.App/Settings/RuntimeSettingsController.cs`; `src/AcDream.App/UI/Layout/CombatUiController.cs`; `src/AcDream.App/Combat/LiveCombatAttackOperations.cs` (`CharacterOptionCombatSettingsSource`) | Server-authoritative reads matching CH3's established precedent are strictly more correct than a client-local snapshot that can silently diverge from the character's real server state; the three deleted fields had zero remaining production consumers once the Combat panel's LEDs were re-pointed, so deleting rather than deprecating avoids a permanently-dead second store. | A fresh install (or a character whose `settings.json` predates this change) shows `ViewCombatTarget` OFF by default where it previously showed ON — a deliberate correctness fix, not a regression, if a future report cites "combat camera doesn't track by default." Any future code that still expects `GameplaySettings` to carry `AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget` fails to compile — a forcing function, not a silent gap. | `CharacterOptions1.Default = 0x50C4A54A` (`PlayerModule::PlayerModule @0x005D51F0`); `CharacterOptionTable.cs`'s `ClientDefault` column; `docs/research/2026-08-11-op4-review-blast.md` §S3 | +| AP-196 | **Filed 2026-08-11 at the OP4 review-fix round (MUST-FIX 3 / blast M2).** OP4's Group-C re-point (`AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget`/`VividTargetingIndicator`/`CoordinatesOnRadar`/`LockUI`/`AcceptLootPermits`/`ToggleRun`) moved these options' EFFECTIVE default source from the client-local `GameplaySettings.Default` record (or a persisted `settings.json`) to the retail constructor word, read live through `RuntimeCharacterOptionsState`/`CharacterOptionTable.ClientDefault` — a fresh install's observable default for `ViewCombatTarget` FLIPS from `true` (the old `GameplaySettings.Default`) to `false` (retail's real `CharacterOptions1` default — bit `0x80` is clear in `0x50C4A54A`). This fix round additionally DELETED the three now-fully-orphaned `GameplaySettings` fields (`AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget`) and their `RuntimeSettingsController` mirror properties/`SetCombatGameplay` method outright — the Combat panel's own three LEDs (`CombatUiController`) now read/write the SAME `RuntimeCharacterOptionsState` seam the Character tab's rows use, closing the "two writable copies" divergence the blast review found. The other five re-pointed options remain present in `GameplaySettings` as WRITE-BEHIND persistence/draft mirrors (`settings.json` still records the last-known value for restart continuity and Settings-panel draft consistency) — but the AUTHORITATIVE read for gameplay behavior is always the live server bit. | `src/AcDream.UI.Abstractions/Panels/Settings/GameplaySettings.cs`; `src/AcDream.App/Settings/RuntimeSettingsController.cs`; `src/AcDream.App/UI/Layout/CombatUiController.cs`; `src/AcDream.App/Combat/LiveCombatAttackOperations.cs` (`CharacterOptionCombatSettingsSource`) | Server-authoritative reads matching CH3's established precedent are strictly more correct than a client-local snapshot that can silently diverge from the character's real server state; the three deleted fields had zero remaining production consumers once the Combat panel's LEDs were re-pointed, so deleting rather than deprecating avoids a permanently-dead second store. | A fresh install (or a character whose `settings.json` predates this change) shows `ViewCombatTarget` OFF by default where it previously showed ON — a deliberate correctness fix, not a regression, if a future report cites "combat camera doesn't track by default." Any future code that still expects `GameplaySettings` to carry `AutoTarget`/`AutoRepeatAttack`/`ViewCombatTarget` fails to compile — a forcing function, not a silent gap. | `CharacterOptions1.Default = 0x50C4A54A` (`PlayerModule::PlayerModule @0x005D51F0`); `CharacterOptionTable.cs`'s `ClientDefault` column; `docs/research/2026-08-11-op4-review-blast.md` §S3. ADDENDUM (2026-08-11, re-review R4): the HEADLESS host's effective defaults changed in the same re-point — `HeadlessGameplayOperations` previously hardcoded `AutoRepeatAttack = false` / `AutoTarget = true` and now reads the live bits, so a bot that declares neither option observes `AutoRepeatAttack` flip `false → true` (the retail constructor word has it ON); a bot that needs the old behavior declares `"AutoRepeatAttack": false` in its `characterOptions` block (OP7) | | AP-197 | **Filed 2026-08-11 at the OP4 review-fix round (SF-1/S4).** "Display Timestamps" hardcodes retail's `PlayerModule` constructor-default format string `"%#H:%M:%S "` rather than reading the PER-CHARACTER override `GenericQualitiesData::InqString(m_pPlayerOptionsData, 1, &m_TimeStampFormat)` carries when the wire's `GenericQualitiesData` string-key `1` is populated — acdream's `PlayerDescription` parser reads and discards that field (wire research doc: "timestamp string (`0x80`) \| read, discarded \| ❌ \| never sent"). | `src/AcDream.Core/Chat/ChatLog.cs` (`FormatTimestampPrefix`); parser site cited at `docs/research/2026-08-10-set-character-options-wire.md:647` | The 2013 client's own constructor default is the only format any fresh/default character would ever show — retail ships no options-panel control that authors a custom one — so hardcoding the one value every real player sees is a safe, honest approximation until a consumer needs the per-character override. | A character whose account somehow carries a non-default persisted timestamp format (a modded/legacy server, or a hypothetical later retail patch exposing a UI for it) sees acdream render the DEFAULT format instead of their stored one — cosmetic only (still a valid H:MM:SS-shaped timestamp), never a wire or data-loss risk. | `PlayerModule::PlayerModule @0x005D51F0` (ctor default literal); `GenericQualitiesData::InqString` call site (wire doc §3.3); `docs/research/2026-08-10-set-character-options-wire.md` U6 | | AP-172 | **Filed 2026-08-08 (#354 fix — spell-bar drag reorder).** Retail removes a lifted favorite from `PlayerModule` (+ UI list + wire) the instant a drag starts and the remaining shortcuts visibly slide left to close the gap for the rest of the gesture (`RecvNotice_ItemListBeginDrag` → `RemoveSpellFromMenu`, live). acdream's controller performs the same PlayerModule/wire removal at drag-begin but DEFERS the whole favorite-list's visual rebuild until the drag concludes (drop or off-bar release) — the lifted cell's icon stays visible in its old slot and siblings do not slide until release, instead of reflowing continuously through the gesture. `DropFavorite` compensates by porting retail's own `AddFavorite`-side index adjustment (decrement the target index by one when the lifted item's original index was before it) against the now-intentionally-stale sibling numbering, so the FINAL landed position is byte-identical to retail's in every case exercised (`DragFavoriteOntoAnotherSlot_ThroughTheRealPointerPipeline_ReordersAndSyncsWire`). **NARROWED + CORRECTED 2026-08-08 (drop-ring change).** Correction: this row originally claimed empty-tail-slot drops were "already-live-count-relative and are untouched" — false. The #354 `-1` adjustment sat inside `DropFavorite`, which the empty-cell path also calls, so its live-count-clamped (post-lift-numbered) index was double-corrected: lifting a non-last favorite onto the empty tail landed it second-to-last instead of last. `FavoriteDropIndex` is now THE one landing computation and applies retail's rule exactly — the `-1` is gated on the lifted spell's pre-lift-numbered removal site (retail's `RemoveSpellFromMenu`-return-gated decrement @0x004C7157), which for a live-numbered empty-tail target is retail's `RemoveSpellFromMenu == -1` no-adjustment case (test `SpellFavoriteDrag_DroppedOnTheEmptyTail_AppendsAtTheEnd` fails against the double-correcting code). Narrowing: the mid-drag presentation now includes retail's authored drag-over Accept ring — `SpellCastSubMenu::OnItemListDragOver` @0x004C5990 setting the per-cell authored DragAccept child (element 0x1000045A, `UIElement_UIItem::PostInit` @0x004E1870) to `ItemSlot_DragOver_Accept` (UIStateId 0x10000040 → authored art 0x060011F9) on the hovered cell while a spell drag is live, cleared on leave/drop (`UiCatalogSlot.DragOverAcceptance` → `UiItemSlot.DrawDragAcceptOverlay`), with the ring and the drop sharing `FavoriteDropIndex` so the ring cannot promise a different landing. | `src/AcDream.App/UI/Layout/SpellcastingUiController.cs` (`BeginFavoriteDrag`, `EndFavoriteDrag`, `DropFavorite`, `Tick` — the `_favoriteDragActive` gate) | `UiRoot`'s subtree-removal safety net (`ClearSubtreeOwnership`, `UiRoot.cs:240-247`) cancels any in-flight drag whose source widget is destroyed, and `Rebuild()` tears down and recreates every cell in the list (`UiItemList.Flush` → `RemoveChild` per cell) rather than incrementally diffing. Left unguarded, the press-time removal's `SpellbookChanged` event would let the very next per-frame `Tick()` (production drives this unconditionally via `RetailUiRuntime.Tick`) destroy the cell driving the gesture and silently cancel the reorder before the user could complete the drop — this was the reported bug. Deferring the rebuild for the gesture's duration is the minimal fix that does not touch the shared `UiRoot` drag machinery every other panel (toolbar/inventory/vendor/paperdoll) also depends on. | A future rewrite that makes `Rebuild()` an incremental per-cell diff (add/remove/reflow one cell) instead of flush-and-recreate-all would make this deferral unnecessary and should retire this row along with it — until then, a player watching their OWN spell bar mid-drag sees the vacated slot's icon linger and siblings snap into place only on release, rather than reflowing live as retail does — and one ring consequence of that frozen bar: when dragging rightward past the source, the Accept ring's SCREEN slot sits one cell right of where the icon finally lands (retail's live-reflowed bar makes them coincide); the ring is on the correct CELL in both — the spell lands immediately before that cell's spell, retail's exact insert-before semantic. No effect on final position, the wire pair sent, or any other panel; cross-window spellbook→favorite drops are live-count-relative and untouched (the empty-tail claim this sentence used to carry was corrected 2026-08-08 — see the Divergence column). | `gmSpellcastingUI::RecvNotice_ItemListBeginDrag` @0x004C7360 (`SpellCastSubMenu::RemoveSpellFromMenu`, immediate live-list removal at lift); `SpellCastSubMenu::AddFavorite` @0x004C7060 (`RemoveSpellFromMenu`'s return value gating the `-1`-if-lifted-before-target `m_numSpells` adjustment before `ItemList_InsertSpellShortcut`); `PlayerModule::AddSpellFavorite` @0x005D43E0 (`InsertPos`); `PlayerModule::RemoveSpellFavorite` @0x005D4910 | | AP-160 | **Filed 2026-08-07, Slice 5.3 (vendor browse lifecycle). CORRECTED AND EXTENDED 2026-08-07 at the Slice 5.3 review corrections (fixes 4/5).** **Correction (fix 4):** this row's own Retail-oracle citation originally grouped `WorldObject_Use.cs:50,57` under the SAME citation as `Vendor.CheckClose`/`GetCylinderDistance`, which read as if the `wo.UseRadius ?? 0.6f` fallback lived inside the close watcher. It does not: `WorldObject_Use.cs:50,57` is `WorldObject.IsWithinUseRadiusOf`, the APPROACH check ("how close you need to be to open the shop") — a wholly different method from `Vendor.CheckClose`, which reads `UseRadius` directly with no fallback of its own (`UseRadius` is `float?`; a nullable comparison against a null right operand is always `false`, so `CheckClose` never closes at all on an unauthored radius). `EnforceRange`'s own code comment carried the same mis-attribution and, worse, actually APPLIED that mis-borrowed 0.6f as its fallback; it now passes the raw authored `UseRadius` with no fallback of any kind (0 when absent/unauthored, matching retail's own memset-zero `PublicWeenieDesc::_useRadius` default — a plain `float` field, `acclient.h:37181`, no sentinel). Retail's own behavior for a radius-0 handler is exactly this: close on the very first nonzero-distance check. **Extension (fix 5):** the watcher reads the SERVER-ECHOED ACCEPTED position snapshot (`RuntimeEntityRecord.Snapshot.Position`), sampled once per advanced frame at the post-network-command-phase, not retail's continuous live-pose push (retail's own client simulates and renders every entity's pose every frame; `CPlayerSystem`'s range handler reads that live pose, never a periodically-echoed one). Between accepted-position updates the watcher's distance measurement is therefore up to one update-interval stale. The one BLIND WINDOW this staleness could open into a wrong in/out-of-range verdict — an in-session portal/teleport, where the player's and vendor's position snapshots can briefly sit in DIFFERENT landblock coordinate frames mid-transit — is closed unconditionally by this same review's fix 1b (`RuntimeWorldTransitState.HasPendingTeleportStart`/`IsTeleportActive` short-circuit the whole distance computation before it runs, closing the session instead of measuring across the transit), so the staleness itself never reaches that particular failure mode; it remains recorded here as a standing precision gap for the window fix 1b does NOT cover (ordinary out-of-transit movement between the same-generation position updates a slow network tick can leave briefly stale). **Original text:** The client-local vendor-panel distance watcher closes on PLAIN 3D center-to-center distance instead of retail/ACE's CYLINDER-GAP distance (both objects' own collision radius and height subtracted from the center distance before comparing to `UseRadius`). Retail: `gmVendorUI::OpenVendor` registers `CPlayerSystem::RegisterObjectRangeHandler` keyed to the vendor's own `PublicWeenieDesc._useRadius`; ACE's server-side belt-and-suspenders `Vendor.CheckClose` closes on `GetCylinderDistance(lastPlayer) > UseRadius`, i.e. `Position::cylinder_distance`/`Physics.Common.Position.CylinderDistance` with each side's real `GetRadius()`/`GetHeight()`. **NARROWED 2026-08-08 (vendor-verify gate): the watcher now measures retail's cylinder-gap via the ResolveObjectTableHost radii — the plain-center shortcut was self-closing sessions inside the walk-to-use acceptance band (opened at 4.29 m center vs authored radius 3, closed same frame). Residuals: heights pass 0, unresolvable hosts degrade to center distance (close-early only).** | `src/AcDream.Runtime/Gameplay/RuntimeVendorRangeQuery.cs` (`EnforceRange`) | `AcDream.Runtime` does not resolve a live per-entity collision radius/height for an arbitrary NPC outside the App-layer's Setup-cylinder resolver (`WorldSelectionQuery`'s `_setupCylinder`, App-only — out of Runtime's reach per the Core-structure rules, and `PhysicsBody`/`RuntimeEntityRecord` carry no radius/height field). Plain center distance is a well-defined, non-degenerate substitute (using `ObjectRangeMath.ObjectsInRange`'s existing `useRadii: false` branch rather than inventing a new metric) for a CLIENT-LOCAL UI convenience that never touches the wire or any authoritative state — closing the panel is not gated by, nor gates, anything server-visible. Reading the accepted-position snapshot rather than a continuously-integrated live pose is the same "Runtime has no live render-side pose, only the last accepted wire snapshot" constraint every other Runtime-side distance query in this codebase already accepts. | The panel can close up to (player radius + vendor radius) sooner than exact retail — typically well under a meter for a two-legged NPC — so a player standing exactly at the boundary of a large-radius vendor's `UseRadius` may see the panel close slightly earlier than retail would. No effect on any transaction, wire message, or authoritative state (Slice 6's buy/sell owns those). Retiring the cylinder-gap half requires a Runtime-owned per-entity collision radius/height source, which does not exist today; retiring the staleness half requires a continuously-updated live-pose source Runtime does not keep either. | `CPlayerSystem::RegisterObjectRangeHandler` pc:203677/0x004C4C34; `gmVendorUI::OnObjectRangeExit` pc:199486/0x004C02F0; ACE `Vendor.CheckClose`/`GetCylinderDistance` (`references/ACE/Source/ACE.Server/WorldObjects/Vendor.cs:322-367`) — a SEPARATE method, `WorldObject.IsWithinUseRadiusOf` (`WorldObject_Use.cs:44-52`), owns the unrelated `?? 0.6f` approach-check fallback; `acclient.h:37181` (`float _useRadius`, plain memset-zero field, no sentinel); `docs/research/2026-08-08-slice5-vendor-browse-research.md` §A.3/§B.1/§B.2 | diff --git a/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs b/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs index aebeaa4d..4e7fc03a 100644 --- a/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs +++ b/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs @@ -873,6 +873,15 @@ internal sealed class RetailInteractionRetainedUiCompositionFactory RetailUiRuntime runtime = lease.Mount( () => RetailUiRuntime.CreateUninitialized(bindings)); checkpoint(InteractionRetainedUiCompositionPoint.UiRuntimeMounted); + // OP4 re-review R2: open option-bearing panels converge on every + // PlayerDescription seed (login + reconnect), closing the + // stale-rows/stale-baseline window a retained panel left open + // across the session boundary would otherwise hold. + d.Settings.ServerOptionsSeeded = () => + { + runtime.OptionsPanelController?.OnServerOptionsSeeded(); + runtime.CombatUiController?.OnServerOptionsSeeded(); + }; inventoryContainer = late.InventoryContainer.Bind(runtime); checkpoint(InteractionRetainedUiCompositionPoint.InventoryContainerBound); diff --git a/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs b/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs index af545f03..ff910c4e 100644 --- a/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs +++ b/src/AcDream.App/Net/LiveSessionRuntimeFactory.cs @@ -338,6 +338,10 @@ internal sealed class LiveSessionRuntimeFactory _interaction.Settings.SyncChatFromServerOptions(options2); _interaction.Settings.SetUiLocked( _domain.Character.Options.GetOptionBit(CharacterOptionId.LockUI)); + // OP4 re-review R2: open option-bearing panels re-read live + // bits at every seed (login + reconnect) — see + // RuntimeSettingsController.ServerOptionsSeeded. + _interaction.Settings.NotifyServerOptionsSeeded(); }); } diff --git a/src/AcDream.App/Settings/RuntimeSettingsController.cs b/src/AcDream.App/Settings/RuntimeSettingsController.cs index d1c5dd0d..9dee6f13 100644 --- a/src/AcDream.App/Settings/RuntimeSettingsController.cs +++ b/src/AcDream.App/Settings/RuntimeSettingsController.cs @@ -633,6 +633,21 @@ internal sealed class RuntimeSettingsController : } } + /// + /// OP4 re-review R2 (2026-08-11): assigned by the retained-UI + /// composition; fired (via ) from + /// the same PlayerDescription seed hook that drives + /// /SetUiLocked, so OPEN + /// option-bearing panels (the Options panel's active page, the Combat + /// panel's three LEDs) re-read live bits at every seed — login AND + /// reconnect — instead of holding stale rows and a stale undo baseline + /// until their next show. + /// + public Action? ServerOptionsSeeded { get; set; } + + /// + public void NotifyServerOptionsSeeded() => ServerOptionsSeeded?.Invoke(); + private void SaveCharacter(CharacterSettings character) { try diff --git a/src/AcDream.App/UI/Layout/CombatUiController.cs b/src/AcDream.App/UI/Layout/CombatUiController.cs index d04c5579..b8b45e9b 100644 --- a/src/AcDream.App/UI/Layout/CombatUiController.cs +++ b/src/AcDream.App/UI/Layout/CombatUiController.cs @@ -200,6 +200,12 @@ public sealed class CombatUiController : IRetainedPanelController private void OnAttackStateChanged() => SyncControls(); + /// OP4 re-review R2: a fresh PlayerDescription seed landed — + /// the three option LEDs re-read the live bits so an OPEN combat panel + /// converges with the Character tab instead of waiting for the next + /// show/mode/attack-state change. + public void OnServerOptionsSeeded() => SyncControls(); + private void SyncControls() { _powerControl.SetScalarPosition(_attacks.DesiredPower); diff --git a/src/AcDream.App/UI/Layout/OptionPageModel.cs b/src/AcDream.App/UI/Layout/OptionPageModel.cs index 07b75858..ad05004e 100644 --- a/src/AcDream.App/UI/Layout/OptionPageModel.cs +++ b/src/AcDream.App/UI/Layout/OptionPageModel.cs @@ -494,6 +494,25 @@ public sealed class OptionPage /// (re)opening): applies + commits, same as . public void OnShown() => Apply(); + /// + /// OP4 re-review R2 (2026-08-11): a fresh PlayerDescription seed + /// replaced the live option words while this page may be VISIBLE — + /// re-read every row's (current, saved) from the live source WITHOUT + /// 's flush (the seed just cleared the dirty module; + /// there is nothing to flush, and an publication + /// here would be spurious). Retail cannot reach this state — its panels + /// are closed during login/reconnect — so this adaptation exists only + /// because acdream's retained panels survive the session boundary; the + /// stale (current, saved) it clears would otherwise let Reset restore + /// pre-reconnect values over the new character's server truth. + /// + public void ReloadFromLive() + { + foreach (IOptionRow row in _rows) + row.SaveCurrentValue(); + OnOptionChanged?.Invoke(); + } + /// PlayerOptionPage::OnVisibilityChanged(false) — the page /// became hidden (a tab switch away, or the window closing): reverts /// uncommitted edits, same as . diff --git a/src/AcDream.App/UI/Layout/OptionsPanelController.cs b/src/AcDream.App/UI/Layout/OptionsPanelController.cs index dec99cca..7b4b22bd 100644 --- a/src/AcDream.App/UI/Layout/OptionsPanelController.cs +++ b/src/AcDream.App/UI/Layout/OptionsPanelController.cs @@ -310,6 +310,16 @@ public sealed class OptionsPanelController : IRetainedPanelController page.OnShown(); } + /// OP4 re-review R2: a fresh PlayerDescription seed landed — + /// re-read the ACTIVE page's rows from live state (flush-free; see + /// ). Hidden pages re-read on + /// their next as always. + public void OnServerOptionsSeeded() + { + if (_pages.TryGetValue(_tabPanel.ActivePageElementId, out OptionPage? page)) + page.ReloadFromLive(); + } + public void Dispose() { if (_disposed) return; diff --git a/src/AcDream.Core/Chat/ChatLog.cs b/src/AcDream.Core/Chat/ChatLog.cs index ab779fbf..397377d9 100644 --- a/src/AcDream.Core/Chat/ChatLog.cs +++ b/src/AcDream.Core/Chat/ChatLog.cs @@ -435,14 +435,14 @@ public sealed class ChatLog private void Append(ChatEntry entry) { - if (DisplayTimestampsSource?.Invoke() == true) - { - entry = entry with - { - Text = FormatTimestampPrefix() + entry.Text, - }; - } - + // OP4 re-review R1 (2026-08-11): the timestamp prefix does NOT touch + // entry.Text here. Retail composes the display line FIRST and carries + // the timestamp as a SEPARATE leading string at display time + // (AddTextToScroll @0x00563C50 receives already-composed lines; + // fprintf("%ls%ls\n", ts, text) @0x00563e5b) — prefixing the raw body + // put the stamp INSIDE the quotes of composed kinds + // ('Alice says, "13:05:09 hi"'). ChatVM's display composition applies + // FormatTimestampPrefix(entry.Received) around FormatEntry instead. _buffer.Enqueue(entry); while (_buffer.Count > _maxEntries) _buffer.TryDequeue(out _); @@ -465,8 +465,9 @@ public sealed class ChatLog /// GenericQualitiesData::InqString(m_pPlayerOptionsData, 1, ...) /// — see register row AP-197. /// - private static string FormatTimestampPrefix() => - DateTime.Now.ToString(@"H\:mm\:ss ", CultureInfo.InvariantCulture); + public static string FormatTimestampPrefix(DateTime receivedUtc) => + receivedUtc.ToLocalTime().ToString( + @"H\:mm\:ss ", CultureInfo.InvariantCulture); public void Clear() { diff --git a/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs b/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs index 586847f2..02a71e50 100644 --- a/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs +++ b/src/AcDream.UI.Abstractions/Panels/Chat/ChatVM.cs @@ -236,10 +236,16 @@ public sealed class ChatVM : IDisposable int count = snap.Length - start; if (count <= 0) return Array.Empty(); + // OP4 re-review R1: read the option once per snapshot so every line + // in one frame renders consistently. + bool timestamps = _log.DisplayTimestampsSource?.Invoke() == true; var lines = new string[count]; for (int i = 0; i < count; i++) { - lines[i] = FormatEntry(snap[start + i]); + var entry = snap[start + i]; + lines[i] = timestamps + ? ChatLog.FormatTimestampPrefix(entry.Received) + FormatEntry(entry) + : FormatEntry(entry); } return lines; } @@ -338,12 +344,20 @@ public sealed class ChatVM : IDisposable int count = snap.Length - start; if (count <= 0) return Array.Empty(); + // OP4 re-review R1: retail prepends the timestamp to the COMPOSED + // display line (a separate leading string — fprintf("%ls%ls", ts, + // text) @0x00563e5b), never to the message body, so tells/says render + // '13:05:09 Alice says, "hi"' and not 'Alice says, "13:05:09 hi"'. + bool timestamps = _log.DisplayTimestampsSource?.Invoke() == true; var lines = new FormattedLine[count]; for (int i = 0; i < count; i++) { var entry = snap[start + i]; + string text = FormatEntry(entry); + if (timestamps) + text = ChatLog.FormatTimestampPrefix(entry.Received) + text; lines[i] = new FormattedLine( - Text: FormatEntry(entry), + Text: text, Kind: entry.Kind, CombatKind: entry.CombatKind, LogTextType: entry.LogTextType); diff --git a/tests/AcDream.App.Tests/UI/Layout/OptionPageModelTests.cs b/tests/AcDream.App.Tests/UI/Layout/OptionPageModelTests.cs index af96c065..b9ead893 100644 --- a/tests/AcDream.App.Tests/UI/Layout/OptionPageModelTests.cs +++ b/tests/AcDream.App.Tests/UI/Layout/OptionPageModelTests.cs @@ -372,4 +372,63 @@ public sealed class OptionPageModelTests Assert.Equal(1, notifyCount); } + + // ── OP4 re-review R3: SaveCurrentValue's live re-read pushes the WIDGET + // too (the refresh delegate), so a re-seed converges both the row's + // model state AND the visible checkbox. ────────────────────────────── + + [Fact] + public void SaveCurrentValue_ReReadsLiveSource_AndPushesTheWidgetRefresh() + { + bool live = false; + bool widgetChecked = true; // deliberately out of sync with live + var row = new BoolOptionRow( + initial: true, + defaultValue: false, + read: () => live, + refresh: value => widgetChecked = value); + + live = false; + row.SaveCurrentValue(); + + // Model AND widget both converge on the live source. + Assert.False(row.Current); + Assert.False(row.Changed); + Assert.False(widgetChecked); + + live = true; + row.SaveCurrentValue(); + Assert.True(row.Current); + Assert.True(widgetChecked); + } + + // ── OP4 re-review R2: ReloadFromLive = per-row live re-read + gating + // re-eval, WITHOUT Apply's AfterApply flush (a seed just cleared the + // dirty module — a flush publication here would be spurious). ──────── + + [Fact] + public void ReloadFromLive_ReReadsRows_WithoutFiringAfterApply() + { + bool live = false; + bool widgetChecked = false; + int flushCount = 0; + int gatingCount = 0; + var page = new OptionPage { AfterApply = () => flushCount++ }; + var row = new BoolOptionRow( + initial: false, + defaultValue: false, + read: () => live, + refresh: value => widgetChecked = value); + page.Register(row); + page.OnOptionChanged = () => gatingCount++; + + live = true; + page.ReloadFromLive(); + + Assert.True(row.Current); + Assert.True(widgetChecked); + Assert.False(row.Changed); // (current, saved) both re-read — no phantom dirt + Assert.Equal(0, flushCount); // NO AfterApply publication on a seed + Assert.Equal(1, gatingCount); // Apply/Reset ghosting re-evaluated + } } diff --git a/tests/AcDream.Core.Tests/Chat/ChatLogTests.cs b/tests/AcDream.Core.Tests/Chat/ChatLogTests.cs index eef5a26d..57e4970b 100644 --- a/tests/AcDream.Core.Tests/Chat/ChatLogTests.cs +++ b/tests/AcDream.Core.Tests/Chat/ChatLogTests.cs @@ -339,78 +339,39 @@ public sealed class ChatLogTests Assert.Equal(0x06u, log.Snapshot()[0].LogTextType); } - // ── SF-1/S1 (OP4 review-fix round, 2026-08-11): DisplayTimestampsSource - // prefixes EVERY producer through the shared Append seam, not just - // RuntimeCommunicationState.AddText's own subset of callers. ──────── - - [Fact] - public void DisplayTimestampsSource_Unbound_NoPrefix() - { - var log = new ChatLog(); - log.OnLocalSpeech("Alice", "hi", 0xAAu, isRanged: false, logTextType: 0x02u); - Assert.Equal("hi", log.Snapshot()[0].Text); - } + // ── OP4 re-review R1 (2026-08-11): the timestamp NEVER touches the stored + // body — retail composes the display line first and prepends the stamp + // as a separate leading string at display time (fprintf("%ls%ls", ts, + // text) @0x00563e5b). ChatVM's display composition owns the prefix; + // these tests pin that the LOG stays clean and the format is invariant. [Theory] [InlineData(false)] [InlineData(true)] - public void DisplayTimestampsSource_GatesThePrefix(bool timestampsOn) + public void StoredEntryText_NeverCarriesTheTimestampPrefix(bool timestampsOn) { var log = new ChatLog { DisplayTimestampsSource = () => timestampsOn }; log.OnLocalSpeech("Alice", "hi", 0xAAu, isRanged: false, logTextType: 0x02u); - string text = log.Snapshot()[0].Text; - if (timestampsOn) - Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} hi$", text); - else - Assert.Equal("hi", text); - } - - [Theory] - // Every public ingestion method — proving the prefix applies at the - // ONE shared Append seam, not per-caller. - [InlineData("OnEmote")] - [InlineData("OnSoulEmote")] - [InlineData("OnChannelBroadcast")] - [InlineData("OnTellReceived")] - [InlineData("OnSystemMessage")] - [InlineData("OnPopup")] - [InlineData("OnCombatLine")] - [InlineData("OnSelfSent")] - [InlineData("OnPlayerKilled")] - public void DisplayTimestampsSource_AppliesToEveryProducer(string method) - { - var log = new ChatLog { DisplayTimestampsSource = () => true }; - - switch (method) - { - case "OnEmote": log.OnEmote("Caith", "waves", 0xCAFEu); break; - case "OnSoulEmote": log.OnSoulEmote("Bob", "dances", 0xBEEFu); break; - case "OnChannelBroadcast": log.OnChannelBroadcast(42u, "Alice", "motd"); break; - case "OnTellReceived": log.OnTellReceived("Alice", "psst", 0xAAu, logTextType: 0x03u); break; - case "OnSystemMessage": log.OnSystemMessage("fizzled", chatType: 5); break; - case "OnPopup": log.OnPopup("modal"); break; - case "OnCombatLine": log.OnCombatLine("hit", logTextType: 0x06u); break; - case "OnSelfSent": log.OnSelfSent(ChatKind.Tell, "hey", logTextType: 0x04u, targetOrChannel: "Alice"); break; - case "OnPlayerKilled": log.OnPlayerKilled("died", 0x1u, 0x2u); break; - } - - string text = log.Snapshot()[0].Text; - Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} .+$", text); + // The stored body is ALWAYS the raw message — the option gates the + // display composition (ChatVM), never the log content. + Assert.Equal("hi", log.Snapshot()[0].Text); } [Fact] - public void DisplayTimestampsSource_UsesLiteralColons_RegardlessOfCurrentCulture() + public void FormatTimestampPrefix_UsesLiteralColons_RegardlessOfCurrentCulture() { CultureInfo original = Thread.CurrentThread.CurrentCulture; try { Thread.CurrentThread.CurrentCulture = CultureInfo.GetCultureInfo("fi-FI"); - var log = new ChatLog { DisplayTimestampsSource = () => true }; - log.OnSystemMessage("fizzled", chatType: 0); + string prefix = ChatLog.FormatTimestampPrefix( + new DateTime(2026, 8, 11, 13, 5, 9, DateTimeKind.Utc)); - Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} fizzled$", log.Snapshot()[0].Text); + // fi-FI's time separator is '.', so a culture-dependent format + // would emit "16.05.09 " here; retail's strftime is a literal ':'. + Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} $", prefix); } finally { diff --git a/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCommunicationStateTests.cs b/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCommunicationStateTests.cs index b771bf3f..1fbfe5a9 100644 --- a/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCommunicationStateTests.cs +++ b/tests/AcDream.Runtime.Tests/Gameplay/RuntimeCommunicationStateTests.cs @@ -267,65 +267,36 @@ public sealed class RuntimeCommunicationStateTests } [Fact] - public void AddText_TimestampsTrue_PrefixesTranscriptLine() + public void AddText_TimestampsTrue_StoredBodyStaysClean() { + // OP4 re-review R1 (2026-08-11): the prefix belongs to the DISPLAY + // composition (ChatVM prepends FormatTimestampPrefix(entry.Received) + // to the COMPOSED line — retail fprintf("%ls%ls", ts, text) + // @0x00563e5b), never to the stored body. The earlier fix round + // prefixed entry.Text here, which put the stamp INSIDE the quotes of + // composed kinds ('Alice says, "13:05:09 hi"'). using var state = new RuntimeCommunicationState { DisplayTimestampsSource = () => true }; state.AddText("Your spell fizzled.", RetailLogTextType.Default); - string text = state.Chat.Snapshot()[0].Text; - Assert.EndsWith("Your spell fizzled.", text); - Assert.NotEqual("Your spell fizzled.", text); - // Retail ctor default format "%#H:%M:%S " (non-zero-padded 24h - // hour, zero-padded minute:second, trailing space before the - // text) — .NET "H\:mm\:ss " (colons ESCAPED, not the - // culture-dependent TimeSeparator placeholder) is the exact - // equivalent (SF-1/S4, OP4 review-fix round, 2026-08-11). - Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} Your spell fizzled\.$", text); + Assert.Equal("Your spell fizzled.", state.Chat.Snapshot()[0].Text); } [Fact] - public void AddText_TimestampsTrue_UsesLiteralColons_RegardlessOfCurrentCulture() + public void DisplayTimestampsSource_ForwardsToChat_SoTheDisplaySeamSeesOneSource() { - // SF-1/S4 (OP4 review-fix round, 2026-08-11): an unescaped "H:mm:ss" - // format string renders ':' as CurrentCulture.DateTimeFormat. - // TimeSeparator, which is NOT ':' on cultures like fi-FI ("."). The - // fix escapes the colons and forces InvariantCulture — prove the - // output stays colon-separated even under a culture that would - // otherwise substitute a different separator. - CultureInfo original = Thread.CurrentThread.CurrentCulture; - try - { - Thread.CurrentThread.CurrentCulture = CultureInfo.GetCultureInfo("fi-FI"); - using var state = new RuntimeCommunicationState { DisplayTimestampsSource = () => true }; - - state.AddText("Your spell fizzled.", RetailLogTextType.Default); - - string text = state.Chat.Snapshot()[0].Text; - Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} Your spell fizzled\.$", text); - } - finally - { - Thread.CurrentThread.CurrentCulture = original; - } - } - - [Fact] - public void DisplayTimestampsSource_ForwardsToChat_TimestampingEveryProducer_NotJustAddText() - { - // S1 (OP4 review-fix round, 2026-08-11, blast lens): AddText's own - // callers (ServerMessage/WeenieError) were a strict SUBSET of - // every chat producer — heard speech, emotes, Turbine channels, - // and combat text all bypassed it by calling ChatLog's own OnXxx - // methods directly. Setting DisplayTimestampsSource on this class - // must timestamp lines that never go through AddText at all. + // R1: this class still owns the ONE forwarding seam — ChatVM reads + // ChatLog.DisplayTimestampsSource at display composition, so setting + // it HERE must reach the log's property (every producer's entries + // then render prefixed, ChatVMTests pins the composed-line shape). using var state = new RuntimeCommunicationState { DisplayTimestampsSource = () => true }; - state.Chat.OnLocalSpeech("Alice", "hi", 0xAAu, isRanged: false, logTextType: 0x02u); + Assert.NotNull(state.Chat.DisplayTimestampsSource); + Assert.True(state.Chat.DisplayTimestampsSource!()); - string text = state.Chat.Snapshot()[0].Text; - Assert.EndsWith("hi", text); - Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} hi$", text); + // And the stored body of a non-AddText producer stays clean too. + state.Chat.OnLocalSpeech("Alice", "hi", 0xAAu, isRanged: false, logTextType: 0x02u); + Assert.Equal("hi", state.Chat.Snapshot()[0].Text); } [Fact] diff --git a/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs b/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs index 71515f29..15cb0cc5 100644 --- a/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs +++ b/tests/AcDream.UI.Abstractions.Tests/ChatVMTests.cs @@ -161,4 +161,37 @@ public sealed class ChatVMTests var entry = Assert.Single(log.Snapshot()); Assert.Equal(0x00u, entry.LogTextType); } + + // ── OP4 re-review R1 (2026-08-11): the timestamp prefixes the COMPOSED + // display line, never the body — '13:05:09 Alice says, "hi"', not + // 'Alice says, "13:05:09 hi"' (retail fprintf("%ls%ls", ts, text) + // @0x00563e5b). ───────────────────────────────────────────────────── + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void DisplayTimestamps_PrefixTheComposedLine_NotTheBody(bool timestampsOn) + { + var log = new ChatLog { DisplayTimestampsSource = () => timestampsOn }; + log.OnLocalSpeech("Alice", "hi", 0xAAu, isRanged: false, logTextType: 0x02u); + var vm = new ChatVM(log); + + string plain = Assert.Single(vm.RecentLines()); + var detailed = Assert.Single(vm.RecentLinesDetailed()); + + if (timestampsOn) + { + // The stamp leads the whole composed line, OUTSIDE the quotes. + Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} Alice says, ""hi""$", plain); + Assert.Matches(@"^\d{1,2}:\d{2}:\d{2} Alice says, ""hi""$", detailed.Text); + } + else + { + Assert.Equal("Alice says, \"hi\"", plain); + Assert.Equal("Alice says, \"hi\"", detailed.Text); + } + + // The stored body never carries the stamp in either state. + Assert.Equal("hi", log.Snapshot()[0].Text); + } }