Addressed review comments

assets/common/items/debug/admin_black_hole.ron

@@ -9,5 +9,5 @@ ItemDef(
     ),
     quality: Debug,
     tags: [],
-    slots: 1,
+    slots: 900,
 )

common/src/comp/inventory/item/mod.rs

@@ -1439,6 +1439,29 @@ impl Item {
         self.update_item_state(ability_map, msm);
     }
 
+    /// If an item is stackable and has an amount greater than 1, creates a new
+    /// item with half the amount (rounded down), and decreases the amount of
+    /// the original item by the same quantity.
+    pub fn take_half(
+        &mut self,
+        ability_map: &AbilityMap,
+        msm: &MaterialStatManifest,
+    ) -> Option<Item> {
+        if self.is_stackable() && self.amount() > 1 {
+            let mut return_item = self.duplicate(ability_map, msm);
+            let returning_amount = self.amount() / 2;
+            self.decrease_amount(returning_amount).ok()?;
+            return_item.set_amount(returning_amount).expect(
+                "return_item.amount() = self.amount() / 2 < self.amount() (since self.amount() ≥ \
+                 1) ≤ self.max_amount() = return_item.max_amount(), since return_item is a \
+                 duplicate of item",
+            );
+            Some(return_item)
+        } else {
+            None
+        }
+    }
+
     #[cfg(test)]
     pub fn create_test_item_from_kind(kind: ItemKind) -> Self {
         let ability_map = &AbilityMap::load().read();

common/src/comp/inventory/mod.rs

@@ -562,6 +562,7 @@ impl Inventory {
     }
 
     /// Remove an item from an overflow slot
+    #[must_use = "Returned items will be lost if not used"]
     pub fn overflow_remove(&mut self, overflow_slot: usize) -> Option<Item> {
         if overflow_slot < self.overflow_items.len() {
             Some(self.overflow_items.remove(overflow_slot))
@@ -595,6 +596,7 @@ impl Inventory {
     }
 
     /// Takes half of the items from a slot in the inventory
+    #[must_use = "Returned items will be lost if not used"]
     pub fn take_half(
         &mut self,
         inv_slot_id: InvSlotId,
@@ -602,25 +604,15 @@ impl Inventory {
         msm: &MaterialStatManifest,
     ) -> Option<Item> {
         if let Some(Some(item)) = self.slot_mut(inv_slot_id) {
-            if item.is_stackable() && item.amount() > 1 {
+            item.take_half(ability_map, msm)
-                let mut return_item = item.duplicate(ability_map, msm);
+                .or(self.remove(inv_slot_id))
-                let returning_amount = item.amount() / 2;
-                item.decrease_amount(returning_amount).ok()?;
-                return_item.set_amount(returning_amount).expect(
-                    "return_item.amount() = item.amount() / 2 < item.amount() (since \
-                     item.amount() ≥ 1) ≤ item.max_amount() = return_item.max_amount(), since \
-                     return_item is a duplicate of item",
-                );
-                Some(return_item)
-            } else {
-                self.remove(inv_slot_id)
-            }
         } else {
             None
         }
     }
 
     /// Takes half of the items from an overflow slot
+    #[must_use = "Returned items will be lost if not used"]
     pub fn overflow_take_half(
         &mut self,
         overflow_slot: usize,
@@ -628,21 +620,8 @@ impl Inventory {
         msm: &MaterialStatManifest,
     ) -> Option<Item> {
         if let Some(item) = self.overflow_items.get_mut(overflow_slot) {
-            if item.is_stackable() && item.amount() > 1 {
+            item.take_half(ability_map, msm)
-                let mut return_item = item.duplicate(ability_map, msm);
+                .or(self.overflow_remove(overflow_slot))
-                let returning_amount = item.amount() / 2;
-                item.decrease_amount(returning_amount).ok()?;
-                return_item.set_amount(returning_amount).expect(
-                    "return_item.amount() = item.amount() / 2 < item.amount() (since \
-                     item.amount() ≥ 1) ≤ item.max_amount() = return_item.max_amount(), since \
-                     return_item is a duplicate of item",
-                );
-                Some(return_item)
-            } else if overflow_slot < self.overflow_items.len() {
-                Some(self.overflow_items.remove(overflow_slot))
-            } else {
-                None
-            }
         } else {
             None
         }

common/src/comp/inventory/slot.rs

@@ -107,16 +107,6 @@ pub enum ArmorSlot {
     Bag4,
 }
 
-impl Slot {
-    pub fn can_hold(self, item_kind: &ItemKind) -> bool {
-        match (self, item_kind) {
-            (Self::Inventory(_), _) => true,
-            (Self::Equip(slot), item_kind) => slot.can_hold(item_kind),
-            (Self::Overflow(_), _) => true,
-        }
-    }
-}
-
 impl EquipSlot {
     pub fn can_hold(self, item_kind: &ItemKind) -> bool {
         match (self, item_kind) {

server/src/persistence/character/conversions.rs

@@ -67,7 +67,7 @@ pub fn convert_items_to_database_items(
         .slots_with_id()
         .map(|(pos, item)| {
             (
-                serde_json::to_string(&pos).expect("failed to serialize InventorySlotPos"),
+                serde_json::to_string(&pos).expect("failed to serialize InvSlotId"),
                 item.as_ref(),
                 inventory_container_id,
             )
@@ -473,8 +473,6 @@ pub fn convert_inventory_from_database_items(
                 &|(inv, f_i): &mut (&mut Inventory, &mut FailedInserts), s| {
                     // Attempts first to access inventory if that slot exists there. If it does not
                     // it instead attempts to access failed inserts list.
-                    // Question for Sharp/XVar: Should this attempt to look in failed inserts list
-                    // first?
                     slot(s)
                         .ok()
                         .and_then(|slot| inv.slot_mut(slot))