Inventory refuses non-positive counts; unknown saved items kept; factories follow reloads

.gitignore

@@ -1,37 +1,37 @@
-# Godot 4.x
+# Godot 4.x
-.godot/
+.godot/
-.idea/
+.idea/
-sdk/
+sdk/
-bin/
+bin/
-obj/
+obj/
-
+
-#ai
+#ai
-.omo/
+.omo/
-.opencode/
+.opencode/
-
+
-gen/
+gen/
-obj/
+obj/
-bin/
+bin/
-
+
-*.tres~
+*.tres~
-*.tscn~
+*.tscn~
-
+
-# GDScript LSP
+# GDScript LSP
-.godot/
+.godot/
-
+
-# Export
+# Export
-*.exe
+*.exe
-*.dmg
+*.dmg
-*.apk
+*.apk
-*.aab
+*.aab
-*.pck
+*.pck
-*.zip
+*.zip
-*.log
+*.log
-
+
-# OS
+# OS
-.DS_Store
+.DS_Store
-Thumbs.db
+Thumbs.db
-
+
-# Editor
+# Editor
-*.import
+*.import
-*.godot.uid
+*.godot.uid

manifest.json

@@ -8,6 +8,9 @@
   "sources": [
     "src/**/*.cs"
   ],
+  "test_sources": [
+    "tests/*.cs"
+  ],
   "depends": ["shell2d_base>=1", "ui_base>=1"],
   "test_suites": [
     {"id": "inventory", "activate": ["--itest=inventory"]}

src/Inventory.cs

@@ -30,6 +30,14 @@ namespace Cthangover.Core.Items
 	{
 		/// <summary>Ordered slot list; order drives UI layout and empty slots are removed on zero stacks.</summary>
 		public List<IItemContainer> Items { get; set; } = new();
+
+		/// <summary>
+		/// Stacks a loaded save names but no loaded mod defines (the mod was
+		/// removed or the id renamed). They are no usable stacks — every
+		/// container in <see cref="Items"/> has an item — but the items slot
+		/// writes them back, so they return with their mod.
+		/// </summary>
+		public List<Settings.CItem> UnresolvedItems { get; } = new();
 		/// <summary>Fires on every successful add/remove with (itemId, signed delta); also mirrored as <see cref="Events.InventoryChangedEvent"/> on the ModEventBus.</summary>
 		public event Action<string, int> Change;
 
@@ -74,7 +82,7 @@ namespace Cthangover.Core.Items
 		{
 			if (string.IsNullOrWhiteSpace(id))
 				return false;
-			return Items.Any(o => o.Item.ID == id);
+			return Items.Any(o => o.Item != null && o.Item.ID == id);
 		}
 
 		/// <summary>Returns the slot holding the given item, or <c>null</c> (null-safe).</summary>
@@ -124,6 +132,12 @@ namespace Cthangover.Core.Items
 		/// </summary>
 		public void Add(string id, int count = 1)
 		{
+			if (count <= 0)
+			{
+				GameLogger.Log("INVENTORY", $"Add('{id}', {count}): count must be positive, ignored", LogLevel.Warning);
+				return;
+			}
+
 			var container = TryGet(id);
 			if (container != null)
 			{
@@ -161,6 +175,10 @@ namespace Cthangover.Core.Items
 		/// </summary>
 		public bool Remove(string id, int count = 1)
 		{
+			// A negative count would pass the stock check and grow the stack.
+			if (count <= 0)
+				return false;
+
 			var container = TryGet(id);
 			if (container == null)
 				return false;

src/ItemActionFactory.cs

@@ -1,63 +1,63 @@
-using System.Collections.Generic;
+using System.Collections.Generic;
-using Cthangover.Core.Items;
+using Cthangover.Core.Items;
-
+
-namespace Cthangover.Core.Factories.Impls
+namespace Cthangover.Core.Factories.Impls
-{
+{
-    /// <summary>
+    /// <summary>
-    /// Registry of item action behaviours, indexed by <c>ID</c>. It discovers
+    /// Registry of item action behaviours, indexed by <c>ID</c>. It discovers
-    /// nothing itself: C# actions of every mod arrive through
+    /// nothing itself: C# actions of every mod arrive through
-    /// <c>ItemActionConsumer</c> as the kernel registers their assembly and
+    /// <c>ItemActionConsumer</c> as the kernel registers their assembly and
-    /// leave with their mod, and GDScript-declared actions arrive through
+    /// leave with their mod, and GDScript-declared actions arrive through
-    /// <c>ItemActionBridge</c>. A mod therefore introduces a new item action
+    /// <c>ItemActionBridge</c>. A mod therefore introduces a new item action
-    /// simply by containing a public class that implements the interface.
+    /// simply by containing a public class that implements the interface.
-    ///
+    ///
-    /// The <c>"null"</c> ID string is treated as a no-op action (the
+    /// The <c>"null"</c> ID string is treated as a no-op action (the
-    /// fallback for items that don't have an on-use effect), avoiding
+    /// fallback for items that don't have an on-use effect), avoiding
-    /// the need for a sentinel <c>NullItemAction</c> class.
+    /// the need for a sentinel <c>NullItemAction</c> class.
-    /// <para/>
+    /// <para/>
-    /// The single instance lives in this mod assembly as a static lazy
+    /// The single instance lives in this mod assembly as a static lazy
-    /// singleton; the static facade stays as the public API.
+    /// singleton; the static facade stays as the public API.
-    /// </summary>
+    /// </summary>
-    public class ItemActionFactory
+    public class ItemActionFactory
-    {
+    {
-        private static ItemActionFactory _instance;
+        private static ItemActionFactory _instance;
-
+
-        /// <summary>Static lazy singleton; kept as the public API.</summary>
+        /// <summary>Static lazy singleton; kept as the public API.</summary>
-        public static ItemActionFactory Instance => _instance ??= new ItemActionFactory();
+        public static ItemActionFactory Instance => _instance ??= new ItemActionFactory();
-
+
-        private readonly IDictionary<string, IItemAction> data = new Dictionary<string, IItemAction>();
+        private readonly IDictionary<string, IItemAction> data = new Dictionary<string, IItemAction>();
-
+
-        internal ItemActionFactory()
+        internal ItemActionFactory()
-        {
+        {
-        }
+        }
-
+
-        /// <summary>Registers a single action under its <see cref="IItemAction.ID"/>; the first action of an id wins, null/empty ids are ignored.</summary>
+        /// <summary>Registers a single action under its <see cref="IItemAction.ID"/>; the first action of an id wins, null/empty ids are ignored.</summary>
-        public void Register(IItemAction action)
+        public void Register(IItemAction action)
-        {
+        {
-            if (action != null && !string.IsNullOrEmpty(action.ID) && !data.ContainsKey(action.ID))
+            if (action != null && !string.IsNullOrEmpty(action.ID) && !data.ContainsKey(action.ID))
-                data[action.ID] = action;
+                data[action.ID] = action;
-        }
+        }
-
+
-        /// <summary>
+        /// <summary>
-        /// Removes exactly <paramref name="action"/> (its mod is being unloaded);
+        /// Removes exactly <paramref name="action"/> (its mod is being unloaded);
-        /// an id owned by another action instance is left alone.
+        /// an id owned by another action instance is left alone.
-        /// </summary>
+        /// </summary>
-        public void Unregister(IItemAction action)
+        public void Unregister(IItemAction action)
-        {
+        {
-            if (action != null && !string.IsNullOrEmpty(action.ID) && data.TryGetValue(action.ID, out var registered) && registered == action)
+            if (action != null && !string.IsNullOrEmpty(action.ID) && data.TryGetValue(action.ID, out var registered) && registered == action)
-                data.Remove(action.ID);
+                data.Remove(action.ID);
-        }
+        }
-
+
-        /// <summary>
+        /// <summary>
-        /// Resolves the behaviour registered under <paramref name="id"/>.
+        /// Resolves the behaviour registered under <paramref name="id"/>.
-        /// The special id <c>"null"</c> (or an empty value) is an explicit
+        /// The special id <c>"null"</c> (or an empty value) is an explicit
-        /// no-action marker and returns <c>null</c>, as does an unknown id.
+        /// no-action marker and returns <c>null</c>, as does an unknown id.
-        /// </summary>
+        /// </summary>
-        public IItemAction Get(string id)
+        public IItemAction Get(string id)
-        {
+        {
-            if (string.IsNullOrWhiteSpace(id) || id == "null")
+            if (string.IsNullOrWhiteSpace(id) || id == "null")
-                return null;
+                return null;
-            return data[id];
+            return data[id];
-        }
+        }
-    }
+    }
-}
+}

src/ItemFactory.cs

@@ -1,6 +1,4 @@
-using System.Collections.Generic;
 using Cthangover.Core.Items;
-using Cthangover.Core.Mods;
 using Cthangover.Core.Mods.Resolvers;
 using Cthangover.Core.Utils;
 
@@ -17,8 +15,6 @@ namespace Cthangover.Core.Factories.Impls
         /// <summary>Static lazy singleton; kept as the public API.</summary>
         public static ItemFactory Instance => _instance ??= new ItemFactory();
 
-        private Dictionary<string, ItemInfo> items;
-
         /// <summary>
         /// Builds a fresh <see cref="Item"/> from the collected <see cref="ItemInfo"/>,
         /// resolving the sprite through <see cref="ItemSpriteFactory"/>, the on-use
@@ -31,14 +27,9 @@ namespace Cthangover.Core.Factories.Impls
             if (string.IsNullOrEmpty(id))
                 return null;
 
-            var infos = items ?? Entities.Collect<ItemInfo>();
+            // Entities.Collect is cached by the entity resolver until the next
-
+            // mod reload, so reading it every time serves the current data.
-            // The registry refuses mod-data access until Boot.IsReady, so
+            var infos = Entities.Collect<ItemInfo>();
-            // this snapshot can never be collected from a half-built mod set;
-            // the cached field is reset when ModManager.Reload rebuilds the
-            // registry.
-            if (ModManager.Instance.IsInitialized)
-                items ??= infos;
 
             if (!infos.TryGetValue(id, out var info))
             {

src/ItemsSaveable.cs

@@ -4,6 +4,7 @@ using Cthangover.Core.Factories.Impls;
 using Cthangover.Core.Items;
 using Cthangover.Core.Mods;
 using Cthangover.Core.Settings;
+using Cthangover.Core.Utils;
 
 namespace Cthangover.Mods.Items
 {
@@ -28,8 +29,13 @@ namespace Cthangover.Mods.Items
             if (runtime == null)
                 return;
 
-            slot.Data[InventoryBlob] = new DataBlob(1, runtime.Inventory.Items.Select(o =>
+            var items = runtime.Inventory.Items
-                new CItem { ID = o.Item.ID, Count = o.Count }).ToList());
+                .Where(o => o.Item != null)
+                .Select(o => new CItem { ID = o.Item.ID, Count = o.Count })
+                .ToList();
+            if (runtime.Inventory is Inventory inventory)
+                items.AddRange(inventory.UnresolvedItems);
+            slot.Data[InventoryBlob] = new DataBlob(1, items);
             slot.Data[RecipesBlob] = new DataBlob(1, runtime.RecipeData.Data.ToList());
         }
 
@@ -43,11 +49,21 @@ namespace Cthangover.Mods.Items
 
             if (slot.Data.TryGetValue(InventoryBlob, out var invBlob))
             {
-                var items = invBlob.ReadJson<List<CItem>>();
+                var containers = new List<IItemContainer>();
-                runtime.Inventory.Items = items?.Select(o =>
+                var unresolved = (runtime.Inventory as Inventory)?.UnresolvedItems;
-                    (IItemContainer)new ItemContainer
+                unresolved?.Clear();
-                        { Item = ItemFactory.Instance.Get(o.ID), Count = o.Count }).ToList()
+                foreach (var saved in invBlob.ReadJson<List<CItem>>() ?? new List<CItem>())
-                    ?? new List<IItemContainer>();
+                {
+                    var item = ItemFactory.Instance.Get(saved.ID);
+                    if (item != null)
+                    {
+                        containers.Add(new ItemContainer { Item = item, Count = saved.Count });
+                        continue;
+                    }
+                    GameLogger.Log("INVENTORY", $"saved item '{saved.ID}' x{saved.Count} is not defined by any loaded mod; kept in the save", LogLevel.Warning);
+                    unresolved?.Add(saved);
+                }
+                runtime.Inventory.Items = containers;
             }
 
             if (slot.Data.TryGetValue(RecipesBlob, out var recipeBlob))

src/RecipeData.cs

@@ -1,50 +1,50 @@
-using System.Collections.Generic;
+using System.Collections.Generic;
-using System.Linq;
+using System.Linq;
-using Cthangover.Core.Factories.Impls;
+using Cthangover.Core.Factories.Impls;
-using Cthangover.Core.Items;
+using Cthangover.Core.Items;
-
+
-namespace Cthangover.Core.Settings
+namespace Cthangover.Core.Settings
-{
+{
-    /// <summary>
+    /// <summary>
-    /// Tracks which crafting recipes the player has unlocked. Internally
+    /// Tracks which crafting recipes the player has unlocked. Internally
-    /// a <see cref="HashSet{String}"/> of recipe IDs that provides <c>O(1)</c>
+    /// a <see cref="HashSet{String}"/> of recipe IDs that provides <c>O(1)</c>
-    /// lookup. Recipes can be queried by <see cref="IRecipe.WorkbenchType"/>
+    /// lookup. Recipes can be queried by <see cref="IRecipe.WorkbenchType"/>
-    /// via <see cref="GetRecipesByType"/>, which resolves each ID through
+    /// via <see cref="GetRecipesByType"/>, which resolves each ID through
-    /// <see cref="Cthangover.Core.Factories.Impls.RecipeFactory"/> and
+    /// <see cref="Cthangover.Core.Factories.Impls.RecipeFactory"/> and
-    /// filters the resulting <see cref="IRecipe"/> objects.
+    /// filters the resulting <see cref="IRecipe"/> objects.
-    /// </summary>
+    /// </summary>
-	public class RecipeData : IRecipeData
+	public class RecipeData : IRecipeData
-	{
+	{
-        /// <summary>Set of known recipe IDs (duplicate insertion is silently ignored).</summary>
+        /// <summary>Set of known recipe IDs (duplicate insertion is silently ignored).</summary>
-		public HashSet<string> Data { get; set; } = new();
+		public HashSet<string> Data { get; set; } = new();
-
+
-        /// <summary>
+        /// <summary>
-        /// Unlocks a recipe by ID. Returns <c>false</c> if the recipe
+        /// Unlocks a recipe by ID. Returns <c>false</c> if the recipe
-        /// was already known (duplicate).
+        /// was already known (duplicate).
-        /// </summary>
+        /// </summary>
-		public bool Add(string id)
+		public bool Add(string id)
-		{
+		{
-			return Data.Add(id);
+			return Data.Add(id);
-		}
+		}
-
+
-        /// <summary><c>true</c> if the recipe with the given ID is already unlocked.</summary>
+        /// <summary><c>true</c> if the recipe with the given ID is already unlocked.</summary>
-		public bool Has(string id)
+		public bool Has(string id)
-		{
+		{
-			return Data.Contains(id);
+			return Data.Contains(id);
-		}
+		}
-
+
-        /// <summary>
+        /// <summary>
-        /// Returns all known recipes that belong to the specified
+        /// Returns all known recipes that belong to the specified
-        /// <see cref="WorkbenchType"/>. Each ID is resolved via
+        /// <see cref="WorkbenchType"/>. Each ID is resolved via
-        /// <see cref="Cthangover.Core.Factories.Impls.RecipeFactory.Instance"/>
+        /// <see cref="Cthangover.Core.Factories.Impls.RecipeFactory.Instance"/>
-        /// and filtered; IDs that resolve to <c>null</c> are silently
+        /// and filtered; IDs that resolve to <c>null</c> are silently
-        /// dropped.
+        /// dropped.
-        /// </summary>
+        /// </summary>
-		public List<IRecipe> GetRecipesByType(WorkbenchType type)
+		public List<IRecipe> GetRecipesByType(WorkbenchType type)
-		{
+		{
-			return Data.Select(o => RecipeFactory.Instance.Get(o))
+			return Data.Select(o => RecipeFactory.Instance.Get(o))
-				.Where(o => o.WorkbenchType == type)
+				.Where(o => o.WorkbenchType == type)
-				.ToList();
+				.ToList();
-		}
+		}
-	}
+	}
-}
+}

src/RecipeFactory.cs

@@ -52,12 +52,13 @@ namespace Cthangover.Core.Factories.Impls
         /// </summary>
         private Dictionary<string, RecipeInfo> EnsureInfos()
         {
-            if (_allInfos != null)
+            // The resolver's dictionary identity is the data snapshot: a new one drops the built recipes.
-                return _allInfos;
-
             var infos = Entities.Collect<RecipeInfo>();
-            if (ModManager.Instance.IsInitialized)
+            if (!ReferenceEquals(infos, _allInfos))
+            {
                 _allInfos = infos;
+                _cache.Clear();
+            }
             return infos;
         }
 

src/ui/InventoryBagBehaviour.cs

@@ -1,41 +1,41 @@
-using System.Collections.Generic;
+using System.Collections.Generic;
-using Cthangover.Core.Items;
+using Cthangover.Core.Items;
-using Cthangover.Mods.Interface.Base.Lists.Impls;
+using Cthangover.Mods.Interface.Base.Lists.Impls;
-
+
-namespace Cthangover.Core.UI.Inventory
+namespace Cthangover.Core.UI.Inventory
-{
+{
-    /// <summary>
+    /// <summary>
-    /// Grid-based inventory display using ColumnCellListWidget. Binds an
+    /// Grid-based inventory display using ColumnCellListWidget. Binds an
-    /// external List&lt;IItemContainer&gt;. The List property setter auto-refreshes
+    /// external List&lt;IItemContainer&gt;. The List property setter auto-refreshes
-    /// the UI — intended for data-driven updates where the inventory source
+    /// the UI — intended for data-driven updates where the inventory source
-    /// pushes a new list reference rather than items mutating in place.
+    /// pushes a new list reference rather than items mutating in place.
-    /// </summary>
+    /// </summary>
-    public partial class InventoryBagBehaviour : ColumnCellListWidget<InventoryItemBehaviour, IItemContainer>
+    public partial class InventoryBagBehaviour : ColumnCellListWidget<InventoryItemBehaviour, IItemContainer>
-    {
+    {
-        private List<IItemContainer> list;
+        private List<IItemContainer> list;
-
+
-        /// <summary>
+        /// <summary>
-        /// Bound inventory data source. Setting this property immediately rebuilds
+        /// Bound inventory data source. Setting this property immediately rebuilds
-        /// the grid display via <see cref="Refresh"/>. The entire grid is
+        /// the grid display via <see cref="Refresh"/>. The entire grid is
-        /// reconstructed from the new list reference.
+        /// reconstructed from the new list reference.
-        /// </summary>
+        /// </summary>
-        public List<IItemContainer> List
+        public List<IItemContainer> List
-        {
+        {
-            get => list;
+            get => list;
-            set
+            set
-            {
+            {
-                list = value;
+                list = value;
-                Refresh();
+                Refresh();
-            }
+            }
-        }
+        }
-
+
-        /// <summary>
+        /// <summary>
-        /// Provides the current <see cref="List"/> as the data collection for the
+        /// Provides the current <see cref="List"/> as the data collection for the
-        /// <see cref="ColumnCellListWidget{TItem, TModel}"/> base.
+        /// <see cref="ColumnCellListWidget{TItem, TModel}"/> base.
-        /// </summary>
+        /// </summary>
-        public override ICollection<IItemContainer> CreateModels()
+        public override ICollection<IItemContainer> CreateModels()
-        {
+        {
-            return list;
+            return list;
-        }
+        }
-    }
+    }
-}
+}

tests/InventoryTests.cs

@@ -0,0 +1,36 @@
+namespace Cthangover.Mods.Items.Tests
+{
+    using System.Linq;
+    using Cthangover.Core.Items;
+    using Cthangover.Core.Mods.Resolvers;
+    using Cthangover.Core.Mods.Testing;
+
+    /// <summary>
+    /// Stack arithmetic of <see cref="Inventory"/>: a count that is not
+    /// positive is no operation at all — a negative removal must not grow a
+    /// stack and an addition must not create an empty or negative one.
+    /// </summary>
+    public class InventoryTests
+    {
+        [ModTest("inventory/non_positive_counts_change_nothing")]
+        static void NonPositiveCountsChangeNothing(ModTestContext ctx)
+        {
+            var id = Entities.Collect<ItemInfo>().Keys.First();
+            var inventory = new Inventory();
+            inventory.Add(id, 2);
+            var before = inventory.CheckCount(id);
+
+            ModAssert.False(inventory.Remove(id, -5), "negative removal is refused");
+            ModAssert.False(inventory.Remove(id, 0), "zero removal is refused");
+            inventory.Add(id, -3);
+            inventory.Add(id, 0);
+            ModAssert.Equal(before, inventory.CheckCount(id), "stack count after non-positive operations");
+
+            var other = Entities.Collect<ItemInfo>().Keys.FirstOrDefault(k => !inventory.HasItem(k));
+            if (other == null)
+                return;
+            inventory.Add(other, 0);
+            ModAssert.False(inventory.HasItem(other), "zero addition creates no stack");
+        }
+    }
+}

tests/ItemsSaveableTests.cs

@@ -0,0 +1,54 @@
+namespace Cthangover.Mods.Items.Tests
+{
+    using System.Collections.Generic;
+    using System.Linq;
+    using Cthangover.Core.Items;
+    using Cthangover.Core.Mods.Resolvers;
+    using Cthangover.Core.Mods.Testing;
+    using Cthangover.Core.Settings;
+
+    /// <summary>
+    /// The items slot across a change of the game's content: a save may name
+    /// an item no loaded mod defines any more (its mod was removed or the id
+    /// renamed). Loading it must keep the rest of the inventory usable, and
+    /// the next save must still be written — with the unknown stack kept, so
+    /// it comes back with its mod.
+    /// </summary>
+    public class ItemsSaveableTests
+    {
+        private const string UnknownId = "__test/no_such_item";
+
+        [ModTest("save/unknown_item_survives_load_and_save")]
+        static void UnknownItemSurvivesLoadAndSave(ModTestContext ctx)
+        {
+            var known = Entities.Collect<ItemInfo>().Keys.First();
+            var runtime = GameData.Instance.Runtime;
+            var original = runtime.Inventory;
+            try
+            {
+                runtime.Inventory = new Inventory();
+                var slot = new ModSlot();
+                slot.Data["Inventory"] = new DataBlob(1, new List<CItem>
+                {
+                    new() { ID = known, Count = 4 },
+                    new() { ID = UnknownId, Count = 3 },
+                });
+                var saveable = new ItemsSaveable();
+
+                ModAssert.True(saveable.OnLoad(slot, out _), "slot with an unknown item loads");
+                ModAssert.Equal(4, runtime.Inventory.CheckCount(known), "known stack restored");
+                ModAssert.False(runtime.Inventory.HasItem(UnknownId), "unknown item is not a usable stack");
+
+                var saved = new ModSlot();
+                saveable.OnSave(saved);
+                var items = saved.Data["Inventory"].ReadJson<List<CItem>>();
+                ModAssert.Equal(4, items.Single(i => i.ID == known).Count, "known stack saved");
+                ModAssert.Equal(3, items.Single(i => i.ID == UnknownId).Count, "unknown stack kept in the save");
+            }
+            finally
+            {
+                runtime.Inventory = original;
+            }
+        }
+    }
+}