From cd02b3a17276b6b5573014845a19112ead9506dd Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 22 Apr 2023 11:37:42 -0400 Subject: [PATCH] Apply TODO from !3883 and refactor to avoid collect in Inventory::damage_items, also reduce to iterating over the hashmap once when culling the recently unequipped items list --- common/src/comp/inventory/item/mod.rs | 4 + common/src/comp/inventory/loadout.rs | 44 +++++------ common/src/comp/inventory/loadout_builder.rs | 2 + common/src/comp/inventory/mod.rs | 78 ++++++-------------- server/src/events/entity_manipulation.rs | 4 + 5 files changed, 52 insertions(+), 80 deletions(-) diff --git a/common/src/comp/inventory/item/mod.rs b/common/src/comp/inventory/item/mod.rs index e3299a46ac..e6e6915696 100644 --- a/common/src/comp/inventory/item/mod.rs +++ b/common/src/comp/inventory/item/mod.rs @@ -1456,6 +1456,10 @@ impl Equivalent for ItemDefinitionId<'_> { fn equivalent(&self, key: &ItemDefinitionIdOwned) -> bool { self == key } } +impl From<&ItemDefinitionId<'_>> for ItemDefinitionIdOwned { + fn from(value: &ItemDefinitionId<'_>) -> Self { value.to_owned() } +} + #[cfg(test)] mod tests { use super::*; diff --git a/common/src/comp/inventory/loadout.rs b/common/src/comp/inventory/loadout.rs index df7a1d0c7b..d4e9645db2 100644 --- a/common/src/comp/inventory/loadout.rs +++ b/common/src/comp/inventory/loadout.rs @@ -120,10 +120,9 @@ 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()) + .entry_ref(&unequipped_item.item_definition_id()) .or_insert((time, 0)); *entry = (time, entry.1.saturating_add(1)); } @@ -263,22 +262,6 @@ impl Loadout { .and_then(|item| item.slot(loadout_slot_id.slot_idx)) } - pub(super) fn inv_slot_with_mutable_recently_unequipped_items( - &mut self, - loadout_slot_id: LoadoutSlotId, - ) -> ( - Option<&InvSlot>, - &mut HashMap, - ) { - ( - self.slots - .get(loadout_slot_id.loadout_idx) - .and_then(|loadout_slot| loadout_slot.slot.as_ref()) - .and_then(|item| item.slot(loadout_slot_id.slot_idx)), - &mut self.recently_unequipped_items, - ) - } - /// Returns the `InvSlot` for a given `LoadoutSlotId` pub(super) fn inv_slot_mut(&mut self, loadout_slot_id: LoadoutSlotId) -> Option<&mut InvSlot> { self.slots @@ -319,6 +302,18 @@ impl Loadout { .flat_map(|loadout_slots| loadout_slots.iter_mut()) //Collapse iter of Vec to iter of InvSlot } + pub(super) fn inv_slots_mut_with_mutable_recently_unequipped_items( + &mut self, + ) -> ( + impl Iterator, + &mut HashMap, + ) { + let slots_mut = self.slots.iter_mut() + .filter_map(|x| x.slot.as_mut().map(|item| item.slots_mut())) // Discard loadout items that have no slots of their own + .flat_map(|loadout_slots| loadout_slots.iter_mut()); //Collapse iter of Vec to iter of InvSlot + (slots_mut, &mut self.recently_unequipped_items) + } + /// Gets the range of loadout-provided inventory slot indexes that are /// provided by the item in the given `EquipSlot` pub(super) fn slot_range_for_equip_slot(&self, equip_slot: EquipSlot) -> Option> { @@ -510,15 +505,14 @@ impl Loadout { } pub(super) fn cull_recently_unequipped_items(&mut self, time: Time) { - for (unequip_time, _count) in self.recently_unequipped_items.values_mut() { - // If somehow time went backwards or faulty unequip time supplied, set unequip - // time to minimum of current time and unequip time - if time.0 < unequip_time.0 { - *unequip_time = time; - } - } self.recently_unequipped_items .retain(|_def, (unequip_time, count)| { + // If somehow time went backwards or faulty unequip time supplied, set unequip + // time to minimum of current time and unequip time + if time.0 < unequip_time.0 { + *unequip_time = time; + } + (time.0 - unequip_time.0 < UNEQUIP_TRACKING_DURATION) && *count > 0 }); } diff --git a/common/src/comp/inventory/loadout_builder.rs b/common/src/comp/inventory/loadout_builder.rs index 42af30bc91..4c614bff07 100644 --- a/common/src/comp/inventory/loadout_builder.rs +++ b/common/src/comp/inventory/loadout_builder.rs @@ -1149,6 +1149,8 @@ impl LoadoutBuilder { .map_or(true, |item| equip_slot.can_hold(&item.kind())) ); + // TODO: What if `with_equipment` is used twice for the same slot. Or defaults + // include an item in this slot. // Used when creating a loadout, so time not needed as it is used to check when // stuff gets unequipped. A new loadout has never unequipped an item. let time = Time(0.0); diff --git a/common/src/comp/inventory/mod.rs b/common/src/comp/inventory/mod.rs index 1bbc2a494b..4d15586854 100644 --- a/common/src/comp/inventory/mod.rs +++ b/common/src/comp/inventory/mod.rs @@ -139,6 +139,18 @@ impl Inventory { self.slots.iter_mut().chain(self.loadout.inv_slots_mut()) } + fn slots_mut_with_mutable_recently_unequipped_items( + &mut self, + ) -> ( + impl Iterator, + &mut HashMap, + ) { + let (slots_mut, recently_unequipped) = self + .loadout + .inv_slots_mut_with_mutable_recently_unequipped_items(); + (self.slots.iter_mut().chain(slots_mut), recently_unequipped) + } + /// An iterator of all inventory slots and their position pub fn slots_with_id(&self) -> impl Iterator { self.slots @@ -579,24 +591,6 @@ impl Inventory { } } - fn slot_with_mutable_recently_unequipped_items( - &mut self, - inv_slot_id: InvSlotId, - ) -> ( - Option<&InvSlot>, - &mut HashMap, - ) { - match SlotId::from(inv_slot_id) { - SlotId::Inventory(slot_idx) => ( - self.slots.get(slot_idx), - &mut self.loadout.recently_unequipped_items, - ), - SlotId::Loadout(loadout_slot_id) => self - .loadout - .inv_slot_with_mutable_recently_unequipped_items(loadout_slot_id), - } - } - pub fn slot_mut(&mut self, inv_slot_id: InvSlotId) -> Option<&mut InvSlot> { match SlotId::from(inv_slot_id) { SlotId::Inventory(slot_idx) => self.slots.get_mut(slot_idx), @@ -912,7 +906,7 @@ impl Inventory { }); } - /// Increments durability of all valid items equipped in loaodut and + /// Increments durability lost for all valid items equipped in loadout and /// recently unequipped from loadout by 1 pub fn damage_items( &mut self, @@ -923,43 +917,17 @@ impl Inventory { self.loadout.damage_items(ability_map, msm); self.loadout.cull_recently_unequipped_items(time); - let slots = self - .slots_with_id() - .filter(|(_slot, item)| { - item.as_ref().map_or(false, |item| { - item.durability_lost() - .map_or(false, |dur| dur < Item::MAX_DURABILITY) - && self - .loadout - .recently_unequipped_items - .contains_key(&item.item_definition_id()) - }) - }) - .map(|(slot, _item)| slot) - .collect::>(); - slots.into_iter().for_each(|slot| { - let slot = if let (Some(Some(item)), recently_unequipped_items) = - self.slot_with_mutable_recently_unequipped_items(slot) + let (slots_mut, recently_unequipped_items) = + self.slots_mut_with_mutable_recently_unequipped_items(); + slots_mut.filter_map(|slot| slot.as_mut()).for_each(|item| { + if item.durability_lost() + .map_or(false, |dur| dur < Item::MAX_DURABILITY) + && let Some((_unequip_time, count)) = + recently_unequipped_items.get_mut(&item.item_definition_id()) + && *count > 0 { - if let Some((_unequip_time, count)) = - recently_unequipped_items.get_mut(&item.item_definition_id()) - { - if *count > 0 { - *count -= 1; - Some(slot) - } else { - None - } - } else { - None - } - } else { - None - }; - if let Some(slot) = slot { - if let Some(Some(item)) = self.slot_mut(slot) { - item.increment_damage(ability_map, msm); - } + *count -= 1; + item.increment_damage(ability_map, msm); } }); } diff --git a/server/src/events/entity_manipulation.rs b/server/src/events/entity_manipulation.rs index ca06333997..d7ecfcabce 100644 --- a/server/src/events/entity_manipulation.rs +++ b/server/src/events/entity_manipulation.rs @@ -507,6 +507,7 @@ pub fn handle_destroy(server: &mut Server, entity: EcsEntity, last_change: Healt true }; + // TODO: Do we need to do this if `should_delete` is true? // Modify durability on all equipped items if let Some(mut inventory) = state.ecs().write_storage::().get_mut(entity) { let ecs = state.ecs(); @@ -1424,6 +1425,9 @@ pub fn handle_entity_attacked_hook(server: &Server, entity: EcsEntity) { let clients = ecs.read_storage::(); let mut agents = ecs.write_storage::(); let mut notify_trade_party = |entity| { + // TODO: Can probably improve UX here for the user that sent the trade + // invite, since right now it may seems like their request was + // purposefully declined, rather than e.g. being interrupted. if let Some(client) = clients.get(entity) { client .send_fallible(ServerGeneral::FinishedTrade(TradeResult::Declined));