Skip to content

Fix count in origin ItemStack for InventoryMoveItemEvent#8087

Closed
Doc94 wants to merge 1 commit into
PaperMC:masterfrom
Doc94:issue8086
Closed

Fix count in origin ItemStack for InventoryMoveItemEvent#8087
Doc94 wants to merge 1 commit into
PaperMC:masterfrom
Doc94:issue8086

Conversation

@Doc94
Copy link
Copy Markdown
Member

@Doc94 Doc94 commented Jul 2, 2022

This fixs #8086 based in the same issue the problem is the item passed to the event not have the change in the amount to pass.

note: need learn more git because when make https://github.com/PaperMC/Paper/blob/master/CONTRIBUTING.md#method-1 this i cannot modify the patch... this was a directly edition in patch.. tested with applyPatches

+ foundItem = true;
+ ItemStack origItemStack = item;
+ ItemStack itemstack = origItemStack;
+ ItemStack itemstack = origItemStack.copy();
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue says about use clone.. but NMS have a method for this.. my issue if can be this method or can be cloneItemStack(true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just said clone because thats what bukkit uses but copy should work too

@kennytv
Copy link
Copy Markdown
Member

kennytv commented Jul 4, 2022

On a quick glance there's more in the patch to be cleaned up if you clone the original item. also, here's how you properly fixup patches https://youtu.be/K_lym3WHoFA

@electronicboy
Copy link
Copy Markdown
Member

blindly cloning the original item negates a good chunk of the perf improvements here, would defo need that to at least be in the fire event, but, afaik, the patch currently maintains the behavior of upstream in terms of how the event works but I'd need to double check

@Doc94
Copy link
Copy Markdown
Member Author

Doc94 commented Jul 4, 2022

On a quick glance there's more in the patch to be cleaned up if you clone the original item. also, here's how you properly fixup patches https://youtu.be/K_lym3WHoFA

not sure why now works.. i try the same thing (based in contribution) and the class was reverted to the original.. maybe a WSL cache thing...

i rebased this and add the same thing to the pull...

About the copy thing not sure if can be another thing i can do in this... i dont tested only with the change in count ignoring the copy...

@montlikadani
Copy link
Copy Markdown
Contributor

montlikadani commented Jul 15, 2022

What about if we just sets the amount of item before calling PushMoveEvent and adding the item into the hopper instead of hard-copying the item? I mean

                final int origCount = origItemStack.getCount();
                final int moved = Math.min(level.spigotConfig.hopperAmount, origCount);
                origItemStack.setCount(moved);

                // We only need to fire the event once to give protection plugins a chance to cancel this event
                // Because nothing uses getItem, every event call should end up the same result.
                if (!skipPushModeEventFire) {
+                   itemstack.setCount(origCount);
                    itemstack = callPushMoveEvent(destination, itemstack, hopper);
                    if (itemstack == null) { // cancelled
                        origItemStack.setCount(origCount);
                        return false;
                    }
                }
                final ItemStack itemstack2 = addItem(hopper, destination, itemstack, enumdirection);
+               itemstack.setCount(moved);
                final int remaining = itemstack2.getCount();

or we could just move this origItemStack.setCount(moved) after addItem?

Copy link
Copy Markdown
Member

@Owen1212055 Owen1212055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. It's a bit eh all the sudden cloning items like this in code that can get very hot like this. It might be worth looking into what we can do to avoid having to reclone the entire item but properly update the count to try to keep things performant here.

@kennytv kennytv added the pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch label Mar 23, 2025
@kennytv kennytv deleted the branch PaperMC:master March 23, 2025 19:15
@kennytv kennytv closed this Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pre-softspoon: never rebased Pre-hardfork pull requests that were not re-opened with the new main branch pre-softspoon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants