Add HeldItem::set_slot #525
Conversation
There was a problem hiding this comment.
The pattern you'r using made use of a second change detection variable, for the fact that you don't want to send the packet when it's the client that update it I suppose.
But I think it would be better to use the events system to solve this problem (like what was use for ablilitites )
I think it's a better pattern because
- events (when Events rework #487 will be usable) will permit the dev to cancel the client behaviour.
- keep the consistencies to not really use getter and setter for our component.
| pub fn set_slot(&mut self, slot: u16) { | ||
| // temp | ||
| assert!( | ||
| (36..=44).contains(&slot), |
There was a problem hiding this comment.
The packet seems to be using 0..8 https://wiki.vg/Protocol#Set_Held_Item
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
yea right, weird, but not the scope of the pr
|
There are no events involved here, this isn't code that responds to some client request, how would I fire off an event from the set_slot fn without any knowledge of anything other than a number of the selected slot? This is a server-side-only thing where the server can force a client to have a specific hotbar slot selected. |
crates/valence_inventory/src/lib.rs
Outdated
| @@ -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, | |||
There was a problem hiding this comment.
Can't we use bevy's built-in change detection instead?
https://docs.rs/bevy/0.9.1/bevy/ecs/query/struct.Changed.html
There was a problem hiding this comment.
no because when the client change slot, we trigger one.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
i havent touched anything related to c2s in this pr, out of scope?
There was a problem hiding this comment.
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.
crates/valence_inventory/src/lib.rs
Outdated
| @@ -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)>) { | |||
There was a problem hiding this comment.
I would rather use bevy change detection to detect when HeldItem is changed, and bypass the change detection when the c2s packet is received.
There was a problem hiding this comment.
how am I so bad I explaining my thought when it was this easy.
Co-authored-by: Carson McManus <[email protected]>
Objective
This PR adds functionality to allow the server to change the users selected hotbar slot using
HeldItem::set_slot(36..44)