From 63bf8f3ac743e44cd3a332705407c69b243b8141 Mon Sep 17 00:00:00 2001 From: Sam Date: Sun, 16 Apr 2023 18:06:59 -0400 Subject: [PATCH] Responded to review comments and fixed tests --- common/src/comp/inventory/loadout.rs | 60 ++++++++++++++++++-------- common/src/comp/inventory/test.rs | 64 +++++++++++++++++++--------- 2 files changed, 87 insertions(+), 37 deletions(-) diff --git a/common/src/comp/inventory/loadout.rs b/common/src/comp/inventory/loadout.rs index 8075bef650..4a6b237bee 100644 --- a/common/src/comp/inventory/loadout.rs +++ b/common/src/comp/inventory/loadout.rs @@ -121,11 +121,12 @@ impl Loadout { .find(|x| x.equip_slot == equip_slot) .and_then(|x| core::mem::replace(&mut x.slot, item)); if let Some(unequipped_item) = unequipped_item.as_ref() { + // TODO: Avoid this allocation when there isn't an insert let entry = self .recently_unequipped_items .entry(unequipped_item.item_definition_id().to_owned()) .or_insert((time, 0)); - *entry = (time, entry.1 + 1); + *entry = (time, entry.1.saturating_add(1)); } unequipped_item } @@ -503,24 +504,27 @@ impl Loadout { } self.recently_unequipped_items .retain(|_def, (unequip_time, count)| { - (time.0 - unequip_time.0 < UNEQUIP_TRACKING_DURATION) || *count > 0 + (time.0 - unequip_time.0 < UNEQUIP_TRACKING_DURATION) && *count > 0 }); } } #[cfg(test)] mod tests { - use crate::comp::{ - inventory::{ - item::{ - armor::{Armor, ArmorKind, Protection}, - ItemKind, + use crate::{ + comp::{ + inventory::{ + item::{ + armor::{Armor, ArmorKind, Protection}, + ItemKind, + }, + loadout::Loadout, + slot::{ArmorSlot, EquipSlot}, + test_helpers::get_test_bag, }, - loadout::Loadout, - slot::{ArmorSlot, EquipSlot}, - test_helpers::get_test_bag, + Item, }, - Item, + resources::Time, }; #[test] @@ -529,7 +533,7 @@ mod tests { let bag1_slot = EquipSlot::Armor(ArmorSlot::Bag1); let bag = get_test_bag(18); - loadout.swap(bag1_slot, Some(bag)); + loadout.swap(bag1_slot, Some(bag), Time(0.0)); let result = loadout.slot_range_for_equip_slot(bag1_slot).unwrap(); @@ -550,7 +554,7 @@ mod tests { let feet_slot = EquipSlot::Armor(ArmorSlot::Feet); let boots = Item::new_from_asset_expect("common.items.testing.test_boots"); - loadout.swap(feet_slot, Some(boots)); + loadout.swap(feet_slot, Some(boots), Time(0.0)); let result = loadout.slot_range_for_equip_slot(feet_slot); assert_eq!(None, result); @@ -560,7 +564,11 @@ mod tests { fn test_get_slot_to_equip_into_second_bag_slot_free() { let mut loadout = Loadout::new_empty(); - loadout.swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(get_test_bag(1))); + loadout.swap( + EquipSlot::Armor(ArmorSlot::Bag1), + Some(get_test_bag(1)), + Time(0.0), + ); let result = loadout .get_slot_to_equip_into(&ItemKind::Armor(Armor::test_armor( @@ -577,10 +585,26 @@ mod tests { fn test_get_slot_to_equip_into_no_bag_slots_free() { let mut loadout = Loadout::new_empty(); - loadout.swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(get_test_bag(1))); - loadout.swap(EquipSlot::Armor(ArmorSlot::Bag2), Some(get_test_bag(1))); - loadout.swap(EquipSlot::Armor(ArmorSlot::Bag3), Some(get_test_bag(1))); - loadout.swap(EquipSlot::Armor(ArmorSlot::Bag4), Some(get_test_bag(1))); + loadout.swap( + EquipSlot::Armor(ArmorSlot::Bag1), + Some(get_test_bag(1)), + Time(0.0), + ); + loadout.swap( + EquipSlot::Armor(ArmorSlot::Bag2), + Some(get_test_bag(1)), + Time(0.0), + ); + loadout.swap( + EquipSlot::Armor(ArmorSlot::Bag3), + Some(get_test_bag(1)), + Time(0.0), + ); + loadout.swap( + EquipSlot::Armor(ArmorSlot::Bag4), + Some(get_test_bag(1)), + Time(0.0), + ); let result = loadout .get_slot_to_equip_into(&ItemKind::Armor(Armor::test_armor( diff --git a/common/src/comp/inventory/test.rs b/common/src/comp/inventory/test.rs index c7d7e65b1e..076ccc56af 100644 --- a/common/src/comp/inventory/test.rs +++ b/common/src/comp/inventory/test.rs @@ -129,7 +129,7 @@ fn free_slots_minus_equipped_item_items_only_present_in_equipped_bag_slots() { let bag = get_test_bag(18); let bag1_slot = EquipSlot::Armor(ArmorSlot::Bag1); inv.loadout - .swap(bag1_slot, Some(bag.duplicate(ability_map, msm))); + .swap(bag1_slot, Some(bag.duplicate(ability_map, msm)), Time(0.0)); assert!(inv.insert_at(InvSlotId::new(15, 0), bag).unwrap().is_none()); @@ -149,10 +149,11 @@ fn free_slots_minus_equipped_item() { let bag = get_test_bag(18); let bag1_slot = EquipSlot::Armor(ArmorSlot::Bag1); inv.loadout - .swap(bag1_slot, Some(bag.duplicate(ability_map, msm))); + .swap(bag1_slot, Some(bag.duplicate(ability_map, msm)), Time(0.0)); inv.loadout.swap( EquipSlot::Armor(ArmorSlot::Bag2), Some(bag.duplicate(ability_map, msm)), + Time(0.0), ); assert!(inv.insert_at(InvSlotId::new(16, 0), bag).unwrap().is_none()); @@ -169,7 +170,7 @@ fn get_slot_range_for_equip_slot() { let mut inv = Inventory::with_empty(); let bag = get_test_bag(18); let bag1_slot = EquipSlot::Armor(ArmorSlot::Bag1); - inv.loadout.swap(bag1_slot, Some(bag)); + inv.loadout.swap(bag1_slot, Some(bag), Time(0.0)); let result = inv.get_slot_range_for_equip_slot(bag1_slot).unwrap(); @@ -198,7 +199,11 @@ fn can_swap_equipped_bag_into_empty_inv_slot( ) { let mut inv = Inventory::with_empty(); - inv.replace_loadout_item(EquipSlot::Armor(ArmorSlot::Bag1), Some(get_test_bag(18))); + inv.replace_loadout_item( + EquipSlot::Armor(ArmorSlot::Bag1), + Some(get_test_bag(18)), + Time(0.0), + ); fill_inv_slots(&mut inv, 18 - free_slots); @@ -211,7 +216,11 @@ fn can_swap_equipped_bag_into_empty_inv_slot( fn can_swap_equipped_bag_into_only_empty_slot_provided_by_itself_should_return_true() { let mut inv = Inventory::with_empty(); - inv.replace_loadout_item(EquipSlot::Armor(ArmorSlot::Bag1), Some(get_test_bag(18))); + inv.replace_loadout_item( + EquipSlot::Armor(ArmorSlot::Bag1), + Some(get_test_bag(18)), + Time(0.0), + ); fill_inv_slots(&mut inv, 35); @@ -231,22 +240,26 @@ fn unequip_items_both_hands() { inv.replace_loadout_item( EquipSlot::ActiveMainhand, Some(sword.duplicate(ability_map, msm)), + Time(0.0), ); inv.replace_loadout_item( EquipSlot::InactiveMainhand, Some(sword.duplicate(ability_map, msm)), + Time(0.0), ); // Fill all inventory slots except one fill_inv_slots(&mut inv, 17); - let result = inv.unequip(EquipSlot::ActiveMainhand); + let result = inv.unequip(EquipSlot::ActiveMainhand, Time(0.0)); // We have space in the inventory, so this should have unequipped assert_eq!(None, inv.equipped(EquipSlot::ActiveMainhand)); assert_eq!(18, inv.populated_slots()); assert!(result.is_ok()); - let result = inv.unequip(EquipSlot::InactiveMainhand).unwrap_err(); + let result = inv + .unequip(EquipSlot::InactiveMainhand, Time(0.0)) + .unwrap_err(); assert_eq!(SlotError::InventoryFull, result); // There is no more space in the inventory, so this should still be equipped @@ -274,9 +287,10 @@ fn equip_replace_already_equipped_item() { starting_sandles .as_ref() .map(|item| item.duplicate(ability_map, msm)), + Time(0.0), ); - let _ = inv.equip(InvSlotId::new(0, 0)); + let _ = inv.equip(InvSlotId::new(0, 0), Time(0.0)); // We should now have the testing boots equipped assert_eq!( @@ -302,7 +316,11 @@ fn equip_equipping_smaller_bag_from_last_slot_of_big_bag() { assert!( inv.loadout - .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(large_bag)) + .swap( + EquipSlot::Armor(ArmorSlot::Bag1), + Some(large_bag), + Time(0.0) + ) .is_none() ); @@ -311,6 +329,7 @@ fn equip_equipping_smaller_bag_from_last_slot_of_big_bag() { let result = inv.swap( Slot::Equip(EquipSlot::Armor(ArmorSlot::Bag1)), Slot::Inventory(InvSlotId::new(15, 15)), + Time(0.0), ); assert_eq!( @@ -327,15 +346,18 @@ fn unequip_unequipping_bag_into_its_own_slot_with_no_other_free_slots_returns_on assert!( inv.loadout - .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag)) + .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag), Time(0.0)) .is_none() ); // Fill all inventory built-in slots fill_inv_slots(&mut inv, 18); - let result = - inv.swap_inventory_loadout(InvSlotId::new(15, 0), EquipSlot::Armor(ArmorSlot::Bag1)); + let result = inv.swap_inventory_loadout( + InvSlotId::new(15, 0), + EquipSlot::Armor(ArmorSlot::Bag1), + Time(0.0), + ); assert_eq!(result.len(), 1); // Because the slot the bag was swapped with no longer exists as it was provided @@ -358,13 +380,14 @@ fn equip_one_bag_equipped_equip_second_bag() { .swap( EquipSlot::Armor(ArmorSlot::Bag1), Some(bag.duplicate(ability_map, msm)), + Time(0.0) ) .is_none() ); inv.push(bag).unwrap(); - let _ = inv.equip(InvSlotId::new(0, 0)); + let _ = inv.equip(InvSlotId::new(0, 0), Time(0.0)); assert!(inv.equipped(EquipSlot::Armor(ArmorSlot::Bag2)).is_some()); } @@ -376,7 +399,7 @@ fn free_after_swap_equipped_item_has_more_slots() { let bag = get_test_bag(18); assert!( inv.loadout - .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag)) + .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag), Time(0.0)) .is_none() ); @@ -399,7 +422,7 @@ fn free_after_swap_equipped_item_has_less_slots() { let bag = get_test_bag(9); assert!( inv.loadout - .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag)) + .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag), Time(0.0)) .is_none() ); @@ -422,7 +445,7 @@ fn free_after_swap_equipped_item_with_slots_swapped_with_empty_inv_slot() { let bag = get_test_bag(9); assert!( inv.loadout - .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag)) + .swap(EquipSlot::Armor(ArmorSlot::Bag1), Some(bag), Time(0.0)) .is_none() ); @@ -475,15 +498,18 @@ fn backpack_crash() { let backpack = Item::new_from_asset_expect("common.items.armor.misc.back.backpack"); inv.loadout - .swap(EquipSlot::Armor(ArmorSlot::Back), Some(backpack)); + .swap(EquipSlot::Armor(ArmorSlot::Back), Some(backpack), Time(0.0)); 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)); + let returned_items = inv.swap_inventory_loadout( + InvSlotId::new(9, 17), + EquipSlot::Armor(ArmorSlot::Back), + Time(0.0), + ); assert_eq!(18, returned_items.len()); assert_eq!( ItemDefinitionId::Simple("common.items.armor.misc.back.backpack"),