Better consume for inventories (#147)

- rework `consume_one` into `consume`
- update `consume_one_held_item` to `consume_held_item` to match
`consume`
- remove an unnecessary clone

Originally, I was planning on removing this function altogether. After
thinking about it for a bit though, I think it provides enough of a
convenience for end users to warrant keeping it.

This also implements the fix that was originally in #124

examples/building.rs

@@ -199,20 +199,20 @@ impl Config for Game {
                                 if let Some(held_block_kind) = stack.item.to_block_kind() {
                                     let block_to_place = BlockState::from_kind(held_block_kind);
 
-                                    if world
-                                        .chunks
-                                        .block_state(location)
-                                        .map(|s| s.is_replaceable())
-                                        .unwrap_or(false)
+                                    if client.game_mode() == GameMode::Creative
+                                        || client.consume_held_item(1).is_ok()
                                     {
-                                        world.chunks.set_block_state(location, block_to_place);
-                                    } else {
-                                        let place_at = location.get_in_direction(face);
-                                        world.chunks.set_block_state(place_at, block_to_place);
-                                    }
-
-                                    if client.game_mode() != GameMode::Creative {
-                                        client.consume_one_held_item();
+                                        if world
+                                            .chunks
+                                            .block_state(location)
+                                            .map(|s| s.is_replaceable())
+                                            .unwrap_or(false)
+                                        {
+                                            world.chunks.set_block_state(location, block_to_place);
+                                        } else {
+                                            let place_at = location.get_in_direction(face);
+                                            world.chunks.set_block_state(place_at, block_to_place);
+                                        }
                                     }
                                 }
                             }

src/client.rs

@@ -22,7 +22,8 @@ use crate::entity::{
 };
 use crate::ident::Ident;
 use crate::inventory::{
-    Inventories, Inventory, InventoryDirtyable, InventoryId, PlayerInventory, WindowInventory,
+    Inventories, Inventory, InventoryDirtyable, InventoryError, InventoryId, PlayerInventory,
+    WindowInventory,
 };
 use crate::item::ItemStack;
 use crate::player_list::{PlayerListId, PlayerLists};
@@ -794,9 +795,10 @@ impl<C: Config> Client<C> {
         self.inventory.slot(self.selected_hotbar_slot)
     }
 
-    /// Consume a single item from the stack that the client is holding.
-    pub fn consume_one_held_item(&mut self) {
-        self.inventory.consume_one(self.selected_hotbar_slot);
+    /// Consume items from the stack in the client's inventory that the client
+    /// is holding.
+    pub fn consume_held_item(&mut self, amount: impl Into<u8>) -> Result<(), InventoryError> {
+        self.inventory.consume(self.selected_hotbar_slot, amount)
     }
 
     /// Makes the client open a window displaying the given inventory.

src/inventory.rs

@@ -1,5 +1,7 @@
 use std::ops::Range;
 
+use thiserror::Error;
+
 use crate::item::ItemStack;
 use crate::protocol::{SlotId, VarInt};
 use crate::slab_versioned::{Key, VersionedSlab};
@@ -23,17 +25,33 @@ pub trait Inventory {
             .collect()
     }
 
-    fn consume_one(&mut self, slot_id: SlotId) {
-        let mut slot = self.slot(slot_id).cloned();
-        if let Some(stack) = slot.as_mut() {
-            stack.set_count(stack.count() - 1);
-            let slot = if stack.count() == 0 {
+    /// Decreases the count for stack in the slot by amount. If there is not
+    /// enough items in the stack to perform the operation, then it will fail.
+    ///
+    /// Returns `Ok` if the stack had enough items, and the operation was
+    /// carried out. Otherwise, it returns `Err` if `amount > stack.count()`,
+    /// and no changes were made to the inventory.
+    #[allow(clippy::unnecessary_unwrap)]
+    fn consume(&mut self, slot_id: SlotId, amount: impl Into<u8>) -> Result<(), InventoryError> {
+        let amount: u8 = amount.into();
+        let slot = self.slot(slot_id).cloned();
+        if slot.is_some() {
+            // Intentionally not using `if let` so stack can be moved out of the slot as mut
+            // to avoid another clone later.
+            let mut stack = slot.unwrap();
+            if amount > stack.count() {
+                return Err(InventoryError);
+            }
+            let slot = if amount == stack.count() {
                 None
             } else {
+                stack.set_count(stack.count() - amount);
                 Some(stack)
             };
-            self.set_slot(slot_id, slot.cloned());
+
+            self.set_slot(slot_id, slot);
         }
+        Ok(())
     }
 }
 
@@ -251,6 +269,10 @@ impl Inventories {
     }
 }
 
+#[derive(Debug, Error)]
+#[error("InventoryError")]
+pub struct InventoryError;
+
 #[cfg(test)]
 mod test {
     use super::*;
@@ -264,4 +286,18 @@ mod test {
         assert_eq!(inv.slot(9), slot.as_ref());
         assert_eq!(prev, None);
     }
+
+    #[test]
+    fn test_consume() {
+        let mut inv = PlayerInventory::new();
+        let slot_id = 9;
+        let slot = Some(ItemStack::new(ItemKind::Bone, 12, None));
+        inv.set_slot(slot_id, slot);
+        assert!(matches!(inv.consume(slot_id, 2), Ok(_)));
+        assert_eq!(inv.slot(slot_id).unwrap().count(), 10);
+        assert!(matches!(inv.consume(slot_id, 20), Err(_)));
+        assert_eq!(inv.slot(slot_id).unwrap().count(), 10);
+        assert!(matches!(inv.consume(slot_id, 10), Ok(_)));
+        assert_eq!(inv.slot(slot_id), None);
+    }
 }