fix(runtime/core): Slice 5.3 review corrections — retirement/transit close, per-unit pricing, guarded auto-close dispatch
Some checks are pending
Headless portability / portable-headless (ubuntu-latest) (push) Waiting to run
Headless portability / portable-headless (windows-latest) (push) Waiting to run
Headless portability / linux-graphical (push) Waiting to run
Headless portability / linux-vulkan (push) Waiting to run
Some checks are pending
Headless portability / portable-headless (ubuntu-latest) (push) Waiting to run
Headless portability / portable-headless (windows-latest) (push) Waiting to run
Headless portability / linux-graphical (push) Waiting to run
Headless portability / linux-vulkan (push) Waiting to run
The adversarial review's three blocking findings, each fixed at root: 1. A vendor session now CLOSES when its entity retires (despawn, death, ObjectDelete) and at teleport BEGIN (HasPendingTeleportStart || IsTeleportActive at the existing per-frame seam — both hosts funnel through RuntimeWorldTransitState.TryQueueTeleportStart, which flips the pending flag strictly before activation). The previous permissive early-return stranded the session forever: panel pinned to a stale guid, ActiveVendorId swallowing Use for the rest of the session. 2. VendorShopItem carries the desc's stack size, and VendorPricing.PerUnitValue ports retail's stack-total division (VendorProfile::VendorSellPrice 0x005D1B00: <= 0 guard, integer division) — a stack of 50 arrows now prices per arrow, not at 50x. 3. VendorState.Close() guards its observer fanout with the dispatcher's catch-and-log semantics — a throwing panel listener can no longer propagate into the unprotected per-frame path. Register honesty rides along: the 0.6 m UseRadius fallback was acdream's invention (ACE's CheckClose has no fallback; retail passes the raw authored radius) — removed, the watcher now uses the raw radius and AP-160's citations are corrected and extended with the accepted-position-snapshot cadence; AD-72 files VendorPricing's double-vs-x87-extended narrowing (AD-33's class, bounded by the ±0.1 margin). Nine tests added. Clean-room complete solution: 11,311 passed / 4 skipped / 0 failed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
parent
9796d71522
commit
609a2dfda0
9 changed files with 369 additions and 23 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<VendorShopItem>());
|
||||
|
||||
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()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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<VendorTransition> 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,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue