wip

Dalamud/Game/Inventory/GameInventory.cs

@@ -1,4 +1,6 @@
-using System.Collections.Generic;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
 
 using Dalamud.Configuration.Internal;
 using Dalamud.Game.Inventory.InventoryChangeArgsTypes;
@@ -22,7 +24,6 @@ internal class GameInventory : IDisposable, IServiceType, IGameInventory
 {
     private static readonly ModuleLog Log = new("GameInventory");
 
-    private readonly List<InventoryEventArgs> allEvents = new();
     private readonly List<InventoryItemAddedArgs> addedEvents = new();
     private readonly List<InventoryItemRemovedArgs> removedEvents = new();
     private readonly List<InventoryItemChangedArgs> changedEvents = new();
@@ -59,10 +60,10 @@ internal class GameInventory : IDisposable, IServiceType, IGameInventory
     public event IGameInventory.InventoryChangedDelegate? ItemRemoved;
 
     /// <inheritdoc/>
-    public event IGameInventory.InventoryChangedDelegate? ItemMoved;
+    public event IGameInventory.InventoryChangedDelegate? ItemChanged;
 
     /// <inheritdoc/>
-    public event IGameInventory.InventoryChangedDelegate? ItemChanged;
+    public event IGameInventory.InventoryChangedDelegate? ItemMoved;
 
     /// <inheritdoc/>
     public void Dispose()
@@ -152,124 +153,135 @@ internal class GameInventory : IDisposable, IServiceType, IGameInventory
         }
 
         // Was there any change? If not, stop further processing.
-        // Note that...
+        // Note that this.movedEvents is not checked; it will be populated after this check.
-        // * this.movedEvents is not checked; it will be populated after this check.
-        // * this.allEvents is not checked; it is a temporary list to be used after this check.
         if (this.addedEvents.Count == 0 && this.removedEvents.Count == 0 && this.changedEvents.Count == 0)
             return;
 
-        try
+        // Broadcast InventoryChangedRaw.
+        InvokeSafely(
+            this.InventoryChangedRaw,
+            new DeferredReadOnlyCollection<InventoryEventArgs>(
+                this.addedEvents.Count +
+                this.removedEvents.Count +
+                this.changedEvents.Count,
+                () => Array.Empty<InventoryEventArgs>()
+                           .Concat(this.addedEvents)
+                           .Concat(this.removedEvents)
+                           .Concat(this.changedEvents)));
+
+        // Resolve changelog for item moved, from 1 added + 1 removed event.
+        for (var iAdded = this.addedEvents.Count - 1; iAdded >= 0; --iAdded)
         {
-            // Broadcast InventoryChangedRaw, if necessary.
+            var added = this.addedEvents[iAdded];
-            if (this.InventoryChangedRaw is not null)
+            for (var iRemoved = this.removedEvents.Count - 1; iRemoved >= 0; --iRemoved)
             {
-                this.allEvents.Clear();
+                var removed = this.removedEvents[iRemoved];
-                this.allEvents.EnsureCapacity(
+                if (added.Item.ItemId != removed.Item.ItemId)
-                    this.addedEvents.Count
+                    continue;
-                    + this.removedEvents.Count
-                    + this.changedEvents.Count);
-                this.allEvents.AddRange(this.addedEvents);
-                this.allEvents.AddRange(this.removedEvents);
-                this.allEvents.AddRange(this.changedEvents);
-                InvokeSafely(this.InventoryChangedRaw, this.allEvents);
-            }
 
-            // Resolve changelog for item moved, from 1 added + 1 removed event.
+                this.movedEvents.Add(new(removed, added));
-            for (var iAdded = this.addedEvents.Count - 1; iAdded >= 0; --iAdded)
+
+                // Remove the reinterpreted entries.
+                this.addedEvents.RemoveAt(iAdded);
+                this.removedEvents.RemoveAt(iRemoved);
+                break;
+            }
+        }
+
+        // Resolve changelog for item moved, from 2 changed events.
+        for (var i = this.changedEvents.Count - 1; i >= 0; --i)
+        {
+            var e1 = this.changedEvents[i];
+            for (var j = i - 1; j >= 0; --j)
             {
-                var added = this.addedEvents[iAdded];
+                var e2 = this.changedEvents[j];
-                for (var iRemoved = this.removedEvents.Count - 1; iRemoved >= 0; --iRemoved)
+                if (e1.Item.ItemId != e2.Item.ItemId || e1.Item.ItemId != e2.Item.ItemId)
-                {
+                    continue;
-                    var removed = this.removedEvents[iRemoved];
-                    if (added.Item.ItemId != removed.Item.ItemId)
-                        continue;
 
-                    this.movedEvents.Add(new(removed, added));
+                // move happened, and e2 has an item
+                if (!e2.Item.IsEmpty)
+                    this.movedEvents.Add(new(e1, e2));
 
-                    // Remove the reinterpreted entries.
+                // move happened, and e1 has an item
-                    this.addedEvents.RemoveAt(iAdded);
+                if (!e1.Item.IsEmpty)
-                    this.removedEvents.RemoveAt(iRemoved);
+                    this.movedEvents.Add(new(e2, e1));
-                    break;
+
-                }
+                // Remove the reinterpreted entries. Note that i > j.
+                this.changedEvents.RemoveAt(i);
+                this.changedEvents.RemoveAt(j);
+                break;
             }
+        }
 
-            // Resolve changelog for item moved, from 2 changed events.
+        // Log only if it matters.
-            for (var i = this.changedEvents.Count - 1; i >= 0; --i)
+        if (this.dalamudConfiguration.LogLevel <= LogEventLevel.Verbose)
-            {
+        {
-                var e1 = this.changedEvents[i];
-                for (var j = i - 1; j >= 0; --j)
-                {
-                    var e2 = this.changedEvents[j];
-                    if (e1.Item.ItemId != e2.Item.ItemId || e1.Item.ItemId != e2.Item.ItemId)
-                        continue;
-
-                    // move happened, and e2 has an item
-                    if (!e2.Item.IsEmpty)
-                        this.movedEvents.Add(new(e1, e2));
-
-                    // move happened, and e1 has an item
-                    if (!e1.Item.IsEmpty)
-                        this.movedEvents.Add(new(e2, e1));
-                        
-                    // Remove the reinterpreted entries. Note that i > j.
-                    this.changedEvents.RemoveAt(i);
-                    this.changedEvents.RemoveAt(j);
-                    break;
-                }
-            }
-
-            // Log only if it matters.
-            if (this.dalamudConfiguration.LogLevel >= LogEventLevel.Verbose)
-            {
-                foreach (var x in this.addedEvents)
-                    Log.Verbose($"{x}");
-            
-                foreach (var x in this.removedEvents)
-                    Log.Verbose($"{x}");
-            
-                foreach (var x in this.changedEvents)
-                    Log.Verbose($"{x}");
-            
-                foreach (var x in this.movedEvents)
-                    Log.Verbose($"{x} (({x.SourceEvent}) + ({x.TargetEvent}))");
-            }
-
-            // Broadcast InventoryChanged, if necessary.
-            if (this.InventoryChanged is not null)
-            {
-                this.allEvents.Clear();
-                this.allEvents.EnsureCapacity(
-                    this.addedEvents.Count
-                    + this.removedEvents.Count
-                    + this.changedEvents.Count
-                    + this.movedEvents.Count);
-                this.allEvents.AddRange(this.addedEvents);
-                this.allEvents.AddRange(this.removedEvents);
-                this.allEvents.AddRange(this.changedEvents);
-                this.allEvents.AddRange(this.movedEvents);
-                InvokeSafely(this.InventoryChanged, this.allEvents);
-            }
-
-            // Broadcast the rest.
             foreach (var x in this.addedEvents)
-                InvokeSafely(this.ItemAdded, x);
+                Log.Verbose($"{x}");
 
             foreach (var x in this.removedEvents)
-                InvokeSafely(this.ItemRemoved, x);
+                Log.Verbose($"{x}");
 
             foreach (var x in this.changedEvents)
-                InvokeSafely(this.ItemChanged, x);
+                Log.Verbose($"{x}");
 
             foreach (var x in this.movedEvents)
-                InvokeSafely(this.ItemMoved, x);
+                Log.Verbose($"{x} (({x.SourceEvent}) + ({x.TargetEvent}))");
         }
-        finally
+
+        // Broadcast the rest.
+        InvokeSafely(
+            this.InventoryChanged,
+            new DeferredReadOnlyCollection<InventoryEventArgs>(
+                this.addedEvents.Count +
+                this.removedEvents.Count +
+                this.changedEvents.Count +
+                this.movedEvents.Count,
+                () => Array.Empty<InventoryEventArgs>()
+                           .Concat(this.addedEvents)
+                           .Concat(this.removedEvents)
+                           .Concat(this.changedEvents)
+                           .Concat(this.movedEvents)));
+
+        foreach (var x in this.addedEvents)
+            InvokeSafely(this.ItemAdded, x);
+
+        foreach (var x in this.removedEvents)
+            InvokeSafely(this.ItemRemoved, x);
+
+        foreach (var x in this.changedEvents)
+            InvokeSafely(this.ItemChanged, x);
+
+        foreach (var x in this.movedEvents)
+            InvokeSafely(this.ItemMoved, x);
+
+        // We're done using the lists. Clean them up.
+        this.addedEvents.Clear();
+        this.removedEvents.Clear();
+        this.changedEvents.Clear();
+        this.movedEvents.Clear();
+    }
+
+    /// <summary>
+    /// A <see cref="IReadOnlyCollection{T}"/> view of <see cref="IEnumerable{T}"/>, so that the number of items
+    /// contained within can be known in advance, and it can be enumerated multiple times.
+    /// </summary>
+    /// <typeparam name="T">The type of elements being enumerated.</typeparam>
+    private class DeferredReadOnlyCollection<T> : IReadOnlyCollection<T>
+    {
+        private readonly Func<IEnumerable<T>> enumerableGenerator;
+
+        public DeferredReadOnlyCollection(int count, Func<IEnumerable<T>> enumerableGenerator)
         {
-            this.addedEvents.Clear();
+            this.enumerableGenerator = enumerableGenerator;
-            this.removedEvents.Clear();
+            this.Count = count;
-            this.changedEvents.Clear();
-            this.movedEvents.Clear();
         }
+
+        public int Count { get; }
+
+        public IEnumerator<T> GetEnumerator() => this.enumerableGenerator().GetEnumerator();
+
+        IEnumerator IEnumerable.GetEnumerator() => this.enumerableGenerator().GetEnumerator();
     }
 }
 
@@ -313,10 +325,10 @@ internal class GameInventoryPluginScoped : IDisposable, IServiceType, IGameInven
     public event IGameInventory.InventoryChangedDelegate? ItemRemoved;
 
     /// <inheritdoc/>
-    public event IGameInventory.InventoryChangedDelegate? ItemMoved;
+    public event IGameInventory.InventoryChangedDelegate? ItemChanged;
 
     /// <inheritdoc/>
-    public event IGameInventory.InventoryChangedDelegate? ItemChanged;
+    public event IGameInventory.InventoryChangedDelegate? ItemMoved;
 
     /// <inheritdoc/>
     public void Dispose()
@@ -325,15 +337,15 @@ internal class GameInventoryPluginScoped : IDisposable, IServiceType, IGameInven
         this.gameInventoryService.InventoryChangedRaw -= this.OnInventoryChangedRawForward;
         this.gameInventoryService.ItemAdded -= this.OnInventoryItemAddedForward;
         this.gameInventoryService.ItemRemoved -= this.OnInventoryItemRemovedForward;
-        this.gameInventoryService.ItemMoved -= this.OnInventoryItemMovedForward;
         this.gameInventoryService.ItemChanged -= this.OnInventoryItemChangedForward;
+        this.gameInventoryService.ItemMoved -= this.OnInventoryItemMovedForward;
 
         this.InventoryChanged = null;
         this.InventoryChangedRaw = null;
         this.ItemAdded = null;
         this.ItemRemoved = null;
-        this.ItemMoved = null;
         this.ItemChanged = null;
+        this.ItemMoved = null;
     }
 
     private void OnInventoryChangedForward(IReadOnlyCollection<InventoryEventArgs> events)
@@ -348,9 +360,9 @@ internal class GameInventoryPluginScoped : IDisposable, IServiceType, IGameInven
     private void OnInventoryItemRemovedForward(GameInventoryEvent type, InventoryEventArgs data)
         => this.ItemRemoved?.Invoke(type, data);
 
-    private void OnInventoryItemMovedForward(GameInventoryEvent type, InventoryEventArgs data)
-        => this.ItemMoved?.Invoke(type, data);
-
     private void OnInventoryItemChangedForward(GameInventoryEvent type, InventoryEventArgs data)
         => this.ItemChanged?.Invoke(type, data);
+
+    private void OnInventoryItemMovedForward(GameInventoryEvent type, InventoryEventArgs data)
+        => this.ItemMoved?.Invoke(type, data);
 }

Dalamud/Game/Inventory/GameInventoryEvent.cs

@@ -3,7 +3,6 @@
 /// <summary>
 /// Class representing a item's changelog state.
 /// </summary>
-[Flags]
 public enum GameInventoryEvent
 {
     /// <summary>
@@ -15,20 +14,20 @@ public enum GameInventoryEvent
     /// <summary>
     /// Item was added to an inventory.
     /// </summary>
-    Added = 1 << 0,
+    Added = 1,
 
     /// <summary>
     /// Item was removed from an inventory.
     /// </summary>
-    Removed = 1 << 1,
+    Removed = 2,
 
     /// <summary>
     /// Properties are changed for an item in an inventory.
     /// </summary>
-    Changed = 1 << 2,
+    Changed = 3,
 
     /// <summary>
     /// Item has been moved, possibly across different inventories.
     /// </summary>
-    Moved = 1 << 3,
+    Moved = 4,
 }

Dalamud/Game/Inventory/GameInventoryItem.cs

@@ -12,11 +12,6 @@ namespace Dalamud.Game.Inventory;
 [StructLayout(LayoutKind.Explicit, Size = StructSizeInBytes)]
 public unsafe struct GameInventoryItem : IEquatable<GameInventoryItem>
 {
-    /// <summary>
-    /// An empty instance of <see cref="GameInventoryItem"/>.
-    /// </summary>
-    internal static readonly GameInventoryItem Empty = default;
-
     /// <summary>
     /// The actual data.
     /// </summary>
@@ -119,7 +114,7 @@ public unsafe struct GameInventoryItem : IEquatable<GameInventoryItem>
     /// <summary>
     /// Gets the glamour id for this item.
     /// </summary>
-    public uint GlmaourId => this.InternalItem.GlamourID;
+    public uint GlamourId => this.InternalItem.GlamourID;
 
     /// <summary>
     /// Gets the items crafter's content id.
@@ -163,6 +158,6 @@ public unsafe struct GameInventoryItem : IEquatable<GameInventoryItem>
     /// <inheritdoc cref="object.ToString"/>
     public override string ToString() =>
         this.IsEmpty
-            ? "<Empty>"
+            ? "no item"
-            : $"Item #{this.ItemId} at slot {this.InventorySlot} in {this.ContainerType}";
+            : $"item #{this.ItemId} at slot {this.InventorySlot} in {this.ContainerType}";
 }

