Follow-up fixes from review of inventory MR

This commit is contained in:
Ben Wallis 2021-01-09 18:10:12 +00:00
parent 9f7ccbb24d
commit c785e75e60
7 changed files with 71 additions and 100 deletions

View File

@ -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>> {
let mut leftover_items = None;
pub fn equip(&mut self, inv_slot: InvSlotId) -> Vec<Item> {
self.get(inv_slot)
.map(|x| x.kind().clone())
.map(|item_kind| {
self.loadout
.get_slot_to_equip_into(&item_kind)
.map(|equip_slot| {
// Special case when equipping into main hand - swap with offhand first
if equip_slot == EquipSlot::Mainhand {
self.loadout
.swap_slots(EquipSlot::Mainhand, EquipSlot::Offhand);
}
.and_then(|item| self.loadout.get_slot_to_equip_into(item.kind()))
.map(|equip_slot| {
// Special case when equipping into main hand - swap with offhand first
if equip_slot == EquipSlot::Mainhand {
self.loadout
.swap_slots(EquipSlot::Mainhand, EquipSlot::Offhand);
}
leftover_items = self.swap_inventory_loadout(inv_slot, equip_slot);
})
});
leftover_items
self.swap_inventory_loadout(inv_slot, equip_slot)
})
.unwrap_or_else(Vec::new)
}
/// 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_kind| self.loadout.get_slot_to_equip_into(&item_kind))
.and_then(|item| 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()
.collect::<Vec<Item>>()
.into_iter()
.chain(once(item))
.for_each(|item| {
self.push(item).unwrap_none();
});
item.drain().for_each(|item| {
self.push(item).unwrap_none();
});
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
// 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()?)");
}
}
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();
// 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.
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 {
let leftovers = match self.push_all(unloaded_items.into_iter()) {
Err(Error::Full(leftovers)) => leftovers,
Ok(_) => vec![],
};
return Some(leftovers);
match self.push_all(unloaded_items.into_iter()) {
Err(Error::Full(leftovers)) => leftovers,
Ok(()) => Vec::new(),
}
None
}
/// Determines if an inventory and loadout slot can be swapped, taking into

View File

@ -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))
.unwrap_none();
let result =
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!(
true,
inv.equipped(EquipSlot::Armor(ArmorSlot::Bag2)).is_some()
);
assert!(inv.equipped(EquipSlot::Armor(ArmorSlot::Bag2)).is_some());
}
#[test]

View File

@ -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
}
}

View File

@ -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())

View File

@ -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));

View File

@ -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(leftover_items.into_iter().map(|x| {
(
*pos,
state
.read_component_copied::<comp::Ori>(entity)
.unwrap_or_default(),
x,
)
}));
}
dropped_items.extend(inventory.equip(slot).into_iter().map(|x| {
(
*pos,
state
.read_component_copied::<comp::Ori>(entity)
.unwrap_or_default(),
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(leftover_items.into_iter().map(|x| {
(
*pos,
state
.read_component_copied::<comp::Ori>(entity)
.unwrap_or_default(),
x,
)
}));
}
dropped_items.extend(inventory.swap(a, b).into_iter().map(|x| {
(
*pos,
state
.read_component_copied::<comp::Ori>(entity)
.unwrap_or_default(),
x,
)
}));
}
}

View File

@ -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()