more tests + fix client changes

Author: maxomatic458

crates/valence_equipment/src/inventory_sync.rs

@@ -1,5 +1,5 @@
 use valence_inventory::player_inventory::PlayerInventory;
-use valence_inventory::{HeldItem, Inventory};
+use valence_inventory::{HeldItem, Inventory, UpdateSelectedSlotEvent};
 use valence_server::entity::player::PlayerEntity;
 
 use super::*;
@@ -9,6 +9,8 @@ pub struct EquipmentInventorySync;
 /// Syncs the player [`Equipment`] with the [`Inventory`].
 /// If a change in the player's inventory and in the equipment occurs in the
 /// same tick, the equipment change has priority.
+/// Note: This system only handles direct changes to the held item (not actual
+/// changes from the client) see [`equipment_held_item_sync_from_client`]
 pub(crate) fn equipment_inventory_sync(
     mut clients: Query<
         (&mut Equipment, &mut Inventory, &mut HeldItem),
@@ -18,13 +20,16 @@ pub(crate) fn equipment_inventory_sync(
             With<PlayerEntity>,
         ),
     >,
+    // mut change_slot_events
 ) {
     for (mut equipment, mut inventory, held_item) in &mut clients {
         // Equipment change has priority over held item changes
         if equipment.changed & (1 << Equipment::MAIN_HAND_IDX) != 0 {
             let item = equipment.main_hand().clone();
             inventory.set_slot(held_item.slot(), item);
         } else if held_item.is_changed() {
+            // This will only be called if we change the held item from valence,
+            // the client change is handled in `equipment_held_item_sync_from_client`
             let item = inventory.slot(held_item.slot()).clone();
             equipment.set_main_hand(item);
         }
@@ -37,19 +42,39 @@ pub(crate) fn equipment_inventory_sync(
             (Equipment::FEET_IDX, PlayerInventory::SLOT_FEET),
         ];
 
+        // We cant rely on the changed bitfield of inventory here
+        // because that gets reset when the client changes the inventory
+
         for (equipment_slot, inventory_slot) in slots {
             // Equipment has priority over inventory changes
             if equipment.changed & (1 << equipment_slot) != 0 {
                 let item = equipment.slot(equipment_slot).clone();
                 inventory.set_slot(inventory_slot, item);
-            } else if inventory.changed & (1 << inventory_slot) != 0 {
+            } else if inventory.is_changed() {
                 let item = inventory.slot(inventory_slot).clone();
                 equipment.set_slot(equipment_slot, item);
             }
         }
     }
 }
 
