diff --git a/docs/architecture/retail-divergence-register.md b/docs/architecture/retail-divergence-register.md index 56f08218..9f1ad051 100644 --- a/docs/architecture/retail-divergence-register.md +++ b/docs/architecture/retail-divergence-register.md @@ -62,7 +62,7 @@ accepted-divergence entries (#96, #49, #50). --- -## 2. Adaptation (AD) — 51 active rows (AD-65 RETIRED and AD-69 FILED 2026-08-07 at Campaign S S4 — the away-arm now snaps per retail @0x00509c50, while AD-66's byte-confirmed sibling landing is WITHHELD pending #341's measurement-anomaly apparatus, and AD-69 records the seam-frame dist gap the same pass discovered; AD-56 RESTORED 2026-08-07 — the a8a7d64b revert had collaterally DELETED it, the inverse of the AD-55 zombie it also created; its plumb-fall-freeze condition is live again since TS-4’s real retirement at Slice 2B; AD-55 RE-RETIRED 2026-08-07 — its 2026-07-30 retirement at 252e8068 was collaterally resurrected by the a8a7d64b revert of the unrelated TS-4 commit; the code kept the cos(10°) fix throughout; AD-68 filed 2026-08-07 at the #338 closure — the async-residency placeholder mover shape (0.4/0.4 steps + capsule) has no retail counterpart because retail loads synchronously; AD-67 filed 2026-08-07 at the #32 closeout — the narrowed `SetContactPlane` keeps its per-write `ContactPlaneCellId`, which retail writes only at `init_contact_plane`; AD-49 filed 2026-08-06 at the #334 fix — the BSP part-array flood runs its outdoor cell rectangle at seed time rather than only from retail’s residency-gated walk, keeping both registration floods on one residency rule; AD-64 filed 2026-08-05 at the C5b architecture review's D1 fix — AD-60's W2 wire-cell REACHABILITY decision is expressed once per host because the two hosts run parallel non-shared inbound routes; the committed VALUE is single-sourced at `RuntimeEntityObjectLifetime.CommitWireCellRebucket`, and unification is filed as #324; AD-60 CORRECTED the same day — its surviving-channel enumeration presented "the local force path, the missile arm" as exhaustive when the entire no-window host belonged in it; AD-1 RETIRED 2026-08-05, C5a deletion sweep — the legacy outdoor demote/restore lift this row described was `PhysicsEngine.Resolve`'s own body, deleted with zero production callers; AD-42 DELETED 2026-08-04, C4 route 3 — its last surviving citation, the headless portal-arrival resync's two-call Resolve/ResolvePlacement split, was retired by the canonical `RuntimeAcceptedPositionDriveController` portal arm; AD-2 amended same route with the deferred-place timing adaptation, the T8 tolerated-overwrite note, and the leash-anchor nuance; AD-63 filed 2026-08-04, cancelled-park presentation rollback — the rollback restores every presentation registration the park's Withdraw removed EXCEPT the player's selection, which is user intent rather than a projection; AD-62 filed 2026-08-03, C4 route 2 round 2 — a deferred ForcePosition retired without committing is not re-applied and its ack is not sent; AD-61 filed 2026-08-02, C3c review round 1 — the #270 settle compression now covers the local player; AD-59/AD-60 filed 2026-08-02, continuation-executor slice) +## 2. Adaptation (AD) — 52 active rows (AD-72 filed 2026-08-08 at the Slice 5.3 review corrections — `VendorPricing`'s double-precision narrowing versus retail's x87 extended precision, same class as AD-33; AD-65 RETIRED and AD-69 FILED 2026-08-07 at Campaign S S4 — the away-arm now snaps per retail @0x00509c50, while AD-66's byte-confirmed sibling landing is WITHHELD pending #341's measurement-anomaly apparatus, and AD-69 records the seam-frame dist gap the same pass discovered; AD-56 RESTORED 2026-08-07 — the a8a7d64b revert had collaterally DELETED it, the inverse of the AD-55 zombie it also created; its plumb-fall-freeze condition is live again since TS-4’s real retirement at Slice 2B; AD-55 RE-RETIRED 2026-08-07 — its 2026-07-30 retirement at 252e8068 was collaterally resurrected by the a8a7d64b revert of the unrelated TS-4 commit; the code kept the cos(10°) fix throughout; AD-68 filed 2026-08-07 at the #338 closure — the async-residency placeholder mover shape (0.4/0.4 steps + capsule) has no retail counterpart because retail loads synchronously; AD-67 filed 2026-08-07 at the #32 closeout — the narrowed `SetContactPlane` keeps its per-write `ContactPlaneCellId`, which retail writes only at `init_contact_plane`; AD-49 filed 2026-08-06 at the #334 fix — the BSP part-array flood runs its outdoor cell rectangle at seed time rather than only from retail’s residency-gated walk, keeping both registration floods on one residency rule; AD-64 filed 2026-08-05 at the C5b architecture review's D1 fix — AD-60's W2 wire-cell REACHABILITY decision is expressed once per host because the two hosts run parallel non-shared inbound routes; the committed VALUE is single-sourced at `RuntimeEntityObjectLifetime.CommitWireCellRebucket`, and unification is filed as #324; AD-60 CORRECTED the same day — its surviving-channel enumeration presented "the local force path, the missile arm" as exhaustive when the entire no-window host belonged in it; AD-1 RETIRED 2026-08-05, C5a deletion sweep — the legacy outdoor demote/restore lift this row described was `PhysicsEngine.Resolve`'s own body, deleted with zero production callers; AD-42 DELETED 2026-08-04, C4 route 3 — its last surviving citation, the headless portal-arrival resync's two-call Resolve/ResolvePlacement split, was retired by the canonical `RuntimeAcceptedPositionDriveController` portal arm; AD-2 amended same route with the deferred-place timing adaptation, the T8 tolerated-overwrite note, and the leash-anchor nuance; AD-63 filed 2026-08-04, cancelled-park presentation rollback — the rollback restores every presentation registration the park's Withdraw removed EXCEPT the player's selection, which is user intent rather than a projection; AD-62 filed 2026-08-03, C4 route 2 round 2 — a deferred ForcePosition retired without committing is not re-applied and its ack is not sent; AD-61 filed 2026-08-02, C3c review round 1 — the #270 settle compression now covers the local player; AD-59/AD-60 filed 2026-08-02, continuation-executor slice) Recent retirements: AD-3/AD-4 retired 2026-07-31 by exact active/per-candidate visible-cell availability, full-catalog containment-root validation, and the @@ -166,6 +166,7 @@ readiness/requeue adaptation. See | AD-69 | **Filed 2026-08-07 at the S4 pseudocode pass (implementer finding, verified against the decomp).** `Transition.AdjustOffset`'s safety push-out computes `dist` WITHOUT the cell-relative correction retail applies: retail's `adjust_offset` (and ACE's port, independently) run the sphere centre through `LandDefs::get_block_offset` against the contact plane's own cell before the plane-distance dot, so a contact plane owned by a DIFFERENT landblock than the mover's current cell measures in the plane's frame. acdream dots the raw world-space centre against the stored plane. Same-landblock contact (the overwhelming case) is identical; a landblock-SEAM contact measures dist offset by the block delta, mis-firing or mis-suppressing the push-out at seams. | `src/AcDream.Core/Physics/TransitionTypes.cs` (`AdjustOffset`, the dist computation ahead of the push-out block) | Discovered during S4 but deliberately not folded in: S4's own AD-66 half was withheld the same night (#341), and a third change in the same block would have made the anomaly investigation unattributable. Fix alongside the AD-66 relanding. | A mover resting on a contact plane owned by the neighbouring landblock (seam walking) gets a push-out computed against a dist that is wrong by the block offset — either a spurious lift or a missed penetration correction, exactly at landblock seams, the #176/#177 symptom neighbourhood. | `CTransition::adjust_offset` 0x0050a370 (pc:272271-272393); `LandDefs::get_block_offset`; ACE `Transition.AdjustOffset` (cross-check); issue #341 (sequencing) | | ~~AD-70~~ | **RETIRED 2026-08-08 (same day, round-2 cdb capture): the row described retail behavior, not a divergence.** Retail's glide alternates exactly as ours does — the capture measured ~1.5 edge_slide entries per find_transitional_position during the glide (the alternation's exact signature: 3 on the arming tick, 0 on the moving tick), lockstep cliff_slide, step_down at 2.5x, and identical stack paths; cliff_slide's bytes match our port and ACE's. The 'retail redirects within the tick' inference misread round-1's set_sliding_normal cadence (per-event, not per-tick). Issue #347 closed without a code change. **Filed 2026-08-08 with the #345 fix.** Our steep-slope glide alternates: the edge-family arming tick absorbs the request (zero yield) and only the next tick's `AdjustOffset` pre-projection moves, then the clean move clears the sliding normal — a strict two-tick cycle. Retail redirects WITHIN the tick (`edge_slide`/`cliff_slide` 594 each over a ~15 s live glide — every 30 Hz tick, lockstep with `set_sliding_normal` 538) and yields motion every tick. | `src/AcDream.Core/Physics/TransitionTypes.cs` (`EdgeSlideAfterStepDownFailed` + the insert's post-constraint continuation) | The #345 landing deliberately touched only `validate_walkable`'s return scoping; the response bodies were freshly user-gated (Campaign S) and AD-66 had just relanded in the same block. | Gliding along a too-steep face at ~half retail's lateral speed; direction and angle-scaling correct. Visible as "slides but slower than retail" in a side-by-side. | `345-retail-glide.cdb.log` counters; `Issue345SteepSlopeGlideTests` tick trace; issue #347 | | AD-71 | **Filed 2026-08-08 (reviewer finding on the #345 fix).** `ValidateWalkable`'s walkable test uses the MUTABLE `sp.WalkableAllowance` where retail's `validate_walkable` calls `CPhysicsObj::is_valid_walkable` @0x0050f530 — a FIXED global threshold (N.z >= [0x8ede5c], the walkable constant; the function reads no object state). Several code paths write `WalkableAllowance = LandingZ` (0.0871557 — TransitionTypes.cs:1688,2264, BSPQuery.cs:2330, FlatBspQuery.cs:2085) and `ClearWalkable()` does not restore it, so a stale-permissive value entering a grounded `!StepDown && OnWalkable` validate makes the guard PASS where retail's fails. Every override is permissive, so the #345 fix cannot REGRESS through this path — but for planes with N.z in (0.0872, 0.6642) a stale allowance leaves the old Adjusted-without-push dead loop reachable. The #345 landing GREW this row's blast radius: the operand now gates the return value (OK vs Adjusted), not merely the push (reviewer B, 2026-08-08). Also folds in: our `FloorZ = 0.6642f` vs ACE's 0.66417414f flips OK/Adjusted in a ~0.002-degree band. | `src/AcDream.Core/Physics/TransitionTypes.cs` (`ValidateWalkable`, the `walkable` guard operand) | Deliberately not folded into the #345 landing: the allowance plumbing is shared with the step-down family and needs its own conformance pass over every WalkableAllowance write/restore site. | A too-steep plane between LandingZ and FloorZ validated right after a placement/landing path that left the allowance permissive: the guard pushes+Adjusts where retail returns OK — the #345 stop, in a narrower band. | capstone decode of 0x0050f530 (reviewer A, 2026-08-08); `docs/research/2026-08-08-345-d0-branch-pin.md` flagged-secondary section | +| AD-72 | **Filed 2026-08-07, Slice 5.3 review corrections (fix 6).** `VendorPricing.BuyPrice`/`SellPrice` compute `rate * perUnitValue * quantity` at C# `double` (64-bit); retail's `ShopSystem::BuyPrice`/`SellPrice` (`0x006B6120`/`0x006B6180`) run the same multiply at x87 `long double` (80-bit extended) — the same narrowing class AD-33 already recorded for `CSequence.FrameNumber`. | `src/AcDream.Core/Items/VendorPricing.cs` (`BuyPrice`/`SellPrice`, the `double raw = (double)rate * perUnitValue * quantity;` line) | `double` is the widest floating-point type available in C# (no 80-bit extended type exists in .NET). The port keeps retail's literal `± 0.1` margin ahead of the floor()/ceil() (see the type's own doc comment) — many orders of magnitude larger than any float/double precision gap at realistic AC item-value magnitudes (rate/value/quantity products in the tens-of-thousands range at most), so the margin absorbs the narrowing before it can move the floor()/ceil() result. | A price computed at a pathological value/rate/quantity combination landing within a double-ULP of the 0.1 margin boundary could floor/ceil to a different integer than retail's 80-bit compute would. No known installed vendor data approaches this boundary. | `ShopSystem::BuyPrice`/`SellPrice` `docs/research/named-retail/acclient_2013_pseudo_c.txt:702082-702128`, `0x006B6120`/`0x006B6180`; AD-33 (same narrowing class, `CSequence.FrameNumber`) | --- @@ -177,7 +178,7 @@ AP-94..AP-112 for the confirmed retail-UI completion gaps. | # | Divergence | Where (file:line) | Why it is safe / justified | Risk if assumption breaks | Retail oracle | |---|---|---|---|---|---| -| AP-160 | **Filed 2026-08-07, Slice 5.3 (vendor browse lifecycle).** 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()`. | `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. | 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 this row requires a Runtime-owned per-entity collision radius/height source, which does not exist today. | `CPlayerSystem::RegisterObjectRangeHandler` pc:203677/0x004C4C34; `gmVendorUI::OnObjectRangeExit` pc:199486/0x004C02F0; ACE `Vendor.CheckClose`/`GetCylinderDistance` (`references/ACE/Source/ACE.Server/WorldObjects/Vendor.cs:322-367`, `WorldObject_Use.cs:50,57`); `docs/research/2026-08-08-slice5-vendor-browse-research.md` §A.3/§B.1/§B.2 | +| 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()`. | `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 | | AP-141 | **Filed 2026-08-04, C4 route 5 (projectile authoritative placement); NARROWED 2026-08-04 at the round-2 delta review (B1/B2) — the far-branch clause was factually wrong for the adopted-body case and is corrected below.** Three related projectile-only shapes, all pinned by design (D-P4) rather than ported: (a) the near-`Interpolate` disposition is a NO-OP for a live missile, where retail would lazily build interpolation machinery (`InterpolateTo` @0x005163AF) for it; (b) the post-operation `ConstrainTo` @0x00454272 (`MakePositionManager` @0x00510523 then `PositionManager::ConstrainTo`) is never ARMED for a projectile — retail's single arming site has no kind test, so retail WOULD build a `PositionManager` on demand and arm a missile's leash on any nonzero `MoveOrTeleport` return; acdream never arms it on any disposition, including the adopted-body case (whose PRE-EXISTING leash the teleport/far branches now un-arm or clear queue state for, but never RE-anchor, per retail's post-operation `ConstrainTo`); (c) a null-classified or `Rejected*` accepted Position for a missile is swallowed (write nothing) rather than caught up through any remote-shaped policy. | `src/AcDream.Runtime/Session/RuntimeRemotePlacementDriveController.cs` (`ApplyAcceptedProjectilePosition`) | acdream deliberately does not construct an `EntityPhysicsHost`/`PositionManager`/`InterpolationManager` chain for a ballistic body — the route-5b split the C4 route 5 contract rejected. The context that makes this safe rather than merely convenient: ACE never sends `UpdatePosition` for a missile (`references/ACE/Source/ACE.Server/WorldObjects/WorldObject_Tick.cs:333-334`, `SendUpdatePosition()` commented out inside the `PhysicsState.Missile` branch at `:265`) — every half of this row is deterministic-test-gated only, never exercised against a real server. **The far branch's `StopInterpolating` skip is retail-faithful ONLY for a BARE missile** (no `RemoteMotion` — retail's own `position_manager != 0` guard @0x005163C9 skips it for a never-interpolated object, so acdream's skip is faithful by consequence there). For the ADOPTED-BODY case (`TryBind`'s shared-body branch: an ordinary remote whose Missile bit was set by a later State packet, still carrying its `RemoteMotion`), retail's guard IS satisfied and retail WOULD clear the queue — acdream now ports this (`route.StopInterpolating && record.RemoteMotion is RemoteMotion adopted → adopted.Interp.Clear()`), matching the teleport branch's equivalent `StopInterpolating` action inside `teleport_hook`. What remains divergent for the adopted case is the post-operation `ConstrainTo` re-anchor @0x00454272 — retail re-anchors an existing leash at the just-updated position on every nonzero return; acdream never arms/re-anchors it on any projectile disposition (clause (b)). | A future change that DOES give projectiles a `PositionManager` (or a headless/no-window remote-motion consumer that expects one) must re-decide this row rather than silently building the machinery ad hoc; until then, a live missile never shows an ARMED constraint leash and never catches up via the near/UnroutedCatchUp policy — both unreachable in play. An adopted-body missile's INHERITED leash (armed before it became a missile) is un-armed by the teleport hook, has its queue cleared by both teleport and far, but is never re-anchored at the new position by either — its brake accumulator (`ConstraintPosOffset`) is not reset to zero at each accepted Position the way retail's @0x00454272 re-anchor does. **Correction, round 3 (2026-08-04): the round-2 wording here — that a stale leash "would drag the body toward a stale anchor" — was wrong and is retracted.** `ConstraintManager.ConstraintPos` is write-only in both retail and the port (never read by `AdjustOffset`), and `ConstraintManager::adjust_offset` @0x00556180 only tapers or zeroes an already-composed per-tick offset while `InContact` — a leash brakes motion the interp/sticky chain already produced; it has no mechanism to move anything toward the anchor. The real residual is confined to one tick of un-reset brake accumulator, contact-gated, and it cannot move an airborne far-snapped missile at all (the clamp branch does not run while airborne). | `CPhysicsObj::MoveOrTeleport` 0x00516330 (`InterpolateTo` @0x005163AF, `IsMovingTo` @0x0050EB10 returning 0 without a `MovementManager`; far branch `StopInterpolating` @0x005163C9-@0x005163CB); `SmartBox::HandleReceivedPosition` 0x00453FD0 (`ConstrainTo` arming site @0x00454272); `CPhysicsObj::ConstrainTo` 0x00510520 (`MakePositionManager` @0x00510523); `ConstraintManager::adjust_offset` 0x00556180 (brake-only taper, write-only anchor); `WorldObject_Tick.cs:333-334`/`:265` (ACE never-sends evidence) | | AP-142 | **Filed 2026-08-04 (C4 route 7, pickup/parent/delete). AMENDED 2026-08-04 at the dual-Opus retail-conformance/architecture review round (R1/A8 MAJOR+LOW; R10 MINOR) — clause (d) added, clause (b) corrected. AMENDED AGAIN 2026-08-04 at the round-3 dual review (N1/N2/N4, B3) — clause (d)'s reasoning corrected and its risk-column scope widened; clause (e) RETIRED — the depth cap it described is deleted outright, replaced by an iterative worklist with no depth concept at all. AMENDED AGAIN 2026-08-05 at the #319 fix — clause (f) added. AMENDED AGAIN 2026-08-05 at the #319 dual-review round (retail PASS, architecture FAIL/6 MAJORs) — clause (f) rewritten: the tripwire moved above the canonical commit and no longer throws (A1), and the deferred late-bind queue A1's fix text originally described was deleted per A6 (both reviews proved it production-unreachable for both producers).** acdream collapses retail's `CPhysicsObj` pair — a `cell` pointer plus a separately-written `objcell_id` — into ONE canonical `RuntimeEntityRecord.FullCellId`, which is also the residency/liveness predicate acdream reads at 45+ sites. Four consequences, all intentional: (a) the removal path propagates ZERO to a subtree's children (withdrawal, delete, `EndGeneration`), where retail's `leave_cell` recursion nulls only each child's `cell` pointer and leaves a STALE non-zero `objcell_id` (`change_cell`'s removal tail @0x005133C1 never touches a child's id) — reproducing that stale-id residue would leave a child "resident" per every acdream predicate while retail's own gating field (`cell == nullptr`) says it is not; (b) retail's same-cell depth-1 per-tick `objcell_id` refresh (`SetPositionInternal` @0x0051539c-@0x005153d8, gated on the parent NOT crossing a cell) is subsumed by the value-idempotent propagation chokepoint (`RuntimeEntityDirectory.SetFullCell`'s "skip a child whose `FullCellId` already equals the target" guard) rather than ported as a separate tick loop — a same-value restamp is unobservable with one field playing both retail roles. **Correction (R10): this is a clean equivalence only on the REMOVAL side.** The skip ALSO prunes the child's whole subtree on a same-value WRITE, which retail's `enter_cell` does not do — it recurses over children unconditionally (@0x00510f03); only `leave_cell` prunes (@0x00510f5b, on `cell != 0`). Currently unreachable-by-construction (after D4 nothing writes a grandchild's cell independently of its own committed parent), but it is an asymmetry, not a proven equivalence; (c) the sustaining propagation itself: retail re-cells children when the parent crosses a cell, recursively, on EVERY `SetPositionInternal`/`change_cell` (@0x00515372/@0x00513390), not only at attach — acdream ports this as a single hook every canonical cell-write funnels through, so an attach-only write (the pre-existing shape) is deliberately NOT what shipped. **(d) retail's `enter_cell` gates its ENTIRE body — the write AND the recursion into children — on `this->part_array != 0` (@0x00510ed8); a child with a null part array receives nothing and its whole subtree is skipped. acdream's propagation has NO analogue and writes unconditionally. CORRECTED reasoning (round-3 review, N1/N2): the original draft of this clause argued acdream's `HasPartArray` means something semantically different from retail's `part_array` (a "renderer built a mesh" flag vs. "this CPhysicsObj has any part array"). That framing is WRONG — retail's `part_array` has exactly ONE assignment site, `CPhysicsObj::makeAnimObject` @0x0050e930 → `CPartArray::CreateSetup`, assigned @0x0050e94d, so retail's flag is ALSO a mesh-construction product; the two are near-synonyms, not different concepts. The REAL reason acdream cannot gate the canonical D1/D2 write on `HasPartArray` is LAYERING, not semantics: Slice J made the Runtime canonical layer presentation-independent by design (`docs/research/2026-07-25-slice-j1-runtime-contract-closeout.md` and the Slice J campaign generally), and `HasPartArray` is populated exclusively by App/graphical code (`EquippedChildRenderController.cs:609`, `DatLiveEntityProjectionMaterializer.cs:203`) — the canonical layer structurally cannot depend on a flag only the presentation layer ever writes, headless or not. CORRECTED scope (round-3 review): this is NOT headless-only. `PrepareAndTryRealize` calls `CommitAcceptedParentCellless` (hence D1's re-cell) BEFORE `TryRealize` sets `HasPartArray = true` at `:609` — so at the exact moment D1 runs, `child.HasPartArray` is FALSE in the GRAPHICAL host too, and gating on it would break attach there as well, not just headless. Retail has no equivalent window at all: `part_array` is assigned once at construction and `enter_cell`'s guard reads that same, already-settled field.** The guard is deliberately NOT reproduced at the canonical layer. **(e) RETIRED 2026-08-04 (round-3 review, N4/B3 — both reviews independently found the same defect).** Previously: recursion depth capped at 64 levels as hostile/buggy-server hardening. The cap's actual failure mode was worse than what it guarded against: a subtree beyond the cap was left at its PRIOR — on the withdraw path, STALE NONZERO — cell PERMANENTLY, logged only under a probe flag nobody runs by default. On the withdraw path that is the #184 shape verbatim: an entity every acdream residency predicate calls resident that retail (and clause (a) above) says is not. Shipping that inside the slice whose headline is fixing exactly this class was unacceptable. Retired by deleting the cap outright and replacing the recursion with an iterative worklist (`RuntimeEntityDirectory._propagationWorklist`), which has no stack-frame-bounded depth at all — the only limit is the number of committed relations actually in the system, matching retail's own genuinely unbounded recursion with no acdream-only cap and therefore no register row for one. **(f) Filed 2026-08-05 (#319 fix).** A CreateObject-carried parent relation (the raw spawn's `Physics.Parent` field, and the same-generation `CreateParentUpdate` envelope) names the parent's GUID and location only — neither wire shape carries a parent instance sequence, matching retail's own GUID-only attach (`PhysicsDesc::get_parent_id` @0x00558a18 → `CObjectMaint::GetObjectA` @0x00558a2d → `CPhysicsObj::set_parent` @0x00558a3e; the reverse `CObjectMaint::SetChildren` @0x00509370 hash-walks by guid with a `GetNullObject` placeholder @0x005093e6 — no instance-sequence field or comparison exists anywhere in either direction). acdream's committed-relation table is nonetheless keyed by (guid, incarnation) (clause (c)'s D1/D2 requirement), so a CreateObject-carried relation must adopt SOME incarnation to file under; it now LATE-BINDS to the parent's LIVE incarnation at accept time (`EquippedChildRenderController.AcceptLateBoundCreateObjectRelation`, both the raw-CreateObject and same-generation `CreateParentUpdate` producers) rather than the previously-hardcoded 0, which silently mis-keyed every player-parented CreateObject relation (a player's `ObjectInstance` is `Character.TotalLogins`, never 0) and defeated D1/D2 for the local player's own login equipment and every remote player's observed equipment (#319). A commit-time tripwire (`ParentAttachmentState.CanCommitIncarnation`, checked BEFORE either half of the commit mutates state — architecture review A1, 2026-08-05, moved it there after the original throw-after-canonical-commit shape was shown to tear the transaction it was built to protect) refuses (logs, returns false, never throws) rather than silently filing a relation under a mismatched incarnation whenever the parent is currently addressable. **A1 also settled A6's design question**: an initial revision queued a relation whose parent was not yet addressable through a deferred/late-bind retry mechanism; both reviews independently proved that queue was structurally unreachable in production for BOTH producers (`RuntimeEntityObjectLifetime.RegisterEntityCore`'s `EnqueueDeferredCreate` gate defers the ENTIRE CreateObject, for both wire shapes, before either producer ever runs) while carrying three latent defects of its own (a missing child POSITION_TS gate, a placeholder-incarnation collision with the generation filters, unbounded accumulation) — it was deleted rather than fixed in place; the unaddressable-parent case now logs and refuses outright, matching the invariant the layer above already enforces. | `src/AcDream.Runtime/Entities/RuntimeEntityDirectory.cs` (`SetFullCell`, `PropagateFullCellToChildren`, `RefreshSnapshot`); `src/AcDream.Runtime/Entities/RuntimeEntityObjectLifetime.cs` (`CommitAcceptedParentCellless`'s D1 half, `WithdrawCommittedChildrenToCellless`); `src/AcDream.Runtime/Entities/ParentAttachmentState.cs` (`TryGetCommittedParent`, `CanCommitIncarnation`, `CommitProjection`); `src/AcDream.Runtime/Entities/RuntimeEntityRecord.cs` (`HasPartArray`); `src/AcDream.App/Rendering/EquippedChildRenderController.cs` (`AcceptLateBoundCreateObjectRelation`, `OnSpawn`, `OnCreateParentAccepted`, `PrepareAndTryRealize`); `src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs` (`ResolveAndCommitChildAttachment`) | Reproducing retail's pointer/id split would require a second field acdream's 45+ liveness call sites would then have to be individually audited for which half they mean — the single-field model is a stated, load-bearing simplification, not an oversight; see `docs/research/2026-08-04-retail-parent-cell-propagation.md` and `docs/research/2026-08-04-c4-route-7-contract.md` D2/D3/D9. Clause (f) is retail-faithful for the identical reason clauses (a)-(d) are: retail's attach has no incarnation gate on this path at all, so adopting the current holder of the guid IS the retail behavior, not an approximation of it. | A future consumer that expects retail's exact stale-`objcell_id`-under-a-null-`cell` shape (none identified) would see a fully cell-less child instead. (d)'s risk: acdream celling a child retail would leave nowhere — none identified in play against a well-behaved ACE, since a server-authored equip always names a real, DAT-resolvable Setup, and the graphical host's own brief pre-`TryRealize` window is bridged by D1 running inside the same synchronous transaction as the rest of the attach commit, not by `HasPartArray` being true. (f)'s risk: none identified against a well-behaved ACE — a CreateObject's parent guid always names the entity that currently holds it by construction. | `CPhysicsObj::change_cell` 0x00513390 (@0x005133C1 removal tail); `CPhysicsObj::enter_cell` 0x00510ed0 (@0x00510ed8 the `part_array` guard); `CPhysicsObj::leave_cell` 0x00510f50; `CPhysicsObj::SetPositionInternal` 0x00515330 (@0x0051536d branch, @0x0051539c-@0x005153d8 same-cell loop, @0x00515372 cell-change branch); `CPhysicsObj::makeAnimObject` 0x0050e930 (`CPartArray::CreateSetup` assignment @0x0050e94d); `PhysicsDesc::get_parent_id` 0x00558a18; `CObjectMaint::GetObjectA` 0x00558a2d; `CPhysicsObj::set_parent` 0x00558a3e; `CObjectMaint::SetChildren` 0x00509370 (`GetNullObject` placeholder @0x005093e6) | | AP-143 | **Filed 2026-08-04 (C4 route 7 D5, headless parent-realize drive). AMENDED 2026-08-04 at the retail-conformance review round (R7 MINOR) — this row originally described only ONE of the three checks the drive skips. Line citations corrected at the round-3 review (N3).** The graphical `EquippedChildRenderController.ValidateParentProjection` performs three retail-anchored checks before accepting a parent-attach request: (1) self-parenting rejection (`relation.ParentGuid == relation.ChildGuid`, `:915-916`); (2) the parent must have a constructed part array (`parent.HasPartArray`, `:920` — the closest acdream analogue to retail's `part_array != 0` guard, AP-142 clause d); (3) `Setup.HoldingLocations` validates the specific holding location (`CSetup::GetHoldingLocation` @0x0050F896, via `PartArray::add_child`). `AcDream.Headless`/`AcDream.Runtime`'s direct-host parent-realize drive (`RuntimeLiveEntitySessionController.ResolveAndCommitChildAttachment`) performs NONE of the three — it commits on the POSITION_TS gate acceptance and relation resolution alone. (1) is inert by construction: D1's re-cell gate reads `parent.FullCellId == 0` (the child was just zeroed by the cell-less edge before D1 runs), and D2's skip-on-equal terminates the resulting one-node cycle — a self-parent headless commits the relation but never observably re-cells through it. (2) has no headless analogue at all (see AP-142 clause d — `HasPartArray` is populated only by the graphical mesh pipeline, never headless, for ANY entity). (3) has no prepared-content surface (repo-wide grep confirms nothing under `src/AcDream.Content`/`AcDream.Bake` carries `Setup.HoldingLocations`). | `src/AcDream.Runtime/Session/RuntimeLiveEntitySessionController.cs` (`ResolveAndCommitChildAttachment`) | Precedent: the content-less host already accepts reduced fidelity elsewhere (`RuntimeLiveEntitySessionController:108-117`'s documented content-less registration). A server-sent self-parent, part-array-less parent, or invalid holding location is unreachable against a well-behaved ACE (ACE only emits `ParentEvent` for a location its own `Player_Inventory`/wield validation already accepted), so this is a defense-in-depth gap, not a live-play one. | A malicious or buggy server could attach a child headless where retail and the graphical host would both reject it — inert against ACE today for all three. Retiring (3) means extending the prepared-content bake format with `Setup.HoldingLocations`, deliberately NOT done in this slice (route 7 contract §4 D5); (2) has no retiring action available until acdream's canonical layer gains its own construction-time part-array concept (a larger architectural question, out of scope here). | `PartArray::add_child` (`CSetup::GetHoldingLocation` 0x0050F896); `CPhysicsObj::enter_cell` 0x00510ed8 (the `part_array` guard); `EquippedChildRenderController.ValidateParentProjection` (graphical port, all three checks) | diff --git a/src/AcDream.Core.Net/GameEventWiring.cs b/src/AcDream.Core.Net/GameEventWiring.cs index 281c5762..ab4f9dab 100644 --- a/src/AcDream.Core.Net/GameEventWiring.cs +++ b/src/AcDream.Core.Net/GameEventWiring.cs @@ -418,7 +418,13 @@ public static class GameEventWiring item.Desc.Name, item.Desc.ItemType, item.Desc.IconId, - item.Desc.Value); + item.Desc.Value, + // Slice 5.3 review fix 2: the DESC's own StackSize (NOT + // item.StackSize above, ItemProfile's separate packed + // supply-count field) -- VendorPricing.PerUnitValue's + // divisor for turning Value's stack-total wire number + // into a per-unit display price. + item.Desc.StackSize); } vendor?.Apply(p.Value.VendorGuid, profile, shopItems); diff --git a/src/AcDream.Core/Items/VendorPricing.cs b/src/AcDream.Core/Items/VendorPricing.cs index e880416e..17368321 100644 --- a/src/AcDream.Core/Items/VendorPricing.cs +++ b/src/AcDream.Core/Items/VendorPricing.cs @@ -49,6 +49,45 @@ namespace AcDream.Core.Items; /// public static class VendorPricing { + /// + /// VendorProfile::VendorSellPrice/VendorBuyPrice's shared + /// per-unit division (0x005D1B00/0x005D1B70, both bodies + /// identical apart from which rate/branch they go on to feed + /// / — read directly from + /// the decompiled body, docs/research/named-retail/acclient_2013_pseudo_c.txt:484801-484813). + /// The wire item's Value field is the STACK's TOTAL value, not a + /// per-unit price — retail divides it by the item's own authored + /// PublicWeenieDesc::_stackSize ( + /// — NOT , which is + /// ItemProfile's separately-packed SUPPLY count, a different wire + /// field entirely) before either price formula ever sees it. Slice 5.3 + /// review fix 2. + /// + /// + /// The item's raw wire Value (retail _value — the whole + /// stack's total value, not one unit's). + /// + /// + /// The item's own PublicWeenieDesc::_stackSize (retail + /// _stackSize). (the field absent on the + /// wire) is treated exactly like retail's zeroed-struct default when the + /// field was never sent — same as an explicit 0 or negative: no + /// division. + /// + /// + /// unchanged when + /// is absent or <= 0 (retail + /// pc:484808-484810: if (_stackSize <= 0) return + /// SellPrice(_value, ...)); otherwise the INTEGER-divided per-unit + /// value (retail pc:484812: COMBINE(0, _value) / _stackSize + /// — a plain non-negative integer divide, .NET's int division + /// truncates toward zero the same way). + /// + public static int PerUnitValue(int stackTotalValue, int? descStackSize) => + descStackSize is { } size && size > 0 + ? stackTotalValue / size + : stackTotalValue; + /// /// ShopSystem::BuyPrice (0x006B6120): the price the vendor /// PAYS the player for units of an item diff --git a/src/AcDream.Core/Items/VendorState.cs b/src/AcDream.Core/Items/VendorState.cs index e0a1cd3e..a60e04cc 100644 --- a/src/AcDream.Core/Items/VendorState.cs +++ b/src/AcDream.Core/Items/VendorState.cs @@ -44,7 +44,20 @@ public readonly record struct VendorShopItem( string? Name, uint? ItemType, uint IconId, - int? Value); + int? Value, + // Slice 5.3 review fix 2: the ITEM'S OWN authored stack depth (retail + // PublicWeenieDesc::_stackSize, wire AcDream.Core.Net.Messages. + // PublicWeenieDescBody.StackSize) -- the divisor VendorPricing.PerUnitValue + // needs to turn Value's STACK-TOTAL wire number into a per-unit display + // price (VendorProfile::VendorSellPrice/VendorBuyPrice, 0x005D1B00/ + // 0x005D1B70). This is a DIFFERENT wire field from StackSize above: + // that one is ItemProfile's packed SUPPLY count (how many the vendor has + // in stock), this one is how many units make up one priced stack (e.g. + // 50 for a stack of arrows). Nullable because the wire field is + // conditionally present (weenieFlags-gated) -- absent maps to null here, + // matching retail's own zeroed-struct default of 0 for the same case + // (see VendorPricing.PerUnitValue's <= 0 guard). + int? DescStackSize = null); public enum VendorStateTransitionKind { @@ -134,6 +147,31 @@ public sealed class VendorState /// different-vendor open superseding this one; see research doc §A.3). /// Returns false if no vendor was open. /// + /// + /// Slice 5.3 review fix 3. Unlike , a failing + /// observer here is never rethrown. Close()'s + /// production caller (RuntimeVendorRangeQuery.EnforceRange) runs + /// inside the per-frame post-network-command-phase callback + /// (GameRuntime.CreateLocalPlayerFrameController's post-network + /// phase) with no try/catch anywhere up the frame-loop chain — an + /// propagating out of here, 's + /// shape, would kill the frame. keeps that + /// collect-and-rethrow shape because ITS callers (session + /// reset/portal-out/logout — a rare, explicit teardown boundary) already + /// tolerate/handle it (e.g. RuntimeInventoryState.Dispose's own + /// Try(...) wrapper collects 's failures + /// alongside every other child's). This still fans out to every listener + /// via GetInvocationList() (one broken observer must not starve + /// another — same resilience as ), but LOGS each + /// failure instead of collecting it into an exception, matching + /// GameEventDispatcher.Dispatch's own boundary contract + /// (src/AcDream.Core.Net/Messages/GameEventDispatcher.cs:95-117 — + /// catch, Console.Error.WriteLine, never rethrow, "the decode + /// thread must survive handler failures"): a per-frame boundary must + /// survive its own observers' failures the same way. Not silent + /// swallowing — the failure surfaces on + /// exactly the way the dispatcher's do. + /// public bool Close() { if (VendorId == 0u) return false; @@ -141,7 +179,20 @@ public sealed class VendorState uint previous = VendorId; ClearFields(); - Changed?.Invoke(new VendorTransition(VendorStateTransitionKind.Closed, previous, 0u)); + var transition = new VendorTransition(VendorStateTransitionKind.Closed, previous, 0u); + Action? listeners = Changed; + if (listeners is not null) + { + foreach (Action listener in listeners.GetInvocationList()) + { + try { listener(transition); } + catch (Exception error) + { + Console.Error.WriteLine( + $"[VendorState] Close() observer threw: {error.Message}"); + } + } + } return true; } diff --git a/src/AcDream.Runtime/Gameplay/RuntimeVendorRangeQuery.cs b/src/AcDream.Runtime/Gameplay/RuntimeVendorRangeQuery.cs index bb1e6680..cf19c932 100644 --- a/src/AcDream.Runtime/Gameplay/RuntimeVendorRangeQuery.cs +++ b/src/AcDream.Runtime/Gameplay/RuntimeVendorRangeQuery.cs @@ -3,6 +3,7 @@ using AcDream.Core.Items; using AcDream.Core.Net.Messages; using AcDream.Core.Physics; using AcDream.Runtime.Entities; +using AcDream.Runtime.World; namespace AcDream.Runtime.Gameplay; @@ -18,22 +19,72 @@ namespace AcDream.Runtime.Gameplay; /// ACE's server-side belt-and-suspenders equivalent, Vendor.CheckClose /// (references/ACE/Source/ACE.Server/WorldObjects/Vendor.cs:322-367), /// polls every 1.5 s and closes when GetCylinderDistance(lastPlayer) > -/// UseRadius, falling back to wo.UseRadius ?? 0.6f when the vendor -/// carries no explicit radius (WorldObject_Use.cs:50,57). +/// UseRadius — a plain nullable-float comparison with NO fallback +/// default of its own (UseRadius is float?, +/// WorldObject_Properties.cs:1242-1246; a nullable comparison against +/// a null right operand is always false, so CheckClose never +/// closes when the vendor carries no explicit radius). The ?? 0.6f +/// fallback this class's earlier revision mis-attributed to +/// Vendor.CheckClose actually lives in a DIFFERENT method, +/// WorldObject.IsWithinUseRadiusOf (WorldObject_Use.cs:50,57) — +/// the APPROACH check ("how close you need to be to open the shop"), never +/// the close/distance-watch path. Slice 5.3 review fix 4. /// public static class RuntimeVendorRangeQuery { - /// ACE WorldObject_Use.cs:50: wo.UseRadius ?? 0.6f. - private const float DefaultUseRadius = 0.6f; - /// /// Close the open vendor session (if any) once the local player has - /// moved beyond the vendor's own UseRadius. No-op when no vendor is - /// open, or when either side's live position cannot be resolved this - /// tick (matches the existing App-layer convention at + /// moved beyond the vendor's own UseRadius, once the vendor entity + /// itself is no longer resolvable, or once an in-session portal/ + /// teleport has begun. No-op only when no vendor is open, or when the + /// PLAYER's own live position cannot be resolved this tick (matches the + /// existing App-layer convention at /// WorldSelectionQuery.IsWithinExternalContainerUseRange: "the /// server remains authoritative while render projection is absent" — - /// never force-close on missing data). + /// never force-close on missing PLAYER data). A missing/retired VENDOR + /// is a different case — see fix 1a below. + /// + /// + /// Fix 1b — in-session transit closes at BEGIN, not arrival (Slice + /// 5.3 review). is the + /// canonical Runtime owner of the F751/teleport lifecycle (J6.2/J6.3). + /// HasPendingTeleportStart flips true the instant + /// TryQueueTeleportStart succeeds — for a GRAPHICAL host this is + /// strictly BEFORE ActivateQueuedTeleport (and therefore before + /// IsTeleportActive), because + /// LocalPlayerTeleportController.TryActivatePendingPresentation + /// defers activation until the host can enter portal space; for a + /// HEADLESS host (RuntimeLiveEntitySessionController.OnTeleportStarted) + /// both flip in the same synchronous call. Checking BOTH flags here + /// observes "transit begin" through "transit still resolving" for + /// either host without a second call site: both hosts + /// (LocalPlayerTeleportController.OnTeleportStarted for + /// graphical, RuntimeLiveEntitySessionController.OnTeleportStarted + /// for headless) reach TryQueueTeleportStart as their sole entry + /// point, and this method already runs once per advanced frame for BOTH + /// hosts via the SAME post-network-command-phase callback + /// () that + /// already exists for the range check below — so this reuses that + /// existing per-frame seam instead of adding a new polling loop or event + /// channel. Unconditional: while a transit is in flight, distance is not + /// even evaluated, matching how a portal/teleport makes the player's and + /// vendor's positions momentarily incomparable (different landblock + /// frames — see register AP-160's extension). + /// + /// + /// + /// Fix 1a — a retired vendor entity closes the session (Slice 5.3 + /// review). Retail's own range watcher + /// (CPlayerSystem::RegisterObjectRangeHandler) dies with its + /// target — it has nothing left to watch once the vendor NPC is + /// retired (despawn, death, ObjectDelete during a recall). The prior + /// revision's early-return here on a failed TryGetActive was a + /// permissive default that stranded the session forever once the + /// vendor left the active entity set. A missing POSITION on a still- + /// active record gets the same treatment: a positionless vendor cannot + /// be range-checked, and retail's watcher likewise has nothing to + /// watch. + /// /// /// /// Distance metric divergence (register AP-160): retail/ACE close @@ -59,6 +110,16 @@ public static class RuntimeVendorRangeQuery if (vendorId == 0u) return; + // Fix 1b: close unconditionally the instant an in-session transit + // has begun — see the class doc above for why HasPendingTeleportStart + // is the earliest observable "transit begin" edge for both hosts. + RuntimeWorldTransitState transit = runtime.TransitOwner; + if (transit.HasPendingTeleportStart || transit.IsTeleportActive) + { + vendor.Close(); + return; + } + uint playerGuid = runtime.PlayerIdentity.ServerGuid; if (playerGuid == 0u || !runtime.EntityObjects.Entities.TryGetActive( @@ -69,15 +130,24 @@ public static class RuntimeVendorRangeQuery return; } + // Fix 1a: a retired vendor entity, or one left with no resolvable + // position, closes the session instead of stranding it open forever. if (!runtime.EntityObjects.Entities.TryGetActive( vendorId, out RuntimeEntityRecord vendorRecord) || vendorRecord.Snapshot.Position is not { } vendorPosition) { + vendor.Close(); return; } - float useRadius = vendorRecord.Snapshot.UseRadius ?? DefaultUseRadius; + // Fix 4: retail passes the vendor's raw authored UseRadius with NO + // client-side fallback (see the class doc's ACE citation correction) + // — an absent/zero radius closes on the very first nonzero-distance + // check, since ObjectRangeMath.ObjectsInRange with range=0 requires + // an EXACT position match. That is retail's own behavior for a + // radius-0 (or unauthored) handler, not a bug to paper over. + float useRadius = vendorRecord.Snapshot.UseRadius ?? 0f; bool inRange = ObjectRangeMath.ObjectsInRange( AbsolutePosition(playerPosition), 0f, diff --git a/tests/AcDream.Core.Net.Tests/Messages/VendorApproachTests.cs b/tests/AcDream.Core.Net.Tests/Messages/VendorApproachTests.cs index 6b9332f3..da5f9e21 100644 --- a/tests/AcDream.Core.Net.Tests/Messages/VendorApproachTests.cs +++ b/tests/AcDream.Core.Net.Tests/Messages/VendorApproachTests.cs @@ -157,8 +157,13 @@ public sealed class VendorApproachTests public void TryParse_TruncatedMidItemPrefix_ReturnsNull() { // The item's packed stack-size dword is present but its guid is - // cut off entirely — truncation inside the per-item PREFIX (before - // PublicWeenieDescParser is even reached) must fail the whole parse. + // cut off entirely. This does NOT reach a guid read that fails -- + // TryParse's own minimum-size guard (`(long)itemCount * 12 > + // payload.Length - pos`, the smallest possible per-item size: + // packed(4) + guid(4) + weenieFlags(4)) rejects the whole parse + // right after itemCount is read (remaining=4 bytes here, the packed + // dword only; 1 * 12 = 12 > 4), before the per-item loop that would + // read the packed dword/guid ever runs. var w = new AceWireWriter(); WriteMinimalProfilePrefix(w, vendorGuid: 0x40000500u); w.Write(1u); // item count = 1 diff --git a/tests/AcDream.Core.Tests/Items/VendorPricingTests.cs b/tests/AcDream.Core.Tests/Items/VendorPricingTests.cs index cd452fba..c9d9c5d0 100644 --- a/tests/AcDream.Core.Tests/Items/VendorPricingTests.cs +++ b/tests/AcDream.Core.Tests/Items/VendorPricingTests.cs @@ -127,4 +127,48 @@ public sealed class VendorPricingTests Assert.Equal(-1, VendorPricing.BuyPrice(-50, (uint)ItemType.Misc, 1.0f, 1)); Assert.Equal(-1, VendorPricing.SellPrice(-50, (uint)ItemType.Misc, 1.0f, 1)); } + + // ---- PerUnitValue (Slice 5.3 review fix 2) ----------------------------- + // VendorProfile::VendorSellPrice/VendorBuyPrice (0x005D1B00/0x005D1B70, + // pc:484801-484813): stackSize <= 0 ? value : value / stackSize + // (INTEGER division of the wire's stack-TOTAL value by the item's own + // authored PublicWeenieDesc.StackSize). + + // ---- 7. Stack of 50 arrows: the motivating case ------------------------ + // Wire Value=500 is the price for the WHOLE stack of 50 arrows; + // per-unit must equal the single-arrow price of 10. + [Fact] + public void StackOf50Arrows_DividesToThePerArrowValue() + { + Assert.Equal(10, VendorPricing.PerUnitValue(500, descStackSize: 50)); + } + + // ---- 8. descStackSize <= 0 guard ---------------------------------------- + // Zero and negative both take retail's "no division" branch — the + // stack-total value passes through unchanged. + [Fact] + public void DescStackSizeZeroOrNegative_ReturnsValueUnchanged() + { + Assert.Equal(250, VendorPricing.PerUnitValue(250, descStackSize: 0)); + Assert.Equal(250, VendorPricing.PerUnitValue(250, descStackSize: -1)); + } + + // ---- 9. descStackSize absent (null) -------------------------------------- + // A non-stackable item's wire PWD never carries a StackSize field at + // all; null must be treated exactly like retail's zeroed struct + // default (0) -- no division, value unchanged. + [Fact] + public void DescStackSizeAbsent_ReturnsValueUnchanged() + { + Assert.Equal(250, VendorPricing.PerUnitValue(250, descStackSize: null)); + } + + // ---- 10. Non-exact division truncates toward zero ----------------------- + // 100 / 3 = 33.33... -> retail's plain integer divide truncates to 33, + // same as .NET's int division. + [Fact] + public void NonExactDivision_TruncatesTowardZero() + { + Assert.Equal(33, VendorPricing.PerUnitValue(100, descStackSize: 3)); + } } diff --git a/tests/AcDream.Core.Tests/Items/VendorStateTests.cs b/tests/AcDream.Core.Tests/Items/VendorStateTests.cs index 3705662e..629fa1f5 100644 --- a/tests/AcDream.Core.Tests/Items/VendorStateTests.cs +++ b/tests/AcDream.Core.Tests/Items/VendorStateTests.cs @@ -120,6 +120,26 @@ public sealed class VendorStateTests Assert.Equal(0u, change.VendorId); } + [Fact] + public void Close_ThrowingObserver_DoesNotPropagateAndStillClosesTheSession() + { + // Slice 5.3 review fix 3: unlike Reset(), Close() must never + // rethrow — its production caller (RuntimeVendorRangeQuery. + // EnforceRange) runs inside an unprotected per-frame callback. + var state = new VendorState(); + state.Apply(0x40000007u, default, Array.Empty()); + + bool secondObserverRan = false; + state.Changed += _ => throw new InvalidOperationException("boom"); + state.Changed += _ => secondObserverRan = true; + + Exception? thrown = Record.Exception(() => { state.Close(); }); + + Assert.Null(thrown); + Assert.True(secondObserverRan); + Assert.Equal(0u, state.VendorId); + } + [Fact] public void Reset_RetryRepublishesAndOneObserverCannotStarveAnother() { diff --git a/tests/AcDream.Runtime.Tests/Gameplay/RuntimeVendorRangeQueryTests.cs b/tests/AcDream.Runtime.Tests/Gameplay/RuntimeVendorRangeQueryTests.cs index 32105e11..b3f1ac87 100644 --- a/tests/AcDream.Runtime.Tests/Gameplay/RuntimeVendorRangeQueryTests.cs +++ b/tests/AcDream.Runtime.Tests/Gameplay/RuntimeVendorRangeQueryTests.cs @@ -81,26 +81,136 @@ public sealed class RuntimeVendorRangeQueryTests } [Fact] - public void EnforceRange_VendorUseRadiusAbsent_FallsBackToTheAceDefault() + public void EnforceRange_VendorUseRadiusAbsent_UsesRawZeroWithNoFallback() { - // ACE WorldObject_Use.cs:50 — `wo.UseRadius ?? 0.6f`. + // Slice 5.3 review fix 4: retail passes the raw authored UseRadius + // with NO client-side fallback (ACE's 0.6f lives in the APPROACH + // check, WorldObject_Use.cs:50/57 — never in the close watcher, + // Vendor.CheckClose, which never closes at all on a null radius). + // acdream's own watcher must actively close, so an absent radius + // maps to the raw retail default of 0 (PublicWeenieDesc's _useRadius + // is memset, not sentineled) — any nonzero distance is then + // out-of-range. using GameRuntime runtime = Create(); runtime.PlayerIdentity.ServerGuid = Player; RuntimeEntityRecord playerRecord = Add(runtime, Player, Landblock, 100f, 100f); - Add(runtime, Vendor, Landblock, 100.5f, 100f, useRadius: null); + Add(runtime, Vendor, Landblock, 100f, 100f, useRadius: null); Open(runtime, Vendor); - // 0.5 m: inside the 0.6 m fallback. + // Exact same position: distance 0 <= radius 0 — still open. RuntimeVendorRangeQuery.EnforceRange(runtime); Assert.Equal(Vendor, runtime.InventoryOwner.Vendor.VendorId); - // Walk to 2 m: outside the 0.6 m fallback. - SetPosition(playerRecord, Landblock, 102.5f, 100f); + // Any nonzero move at all — even 5 cm — is out of range at radius 0. + SetPosition(playerRecord, Landblock, 100.05f, 100f); RuntimeVendorRangeQuery.EnforceRange(runtime); Assert.Equal(0u, runtime.InventoryOwner.Vendor.VendorId); } + [Fact] + public void EnforceRange_VendorEntityRetired_ClosesTheSession() + { + // Fix 1a: retail's own range watcher dies with its target. The + // permissive early-return this used to take on a failed + // TryGetActive stranded the session open forever once the vendor + // NPC despawned/died/was ObjectDeleted (e.g. during a recall). + using GameRuntime runtime = Create(); + runtime.PlayerIdentity.ServerGuid = Player; + Add(runtime, Player, Landblock, 100f, 100f); + RuntimeEntityRecord vendorRecord = + Add(runtime, Vendor, Landblock, 102f, 100f, useRadius: 3f); + Open(runtime, Vendor); + + // Directly removes the vendor from the active set — the same + // terminal state a despawn/death/ObjectDelete leaves behind + // (RuntimeEntityDirectory.RemoveActive is what TryAcceptDelete + // itself calls once a delete's InstanceSequence matches). + Assert.True(runtime.EntityObjects.Entities.RemoveActive(vendorRecord)); + + RuntimeVendorRangeQuery.EnforceRange(runtime); + + Assert.Equal(0u, runtime.InventoryOwner.Vendor.VendorId); + } + + [Fact] + public void EnforceRange_TeleportQueued_ClosesTheSessionBeforeArrival() + { + // Fix 1b: an in-session portal/teleport must close at transit + // BEGIN (TryQueueTeleportStart succeeding, i.e. + // HasPendingTeleportStart), not wait for the arrival frame + // (ActivateQueuedTeleport / IsTeleportActive). The player and + // vendor stay well within range the whole time — only the queued + // transit forces the close. + using GameRuntime runtime = Create(); + runtime.PlayerIdentity.ServerGuid = Player; + Add(runtime, Player, Landblock, 100f, 100f); + Add(runtime, Vendor, Landblock, 100f, 100f, useRadius: 3f); + Open(runtime, Vendor); + + Assert.True(runtime.TransitOwner.TryQueueTeleportStart(1)); + Assert.True(runtime.TransitOwner.HasPendingTeleportStart); + Assert.False(runtime.TransitOwner.IsTeleportActive); + + RuntimeVendorRangeQuery.EnforceRange(runtime); + + Assert.Equal(0u, runtime.InventoryOwner.Vendor.VendorId); + } + + [Fact] + public void EnforceRange_TeleportActive_ClosesTheSession() + { + // The other half of the transit window: once the queued teleport + // has been promoted to active (ActivateQueuedTeleport), the vendor + // session stays closed rather than being able to reopen mid-flight. + using GameRuntime runtime = Create(); + runtime.PlayerIdentity.ServerGuid = Player; + Add(runtime, Player, Landblock, 100f, 100f); + Add(runtime, Vendor, Landblock, 100f, 100f, useRadius: 3f); + Open(runtime, Vendor); + + Assert.True(runtime.TransitOwner.TryQueueTeleportStart(1)); + Assert.True(runtime.TransitOwner.ActivateQueuedTeleport()); + Assert.True(runtime.TransitOwner.IsTeleportActive); + + RuntimeVendorRangeQuery.EnforceRange(runtime); + + Assert.Equal(0u, runtime.InventoryOwner.Vendor.VendorId); + } + + [Fact] + public void EnforceRange_ThrowingChangedObserverDuringAutoClose_DoesNotPropagate() + { + // Fix 3: VendorState.Close()'s sole production caller is this + // per-frame query, with no try/catch anywhere up the frame-loop + // chain. A throwing presentation observer must not kill the frame, + // and the session must still end up closed. + using GameRuntime runtime = Create(); + runtime.PlayerIdentity.ServerGuid = Player; + RuntimeEntityRecord playerRecord = + Add(runtime, Player, Landblock, 100f, 100f); + Add(runtime, Vendor, Landblock, 102f, 100f, useRadius: 3f); + Open(runtime, Vendor); + + Action throwingObserver = + _ => throw new InvalidOperationException("boom"); + runtime.InventoryOwner.Vendor.Changed += throwingObserver; + + SetPosition(playerRecord, Landblock, 122f, 100f); + var exception = Record.Exception( + () => RuntimeVendorRangeQuery.EnforceRange(runtime)); + + Assert.Null(exception); + Assert.Equal(0u, runtime.InventoryOwner.Vendor.VendorId); + + // Detach before this scope's `using` disposal reaches + // RuntimeInventoryState.Dispose -> Vendor.Reset(), which — unlike + // Close() — legitimately rethrows (see Close()'s doc comment for + // why the two diverge). Leaving this attached would fail at + // teardown, not at the EnforceRange call this test exercises. + runtime.InventoryOwner.Vendor.Changed -= throwingObserver; + } + private static void Open(GameRuntime runtime, uint vendorGuid) => Assert.True(runtime.InventoryOwner.Vendor.Apply( vendorGuid,