Container API first pass refactor and cleanup #85

StripedMonkey · 2024-09-08T15:48:26Z

This is a refactor of the Container API to make it a little nicer.

I'm looking for feedback from @Bryntet above anything, but we'll see when we get there.

Once changes have been discussed, I will chunk out commits a little better.

Bryntet

Very nice PR, still a few todo!():s that need to be sorted out, and a few places where you can shorten what's being written. but overall very good, all the structural changes have my full approval!

Bryntet · 2024-09-08T16:27:21Z

pumpkin-inventory/src/lib.rs

    }

    fn internal_pumpkin_id(&self) -> u64 {
        0
    }
+
+    fn get_slot(&self, slot: usize) -> Option<&Option<ItemStack>>;


Maybe put a doc comment here, explaining the difference between the two Option

pumpkin-inventory/src/lib.rs

Bryntet · 2024-09-08T16:35:25Z

pumpkin/src/client/container.rs

-                        *container.all_slots()[slot] = item_slot;
-                    }
+                    let all_slots = container.iter_slots_mut().enumerate();
+                    // let slots = if slot > 35 {


this code works, but only for the case of being inside of Inventory. Implementing this should be easy now if we use Container::size

Bryntet · 2024-09-08T16:38:33Z

pumpkin-inventory/src/player.rs

