From f47838ee9d17afed8d9fc9af948411faab368db8 Mon Sep 17 00:00:00 2001
From: Ben Wallis <atomyc@gmail.com>
Date: Sat, 12 Jun 2021 13:43:53 +0100
Subject: [PATCH] Improved inventory swap

---
 common/src/comp/inventory/mod.rs  | 21 ++++------------
 common/src/comp/inventory/test.rs | 40 +++++++++++++++++++++++++++----
 2 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/common/src/comp/inventory/mod.rs b/common/src/comp/inventory/mod.rs
index a08b7d1a0e..7152f130a2 100644
--- a/common/src/comp/inventory/mod.rs
+++ b/common/src/comp/inventory/mod.rs
@@ -710,15 +710,14 @@ impl Inventory {
         let unloaded_items = from_equip
             .map(|mut from_equip| {
                 // Unload any items held inside the previously equipped item
-                let items: Vec<Item> = from_equip.drain().collect();
+                let mut items: Vec<Item> = from_equip.drain().collect();
 
                 // Attempt to put the unequipped item in the same slot that the inventory item
                 // was in - if that slot no longer exists (because a large container was
-                // swapped for a smaller one) then push the item to the first free
-                // inventory slot instead.
+                // swapped for a smaller one) then we will attempt to push it to the inventory
+                // with the rest of the unloaded items.
                 if let Err(returned) = self.insert_at(inv_slot_id, from_equip) {
-                    self.push(returned)
-                        .expect("Unable to push to inventory, no slots (bug in can_swap()?)");
+                    items.insert(0, returned);
                 }
 
                 items
@@ -778,17 +777,7 @@ impl Inventory {
             return false;
         }
 
-        // If we're swapping an equipped item with an empty inventory slot, make
-        // sure  that there will be enough space in the inventory after any
-        // slots  granted by the item being unequipped have been removed.
-        if let Some(inv_slot) = self.slot(inv_slot_id) {
-            if inv_slot.is_none() && self.free_slots_minus_equipped_item(equip_slot) == 0 {
-                // No free inventory slots after slots provided by the equipped
-                //item are discounted
-                trace!("can_swap = false, no free slots minus item");
-                return false;
-            }
-        } else {
+        if self.slot(inv_slot_id).is_none() {
             debug!(
                 "can_swap = false, tried to swap into non-existent inventory slot: {:?}",
                 inv_slot_id
diff --git a/common/src/comp/inventory/test.rs b/common/src/comp/inventory/test.rs
index f2acbb1775..e75c923ce7 100644
--- a/common/src/comp/inventory/test.rs
+++ b/common/src/comp/inventory/test.rs
@@ -207,7 +207,7 @@ fn can_swap_equipped_bag_into_empty_inv_slot(
 }
 
 #[test]
-fn can_swap_equipped_bag_into_only_empty_slot_provided_by_itself_should_return_false() {
+fn can_swap_equipped_bag_into_only_empty_slot_provided_by_itself_should_return_true() {
     let mut inv = Inventory::new_empty();
 
     inv.replace_loadout_item(EquipSlot::Armor(ArmorSlot::Bag1), Some(get_test_bag(18)));
@@ -216,7 +216,7 @@ fn can_swap_equipped_bag_into_only_empty_slot_provided_by_itself_should_return_f
 
     let result = inv.can_swap(InvSlotId::new(15, 17), EquipSlot::Armor(ArmorSlot::Bag1));
 
-    assert!(!result);
+    assert!(result);
 }
 
 #[test]
@@ -320,7 +320,7 @@ fn equip_equipping_smaller_bag_from_last_slot_of_big_bag() {
 }
 
 #[test]
-fn unequip_unequipping_bag_into_its_own_slot_with_no_other_free_slots() {
+fn unequip_unequipping_bag_into_its_own_slot_with_no_other_free_slots_returns_one_item() {
     let mut inv = Inventory::new_empty();
     let bag = get_test_bag(9);
 
@@ -335,7 +335,14 @@ fn unequip_unequipping_bag_into_its_own_slot_with_no_other_free_slots() {
 
     let result =
         inv.swap_inventory_loadout(InvSlotId::new(15, 0), EquipSlot::Armor(ArmorSlot::Bag1));
-    assert!(result.is_empty())
+
+    assert_eq!(result.len(), 1);
+    // Because the slot the bag was swapped with no longer exists as it was provided
+    // by itself, the bag is returned to the caller
+    assert_eq!(
+        result[0].item_definition_id(),
+        "common.items.testing.test_bag"
+    );
 }
 
 #[test]
@@ -458,6 +465,31 @@ fn free_after_swap_inv_item_without_slots_swapped_with_empty_equip_slot() {
     assert_eq!(13, result);
 }
 
+// This test is a regression test for a bug that crashed the server when
+// swapping an equipped item providing slots with an item that does not
+// provide slots.
+#[test]
+fn backpack_crash() {
+    let mut inv = Inventory::new_empty();
+
+    let backpack = Item::new_from_asset_expect("common.items.armor.misc.back.backpack");
+    inv.loadout
+        .swap(EquipSlot::Armor(ArmorSlot::Back), Some(backpack));
+
+    fill_inv_slots(&mut inv, 35);
+
+    let cape = Item::new_from_asset_expect("common.items.armor.misc.back.admin");
+    assert!(inv.push(cape).is_ok());
+
+    let returned_items =
+        inv.swap_inventory_loadout(InvSlotId::new(9, 17), EquipSlot::Armor(ArmorSlot::Back));
+    assert_eq!(18, returned_items.len());
+    assert_eq!(
+        "common.items.armor.misc.back.backpack",
+        returned_items[0].item_definition_id()
+    );
+}
+
 fn fill_inv_slots(inv: &mut Inventory, items: u16) {
     let msm = &MaterialStatManifest::default();
     let ability_map = &AbilityMap::default();