fix so cursor packet is sent to client with open inventories (#653)

# Objective

Attempts to solve the issue shown in discord
https://discord.com/channels/998132822239870997/1292890441011957830
where doing  has no effect in a "open inventory"

Looks like an oversight in the system to update the open inventories, no
attempt to send any packet to the user was made when the `CursorItem`
component was updated by any system

Playground to reproduce the issue solved by this pr.
```rust
use valence::interact_block::InteractBlockEvent;
use valence::inventory::ClickSlotEvent;
use valence::log::LogPlugin;
use valence::prelude::*;

#[allow(unused_imports)]
use crate::extras::*;

const SPAWN_Y: i32 = 64;
const CHEST_POS: [i32; 3] = [0, SPAWN_Y + 1, 3];

pub(crate) fn build_app(app: &mut App) {
    app.insert_resource(NetworkSettings {
        connection_mode: ConnectionMode::Offline,
        ..Default::default()
    })
    .add_plugins(DefaultPlugins.build().disable::<LogPlugin>())
    .add_systems(Startup, setup)
    .add_systems(
        EventLoopUpdate,
        (toggle_gamemode_on_sneak, open_chest, select_menu_item),
    )
    .add_systems(Update, (init_clients, despawn_disconnected_clients))
    .run();
}

fn setup(
    mut commands: Commands,
    server: Res<Server>,
    biomes: Res<BiomeRegistry>,
    dimensions: Res<DimensionTypeRegistry>,
) {
    let mut layer = LayerBundle::new(ident!("overworld"), &dimensions, &biomes, &server);

    for z in -5..5 {
        for x in -5..5 {
            layer.chunk.insert_chunk([x, z], UnloadedChunk::new());
        }
    }

    for z in -25..25 {
        for x in -25..25 {
            layer
                .chunk
                .set_block([x, SPAWN_Y, z], BlockState::GRASS_BLOCK);
        }
    }
    layer.chunk.set_block(CHEST_POS, BlockState::CHEST);

    commands.spawn(layer);

    let mut inventory = Inventory::new(InventoryKind::Generic9x3);
    inventory.set_slot(1, ItemStack::new(ItemKind::Apple, 64, None));
    inventory.set_slot(2, ItemStack::new(ItemKind::Diamond, 40, None));
    inventory.set_slot(3, ItemStack::new(ItemKind::Diamond, 30, None));
    commands.spawn(inventory);
}

fn init_clients(
    mut clients: Query<
        (
            &mut EntityLayerId,
            &mut VisibleChunkLayer,
            &mut VisibleEntityLayers,
            &mut Position,
        ),
        Added<Client>,
    >,
    layers: Query<Entity, (With<ChunkLayer>, With<EntityLayer>)>,
) {
    for (mut layer_id, mut visible_chunk_layer, mut visible_entity_layers, mut pos) in &mut clients
    {
        let layer = layers.single();

        layer_id.0 = layer;
        visible_chunk_layer.0 = layer;
        visible_entity_layers.0.insert(layer);
        pos.set([0.0, f64::from(SPAWN_Y) + 1.0, 0.0]);
    }
}

fn open_chest(
    mut commands: Commands,
    inventories: Query<Entity, (With<Inventory>, Without<Client>)>,
    mut events: EventReader<InteractBlockEvent>,
) {
    for event in events.read() {
        if event.position != CHEST_POS.into() {
            continue;
        }
        let open_inventory = OpenInventory::new(inventories.single());
        commands.entity(event.client).insert(open_inventory);
    }
}
fn select_menu_item(
    mut clients: Query<(Entity, &mut CursorItem)>,
    mut events: EventReader<ClickSlotEvent>,
) {
    for event in events.read() {
        let Ok((_player, mut cursor_item)) = clients.get_mut(event.client) else {
            continue;
        };

        // this does not work properly
        cursor_item.set_if_neq(CursorItem(ItemStack::EMPTY));
    }
}
// Add more systems here!
```



# Solution

Copied the approach to sync the `CursorItem` state from the
`update_player_inventories` system to the `update_open_inventories`
where it was previously left out (my assumption is that it was left out
by mistake).

crates/valence_inventory/src/lib.rs

@@ -42,6 +42,7 @@ impl Plugin for InventoryPlugin {
                 update_player_selected_slot,
                 update_open_inventories,
                 update_player_inventories,
+                update_cursor_item,
             )
                 .before(FlushPacketsSet),
         )
