From f4a771c19d3cd75c3ff8c3b479a6f72a86e278dc Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Thu, 16 Jul 2026 18:51:04 -0700 Subject: [PATCH] fix: Items dropped on the ground never decay (#2536) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Items dropped on the ground never decay. Corpses do, which makes the breakage look selective — but corpses are unaffected only because `Corpse.BeginDecay` runs its own `InternalTimer` and never touches `DecayScheduler`. Ordinary items are the only things that depend on the scheduler. ## Root cause `Item.MoveToWorld` called `SetLastMoved()` — which triggers `UpdateDecayRegistration()` — at the *top* of the method, before detaching the item from its parent and before assigning the new map. `CanDecay()` reads `Decays`, `Parent`, **and** `Map`, so registration was evaluated against the item's *pre-move* state. Because the `Item` constructor sets `m_Map = Map.Internal`, and `Mobile.Lift` calls `item.Internalize()` to put an item on the cursor, registration was consistently one step behind: | State | Tracked for decay? | | |---|---|---| | Item on the ground | **No** | never decays | | Item held on the cursor | **Yes** | backwards | Nothing corrected it afterwards: the later `m_Map = map` assigns the field directly, bypassing the `Map` property setter, and that setter does not refresh registration either. With no parent, `RemoveItem` (which *does* re-register) never runs. World load masked this — `ItemPersistence.PostDeserialize` re-registers every item against its final state, so decay appears to work for items that survive a restart. Only freshly dropped items are affected. **Fix:** stamp `LastMoved` up front so decay math stays correct, then call `UpdateDecayRegistration()` once the parent, map, and location are final. ## Audit of the rest of the call sites All 16 `SetLastMoved()` call sites were reviewed. `SetLastMoved()` must keep refreshing registration — `LastMoved` feeds `ScheduledDecayTime` and therefore which bucket an item belongs in — but it may only run once parent/map are final. The vendor, house, lift and drop sites already satisfy that. The rest of this PR fixes the ones that did not, plus what the audit turned up: - **`Item.Deserialize`** stamped via `SetLastMoved()` before the version data was read, registering against an unread `Map`/`Parent`. Safe only by accident (the `Item(Serial)` ctor leaves flags at 0, so `Decays` is false), and it cost an unregister per item per world load. Now stamps only; `PostDeserialize` does the registration. - **`Item` constructor** registered then immediately unregistered every item — the `Movable` setter saw `m_Map` still null, so `CanDecay()` was true. Also removes a `Configure`-order landmine: constructing an `Item` before `DecayScheduler.Configure()` would have thrown in `Shared.Start()`. - **`Container.Destroy`** stamped `LastMoved` immediately before `MoveToWorld`, which now stamps it itself. - **`Unregister` was documented O(1)** but scanned twelve buckets and did a linear `PriorityQueue.Remove`, on every construction, deserialize and move. Items now record where they are tracked in `Item.DecaySlot` (1 byte), so untracked items — the common case — leave in O(1). - **A refused decay silently dropped the item.** `ProcessActiveQueue` deleted on `OnDecay() == true` but did nothing when a region refused, leaving the item dequeued, untracked and on the ground forever. It now restarts the decay clock; re-registering as-is would spin, since `ScheduledDecayTime` is already past. ## Two content bugs of the same class The decay system replaced a polling sweep. Under polling, a `Decays`/`DecayTime` override could read live state every pass. Under a registration model it cannot — the scheduler drops items that stop being eligible, but nothing enrols one that becomes eligible while untracked. - **`TreasureChestLevel1-4`** overrode `DecayTime` as `Utility.Random(15, 60)` — a fresh roll on every read. `ScheduledDecayTime` is read repeatedly (to bucket, to re-bucket on rotation, to test whether due), so those reads disagreed: the chest re-bucketed every tick and decayed early instead of after its intended interval. Now rolled once per chest. Distribution unchanged — `Utility.Random(from, count)` is RunUO's `from + Next(count)`, so this is 15–74 minutes, as before. - **`StrongBox`** overrode `Decays` with a live check on `_house`, `_owner.Deleted` and `IsCoOwner`. Nothing notifies the box when any of those change, so it was never enrolled and the override never decayed anything — and it could not have: `HouseRegion.OnDecay` refuses a secured item inside a standing house, and the box is in `Secures`. Decay was never the mechanism here. - A strongbox is only ever its owner's. Without a house, or without an owner still co-owning that house, it would be a free container anyone could loot, so `Validate()` destroys it. The old check missed exactly those two cases — it required a non-null owner and treated a null house as valid. A deleted owner deserializes back as null, which `IsCoOwner` rejects. The now meaningless `Decays`/`DecayTime` overrides and an unhelpful `Console.WriteLine` are gone. `DecayScheduler` now documents both constraints. ## Tests `DecayRegistrationTests` (Server) covers world placement, lift/drop, container round-trip, cursor-held (must *not* track), `Container.Destroy` spill, `DecaySlot`/structure agreement, refused decay, and a full decay lifecycle driven through the scheduler. `TreasureChestDecayTests` (UOContent) locks `DecayTime`/`ScheduledDecayTime` stability across all four chest levels. To make the lifecycle testable deterministically, `DecayScheduler` gains `internal` members (visible only via existing `InternalsVisibleTo`): `IsRegistered()`, `ProcessTick(now)` — extracted from `OnTick()` with no behaviour change — and `ResetForTests()`. Red/green verified. Without the `MoveToWorld` fix, 5 of 6 of the original tests fail, including `ItemOnGround_ActuallyDecaysAfterDecayTime`, which shows a ground item never decays even after a full simulated hour. Without the chest fix, 8 of 8 chest tests fail. Without the refused-decay fix, that test fails. Server.Tests 737/737 and UOContent.Tests 509/509 pass. --- .../Tests/Items/DecayRegistrationTests.cs | 232 ++++++++++++++++++ Projects/Server/Items/Container.cs | 4 +- Projects/Server/Items/DecayScheduler.cs | 140 +++++++++-- Projects/Server/Items/Item.cs | 22 +- .../TreasureChests/TreasureChestDecayTests.cs | 65 +++++ .../UOContent/Items/Containers/Strongbox.cs | 10 +- .../TreasureChests/TreasureChestLevel1.cs | 5 +- .../TreasureChests/TreasureChestLevel2.cs | 5 +- .../TreasureChests/TreasureChestLevel3.cs | 5 +- .../TreasureChests/TreasureChestLevel4.cs | 5 +- 10 files changed, 453 insertions(+), 40 deletions(-) create mode 100644 Projects/Server.Tests/Tests/Items/DecayRegistrationTests.cs create mode 100644 Projects/UOContent.Tests/Tests/Items/TreasureChests/TreasureChestDecayTests.cs diff --git a/Projects/Server.Tests/Tests/Items/DecayRegistrationTests.cs b/Projects/Server.Tests/Tests/Items/DecayRegistrationTests.cs new file mode 100644 index 000000000..0e48977ef --- /dev/null +++ b/Projects/Server.Tests/Tests/Items/DecayRegistrationTests.cs @@ -0,0 +1,232 @@ +using System; +using Server.Items; +using Xunit; + +namespace Server.Tests; + +[Collection("Sequential Server Tests")] +public class DecayRegistrationTests +{ + // The wheel is static and these tests move the clock, so start each from a known schedule. + public DecayRegistrationTests() => DecayScheduler.ResetForTests(); + + // Stands in for a region that refuses decay, e.g. HouseRegion over a locked down item. + private class RefusesDecayItem : Item + { + public RefusesDecayItem() : base(0x1234) + { + } + + public override bool OnDecay() => false; + } + + // The item is already dequeued, so a refusal that does not reschedule strands it forever. + [Fact] + public void ItemWhoseDecayIsRefused_StaysTrackedAndSurvives() + { + var start = Core._now; + + try + { + var item = new RefusesDecayItem(); + item.MoveToWorld(new Point3D(106, 100, 0), Map.Felucca); + + AdvanceDecay(start, item.DecayTime + TimeSpan.FromMinutes(2), item); + + Assert.False(item.Deleted, "A refused decay must not delete the item."); + Assert.True( + DecayScheduler.IsRegistered(item), + "A refused decay must leave the item tracked so it is retried." + ); + + item.Delete(); + } + finally + { + Core._now = start; + } + } + + // Drives the scheduler and the game clock without depending on the shared timer wheel. + private static void AdvanceDecay(DateTime start, TimeSpan duration, Item item) + { + var deadline = start + duration; + + for (var now = start; now <= deadline && !item.Deleted; now += TimeSpan.FromMilliseconds(256)) + { + Core._now = now; + DecayScheduler.ProcessTick(now); + } + } + + [Fact] + public void ItemOnGround_ActuallyDecaysAfterDecayTime() + { + var start = Core._now; + + try + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + Assert.True(DecayScheduler.IsRegistered(item)); + + AdvanceDecay(start, item.DecayTime + TimeSpan.FromMinutes(2), item); + + Assert.True(item.Deleted, "Item on the ground must decay once DecayTime elapses."); + } + finally + { + Core._now = start; + } + } + + [Fact] + public void ItemOnGround_DoesNotDecayBeforeDecayTime() + { + var start = Core._now; + + try + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(102, 100, 0), Map.Felucca); + + AdvanceDecay(start, item.DecayTime - TimeSpan.FromMinutes(2), item); + + Assert.False(item.Deleted, "Item must not decay before DecayTime elapses."); + + item.Delete(); + } + finally + { + Core._now = start; + } + } + + [Fact] + public void ItemPlacedInWorld_IsRegisteredForDecay() + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + Assert.True(item.CanDecay()); + Assert.True(DecayScheduler.IsRegistered(item), "Item on the ground must be tracked for decay."); + + item.Delete(); + } + + // The real client path: Mobile.Lift internalizes, then Mobile.Drop calls MoveToWorld. + [Fact] + public void ItemLiftedThenDroppedToGround_IsRegisteredForDecay() + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + // Player lifts the item to the cursor. + item.Internalize(); + Assert.Equal(Map.Internal, item.Map); + + // Player drops it back onto the ground. + item.MoveToWorld(new Point3D(101, 100, 0), Map.Felucca); + + Assert.True(item.CanDecay()); + Assert.True( + DecayScheduler.IsRegistered(item), + "Item dropped to the ground after being lifted must be tracked for decay." + ); + + item.Delete(); + } + + // Held on the cursor means Map.Internal, which must not be tracked. + [Fact] + public void ItemLiftedToCursor_IsNotRegisteredForDecay() + { + var item = new Item(0x1234); + item.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + item.Internalize(); + + Assert.False(item.CanDecay()); + Assert.False(DecayScheduler.IsRegistered(item), "Item held on the cursor must not be tracked for decay."); + + item.Delete(); + } + + // A new item is on Map.Internal, so construction must not touch the scheduler. + [Fact] + public void NewItem_IsNotTracked() + { + var item = new Item(0x1234); + + Assert.Equal(DecayScheduler.SlotNone, item.DecaySlot); + Assert.False(DecayScheduler.IsRegistered(item), "A newly constructed item must not be tracked."); + + item.Delete(); + } + + // Unregister() trusts DecaySlot; IsRegistered() scans the structures. Assert both to catch drift. + [Fact] + public void DecaySlot_AgreesWithStructures_AcrossLifecycle() + { + var item = new Item(0x1234); + + item.MoveToWorld(new Point3D(103, 100, 0), Map.Felucca); + Assert.NotEqual(DecayScheduler.SlotNone, item.DecaySlot); + Assert.True(DecayScheduler.IsRegistered(item)); + + // Lifted to the cursor: untracked, and no stale slot left behind. + item.Internalize(); + Assert.Equal(DecayScheduler.SlotNone, item.DecaySlot); + Assert.False(DecayScheduler.IsRegistered(item)); + + // Back to the ground: tracked again. + item.MoveToWorld(new Point3D(104, 100, 0), Map.Felucca); + Assert.NotEqual(DecayScheduler.SlotNone, item.DecaySlot); + Assert.True(DecayScheduler.IsRegistered(item)); + + item.Delete(); + Assert.False(DecayScheduler.IsRegistered(item), "A deleted item must leave no trace in the scheduler."); + } + + // Contents spilled onto the ground must start decaying. + [Fact] + public void ContainerDestroy_ContentsDroppedToGroundAreTracked() + { + var pack = new Container(0xE75); + pack.MoveToWorld(new Point3D(105, 100, 0), Map.Felucca); + + var item = new Item(0x1234); + pack.AddItem(item); + Assert.False(DecayScheduler.IsRegistered(item)); + + pack.Destroy(); + + Assert.True(item.CanDecay()); + Assert.True(DecayScheduler.IsRegistered(item), "Contents spilled by Container.Destroy must be tracked."); + + item.Delete(); + } + + // Dropping into a container must untrack; taking it back out to the ground must re-track. + [Fact] + public void ItemMovedIntoContainerThenBackToGround_IsRegisteredForDecay() + { + var pack = new Container(0xE75); + pack.MoveToWorld(new Point3D(100, 100, 0), Map.Felucca); + + var item = new Item(0x1234); + pack.AddItem(item); + + Assert.False(DecayScheduler.IsRegistered(item), "Item inside a container must not be tracked for decay."); + + item.Internalize(); + item.MoveToWorld(new Point3D(101, 100, 0), Map.Felucca); + + Assert.True(item.CanDecay()); + Assert.True(DecayScheduler.IsRegistered(item), "Item taken out of a container must be tracked for decay."); + + item.Delete(); + pack.Delete(); + } +} diff --git a/Projects/Server/Items/Container.cs b/Projects/Server/Items/Container.cs index 34dbbbbb1..f139da24b 100644 --- a/Projects/Server/Items/Container.cs +++ b/Projects/Server/Items/Container.cs @@ -437,9 +437,7 @@ public partial class Container : Item for (var i = items.Count - 1; i >= 0; --i) { - var item = items[i]; - item.SetLastMoved(); - item.MoveToWorld(loc, map); + items[i].MoveToWorld(loc, map); } Delete(); diff --git a/Projects/Server/Items/DecayScheduler.cs b/Projects/Server/Items/DecayScheduler.cs index 165c1c274..694d9b6b2 100644 --- a/Projects/Server/Items/DecayScheduler.cs +++ b/Projects/Server/Items/DecayScheduler.cs @@ -22,9 +22,24 @@ namespace Server.Items; /// Timer wheel-based decay scheduler with O(1) registration/unregistration. /// Uses coarse-grained HashSet buckets for time intervals, with a PriorityQueue /// for fine-grained processing of items due within the current window. +/// +/// An item that becomes decay eligible must be passed to +/// once its parent, map and location are final. The scheduler drops items that stop being eligible, +/// but nothing enrolls one that becomes eligible while untracked. +/// +/// +/// and overrides must be stable rather than +/// recomputed per read: is read repeatedly, and a value that +/// changes between reads re-buckets the item every tick and decays it at the wrong time. +/// /// public class DecayScheduler : Timer { + // Item.DecaySlot values; any non-negative value is a bucket index. + internal const sbyte SlotNone = -1; + internal const sbyte SlotOverflow = -2; + internal const sbyte SlotActive = -3; + private const int BucketCount = 12; private static int _maxItemsPerTick; private static TimeSpan _tickInterval; @@ -74,6 +89,65 @@ public class DecayScheduler : Timer { } + /// + /// Test hook: drops all tracked items and re-bases the rotation schedule on + /// , so deadlines left by a test that moved the clock do not suppress + /// rotations in the next one. + /// + internal static void ResetForTests() + { + Shared?.Stop(); + + _activeQueue.Clear(); + _overflowBucket.Clear(); + + for (var i = 0; i < BucketCount; i++) + { + _buckets[i].Clear(); + } + + _currentBucketIndex = 0; + + var now = Core.Now; + _nextBucketRotation = now + _bucketInterval; + _nextOverflowCheck = now + _totalBucketSpan; + } + + /// + /// Diagnostics: true if the item is currently tracked in any bucket, the overflow + /// bucket, or the active queue. + /// + internal static bool IsRegistered(Item item) + { + if (item == null) + { + return false; + } + + if (_overflowBucket.Contains(item)) + { + return true; + } + + for (var i = 0; i < BucketCount; i++) + { + if (_buckets[i].Contains(item)) + { + return true; + } + } + + foreach (var (queued, _) in _activeQueue.UnorderedItems) + { + if (queued == item) + { + return true; + } + } + + return false; + } + /// /// Checks if all decay tracking structures are empty. /// @@ -115,11 +189,13 @@ public class DecayScheduler : Timer if (timeUntilDecay <= _bucketInterval) { _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } // If beyond the total bucket span, add to overflow bucket else if (timeUntilDecay > _totalBucketSpan) { _overflowBucket.Add(item); + item.DecaySlot = SlotOverflow; } else { @@ -127,6 +203,7 @@ public class DecayScheduler : Timer var bucketOffset = GetBucketOffset(timeUntilDecay); var absoluteIndex = (_currentBucketIndex + bucketOffset) % BucketCount; _buckets[absoluteIndex].Add(item); + item.DecaySlot = (sbyte)absoluteIndex; } } @@ -135,27 +212,26 @@ public class DecayScheduler : Timer /// public static void Unregister(Item item) { - if (item?.Deleted != false) + if (item == null || item.DecaySlot == SlotNone) { return; } - // Check overflow bucket first - if (_overflowBucket.Remove(item)) - { - return; - } + var slot = item.DecaySlot; + item.DecaySlot = SlotNone; - // Check regular buckets - for (var i = 0; i < BucketCount; i++) + if (slot == SlotOverflow) { - if (_buckets[i].Remove(item)) - { - return; - } + _overflowBucket.Remove(item); + } + else if (slot == SlotActive) + { + _activeQueue.Remove(item, out _, out _); + } + else + { + _buckets[slot].Remove(item); } - - _activeQueue.Remove(item, out _, out _); } /// @@ -171,10 +247,11 @@ public class DecayScheduler : Timer return Math.Clamp(bucketOffset, 0, BucketCount - 1); } - protected override void OnTick() + /// + /// Advances the wheel for . Returns true when nothing is left to track. + /// + internal static bool ProcessTick(DateTime now) { - var now = Core.Now; - // Process items from active queue first (smaller queue = faster log(n) operations) ProcessActiveQueue(now); @@ -190,8 +267,13 @@ public class DecayScheduler : Timer ProcessOverflow(now); } + return IsEmpty(); + } + + protected override void OnTick() + { // Stop timer if nothing left to track - if (IsEmpty()) + if (ProcessTick(Core.Now)) { Stop(); return; @@ -220,7 +302,8 @@ public class DecayScheduler : Timer { if (item.Deleted || !item.CanDecay()) { - continue; // Lazy cleanup - item was picked up or deleted + item.DecaySlot = SlotNone; // Lazy cleanup - item was picked up or deleted + continue; } var decayTime = item.ScheduledDecayTime; @@ -233,6 +316,7 @@ public class DecayScheduler : Timer { // Extended beyond total span - move to overflow _overflowBucket.Add(item); + item.DecaySlot = SlotOverflow; } else { @@ -243,11 +327,13 @@ public class DecayScheduler : Timer if (actualIndex != _currentBucketIndex) { _buckets[actualIndex].Add(item); + item.DecaySlot = (sbyte)actualIndex; } else { // Still maps to current bucket - add to active queue _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } } continue; @@ -255,6 +341,7 @@ public class DecayScheduler : Timer // Due within next window - add to active queue _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } // Clear the processed bucket @@ -276,10 +363,12 @@ public class DecayScheduler : Timer { if (item.Deleted || !item.CanDecay()) { + item.DecaySlot = SlotNone; return true; // Remove invalid items } - var timeUntilDecay = item.ScheduledDecayTime - now; + var decayTime = item.ScheduledDecayTime; + var timeUntilDecay = decayTime - now; if (timeUntilDecay > _totalBucketSpan) { @@ -289,13 +378,15 @@ public class DecayScheduler : Timer // Now within regular bucket range - move to appropriate bucket if (timeUntilDecay <= _bucketInterval) { - _activeQueue.Enqueue(item, item.ScheduledDecayTime); + _activeQueue.Enqueue(item, decayTime); + item.DecaySlot = SlotActive; } else { var bucketOffset = GetBucketOffset(timeUntilDecay); var actualIndex = (_currentBucketIndex + bucketOffset) % BucketCount; _buckets[actualIndex].Add(item); + item.DecaySlot = (sbyte)actualIndex; } return true; // Remove from overflow @@ -319,6 +410,7 @@ public class DecayScheduler : Timer processed++; _activeQueue.Dequeue(); + item.DecaySlot = SlotNone; if (item.Deleted || !item.CanDecay()) { @@ -333,6 +425,12 @@ public class DecayScheduler : Timer { item.Delete(); } + else + { + // Refused by the region. Restart the clock rather than dropping the item, which has + // already left the queue; re-registering as-is would spin on a due time in the past. + item.SetLastMoved(); + } } } } diff --git a/Projects/Server/Items/Item.cs b/Projects/Server/Items/Item.cs index 3a8adafe1..ee066c657 100644 --- a/Projects/Server/Items/Item.cs +++ b/Projects/Server/Items/Item.cs @@ -208,6 +208,10 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert private ItemDelta m_DeltaFlags; private Direction m_Direction; private ImplFlag m_Flags; + + // Where DecayScheduler tracks this item. Not serialized; PostDeserialize re-registers. + internal sbyte DecaySlot = DecayScheduler.SlotNone; + private int m_Hue; private int m_ItemID; private Layer m_Layer; @@ -225,12 +229,12 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert m_ItemID = itemID; Serial = World.NewItem; - Visible = true; - Movable = true; + // Assigned directly: the Visible/Movable setters and SetLastMoved() would register the item + // for decay while m_Map is still null, then unregister it once it is Map.Internal. + m_Flags = ImplFlag.Visible | ImplFlag.Movable; Amount = 1; m_Map = Map.Internal; - - SetLastMoved(); + LastMoved = Core.Now; World.AddEntity(this); } @@ -1107,7 +1111,9 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert var oldLocation = GetWorldLocation(); var oldRealLocation = m_Location; - SetLastMoved(); + // Register at the end instead: CanDecay() reads parent, map and location, none of which + // are final yet. + LastMoved = Core.Now; if (Parent is Mobile mobile) { @@ -1242,6 +1248,8 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert Map = map; Location = location; } + + UpdateDecayRegistration(); } /// @@ -2601,7 +2609,9 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert { var version = reader.ReadInt(); - SetLastMoved(); + // Default for versions without a saved value. Stamp only: map and parent are still unread, + // and PostDeserialize registers every item once its state is final. + LastMoved = Core.Now; switch (version) { diff --git a/Projects/UOContent.Tests/Tests/Items/TreasureChests/TreasureChestDecayTests.cs b/Projects/UOContent.Tests/Tests/Items/TreasureChests/TreasureChestDecayTests.cs new file mode 100644 index 000000000..31a839ca0 --- /dev/null +++ b/Projects/UOContent.Tests/Tests/Items/TreasureChests/TreasureChestDecayTests.cs @@ -0,0 +1,65 @@ +using System; +using Server; +using Server.Items; +using Server.Tests; +using Xunit; + +namespace UOContent.Tests; + +[Collection("Sequential UOContent Tests")] +public class TreasureChestDecayTests +{ + public static TheoryData ChestTypes => + [ + typeof(TreasureChestLevel1), + typeof(TreasureChestLevel2), + typeof(TreasureChestLevel3), + typeof(TreasureChestLevel4) + ]; + + // DecayScheduler reads ScheduledDecayTime repeatedly and needs the reads to agree, so the + // random interval must be rolled once per chest rather than per read. + [Theory] + [MemberData(nameof(ChestTypes))] + public void DecayTimeIsStableAcrossReads(Type chestType) + { + var chest = chestType.CreateInstance(); + + try + { + var expected = chest.DecayTime; + + for (var i = 0; i < 100; i++) + { + Assert.Equal(expected, chest.DecayTime); + } + } + finally + { + chest.Delete(); + } + } + + [Theory] + [MemberData(nameof(ChestTypes))] + public void ScheduledDecayTimeIsStableAcrossReads(Type chestType) + { + var chest = chestType.CreateInstance(); + + try + { + chest.MoveToWorld(new Point3D(150, 150, 0), Map.Felucca); + + var expected = chest.ScheduledDecayTime; + + for (var i = 0; i < 100; i++) + { + Assert.Equal(expected, chest.ScheduledDecayTime); + } + } + finally + { + chest.Delete(); + } + } +} diff --git a/Projects/UOContent/Items/Containers/Strongbox.cs b/Projects/UOContent/Items/Containers/Strongbox.cs index aa5e63925..7d461d715 100644 --- a/Projects/UOContent/Items/Containers/Strongbox.cs +++ b/Projects/UOContent/Items/Containers/Strongbox.cs @@ -32,10 +32,6 @@ public partial class StrongBox : BaseContainer, IChoppable public override int DefaultMaxWeight => 0; - public override bool Decays => _house == null || _owner?.Deleted != false || !_house.IsCoOwner(_owner); - - public override TimeSpan DecayTime => TimeSpan.FromMinutes(30.0); - public void OnChop(Mobile from) { if (_house?.Deleted != false || _owner?.Deleted != false || from == _owner || _house.IsOwner(from)) @@ -50,11 +46,13 @@ public partial class StrongBox : BaseContainer, IChoppable Timer.StartTimer(TimeSpan.FromSeconds(1.0), Validate); } + // A strongbox is only ever its owner's. Without a house, or without an owner still co-owning + // that house, it would be a free container anyone could loot, so it goes away instead. A deleted + // owner deserializes back as null, which IsCoOwner rejects. private void Validate() { - if (_owner != null && _house?.IsCoOwner(_owner) == false) + if (_house?.IsCoOwner(_owner) != true) { - Console.WriteLine("Warning: Destroying strongbox of {0}", _owner.Name); Destroy(); } } diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs index 61dfd853d..929903171 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel1.cs @@ -72,7 +72,10 @@ public partial class TreasureChestLevel1 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x44; diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs index bc3fe877c..f212d3e71 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel2.cs @@ -69,7 +69,10 @@ public partial class TreasureChestLevel2 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x42; diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs index 9ec26c7bb..04b4577c9 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel3.cs @@ -114,7 +114,10 @@ public partial class TreasureChestLevel3 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x42; diff --git a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs index a4bbd17bc..12639638d 100644 --- a/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs +++ b/Projects/UOContent/Items/TreasureChests/TreasureChestLevel4.cs @@ -117,7 +117,10 @@ public partial class TreasureChestLevel4 : LockableContainer public override bool IsDecoContainer => false; - public override TimeSpan DecayTime => TimeSpan.FromMinutes(Utility.Random(15, 60)); + // Rolled once: DecayScheduler reads ScheduledDecayTime repeatedly and needs a stable value. + private readonly TimeSpan _decayTime = TimeSpan.FromMinutes(Utility.Random(15, 60)); + + public override TimeSpan DecayTime => _decayTime; public override int DefaultGumpID => 0x42;