+/// Handles the case where the client changes the slot (the bevy change is
+/// suppressed for this). See
+/// [`valence_inventory::handle_update_selected_slot`].
+pub(crate) fn equipment_held_item_sync_from_client(
+    mut clients: Query<(&HeldItem, &Inventory, &mut Equipment), With<EquipmentInventorySync>>,
+    mut events: EventReader<UpdateSelectedSlotEvent>,
+) {
+    for event in events.read() {
+        let Ok((held_item, inventory, mut equipment)) = clients.get_mut(event.client) else {
+            continue;
+        };
+
+        let item = inventory.slot(held_item.slot()).clone();
+        equipment.set_main_hand(item);
+    }
+}
+
 pub(crate) fn on_attach_inventory_sync(
     entities: Query<Option<&PlayerEntity>, (Added<EquipmentInventorySync>, With<Inventory>)>,
 ) {

crates/valence_equipment/src/lib.rs

@@ -22,6 +22,7 @@ impl Plugin for EquipmentPlugin {
                 on_entity_init,
                 inventory_sync::on_attach_inventory_sync,
                 inventory_sync::equipment_inventory_sync,
+                inventory_sync::equipment_held_item_sync_from_client,
             ),
         )
         .add_systems(

crates/valence_inventory/src/lib.rs

@@ -1408,6 +1408,9 @@ fn handle_update_selected_slot(
     for packet in packets.read() {
         if let Some(pkt) = packet.decode::<UpdateSelectedSlotC2s>() {
             if let Ok(mut mut_held) = clients.get_mut(packet.client) {
+                // We bypass the change detection here because the server listens for changes
+                // of `HeldItem` in order to send the update to the client.
+                // This is not required here because the update is coming from the client.
                 let held = mut_held.bypass_change_detection();
                 if pkt.slot > 8 {
                     // The client is trying to interact with a slot that does not exist, ignore.

src/tests/equipment.rs

@@ -1,12 +1,14 @@
 use valence_equipment::{Equipment, EquipmentInventorySync};
 use valence_inventory::player_inventory::PlayerInventory;
-use valence_inventory::Inventory;
+use valence_inventory::{ClickMode, ClientInventoryState, Inventory, SlotChange};
 use valence_server::entity::armor_stand::ArmorStandEntityBundle;
 use valence_server::entity::item::ItemEntityBundle;
 use valence_server::entity::zombie::ZombieEntityBundle;
 use valence_server::entity::{EntityLayerId, Position};
 use valence_server::math::DVec3;
-use valence_server::protocol::packets::play::EntityEquipmentUpdateS2c;
+use valence_server::protocol::packets::play::{
+    ClickSlotC2s, EntityEquipmentUpdateS2c, UpdateSelectedSlotC2s,
+};
 use valence_server::{ItemKind, ItemStack};
 
 use crate::testing::ScenarioSingleClient;
@@ -298,7 +300,7 @@ fn test_inventory_sync_from_equipment() {
 }
 
 #[test]
-fn test_inventory_sync_from_inventory() {
+fn test_equipment_sync_from_inventory() {
     let ScenarioSingleClient {
         mut app,
         client,
@@ -405,3 +407,137 @@ fn test_equipment_priority_over_inventory() {
         ItemStack::new(ItemKind::GoldenChestplate, 1, None)
     );
 }
+
+#[test]
+fn test_equipment_change_from_player() {
+    let ScenarioSingleClient {
+        mut app,
+        client,
+        mut helper,
+        ..
+    } = ScenarioSingleClient::new();
+
+    // Process a tick to get past the "on join" logic.
+    app.update();
+    helper.clear_received();
+
+    app.world_mut()
+        .entity_mut(client)
+        .insert(EquipmentInventorySync);
+
+    let mut player_inventory = app
+        .world_mut()
+        .get_mut::<Inventory>(client)
+        .expect("could not get player equipment");
+
+    player_inventory.set_slot(36, ItemStack::new(ItemKind::DiamondChestplate, 1, None));
+    app.update();
+    helper.clear_received();
+
+    let state_id = app
+        .world()
+        .get::<ClientInventoryState>(client)
+        .expect("could not get player equipment")
+        .state_id();
+
+    app.update();
+
+    helper.send(&ClickSlotC2s {
+        window_id: 0,
+        button: 0,
+        mode: ClickMode::Hotbar,
+        state_id: state_id.0.into(),
+        slot_idx: 36,
+        slot_changes: vec![
+            SlotChange {
+                idx: 36,
+                stack: ItemStack::EMPTY,
+            },
+            SlotChange {
+                idx: PlayerInventory::SLOT_CHEST as i16,
+                stack: ItemStack::new(ItemKind::DiamondChestplate, 1, None),
+            },
+        ]
+        .into(),
+        carried_item: ItemStack::EMPTY,
+    });
+
+    app.update();
+    app.update();
+
+    let player_inventory = app
+        .world()
+        .get::<Inventory>(client)
+        .expect("could not get player equipment");
+
+    let player_equipment = app
+        .world()
+        .get::<Equipment>(client)
+        .expect("could not get player equipment");
+
+    assert_eq!(
+        player_inventory.slot(PlayerInventory::SLOT_CHEST),
+        &ItemStack::new(ItemKind::DiamondChestplate, 1, None)
+    );
+
+    assert_eq!(player_inventory.slot(36), &ItemStack::EMPTY);
+
+    assert_eq!(
+        player_equipment.chest(),
+        &ItemStack::new(ItemKind::DiamondChestplate, 1, None)
+    );
+}
+
+#[test]
+fn test_held_item_change_from_client() {
+    let ScenarioSingleClient {
+        mut app,
+        client,
+        mut helper,
+        ..
+    } = ScenarioSingleClient::new();
+
+    // Process a tick to get past the "on join" logic.
+    app.update();
+    helper.clear_received();
+
+    app.world_mut()
+        .entity_mut(client)
+        .insert(EquipmentInventorySync);
+
+    let mut player_inventory = app
+        .world_mut()
+        .get_mut::<Inventory>(client)
+        .expect("could not get player equipment");
+
+    player_inventory.set_slot(36, ItemStack::new(ItemKind::DiamondSword, 1, None));
+    player_inventory.set_slot(37, ItemStack::new(ItemKind::IronSword, 1, None));
+
+    app.update();
+
+    let player_equipment = app
+        .world()
+        .get::<Equipment>(client)
+        .expect("could not get player equipment");
+
+    assert_eq!(
+        player_equipment.main_hand(),
+        &ItemStack::new(ItemKind::DiamondSword, 1, None)
+    );
+
+    // Change the held item from the client
+    helper.send(&UpdateSelectedSlotC2s { slot: 1 });
+
+    app.update(); // handle change slot
+    app.update(); // handle change equipment
+
+    let player_equipment = app
+        .world()
+        .get::<Equipment>(client)
+        .expect("could not get player equipment");
+
+    assert_eq!(
+        player_equipment.main_hand(),
+        &ItemStack::new(ItemKind::IronSword, 1, None)
+    );
+}