Merge branch 'xvar/inventory-followup' into 'master'

Inventory followup fixes

Closes #920

See merge request veloren/veloren!1685

common/src/comp/inventory/mod.rs

@@ -1,5 +1,5 @@
 use core::ops::Not;
-use std::{collections::HashMap, convert::TryFrom, iter::once, mem, ops::Range};
+use std::{collections::HashMap, convert::TryFrom, mem, ops::Range};
 
 use serde::{Deserialize, Serialize};
 use specs::{Component, DerefFlaggedStorage};
@@ -392,25 +392,19 @@ impl Inventory {
     /// go into inventory. If the item is going to mainhand, put mainhand in
     /// offhand and place offhand into inventory.
     #[must_use = "Returned items will be lost if not used"]
-    pub fn equip(&mut self, inv_slot: InvSlotId) -> Option<Vec<Item>> {
+    pub fn equip(&mut self, inv_slot: InvSlotId) -> Vec<Item> {
-        let mut leftover_items = None;
         self.get(inv_slot)
-            .map(|x| x.kind().clone())
+            .and_then(|item| self.loadout.get_slot_to_equip_into(item.kind()))
-            .map(|item_kind| {
+            .map(|equip_slot| {
-                self.loadout
+                // Special case when equipping into main hand - swap with offhand first
-                    .get_slot_to_equip_into(&item_kind)
+                if equip_slot == EquipSlot::Mainhand {
-                    .map(|equip_slot| {
+                    self.loadout
-                        // Special case when equipping into main hand - swap with offhand first
+                        .swap_slots(EquipSlot::Mainhand, EquipSlot::Offhand);
-                        if equip_slot == EquipSlot::Mainhand {
+                }
-                            self.loadout
-                                .swap_slots(EquipSlot::Mainhand, EquipSlot::Offhand);
-                        }
 
-                        leftover_items = self.swap_inventory_loadout(inv_slot, equip_slot);
+                self.swap_inventory_loadout(inv_slot, equip_slot)
-                    })
+            })
-            });
+            .unwrap_or_else(Vec::new)
-
-        leftover_items
     }
 
     /// Determines how many free inventory slots will be left after equipping an
@@ -419,8 +413,7 @@ impl Inventory {
     pub fn free_after_equip(&self, inv_slot: InvSlotId) -> i32 {
         let (inv_slot_for_equipped, slots_from_equipped) = self
             .get(inv_slot)
-            .map(|x| x.kind().clone())
+            .and_then(|item| self.loadout.get_slot_to_equip_into(item.kind()))
-            .and_then(|item_kind| self.loadout.get_slot_to_equip_into(&item_kind))
             .and_then(|equip_slot| self.equipped(equip_slot))
             .map_or((1, 0), |item| (0, item.slots().len()));
 
@@ -454,13 +447,10 @@ impl Inventory {
         // Unload any items contained within the item, and push those items and the item
         // itself into the inventory. We already know that there are enough free slots
         // so push will never give us an item back.
-        item.drain()
+        item.drain().for_each(|item| {
-            .collect::<Vec<Item>>()
+            self.push(item).unwrap_none();
-            .into_iter()
+        });
-            .chain(once(item))
+        self.push(item).unwrap_none();
-            .for_each(|item| {
-                self.push(item).unwrap_none();
-            });
 
         Ok(())
     }
@@ -486,7 +476,7 @@ impl Inventory {
                 // any that don't fit to be to be dropped on the floor by the caller
                 match self.push_all(unloaded_items.into_iter()) {
                     Err(Error::Full(leftovers)) => Some(leftovers),
-                    Ok(_) => None,
+                    Ok(()) => None,
                 }
             }))
     }
@@ -509,11 +499,11 @@ impl Inventory {
     /// Swaps items from two slots, regardless of if either is inventory or
     /// loadout.
     #[must_use = "Returned items will be lost if not used"]
-    pub fn swap(&mut self, slot_a: Slot, slot_b: Slot) -> Option<Vec<Item>> {
+    pub fn swap(&mut self, slot_a: Slot, slot_b: Slot) -> Vec<Item> {
         match (slot_a, slot_b) {
             (Slot::Inventory(slot_a), Slot::Inventory(slot_b)) => {
                 self.swap_slots(slot_a, slot_b);
-                None
+                Vec::new()
             },
             (Slot::Inventory(inv_slot), Slot::Equip(equip_slot))
             | (Slot::Equip(equip_slot), Slot::Inventory(inv_slot)) => {
@@ -521,7 +511,7 @@ impl Inventory {
             },
             (Slot::Equip(slot_a), Slot::Equip(slot_b)) => {
                 self.loadout.swap_slots(slot_a, slot_b);
-                None
+                Vec::new()
             },
         }
     }
@@ -556,47 +546,42 @@ impl Inventory {
         &mut self,
         inv_slot_id: InvSlotId,
         equip_slot: EquipSlot,
-    ) -> Option<Vec<Item>> {
+    ) -> Vec<Item> {
         if !self.can_swap(inv_slot_id, equip_slot) {
-            return None;
+            return Vec::new();
         }
 
-        let mut unloaded_items = None;
-
         // Take the item from the inventory
         let from_inv = self.remove(inv_slot_id);
 
         // Swap the equipped item for the item from the inventory
         let from_equip = self.loadout.swap(equip_slot, from_inv);
-        if let Some(mut from_equip) = from_equip {
-            // Unload any items held inside the previously equipped item
-            let items: Vec<Item> = from_equip.drain().collect();
-            if items.iter().len() > 0 {
-                unloaded_items = Some(items);
-            }
 
-            // Attempt to put the unequipped item in the same slot that the inventory item
+        let unloaded_items = from_equip
-            // was in - if that slot no longer exists (because a large container was
+            .map(|mut from_equip| {
-            // swapped for a smaller one) then push the item to the first free
+                // Unload any items held inside the previously equipped item
-            // inventory slot instead.
+                let items: Vec<Item> = from_equip.drain().collect();
-            if let Err(returned) = self.insert_at(inv_slot_id, from_equip) {
+
-                self.push(returned)
+                // Attempt to put the unequipped item in the same slot that the inventory item
-                    .expect_none("Unable to push to inventory, no slots (bug in can_swap()?)");
+                // 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.
+                if let Err(returned) = self.insert_at(inv_slot_id, from_equip) {
+                    self.push(returned)
+                        .expect_none("Unable to push to inventory, no slots (bug in can_swap()?)");
+                }
+
+                items
+            })
+            .unwrap_or_default();
 
         // Attempt to put any items unloaded from the unequipped item into empty
         // inventory slots and return any that don't fit to the caller where they
         // will be dropped on the ground
-        if let Some(unloaded_items) = unloaded_items {
+        match self.push_all(unloaded_items.into_iter()) {
-            let leftovers = match self.push_all(unloaded_items.into_iter()) {
+            Err(Error::Full(leftovers)) => leftovers,
-                Err(Error::Full(leftovers)) => leftovers,
+            Ok(()) => Vec::new(),
-                Ok(_) => vec![],
-            };
-            return Some(leftovers);
         }
-
-        None
     }
 
     /// Determines if an inventory and loadout slot can be swapped, taking into

common/src/comp/inventory/test.rs

@@ -167,7 +167,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_eq!(false, result);
+    assert!(!result);
 }
 
 #[test]
@@ -211,7 +211,7 @@ fn equip_replace_already_equipped_item() {
     inv.push(boots.clone());
     inv.replace_loadout_item(EquipSlot::Armor(ArmorSlot::Feet), starting_sandles.clone());
 
-    inv.equip(InvSlotId::new(0, 0)).unwrap_none();
+    let _ = inv.equip(InvSlotId::new(0, 0));
 
     // We should now have the testing boots equipped
     assert_eq!(
@@ -253,7 +253,7 @@ fn equip_equipping_smaller_bag_from_last_slot_of_big_bag() {
         inv.get(InvSlotId::new(0, 0)).unwrap().item_definition_id(),
         LARGE_BAG_ID
     );
-    assert_eq!(result, None);
+    assert!(result.is_empty());
 }
 
 #[test]
@@ -268,8 +268,9 @@ fn unequip_unequipping_bag_into_its_own_slot_with_no_other_free_slots() {
     // Fill all inventory built-in slots
     fill_inv_slots(&mut inv, 18);
 
-    inv.swap_inventory_loadout(InvSlotId::new(15, 0), EquipSlot::Armor(ArmorSlot::Bag1))
+    let result =
-        .unwrap_none();
+        inv.swap_inventory_loadout(InvSlotId::new(15, 0), EquipSlot::Armor(ArmorSlot::Bag1));
+    assert!(result.is_empty())
 }
 
 #[test]
@@ -283,12 +284,9 @@ fn equip_one_bag_equipped_equip_second_bag() {
 
     inv.push(bag);
 
-    inv.equip(InvSlotId::new(0, 0)).unwrap_none();
+    let _ = inv.equip(InvSlotId::new(0, 0));
 
-    assert_eq!(
+    assert!(inv.equipped(EquipSlot::Armor(ArmorSlot::Bag2)).is_some());
-        true,
-        inv.equipped(EquipSlot::Armor(ArmorSlot::Bag2)).is_some()
-    );
 }
 
 #[test]

common/sys/src/character_behavior.rs

@@ -37,6 +37,7 @@ fn incorporate_update(tuple: &mut JoinTuple, state_update: StateUpdate) {
                 Slot::Equip(EquipSlot::Mainhand),
                 Slot::Equip(EquipSlot::Offhand),
             )
+            .first()
             .unwrap_none(); // Swapping main and offhand never results in leftover items
     }
 }

server-cli/src/logging.rs

@@ -16,11 +16,6 @@ pub fn init(basic: bool) {
     let base_exceptions = |env: EnvFilter| {
         env.add_directive("veloren_world::sim=info".parse().unwrap())
             .add_directive("veloren_world::civ=info".parse().unwrap())
-            .add_directive(
-                "veloren_common::comp::inventory::slot=info"
-                    .parse()
-                    .unwrap(),
-            )
             .add_directive("uvth=warn".parse().unwrap())
             .add_directive("tiny_http=warn".parse().unwrap())
             .add_directive("mio::sys::windows=debug".parse().unwrap())

server/src/events/interaction.rs

@@ -183,6 +183,7 @@ pub fn handle_possess(server: &Server, possessor_uid: Uid, possesse_uid: Uid) {
                     Slot::Equip(EquipSlot::Mainhand),
                     Slot::Equip(EquipSlot::Offhand),
                 )
+                .first()
                 .unwrap_none(); // Swapping main and offhand never results in leftover items
 
             inventory.replace_loadout_item(EquipSlot::Mainhand, Some(debug_item));

server/src/events/inventory_manip.rs

@@ -234,17 +234,15 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                             swap_lantern(&mut state.ecs().write_storage(), entity, &lantern);
                         }
                         if let Some(pos) = state.ecs().read_storage::<comp::Pos>().get(entity) {
-                            if let Some(leftover_items) = inventory.equip(slot) {
+                            dropped_items.extend(inventory.equip(slot).into_iter().map(|x| {
-                                dropped_items.extend(leftover_items.into_iter().map(|x| {
+                                (
-                                    (
+                                    *pos,
-                                        *pos,
+                                    state
-                                        state
+                                        .read_component_copied::<comp::Ori>(entity)
-                                            .read_component_copied::<comp::Ori>(entity)
+                                        .unwrap_or_default(),
-                                            .unwrap_or_default(),
+                                    x,
-                                        x,
+                                )
-                                    )
+                            }));
-                                }));
-                            }
                         }
                         Some(comp::InventoryUpdateEvent::Used)
                     } else if let Some(item) = inventory.take(slot) {
@@ -413,17 +411,15 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
             if let Some(pos) = ecs.read_storage::<comp::Pos>().get(entity) {
                 if let Some(mut inventory) = ecs.write_storage::<comp::Inventory>().get_mut(entity)
                 {
-                    if let Some(leftover_items) = inventory.swap(a, b) {
+                    dropped_items.extend(inventory.swap(a, b).into_iter().map(|x| {
-                        dropped_items.extend(leftover_items.into_iter().map(|x| {
+                        (
-                            (
+                            *pos,
-                                *pos,
+                            state
-                                state
+                                .read_component_copied::<comp::Ori>(entity)
-                                    .read_component_copied::<comp::Ori>(entity)
+                                .unwrap_or_default(),
-                                    .unwrap_or_default(),
+                            x,
-                                x,
+                        )
-                            )
+                    }));
-                        }));
-                    }
                 }
             }
 

voxygen/src/logging.rs

@@ -45,11 +45,6 @@ pub fn init(settings: &Settings) -> Vec<impl Drop> {
             .add_directive("uvth=warn".parse().unwrap())
             .add_directive("tiny_http=warn".parse().unwrap())
             .add_directive("mio::sys::windows=debug".parse().unwrap())
-            .add_directive(
-                "veloren_common::comp::inventory::slot=info"
-                    .parse()
-                    .unwrap(),
-            )
             .add_directive(
                 "veloren_server::persistence::character=info"
                     .parse()