diff --git a/docs/architecture/retail-divergence-register.md b/docs/architecture/retail-divergence-register.md index 6c497ed9..a510532f 100644 --- a/docs/architecture/retail-divergence-register.md +++ b/docs/architecture/retail-divergence-register.md @@ -288,9 +288,9 @@ AP-94..AP-112 for the confirmed retail-UI completion gaps. | AP-108 | Paperdoll/AutoWield still omit the remaining missile/held restrictions and corrupt-mask branch of full `AutoWieldIsLegal`, dual-wield/off-hand rules, double-click examine/drag from the doll, body-part selection lighting, and retail's synchronous `" - cannot unwield the %s"` failure suffix (the current send seam reports rejection asynchronously). **AutoWear legality retired from this row 2026-07-23:** inventory activation and paperdoll drops now apply the retail clothing-priority/location blocker lookup and exact `"You must remove your %s to wear that"` system notice. **Primary replacement retired 2026-07-14; Aetheria retired 2026-07-13.** | `src/AcDream.App/UI/Layout/PaperdollController.cs`; `src/AcDream.App/UI/AutoWieldController.cs` | Basic equip slots, Aetheria, live doll, AutoWear conflict reporting, and primary weapon/incompatible shield/mismatched ammo blocker sequencing work in peace and war | Remaining illegal/off-hand cases, asynchronous dequip rejection wording, doll examine/drag, and selection lighting still differ functionally | `CPlayerSystem::AutoWieldIsLegal @ 0x0055ED60`; `CPlayerSystem::AutoWearIsLegal @ 0x0055EF40`; `CPlayerSystem::AutoWield @ 0x00560A60`; `gmPaperDollUI @ 0x004A3590..0x004A5F90` | | AP-109 | Character Titles page is inert and live displayed-title/luminance state is absent | `src/AcDream.App/UI/Layout/CharacterStatController.cs`; `CharacterSheetProvider.cs` | Attributes/skills core output is user-accepted | Titles cannot be selected/displayed and level-200 luminance fields are missing | `gmCharacterTitleUI @ 0x0049A610`; `gmStatManagementUI::UpdateExperience @ 0x004F0A70` | | AP-110 | **NARROWED 2026-08-09 (Slice 5.4, vendor browse panel) — "vendor" retired from the absent-panels list; see AP-161 for the precise successor (Buy/Sell transaction UI, Slice 6).** Remaining retained gameplay panels and world HUD are absent: advanced-combat powerbar, residual social/floating chat, quests/map/options/smartbox, trade/salvage/tinkering, mini-game gameplay, Link Status NAK/retransmission packet-loss averaging, and D.6 nameplates/floaters. Examination has its independent authored floaty layout, inscription transaction, retail creature stat/rating/animated-preview presentation, default selection-follow, authored local spell subview with appropriate-formula component state, and the full EoR item-report dispatch: appraisal-only unknowns; exact equipment-set/rating/tinkering/weapon/armor/caster/requirement/XP/healer/rare prose and intentional blank section rows; ordinary/enchantment DAT spell descriptions; live material-decorated appropriate titles plus DAT material and creature names; expiry, decorated material/gem descriptions; and portal/PK restrictions with authored item colors. It still lacks item-object preview, player-dependent effective shield projection, live cooldown-remaining projection, localized augmentation-cost `StringInfo`, exhaustive character detail regions, and exact creature appraisal FontInfo-list selection. | `src/AcDream.App/UI/RetailUiRuntime.cs`; `src/AcDream.App/UI/Layout/AppraisalUiController.cs`; `src/AcDream.App/UI/Layout/ItemAppraisalTextFormatter.cs`; `src/AcDream.App/UI/Layout/RetailAppraisalNameResolver.cs`; `src/AcDream.App/UI/Layout/CreatureAppraisalRows.cs`; `src/AcDream.App/Rendering/CreatureAppraisalPresentation.cs`; `src/AcDream.Core.Net/LinkStatusSnapshot.cs`; D.5/D.6 roadmap | Basic combat, M3 magic/Link/Vitae surfaces, the Slice 5.4 vendor "Items" browse panel (category-filtered stock list, retail's quantity-correct pricing — `ItemHolder::GetObjectSplitSize`'s split-exempt mask, not a flat per-unit price), and the core examination request/presentation/inscription/creature-preview/item-report loop cover the active loops; the residual examination mechanisms require live player/enchantment/localization state or object-preview ownership rather than fabricated content | Item assessments omit only the listed live/localized/preview projections; enchanted/incomplete creature appraisal rows use the normal authored font until the exact FontInfo list is bound; other absent panels remain unavailable; real packet loss is displayed as 0.00% instead of retail's moving average | `BasicCreatureExamineUI::Init @ 0x004AB9C0`; `CreatureExamineUI::SetAppraiseInfo @ 0x004B3FF0`; `gmExaminationUI::RecvNotice_SelectionChanged @ 0x004AB3D0`; `gmExaminationUI::ExamineSpell @ 0x004B6900`; `SpellExamineUI::ExamineSpell @ 0x004B6210`; `AttributeInfoRegion::Update @ 0x004F1D90`; `gmExaminationUI::SetAppraiseInfo @ 0x004ADAE0`; `ACCWeenieObject::GetObjectName @ 0x0058E6E0`; `ItemExamineUI::SetAppraiseInfo @ 0x004B72B0`; `ItemExamineUI::AddItemInfo @ 0x004AC050`; `ItemExamineUI::Appraisal_ShowCapacity @ 0x004B2680`; `ItemExamineUI::Appraisal_ShowSpecialProperties @ 0x004B0140`; `ItemExamineUI::Appraisal_ShowWeaponAndArmorData @ 0x004B10E0`; `ItemExamineUI::Appraisal_ShowMagicInfo @ 0x004B2E10`; `ItemExamineUI::Appraisal_ShowDescription @ 0x004B6990`; `MaterialTypeEnumMapper::MaterialTypeToString @ 0x005CD500`; `ItemExamineUI::SetInscription @ 0x004AE2F0`; `CM_Writing::Event_SetInscription @ 0x006A98B0`; `CLinkStatusAverages::GetAveragePacketLoss @ 0x00546610`; LayoutDesc catalog | -| AP-161 | **NARROWED 2026-08-09 (Slice 6.1-6.3, buy arc) — TWO of the four consciously-deferred residuals below CLOSE.** Private per-panel selection is GONE: `SelectionState` gains a `Vendor` change source (`SelectionChangeSource.Vendor`) and is now the AUTHORITY — row clicks, the F4 auto-select-first-item fallback, and right-click examine all call `SelectionState.Select`/`Clear`; `VendorUiController` is a CONSUMER (`OnSelectionTransition`) exactly like every sibling panel, matching retail's global `ACCWeenieObject::selectedID`. The examine gap (F7c) is GONE too: `VendorShopItemMaterializer` (`src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs`, Slice 6.1) registers every `ApproachVendor` shop item into `ClientObjectTable` (guid, `ContainerId = vendorGuid`, merge-upserted via the ordinary `Ingest` path, retired on session Close/Reset/vendor-replace via the SAME `VendorState.Changed` subscription) so `AppraisalUiController.Apply`'s lookup now succeeds; `VendorUiController.ExamineItem` wires `UiItemList.ExamineItemRequested` to `ItemInteractionController.ExamineSelectedOrEnterMode`, mirroring `ExternalContainerController`. **Double-click-to-buy was investigated (research doc `docs/research/2026-08-08-slice6-vendor-transactions-research.md` §B.2) and confirmed ABSENT from retail** — no `gmVendorUI::CheckForDoubleClick`/`VendorItemsUI::CheckForDoubleClick` symbol exists anywhere in the 18,366-function named table, unlike sibling panels (`gmContractsUI::CheckForDoubleClick`, `gmPageListUI::CheckForDoubleClick`) that DO have one; acdream intentionally does NOT add a double-click shortcut — a user request for it as a deliberate acdream-only UX addition would need its own AP row, per CLAUDE.md's no-invented-mechanisms discipline. The remaining two residuals (dropdown arrow-cap glyph, alt-currency `m_last_sale` simplification) are UNCHANGED — see below. New approximations this pass introduced are filed separately as AP-162 (no client-side Buy pre-check) and AP-163 (shop-item guid-collision policy). **Original REWRITTEN text follows, retained for the two still-open residuals:** `VendorUiController` mounts LayoutDesc `0x21000012`/root `0x100000B7` and fully wires only the authored "Items" tab (`0x100000B9` — `VendorItemsUI`: category-filtered browse list with retail's quantity-correct pricing, `ItemHolder::GetObjectSplitSize`'s `0xDC41CB0` split-exempt mask ported locally rather than a flat per-unit price). The other two authored tabs render and switch pages (so the layout looks complete) but are otherwise INERT: "Buying" (`0x100000BA`, `VendorBuyUI` — staged-purchase review/confirm, buttons `0x100000C9`/`CA`/`CB`/`CC`) and "Selling" (`0x100000BB`, `VendorSellUI` — staged-sale review/confirm, buttons `0x100000D2`/`D3`/`D4`/`D5`) have no data binding at all. The "Items" page's own `Buy`/`Add to List` buttons (`0x100000C2`/`C3`) correctly enable/disable with selection (`UiButton.Enabled`, retail `SetState(1)`/`SetState(0xd)`); Slice 6.3 wires `Buy` to a real immediate single-item purchase (`gmVendorUI::BuySingleItem`, `pc:201661` — `VendorRequests.BuildBuy`/`WorldSession.SendBuy`, opcode `0x005F`), while `Add to List` (staging) remains unimplemented. The Buy opcode exists on the wire now; Sell (`0x0060`) does not. `VendorProfile::InqAcceptability` (sell-eligibility filtering) is unread — moot without a sell UI. Two divergences remain of the four the F1-F8 fix pass originally recorded — the other two (private per-panel selection, unwired shop-item examine) CLOSED at Slice 6.1/6.2, see the NARROWED note above: (1) the closed-dropdown button face reuses the row template's own two sprites (`0x060012B3` normal/`0x060012B4` open) through `UiMenu`'s existing single-texture 3-slice `DrawButtonFace` instead of retail's authored two-piece label+arrow-cap assembly (label `0x1000034D` + a separate 17x19 arrow cap `0x1000034E` with its own `0x060012B1`/`0x060012B2` states) — a cosmetic gap only; the popup panel and its rows render with the exact authored geometry and sprites; (2) the alt-currency "you have" holding reads `VendorShopProfile.AlternateCurrencyAmount` directly instead of tracking retail's `gmVendorUI.m_last_sale` purchase debit — moot until a sell path exists to actually debit it, since `m_last_sale` only changes on a completed SALE (retail's own `m_last_sale == 0` case, `pc:204091`/`OpenVendor`'s `this->m_last_sale = 0` reset at `pc:203790`/`203800`); Slice 6.3's buy path does not touch `m_last_sale` either (retail's own buy flow never writes it), so this residual is unaffected by the buy arc landing. The "Buying"/"Selling" staging tabs (`VendorBuyUI`/`VendorSellUI`) and the full Sell wire remain unwired — unchanged Slice 6b/6c territory per contract decision 6, not a residual of THIS row. | `src/AcDream.App/UI/Layout/VendorUiController.cs`; `src/AcDream.Core/Items/VendorState.cs`; `src/AcDream.Core.Net/GameEventWiring.cs`; `src/AcDream.App/UI/RetailUiRuntime.cs` | Slice 6 (`docs/plans/2026-07-23-world-interaction-completion.md`) owns the authoritative buy/sell transaction command, quantity/stack-split selection, drag-to-sell consumption, and `InqAcceptability`-gated sell UI — Slice 5.4's charter was browse only. Buy (6.3), the global `ACCWeenieObject::selectedID` coupling (6.2), and shop-item `ClientObjectTable` registration (6.1) are now DONE, landing exactly the seam this row's original filing fenced off; drag-to-sell consumption and `InqAcceptability`-gated sell UI remain Slice 6b/6c territory. | A player can browse, select, examine, and BUY (Slice 6.3) — the only remaining unbuilt transaction is Sell. Clicking "Buying"/"Selling" still shows an empty page with no error or explanation, matching "present but does nothing" rather than a disabled/hidden affordance. The dropdown's closed-state button face is missing its separate arrow-cap glyph — a minor visual gap, not a functional one; the open popup itself is pixel-faithful to the authored template. | `gmVendorUI::OpenVendor` pc:203650 (`m_itemsUI`/`m_buyUI`/`m_sellUI` construction, `PostInit` pc:199906, `m_last_sale` reset pc:203790/203800); `VendorBuyUI::VendorBuyUI` pc:199717; `VendorSellUI::VendorSellUI` pc:199753; `VendorProfile::InqAcceptability` pc:484768-484797; `UIElement_Menu::MakePopup` pc:120705-120764, `::Initialize` pc:120789-120828; `VendorItemsUI::UpdateItemsUI` pc:202539-202820; `VendorItemsUI::UpdateItemsList` pc:201029-201190; `ItemHolder::GetObjectSplitSize` pc:401465-401477; `gmToolbarUI::HandleSelectionChanged` pc:198740-198790 (mask `0xDC41CB0` at pc:198784); `ACCWeenieObject::GetObjectName` pc:409056-409132; `docs/research/2026-08-08-slice5-vendor-browse-research.md` §B.4, §D | -| AP-162 | **Filed 2026-08-09, Slice 6.3 (buy wire + button).** Retail's `BuySingleItem` (`pc:201661`) performs TWO client-side pre-checks before ever sending `CM_Vendor::Event_Buy`: (a) an affordability check against `this->m_totalValue` (pyreal) or `shopVendorProfile->trade_num - m_last_sale` (alt-currency), showing a LOCAL string via `ECM_UI::SendNotice_DisplayStringInfo` and returning without sending anything on failure (`pc:201686-201717`); (b) a pack/container-capacity pre-check (`pc:201730-201746`) mirroring the server's own check. acdream's `ItemInteractionController.TryBuy` sends unconditionally once the shared use/inventory gate is free — no client-side affordability or capacity check runs before dispatch. Every refused purchase pays a full round-trip (send → server rejects → `UseDone`/`GameEventInventoryServerSaveFailed`) instead of failing instantly and silently client-side. | `src/AcDream.App/UI/ItemInteractionController.cs` (`TryBuy`) | The research doc's own open question 1 (`docs/research/2026-08-08-slice6-vendor-transactions-research.md`) recommends deferring this: the server is authoritative either way (ACE re-validates both affordability and capacity server-side — `Vendor.BuyItems_ValidateTransaction`, `Vendor.cs:431-571`), so omitting the client pre-check is a LATENCY/UX gap, not a correctness one — a refused purchase still fails cleanly, just one round-trip later than retail. | A player attempting to buy something they cannot afford or have no room for sees the failure arrive after a network round-trip instead of instantly; against a well-behaved ACE server no purchase can succeed that retail's pre-check would have blocked, so no transaction outcome differs — only its latency. Retiring this row means porting `BuySingleItem`'s two pre-check branches (`pc:201686-201746`) into `TryBuy` before dispatch. | `gmVendorUI::BuySingleItem` pc:201661/0x004C2820 (affordability pc:201686-201717, capacity pc:201730-201746); `Vendor.BuyItems_ValidateTransaction` (`references/ACE/Source/ACE.Server/WorldObjects/Vendor.cs:431-571`); `docs/research/2026-08-08-slice6-vendor-transactions-research.md` §D point 4, Open question 1 | -| AP-163 | **Filed 2026-08-09, Slice 6.1 (shop-item materialization).** `VendorShopItemMaterializer` registers each `ApproachVendor` shop item into `ClientObjectTable` keyed by its own server guid. ACE's `UniqueItemsForSale` (`Vendor.cs:34,638`) can list the EXACT guid a player last held (an item sold to this vendor keeps its original guid), so a guid collision against an existing, differently-owned `ClientObjectTable` entry is a real, if rare, possibility. No retail behavior for this exact case was traced (retail's `ClientObjMaintSystem`/`CObjectMaint` guid-keyed registration internals were not decompiled for this pass). acdream's policy is a conscious, conservative default: a guid this materializer did NOT itself add to the table on a previous cycle is treated as owned by something else and is left completely untouched — never overwritten, never later removed by this class. | `src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs` (`OnVendorTransition`'s collision guard) | Skip-not-clobber is the safe default absent a traced retail mechanism: silently reparenting a live entity's or another container's item into the vendor's `ContainerId` would corrupt real ownership state (equipment tracking, burden, radar) for a guid this code does not own, which is strictly worse than a single shop row's status-bar/appraisal projection staying blank. The vendor list itself is unaffected either way — `VendorUiController` reads display fields straight off `VendorShopItem`, never through `ClientObjectTable`. | If retail's actual behavior differs (e.g. it always overwrites, or a real `UniqueItemsForSale` collision is more common than assumed), the one colliding shop row's status-bar/appraisal projection stays stale/blank instead of showing the vendor listing — a narrow, single-row display gap, never a corrupted non-vendor object. Retiring this row requires tracing retail's `ClientObjMaintSystem` registration behavior on a guid collision, which was out of scope for this pass. | No direct retail citation traced this pass — `Vendor.cs:34,638` (`UniqueItemsForSale`, ACE) establishes the collision is POSSIBLE, not what retail does about it; `docs/research/2026-08-08-slice6-vendor-transactions-research.md` (task brief: "study how ACE guids vendor stock and state your collision policy with evidence") | +| AP-161 | **REVIEW CORRECTIONS 2026-08-09 (Opus review of `97cf8738`, findings F1-F9):** none of these are NEW divergences from retail — they are bug fixes that make this row's own claims actually true, so no new AP row is filed for them. F2 fixed the priced/named quantity freezing at a selection-time seed while the Buy button separately read the LIVE slider — both now share one `ResolveBuyQuantity` computation, so the displayed price always equals what a purchase actually charges (retail: `gmVendorUI::RecvNotice_StackSliderChanged` re-runs the SAME display update on every slider change, `pc:203262-203278`). F6 corrected an unauthored "preserve the prior selection if it survives the filter" rule to retail's actual UNCONDITIONAL reselect-to-first-item on every rebuild this controller reaches (`VendorItemsUI::UpdateItemsList`'s notify=1 path, `pc:201180-201184`, confirmed reached by a fresh open AND a same-vendor refresh via `VendorItemsUI::OpenVendor`'s unconditional `SetSelectedItem(...,1)`, `pc:201022`). F7 ported `BuySingleItem`'s stack-size-1 quantity clamp (`pc:201674-201681`) so a stale slider value left over from a previously-selected, DIFFERENT stackable item cannot leak into a non-stack purchase. F8 is recorded inline below, where it corrects this row's own stale claim about the Add-to-List button. **NARROWED 2026-08-09 (Slice 6.1-6.3, buy arc) — TWO of the four consciously-deferred residuals below CLOSE.** Private per-panel selection is GONE: `SelectionState` gains a `Vendor` change source (`SelectionChangeSource.Vendor`) and is now the AUTHORITY — row clicks, the F4 auto-select-first-item fallback, and right-click examine all call `SelectionState.Select`/`Clear`; `VendorUiController` is a CONSUMER (`OnSelectionTransition`) exactly like every sibling panel, matching retail's global `ACCWeenieObject::selectedID`. The examine gap (F7c) is GONE too: `VendorShopItemMaterializer` (`src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs`, Slice 6.1) registers every `ApproachVendor` shop item into `ClientObjectTable` (guid, `ContainerId = vendorGuid`, merge-upserted via the ordinary `Ingest` path, retired on session Close/Reset/vendor-replace via the SAME `VendorState.Changed` subscription) so `AppraisalUiController.Apply`'s lookup now succeeds; `VendorUiController.ExamineItem` wires `UiItemList.ExamineItemRequested` to `ItemInteractionController.ExamineSelectedOrEnterMode`, mirroring `ExternalContainerController`. **Double-click-to-buy was investigated (research doc `docs/research/2026-08-08-slice6-vendor-transactions-research.md` §B.2) and confirmed ABSENT from retail** — no `gmVendorUI::CheckForDoubleClick`/`VendorItemsUI::CheckForDoubleClick` symbol exists anywhere in the 18,366-function named table, unlike sibling panels (`gmContractsUI::CheckForDoubleClick`, `gmPageListUI::CheckForDoubleClick`) that DO have one; acdream intentionally does NOT add a double-click shortcut — a user request for it as a deliberate acdream-only UX addition would need its own AP row, per CLAUDE.md's no-invented-mechanisms discipline. The remaining two residuals (dropdown arrow-cap glyph, alt-currency `m_last_sale` simplification) are UNCHANGED — see below. New approximations this pass introduced are filed separately as AP-162 (no client-side Buy pre-check) and AP-163 (shop-item guid-collision policy). **Original REWRITTEN text follows, retained for the two still-open residuals:** `VendorUiController` mounts LayoutDesc `0x21000012`/root `0x100000B7` and fully wires only the authored "Items" tab (`0x100000B9` — `VendorItemsUI`: category-filtered browse list with retail's quantity-correct pricing, `ItemHolder::GetObjectSplitSize`'s `0xDC41CB0` split-exempt mask ported locally rather than a flat per-unit price). The other two authored tabs render and switch pages (so the layout looks complete) but are otherwise INERT: "Buying" (`0x100000BA`, `VendorBuyUI` — staged-purchase review/confirm, buttons `0x100000C9`/`CA`/`CB`/`CC`) and "Selling" (`0x100000BB`, `VendorSellUI` — staged-sale review/confirm, buttons `0x100000D2`/`D3`/`D4`/`D5`) have no data binding at all. The "Items" page's own `Buy` button (`0x100000C2`) correctly enables/disables with selection (`UiButton.Enabled`, retail `SetState(1)`/`SetState(0xd)`) and Slice 6.3 wires it to a real immediate single-item purchase (`gmVendorUI::BuySingleItem`, `pc:201661` — `VendorRequests.BuildBuy`/`WorldSession.SendBuy`, opcode `0x005F`). **Review correction 2026-08-09 (F8):** `Add to List` (`0x100000C3`, staging) does NOT enable/disable with selection — it is PERMANENTLY disabled (`VendorUiController.SetActionButtonsEnabled`), because it has no wired `OnClick` at all; an enabled-but-dead button is a worse affordance than a disabled one, so it stays disabled until the "Buying" tab's staging list is actually implemented. The Buy opcode exists on the wire now; Sell (`0x0060`) does not. `VendorProfile::InqAcceptability` (sell-eligibility filtering) is unread — moot without a sell UI. Two divergences remain of the four the F1-F8 fix pass originally recorded — the other two (private per-panel selection, unwired shop-item examine) CLOSED at Slice 6.1/6.2, see the NARROWED note above: (1) the closed-dropdown button face reuses the row template's own two sprites (`0x060012B3` normal/`0x060012B4` open) through `UiMenu`'s existing single-texture 3-slice `DrawButtonFace` instead of retail's authored two-piece label+arrow-cap assembly (label `0x1000034D` + a separate 17x19 arrow cap `0x1000034E` with its own `0x060012B1`/`0x060012B2` states) — a cosmetic gap only; the popup panel and its rows render with the exact authored geometry and sprites; (2) the alt-currency "you have" holding reads `VendorShopProfile.AlternateCurrencyAmount` directly instead of tracking retail's `gmVendorUI.m_last_sale` purchase debit — moot until a sell path exists to actually debit it, since `m_last_sale` only changes on a completed SALE (retail's own `m_last_sale == 0` case, `pc:204091`/`OpenVendor`'s `this->m_last_sale = 0` reset at `pc:203790`/`203800`); Slice 6.3's buy path does not touch `m_last_sale` either (retail's own buy flow never writes it), so this residual is unaffected by the buy arc landing. The "Buying"/"Selling" staging tabs (`VendorBuyUI`/`VendorSellUI`) and the full Sell wire remain unwired — unchanged Slice 6b/6c territory per contract decision 6, not a residual of THIS row. | `src/AcDream.App/UI/Layout/VendorUiController.cs`; `src/AcDream.Core/Items/VendorState.cs`; `src/AcDream.Core.Net/GameEventWiring.cs`; `src/AcDream.App/UI/RetailUiRuntime.cs` | Slice 6 (`docs/plans/2026-07-23-world-interaction-completion.md`) owns the authoritative buy/sell transaction command, quantity/stack-split selection, drag-to-sell consumption, and `InqAcceptability`-gated sell UI — Slice 5.4's charter was browse only. Buy (6.3), the global `ACCWeenieObject::selectedID` coupling (6.2), and shop-item `ClientObjectTable` registration (6.1) are now DONE, landing exactly the seam this row's original filing fenced off; drag-to-sell consumption and `InqAcceptability`-gated sell UI remain Slice 6b/6c territory. | A player can browse, select, examine, and BUY (Slice 6.3) — the only remaining unbuilt transaction is Sell. Clicking "Buying"/"Selling" still shows an empty page with no error or explanation, matching "present but does nothing" rather than a disabled/hidden affordance. The dropdown's closed-state button face is missing its separate arrow-cap glyph — a minor visual gap, not a functional one; the open popup itself is pixel-faithful to the authored template. | `gmVendorUI::OpenVendor` pc:203650 (`m_itemsUI`/`m_buyUI`/`m_sellUI` construction, `PostInit` pc:199906, `m_last_sale` reset pc:203790/203800); `VendorBuyUI::VendorBuyUI` pc:199717; `VendorSellUI::VendorSellUI` pc:199753; `VendorProfile::InqAcceptability` pc:484768-484797; `UIElement_Menu::MakePopup` pc:120705-120764, `::Initialize` pc:120789-120828; `VendorItemsUI::UpdateItemsUI` pc:202539-202820; `VendorItemsUI::UpdateItemsList` pc:201029-201190; `ItemHolder::GetObjectSplitSize` pc:401465-401477; `gmToolbarUI::HandleSelectionChanged` pc:198740-198790 (mask `0xDC41CB0` at pc:198784); `ACCWeenieObject::GetObjectName` pc:409056-409132; `docs/research/2026-08-08-slice5-vendor-browse-research.md` §B.4, §D | +| AP-162 | **Filed 2026-08-09, Slice 6.3 (buy wire + button).** Retail's `BuySingleItem` (`pc:201661`) performs TWO client-side pre-checks before ever sending `CM_Vendor::Event_Buy`: (a) an affordability check against `this->m_totalValue` (pyreal) or `shopVendorProfile->trade_num - m_last_sale` (alt-currency), showing a LOCAL string via `ECM_UI::SendNotice_DisplayStringInfo` and returning without sending anything on failure (`pc:201686-201717`); (b) a pack/container-capacity pre-check (`pc:201730-201746`) mirroring the server's own check. acdream's `ItemInteractionController.TryBuy` sends unconditionally once the shared use/inventory gate is free — no client-side affordability or capacity check runs before dispatch. Every refused purchase pays a full round-trip (send → server rejects → `UseDone`/`GameEventInventoryServerSaveFailed`) instead of failing instantly and silently client-side. **Swept 2026-08-09 (F4 review fix):** `TryBuy` now also checks whether `sendBuy` actually reached a live, in-world session before marking the reservation dispatched — an orthogonal reservation-leak bug fix (no session ever produced a stray permanent busy-lock), not an affordability/capacity check; this row's scope and residual are unchanged. | `src/AcDream.App/UI/ItemInteractionController.cs` (`TryBuy`) | The research doc's own open question 1 (`docs/research/2026-08-08-slice6-vendor-transactions-research.md`) recommends deferring this: the server is authoritative either way (ACE re-validates both affordability and capacity server-side — `Vendor.BuyItems_ValidateTransaction`, `Vendor.cs:431-571`), so omitting the client pre-check is a LATENCY/UX gap, not a correctness one — a refused purchase still fails cleanly, just one round-trip later than retail. | A player attempting to buy something they cannot afford or have no room for sees the failure arrive after a network round-trip instead of instantly; against a well-behaved ACE server no purchase can succeed that retail's pre-check would have blocked, so no transaction outcome differs — only its latency. Retiring this row means porting `BuySingleItem`'s two pre-check branches (`pc:201686-201746`) into `TryBuy` before dispatch. | `gmVendorUI::BuySingleItem` pc:201661/0x004C2820 (affordability pc:201686-201717, capacity pc:201730-201746); `Vendor.BuyItems_ValidateTransaction` (`references/ACE/Source/ACE.Server/WorldObjects/Vendor.cs:431-571`); `docs/research/2026-08-08-slice6-vendor-transactions-research.md` §D point 4, Open question 1 | +| AP-163 | **REVIEW CORRECTION 2026-08-09 (Opus review of `97cf8738`, finding F1):** this row's ownership discipline is now COMPLETE on both halves, not just the add-time collision guard described below. The retire pass (`OnVendorTransition`'s loop over guids missing from the new `ApproachVendor` snapshot) previously deleted ANY such guid unconditionally — a plain bug, not a documented divergence, since buying a UNIQUE vendor item re-containers that SAME guid into the buyer's own pack (`Player_Commerce.cs:86-108`) BEFORE the post-buy refresh that drops it from the shop's own list arrives; the old retire pass would have stripped the just-purchased item straight back out of the buyer's inventory. **The exact rule now enforced:** each owned guid remembers the vendor id it was registered under (`Dictionary`, guid -> vendorId), and the retire pass calls `ClientObjectTable.Remove` ONLY when the live object's CURRENT `ContainerId` still equals that recorded vendor id; when it differs (or the object is already gone), the tracking entry is dropped silently and the object itself is left completely untouched — the SAME skip-not-clobber discipline the add-time collision guard below already used, now applied symmetrically on the way out. This is a bug fix, not a new divergence, and does not change this row's still-open scope: retail's actual `ClientObjMaintSystem`/`CObjectMaint` collision behavior on a guid collision remains untraced. **Filed 2026-08-09, Slice 6.1 (shop-item materialization).** `VendorShopItemMaterializer` registers each `ApproachVendor` shop item into `ClientObjectTable` keyed by its own server guid. ACE's `UniqueItemsForSale` (`Vendor.cs:34,638`) can list the EXACT guid a player last held (an item sold to this vendor keeps its original guid), so a guid collision against an existing, differently-owned `ClientObjectTable` entry is a real, if rare, possibility. No retail behavior for this exact case was traced (retail's `ClientObjMaintSystem`/`CObjectMaint` guid-keyed registration internals were not decompiled for this pass). acdream's policy is a conscious, conservative default: a guid this materializer did NOT itself add to the table on a previous cycle is treated as owned by something else and is left completely untouched — never overwritten, never later removed by this class. | `src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs` (`OnVendorTransition`'s collision guard) | Skip-not-clobber is the safe default absent a traced retail mechanism: silently reparenting a live entity's or another container's item into the vendor's `ContainerId` would corrupt real ownership state (equipment tracking, burden, radar) for a guid this code does not own, which is strictly worse than a single shop row's status-bar/appraisal projection staying blank. The vendor list itself is unaffected either way — `VendorUiController` reads display fields straight off `VendorShopItem`, never through `ClientObjectTable`. | If retail's actual behavior differs (e.g. it always overwrites, or a real `UniqueItemsForSale` collision is more common than assumed), the one colliding shop row's status-bar/appraisal projection stays stale/blank instead of showing the vendor listing — a narrow, single-row display gap, never a corrupted non-vendor object. Retiring this row requires tracing retail's `ClientObjMaintSystem` registration behavior on a guid collision, which was out of scope for this pass. | No direct retail citation traced this pass — `Vendor.cs:34,638` (`UniqueItemsForSale`, ACE) establishes the collision is POSSIBLE, not what retail does about it; `docs/research/2026-08-08-slice6-vendor-transactions-research.md` (task brief: "study how ACE guids vendor stock and state your collision policy with evidence") | | ~~AP-111~~ | **RETIRED 2026-07-11 (M2 held-object parenting)** — equipped hand items are no longer omitted from the render world. CreateObject now preserves Placement/Parent/position timestamp bootstrap; live `0xF749` ParentEvent is parsed with retail sequence freshness; a focused render controller resolves `Setup.HoldingLocations`, applies the child's placement frame, and recomposes the separate child entity after every parent animation tick. Pickup retains the weenie's visual metadata for a later wield. | `src/AcDream.Core.Net/Messages/{CreateObject,ParentEvent}.cs`; `src/AcDream.Core/Meshing/EquippedChildAttachment.cs`; `src/AcDream.App/Rendering/EquippedChildRenderController.cs` | — | — | `ClientCombatSystem::GetDefaultCombatMode @ 0x0056B310`; `SmartBox::HandleParentEvent @ 0x004535D0`; `CPhysicsObj::set_parent @ 0x00515A90`; `CPhysicsObj::UpdateChild @ 0x00512D50` | | AP-112 | The basic combat bar ports visibility, height selection, desired-power slider, exact 1.0/0.8-second charge, ready-stance gating, request/release, `MaybeStopCompletely`, server-response queueing, and auto-repeat, but still omits `StartAttackRequest`'s `FinishJump` call and exact trained-Recklessness visibility semantics (IA-20 keeps the dark range as the accepted baseline) | `src/AcDream.Runtime/Gameplay/RuntimeCombatAttackState.cs`; `src/AcDream.App/UI/Layout/CombatUiController.cs` | The shared player movement owner now performs retail's server-control-gated full stop and movement report before an attack build; the remaining seams require the jump owner and a distinct Recklessness treatment | Starting an attack while charging a jump may not finish that jump exactly when retail does; trained/untrained Recklessness presentation is identical | `ClientCombatSystem::StartAttackRequest @ 0x0056C040`; `CommandInterpreter::MaybeStopCompletely @ 0x006B3B90`; `gmCombatUI::ListenToElementMessage @ 0x004CC430` | | AP-113 | Invalid lifestone-command arguments display the local text `Usage: /lifestone`; retail definitely emits a local usage/error line but Binary Ninja misidentifies the referenced wide-string address, so its exact wording is not yet recovered | `src/AcDream.UI.Abstractions/Panels/Chat/ChatCommandRouter.cs`; `RetailClientCommandCatalog.cs` | The behavior boundary is exact (handled locally, no chat and no game action); only a low-impact diagnostic sentence differs | `/ls now` can show different wording/color from retail while still refusing the invalid request correctly | `ClientCommunicationSystem::DoLifestone @ 0x0056FC70` | diff --git a/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs b/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs index 71fc2295..137a2efb 100644 --- a/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs +++ b/src/AcDream.App/Composition/InteractionRetainedUiComposition.cs @@ -365,12 +365,17 @@ internal sealed class RetailInteractionRetainedUiCompositionFactory // Slice 6.3: ItemInteractionController.TryBuy owns the // reservation dance itself (see its doc comment); this is a // plain wire send, not a second requestUse-shaped delegate. + // F4 (Slice 6 review): report whether the send actually + // happened — a null CurrentSession or a not-in-world session + // must return false so TryBuy releases the reservation instead + // of marking it dispatched for a request nothing ever sent. sendBuy: (vendorGuid, itemGuid, amount, alternateCurrencyId) => - session.CurrentSession?.SendBuy( - vendorGuid, - itemGuid, - amount, - alternateCurrencyId)); + { + if (session.CurrentSession is not { } activeSession || !session.IsInWorld) + return false; + activeSession.SendBuy(vendorGuid, itemGuid, amount, alternateCurrencyId); + return true; + }); } public MagicRuntime CreateMagicRuntime( @@ -671,8 +676,8 @@ internal sealed class RetailInteractionRetainedUiCompositionFactory // guid" — Slice 6.1 guarantees a materialized shop // item's ContainerId IS the vendor's guid, so this reads // straight off the same ClientObjectTable/VendorState - // pair VendorUiController.VendorSplitSize's display-only - // copy also reads, through the SAME VendorSplitPolicy + // pair VendorUiController.ResolveBuyQuantity (F2, Slice 6 + // review) also reads, through the SAME VendorSplitPolicy // mask helper (no second mask copy). guid => d.Inventory.Vendor.VendorId != 0u diff --git a/src/AcDream.App/UI/ItemInteractionController.cs b/src/AcDream.App/UI/ItemInteractionController.cs index 93a78569..175d0f73 100644 --- a/src/AcDream.App/UI/ItemInteractionController.cs +++ b/src/AcDream.App/UI/ItemInteractionController.cs @@ -61,8 +61,12 @@ public sealed class ItemInteractionController : IDisposable private readonly Action? _systemMessage; private readonly AutoWieldController _autoWield; private readonly Action? _requestUse; - // Slice 6.3: vendorGuid, itemGuid, amount, alternateCurrencyId. - private readonly Action? _sendBuy; + // Slice 6.3: vendorGuid, itemGuid, amount, alternateCurrencyId -> true + // when the wire send actually happened. F4 (Slice 6 review): a plain + // Action can't tell TryBuy apart from a silent no-op (no session / not + // in world) — the bool return is what lets TryBuy release the + // reservation instead of leaking BusyCount forever. + private readonly Func? _sendBuy; private readonly RuntimeInteractionTransactionState _runtimeTransactions; private readonly InventoryTransactionState _transactions; @@ -104,7 +108,7 @@ public sealed class ItemInteractionController : IDisposable CombatState? combatState = null, Action? sendChangeCombatMode = null, Action? requestUse = null, - Action? sendBuy = null) + Func? sendBuy = null) { _objects = objects ?? throw new ArgumentNullException(nameof(objects)); _playerGuid = playerGuid ?? throw new ArgumentNullException(nameof(playerGuid)); @@ -242,6 +246,20 @@ public sealed class ItemInteractionController : IDisposable /// exactly what 's /// BusyCount == 0 check already guards for every other request. /// + /// + /// F4 (Slice 6 review): reports whether the wire + /// send actually happened (false when there is no live session or the + /// session is not in world). Mirrors + /// RuntimeInteractionTransactionState.TryDispatchUse's shape — + /// every rejecting path calls + /// before returning, never . + /// The OLD void-returning delegate could not distinguish "sent" from a + /// session-null no-op, so a buy attempted while disconnected/mid-teardown + /// permanently marked the reservation dispatched — BusyCount then + /// never balances, because no UseDone will ever arrive for a + /// request that was never actually sent, leaking "You can only move or + /// use one item at a time" forever. + /// public bool TryBuy(uint vendorGuid, uint itemGuid, int amount, uint alternateCurrencyId) { if (vendorGuid == 0u || itemGuid == 0u || amount <= 0 || _sendBuy is null) @@ -250,15 +268,23 @@ public sealed class ItemInteractionController : IDisposable return false; ItemUseRequestReservation reservation = BeginUseRequestReservation(); + bool dispatched; try { - _sendBuy(vendorGuid, itemGuid, amount, alternateCurrencyId); + dispatched = _sendBuy(vendorGuid, itemGuid, amount, alternateCurrencyId); } catch { reservation.CancelBeforeDispatch(); throw; } + + if (!dispatched) + { + reservation.CancelBeforeDispatch(); + return false; + } + reservation.MarkDispatched(); return true; } diff --git a/src/AcDream.App/UI/Layout/SelectedObjectController.cs b/src/AcDream.App/UI/Layout/SelectedObjectController.cs index b101a5dc..9fa2642a 100644 --- a/src/AcDream.App/UI/Layout/SelectedObjectController.cs +++ b/src/AcDream.App/UI/Layout/SelectedObjectController.cs @@ -261,9 +261,11 @@ public sealed class SelectedObjectController : IRetainedPanelController /// equals VendorState.VendorId) AND its type intersects /// . When true, a stack /// seeds to quantity 1 instead of the full authored stack size — see - /// . VendorUiController.VendorSplitSize - /// answers the SAME question for vendor's own display text via the SAME - /// helper, so the mask exists in exactly + /// . F2 (Slice 6 review): + /// VendorUiController.ResolveBuyQuantity answers the equivalent + /// LIVE question for vendor's own display text and Buy dispatch (the + /// CURRENT slider value, not the seed) via the SAME + /// mask, so the mask exists in exactly /// one place (composed at InteractionRetainedUiComposition). /// public static SelectedObjectController Bind( diff --git a/src/AcDream.App/UI/Layout/VendorUiController.cs b/src/AcDream.App/UI/Layout/VendorUiController.cs index 2985cfca..f3b341d6 100644 --- a/src/AcDream.App/UI/Layout/VendorUiController.cs +++ b/src/AcDream.App/UI/Layout/VendorUiController.cs @@ -273,9 +273,14 @@ public sealed class VendorUiController : IRetainedPanelController _itemList.FillVisibleEmptySlots = true; if (emptySlotSprite != 0u) _itemList.CellEmptySprite = emptySlotSprite; + // F3 (Slice 6 review): vendor rows are never drag sources — see + // UiItemSlot.AllowDragSource. Set here too (empty cells are already + // non-sources via ItemId==0) so the invariant holds by construction + // rather than incidentally. _itemList.EmptySlotFactory = () => new UiItemSlot { SpriteResolve = _itemList.SpriteResolve, + AllowDragSource = false, }; // Slice 6.1: mirrors ExternalContainerController's own // right-click-examine wiring (UiItemSlot.OnEvent's RightClick case). @@ -352,6 +357,11 @@ public sealed class VendorUiController : IRetainedPanelController // increments BusyCount synchronously, before the wire send), and // re-enable on the matching UseDone/cancel, without polling. _itemInteraction.StateChanged += OnInteractionStateChanged; + // F2 (Slice 6 review): retail re-runs UpdateItemsUI on every slider + // change (gmVendorUI::RecvNotice_StackSliderChanged, pc:203262-203278) + // — the displayed name/price must track the LIVE split, not just the + // value at selection time. + _splitQuantity.Changed += OnSplitQuantityChanged; } /// @@ -579,18 +589,38 @@ public sealed class VendorUiController : IRetainedPanelController /// selected) shows zero rows, matching retail. /// /// - /// F4/F7a (Slice 5.4 review): the tail of the SAME retail function - /// (pc:201180-201190) — after every rebuild (fresh open OR an - /// explicit category switch, both funnel through this method) the - /// FIRST item that passed the filter becomes the display selection when - /// the previous one didn't survive it, and the list unconditionally - /// scrolls back to its start (ScrollToShow(m_shopList, 0)). - /// Slice 6.2: retail routes the selection through the global + /// F4/F7a (Slice 5.4 review) + F6 (Slice 6 review): the tail of the SAME + /// retail function (pc:201180-201190) — after every rebuild the + /// list unconditionally scrolls back to its start + /// (ScrollToShow(m_shopList, 0)) and, on the retail caller's + /// "notify" paths, the FIRST item that passed the filter becomes the + /// selection UNCONDITIONALLY — there is no survival test + /// (pc:201180-201184: if (arg3 != 0) SetSelectedObject(i_1, 0), + /// where i_1 is simply the first matching item, 0/none if the + /// filter matched nothing). F6 confirmed all THREE of our call sites are + /// retail's notify=1 case, not notify=0: a fresh vendor open AND a + /// same-vendor post-buy/sell refresh both run + /// VendorItemsUI::OpenVendor UNCONDITIONALLY (pc:203852, + /// not gated on the sameVendor flag), which clamps the dropdown index + /// and calls UIElement_Menu::SetSelectedItem(..., 1) + /// (pc:200783-201022) — that trailing 1 is retail's + /// selection-changed notify flag, and setting the menu's selection + /// synchronously cascades into UpdateItemsList(0, 1) via + /// gmVendorUI::ListenToElementMessage's idMessage==7 case + /// (pc:204302-204303) — the SAME idMessage==7/notify=1 path a + /// manual category switch () already takes. + /// Retail's ONLY notify=0 (no-reselect) call site is a bare tab-page-open + /// with no category/vendor change (idMessage==0x2c, + /// m_OpenPageToken==0x100000bc, pc:204283-204285) — this + /// controller has no equivalent call site ( never + /// calls this method), so every path that reaches + /// is retail's notify=1 case. Slice 6.2: + /// retail routes the selection through the global /// ACCWeenieObject::selectedID/SetSelectedObject - /// (pc:201184, confirmed to be the SAME primitive as the fallback - /// select here) — this now calls - /// instead of the retired private field, so the toolbar status bar and - /// slider light up for the auto-selected item too. + /// (pc:201184) — this calls / + /// instead of the retired private + /// field, so the toolbar status bar and slider light up for the + /// auto-selected item too. /// private void RebuildItemList() { @@ -600,8 +630,14 @@ public sealed class VendorUiController : IRetainedPanelController uint maskValue = (uint)activeMask; IReadOnlyList items = _vendor.Items; + // Best-effort initial paint only — the OLD selection, captured + // before this rebuild. The unconditional Select/Clear call below is + // what actually decides the post-rebuild selection (F6); this just + // avoids a one-frame flash of "nothing highlighted" in the common + // case where the old and new selections turn out to be the same + // guid (SelectionState.Select is a no-op then and won't re-fire + // Changed to correct it). uint? selectedGuid = _selection.SelectedObjectId; - bool selectionStillPresent = false; VendorShopItem? firstItem = null; using (_itemList.DeferLayout()) @@ -614,7 +650,6 @@ public sealed class VendorUiController : IRetainedPanelController if (((item.ItemType ?? 0u) & maskValue) == 0u) continue; firstItem ??= item; - if (item.ItemGuid == selectedGuid) selectionStillPresent = true; // F5 (Slice 5.4 review): forward the icon underlay/ // overlay/effects PublicWeenieDescParser already @@ -630,6 +665,11 @@ public sealed class VendorUiController : IRetainedPanelController { SpriteResolve = _itemList.SpriteResolve, SlotIndex = _itemList.GetNumUIItems(), + // F3 (Slice 6 review): a shop row must never become a + // drag source — see UiItemSlot.AllowDragSource's doc + // comment for why this must be gated at the source, + // not left to every destination handler to reject. + AllowDragSource = false, }; cell.SetItem(item.ItemGuid, icon); cell.Selected = item.ItemGuid == selectedGuid; @@ -640,13 +680,14 @@ public sealed class VendorUiController : IRetainedPanelController } } - if (!selectionStillPresent) - { - if (firstItem is { } first) - _selection.Select(first.ItemGuid, SelectionChangeSource.Vendor); - else - _selection.Clear(SelectionChangeSource.Vendor); - } + // F6: unconditional — no survival test. Every rebuild call site + // (Opened/Refreshed via RebuildCategories, and a manual category + // switch via SelectCategory) is retail's notify=1 case; see the + // remarks above for the decomp trace. + if (firstItem is { } first) + _selection.Select(first.ItemGuid, SelectionChangeSource.Vendor); + else + _selection.Clear(SelectionChangeSource.Vendor); // F7a: unconditional scroll-to-start on every rebuild (retail only // guards on the list being non-empty; resetting an already-empty @@ -666,12 +707,22 @@ public sealed class VendorUiController : IRetainedPanelController /// directly and let this method react). /// /// - /// F2/F3 (Slice 5.4 review): the priced/named QUANTITY is retail's - /// ItemHolder::GetObjectSplitSize (0x00586F00, - /// pc:401465-401477) AS SEEDED for a vendor-shop item by - /// gmToolbarUI::HandleSelectionChanged's vendor branch — see - /// for the exact mask citation. Name: - /// ACCWeenieObject::GetObjectName (0x0058E6E0, + /// F2/F3 (Slice 5.4 review), rewired for F2 (Slice 6 review): the + /// priced/named QUANTITY is retail's ItemHolder::GetObjectSplitSize + /// (0x00586F00, pc:401465-401477) read LIVE — retail + /// re-runs this SAME display update on every slider change + /// (gmVendorUI::RecvNotice_StackSliderChanged, + /// pc:203262-203278, 0x004C4500, wired via + /// below), reading the singular/ + /// plural gate, the count, and the price off the CURRENT split value + /// (pc:202602/202621/202644), not a value frozen at + /// selection time. is the SAME + /// computation uses to decide what it + /// actually sends, so the displayed price always equals what a Buy press + /// would charge. VendorSplitPolicy.SeedQuantity stays only in + /// 's real seeding path (the + /// toolbar slider's INITIAL value); this display no longer reads it. + /// Name: ACCWeenieObject::GetObjectName (0x0058E6E0, /// pc:409056-409132) — NAME_SINGULAR for quantity <= 1, /// NAME_PLURAL for quantity > 1; when no plural is authored /// (m_len==1, an empty PString) retail falls back to the @@ -694,7 +745,7 @@ public sealed class VendorUiController : IRetainedPanelController cell.Selected = cell.ItemId == item.ItemGuid; } - int quantity = VendorSplitSize(item); + int quantity = (int)ResolveBuyQuantity(item); string baseName = quantity <= 1 ? item.Name ?? string.Empty @@ -739,6 +790,24 @@ public sealed class VendorUiController : IRetainedPanelController private void OnSelectionTransition(SelectionTransition transition) { _ = transition; + RefreshSelectionDisplay(); + } + + /// + /// F2 (Slice 6 review): retail's gmVendorUI::RecvNotice_StackSliderChanged + /// (pc:203262-203278, 0x004C4500) — a registered listener + /// on the SAME global slider-changed notice + /// broadcasts, gated (in retail) + /// on the panel being visible and the globally-selected item being one + /// of the vendor's own. already + /// applies that same gate (it no-ops to + /// when the current selection isn't a vendor item), so no separate + /// visibility check is needed here. + /// + private void OnSplitQuantityChanged() => RefreshSelectionDisplay(); + + private void RefreshSelectionDisplay() + { uint? selected = _selection.SelectedObjectId; if (selected is { } guid) { @@ -776,21 +845,26 @@ public sealed class VendorUiController : IRetainedPanelController } /// - /// The quantity retail prices/names a vendor-shop selection at — - /// gmToolbarUI::HandleSelectionChanged's vendor-owned branch - /// (pc:198779-198790). Every row - /// shows IS vendor-owned (its container is unconditionally the open - /// vendor), so the "does this item belong to the open vendor" gate that - /// precedes the mask check in retail's function is always true here and - /// is not reproduced separately. Slice 6.2: delegates to - /// — the single source of truth for the - /// 0xDC41CB0 mask, also used by SelectedObjectController's - /// REAL seeding path (InteractionRetainedUiComposition's - /// isVendorSplitExempt delegate) so the mask exists in exactly - /// one place. + /// F2/F7 (Slice 6 review): the SINGLE quantity computation both the + /// display () and the actual purchase + /// () use — retail's + /// gmVendorUI::BuySingleItem (pc:201661, + /// 0x004C2820): quantity 1 when the item's own authored stack + /// size is <= 1 (pc:201674-201681) — there is no split + /// UI for a non-stack item, so a leftover slider value from a + /// PREVIOUSLY-selected DIFFERENT stackable item must never leak into + /// this one — otherwise the CURRENT slider value via + /// ItemHolder::GetObjectSplitSize (0x00586F00). /// - private static int VendorSplitSize(VendorShopItem item) => - VendorSplitPolicy.SeedQuantity((ItemType)(item.ItemType ?? 0u), item.DescStackSize); + private uint ResolveBuyQuantity(VendorShopItem item) + { + uint stackSize = (uint)Math.Max(item.DescStackSize ?? 1, 1); + if (stackSize <= 1u) + return 1u; + + uint selected = _selection.SelectedObjectId ?? item.ItemGuid; + return _splitQuantity.GetObjectSplitSize(item.ItemGuid, selected, stackSize); + } /// /// Cost sentence — VendorItemsUI::UpdateItemsUI's tail @@ -852,10 +926,20 @@ public sealed class VendorUiController : IRetainedPanelController SetActionButtonsEnabled(false); } + /// + /// F8 (Slice 6 review): "Add to List" (staging into the "Buying" tab, + /// contract decision 6) has NO wired OnClick at all — before this + /// fix it enabled on selection exactly like Buy, so it read as a working + /// affordance that silently did nothing on click, which is worse than a + /// disabled button (a disabled Add correctly signals "not available + /// yet"; an enabled dead Add signals a bug). Permanently disabled until + /// the "Buying" tab's staging list is actually implemented — see the + /// register, AP-161. + /// private void SetActionButtonsEnabled(bool enabled) { _buyEnabledBySelection = enabled; - if (_addButton is not null) _addButton.Enabled = enabled; + if (_addButton is not null) _addButton.Enabled = false; RecomputeBuyButtonEnabled(); } @@ -864,8 +948,9 @@ public sealed class VendorUiController : IRetainedPanelController /// something selected" (, set by /// ) and "is the shared inventory/ /// use gate free right now" (). - /// The Add button (staging, contract decision 6 — unwired this pass) - /// stays selection-only. Called on every selection change AND on every + /// The Add button (staging, contract decision 6 — unwired this pass) is + /// PERMANENTLY disabled instead (F8, Slice 6 review) — see + /// . Called on every selection change AND on every /// tick, so the /// button disables the instant /// takes its reservation and re-enables on the matching completion — @@ -881,13 +966,16 @@ public sealed class VendorUiController : IRetainedPanelController /// /// Slice 6.3: retail gmVendorUI::BuySingleItem (pc:201661). - /// Reads the CURRENT globally-selected shop item and the CURRENT split - /// quantity, then dispatches a single-item purchase through the shared - /// use/inventory reservation. Client-side affordability/capacity - /// pre-checks are deliberately NOT ported (research doc's open question - /// 1: the server is authoritative either way and pre-checks are latency/ - /// UX polish, not correctness — deferred as a fast follow-up if the - /// round-trip lag on a refused purchase is noticeable live). + /// Reads the CURRENT globally-selected shop item and dispatches a + /// single-item purchase through the shared use/inventory reservation, + /// using — the SAME quantity + /// computation prices/names the selection + /// at (F2), so the amount actually sent always matches what was shown. + /// Client-side affordability/capacity pre-checks are deliberately NOT + /// ported (research doc's open question 1: the server is authoritative + /// either way and pre-checks are latency/UX polish, not correctness — + /// deferred as a fast follow-up if the round-trip lag on a refused + /// purchase is noticeable live). /// private void BuySelectedItem() { @@ -906,8 +994,7 @@ public sealed class VendorUiController : IRetainedPanelController if (selected is not { } shopItem) return; - uint stackSize = (uint)Math.Max(shopItem.DescStackSize ?? 1, 1); - uint quantity = _splitQuantity.GetObjectSplitSize(shopItem.ItemGuid, guid, stackSize); + uint quantity = ResolveBuyQuantity(shopItem); _itemInteraction.TryBuy( _vendor.VendorId, shopItem.ItemGuid, @@ -947,6 +1034,7 @@ public sealed class VendorUiController : IRetainedPanelController _selection.Changed -= OnSelectionTransition; _objects.ObjectRemoved -= OnObjectRemoved; _itemInteraction.StateChanged -= OnInteractionStateChanged; + _splitQuantity.Changed -= OnSplitQuantityChanged; RetailTabBinding.SetClick(_itemsTab, null); RetailTabBinding.SetClick(_buyingTab, null); RetailTabBinding.SetClick(_sellingTab, null); diff --git a/src/AcDream.App/UI/UiItemSlot.cs b/src/AcDream.App/UI/UiItemSlot.cs index 898c14af..d99f9869 100644 --- a/src/AcDream.App/UI/UiItemSlot.cs +++ b/src/AcDream.App/UI/UiItemSlot.cs @@ -140,7 +140,7 @@ public class UiItemSlot : UiElement /// public override object? GetDragPayload() - => ItemId != 0 && !_primaryPressConsumed + => AllowDragSource && ItemId != 0 && !_primaryPressConsumed ? new ItemDragPayload(ItemId, SourceKind, SlotIndex, this, Shortcut) : null; @@ -158,7 +158,8 @@ public class UiItemSlot : UiElement internal override void SetDragSourceActive(bool active, object? payload) { // ItemList_BeginDrag ghosts physical lists, but explicitly excludes shortcut lists - // (along with vendor/salvage lists, which acdream does not model as ItemDragSource). + // (along with vendor/salvage lists — see AllowDragSource above, F3: those never + // reach here at all, since IsDragSource is false for them). // Keep the source's full cell icon in place and reveal the authored grey mesh over it. SetWaitingState(active && SourceKind != ItemDragSource.ShortcutBar); } @@ -171,13 +172,35 @@ public class UiItemSlot : UiElement internal void SetWaitingState(bool waiting) => _waiting = waiting && ItemId != 0; + /// + /// F3 (Slice 6 review): opt-out for lists whose rows must NEVER initiate + /// a drag, regardless of occupancy — retail's ItemList_BeginDrag + /// explicitly excludes vendor/salvage lists from drag-drop (see the + /// comment above). Before shop items + /// had real identity (Slice 6.1) a + /// dragged shop guid failed every destination's existence guard as a + /// harmless no-op; once it resolved, the SAME drag would pass a pack + /// drop's PutItemInContainer, persist a dangling shortcut-bar + /// entry, or fire PlaceIn3D — reparenting a vendor's stock + /// without ever going through Buy. Gating the SOURCE here (rather than + /// asking every destination handler to reject a vendor-tagged payload) + /// means the drag never starts at all, so no future drop handler can + /// regress this by forgetting a check. Defaults true — every existing + /// physical list (inventory, paperdoll, container, shortcut bar) is + /// unaffected; is the only caller that + /// sets it false. + /// + public bool AllowDragSource { get; set; } = true; + /// An OCCUPIED slot is a drag source — a press-and-move picks up the item /// rather than moving the toolbar window. An EMPTY slot is NOT a drag source, so a /// press-and-move there falls through to the IA-12 whole-window-drag, keeping the bar /// movable by its empty cells / chrome. Drives 's mousedown /// window-vs-item disambiguation (retail moves the window via a dragbar, never cells; - /// our whole-window-drag approximation reconciles by gating on occupancy). - public override bool IsDragSource => ItemId != 0; + /// our whole-window-drag approximation reconciles by gating on occupancy). + /// is an additional, independent gate (F3) — false for + /// vendor rows regardless of ItemId. + public override bool IsDragSource => ItemId != 0 && AllowDragSource; /// Walk up to the containing (the drop handler owner). protected UiItemList? FindList() diff --git a/src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs b/src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs index 728b26f5..2177bfc4 100644 --- a/src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs +++ b/src/AcDream.Runtime/Gameplay/VendorShopItemMaterializer.cs @@ -86,12 +86,40 @@ namespace AcDream.Runtime.Gameplay; /// already was, which is safe by construction and never corrupts a real /// object's ownership. /// +/// +/// +/// Ownership re-check on retire (review finding F1). Buying a +/// UNIQUE vendor item does not merely drop it from the next +/// ApproachVendor snapshot — ACE first re-containers the SAME guid +/// into the BUYER's own pack via CreateObject +/// (Player_Commerce.cs:86-108, A.2 of the Slice 6 research doc) and +/// only THEN sends the full-replace refresh that no longer lists it. If +/// the retire pass below removed every guid merely absent from the new +/// snapshot, it would delete the just-purchased item straight back out of +/// the buyer's own inventory the instant the post-buy refresh landed. Each +/// owned guid therefore remembers the vendor id it was registered under, +/// and the retire pass only calls +/// when the LIVE object's current ContainerId still equals that +/// recorded vendor id — i.e. nothing else has re-containered it since. +/// When it no longer matches (a purchase moved it to the buyer, or some +/// other owner claimed it), the tracking entry is dropped silently and the +/// object itself is left completely untouched, mirroring the collision +/// policy above. +/// /// public sealed class VendorShopItemMaterializer : IDisposable { private readonly VendorState _vendor; private readonly ClientObjectTable _objects; - private readonly HashSet _ownedGuids = new(); + + /// + /// Guids this materializer currently owns in , + /// mapped to the vendor id they were registered under. F1: the retire + /// pass re-checks the live object's ContainerId against this + /// recorded value before deleting anything — see the class doc's + /// "Ownership re-check on retire" section. + /// + private readonly Dictionary _ownedGuids = new(); private bool _disposed; public VendorShopItemMaterializer(VendorState vendor, ClientObjectTable objects) @@ -110,7 +138,7 @@ public sealed class VendorShopItemMaterializer : IDisposable public int OwnedCount => _ownedGuids.Count; /// True if is a shop item this materializer put in the table. - public bool Owns(uint guid) => _ownedGuids.Contains(guid); + public bool Owns(uint guid) => _ownedGuids.ContainsKey(guid); private void OnVendorTransition(VendorTransition transition) { @@ -119,40 +147,78 @@ public sealed class VendorShopItemMaterializer : IDisposable foreach (VendorShopItem item in currentItems) stillListed.Add(item.ItemGuid); - // Retire every guid we own that fell out of the new snapshot (sold - // out, session closed/reset, or a different vendor superseded this - // one — in every one of those cases stillListed is missing it). - // Runs BEFORE the materialize loop below: "on REPLACE, the old - // vendor's items go before the new ones land." - foreach (uint guid in _ownedGuids) + var nextOwned = new Dictionary(currentItems.Count); + try { - if (!stillListed.Contains(guid)) - _objects.Remove(guid); - } - - var nextOwned = new HashSet(currentItems.Count); - foreach (VendorShopItem item in currentItems) - { - bool ownedAlready = _ownedGuids.Contains(item.ItemGuid); - if (!ownedAlready && _objects.Get(item.ItemGuid) is not null) + // Retire every guid we own that fell out of the new snapshot (sold + // out, session closed/reset, or a different vendor superseded this + // one — in every one of those cases stillListed is missing it). + // Runs BEFORE the materialize loop below: "on REPLACE, the old + // vendor's items go before the new ones land." + // + // Iterate a SNAPSHOT (F5): ClientObjectTable.Remove synchronously + // fires ObjectRemoved to every subscriber with no per-listener + // isolation (unlike VendorState's own Changed dispatch). A + // throwing external observer must not abort this loop midway and + // strand the remaining guids un-retired. + foreach (KeyValuePair owned in new List>(_ownedGuids)) { - // Collision guard — see class doc. Never take ownership of a - // guid this materializer did not itself add. - Console.Error.WriteLine( - "[VendorShopItemMaterializer] skipped guid=0x" - + item.ItemGuid.ToString("X8") - + " — already present in ClientObjectTable and not " - + "owned by this vendor session."); - continue; + if (stillListed.Contains(owned.Key)) + continue; + + // F1 — re-check ownership before removing. A purchase can + // have already re-containered this guid into the buyer's + // pack (see class doc); only retire it if it is STILL the + // vendor's, i.e. the live object's ContainerId still equals + // the vendor id we registered it under. If it moved, drop + // the tracking entry silently and leave the (now + // someone-else's) object completely untouched. + ClientObject? live = _objects.Get(owned.Key); + if (live is null || live.ContainerId != owned.Value) + continue; + + try + { + _objects.Remove(owned.Key); + } + catch (Exception error) + { + System.Diagnostics.Trace.TraceError( + "[VendorShopItemMaterializer] ObjectRemoved observer " + + "threw retiring guid=0x{0}: {1}", + owned.Key.ToString("X8"), + error); + } } - _objects.Ingest(ToWeenieData(item, transition.VendorId)); - nextOwned.Add(item.ItemGuid); - } + foreach (VendorShopItem item in currentItems) + { + bool ownedAlready = _ownedGuids.ContainsKey(item.ItemGuid); + if (!ownedAlready && _objects.Get(item.ItemGuid) is not null) + { + // Collision guard — see class doc. Never take ownership of a + // guid this materializer did not itself add. + Console.Error.WriteLine( + "[VendorShopItemMaterializer] skipped guid=0x" + + item.ItemGuid.ToString("X8") + + " — already present in ClientObjectTable and not " + + "owned by this vendor session."); + continue; + } - _ownedGuids.Clear(); - foreach (uint guid in nextOwned) - _ownedGuids.Add(guid); + _objects.Ingest(ToWeenieData(item, transition.VendorId)); + nextOwned[item.ItemGuid] = transition.VendorId; + } + } + finally + { + // F5: guaranteed to run even if the retire/materialize passes + // above throw somewhere this method doesn't already catch, so + // _ownedGuids never straddles two inconsistent generations. + _ownedGuids.Clear(); + foreach (KeyValuePair entry in nextOwned) + _ownedGuids[entry.Key] = entry.Value; + } } /// @@ -198,5 +264,12 @@ public sealed class VendorShopItemMaterializer : IDisposable if (_disposed) return; _disposed = true; _vendor.Changed -= OnVendorTransition; + // F5 safety net: RuntimeInventoryState.Dispose() always runs + // Vendor.Reset() first (which drives OnVendorTransition's own retire + // pass down to zero), but a caller that disposes this class directly + // without a prior Reset()/Close() must not leave stale tracking + // entries behind — OwnedCount feeds + // RuntimeInventoryOwnershipSnapshot.IsConverged. + _ownedGuids.Clear(); } } diff --git a/tests/AcDream.App.Tests/UI/ItemInteractionControllerTests.cs b/tests/AcDream.App.Tests/UI/ItemInteractionControllerTests.cs index c98935e8..9ed61758 100644 --- a/tests/AcDream.App.Tests/UI/ItemInteractionControllerTests.cs +++ b/tests/AcDream.App.Tests/UI/ItemInteractionControllerTests.cs @@ -28,6 +28,11 @@ public sealed class ItemInteractionControllerTests public readonly List<(uint Item, uint Amount)> SplitDrops = new(); public readonly List<(uint Target, uint Item, uint Amount)> Gives = new(); public readonly List<(uint VendorGuid, uint ItemGuid, int Amount, uint AlternateCurrencyId)> Buys = new(); + // F4 (Slice 6 review): simulates the composition-root sendBuy + // delegate's "no live session / not in world" no-op case — + // TryBuy must see this as false and release the reservation + // rather than mark it dispatched for a request nothing sent. + public bool SendBuySucceeds = true; public readonly List Toasts = new(); public readonly List SystemMessages = new(); public readonly List CombatModeRequests = new(); @@ -97,7 +102,12 @@ public sealed class ItemInteractionControllerTests sendChangeCombatMode: CombatModeRequests.Add, requestUse: requestUse, sendBuy: (vendorGuid, itemGuid, amount, alternateCurrencyId) => - Buys.Add((vendorGuid, itemGuid, amount, alternateCurrencyId))); + { + if (!SendBuySucceeds) + return false; + Buys.Add((vendorGuid, itemGuid, amount, alternateCurrencyId)); + return true; + }); } public ItemInteractionController Controller { get; } @@ -2267,4 +2277,35 @@ public sealed class ItemInteractionControllerTests Assert.Equal(0, h.Controller.BusyCount); } + + [Fact] + public void TryBuy_NoSessionToSendOn_ReleasesTheReservation_AndASubsequentBuyWorks() + { + // F4 (Slice 6 review): the composition-root sendBuy delegate + // returns false when there is no live session (or it's not in + // world) -- BEFORE this fix, sendBuy was a plain Action, so TryBuy + // could not tell "sent" apart from a silent no-op and always + // called MarkDispatched(). Since nothing was actually sent, no + // UseDone would ever arrive to balance it, leaking BusyCount and + // permanently wedging every future Use/Buy behind "You can only + // move or use one item at a time." + var h = new Harness(); + h.SendBuySucceeds = false; + + bool result = h.Controller.TryBuy(0x40001000u, 0x50002000u, 1, 0u); + + Assert.False(result); + Assert.Empty(h.Buys); + // The reservation was cancelled, not dispatched -- the gate is + // free again immediately, with no UseDone needed to release it. + Assert.Equal(0, h.Controller.BusyCount); + + // A subsequent buy, once a session IS available, proceeds normally. + h.SendBuySucceeds = true; + bool second = h.Controller.TryBuy(0x40001000u, 0x50002001u, 1, 0u); + + Assert.True(second); + Assert.Single(h.Buys); + Assert.Equal(1, h.Controller.BusyCount); + } } diff --git a/tests/AcDream.App.Tests/UI/Layout/VendorUiControllerTests.cs b/tests/AcDream.App.Tests/UI/Layout/VendorUiControllerTests.cs index 44ea0853..0687fd43 100644 --- a/tests/AcDream.App.Tests/UI/Layout/VendorUiControllerTests.cs +++ b/tests/AcDream.App.Tests/UI/Layout/VendorUiControllerTests.cs @@ -176,7 +176,10 @@ public sealed class VendorUiControllerTests sendDrop: null, sendExamine: Examines.Add, sendBuy: (vendorGuid, itemGuid, amount, alternateCurrencyId) => - Buys.Add((vendorGuid, itemGuid, amount, alternateCurrencyId))); + { + Buys.Add((vendorGuid, itemGuid, amount, alternateCurrencyId)); + return true; + }); Controller = VendorUiController.Bind( layout, @@ -293,6 +296,15 @@ public sealed class VendorUiControllerTests }); h.ItemList.GetItem(0)!.Clicked?.Invoke(); + // F2 (Slice 6 review): the display now reads the LIVE split state + // instead of recomputing a static seed. Production seeds this via + // SelectedObjectController (mounted on the toolbar, subscribed to + // SelectionState.Changed BEFORE VendorUiController — RetailUiRuntime + // MountToolbar() runs before MountVendor()); this harness doesn't + // mount that controller, so the test seeds SplitQuantity directly to + // stand in for it, matching the SAME not-exempt full-stack seed + // SelectedObjectController.ApplySelection would have computed. + h.SplitQuantity.Reset(100u, initialValue: 100u); // No authored PluralName -> ACCWeenieObject::GetObjectName falls back // to the singular name UNCHANGED (not an invented "Arrows" + "s"). @@ -319,6 +331,10 @@ public sealed class VendorUiControllerTests }); h.ItemList.GetItem(0)!.Clicked?.Invoke(); + // F2: exempt seed is 1 regardless of the 50-unit authored stack — + // see the split-exempt comment on the sibling test above for why + // this harness seeds SplitQuantity explicitly. + h.SplitQuantity.Reset(50u, initialValue: 1u); Assert.Equal("Bread", GetText(h.ItemNameText)); Assert.Equal( @@ -338,10 +354,56 @@ public sealed class VendorUiControllerTests }); h.ItemList.GetItem(0)!.Clicked?.Invoke(); + // F2: Key is not split-exempt -> full authored stack (10). + h.SplitQuantity.Reset(10u, initialValue: 10u); Assert.Equal("10 Iron Keys", GetText(h.ItemNameText)); } + [Fact] + public void SliderChange_AfterSelection_UpdatesNameAndPriceToLiveQuantity_MatchingWhatBuyWouldCharge() + { + // F2 (Slice 6 review): retail re-runs UpdateItemsUI on every slider + // change (gmVendorUI::RecvNotice_StackSliderChanged, + // pc:203262-203278) -- the OLD implementation froze the display's + // quantity at a static per-selection seed while BuySelectedItem + // separately read the LIVE split, so a slider drag after selecting + // never updated the sentence/price and could show a DIFFERENT price + // than what Buy would actually charge. Both now go through the SAME + // ResolveBuyQuantity, proven here by asserting the displayed price + // equals the amount the subsequent Buy press actually sends. + var h = new Harness(); + h.State.Apply(VendorGuid, Profile(sellRate: 2.0f), new[] + { + // perUnit = 1000/100 = 10 (MissileWeapon, not split-exempt). + new VendorShopItem( + StackedItemGuid, -1, 3u, "Arrows", (uint)ItemType.MissileWeapon, 300u, 1000, + DescStackSize: 100), + }); + // F6 auto-selects the sole item; the harness's un-mounted-toolbar + // split state starts at its class default (Value=1/Maximum=1) — + // singular display, matching a fresh, not-yet-slider-touched + // selection. + Assert.Equal("Arrows", GetText(h.ItemNameText)); + Assert.Equal( + $"costs {20:N0} (you have {Harness.DefaultPlayerCoinValue:N0})", + GetText(h.ItemCostText)); + + // Player drags the slider to 40 AFTER selecting -- no re-click, no + // re-selection, purely a StackSplitQuantityState.Changed event. + h.SplitQuantity.Reset(100u, initialValue: 40u); + + // SellPrice = ceil(2.0*10*40 - 0.1) = 800. + Assert.Equal("40 Arrows", GetText(h.ItemNameText)); + Assert.Equal( + $"cost {800:N0} (you have {Harness.DefaultPlayerCoinValue:N0})", + GetText(h.ItemCostText)); + + h.BuyButton.OnClick!.Invoke(); + + Assert.Equal(40, h.Buys.Single().Amount); + } + [Fact] public void SelectingItem_WithAlternateCurrency_ShowsFullRetailCostSentence() { @@ -370,10 +432,34 @@ public sealed class VendorUiControllerTests } [Fact] - public void NoSelection_DisablesBuyAndAddButtons_SelectionEnablesThem() + public void NoSelection_DisablesBuyButton_SelectionEnablesIt() { var h = new Harness(); Assert.False(h.BuyButton.Enabled); + + h.State.Apply(VendorGuid, Profile(), new[] + { + new VendorShopItem(ArmorItemGuid, -1, 2u, "Chainmail", (uint)ItemType.Armor, 200u, 500), + }); + + // F4 auto-selects the sole item on open, so Buy is already enabled + // — SetState(1), pc:202784-202789. + Assert.True(h.BuyButton.Enabled); + + // Closing clears the selection -> SetState(0xd), pc:202572-202577. + h.State.Close(); + Assert.False(h.BuyButton.Enabled); + } + + [Fact] + public void AddButton_IsPermanentlyDisabled_RegardlessOfSelection() + { + // F8 (Slice 6 review): "Add to List" has no wired OnClick at all + // (staging into the "Buying" tab is deferred, contract decision 6) + // — an enabled button that silently does nothing on click is a + // dead-affordance bug, worse than a disabled one. It must never + // enable, with or without a selection. + var h = new Harness(); Assert.False(h.AddButton.Enabled); h.State.Apply(VendorGuid, Profile(), new[] @@ -381,14 +467,10 @@ public sealed class VendorUiControllerTests new VendorShopItem(ArmorItemGuid, -1, 2u, "Chainmail", (uint)ItemType.Armor, 200u, 500), }); - // F4 auto-selects the sole item on open, so the buttons are already - // enabled — SetState(1), pc:202784-202789. - Assert.True(h.BuyButton.Enabled); - Assert.True(h.AddButton.Enabled); + // F4/F6 auto-selects the sole item on open -- Add stays disabled. + Assert.False(h.AddButton.Enabled); - // Closing clears the selection -> SetState(0xd), pc:202572-202577. h.State.Close(); - Assert.False(h.BuyButton.Enabled); Assert.False(h.AddButton.Enabled); } @@ -473,6 +555,29 @@ public sealed class VendorUiControllerTests Assert.NotNull(h.ItemList.EmptySlotFactory); } + [Fact] + public void ShopRow_NeverMintsADragPayload() + { + // F3 (Slice 6 review): with shop items materialized into + // ClientObjectTable (Slice 6.1), a draggable vendor cell would pass + // every destination handler's existence guard -- a pack drop would + // send a real PutItemInContainer for vendor stock, the shortcut bar + // would persist a dangling guid, and PlaceIn3D would fire. Retail + // excludes vendor/salvage lists from ItemList_BeginDrag entirely; a + // populated shop row must never become a drag source at all. + var h = new Harness(); + h.State.Apply(VendorGuid, Profile(), new[] + { + new VendorShopItem(ArmorItemGuid, -1, 2u, "Chainmail", (uint)ItemType.Armor, 200u, 500), + }); + + UiItemSlot cell = h.ItemList.GetItem(0)!; + + Assert.NotEqual(0u, cell.ItemId); // occupied -- would otherwise be a drag source by default + Assert.False(cell.IsDragSource); + Assert.Null(cell.GetDragPayload()); + } + [Fact] public void Opened_WithDifferentVendor_ResetsToFirstPresentCategory_NotThePreviousVendors() { @@ -529,6 +634,43 @@ public sealed class VendorUiControllerTests Assert.Equal("Food", h.TypeMenu.Items.Single(i => Equals(i.Payload, h.TypeMenu.Selected)).Label); } + [Fact] + public void Refreshed_SameVendor_ReselectsFirstItemUnconditionally_NotThePreviouslySelectedSurvivor() + { + // F6 (Slice 6 review): retail's VendorItemsUI::UpdateItemsList + // selects UNCONDITIONALLY on every rebuild call site this + // controller reaches (pc:201180-201184, confirmed via + // VendorItemsUI::OpenVendor's unconditional clamp + + // SetSelectedItem(...,1) at pc:201022, which every vendor + // open/refresh runs regardless of the sameVendor flag) -- there is + // NO survival test. A same-vendor refresh that re-lists the SAME + // two items in the SAME order must reset to the FIRST one even + // though the player's prior selection (the second item) is still + // present in the new snapshot -- this is the case that would have + // diverged under the OLD "preserve if it survives" logic. + const uint SecondArmorGuid = 0x60000110u; + var h = new Harness(); + var items = new[] + { + new VendorShopItem(ArmorItemGuid, -1, 2u, "Chainmail", (uint)ItemType.Armor, 200u, 500), + new VendorShopItem(SecondArmorGuid, -1, 4u, "Buckler", (uint)ItemType.Armor, 900u, 40), + }; + h.State.Apply(VendorGuid, Profile(), items); + Assert.Equal(ArmorItemGuid, h.ItemList.GetItem(0)!.ItemId); // auto-selected first + + // Player explicitly picks the SECOND item. + h.ItemList.GetItem(1)!.Clicked?.Invoke(); + Assert.True(h.ItemList.GetItem(1)!.Selected); + + // SAME vendor, SAME two items, SAME order -> Refreshed transition. + h.State.Apply(VendorGuid, Profile(), items); + + // Retail resets to the FIRST item, not the still-present second one. + Assert.True(h.ItemList.GetItem(0)!.Selected); + Assert.False(h.ItemList.GetItem(1)!.Selected); + Assert.Equal("Chainmail", GetText(h.ItemNameText)); + } + [Fact] public void CategoryMenu_OpensAndSelectsThroughRealHitPath_UsingAuthoredPopupGeometry() { @@ -723,6 +865,36 @@ public sealed class VendorUiControllerTests Assert.Equal(25, h.Buys.Single().Amount); } + [Fact] + public void BuyButton_Press_NonStackedItem_IgnoresAStaleSliderFromAPreviouslySelectedStackableItem() + { + // F7 (Slice 6 review): retail's BuySingleItem (pc:201674-201681) + // gates on the SELECTED item's OWN stack size FIRST -- quantity 1 + // whenever it is <=1, regardless of whatever + // GenItemHolder::splitSize still holds from a PREVIOUSLY selected, + // DIFFERENT, stackable item. Without this guard, + // ResolveBuyQuantity's GetObjectSplitSize(item.ItemGuid, selected, + // stackSize) call trivially returns the live Value for ANY selected + // item (item.ItemGuid always equals the selection guid at this call + // site) -- leaking a stale quantity into a non-stack purchase. + var h = new Harness(); + h.State.Apply(VendorGuid, Profile(), new[] + { + new VendorShopItem(ArmorItemGuid, -1, 2u, "Chainmail", (uint)ItemType.Armor, 200u, 500), + }); + // Simulate a stale slider left over from a PREVIOUSLY selected, + // different, stackable item -- this harness doesn't mount + // SelectedObjectController, which in production would already have + // reset the slider on selecting the (non-stack) armor; seeding it + // directly here proves the clamp doesn't depend on that reset + // having run. + h.SplitQuantity.Reset(50u, initialValue: 30u); + + h.BuyButton.OnClick!.Invoke(); + + Assert.Equal(1, h.Buys.Single().Amount); + } + [Fact] public void BuyButton_Press_AlternateCurrencyVendor_ForwardsTheVendorsTradeWcid() { diff --git a/tests/AcDream.App.Tests/UI/UiItemSlotTests.cs b/tests/AcDream.App.Tests/UI/UiItemSlotTests.cs index de5b6946..10907207 100644 --- a/tests/AcDream.App.Tests/UI/UiItemSlotTests.cs +++ b/tests/AcDream.App.Tests/UI/UiItemSlotTests.cs @@ -66,6 +66,33 @@ public class UiItemSlotTests Assert.Null(slot.GetDragPayload()); } + // ── F3 (Slice 6 review): AllowDragSource ──────────────────────────────── + + [Fact] + public void AllowDragSource_DefaultsTrue_OccupiedSlotIsADragSource() + { + var s = new UiItemSlot(); + s.SetItem(0x5001u, 0x99u); + + Assert.True(s.IsDragSource); + Assert.NotNull(s.GetDragPayload()); + } + + [Fact] + public void AllowDragSource_False_OccupiedSlotIsNeverADragSource() + { + // A vendor row (or any future list retail excludes from + // ItemList_BeginDrag, e.g. salvage) sets this false. IsDragSource is + // the sole gate UiRoot reads at MouseDown to decide whether a + // press-and-move even becomes a drag CANDIDATE -- GetDragPayload() + // is never reached at all when this is false, so no destination + // drop handler needs to reject anything. + var s = new UiItemSlot { AllowDragSource = false }; + s.SetItem(0x5001u, 0x99u); + + Assert.False(s.IsDragSource); + } + // ── Shortcut number tests ──────────────────────────────────────────────── // Port of UIElement_UIItem::SetShortcutNum (acclient_2013_pseudo_c.txt:229465). diff --git a/tests/AcDream.Runtime.Tests/Gameplay/VendorShopItemMaterializerTests.cs b/tests/AcDream.Runtime.Tests/Gameplay/VendorShopItemMaterializerTests.cs index 7f3408b5..1f943189 100644 --- a/tests/AcDream.Runtime.Tests/Gameplay/VendorShopItemMaterializerTests.cs +++ b/tests/AcDream.Runtime.Tests/Gameplay/VendorShopItemMaterializerTests.cs @@ -119,8 +119,15 @@ public sealed class VendorShopItemMaterializerTests [Fact] public void Refreshed_ItemNoLongerListed_IsRemoved() { - // A unique item sold out (bought up / delisted) between one - // ApproachVendor and the next same-vendor refresh. + // An item drops out of the vendor's own list WITHOUT ever being + // re-containered elsewhere (e.g. admin-removed stock, a delisted + // line item) -- its live ContainerId is still the vendor's own + // guid, so the retire pass's F1 ownership re-check finds a match + // and removes it. This is deliberately NOT "I bought it" -- see + // Refreshed_ItemPurchased_ReparentedIntoBuyerPack_Survives for that + // case, where the SAME "missing from the new snapshot" trigger must + // NOT delete the item because a real purchase already moved it into + // the buyer's own pack before this refresh arrived. var vendor = new VendorState(); var objects = new ClientObjectTable(); using var materializer = new VendorShopItemMaterializer(vendor, objects); @@ -133,6 +140,62 @@ public sealed class VendorShopItemMaterializerTests Assert.Equal(1, materializer.OwnedCount); } + [Fact] + public void Refreshed_ItemPurchased_ReparentedIntoBuyerPack_Survives() + { + // F1: buying a UNIQUE vendor item does not merely drop it from the + // next ApproachVendor snapshot -- ACE first re-containers the SAME + // guid into the BUYER's own pack via CreateObject + // (Player_Commerce.cs:86-108) and only THEN sends the full-replace + // refresh that no longer lists it. The retire pass must re-check + // live ownership (ContainerId) before deleting, or it strips the + // just-purchased item straight back out of the buyer's inventory. + var vendor = new VendorState(); + var objects = new ClientObjectTable(); + using var materializer = new VendorShopItemMaterializer(vendor, objects); + vendor.Apply(VendorGuid, default, new[] { Item(ItemA, "Unique Sword"), Item(ItemB) }); + Assert.Equal(VendorGuid, objects.Get(ItemA)!.ContainerId); + + const uint BuyerGuid = 0x50009000u; + // Simulate the purchase's CreateObject: ACE re-containers the SAME + // guid into the buyer's pack BEFORE the post-buy ApproachVendor + // refresh arrives. + objects.Ingest(new WeenieData( + Guid: ItemA, + Name: null, + Type: null, + WeenieClassId: 0, + IconId: 0, + IconOverlayId: 0, + IconUnderlayId: 0, + Effects: 0, + Value: null, + StackSize: null, + StackSizeMax: null, + Burden: null, + ContainerId: BuyerGuid, + WielderId: null, + ValidLocations: null, + CurrentWieldedLocation: null, + Priority: null, + ItemsCapacity: null, + ContainersCapacity: null, + Structure: null, + MaxStructure: null, + Workmanship: null)); + Assert.Equal(BuyerGuid, objects.Get(ItemA)!.ContainerId); + + // Post-buy ApproachVendor refresh: the purchased item is gone from + // the shop's own list. + vendor.Apply(VendorGuid, default, new[] { Item(ItemB) }); + + ClientObject? survivor = objects.Get(ItemA); + Assert.NotNull(survivor); + Assert.Equal(BuyerGuid, survivor!.ContainerId); + Assert.False(materializer.Owns(ItemA)); + Assert.Equal(1, materializer.OwnedCount); + } + [Fact] public void CollidingGuid_AlreadyOwnedBySomethingElse_IsNeverClobbered() { @@ -188,4 +251,38 @@ public sealed class VendorShopItemMaterializerTests vendor.Close(); Assert.NotNull(objects.Get(ItemA)); } + + [Fact] + public void Retire_ThrowingObjectRemovedObserver_StillRetiresRemainingGuidsAndConverges() + { + // F5: ClientObjectTable.Remove fires ObjectRemoved with NO + // per-listener isolation (unlike VendorState's own Changed + // dispatch). One throwing observer must not abort the retire loop + // partway through and strand _ownedGuids -- every owned guid still + // retires, and OwnedCount still converges to zero. + var vendor = new VendorState(); + var objects = new ClientObjectTable(); + using var materializer = new VendorShopItemMaterializer(vendor, objects); + vendor.Apply(VendorGuid, default, new[] { Item(ItemA), Item(ItemB) }); + Assert.Equal(2, materializer.OwnedCount); + + // A throwing ObjectRemoved observer fires on EVERY Remove() call + // (both ItemA's and ItemB's) -- multicast delegate invocation has + // no per-listener isolation, so each Remove() call itself throws. + objects.ObjectRemoved += _ => throw new InvalidOperationException("boom"); + + vendor.Close(); + + // Both guids are gone from the table -- the throwing first observer + // did not stop the second guid's Remove() call from happening. + Assert.Null(objects.Get(ItemA)); + Assert.Null(objects.Get(ItemB)); + Assert.Equal(0, materializer.OwnedCount); + Assert.False(materializer.Owns(ItemA)); + Assert.False(materializer.Owns(ItemB)); + + // Dispose still converges cleanly afterward. + materializer.Dispose(); + Assert.Equal(0, materializer.OwnedCount); + } }