Dalamud/Game/Inventory/InventoryChangeArgsTypes/InventoryEventArgs.cs

@@ -1,13 +1,12 @@
-using System.Diagnostics.CodeAnalysis;
+namespace Dalamud.Game.Inventory.InventoryChangeArgsTypes;
-
-namespace Dalamud.Game.Inventory.InventoryChangeArgsTypes;
 
 /// <summary>
 /// Abstract base class representing inventory changed events.
 /// </summary>
-[SuppressMessage("StyleCop.CSharp.OrderingRules", "SA1206:Declaration keywords should follow order", Justification = "It literally says <access modifiers>, <static>, and then <all other keywords>. required is not an access modifier.")]
 public abstract class InventoryEventArgs
 {
+    private readonly GameInventoryItem item;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="InventoryEventArgs"/> class.
     /// </summary>
@@ -16,7 +15,7 @@ public abstract class InventoryEventArgs
     protected InventoryEventArgs(GameInventoryEvent type, in GameInventoryItem item)
     {
         this.Type = type;
-        this.Item = item;
+        this.item = item;
     }
 
     /// <summary>
@@ -28,7 +27,10 @@ public abstract class InventoryEventArgs
     /// Gets the item associated with this event.
     /// <remarks><em>This is a copy of the item data.</em></remarks>
     /// </summary>
-    public GameInventoryItem Item { get; }
+    // impl note: we return a ref readonly view, to avoid making copies every time this property is accessed.
+    // see: https://devblogs.microsoft.com/premier-developer/avoiding-struct-and-readonly-reference-performance-pitfalls-with-errorprone-net/
+    // "Consider using ref readonly locals and ref return for library code"
+    public ref readonly GameInventoryItem Item => ref this.item;
 
     /// <inheritdoc/>
     public override string ToString() => $"<{this.Type}> ({this.Item})";

Dalamud/Game/Inventory/InventoryChangeArgsTypes/InventoryItemChangedArgs.cs

@@ -6,6 +6,8 @@
 /// </summary>
 public class InventoryItemChangedArgs : InventoryEventArgs
 {
+    private readonly GameInventoryItem oldItemState;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="InventoryItemChangedArgs"/> class.
     /// </summary>
@@ -14,7 +16,7 @@ public class InventoryItemChangedArgs : InventoryEventArgs
     internal InventoryItemChangedArgs(in GameInventoryItem oldItem, in GameInventoryItem newItem)
         : base(GameInventoryEvent.Changed, newItem)
     {
-        this.OldItemState = oldItem;
+        this.oldItemState = oldItem;
     }
 
     /// <summary>
@@ -29,6 +31,8 @@ public class InventoryItemChangedArgs : InventoryEventArgs
 
     /// <summary>
     /// Gets the state of the item from before it was changed.
+    /// <remarks><em>This is a copy of the item data.</em></remarks>
     /// </summary>
-    public GameInventoryItem OldItemState { get; init; }
+    // impl note: see InventoryEventArgs.Item.
+    public ref readonly GameInventoryItem OldItemState => ref this.oldItemState;
 }

Dalamud/Game/Inventory/InventoryChangeArgsTypes/InventoryItemMovedArgs.cs

@@ -1,11 +1,8 @@
-using System.Diagnostics.CodeAnalysis;
+namespace Dalamud.Game.Inventory.InventoryChangeArgsTypes;
-
-namespace Dalamud.Game.Inventory.InventoryChangeArgsTypes;
 
 /// <summary>
 /// Represents the data associated with an item being moved from one inventory and added to another.
 /// </summary>
-[SuppressMessage("StyleCop.CSharp.OrderingRules", "SA1206:Declaration keywords should follow order", Justification = "It literally says <access modifiers>, <static>, and then <all other keywords>. required is not an access modifier.")]
 public class InventoryItemMovedArgs : InventoryEventArgs
 {
     /// <summary>