Add packet SClickContainer and abstractions #60

Bryntet · 2024-08-24T18:08:11Z

Only the packet and abstraction has been implemented so far, the logic will be implemented shortly.

we will store all opened containers there until closed by all players, and when closed we will save contents to the block in the world. We should implement Drop for OpenedContainer to achieve this.

Bryntet · 2024-09-06T19:53:01Z

Tried to get double-clicking to work, but since everything else works, and I'm kind of tired of Inventory work, I'll just leave my PR as this.

Also, do note that due to how Pumpkin currently handles packets in quick succession (it doesn't, it sometimes drops them), dragging items only partially works.

Bryntet · 2024-09-06T20:18:46Z

In this PR, I have also added the command /echest which is very helpful when testing out the inventory system.

It is a global container, that all players can look at at the same time.

Snowiiii · 2024-09-07T11:11:08Z

pumpkin/src/server/mod.rs

+    pub open_containers: HashMap<u64, OpenContainer>,
+    pub drag_handler: DragHandler,


Luckly. I refactored the Server struct. Now we split most of the things game related into the world. We should put this into World aswell

IMO, containers should not have to be locked on World. World should save a unique ID for each container, and try to lock the container that way.

Snowiiii · 2024-09-07T11:16:34Z

pumpkin-inventory/Cargo.toml

 num-traits = "0.2"
 num-derive = "0.4"
-
-
+thiserror = "1.0.63"


i really should make thiserror a workspace dependency. For now you should just use thiserror = "1.0"

Snowiiii · 2024-09-07T11:17:14Z

pumpkin-inventory/Cargo.toml

+thiserror = "1.0.63"
+itertools = "0.13.0"
+# For items
+pumpkin-world = { path = "../pumpkin-world"}


I usually prefer local crate imports above all other deps

pumpkin-inventory/src/container_click.rs

Snowiiii · 2024-09-07T11:33:41Z

pumpkin-inventory/src/open_container.rs

+    fn all_slots(&mut self) -> Vec<&mut Option<ItemStack>> {
+        self.0.iter_mut().collect()
+    }
+
+    fn all_slots_ref(&self) -> Vec<Option<&ItemStack>> {
+        self.0.iter().map(|slot| slot.as_ref()).collect()
+    }


Does this really need to be an Vec?

Snowiiii · 2024-09-07T11:34:12Z

pumpkin-inventory/src/player.rs

+    fn all_combinable_slots_mut(&mut self) -> Vec<&mut Option<ItemStack>> {
+        self.items.iter_mut().collect()
+    }


Does this really need to be an Vec?

It gets combined with a separate Vec<&mut Option<ItemStack>> later, so we'd have to convert it into a Vec anyways

Doing that as late as possible would be ideal, and it's not a requirement to allocate "right now"

Snowiiii · 2024-09-07T11:35:54Z

pumpkin-protocol/src/server/play/s_click_container.rs

+impl<'de> Deserialize<'de> for SClickContainer {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: de::Deserializer<'de>,
+    {
+        struct Visitor;
+        impl<'de> de::Visitor<'de> for Visitor {
+            type Value = SClickContainer;
+
+            fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
+                formatter.write_str("a valid VarInt encoded in a byte sequence")
+            }
+
+            fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
+            where
+                A: SeqAccess<'de>,
+            {
+                let window_id = seq
+                    .next_element::<u8>()?
+                    .ok_or(de::Error::custom("Failed to decode u8"))?;
+                let state_id = seq
+                    .next_element::<VarInt>()?
+                    .ok_or(de::Error::custom("Failed to decode VarInt"))?;
+
+                let slot = seq
+                    .next_element::<i16>()?
+                    .ok_or(de::Error::custom("Failed to decode i16"))?;
+                let button = seq
+                    .next_element::<i8>()?
+                    .ok_or(de::Error::custom("Failed to decode i8"))?;
+                let mode = seq
+                    .next_element::<VarInt>()?
+                    .ok_or(de::Error::custom("Failed to decode VarInt"))?;
+                let length_of_array = seq
+                    .next_element::<VarInt>()?
+                    .ok_or(de::Error::custom("Failed to decode VarInt"))?;
+                let mut array_of_changed_slots = vec![];
+                for _ in 0..length_of_array.0 {
+                    let slot_number = seq
+                        .next_element::<i16>()?
+                        .ok_or(de::Error::custom("Unable to parse slot"))?;
+                    let slot = seq
+                        .next_element::<Slot>()?
+                        .ok_or(de::Error::custom("Unable to parse item"))?;
+                    array_of_changed_slots.push((slot_number, slot));
+                }
+
+                let carried_item = seq
+                    .next_element::<Slot>()?
+                    .ok_or(de::Error::custom("Failed to decode carried item"))?;
+
+                Ok(SClickContainer {
+                    window_id,
+                    state_id,
+                    slot,
+                    button,
+                    mode,
+                    length_of_array,
+                    array_of_changed_slots,
+                    carried_item,
+                })
+            }
+        }
+
+        deserializer.deserialize_seq(Visitor)
+    }
+}


