From 475639dec09a27901dffa4766a37c2733a3cccc4 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 5 Jun 2023 15:14:03 -0400 Subject: [PATCH 1/5] refactor handle_click_slot to primarily emit ClickSlot events, and handle those in a new system --- crates/valence_inventory/src/lib.rs | 113 +++++++++++++++++++--------- 1 file changed, 76 insertions(+), 37 deletions(-) diff --git a/crates/valence_inventory/src/lib.rs b/crates/valence_inventory/src/lib.rs index 1bb6fe3f0..d0191638d 100644 --- a/crates/valence_inventory/src/lib.rs +++ b/crates/valence_inventory/src/lib.rs @@ -25,6 +25,7 @@ use std::ops::Range; use bevy_app::prelude::*; use bevy_ecs::prelude::*; +use menu::OpenMenu; use packet::{ ClickMode, ClickSlotC2s, CloseHandledScreenC2s, CloseScreenS2c, CreativeInventoryActionC2s, InventoryS2c, OpenScreenS2c, ScreenHandlerSlotUpdateS2c, SlotChange, UpdateSelectedSlotC2s, @@ -40,6 +41,7 @@ use valence_core::protocol::encode::WritePacket; use valence_core::protocol::var_int::VarInt; use valence_core::text::Text; +pub mod menu; pub mod packet; mod validate; @@ -65,7 +67,8 @@ impl Plugin for InventoryPlugin { .add_systems( ( handle_update_selected_slot, - handle_click_slot, + emit_events_from_packets, + handle_click_slot.after(emit_events_from_packets), handle_creative_inventory_action, handle_close_handled_screen, handle_player_actions, @@ -332,11 +335,11 @@ pub struct ClientInventoryState { state_id: Wrapping, /// 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, + pub(crate) 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, + pub(crate) client_updated_cursor_item: bool, // TODO: make this a separate modifiable component. held_item_slot: u16, } @@ -754,7 +757,7 @@ pub struct DropItemStack { pub stack: ItemStack, } -fn handle_click_slot( +fn emit_events_from_packets( mut packets: EventReader, mut clients: Query<( &mut Client, @@ -969,22 +972,6 @@ fn handle_click_slot( continue; } - - cursor_item.set_if_neq(CursorItem(pkt.carried_item.clone())); - - for slot in pkt.slot_changes.clone() { - if (0i16..target_inventory.slot_count() as i16).contains(&slot.idx) { - // The client is interacting with a slot in the target inventory. - target_inventory.set_slot(slot.idx as u16, slot.item); - open_inventory.client_changed |= 1 << slot.idx; - } else { - // The client is interacting with a slot in their own inventory. - let slot_id = - convert_to_player_slot_id(target_inventory.kind, slot.idx as u16); - client_inv.set_slot(slot_id, slot.item); - inv_state.slots_changed |= 1 << slot_id; - } - } } else { // The client is interacting with their own inventory. @@ -1004,23 +991,6 @@ fn handle_click_slot( continue; } - - cursor_item.set_if_neq(CursorItem(pkt.carried_item.clone())); - inv_state.client_updated_cursor_item = true; - - for slot in pkt.slot_changes.clone() { - if (0i16..client_inv.slot_count() as i16).contains(&slot.idx) { - client_inv.set_slot(slot.idx as u16, slot.item); - inv_state.slots_changed |= 1 << slot.idx; - } else { - // The client is trying to interact with a slot that does not exist, - // ignore. - warn!( - "Client attempted to interact with slot {} which does not exist", - slot.idx - ); - } - } } click_slot_events.send(ClickSlot { @@ -1037,6 +1007,75 @@ fn handle_click_slot( } } +fn handle_click_slot( + mut click_slots: EventReader, + mut clients: Query<( + &mut Client, + &mut Inventory, + &mut ClientInventoryState, + Option<&mut OpenInventory>, + &mut CursorItem, + )>, + mut inventories: Query<&mut Inventory, Without>, +) { + for click in click_slots.iter() { + // The player is clicking a slot in an inventory. + let Ok(( + mut client, + mut client_inv, + mut inv_state, + open_inventory, + mut cursor_item + )) = clients.get_mut(click.client) else { + // The client does not exist, ignore. + continue; + }; + + if let Some(mut 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; + }; + + cursor_item.set_if_neq(CursorItem(click.carried_item.clone())); + inv_state.client_updated_cursor_item = true; + + for slot in click.slot_changes.clone() { + if (0i16..target_inventory.slot_count() as i16).contains(&slot.idx) { + // The client is interacting with a slot in the target inventory. + target_inventory.set_slot(slot.idx as u16, slot.item); + open_inventory.client_changed |= 1 << slot.idx; + } else { + // The client is interacting with a slot in their own inventory. + let slot_id = convert_to_player_slot_id(target_inventory.kind, slot.idx as u16); + client_inv.set_slot(slot_id, slot.item); + inv_state.slots_changed |= 1 << slot_id; + } + } + } else { + // The client is interacting with their own inventory. + cursor_item.set_if_neq(CursorItem(click.carried_item.clone())); + inv_state.client_updated_cursor_item = true; + + for slot in click.slot_changes.clone() { + if (0i16..client_inv.slot_count() as i16).contains(&slot.idx) { + client_inv.set_slot(slot.idx as u16, slot.item); + inv_state.slots_changed |= 1 << slot.idx; + } else { + // The client is trying to interact with a slot that does not exist, + // ignore. + warn!( + "Client attempted to interact with slot {} which does not exist", + slot.idx + ); + } + } + } + } +} + fn handle_player_actions( mut packets: EventReader, mut clients: Query<(&mut Inventory, &mut ClientInventoryState)>, From 7add6230e53a77c3f378ee9e387a84cc86b28f46 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 6 Jun 2023 09:47:52 -0400 Subject: [PATCH 2/5] fix cursor item updates not being sent if an inventory is open --- crates/valence/src/tests/inventory.rs | 42 +++++++++++++++++++++++++++ crates/valence_inventory/src/lib.rs | 21 ++++++++++++-- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/crates/valence/src/tests/inventory.rs b/crates/valence/src/tests/inventory.rs index 19fea190e..fdaf04988 100644 --- a/crates/valence/src/tests/inventory.rs +++ b/crates/valence/src/tests/inventory.rs @@ -339,6 +339,48 @@ fn test_should_modify_open_inventory_server_side() { ); } +#[test] +fn test_should_modify_cursor_item_open_inventory_server_side() { + let mut app = App::new(); + let (client_ent, mut client_helper) = scenario_single_client(&mut app); + 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(); + + // Modify the cursor item. + let mut cursor_item = app + .world + .get_mut::(client_ent) + .expect("could not find inventory for client"); + cursor_item.0 = Some(ItemStack::new(ItemKind::IronIngot, 3, None)); + + app.update(); + + // Make assertions + let sent_packets = client_helper.collect_sent(); + + // because the cursor item was modified server side, the client needs to be + // updated with the change. + sent_packets.assert_count::(1); + for pkt in sent_packets.0 { + if let Ok(pkt) = pkt.decode::() { + assert_eq!(pkt.window_id, -1); + assert_eq!(pkt.slot_idx, -1); + } + } + + let cursor_item = app + .world + .get_mut::(client_ent) + .expect("could not find inventory for client"); + assert_eq!( + cursor_item.0, + Some(ItemStack::new(ItemKind::IronIngot, 3, None)) + ); +} + #[test] fn test_should_sync_entire_open_inventory() { let mut app = App::new(); diff --git a/crates/valence_inventory/src/lib.rs b/crates/valence_inventory/src/lib.rs index d0191638d..68dc05fa9 100644 --- a/crates/valence_inventory/src/lib.rs +++ b/crates/valence_inventory/src/lib.rs @@ -607,6 +607,7 @@ fn update_player_inventories( // state ID here because the client doesn't actually acknowledge the // state_id change for this packet specifically. See #304. + debug!("sending cursor item update"); client.write_packet(&ScreenHandlerSlotUpdateS2c { window_id: -1, state_id: VarInt(inv_state.state_id.0), @@ -627,7 +628,7 @@ fn update_open_inventories( Entity, &mut Client, &mut ClientInventoryState, - &CursorItem, + Ref, &mut OpenInventory, )>, mut inventories: Query<&mut Inventory>, @@ -692,6 +693,7 @@ fn update_open_inventories( for (i, slot) in inventory.slots.iter().enumerate() { if (changed_filtered >> i) & 1 == 1 { + debug!("sending slot update for slot {}", i); client.write_packet(&ScreenHandlerSlotUpdateS2c { window_id: inv_state.window_id as i8, state_id: VarInt(inv_state.state_id.0), @@ -706,8 +708,23 @@ fn update_open_inventories( open_inventory.client_changed = 0; inv_state.slots_changed = 0; - inv_state.client_updated_cursor_item = false; inventory.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. + + debug!("sending cursor item update"); + 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; } } From 0f8199eabb47f0fa80f17d9ab2acd6edd859d5a2 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 6 Jun 2023 09:54:14 -0400 Subject: [PATCH 3/5] very rough inventory menu behavior --- crates/valence_inventory/src/menu.rs | 77 ++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 crates/valence_inventory/src/menu.rs diff --git a/crates/valence_inventory/src/menu.rs b/crates/valence_inventory/src/menu.rs new file mode 100644 index 000000000..41acf1608 --- /dev/null +++ b/crates/valence_inventory/src/menu.rs @@ -0,0 +1,77 @@ +use std::borrow::Cow; + +use bevy_app::{CoreSet, IntoSystemAppConfig, Plugin}; +use bevy_ecs::prelude::*; +use tracing::debug; +use valence_client::event_loop::{EventLoopSchedule, PacketEvent}; +use valence_client::Client; +use valence_core::__private::VarInt; +use valence_core::item::ItemStack; +use valence_core::protocol::encode::WritePacket; +use valence_core::text::Text; + +use crate::packet::{ + ClickSlotC2s, InventoryS2c, OpenScreenS2c, ScreenHandlerSlotUpdateS2c, WindowType, +}; +use crate::{ + validate, ClickSlot, ClientInventoryState, CursorItem, DropItemStack, Inventory, InventoryKind, + OpenInventory, +}; + +#[derive(Debug, Default)] +pub struct InventoryMenuPlugin; + +impl Plugin for InventoryMenuPlugin { + fn build(&self, app: &mut bevy_app::App) { + app.add_system( + handle_click_slot + .in_schedule(EventLoopSchedule) + .in_base_set(CoreSet::PreUpdate), + ); + } +} + +#[derive(Debug, Component)] +pub struct OpenMenu; + +#[derive(Debug, Component)] +pub struct InventoryMenu; + +fn handle_click_slot( + mut clicks: EventReader, + mut clients: Query< + ( + &mut Client, + &mut Inventory, + &mut ClientInventoryState, + &mut CursorItem, + &mut OpenInventory, + ), + With, + >, + mut inventories: Query<&mut Inventory, (Without, With)>, +) { + for click in clicks.iter() { + println!("menu click: {:?}", click); + if let Ok((mut client, mut inv, mut inv_state, mut cursor_item, open_inv)) = + clients.get_mut(click.client) + { + let Ok(mut target) = inventories.get_mut(open_inv.entity) else { + continue; + }; + println!("reset clicked slot"); + target.set_slot(click.slot_id as u16, click.carried_item.clone()); + inv_state.slots_changed &= !(1 << click.slot_id); + client.write_packet(&ScreenHandlerSlotUpdateS2c { + window_id: inv_state.window_id as i8, + state_id: VarInt(inv_state.state_id.0), + slot_idx: click.slot_id, + slot_data: Cow::Borrowed(&click.carried_item), + }); + + println!("clearing cursor item"); + cursor_item.0 = None; + inv_state.client_updated_cursor_item = false; + } + } +} From db94ce78a3622045a32bbfa040ce45ff6e58ba04 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 6 Jun 2023 10:26:11 -0400 Subject: [PATCH 4/5] only allow ClickMode::Click --- crates/valence_inventory/src/menu.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/valence_inventory/src/menu.rs b/crates/valence_inventory/src/menu.rs index 41acf1608..8868cc911 100644 --- a/crates/valence_inventory/src/menu.rs +++ b/crates/valence_inventory/src/menu.rs @@ -11,7 +11,7 @@ use valence_core::protocol::encode::WritePacket; use valence_core::text::Text; use crate::packet::{ - ClickSlotC2s, InventoryS2c, OpenScreenS2c, ScreenHandlerSlotUpdateS2c, WindowType, + ClickMode, ClickSlotC2s, InventoryS2c, OpenScreenS2c, ScreenHandlerSlotUpdateS2c, WindowType, }; use crate::{ validate, ClickSlot, ClientInventoryState, CursorItem, DropItemStack, Inventory, InventoryKind, @@ -52,6 +52,9 @@ fn handle_click_slot( mut inventories: Query<&mut Inventory, (Without, With)>, ) { for click in clicks.iter() { + if click.mode != ClickMode::Click { + continue; + } println!("menu click: {:?}", click); if let Ok((mut client, mut inv, mut inv_state, mut cursor_item, open_inv)) = clients.get_mut(click.client) From de60cbacfe35284122a3aa98c8459eee03fcb66f Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 6 Jun 2023 10:32:30 -0400 Subject: [PATCH 5/5] emit MenuClickEvents for inventory menu clicks --- crates/valence_inventory/src/menu.rs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/crates/valence_inventory/src/menu.rs b/crates/valence_inventory/src/menu.rs index 8868cc911..e4d6d1f79 100644 --- a/crates/valence_inventory/src/menu.rs +++ b/crates/valence_inventory/src/menu.rs @@ -27,7 +27,8 @@ impl Plugin for InventoryMenuPlugin { handle_click_slot .in_schedule(EventLoopSchedule) .in_base_set(CoreSet::PreUpdate), - ); + ) + .add_event::(); } } @@ -50,6 +51,7 @@ fn handle_click_slot( With, >, mut inventories: Query<&mut Inventory, (Without, With)>, + mut menu_clicks: EventWriter, ) { for click in clicks.iter() { if click.mode != ClickMode::Click { @@ -62,6 +64,9 @@ fn handle_click_slot( let Ok(mut target) = inventories.get_mut(open_inv.entity) else { continue; }; + if click.slot_id < 0 || click.slot_id >= (target.slot_count() as i16) { + continue; + } println!("reset clicked slot"); target.set_slot(click.slot_id as u16, click.carried_item.clone()); inv_state.slots_changed &= !(1 << click.slot_id); @@ -75,6 +80,19 @@ fn handle_click_slot( println!("clearing cursor item"); cursor_item.0 = None; inv_state.client_updated_cursor_item = false; + + menu_clicks.send(MenuClickEvent { + client: click.client, + slot_id: click.slot_id as u16, + button: click.button, + }); } } } + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct MenuClickEvent { + pub client: Entity, + pub slot_id: u16, + pub button: i8, +}