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

@@ -1456,6 +1456,10 @@ impl Equivalent<ItemDefinitionIdOwned> 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::*;

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<ItemDefinitionIdOwned, (Time, u8)>,
-    ) {
-        (
-            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<InvSlot> to iter of InvSlot
     }
 
+    pub(super) fn inv_slots_mut_with_mutable_recently_unequipped_items(
+        &mut self,
+    ) -> (
+        impl Iterator<Item = &mut InvSlot>,
+        &mut HashMap<ItemDefinitionIdOwned, (Time, u8)>,
+    ) {
+        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<InvSlot> 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<Range<usize>> {
@@ -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() {
+        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;
                 }
-        }
+
-        self.recently_unequipped_items
-            .retain(|_def, (unequip_time, count)| {
                 (time.0 - unequip_time.0 < UNEQUIP_TRACKING_DURATION) && *count > 0
             });
     }

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

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<Item = &mut InvSlot>,
+        &mut HashMap<ItemDefinitionIdOwned, (Time, u8)>,
+    ) {
+        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<Item = (InvSlotId, &InvSlot)> {
         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<ItemDefinitionIdOwned, (Time, u8)>,
-    ) {
-        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,44 +917,18 @@ impl Inventory {
         self.loadout.damage_items(ability_map, msm);
         self.loadout.cull_recently_unequipped_items(time);
 
-        let slots = self
+        let (slots_mut, recently_unequipped_items) =
-            .slots_with_id()
+            self.slots_mut_with_mutable_recently_unequipped_items();
-            .filter(|(_slot, item)| {
+        slots_mut.filter_map(|slot| slot.as_mut()).for_each(|item| {
-                item.as_ref().map_or(false, |item| {
+            if item.durability_lost()
-                    item.durability_lost()
                     .map_or(false, |dur| dur < Item::MAX_DURABILITY)
-                        && self
+                && let Some((_unequip_time, count)) =
-                            .loadout
-                            .recently_unequipped_items
-                            .contains_key(&item.item_definition_id())
-                })
-            })
-            .map(|(slot, _item)| slot)
-            .collect::<Vec<_>>();
-        slots.into_iter().for_each(|slot| {
-            let slot = if let (Some(Some(item)), recently_unequipped_items) =
-                self.slot_with_mutable_recently_unequipped_items(slot)
-            {
-                if let Some((_unequip_time, count)) =
                    recently_unequipped_items.get_mut(&item.item_definition_id())
+                && *count > 0
             {
-                    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);
             }
-            }
         });
     }
 

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::<Inventory>().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::<Client>();
                     let mut agents = ecs.write_storage::<Agent>();
                     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));