@@ -357,10 +358,12 @@ pub struct ClientInventoryState {
     /// Tracks what slots have been changed by this client in this tick, so we
     /// don't need to send updates for them.
     slots_changed: u64,
-    /// Whether the client has updated the cursor item in this tick. This is not
-    /// on the `CursorItem` component to make maintaining accurate change
-    /// detection for end users easier.
-    client_updated_cursor_item: bool,
+    /// If `Some`: The item the user thinks they updated their cursor item to on
+    /// the last tick.
+    /// If `None`: the user did not update their cursor item in the last tick.
+    /// This is so we can inform the user of the update through change detection
+    /// when they differ in a given tick
+    client_updated_cursor_item: Option<ItemStack>,
 }
 
 impl ClientInventoryState {
@@ -579,7 +582,7 @@ fn init_new_client_inventories(clients: Query<Entity, Added<Client>>, mut comman
                 window_id: 0,
                 state_id: Wrapping(0),
                 slots_changed: 0,
-                client_updated_cursor_item: false,
+                client_updated_cursor_item: None,
             },
             HeldItem {
                 // First slot of the hotbar.
@@ -596,7 +599,7 @@ fn update_player_inventories(
             &mut Inventory,
             &mut Client,
             &mut ClientInventoryState,
-            Ref<CursorItem>,
+            &CursorItem,
         ),
         Without<OpenInventory>,
     >,
@@ -647,21 +650,6 @@ fn update_player_inventories(
             inventory.changed = 0;
             inv_state.slots_changed = 0;
         }
-
-        if cursor_item.is_changed() && !inv_state.client_updated_cursor_item {
-            // Contrary to what you might think, we actually don't want to increment the
-            // state ID here because the client doesn't actually acknowledge the
-            // state_id change for this packet specifically. See #304.
-
-            client.write_packet(&ScreenHandlerSlotUpdateS2c {
-                window_id: -1,
-                state_id: VarInt(inv_state.state_id.0),
-                slot_idx: -1,
-                slot_data: Cow::Borrowed(&cursor_item.0),
-            });
-        }
-
-        inv_state.client_updated_cursor_item = false;
     }
 }
 
@@ -752,11 +740,31 @@ fn update_open_inventories(
 
         open_inventory.client_changed = 0;
         inv_state.slots_changed = 0;
-        inv_state.client_updated_cursor_item = false;
         inventory.changed = 0;
     }
 }
 
+fn update_cursor_item(
+    mut clients: Query<(&mut Client, &mut ClientInventoryState, &CursorItem), Changed<CursorItem>>,
+) {
+    for (mut client, mut inv_state, cursor_item) in &mut clients {
+        // The cursor item was not the item the user themselves interacted with
+        if inv_state.client_updated_cursor_item.as_ref() != Some(&cursor_item.0) {
+            // Contrary to what you might think, we actually don't want to increment the
+            // state ID here because the client doesn't actually acknowledge the
+            // state_id change for this packet specifically. See #304.
+            client.write_packet(&ScreenHandlerSlotUpdateS2c {
+                window_id: -1,
+                state_id: VarInt(inv_state.state_id.0),
+                slot_idx: -1,
+                slot_data: Cow::Borrowed(&cursor_item.0),
+            });
+        }
+
+        inv_state.client_updated_cursor_item = None;
+    }
+}
+
 /// Handles clients telling the server that they are closing an inventory.
 fn handle_close_handled_screen(mut packets: EventReader<PacketEvent>, mut commands: Commands) {
     for packet in packets.read() {
@@ -1019,6 +1027,7 @@ fn handle_click_slot(
                 }
 
                 cursor_item.set_if_neq(CursorItem(pkt.carried_item.clone()));
+                inv_state.client_updated_cursor_item = Some(pkt.carried_item.clone());
 
                 for slot in pkt.slot_changes.iter() {
                     if (0_i16..target_inventory.slot_count() as i16).contains(&slot.idx) {
@@ -1054,7 +1063,7 @@ fn handle_click_slot(
                 }
 
                 cursor_item.set_if_neq(CursorItem(pkt.carried_item.clone()));
-                inv_state.client_updated_cursor_item = true;
+                inv_state.client_updated_cursor_item = Some(pkt.carried_item.clone());
 
                 for slot in pkt.slot_changes.iter() {
                     if (0_i16..client_inv.slot_count() as i16).contains(&slot.idx) {

src/tests/inventory.rs

@@ -575,6 +575,29 @@ fn should_not_increment_state_id_on_cursor_item_change() {
     );
 }
 
+#[test]
+fn should_send_cursor_item_change_when_modified_on_the_server() {
+    let ScenarioSingleClient {
+        mut app,
+        client,
+        mut helper,
+        ..
+    } = ScenarioSingleClient::new();
+
+    // Process a tick to get past the "on join" logic.
+    app.update();
+    helper.clear_received();
+
+    let mut cursor_item = app.world_mut().get_mut::<CursorItem>(client).unwrap();
+    cursor_item.0 = ItemStack::new(ItemKind::Diamond, 2, None);
+
+    app.update();
+
+    let sent_packets = helper.collect_received();
+
+    sent_packets.assert_count::<ScreenHandlerSlotUpdateS2c>(1);
+}
+
 mod dropping_items {
     use super::*;
     use crate::inventory::{convert_to_player_slot_id, PlayerAction};