From 2b845b0d8d1a8d164f8a7a8cd40967c45702ed3e Mon Sep 17 00:00:00 2001 From: Kamron Batman <3953314+kamronbatman@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:10:31 -0700 Subject: [PATCH] fix: stop stack merges and splits from laundering PlayerConstructed PlayerConstructed is per-instance provenance, and #2574 put it on every crafted item -- including potions, arrows and other stackables. Stack operations were written when no item carried provenance of any kind, so they treated two piles of the same graphic as interchangeable. Merging keeps the receiving stack's value. Dropping bought potions onto a crafted stack made the whole pile count as crafted; the reverse order erased it. Which happened was decided by drag direction alone. CanStackWith now compares PlayerConstructed, so the two never merge into one indistinguishable pile. Splitting rebuilds one half in Mobile.LiftItemDupe, which copies a fixed list of fields rather than going through Dupe/CopyProperties -- PlayerConstructed was not on that list, so dragging part of a pile off stripped the new half. Note that [IgnoreDupe] does not govern this path; it only applies to Dupe(). LiftItemDupe now copies the flag, so a split cannot produce halves that disagree about what they are. Refusing to merge is the whole fix: a stack has nowhere to record provenance, so the only coherent behaviour is to keep the two piles apart. Paths that genuinely virtualize an item -- pouring a potion keg, for one -- rebuild it without the flag, and the result is simply treated as not crafted. Adds 7 tests: both merge directions, the matching case, split copying, and the split/re-merge round trip. --- .../Items/PlayerConstructedStackingTests.cs | 136 ++++++++++++++++++ Projects/Server/Items/Item.cs | 4 + Projects/Server/Mobiles/Mobile.cs | 6 + 3 files changed, 146 insertions(+) create mode 100644 Projects/Server.Tests/Tests/Items/PlayerConstructedStackingTests.cs diff --git a/Projects/Server.Tests/Tests/Items/PlayerConstructedStackingTests.cs b/Projects/Server.Tests/Tests/Items/PlayerConstructedStackingTests.cs new file mode 100644 index 000000000..05595068a --- /dev/null +++ b/Projects/Server.Tests/Tests/Items/PlayerConstructedStackingTests.cs @@ -0,0 +1,136 @@ +using Xunit; + +namespace Server.Tests; + +[Collection("Sequential Server Tests")] +public class PlayerConstructedStackingTests +{ + // PlayerConstructed is per-instance provenance, and stack operations were written when no + // item carried any. Merging keeps the receiver's copy of a field and splitting rebuilds one + // half from a fixed list of fields, so a flag that is not accounted for in both places is + // one that ordinary stacking can launder or erase. + + // Stands in for a real stackable type. LiftItemDupe builds the remainder through the + // parameterless constructor and copies only a fixed list of fields onto it -- Stackable is + // not on that list -- so the remainder is only stackable if the type restores it the way + // every genuine stackable does. + private class StackableItem : Item + { + public StackableItem() => Stackable = true; + + public StackableItem(Serial serial) : base(serial) => Stackable = true; + } + + private static StackableItem MakeStack(Serial serial, int amount, bool playerConstructed) => + new(serial) { Amount = amount, PlayerConstructed = playerConstructed }; + + [Fact] + public void CanStackWith_IsFalseWhenProvenanceDiffers() + { + var bought = MakeStack((Serial)0x1, 5, false); + var crafted = MakeStack((Serial)0x2, 5, true); + + try + { + // Both orders must fail. Whichever is the receiver decides the merged pile's flag, + // so allowing either one means the result is decided by drag direction. + Assert.False(bought.CanStackWith(crafted)); + Assert.False(crafted.CanStackWith(bought)); + } + finally + { + bought.Delete(); + crafted.Delete(); + } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void CanStackWith_IsTrueWhenProvenanceMatches(bool playerConstructed) + { + var first = MakeStack((Serial)0x1, 5, playerConstructed); + var second = MakeStack((Serial)0x2, 7, playerConstructed); + + try + { + Assert.True(first.CanStackWith(second)); + } + finally + { + first.Delete(); + second.Delete(); + } + } + + [Fact] + public void StackWith_RefusesToMergeAcrossProvenance() + { + var bought = MakeStack((Serial)0x1, 5, false); + var crafted = MakeStack((Serial)0x2, 5, true); + + try + { + Assert.False(bought.StackWith(null, crafted, false)); + Assert.Equal(5, bought.Amount); + Assert.Equal(5, crafted.Amount); + Assert.False(bought.PlayerConstructed); + Assert.False(crafted.Deleted); + } + finally + { + bought.Delete(); + crafted.Delete(); + } + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LiftItemDupe_CopiesPlayerConstructedToRemainder(bool playerConstructed) + { + var stack = MakeStack((Serial)0x1, 10, playerConstructed); + Item remainder = null; + + try + { + remainder = Mobile.LiftItemDupe(stack, 4); + + Assert.NotNull(remainder); + Assert.NotSame(stack, remainder); + Assert.Equal(4, stack.Amount); + Assert.Equal(6, remainder.Amount); + Assert.Equal(playerConstructed, remainder.PlayerConstructed); + } + finally + { + stack.Delete(); + remainder?.Delete(); + } + } + + [Fact] + public void SplitHalvesRemainStackableWithEachOther() + { + // The two halves of a split must still be one pile's worth: if the split dropped the + // flag, the remainder would no longer stack back onto what it came from. + var stack = MakeStack((Serial)0x1, 10, true); + Item remainder = null; + + try + { + remainder = Mobile.LiftItemDupe(stack, 4); + Assert.NotNull(remainder); + + Assert.True(stack.CanStackWith(remainder)); + Assert.True(stack.StackWith(null, remainder, false)); + Assert.Equal(10, stack.Amount); + Assert.True(stack.PlayerConstructed); + } + finally + { + stack.Delete(); + remainder?.Delete(); + } + } +} diff --git a/Projects/Server/Items/Item.cs b/Projects/Server/Items/Item.cs index 61e6f644f..204872e90 100644 --- a/Projects/Server/Items/Item.cs +++ b/Projects/Server/Items/Item.cs @@ -2344,12 +2344,16 @@ public partial class Item : IHued, IComparable, ISpawnable, IObjectPropert UpdateDecayRegistration(); } + // PlayerConstructed is part of stack identity. A merge keeps the receiving stack's value, so + // without this term the outcome depends on which side was dropped onto which: one order + // launders the flag onto items that never earned it, the other erases it from items that did. public virtual bool CanStackWith(Item dropped) => dropped.Stackable && Stackable && dropped.GetType() == GetType() && dropped.ItemID == ItemID && dropped.Hue == Hue && dropped.Name == Name && + dropped.PlayerConstructed == PlayerConstructed && dropped.Amount + Amount <= 60000 && dropped != this; diff --git a/Projects/Server/Mobiles/Mobile.cs b/Projects/Server/Mobiles/Mobile.cs index e47f977a7..69b57beee 100644 --- a/Projects/Server/Mobiles/Mobile.cs +++ b/Projects/Server/Mobiles/Mobile.cs @@ -5248,6 +5248,12 @@ public partial class Mobile : IHued, IComparable, ISpawnable, IObjectPro item.Name = oldItem.Name; item.Weight = oldItem.Weight; + // Splitting a stack must not change what either half is. This copies a fixed list of + // fields rather than going through Dupe/CopyProperties, so anything omitted here is + // silently dropped on the new half -- for PlayerConstructed that would mean dragging + // part of a pile off is enough to strip its provenance. + item.PlayerConstructed = oldItem.PlayerConstructed; + item.Amount = oldAmount - amount; item.Map = oldItem.Map;