From f05c120fabbd4805e4b2323a1bb1ab439a114600 Mon Sep 17 00:00:00 2001
From: termac <termac@gmx.net>
Date: Tue, 25 Aug 2020 10:48:19 +0200
Subject: [PATCH] Fix bug collecting items into full inventory

When the inventory is full and a player tries to reclaim an item from a
block, collecting always failed. If the item is stackable and already
present inside the inventory it should be collected though.
The collect case now behaves more like the pickup case, using
inventory's 'push' method to add the item and implicitly check for
available space.
---
 common/src/state.rs                  | 12 ++++--
 server/src/events/inventory_manip.rs | 63 +++++++++++++++++++++-------
 server/src/state_ext.rs              | 18 --------
 3 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/common/src/state.rs b/common/src/state.rs
index 74919be12e..e856b45ed0 100644
--- a/common/src/state.rs
+++ b/common/src/state.rs
@@ -249,10 +249,14 @@ impl State {
         self.ecs.write_resource::<BlockChange>().set(pos, block);
     }
 
-    /// Set a block in this state's terrain. Will return `None` if the block has
-    /// already been modified this tick.
-    pub fn try_set_block(&mut self, pos: Vec3<i32>, block: Block) -> Option<()> {
-        self.ecs.write_resource::<BlockChange>().try_set(pos, block)
+    /// Check if the block at given position `pos` has already been modified
+    /// this tick.
+    pub fn can_set_block(&mut self, pos: Vec3<i32>) -> bool {
+        !self
+            .ecs
+            .write_resource::<BlockChange>()
+            .blocks
+            .contains_key(&pos)
     }
 
     /// Removes every chunk of the terrain.
diff --git a/server/src/events/inventory_manip.rs b/server/src/events/inventory_manip.rs
index 4d2a1a6650..e517060ea3 100644
--- a/server/src/events/inventory_manip.rs
+++ b/server/src/events/inventory_manip.rs
@@ -100,23 +100,54 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
             let block = state.terrain().get(pos).ok().copied();
 
             if let Some(block) = block {
-                let has_inv_space = state
-                    .ecs()
-                    .read_storage::<comp::Inventory>()
-                    .get(entity)
-                    .map(|inv| !inv.is_full())
-                    .unwrap_or(false);
-
-                if !has_inv_space {
-                    state.write_component(
-                        entity,
-                        comp::InventoryUpdate::new(comp::InventoryUpdateEvent::CollectFailed),
+                if block.is_collectible() && state.can_set_block(pos) {
+                    if let Some(item) = comp::Item::try_reclaim_from_block(block) {
+                        let (event, item_was_added) = if let Some(inv) = state
+                            .ecs()
+                            .write_storage::<comp::Inventory>()
+                            .get_mut(entity)
+                        {
+                            match inv.push(item.clone()) {
+                                None => (
+                                    Some(comp::InventoryUpdate::new(
+                                        comp::InventoryUpdateEvent::Collected(item),
+                                    )),
+                                    true,
+                                ),
+                                Some(_) => (
+                                    Some(comp::InventoryUpdate::new(
+                                        comp::InventoryUpdateEvent::CollectFailed,
+                                    )),
+                                    false,
+                                ),
+                            }
+                        } else {
+                            debug!(
+                                "Can't add item to inventory: entity has no inventory ({:?})",
+                                entity
+                            );
+                            (None, false)
+                        };
+                        if let Some(event) = event {
+                            state.write_component(entity, event);
+                            if item_was_added {
+                                // we made sure earlier the block was not already modified this tick
+                                state.set_block(pos, Block::empty())
+                            };
+                        }
+                    } else {
+                        debug!(
+                            "Failed to reclaim item from block at pos={} or entity had no \
+                             inventory",
+                            pos
+                        )
+                    }
+                } else {
+                    debug!(
+                        "Can't reclaim item from block at pos={}: block is not collectable or was \
+                         already set this tick.",
+                        pos
                     );
-                } else if block.is_collectible()
-                    && state.try_set_block(pos, Block::empty()).is_some()
-                {
-                    comp::Item::try_reclaim_from_block(block)
-                        .map(|item| state.give_item(entity, item));
                 }
             }
         },
diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs
index 51646d9ab4..d7190228d6 100644
--- a/server/src/state_ext.rs
+++ b/server/src/state_ext.rs
@@ -18,8 +18,6 @@ use tracing::warn;
 use vek::*;
 
 pub trait StateExt {
-    /// Push an item into the provided entity's inventory
-    fn give_item(&mut self, entity: EcsEntity, item: comp::Item) -> bool;
     /// Updates a component associated with the entity based on the `Effect`
     fn apply_effect(&mut self, entity: EcsEntity, effect: Effect);
     /// Build a non-player character
@@ -56,22 +54,6 @@ pub trait StateExt {
 }
 
 impl StateExt for State {
-    fn give_item(&mut self, entity: EcsEntity, item: comp::Item) -> bool {
-        let success = self
-            .ecs()
-            .write_storage::<comp::Inventory>()
-            .get_mut(entity)
-            .map(|inv| inv.push(item.clone()).is_none())
-            .unwrap_or(false);
-        if success {
-            self.write_component(
-                entity,
-                comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Collected(item)),
-            );
-        }
-        success
-    }
-
     fn apply_effect(&mut self, entity: EcsEntity, effect: Effect) {
         match effect {
             Effect::Health(change) => {