PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
10.05k stars 2.34k forks source link

InventoryMoveItemEvent: First stack in an inventory returns amount 1 even if stack is bigger #9028

Open SebiTimeWaster opened 1 year ago

SebiTimeWaster commented 1 year ago

Expected behavior

All stack sizes in an inventory are correctly counted.

Observed/Actual behavior

Given an hopper inventory with the following iron stacks: 0 64 0 0 0 - The amount of items in all stacks combined returns 1. Given an hopper inventory with the following iron stacks: 64 0 0 0 0 - The amount of items in all stacks combined returns 1. Given an hopper inventory with the following iron stacks: 0 0 0 0 64 - The amount of items in all stacks combined returns 1. Given an hopper inventory with the following iron stacks: 0 64 0 32 0 - The amount of items in all stacks combined returns 33. Given an hopper inventory with the following iron stacks: 0 32 0 64 0 - The amount of items in all stacks combined returns 65.

The amount of items in the stack of the first non-empty inventory slot is always counted as 1.

This also affects Inventory.contains, Inventory.containsAtLeast and Inventory.removeItem, since they also count the stack sizes.

This bug probably only happens in an InventoryMoveItemEvent, i did not test it outside of this event.

Steps/models to reproduce

Count the inventory of a hopper in an InventoryMoveItemEvent:

ItemStack[] stacks = YOUR_INVENTORY.getContents();
Integer amount = 0;

for (Integer j = 0; j < stacks.length; j++) {
    if (stacks[j] != null) amount += stacks[j].getAmount();
}

System.out.println(amount);

Plugin and Datapack List

none, fresh install (my plugin)

Paper version

This server is running Paper version git-Paper-466 (MC: 1.19.4) (Implementing API version 1.19.4-R0.1-SNAPSHOT) (Git: 7af4cd3) You are running the latest version

Other

i think this bug report is related, but i may be wrong on that account: https://github.com/PaperMC/Paper/issues/8806

Machine-Maker commented 1 year ago

I'm guessing you are using the inventory from the InventoryMoveItemEvent. Cause I cannot reproduce this just getting the inventory from the player's target block in a command. So it's not always.

SebiTimeWaster commented 1 year ago

I'm guessing you are using the inventory from the InventoryMoveItemEvent. Cause I cannot reproduce this just getting the inventory from the player's target block in a command. So it's not always.

yeah, i added more information regarding this already. was your target block a hopper?

Machine-Maker commented 1 year ago

was your target block a hopper?

yeah, and it reported the stacks correctly. So its almost certainly related to that event as the other issue is also related to it. Some odd stuff happens there for performance optimizations so it might need some looking into.

Not gonna close this as a duplicate since the other issue is specifically about the moved stack, not the inventory relating to the movement.

Machine-Maker commented 1 year ago

@SebiTimeWaster I can still replicate that other issue (where the source stack is null). But I can't seem to replicate your issue. I've got a hopper pointing into a chest, I put 1 item in all the slots, and the rest of the stack in the first slot. When I enabled the hopper, it printed out 1 item in each slot. This is still wrong, and is what the first issue says, but it's not what your issue describes. Can you give more concrete reproduction steps?

electronicboy commented 1 year ago

Pretty sure this is a duplicate, at least this was brought up recently, hopper logic sets the stack size to 1 somewhere in it's logic before setting it to the correct value later on

SebiTimeWaster commented 1 year ago

it printed out 1 item in each slot. This is still wrong, and is what the first issue says, but it's not what your issue describes.

what? that is exactly what i described here:

"The stack in the first non-empty inventory slot is always counted as 1."

in case the sentence was not clear enough, i changed it to:

"The amount of items in the in the stack of the first non-empty inventory slot is always counted as 1."

SebiTimeWaster commented 1 year ago

Pretty sure this is a duplicate, at least this was brought up recently, hopper logic sets the stack size to 1 somewhere in it's logic before setting it to the correct value later on

that may be (still not sure about it since i did not really understand what bug #8806 talks about), but i think my report describes the underlying problem much better.

Machine-Maker commented 1 year ago

Given an hopper inventory with the following iron stacks: 0 64 0 0 0 getting amount of all stacks returns 1.

You know, I probably mis-interpreted what this meant. I thought this meant that all stacks in the hopper showed 1 as the amount. Not the total of all stacks, each stack individually showed one. That's why I thought this wasn't the same issue.

SebiTimeWaster commented 1 year ago

Given an hopper inventory with the following iron stacks: 0 64 0 0 0 getting amount of all stacks returns 1.

You know, I probably mis-interpreted what this meant. I thought this meant that all stacks in the hopper showed 1 as the amount. Not the total of all stacks, each stack individually showed one. That's why I thought this wasn't the same issue.

fair enough, i changed the wording to be more precise.

SebiTimeWaster commented 1 year ago

@Machine-Maker could you remove the "waiting on reporter" label? thx :)

SebiTimeWaster commented 1 year ago

could someone please remove the "waiting on reporter" label?

Stis780 commented 1 month ago

This is still an issue on latest Paper, I think it was introduced by Aikar's Hopper patch...

here: https://github.com/PaperMC/Paper/blob/master/patches/server/0999-Optimize-Hoppers.patch#L227

This was done for performance reasons? Since it's only changing stack amount I kinda doubt it can impact performance...

electronicboy commented 1 month ago

changing the stack amount is how he prevents cloning the entire stack, this is no longer a huge concern given that components can be lazy/shallow copied

Stis780 commented 1 month ago

So in terms of performance this likely wouldn't be an issue anymore?