@@ -57,13 +57,19 @@ impl PlayerInventory {
            if !(0..=45).contains(&slot) {


this can be removed, as you added a check below

Bryntet · 2024-09-08T16:42:19Z

pumpkin-inventory/src/player.rs

@@ -57,13 +57,19 @@ impl PlayerInventory {
            if !(0..=45).contains(&slot) {
                Err(InventoryError::InvalidSlot)?
            }
-            *self.all_slots()[slot] = item;
+            let Some(slot) = self.get_slot_mut(slot) else {


change this to let slot = self.get_slot_mut(slot).ok_or(Err(InventoryError::InvalidSlot))?;

Bryntet · 2024-09-08T16:43:21Z

pumpkin-inventory/src/player.rs

            return Ok(());
        }
        let slot_condition = self.slot_condition(slot)?;
        if let Some(item) = item {
            if slot_condition(&item) {
-                *self.all_slots()[slot] = Some(item);
+                let Some(slot) = self.get_slot_mut(slot) else {


change this to let slot = self.get_slot_mut(slot).ok_or(Err(InventoryError::InvalidSlot))?;

pumpkin-inventory/src/player.rs

pumpkin-inventory/src/lib.rs

pumpkin-inventory/src/open_container.rs

Bryntet · 2024-09-09T07:27:40Z

pumpkin-inventory/src/lib.rs

+            Some(container) => Box::new(
+                container
+                    .iter_slots_mut()
+                    .chain(self.inventory.iter_slots_mut()),


If container is Some on OptionallyCombinedContainer only chain it with the main inventory slots on PlayerInventory. The client is not able to change things (like armour) while inside of a container.

See https://wiki.vg/Inventory for a nice visualisation of where each slot goes, depending on which container that is opened.

Bryntet · 2024-09-09T07:49:46Z

pumpkin-inventory/src/lib.rs

+            return self.inventory.get_slot(slot);
+        }
+        if let Some(container) = &self.container {
+            return container.get_slot(slot);


this should be a match statement, looking something like:

match &self.container { Some(container) => { if (0..container.size()).contains(&slot) { container.get_slot(slot) } else { self.inventory.items.get(slot-self.container.size()) } None => self.inventory.get_slot(slot) }

see https://wiki.vg/Inventory#Chest

this also fixes ordering issue

Bryntet · 2024-09-09T07:51:44Z

pumpkin-inventory/src/lib.rs

+            return self.inventory.get_slot_mut(slot);
+        }
+        if let Some(container) = &mut self.container {
+            return container.get_slot_mut(slot);


this should be a match statement, looking something like:

match &mut self.container { Some(container) => { if (0..container.size()).contains(&slot) { container.get_slot_mut(slot) } else { self.inventory.items.get_mut(slot-self.container.size()) } None => self.inventory.get_slot_mut(slot) }

see https://wiki.vg/Inventory#Chest

this also fixes ordering issue

Bryntet · 2024-10-08T08:02:09Z

Hey, I'd love to help getting this refactor and cleanup merged. Would you like some help?

StripedMonkey · 2024-10-08T14:08:40Z

I'd like some,

I've been working on other things since this PR. I'll give you access if you don't already this afternoon.

Bryntet · 2024-10-14T10:24:41Z

@StripedMonkey I have now fixed all the major issues with this PR, so the whole system works once again!

I still want to polish up a few things, but very close to wanting this merged :)

Snowiiii · 2024-11-12T21:21:43Z

I will close this for now, Feel free to reopen. I think that there are already so many conflicts that it would be easier to Start from scratch again

Bryntet · 2024-11-12T21:42:51Z

I'd like this not to be closed, I'll solve the merge conflicts.

StripedMonkey changed the title ~~first pass refactor and cleanup~~ Container API first pass refactor and cleanup Sep 8, 2024

This comment was marked as outdated.

Sign in to view

Bryntet reviewed Sep 8, 2024

View reviewed changes

StripedMonkey force-pushed the container_refactor branch from 72c4df9 to 4adac0d Compare September 9, 2024 02:15

Bryntet reviewed Sep 9, 2024

View reviewed changes

StripedMonkey force-pushed the container_refactor branch from 4adac0d to 52b6a98 Compare September 9, 2024 21:45

StripedMonkey and others added 8 commits October 11, 2024 14:13

first pass refactor and cleanup

5c191cc

eliminate player-specific get function

b5c9e44

use more consistent names on iterating slots and getting slots

011fbae

remove exact sized restriction

c26ce43

implement inventory methods

2894f49

"complete" container methods

5de7cd5

Add correct bounds and fix some combination logic of containers

943fd40

un-atomise things that should not be atomic.

08baa6a

Bryntet force-pushed the container_refactor branch from 52b6a98 to 3518c49 Compare October 14, 2024 10:19

fix proper ranges

87b5300

Bryntet force-pushed the container_refactor branch from 3518c49 to 87b5300 Compare October 14, 2024 10:22

Merge branch 'master' into container_refactor

8cc0a54

fix clippy lint

c702692

Snowiiii closed this Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container API first pass refactor and cleanup #85

Container API first pass refactor and cleanup #85

StripedMonkey commented Sep 8, 2024

This comment was marked as outdated.

Bryntet left a comment

Bryntet Sep 8, 2024

Bryntet Sep 8, 2024

Bryntet Sep 8, 2024

Bryntet Sep 8, 2024

Bryntet Sep 8, 2024

Bryntet Sep 9, 2024

This comment was marked as outdated.

Bryntet Sep 9, 2024 •

edited

Loading

This comment was marked as outdated.

Bryntet Sep 9, 2024

Bryntet commented Oct 8, 2024

StripedMonkey commented Oct 8, 2024

Bryntet commented Oct 14, 2024

Snowiiii commented Nov 12, 2024

Bryntet commented Nov 12, 2024

		@@ -57,13 +57,19 @@ impl PlayerInventory {
		if !(0..=45).contains(&slot) {

Container API first pass refactor and cleanup #85

Container API first pass refactor and cleanup #85

Conversation

StripedMonkey commented Sep 8, 2024

This comment was marked as outdated.

Bryntet left a comment

Choose a reason for hiding this comment

Bryntet Sep 8, 2024

Choose a reason for hiding this comment

Bryntet Sep 8, 2024

Choose a reason for hiding this comment

Bryntet Sep 8, 2024

Choose a reason for hiding this comment

Bryntet Sep 8, 2024

Choose a reason for hiding this comment

Bryntet Sep 8, 2024

Choose a reason for hiding this comment

Bryntet Sep 9, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

Bryntet Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

Bryntet Sep 9, 2024

Choose a reason for hiding this comment

Bryntet commented Oct 8, 2024

StripedMonkey commented Oct 8, 2024

Bryntet commented Oct 14, 2024

Snowiiii commented Nov 12, 2024

Bryntet commented Nov 12, 2024

Bryntet Sep 9, 2024 •

edited

Loading