From 3109f59d4d90fd8bd0740191d3bc1d5bb484ba2e Mon Sep 17 00:00:00 2001 From: Voxpire Date: Thu, 23 Nov 2023 17:29:45 +0000 Subject: [PATCH] fix: Updates container searches for non-generic types (#1567) --- .../Tests/Items/ContainerTests.cs | 51 +++++ Projects/Server/Items/Container.Enumerable.cs | 183 ++++++++++++------ Projects/Server/Items/Container.cs | 173 +++++++---------- Projects/Server/Items/Item.cs | 1 - .../ContainedCommandImplementor.cs | 7 +- .../ML Quests/Objectives/DeliverObjective.cs | 2 +- .../UOContent/Items/Containers/SalvageBag.cs | 2 +- Projects/UOContent/Items/Misc/Key.cs | 2 +- Projects/UOContent/Mobiles/PlayerMobile.cs | 2 +- 9 files changed, 248 insertions(+), 175 deletions(-) diff --git a/Projects/Server.Tests/Tests/Items/ContainerTests.cs b/Projects/Server.Tests/Tests/Items/ContainerTests.cs index d500240df..d6d597754 100644 --- a/Projects/Server.Tests/Tests/Items/ContainerTests.cs +++ b/Projects/Server.Tests/Tests/Items/ContainerTests.cs @@ -1,3 +1,4 @@ +using System; using System.Collections.Generic; using Server.Items; using Xunit; @@ -70,4 +71,54 @@ public class ContainerTests : IClassFixture Assert.Null(staticItem); } + + [Fact] + public void TestFindItemsByTypeShouldThrowWhenModified() + { + var container = new Container((Serial)0x1); + container.AddItem(new Item((Serial)0x2)); + var staticItem = new Static((Serial)0x3); + container.AddItem(staticItem); + container.AddItem(new Item((Serial)0x4)); + + Assert.Throws( + () => + { + foreach (var item in container.FindItemsByType()) + { + if (item == staticItem) + { + container.RemoveItem(staticItem); + } + } + } + ); + } + + [Fact] + public void TestEnumerateItemsByTypeWhenModified() + { + var container = new Container((Serial)0x1); + + var item1 = new Item((Serial)0x2); + container.AddItem(item1); + var item2 = new Static((Serial)0x3); + container.AddItem(item2); + var item3 = new Item((Serial)0x4); + container.AddItem(item3); + + foreach (var item in container.EnumerateItemsByType()) + { + if (item == item2) + { + container.RemoveItem(item2); + } + } + + Assert.Equal(2, container.Items.Count); + Assert.Collection(container.Items, + item => Assert.Equal(item1, item), + item => Assert.Equal(item3, item) + ); + } } diff --git a/Projects/Server/Items/Container.Enumerable.cs b/Projects/Server/Items/Container.Enumerable.cs index 3aeed3a01..fc652c636 100644 --- a/Projects/Server/Items/Container.Enumerable.cs +++ b/Projects/Server/Items/Container.Enumerable.cs @@ -22,20 +22,17 @@ namespace Server.Items; public partial class Container { - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public FindItemsByTypeEnumerator FindItems(bool recurse = true, Predicate predicate = null) - => FindItemsByType(recurse, predicate); - /// /// Performs a breadth-first search through all the s and /// nested s within this . /// /// - /// DO NOT consume, delete, or move items while iterating + /// DO NOT consume, delete, or move items while iterating with any FindItemByType or FindItems overloads /// /// /// /// var total = 0; + /// /// foreach (var gold in cont.FindItemsByType<Gold>()) /// { /// total += gold.Amount; @@ -57,12 +54,20 @@ public partial class Container /// . /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - public FindItemsByTypeEnumerator FindItemsByType(bool recurse = true, Predicate predicate = null) - where T : Item => new(this, recurse, predicate); + public FindItemsByTypeEnumerator FindItemsByType(bool recurse = true, Predicate predicate = null) where T : Item => + new(this, recurse, predicate); [MethodImpl(MethodImplOptions.AggressiveInlining)] - public QueuedItemsEnumerator EnumerateItems(bool recurse = true, Predicate predicate = null) - => EnumerateItemsByType(recurse, predicate); + public FindItemsByTypeEnumerator FindItemsByType(Type type, bool recurse = true) => + new(this, recurse, type.IsInstanceOfType); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public FindItemsByTypeEnumerator FindItemsByType(Type[] types, bool recurse = true) => + new(this, recurse, item => item.InTypeList(types)); + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public FindItemsByTypeEnumerator FindItems(bool recurse = true, Predicate predicate = null) => + new(this, recurse, predicate); /// /// Safely enumerates items using a breadth-first search through all the s and @@ -70,7 +75,8 @@ public partial class Container /// /// /// Use EnumerateItemsByType for situations where the item might be manipulated, consumed, or moved. - /// Note: This method scans through the container before returning the enumerator for iteration. + /// Note: This method scans through the container before returning the enumerator for iteration and therefore + /// incurs a performance penalty from the overhead. /// /// /// @@ -98,16 +104,10 @@ public partial class Container /// . /// [MethodImpl(MethodImplOptions.AggressiveInlining)] - public QueuedItemsEnumerator EnumerateItemsByType(bool recurse = true, Predicate predicate = null) - where T : Item => new(QueueItemsByType(recurse, predicate)); - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public PooledRefQueue QueueItems(bool recurse = true, Predicate predicate = null) => - QueueItemsByType(recurse, predicate); - - public PooledRefQueue QueueItemsByType(bool recurse = true, Predicate predicate = null) where T : Item + public PooledRefQueue EnumerateItemsByType(bool recurse = true, Predicate predicate = null) where T : Item { - var queue = PooledRefQueue.Create(); + var queue = PooledRefQueue.Create(128); + foreach (var item in FindItemsByType(recurse, predicate)) { queue.Enqueue(item); @@ -117,12 +117,45 @@ public partial class Container } [MethodImpl(MethodImplOptions.AggressiveInlining)] - public PooledRefList ListItems(bool recurse = true, Predicate predicate = null) => - ListItemsByType(recurse, predicate); + public PooledRefQueue EnumerateItemsByType(Type type, bool recurse = true) + { + var queue = PooledRefQueue.Create(128); + + foreach (var item in FindItemsByType(recurse)) + { + if (type.IsInstanceOfType(item)) + { + queue.Enqueue(item); + } + } + + return queue; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public PooledRefQueue EnumerateItemsByType(Type[] types, bool recurse = true) + { + var queue = PooledRefQueue.Create(128); + + foreach (var item in FindItemsByType(recurse)) + { + if (item.InTypeList(types)) + { + queue.Enqueue(item); + } + } + + return queue; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public PooledRefQueue EnumerateItems(bool recurse = true, Predicate predicate = null) => + EnumerateItemsByType(recurse, predicate); public PooledRefList ListItemsByType(bool recurse = true, Predicate predicate = null) where T : Item { - var list = PooledRefList.Create(); + var list = PooledRefList.Create(128); + foreach (var item in FindItemsByType(recurse, predicate)) { list.Add(item); @@ -131,22 +164,69 @@ public partial class Container return list; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public PooledRefList ListItemsByType(Type type, bool recurse = true) + { + var list = PooledRefList.Create(128); + + foreach (var item in FindItemsByType(recurse)) + { + if (type.IsInstanceOfType(item)) + { + list.Add(item); + } + } + + return list; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public PooledRefList ListItemsByType(Type[] types, bool recurse = true) + { + var list = PooledRefList.Create(128); + + foreach (var item in FindItemsByType(recurse)) + { + if (item.InTypeList(types)) + { + list.Add(item); + } + } + + return list; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public PooledRefList ListItems(bool recurse = true, Predicate predicate = null) => + ListItemsByType(recurse, predicate); + public ref struct FindItemsByTypeEnumerator where T : Item { + private const string InvalidOperation_EnumFailedVersion = + "Container was modified after enumerator was instantiated. Use Container.EnumerateItems method instead for safe enumerations."; + private PooledRefQueue _containers; private Span _items; private int _index; private T _current; - private bool _recurse; - private Predicate _predicate; + private readonly bool _recurse; + private readonly Predicate _predicate; + private Container _currentContainer; + private int _version; public FindItemsByTypeEnumerator(Container container, bool recurse, Predicate predicate) { - _containers = PooledRefQueue.Create(); + _containers = PooledRefQueue.Create(_recurse ? 64 : 0); - if (container?.m_Items != null) + if (container != null) { - _items = CollectionsMarshal.AsSpan(container.m_Items); + if (container.m_Items != null) + { + _items = CollectionsMarshal.AsSpan(container.m_Items); + } + + _currentContainer = container; + _version = container._version; } _current = default; @@ -163,8 +243,11 @@ public partial class Container { while (_containers.TryDequeue(out var c)) { + _currentContainer = c; _items = CollectionsMarshal.AsSpan(c.m_Items); _index = 0; + _version = c._version; + if (SetNextItem()) { return true; @@ -177,6 +260,11 @@ public partial class Container [MethodImpl(MethodImplOptions.AggressiveInlining)] private bool SetNextItem() { + if (_version != _currentContainer._version) + { + throw new InvalidOperationException(InvalidOperation_EnumFailedVersion); + } + while (_index < _items.Length) { Item item = _items[_index++]; @@ -187,6 +275,11 @@ public partial class Container if (item is T t && _predicate?.Invoke(t) != false) { + if (_version != _currentContainer._version) + { + throw new InvalidOperationException(InvalidOperation_EnumFailedVersion); + } + _current = t; return true; } @@ -207,40 +300,4 @@ public partial class Container [MethodImpl(MethodImplOptions.AggressiveInlining)] public FindItemsByTypeEnumerator GetEnumerator() => this; } - - public ref struct QueuedItemsEnumerator where T : Item - { - private PooledRefQueue _queue; - private T _current; - - public QueuedItemsEnumerator(PooledRefQueue queue) - { - _queue = queue; - _current = default; - } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public bool MoveNext() - { - if (_queue.TryDequeue(out var item)) - { - _current = item; - return true; - } - - return false; - } - - public T Current - { - [MethodImpl(MethodImplOptions.AggressiveInlining)] - get => _current; - } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public void Dispose() => _queue.Dispose(); - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - public QueuedItemsEnumerator GetEnumerator() => this; - } } diff --git a/Projects/Server/Items/Container.cs b/Projects/Server/Items/Container.cs index 3f7665ada..53b4fb0bf 100644 --- a/Projects/Server/Items/Container.cs +++ b/Projects/Server/Items/Container.cs @@ -1,7 +1,6 @@ using System; using System.Collections.Generic; using System.IO; -using System.Runtime.InteropServices; using Server.Collections; using Server.Logging; using Server.Network; @@ -27,6 +26,7 @@ public partial class Container : Item private int m_TotalItems; private int m_TotalWeight; + private int _version; public Container(int itemID) : base(itemID) { @@ -464,6 +464,18 @@ public partial class Container : Item } } + public override void OnItemAdded(Item item) + { + base.OnItemAdded(item); + _version++; + } + + public override void OnItemRemoved(Item item) + { + base.OnItemRemoved(item); + _version++; + } + public virtual bool OnStackAttempt(Mobile from, Item stack, Item dropped) => CheckHold(from, dropped, true, false) && stack.StackWith(from, dropped); @@ -800,14 +812,7 @@ public partial class Container : Item throw new ArgumentNullException(nameof(grouper)); } - using var typedItems = PooledRefList.Create(); - foreach (var item in FindItems(recurse)) - { - if (type.IsInstanceOfType(item)) - { - typedItems.Add(item); - } - } + using var typedItems = ListItemsByType(type, recurse); var groups = new List>(); var idx = 0; @@ -815,9 +820,10 @@ public partial class Container : Item while (idx < typedItems.Count) { var a = typedItems[idx++]; - var group = new List(); - - group.Add(a); + var group = new List + { + a + }; while (idx < typedItems.Count) { @@ -920,14 +926,8 @@ public partial class Container : Item for (var i = 0; i < types.Length; ++i) { var type = types[i]; - using var typedItems = PooledRefList.Create(); - foreach (var item in FindItems(recurse)) - { - if (type.IsInstanceOfType(item)) - { - typedItems.Add(item); - } - } + + using var typedItems = ListItemsByType(type, recurse); var groups = new List>(); var idx = 0; @@ -935,9 +935,10 @@ public partial class Container : Item while (idx < typedItems.Count) { var a = typedItems[idx++]; - var group = new List(); - - group.Add(a); + var group = new List + { + a + }; while (idx < typedItems.Count) { @@ -1043,19 +1044,20 @@ public partial class Container : Item for (var i = 0; i < types.Length; ++i) { - var typedItems = CollectionsMarshal.AsSpan(FindItemsByType(types[i], recurse)); + using var typedItems = ListItemsByType(types[i], recurse); var groups = new List>(); var idx = 0; - while (idx < typedItems.Length) + while (idx < typedItems.Count) { var a = typedItems[idx++]; - var group = new List(); + var group = new List + { + a + }; - group.Add(a); - - while (idx < typedItems.Length) + while (idx < typedItems.Count) { var b = typedItems[idx]; var v = grouper(a, b); @@ -1146,16 +1148,19 @@ public partial class Container : Item throw new ArgumentException("length of types and amounts must match"); } - var items = new List[types.Length]; + var items = new Item[types.Length][]; var totals = new int[types.Length]; for (var i = 0; i < types.Length; ++i) { - items[i] = FindItemsByType(types[i], recurse); + using var typedItems = ListItemsByType(types[i], recurse); - for (var j = 0; j < items[i].Count; ++j) + items[i] = new Item[typedItems.Count]; + + for (var j = 0; j < typedItems.Count; ++j) { - totals[i] += items[i][j].Amount; + items[i][j] = typedItems[j]; + totals[i] += typedItems[j].Amount; } if (totals[i] < amounts[i]) @@ -1168,7 +1173,7 @@ public partial class Container : Item { var need = amounts[i]; - for (var j = 0; j < items[i].Count; ++j) + for (var j = 0; j < items[i].Length; ++j) { var item = items[i][j]; @@ -1201,19 +1206,19 @@ public partial class Container : Item throw new ArgumentException("length of types and amounts must match"); } - var items = new List[types.Length]; + var items = new Item[types.Length][]; var totals = new int[types.Length]; for (var i = 0; i < types.Length; ++i) { - var itemList = items[i] = new List(); - foreach (var item in FindItems()) + using var typedItems = ListItemsByType(types[i], recurse); + + items[i] = new Item[typedItems.Count]; + + for (var j = 0; j < typedItems.Count; ++j) { - if (types[i].IsInstanceOfType(item)) - { - totals[i] += item.Amount; - itemList.Add(item); - } + items[i][j] = typedItems[j]; + totals[i] += typedItems[j].Amount; } if (totals[i] < amounts[i]) @@ -1226,7 +1231,7 @@ public partial class Container : Item { var need = amounts[i]; - for (var j = 0; j < items[i].Count; ++j) + for (var j = 0; j < items[i].Length; ++j) { var item = items[i][j]; @@ -1255,20 +1260,17 @@ public partial class Container : Item public bool ConsumeTotal(Type type, int amount = 1, bool recurse = true, OnItemConsumed callback = null) { var total = 0; - using var items = PooledRefQueue.Create(); + + using var typedItems = ListItemsByType(type, recurse); // First pass, compute total - foreach (var item in FindItems(recurse)) + foreach (var item in typedItems) { - if (type.IsInstanceOfType(item)) - { - items.Enqueue(item); + total += item.Amount; - total += item.Amount; - if (total >= amount) - { - break; - } + if (total >= amount) + { + break; } } @@ -1277,10 +1279,8 @@ public partial class Container : Item { var need = amount; - while (items.Count > 0) + foreach (var item in typedItems) { - var item = items.Dequeue(); - var theirAmount = item.Amount; if (theirAmount < need) @@ -1308,6 +1308,7 @@ public partial class Container : Item var consumed = 0; using var toDelete = PooledRefQueue.Create(); + RecurseConsumeUpTo(this, type, amount, recurse, ref consumed, toDelete); while (toDelete.Count > 0) @@ -1368,14 +1369,7 @@ public partial class Container : Item var best = 0; - using var typedItems = PooledRefList.Create(); - foreach (var item in FindItems(recurse)) - { - if (type.IsInstanceOfType(item)) - { - typedItems.Add(item); - } - } + using var typedItems = ListItemsByType(type, recurse); var groups = new List>(); var idx = 0; @@ -1437,12 +1431,12 @@ public partial class Container : Item var best = 0; - var typedItems = CollectionsMarshal.AsSpan(FindItemsByType(types, recurse)); + var typedItems = ListItemsByType(types, recurse); var groups = new List>(); var idx = 0; - while (idx < typedItems.Length) + while (idx < typedItems.Count) { var a = typedItems[idx++]; var group = new List @@ -1450,7 +1444,7 @@ public partial class Container : Item a }; - while (idx < typedItems.Length) + while (idx < typedItems.Count) { var b = typedItems[idx]; var v = grouper(a, b); @@ -1474,6 +1468,7 @@ public partial class Container : Item { var items = groups[j].ToArray(); var total = 0; + foreach (var item in items) { total += item.Amount; @@ -1499,19 +1494,20 @@ public partial class Container : Item for (var i = 0; i < types.Length; ++i) { - var typedItems = CollectionsMarshal.AsSpan(FindItemsByType(types[i], recurse)); + using var typedItems = ListItemsByType(types[i], recurse); var groups = new List>(); var idx = 0; - while (idx < typedItems.Length) + while (idx < typedItems.Count) { var a = typedItems[idx++]; - var group = new List(); + var group = new List + { + a + }; - group.Add(a); - - while (idx < typedItems.Length) + while (idx < typedItems.Count) { var b = typedItems[idx]; var v = grouper(a, b); @@ -1554,6 +1550,7 @@ public partial class Container : Item public int GetAmount(Type type, bool recurse = true) { var total = 0; + foreach (var item in FindItems(recurse)) { if (type.IsInstanceOfType(item)) @@ -1568,6 +1565,7 @@ public partial class Container : Item public int GetAmount(Type[] types, bool recurse = true) { var total = 0; + foreach (var item in FindItems(recurse)) { if (item.InTypeList(types)) @@ -1578,35 +1576,6 @@ public partial class Container : Item return total; } - - public List FindItemsByType(Type type, bool recurse = true) - { - var items = new List(); - foreach (var item in FindItems(recurse)) - { - if (type.IsInstanceOfType(item)) - { - items.Add(item); - } - } - - return items; - } - - public List FindItemsByType(Type[] types, bool recurse = true) - { - var items = new List(); - foreach (var item in FindItems(recurse)) - { - if (item.InTypeList(types)) - { - items.Add(item); - } - } - - return items; - } - public Item FindItemByType(Type type, bool recurse = true) { foreach (var item in FindItems(recurse)) diff --git a/Projects/Server/Items/Item.cs b/Projects/Server/Items/Item.cs index 903efa702..d78731611 100644 --- a/Projects/Server/Items/Item.cs +++ b/Projects/Server/Items/Item.cs @@ -209,7 +209,6 @@ public class Item : IHued, IComparable, ISpawnable, IObjectPropertyListEnt m_ItemID = itemID; Serial = World.NewItem; - // m_Items = new ArrayList( 1 ); Visible = true; Movable = true; Amount = 1; diff --git a/Projects/UOContent/Commands/Generic/Implementors/ContainedCommandImplementor.cs b/Projects/UOContent/Commands/Generic/Implementors/ContainedCommandImplementor.cs index f2b99aa0e..225871532 100644 --- a/Projects/UOContent/Commands/Generic/Implementors/ContainedCommandImplementor.cs +++ b/Projects/UOContent/Commands/Generic/Implementors/ContainedCommandImplementor.cs @@ -67,12 +67,9 @@ namespace Server.Commands.Generic var list = new List(); - foreach (var item in cont.FindItems()) + foreach (var item in cont.EnumerateItems(true, ext.IsValid)) { - if (ext.IsValid(item)) - { - list.Add(item); - } + list.Add(item); } ext.Filter(list); diff --git a/Projects/UOContent/Engines/ML Quests/Objectives/DeliverObjective.cs b/Projects/UOContent/Engines/ML Quests/Objectives/DeliverObjective.cs index 9137b0152..124f17def 100644 --- a/Projects/UOContent/Engines/ML Quests/Objectives/DeliverObjective.cs +++ b/Projects/UOContent/Engines/ML Quests/Objectives/DeliverObjective.cs @@ -193,7 +193,7 @@ namespace Server.Engines.MLQuests.Objectives var left = Objective.Amount; - foreach (var item in pack.EnumerateItemsByType(false, ClaimTypePredicate)) + foreach (var item in pack.EnumerateItems(false, ClaimTypePredicate)) { if (left == 0) { diff --git a/Projects/UOContent/Items/Containers/SalvageBag.cs b/Projects/UOContent/Items/Containers/SalvageBag.cs index b1f19e052..d9c846104 100644 --- a/Projects/UOContent/Items/Containers/SalvageBag.cs +++ b/Projects/UOContent/Items/Containers/SalvageBag.cs @@ -251,7 +251,7 @@ public partial class SalvageBag : Bag var salvaged = 0; var notSalvaged = 0; - foreach (var item in EnumerateItemsByType()) + foreach (var item in EnumerateItems()) { if (item is not IScissorable scissorable) { diff --git a/Projects/UOContent/Items/Misc/Key.cs b/Projects/UOContent/Items/Misc/Key.cs index 8685f6433..5c557c65d 100644 --- a/Projects/UOContent/Items/Misc/Key.cs +++ b/Projects/UOContent/Items/Misc/Key.cs @@ -97,7 +97,7 @@ public partial class Key : Item return false; } - foreach (var item in cont.EnumerateItems()) + foreach (var item in cont.FindItems()) { if (item is Key key) { diff --git a/Projects/UOContent/Mobiles/PlayerMobile.cs b/Projects/UOContent/Mobiles/PlayerMobile.cs index 940b9520f..014f06230 100644 --- a/Projects/UOContent/Mobiles/PlayerMobile.cs +++ b/Projects/UOContent/Mobiles/PlayerMobile.cs @@ -2394,7 +2394,7 @@ namespace Server.Mobiles // This fixes a "bug" where players put blessed items in nested bags and they were dropped on death if (Core.AOS && Backpack?.Deleted == false) { - foreach (var item in Backpack.EnumerateItemsByType(predicate: FindItems_Callback)) + foreach (var item in Backpack.EnumerateItems(true, FindItems_Callback)) { Backpack.AddItem(item); }