PaperMC / PaperMC/Paper

Getting the source inventory on `InventoryMoveItemEvent` the moved item size is always one.

Open
#8,086 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

status: accepted type: bug version: 1.19
Dominant language
Java
Stars
12.7k
Forks
3.5k
Avg merge
3d 13h
Merged PRs (30d)
11

Description

Expected behavior

event.getSource().getItem(0).getAmount() on InventoryMoveItemEvent should return the amount of items that are actually in slot 0 in the inventory.

Observed/Actual behavior

The above method will always return one, where as it should only be that event.getItem().getAmount() should return one.

Steps/models to reproduce
package dev.omega24.test;

public class TestPlugin extends JavaPlugin, implements Listener {

    @Override
    public void onEnable() {
        getServer().getPluginManager().registerEvents(this, this);
    }

@EventHandler
    public void onInventoryMoveItem(InventoryMoveItemEvent event) {
        if (!(event.getSource().getHolder(false) instanceof Hopper hopper)) {
            return;
        }

        System.out.println("item: " + event.getItem().getAmount());
            System.out.println("source: " + event.getSource().getItem(0).getAmount());
    }
}
Plugin and Datapack List

[17:36:40 INFO]: Plugins (1): HopperSort

Paper version
[17:35:41 INFO]: This server is running Paper version git-Paper-43 (MC: 1.19) (Implementing API version 1.19-R0.1-SNAPSHOT) (Git: 9d0650e)
You are running the latest version
Previous version: git-Paper-42 (MC: 1.19)
Other

The problem stems from the HopperBlockEntity.hopperPush function.

What happens is that on the eight line of the function the original item stack is copied to a new variable, called itemstack (ItemStack itemstack = origItemStack;). This is done so that event.getItem() always has an amount of one, but because of Java's "pass by reference" nature, this always makes all changes to origItemStack happen to itemstack.

The problem line is origItemStack.setCount(moved);. On this line the original item stack is set to the count to be moved (this is where the number one comes from), which allows event.getItem() to have the correct amount. However this also makes it for a split second that the Hopper inventory's first (or whatever slot origItemStack is in) slot has a stack size of one.

To fix this there needs to be two changes:

  1. ItemStack itemstack = origItemStack; needs to turn into ItemStack itemstack = origItemStack.clone();
  2. origItemStack.setCount(moved); needs to turn into itemstack.setCount(moved);

This does bring up performance questions on how badly .clone() will affect the server performance, but if thats a concern why is there a whole new variable being created just to reference the original?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at HopperBlockEntity.hopperPush and inspect how the original item stack is assigned and modified around the reported lines. Reproduce InventoryMoveItemEvent with a stacked source item, then verify that event.getItem().getAmount() reflects the moved amount while event.getSource().getItem(0).getAmount() retains the inventory stack size.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
game-dev
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.