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.

[IAISI]

It appears paper has now added HopperInventorySearchEvent, however InventoryMoveItemEvent is still "broken". Now that I thought about it some more I think, it would be way more efficient if "protection" plugins just used HopperInventorySearchEvent, set target inventory to null and set cooldown, instead of doing processing in IMIE where stacks are already being processed.

And if plugins move from IMIE to HopperInventorySearchEvent, IMIE functionality could be restored so that IMIE is fired for every slot and that could be extended even further so that item taken from source inventory is actually item passed over by getItem from IMIE.

I've only added support for changing item amount (for even better implementation IMIE and its extended Paper's IMIE should check if not only getItem but also setItem was called and adjust items accordingly), just the change that restores full IMIE functionally and allows plugins to change amount of moved item would look like this:

Why? There are plenty of plugins trying to mess with hoppers, ie change amounts, add filters whatever. With current IMIE implementation that is next to impossible, developer is left to implement own hopper ticking for special hoppers or use delayed task and do lots of processing that could be skipped with proper IMIE implementation. Both of which would obviously add way more overhead. And even if you'd be willing to eat overhead there's an issue where all this logic would break with other plugins (unless IMIE is fired again by plugin trying to implement own hopper logic, but that would create even more mess and overhead).

Another "nice" idea would probably be to add hopper transfer amount to each hopper, same way cooldowns are added, allowing plugins to change transfer amount per hopper.

Thoughts?