Formalize inventory system with Item, ItemStack, and ItemSlot classes

[Copilot]

The inventory system used a flat Dictionary<ItemType, int> structure. This PR formalizes the concept hierarchy: Item → ItemStack → ItemSlot → Inventory, enabling item stacks to exist as world drops and providing extensibility for future features (stack limits, item metadata, equipment slots).

Architecture

Before:

private Dictionary<ItemType, int> items;

After:

private List<ItemSlot> slots;  // Each slot contains an optional ItemStack of Items

Changes

• Item - Represents an item type with null-safe comparison
• ItemStack - Manages quantity for stackable items, supports world drops
• ItemSlot - Container for ItemStack with empty state management
• ItemDrop - Entity subclass rendering ItemStacks in the world (colored spheres based on type)
• Inventory - Refactored internals to slot-based architecture, public API unchanged

Backward Compatibility

All existing Inventory methods (getNumItems, addItem, removeItem, etc.) work identically. No call sites require changes.

Example Usage

// Create a world drop
ItemStack drop = new ItemStack(ItemType.COIN, 50);
ItemDrop entity = new ItemDrop(position, drop);

// Access slots directly for advanced use cases
foreach (ItemSlot slot in inventory.getSlots()) {
    if (!slot.isEmpty()) {
        ItemStack stack = slot.getItemStack();
        // Handle stack...
    }
}

Tests

Added 25+ test cases covering Item, ItemStack, ItemSlot classes and enhanced Inventory tests for edge cases (null handling, negative quantities, slot management).

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Improved Inventory System</issue_title>
<issue_description>The Inventory class should contain a list of item slots, which can contain an item stack, which can contain items.

This will involve formalizing a lot of concepts & creating the following classes:

• ItemSlot
• ItemStack
• Item

It should be possible for item stacks to exist in the world as drops, as well.</issue_description>

Comments on the Issue (you are @copilot in this section)

• Fixes Improved Inventory System #174

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[Copilot]

Pull request overview

This PR refactors Beyond Nations’ inventory from a flat Dictionary<ItemType,int> into a structured model (Item → ItemStack → ItemSlot → Inventory) and adds a world-drop entity for item stacks.

Changes:

• Introduces Item, ItemStack, and ItemSlot domain classes and wires Inventory to use slot-based storage.
• Adds ItemDrop as a new Entity type to represent item stacks in the world.
• Expands the test suite and adds documentation for the new inventory architecture.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
src/c#/main/inventory/Inventory.cs	Refactors inventory internals to a List<ItemSlot> and adds getSlots()
src/c#/main/item/Item.cs	Adds base Item type wrapper
src/c#/main/item/ItemStack.cs	Adds stack abstraction around item + quantity
src/c#/main/item/ItemSlot.cs	Adds slot container for optional stacks
src/c#/main/entity/entities/ItemDrop.cs	Adds world entity representation for an ItemStack
src/c#/main/entity/EntityType.cs	Adds ITEM_DROP enum value
src/c#/tests/inventory/TestInventory.cs	Adds/extends inventory tests for new behaviors and slot access
src/c#/tests/item/TestItem.cs	Adds tests for Item
src/c#/tests/item/TestItemStack.cs	Adds tests for ItemStack behaviors
src/c#/tests/item/TestItemSlot.cs	Adds tests for ItemSlot behaviors
src/c#/tests/Tests.cs	Registers new item-related tests in the test runner
docs/INVENTORY_SYSTEM.md	Documents the new inventory system architecture and usage

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ItemDrop assumes itemStack is non-null (it’s dereferenced in getItemDropName and createGameObject). Add an explicit null check in the constructor (and throw a clear exception) to avoid hard-to-diagnose runtime crashes if a caller passes null.

Suggested change

	public ItemDrop(Vector3 position, ItemStack itemStack) : base(EntityType.ITEM_DROP, getItemDropName(itemStack)) {
	public ItemDrop(Vector3 position, ItemStack itemStack) : base(EntityType.ITEM_DROP, itemStack != null ? getItemDropName(itemStack) : "Invalid ItemDrop") {
	if (itemStack == null) {
	throw new System.ArgumentNullException(nameof(itemStack), "ItemDrop requires a non-null ItemStack.");
	}

[Copilot]

transferContentsOfInventory will behave badly (and can throw due to collection modification during foreach) if otherInventory is the same instance as this. Consider adding a guard (if (otherInventory == this) return; or throw) or copying the source slots before iterating.

[Copilot]

getSlots() returns the backing List<ItemSlot> directly, which allows callers to add/remove/reorder slots (or create duplicate item types). That can silently break invariants used by getNumItems/addItem/removeItem. Consider returning an IReadOnlyList<ItemSlot>/IEnumerable<ItemSlot> or a defensive copy so external code can enumerate slots without mutating the inventory structure.

Suggested change

	public List<ItemSlot> getSlots() {
	return slots;
	public IReadOnlyList<ItemSlot> getSlots() {
	return slots.AsReadOnly();

[Copilot]

Unity scripts in this repo appear to be checked in with .meta files (e.g., ItemType.cs.meta, Inventory.cs.meta), but the new scripts under src/c#/main/item/ don’t have .meta files. Please add the corresponding .meta files to avoid GUID churn and broken references in Unity projects.

[Copilot]

The new test scripts under src/c#/tests/item/ don’t include .meta files, while other tests do (e.g., TestInventory.cs.meta). In a Unity project this can cause GUID changes and reference issues—please add the generated .meta files for these new test assets.

[Copilot]

getNumItems returns the quantity from the first slot that matches itemType. If the inventory ever contains multiple stacks of the same type (possible if getSlots() is mutated externally, or if future stack limits require splitting across slots), this will undercount. Either enforce “at most one slot per item type” or sum quantities across all matching slots.

Suggested change

	foreach (ItemSlot slot in slots) {
	if (slot.hasItemOfType(itemType)) {
	return slot.getQuantity();
	}
	}
	return 0;
	int total = 0;
	foreach (ItemSlot slot in slots) {
	if (slot.hasItemOfType(itemType)) {
	total += slot.getQuantity();
	}
	}
	return total;

[Copilot]

The constructor comments/structure imply a dedicated “coins slot” and fixed slot-to-item-type mapping, but addItem will fill the first empty slot (so an empty coin slot can later become WOOD/STONE/etc.). Either enforce stable per-type slots (e.g., by indexing by ItemType) or update the comments/docs/tests so they don’t imply reserved ordering.

[Copilot]

removeItem clears the slot when the quantity becomes <= 0, which changes semantics vs the previous dictionary implementation (where counts could go negative and getNumItems would reflect that). If callers rely on exact counts (including underflow), avoid clearing and preserve the resulting quantity (or clamp to 0 consistently and update the backward-compatibility claim/docs).

[Copilot]

isEmpty() can return true while itemStack remains non-null (e.g., quantity == 0), so getItemStack() may return a non-null stack for an “empty” slot. This is easy to misuse and conflicts with the “optional ItemStack” model. Consider normalizing empty stacks to null (e.g., clear when setting or when quantity becomes <= 0) or make getItemStack() return null when isEmpty() is true.

[Copilot]

ItemStack(Item item, int quantity) allows item to be null, but getItemType() and canStackWith() dereference item, leading to a guaranteed NullReferenceException for a null input. Either validate and throw an ArgumentNullException in the constructor, or make methods null-safe if null items are intended.

Suggested change

	public ItemStack(Item item, int quantity) {
	public ItemStack(Item item, int quantity) {
	if (item == null) {
	throw new System.ArgumentNullException(nameof(item));
	}