Paper icon indicating copy to clipboard operation
Paper copied to clipboard

Fix count in origin ItemStack for InventoryMoveItemEvent

Open Doc94 opened this issue 3 years ago • 4 comments

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

Doc94 avatar Jul 02 '22 22:07 Doc94

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

kennytv avatar Jul 04 '22 14:07 kennytv

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

electronicboy avatar Jul 04 '22 15:07 electronicboy

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...

Doc94 avatar Jul 04 '22 16:07 Doc94

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?

montlikadani avatar Jul 15 '22 19:07 montlikadani