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); + } }