PreInventoryMoveItemEvent

[IAISI]

Is your feature request related to a problem?

I'm thinking it might be nice idea to add PreInventoryMoveItemEvent, which would be fired before source and target inventories are checked, so at this point server doesn't even know if it should move item, but it does know it will have to try to.

This would allow protection plugins to cancel such events before expensive itemstack checks are done, and it would also allow plugins that modify say hopper behavior to do the same and skip whole builtin logic.

Furthermore, source inventory passed in InventoryMoveItemEvent has first amount set to 1 instead of actual value, which further limits plugins.

Describe the solution you'd like.

Add PreInventoryMoveItemEvent which is fired before any checks are done on from/to inventories.

Describe alternatives you've considered.

Running delayed task on certain InventoryMoveItemEvent events, but I feel like that's not too great solution as processing is actually delayed by 1 tick, and there's extra performance penalty for work done before InventoryMoveItemEvent is cancelled.

Other

No response

[masmc05]

From my personal observation, it was never the checks being expensive, 90% of the time it is the protection plugins being extremely bad which causes lags, because there are often hundreds or thousands of hoppers transferring items each tick, and protection plugins just can't properly check all of those that fast so after 1000 checks in a tick they would still be in reasonable <1 ms

[IAISI]

Depends on amount of hoppers?

First, first slot is pulled, that item stack would have to be matched against every itemstack in target? Then same for 2nd slot, 3rd slot, etc... having say, 100 players, 50k hoppers across a few loaded chunks, think it would add quickly, but you're right I should check spark reports.

[electronicboy]

the source stack being sized 1 is due to the patch attempting to avoid cloning due to how expensive it is, for the root tree this is now much cheaper, the only real issue is the handling of the PDC is potentially going to cause issues here if we wanted to restore some of the cloning back now that it's supposed to be cheaper.

Adding more events inside of this tightly defined loop is fairly meh, however.

[masmc05]

@IAISI A protection plugin could potentially use https://jd.papermc.io/paper/1.21.1/org/bukkit/block/Hopper.html#setTransferCooldown(int) to block hoppers to protect regions, this may be the fastest and working solution

[IAISI]

I guess you're right on performance end of things, as well as firing too many events in such a short loop.

But, in terms of plugins/API handling hoppers it would still be an issue? And looking at the code there's not many we could change InventoryMoveItemEvent to mitigate issue of setting slot amount to 1? Not without breaking other stuff and existing plugins?

Maybe the solution would be to extend Hopper api instead? And make it so that each hopper has own transfer amount that could be modified? Not quite sure how doable that would be tho. It also might break existing plugins that just assume amount would always be 1? Then again amount wouldn't be 1 it would always be whatever is set in spigot.yml config file.

I actually went explore this trying to implement own Hopper plugin but ran into issues were amount in source inventory not being the actual amount.

[electronicboy]

The amount thing was down to aikar attempting to avoid cloning due to highly nested NBT structures, I don't think that this is a real issue anymore given mojang rewriting itemstacks, the only caveat would be the compound tag data component, but, that is generally not going to be populated to the degree that was causing issues in the past.