Skip to content
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions crates/valence_inventory/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ pub use valence_server::protocol::packets::play::player_action_c2s::PlayerAction
use valence_server::protocol::packets::play::{
ClickSlotC2s, CloseHandledScreenC2s, CloseScreenS2c, CreativeInventoryActionC2s, InventoryS2c,
OpenScreenS2c, PlayerActionC2s, ScreenHandlerSlotUpdateS2c, UpdateSelectedSlotC2s,
UpdateSelectedSlotS2c,
};
use valence_server::protocol::{VarInt, WritePacket};
use valence_server::text::IntoText;
Expand All @@ -54,6 +55,7 @@ impl Plugin for InventoryPlugin {
PostUpdate,
(
update_client_on_close_inventory.before(update_open_inventories),
update_player_selected_slot,
update_open_inventories,
update_player_inventories,
)
Expand Down Expand Up @@ -397,6 +399,8 @@ impl ClientInventoryState {
#[derive(Debug, Clone, Copy, PartialEq, Eq, Component, Deref)]
pub struct HeldItem {
held_item_slot: u16,
#[deref(ignore)]
changed: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we use bevy's built-in change detection instead?
https://docs.rs/bevy/0.9.1/bevy/ecs/query/struct.Changed.html

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no because when the client change slot, we trigger one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that's where I wanted to go with my review, what I proposing is that the system, who handle the packet bypass the change detection, so the only way the user of the api know that something occurs would be the event send.
now you would'nt need a seconde variable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont really understand what needs to be bypassed, as i mentioned, this isnt triggered from something automatically, this function gives a developer the option to change a clients selected slot index. Imo, this is simple enough to use as-is, and when #487 gets finalized, this can be look at again if required?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about the system that handle the c2s held_item packet, the things I don't like here is the fact that you need a double change detection

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i havent touched anything related to c2s in this pr, out of scope?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no because, how the c2s was implemented was making sense when you didn't care that the change detection was trigger, but now you care for your purpose.

}

impl HeldItem {
Expand All @@ -405,6 +409,17 @@ impl HeldItem {
pub fn slot(&self) -> u16 {
self.held_item_slot
}

pub fn set_slot(&mut self, slot: u16) {
// temp
assert!(
(36..=44).contains(&slot),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The packet seems to be using 0..8 https://wiki.vg/Protocol#Set_Held_Item

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware that the packet uses 0..8, the reason why i'm not using it here is to keep it consistent with the slot() return value, which is 36..44, and changing that is out of scope of this PR imo and backwards-compat breaking. (see line 684 where it gets converted)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea right, weird, but not the scope of the pr

"slot index of {slot} out of bounds"
);

self.held_item_slot = slot;
self.changed = true;
}
}

/// The item stack that the client thinks it's holding under the mouse
Expand Down Expand Up @@ -578,6 +593,7 @@ fn init_new_client_inventories(clients: Query<Entity, Added<Client>>, mut comman
HeldItem {
// First slot of the hotbar.
held_item_slot: 36,
changed: false,
},
));
}
Expand Down Expand Up @@ -659,6 +675,20 @@ fn update_player_inventories(
}
}

/// Handles the `HeldItem` component being changed on a client, which
/// indicates that the server has changed the selected hotbar slot.
fn update_player_selected_slot(mut clients: Query<(&mut Client, &mut HeldItem)>) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather use bevy change detection to detect when HeldItem is changed, and bypass the change detection when the c2s packet is received.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how am I so bad I explaining my thought when it was this easy.

for (mut client, mut held_item) in &mut clients {
if held_item.changed {
client.write_packet(&UpdateSelectedSlotS2c {
slot: (held_item.held_item_slot - PLAYER_INVENTORY_MAIN_SLOTS_COUNT) as u8,
});

held_item.changed = false;
}
}
}

/// Handles the `OpenInventory` component being added to a client, which
/// indicates that the client is now viewing an inventory, and sends inventory
/// updates to the client when the inventory is modified.
Expand Down