From 4342a84884a841e4226a6dbc358a2af4ad5489de Mon Sep 17 00:00:00 2001
From: Leon Friedrich <60421075+ElectroJr@users.noreply.github.com>
Date: Mon, 22 Nov 2021 14:50:43 +1300
Subject: [PATCH] Fix ItemSlots error (#5439)

* fix duplicate slots

* fix comment

* comments
---
 .../Containers/ItemSlot/ItemSlotsComponent.cs | 24 ++++++++++++++++---
 .../Containers/ItemSlot/ItemSlotsSystem.cs    |  4 +++-
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/Content.Shared/Containers/ItemSlot/ItemSlotsComponent.cs b/Content.Shared/Containers/ItemSlot/ItemSlotsComponent.cs
index 0c450f8a6f..25b4b2ba8f 100644
--- a/Content.Shared/Containers/ItemSlot/ItemSlotsComponent.cs
+++ b/Content.Shared/Containers/ItemSlot/ItemSlotsComponent.cs
@@ -23,9 +23,27 @@ namespace Content.Shared.Containers.ItemSlots
     {
         public override string Name => "ItemSlots";
 
-        [ViewVariables]
-        [DataField("slots")]
-        public Dictionary<string, ItemSlot> Slots = new();
+        /// <summary>
+        ///     The dictionary that stores all of the item slots whose interactions will be managed by the <see
+        ///     cref="ItemSlotsSystem"/>.
+        /// </summary>
+        [DataField("slots", readOnly:true)]
+        public readonly Dictionary<string, ItemSlot> Slots = new();
+
+        // There are two ways to use item slots:
+        //
+        // #1 - Give your component an ItemSlot datafield, and add/remove the item slot through the ItemSlotsSystem on
+        // component init/remove.
+        //
+        // #2 - Give your component a key string datafield, and make sure that every entity with that component also has
+        // an ItemSlots component with a matching key. Then use ItemSlots system to get the slot with this key whenever
+        // you need it, or just get a reference to the slot on init and store it. This is how generic entity containers
+        // are usually used.
+        //
+        // In order to avoid #1 leading to duplicate slots when saving a map, the Slots dictionary is a read-only
+        // datafield. This means that if your system/component dynamically changes the item slot (e.g., updating
+        // whitelist or whatever), you should use #1. Alternatively: split the Slots dictionary here into two: one
+        // datafield, one that is actually used by the ItemSlotsSystem for keeping track of slots.
     }
 
     [Serializable, NetSerializable]
diff --git a/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs b/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs
index 9e2cabf784..e6095ad0d1 100644
--- a/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs
+++ b/Content.Shared/Containers/ItemSlot/ItemSlotsSystem.cs
@@ -8,6 +8,7 @@ using Robust.Shared.GameObjects;
 using Robust.Shared.GameStates;
 using Robust.Shared.IoC;
 using Robust.Shared.Localization;
+using Robust.Shared.Log;
 using Robust.Shared.Player;
 using Robust.Shared.Utility;
 using System.Collections.Generic;
@@ -74,7 +75,8 @@ namespace Content.Shared.Containers.ItemSlots
         {
             var itemSlots = EntityManager.EnsureComponent<ItemSlotsComponent>(uid);
             slot.ContainerSlot = ContainerHelpers.EnsureContainer<ContainerSlot>(itemSlots.Owner, id);
-            DebugTools.Assert(!itemSlots.Slots.ContainsKey(id));
+            if (itemSlots.Slots.ContainsKey(id))
+                Logger.Error($"Duplicate item slot key. Entity: {itemSlots.Owner.Name} ({uid}), key: {id}");
             itemSlots.Slots[id] = slot;
         }