Fixed Panic when dropping item from inventory while chest is open #295 (#320)

## Description

Fixed #295 by checking the slot ids of the drop event and only updating
the player inventory/the target inventory accordingly.

## Test Plan
Steps:
1. Rerun the test from #295, it works
2. Test other inventories in game
...

(No details, it only uses the chest example.)

---------

Co-authored-by: Sese Mueller <sese4dasbinichagmail.com>

crates/valence/src/inventory.rs

@@ -763,27 +763,111 @@ fn handle_click_slot(
             // The client is dropping an item by pressing the drop key.
 
             let entire_stack = pkt.button == 1;
-            if let Some(stack) = client_inv.slot(pkt.slot_idx as u16) {
-                // TODO: is the use of `replace_slot` here causing unnecessary packets to be
-                // sent?
-                let dropped = if entire_stack || stack.count() == 1 {
-                    client_inv.replace_slot(pkt.slot_idx as u16, None)
-                } else {
-                    let mut stack = stack.clone();
-                    stack.set_count(stack.count() - 1);
-                    let mut old_slot = client_inv.replace_slot(pkt.slot_idx as u16, Some(stack));
-                    // we already checked that the slot was not empty and that the
-                    // stack count is > 1
-                    old_slot.as_mut().unwrap().set_count(1);
-                    old_slot
-                }
-                .expect("dropped item should exist"); // we already checked that the slot was not empty
 
-                drop_item_stack_events.send(DropItemStack {
-                    client: packet.client,
-                    from_slot: Some(pkt.slot_idx as u16),
-                    stack: dropped,
-                });
+            // Needs to open the inventory for if the player is dropping an item while having an inventory open.
+            if let Some(open_inventory) = open_inventory {
+                // The player is interacting with an inventory that is open.
+
+                let Ok(mut target_inventory) = inventories.get_mut(open_inventory.entity) else {
+                    // The inventory does not exist, ignore.
+                    continue;
+                };
+
+                if inv_state.state_id.0 != pkt.state_id.0 {
+                    // Client is out of sync. Resync and ignore click.
+
+                    debug!("Client state id mismatch, resyncing");
+
+                    inv_state.state_id += 1;
+
+                    client.write_packet(&InventoryS2c {
+                        window_id: inv_state.window_id,
+                        state_id: VarInt(inv_state.state_id.0),
+                        slots: Cow::Borrowed(target_inventory.slot_slice()),
+                        carried_item: Cow::Borrowed(&cursor_item.0),
+                    });
+
+                    continue;
+                }
+
+                if (0i16..target_inventory.slot_count() as i16).contains(&pkt.slot_idx) {
+                    // The player is dropping an item from another inventory.
+
+                    if let Some(stack) = target_inventory.slot(pkt.slot_idx as u16) {
+                        // TODO: is the use of `replace_slot` here causing unnecessary packets to be
+                        // sent?
+                        let dropped = if entire_stack || stack.count() == 1 {
+                            target_inventory.replace_slot(pkt.slot_idx as u16, None)
+                        } else {
+                            let mut stack = stack.clone();
+                            stack.set_count(stack.count() - 1);
+                            let mut old_slot =
+                                target_inventory.replace_slot(pkt.slot_idx as u16, Some(stack));
+                            // we already checked that the slot was not empty and that the
+                            // stack count is > 1
+                            old_slot.as_mut().unwrap().set_count(1);
+                            old_slot
+                        }
+                        .expect("dropped item should exist"); // we already checked that the slot was not empty
+
+                        drop_item_stack_events.send(DropItemStack {
+                            client: packet.client,
+                            from_slot: Some(pkt.slot_idx as u16),
+                            stack: dropped,
+                        });
+                    }
+                } else {
+                    // The player is dropping an item from their inventory.
+                    let slot_id =
+                        convert_to_player_slot_id(target_inventory.kind, pkt.slot_idx as u16);
+                    if let Some(stack) = client_inv.slot(slot_id) {
+                        // TODO: is the use of `replace_slot` here causing unnecessary packets to be
+                        // sent?
+                        let dropped = if entire_stack || stack.count() == 1 {
+                            client_inv.replace_slot(slot_id, None)
+                        } else {
+                            let mut stack = stack.clone();
+                            stack.set_count(stack.count() - 1);
+                            let mut old_slot = client_inv.replace_slot(slot_id, Some(stack));
+                            // we already checked that the slot was not empty and that the
+                            // stack count is > 1
+                            old_slot.as_mut().unwrap().set_count(1);
+                            old_slot
+                        }
+                        .expect("dropped item should exist"); // we already checked that the slot was not empty
+
+                        drop_item_stack_events.send(DropItemStack {
+                            client: packet.client,
+                            from_slot: Some(slot_id),
+                            stack: dropped,
+                        });
+                    }
+                }
+            } else {
+                // The player has no inventory open and is dropping an item from their inventory.
+                if let Some(stack) = client_inv.slot(pkt.slot_idx as u16) {
+                    // TODO: is the use of `replace_slot` here causing unnecessary packets to be
+                    // sent?
+                    let dropped = if entire_stack || stack.count() == 1 {
+                        client_inv.replace_slot(pkt.slot_idx as u16, None)
+                    } else {
+                        let mut stack = stack.clone();
+                        stack.set_count(stack.count() - 1);
+                        let mut old_slot =
+                            client_inv.replace_slot(pkt.slot_idx as u16, Some(stack));
+                        // we already checked that the slot was not empty and that the
+                        // stack count is > 1
+                        old_slot.as_mut().unwrap().set_count(1);
+                        old_slot
+                    }
+                    .expect("dropped item should exist"); // we already checked that the slot was not empty
+
+                    drop_item_stack_events.send(DropItemStack {
+                        client: packet.client,
+                        from_slot: Some(pkt.slot_idx as u16),
+                        stack: dropped,
+                    });
+                }
             }
         } else {
             // The player is clicking a slot in an inventory.
@@ -2025,6 +2109,145 @@ mod test {
                 ItemStack::new(ItemKind::IronIngot, 32, None)
             );
         }
+
+        #[test]
+        fn should_drop_item_player_open_inventory_with_dropkey() {
+            // The item should be dropped successfully, if the player has an inventory open and the slot id points to his inventory.
+
+            let mut app = App::new();
+            let (client_ent, mut client_helper) = scenario_single_client(&mut app);
+            let mut inventory = app
+                .world
+                .get_mut::<Inventory>(client_ent)
+                .expect("could not find inventory");
+            inventory.set_slot(
+                convert_to_player_slot_id(InventoryKind::Generic9x3, 50),
+                ItemStack::new(ItemKind::IronIngot, 32, None),
+            );
+            let _inventory_ent = set_up_open_inventory(&mut app, client_ent);
+
+            // Process a tick to get past the "on join" logic.
+            app.update();
+            client_helper.clear_sent();
+
+            let inv_state = app
+                .world
+                .get_mut::<ClientInventoryState>(client_ent)
+                .expect("could not find client");
+
+            let state_id = inv_state.state_id.0;
+            let window_id = inv_state.window_id;
+
+            client_helper.send(&valence_protocol::packet::c2s::play::ClickSlotC2s {
+                window_id,
+                slot_idx: 50,
+                button: 0, // not pressing control
+                mode: ClickMode::DropKey,
+                state_id: VarInt(state_id),
+                slot_changes: vec![Slot {
+                    idx: 50,
+                    item: Some(ItemStack::new(ItemKind::IronIngot, 31, None)),
+                }],
+                carried_item: None,
+            });
+
+            app.update();
+
+            // Make assertions
+            let events = app
+                .world
+                .get_resource::<Events<DropItemStack>>()
+                .expect("expected drop item stack events");
+            let player_inventory = app
+                .world
+                .get::<Inventory>(client_ent)
+                .expect("could not find inventory");
+            let events = events.iter_current_update_events().collect::<Vec<_>>();
+            assert_eq!(events.len(), 1);
+            assert_eq!(events[0].client, client_ent);
+            assert_eq!(
+                events[0].from_slot,
+                Some(convert_to_player_slot_id(InventoryKind::Generic9x3, 50))
+            );
+            assert_eq!(
+                events[0].stack,
+                ItemStack::new(ItemKind::IronIngot, 1, None)
+            );
+            // Also make sure that the player inventory was updated correctly.
+            let expected_player_slot_id = convert_to_player_slot_id(InventoryKind::Generic9x3, 50);
+            assert_eq!(
+                player_inventory.slot(expected_player_slot_id),
+                Some(&ItemStack::new(ItemKind::IronIngot, 31, None))
+            );
+        }
+    }
+
+    #[test]
+    fn should_drop_item_stack_player_open_inventory_with_dropkey() {
+        // The item stack should be dropped successfully, if the player has an inventory open and the slot id points to his inventory.
+
+        let mut app = App::new();
+        let (client_ent, mut client_helper) = scenario_single_client(&mut app);
+        let mut inventory = app
+            .world
+            .get_mut::<Inventory>(client_ent)
+            .expect("could not find inventory");
+        inventory.set_slot(
+            convert_to_player_slot_id(InventoryKind::Generic9x3, 50),
+            ItemStack::new(ItemKind::IronIngot, 32, None),
+        );
+        let _inventory_ent = set_up_open_inventory(&mut app, client_ent);
+
+        // Process a tick to get past the "on join" logic.
+        app.update();
+        client_helper.clear_sent();
+
+        let inv_state = app
+            .world
+            .get_mut::<ClientInventoryState>(client_ent)
+            .expect("could not find client");
+
+        let state_id = inv_state.state_id.0;
+        let window_id = inv_state.window_id;
+
+        client_helper.send(&valence_protocol::packet::c2s::play::ClickSlotC2s {
+            window_id,
+            slot_idx: 50,
+            button: 1, // pressing control, the whole stack is dropped
+            mode: ClickMode::DropKey,
+            state_id: VarInt(state_id),
+            slot_changes: vec![Slot {
+                idx: 50,
+                item: None,
+            }],
+            carried_item: None,
+        });
+
+        app.update();
+
+        // Make assertions
+        let events = app
+            .world
+            .get_resource::<Events<DropItemStack>>()
+            .expect("expected drop item stack events");
+        let player_inventory = app
+            .world
+            .get::<Inventory>(client_ent)
+            .expect("could not find inventory");
+        let events = events.iter_current_update_events().collect::<Vec<_>>();
+        assert_eq!(events.len(), 1);
+        assert_eq!(events[0].client, client_ent);
+        assert_eq!(
+            events[0].from_slot,
+            Some(convert_to_player_slot_id(InventoryKind::Generic9x3, 50))
+        );
+        assert_eq!(
+            events[0].stack,
+            ItemStack::new(ItemKind::IronIngot, 32, None)
+        );
+        // Also make sure that the player inventory was updated correctly.
+        let expected_player_slot_id = convert_to_player_slot_id(InventoryKind::Generic9x3, 50);
+        assert_eq!(player_inventory.slot(expected_player_slot_id), None);
     }
 
     #[test]