address review comments

client/src/lib.rs

@@ -99,7 +99,7 @@ const PING_ROLLING_AVERAGE_SECS: usize = 10;
 #[derive(Debug)]
 pub enum Event {
     Chat(comp::ChatMsg),
-    GroupInventoryUpdate(comp::Item, Uid),
+    GroupInventoryUpdate(comp::FrontendItem, Uid),
     InviteComplete {
         target: Uid,
         answer: InviteAnswer,

common/net/src/msg/server.rs

@@ -163,7 +163,7 @@ pub enum ServerGeneral {
     /// currently pending
     InvitePending(Uid),
     /// Update the HUD of the clients in the group
-    GroupInventoryUpdate(comp::Item, Uid),
+    GroupInventoryUpdate(comp::FrontendItem, Uid),
     /// Note: this could potentially include all the failure cases such as
     /// inviting yourself in which case the `InvitePending` message could be
     /// removed and the client could consider their invite pending until

common/src/comp/inventory/item/mod.rs

@@ -475,6 +475,11 @@ pub struct Item {
     durability_lost: Option<u32>,
 }
 
+/// Newtype around [`Item`] used for frontend events to prevent it accidentally
+/// being used for anything other than frontend events
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
+pub struct FrontendItem(Item);
+
 // An item that is dropped into the world an can be picked up. It can stack with
 // other items of the same type regardless of the stack limit, when picked up
 // the last item from the list is popped
@@ -1034,6 +1039,16 @@ impl Item {
         ))
     }
 
+    /// Creates a [`FrontendItem`] out of this item for frontend use
+    #[must_use]
+    pub fn frontend_item(
+        &self,
+        ability_map: &AbilityMap,
+        msm: &MaterialStatManifest,
+    ) -> FrontendItem {
+        FrontendItem(self.duplicate(ability_map, msm))
+    }
+
     /// Duplicates an item, creating an exact copy but with a new item ID
     #[must_use]
     pub fn duplicate(&self, ability_map: &AbilityMap, msm: &MaterialStatManifest) -> Self {
@@ -1532,6 +1547,19 @@ impl Item {
     }
 }
 
+impl FrontendItem {
+    /// See [`Item::duplicate`], the returned item will still be a
+    /// [`FrontendItem`]
+    #[must_use]
+    pub fn duplicate(&self, ability_map: &AbilityMap, msm: &MaterialStatManifest) -> Self {
+        FrontendItem(self.0.duplicate(ability_map, msm))
+    }
+
+    pub fn set_amount(&mut self, amount: u32) -> Result<(), OperationFailure> {
+        self.0.set_amount(amount)
+    }
+}
+
 impl PickupItem {
     pub fn new(item: Item, time: ProgramTime) -> Self {
         Self {
@@ -1727,6 +1755,42 @@ impl ItemDesc for Item {
     }
 }
 
+impl ItemDesc for FrontendItem {
+    fn description(&self) -> &str {
+        #[allow(deprecated)]
+        self.0.description()
+    }
+
+    fn name(&self) -> Cow<str> {
+        #[allow(deprecated)]
+        self.0.name()
+    }
+
+    fn kind(&self) -> Cow<ItemKind> { self.0.kind() }
+
+    fn amount(&self) -> NonZeroU32 { self.0.amount }
+
+    fn quality(&self) -> Quality { self.0.quality() }
+
+    fn num_slots(&self) -> u16 { self.0.num_slots() }
+
+    fn item_definition_id(&self) -> ItemDefinitionId<'_> { self.0.item_definition_id() }
+
+    fn tags(&self) -> Vec<ItemTag> { self.0.tags() }
+
+    fn is_modular(&self) -> bool { self.0.is_modular() }
+
+    fn components(&self) -> &[Item] { self.0.components() }
+
+    fn has_durability(&self) -> bool { self.0.has_durability() }
+
+    fn durability_lost(&self) -> Option<u32> { self.0.durability_lost() }
+
+    fn stats_durability_multiplier(&self) -> DurabilityMultiplier {
+        self.0.stats_durability_multiplier()
+    }
+}
+
 impl ItemDesc for ItemDef {
     fn description(&self) -> &str {
         #[allow(deprecated)]

common/src/comp/inventory/mod.rs

@@ -26,6 +26,8 @@ use crate::{
     LoadoutBuilder,
 };
 
+use super::FrontendItem;
+
 pub mod item;
 pub mod loadout;
 pub mod loadout_builder;
@@ -255,20 +257,25 @@ impl Inventory {
     /// called
     pub fn next_sort_order(&self) -> InventorySortOrder { self.next_sort_order }
 
-    /// Adds a new item to the first fitting group of the inventory or starts a
+    /// Adds a new item to the fitting slots of the inventory or starts a
-    /// new group. Returns the item in an error if no space was found, otherwise
+    /// new group. Returns the item in an error if no space was found.
-    /// returns the found slot.
+    ///
-    pub fn push(&mut self, mut item: Item) -> Result<(), Item> {
+    /// WARNING: This **may** make inventory modifications if `Err(item)` is
+    /// returned. The second tuple field in the error is the number of items
+    /// that were successfully inserted into the inventory.
+    pub fn push(&mut self, mut item: Item) -> Result<(), (Item, Option<NonZeroU32>)> {
         // If the item is stackable, we can increase the amount of other equal items up
         // to max_amount before inserting a new item if there is still a remaining
         // amount (caused by overflow or no other equal stackable being present in the
         // inventory).
         if item.is_stackable() {
+            let total_amount = item.amount();
+
             let remaining = self
                 .slots_mut()
                 .filter_map(Option::as_mut)
                 .filter(|s| *s == &item)
-                .try_fold(item.amount(), |remaining, current| {
+                .try_fold(total_amount, |remaining, current| {
                     debug_assert_eq!(
                         item.max_amount(),
                         current.max_amount(),
@@ -301,13 +308,13 @@ impl Inventory {
                 item.set_amount(remaining)
                     .expect("Remaining is known to be > 0");
                 self.insert(item)
+                    .map_err(|item| (item, NonZeroU32::new(total_amount - remaining)))
             } else {
                 Ok(())
             }
         } else {
-            // No existing item to stack with or item not stackable, put the item in a new
+            // The item isn't stackable, insert it directly
-            // slot
+            self.insert(item).map_err(|item| (item, None))
-            self.insert(item)
         }
     }
 
@@ -317,7 +324,7 @@ impl Inventory {
         // Vec doesn't allocate for zero elements so this should be cheap
         let mut leftovers = Vec::new();
         for item in items {
-            if let Err(item) = self.push(item) {
+            if let Err((item, _)) = self.push(item) {
                 leftovers.push(item);
             }
         }
@@ -341,7 +348,7 @@ impl Inventory {
         let mut leftovers = Vec::new();
         for item in &mut items {
             if self.contains(&item).not() {
-                if let Err(overflow) = self.push(item) {
+                if let Err((overflow, _)) = self.push(item) {
                     leftovers.push(overflow);
                 }
             } // else drop item if it was already in
@@ -593,7 +600,8 @@ impl Inventory {
         }
     }
 
-    /// Takes an amount of items from a slot
+    /// Takes an amount of items from a slot. If the amount to take is larger
+    /// than the item amount, the item amount will be returned instead.
     pub fn take_amount(
         &mut self,
         inv_slot_id: InvSlotId,
@@ -602,23 +610,17 @@ impl Inventory {
         msm: &MaterialStatManifest,
     ) -> Option<Item> {
         if let Some(Some(item)) = self.slot_mut(inv_slot_id) {
-            // We can return at most item.amount()
+            if item.is_stackable() && item.amount() > amount.get() {
-            let return_amount = amount.get().min(item.amount());
-
-            if item.is_stackable() && item.amount() > 1 && return_amount != item.amount() {
                 let mut return_item = item.duplicate(ability_map, msm);
+                let return_amount = amount.get();
+                // Will never overflow since we know item.amount() > amount.get()
+                let new_amount = item.amount() - return_amount;
+
                 return_item
                     .set_amount(return_amount)
-                    .expect("We know that 0 < return_amount <= item.amount()");
+                    .expect("We know that 0 < return_amount < item.amount()");
-
+                item.set_amount(new_amount)
-                let new_amount = item
+                    .expect("new_amount must be > 0 since return item is < item.amount");
-                    .amount()
-                    .checked_sub(return_amount)
-                    .expect("This subtraction must succeed because return_amount < item.amount()");
-                item.set_amount(new_amount).expect(
-                    "The result of item.amount() - return_amount must be > 0 since return_amount \
-                     < item.amount()",
-                );
 
                 Some(return_item)
             } else {
@@ -784,13 +786,16 @@ impl Inventory {
     /// picked up and pushing them to the inventory to ensure that items
     /// containing items aren't inserted into the inventory as this is not
     /// currently supported.
-    pub fn pickup_item(&mut self, mut item: Item) -> Result<(), Item> {
+    ///
+    /// WARNING: The `Err(_)` variant may still cause inventory modifications,
+    /// note on [`Inventory::push`]
+    pub fn pickup_item(&mut self, mut item: Item) -> Result<(), (Item, Option<NonZeroU32>)> {
         if item.is_stackable() {
             return self.push(item);
         }
 
         if self.free_slots() < item.populated_slots() + 1 {
-            return Err(item);
+            return Err((item, None));
         }
 
         // Unload any items contained within the item, and push those items and the item
@@ -1104,7 +1109,7 @@ pub enum InventoryUpdateEvent {
     Given,
     Swapped,
     Dropped,
-    Collected(Item),
+    Collected(FrontendItem),
     BlockCollectFailed {
         pos: Vec3<i32>,
         reason: CollectFailedReason,

common/src/comp/inventory/test.rs

@@ -28,7 +28,7 @@ fn push_full() {
     assert_eq!(
         inv.push(TEST_ITEMS[0].duplicate(ability_map, msm))
             .unwrap_err(),
-        TEST_ITEMS[0].duplicate(ability_map, msm)
+        (TEST_ITEMS[0].duplicate(ability_map, msm), None)
     )
 }
 

common/src/comp/mod.rs

@@ -78,7 +78,7 @@ pub use self::{
             self,
             item_key::ItemKey,
             tool::{self, AbilityItem},
-            Item, ItemConfig, ItemDrops, PickupItem,
+            FrontendItem, Item, ItemConfig, ItemDrops, PickupItem,
         },
         slot, CollectFailedReason, Inventory, InventoryUpdate, InventoryUpdateEvent,
     },

server/src/cmd.rs

@@ -71,7 +71,7 @@ use hashbrown::{HashMap, HashSet};
 use humantime::Duration as HumanDuration;
 use rand::{thread_rng, Rng};
 use specs::{storage::StorageEntry, Builder, Entity as EcsEntity, Join, LendJoin, WorldExt};
-use std::{fmt::Write, ops::DerefMut, str::FromStr, sync::Arc, time::Duration};
+use std::{fmt::Write, num::NonZeroU32, ops::DerefMut, str::FromStr, sync::Arc, time::Duration};
 use vek::*;
 use wiring::{Circuit, Wire, WireNode, WiringAction, WiringActionEffect, WiringElement};
 use world::util::{Sampler, LOCALITY};
@@ -2691,7 +2691,7 @@ fn push_item(
     item_id: KitEntry,
     quantity: u32,
     server: &Server,
-    push: &mut dyn FnMut(Item) -> Result<(), Item>,
+    push: &mut dyn FnMut(Item) -> Result<(), (Item, Option<NonZeroU32>)>,
 ) -> CmdResult<()> {
     let items = match item_id {
         KitEntry::Spec(KitSpec::Item(item_id)) => vec![

server/src/events/inventory_manip.rs

@@ -242,8 +242,7 @@ impl ServerEvent for InventoryManipEvent {
 
                     let (item, reinsert_item) = item.pick_up();
 
-                    // NOTE: We dup the item for message purposes.
+                    let mut item_msg = item.frontend_item(&data.ability_map, &data.msm);
-                    let item_msg = item.duplicate(&data.ability_map, &data.msm);
 
                     // Next, we try to equip the picked up item
                     let event = match inventory.try_equip(item).or_else(|returned_item| {
@@ -251,7 +250,7 @@ impl ServerEvent for InventoryManipEvent {
                         // attempt to add the item to the entity's inventory
                         inventory.pickup_item(returned_item)
                     }) {
-                        Err(returned_item) => {
+                        Err((returned_item, inserted)) => {
                             // If we had a `reinsert_item`, merge returned_item into it
                             let returned_item = if let Some(mut reinsert_item) = reinsert_item {
                                 reinsert_item
@@ -271,10 +270,39 @@ impl ServerEvent for InventoryManipEvent {
                             data.items
                                 .insert(item_entity, returned_item)
                                 .expect(ITEM_ENTITY_EXPECT_MESSAGE);
-                            comp::InventoryUpdate::new(InventoryUpdateEvent::EntityCollectFailed {
+
+                            // If the item was partially picked up, send a loot annoucement.
+                            if let Some(inserted) = inserted {
+                                // Update the frontend item to the new amount
+                                item_msg
+                                    .set_amount(inserted.get())
+                                    .expect("Inserted must be > 0 and <= item.max_amount()");
+
+                                if let Some(group_id) = data.groups.get(entity) {
+                                    announce_loot_to_group(
+                                        group_id,
+                                        entity,
+                                        item_msg.duplicate(&data.ability_map, &data.msm),
+                                        &data.clients,
+                                        &data.uids,
+                                        &data.groups,
+                                        &data.alignments,
+                                        &data.entities,
+                                        &data.ability_map,
+                                        &data.msm,
+                                    );
+                                }
+                                comp::InventoryUpdate::new(InventoryUpdateEvent::Collected(
+                                    item_msg,
+                                ))
+                            } else {
+                                comp::InventoryUpdate::new(
+                                    InventoryUpdateEvent::EntityCollectFailed {
                                         entity: pickup_uid,
                                         reason: CollectFailedReason::InventoryFull,
-                            })
+                                    },
+                                )
+                            }
                         },
                         Ok(_) => {
                             // We succeeded in picking up the item, so we may now delete its old
@@ -334,16 +362,30 @@ impl ServerEvent for InventoryManipEvent {
                                 for item in
                                     flatten_counted_items(&items, &data.ability_map, &data.msm)
                                 {
-                                    // NOTE: We dup the item for message purposes.
+                                    let mut item_msg =
-                                    let item_msg = item.duplicate(&data.ability_map, &data.msm);
+                                        item.frontend_item(&data.ability_map, &data.msm);
-                                    match inventory.push(item) {
+                                    let do_announce = match inventory.push(item) {
-                                        Ok(_) => {
+                                        Ok(_) => true,
+                                        Err((item, inserted)) => {
+                                            drop_items.push(item);
+                                            if let Some(inserted) = inserted {
+                                                // Update the amount of the frontend item
+                                                item_msg.set_amount(inserted.get()).expect(
+                                                    "Inserted must be > 0 and <= item.max_amount()",
+                                                );
+                                                true
+                                            } else {
+                                                false
+                                            }
+                                        },
+                                    };
+
+                                    if do_announce {
                                         if let Some(group_id) = data.groups.get(entity) {
                                             announce_loot_to_group(
                                                 group_id,
                                                 entity,
-                                                    item_msg
+                                                item_msg.duplicate(&data.ability_map, &data.msm),
-                                                        .duplicate(&data.ability_map, &data.msm),
                                                 &data.clients,
                                                 &data.uids,
                                                 &data.groups,
@@ -355,12 +397,6 @@ impl ServerEvent for InventoryManipEvent {
                                         }
                                         inventory_update
                                             .push(InventoryUpdateEvent::Collected(item_msg));
-                                        },
-                                        // The item we created was in some sense "fake" so it's safe
-                                        // to drop it.
-                                        Err(_) => {
-                                            drop_items.push(item_msg);
-                                        },
                                     }
                                 }
                             }
@@ -978,7 +1014,7 @@ impl ServerEvent for InventoryManipEvent {
                     let items_were_crafted = if let Some(crafted_items) = crafted_items {
                         let mut dropped: Vec<PickupItem> = Vec::new();
                         for item in crafted_items {
-                            if let Err(item) = inventory.push(item) {
+                            if let Err((item, _inserted)) = inventory.push(item) {
                                 let item = PickupItem::new(item, *data.program_time);
                                 if let Some(can_merge) =
                                     dropped.iter_mut().find(|other| other.can_merge(&item))
@@ -1116,7 +1152,7 @@ fn within_pickup_range<S: FindDist<find_dist::Cylinder>>(
 fn announce_loot_to_group(
     group_id: &Group,
     entity: EcsEntity,
-    item: comp::Item,
+    item: comp::FrontendItem,
     clients: &ReadStorage<Client>,
     uids: &ReadStorage<Uid>,
     groups: &ReadStorage<comp::Group>,

server/src/events/trade.rs

@@ -376,11 +376,13 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
     for who in [0, 1].iter().cloned() {
         for (slot, quantity) in trade.offers[who].iter() {
             if let Some(quantity) = NonZeroU32::new(*quantity) {
-                inventories
+                if let Some(item) = inventories
                     .get_mut(entities[who])
                     .expect(invmsg)
                     .take_amount(*slot, quantity, &ability_map, &msm)
-                    .map(|item| items[who].push(item));
+                {
+                    items[who].push(item);
+                }
             }
         }
     }

voxygen/src/audio/sfx/mod.rs

@@ -87,7 +87,7 @@ use common::{
     assets::{self, AssetExt, AssetHandle},
     comp::{
         beam, biped_large, biped_small, bird_large, golem, humanoid,
-        item::{item_key::ItemKey, AbilitySpec, ItemDefinitionId, ItemKind, ToolKind},
+        item::{item_key::ItemKey, AbilitySpec, ItemDefinitionId, ItemDesc, ItemKind, ToolKind},
         object,
         poise::PoiseState,
         quadruped_low, quadruped_medium, quadruped_small, Body, CharacterAbilityType, Health,

voxygen/src/hud/loot_scroller.rs

@@ -7,7 +7,10 @@ use super::{
 use crate::ui::{fonts::Fonts, ImageFrame, ItemTooltip, ItemTooltipManager, ItemTooltipable};
 use client::Client;
 use common::{
-    comp::inventory::item::{Item, ItemDesc, ItemI18n, MaterialStatManifest, Quality},
+    comp::{
+        inventory::item::{ItemDesc, ItemI18n, MaterialStatManifest, Quality},
+        FrontendItem,
+    },
     uid::Uid,
 };
 use common_net::sync::WorldSyncExt;
@@ -18,7 +21,7 @@ use conrod_core::{
     widget_ids, Color, Colorable, Positionable, Sizeable, Widget, WidgetCommon,
 };
 use i18n::Localization;
-use std::collections::VecDeque;
+use std::{collections::VecDeque, num::NonZeroU32};
 
 widget_ids! {
     struct Ids{
@@ -107,8 +110,8 @@ impl<'a> LootScroller<'a> {
 
 #[derive(Debug, PartialEq)]
 pub struct LootMessage {
-    pub item: Item,
+    pub item: FrontendItem,
-    pub amount: u32,
+    pub amount: NonZeroU32,
     pub taken_by: Uid,
 }
 
@@ -179,7 +182,9 @@ impl<'a> Widget for LootScroller<'a> {
                             m.item.item_definition_id() == message.item.item_definition_id()
                                 && m.taken_by == message.taken_by
                         }) {
-                            self.new_messages[i].amount += message.amount;
+                            self.new_messages[i].amount = self.new_messages[i]
+                                .amount
+                                .saturating_add(message.amount.get());
                             false
                         } else {
                             true
@@ -393,7 +398,7 @@ impl<'a> Widget for LootScroller<'a> {
                             "hud-loot-pickup-msg-you",
                             &i18n::fluent_args! {
                                   "gender" => user_gender,
-                                  "amount" => amount,
+                                  "amount" => amount.get(),
                                   "item" => {
                                       let (name, _) =
                                           util::item_text(&item, self.localized_strings, self.item_i18n);
@@ -407,7 +412,7 @@ impl<'a> Widget for LootScroller<'a> {
                             &i18n::fluent_args! {
                                   "gender" => user_gender,
                                   "actor" => target_name,
-                                  "amount" => amount,
+                                  "amount" => amount.get(),
                                   "item" => {
                                       let (name, _) =
                                           util::item_text(&item, self.localized_strings, self.item_i18n);