Why you did this?. Do you need to Deserialize a custom way?. We use an custom ServerPacket impl theń

See line 53 to 65 for the reason why it has to be manually deserialized.

Snowiiii · 2024-09-07T11:37:36Z

pumpkin/src/client/container.rs

+                        match slot {
+                            Some(item) => {
+                                if item.item_id == item_in_pressed_slot.item_id
+                                    && item.item_count != 64


Stack sized does varie. We parse it from items.json

Yes, I thought i put a TODO in the places where needed 👍

Snowiiii · 2024-09-07T11:40:11Z

pumpkin/src/client/container.rs

+        let mut container = OptionallyCombinedContainer::new(&mut self.inventory, opened_container);
+
+        container.handle_item_change(&mut changing_item_slot, slot, MouseClick::Left)?;
+        *self.inventory.get_slot(changing_slot as usize)? = changing_item_slot;


You could also return this

StripedMonkey · 2024-09-07T12:55:29Z

pumpkin-inventory/src/open_container.rs

+        &WindowType::Generic9x3
+    }
+
+    fn all_slots(&mut self) -> Vec<&mut Option<ItemStack>> {


It's possible for this to be a &[Option<&ItemStack>] or a &mut [Option<&ItemStack>] but more than that, I believe it's worth making Chest as a generic interface for any chest, rather than hardcoding it as a 27 item chest. Both double and single chests exist. Is there actually a need for a Chest to exist as more than an alias for a Container<WindowType::Generic9x3>?

My thinking is that maybe a plugin will want to have their own custom rules, and hook in to item presses and such, then they'd just have to define their own struct that implements Container

and the thought is that other containers that require special things be placed/taken from special places, will handle it here.

I get that Chest as it is now is only a wrapper, but since you can't impl a trait for a specific enum variant, it's much easier if you abstract it like this in the long run IMO

StripedMonkey · 2024-09-07T12:57:01Z

pumpkin-inventory/src/player.rs

+    crafting: [Option<ItemStack>; 4],
+    crafting_output: Option<ItemStack>,
+    items: [Option<ItemStack>; 36],
+    armor: [Option<ItemStack>; 4],
+    offhand: Option<ItemStack>,


Similar to above, why not make these Generic containers instead of handling them "manually"

StripedMonkey · 2024-09-07T12:59:04Z

pumpkin-inventory/src/player.rs

+    fn all_combinable_slots_mut(&mut self) -> Vec<&mut Option<ItemStack>> {
+        self.items.iter_mut().collect()
+    }


Doing that as late as possible would be ideal, and it's not a requirement to allocate "right now"

pumpkin/Cargo.toml

…les on handle_item_change

Bryntet and others added 30 commits August 24, 2024 19:15

add packet SClickContainer

507b521

add container click abstraction

83231f4

pass CI

c408244

Merge branch 'Snowiiii:master' into master

d905601

make packet public and more abstraction thing

c968131

Merge remote-tracking branch 'origin/master'

39f1a22

fix deserialization of SClickContainer

7b05ded

add debug of mouse_click

b90fd4c

Add all functionality of mode zero for packet SClickContainer

e0a20c1

remove debug

10e0600

add shift click to packet SClickContainer

b027c88

simplify shift mouse click function

5e466a1

add number button keyclick to packet SClickContainer

e015766

add creative pick item to packet SClickContainer

0f9ac22

refactor to make SClickContainer work in containers other than inventory

d7e059c

Merge branch 'master' into master

b6a6320

Merge branch 'Snowiiii:master' into master

ada8adc

add OpenedContainer to Server

53c3a01

we will store all opened containers there until closed by all players, and when closed we will save contents to the block in the world. We should implement Drop for OpenedContainer to achieve this.

fix lints

f34a483

add development command for opening a global enderchest

909943b

add state id

1d06564

make window_type static in trait

d213fad

change to use correct function

f465bc0

various small changes

460d150

Merge branch 'master' into master

c98c0ec

add working container state and change to fix with latest current branch

d1c4e59

refactor away unnecessary abstraction

2829243

add drag and error handling for containers

bb6677d

clippy and format

4551d0b

add checking gamemode for middleclick dragging

db70ea1

Bryntet added 7 commits September 5, 2024 16:12

add sending packets on container change

516ef30

Merge remote-tracking branch 'upstream/master'

55765fc

clippy + format

602c1c0

try to combat deadlocks

d3ec496

actually fixed deadlock

daaafee

fix so that it actually always updates both clients

d07eec2

refactored state_id to PlayerInventory

2acd1ed

Snowiiii added the enhancement New feature or request label Sep 6, 2024

start trying to drag, it doesn't work though

3b93892

Bryntet marked this pull request as ready for review September 6, 2024 19:53

Merge branch 'master' into master

72d95cd

Bryntet force-pushed the master branch from e8ebe7d to 72d95cd Compare September 6, 2024 19:56

Snowiiii reviewed Sep 7, 2024

View reviewed changes

StripedMonkey reviewed Sep 7, 2024

View reviewed changes

Bryntet and others added 6 commits September 7, 2024 16:03

move around dependencies to match project spec

c6dbc3d

remove debug leftover

814b48a

add error handling to Click functions

3b6fc48

a bit of refactor, and actually enforce player inventory condition ru…

59a950c

…les on handle_item_change

Merge branch 'master' into master

33a5a10

Merge branch 'Snowiiii:master' into master

4ed84f2

Snowiiii merged commit 8b29d9e into Pumpkin-MC:master Sep 8, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add packet SClickContainer and abstractions #60

Add packet SClickContainer and abstractions #60

Bryntet commented Aug 24, 2024

Bryntet commented Sep 6, 2024

Bryntet commented Sep 6, 2024

Snowiiii Sep 7, 2024

Bryntet Sep 7, 2024

Snowiiii Sep 7, 2024

Snowiiii Sep 7, 2024

Snowiiii Sep 7, 2024

Snowiiii Sep 7, 2024

Bryntet Sep 7, 2024

StripedMonkey Sep 7, 2024

Snowiiii Sep 7, 2024

Bryntet Sep 7, 2024

Snowiiii Sep 7, 2024

Bryntet Sep 7, 2024

Snowiiii Sep 7, 2024

StripedMonkey Sep 7, 2024

Bryntet Sep 7, 2024

StripedMonkey Sep 7, 2024

StripedMonkey Sep 7, 2024

		pub open_containers: HashMap<u64, OpenContainer>,
		pub drag_handler: DragHandler,

Add packet SClickContainer and abstractions #60

Add packet SClickContainer and abstractions #60

Conversation

Bryntet commented Aug 24, 2024

Bryntet commented Sep 6, 2024

Bryntet commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment