InventoryMoveItemEvent Fires Even If Move Fails For Hopper Minecart
KyGuy2002 opened this issue · 5 comments
Expected behavior
Exactly like normal hoppers, if the hopper is full and an item tries to transfer into it, the InventoryMoveItemEvent isn't fired and nothing happens. The same should apply when an item tries to transfer into a minecart hopper.
Observed/Actual behavior
The event is spammed, causing any plugins that rely on it to perform their logic many times for no reason. There's no way to check if the transfer succeeded.
Steps/models to reproduce
Place a chest above a minecart hopper, and fill both containers to their limit with junk items.
If you have to debug when the InventoryMoveItemEvent event fires, you will see it fires even if no items are able to move.
Plugin and Datapack List
Just my debug plugin to print the event.
Paper version
paper-1.18.2-241
Other
It appears that in the Hopper class, there are checks in place to prevent this from happening. These checks must not be in the hopper minecart one.
In the javadocs for the event, it says
If this event is not canceled, the initiator will try to put the ItemStack into the destination inventory. If this is not possible and the ItemStack has not been modified, the source inventory slot will be restored to its former state. Otherwise, any additional items will be discarded.
So maybe this is more of a feature request, but I don't believe so since this behavior is not observed for the hopper, strictly the minecart.
Well, in my opinion, this is at least with the current game logic valid behavior. The hopper minecart simply attempts to pull items every tick (even if full) and hence the event is called for the attempted move.
If it even makes sense for the hopper minecart to attempt pulling an item even if its own inventory is filled is a different question and presumably more of a feature request as this is vanilla behavior.
This is entirely on how hoppers and this event works, the event has been fired for every move attempt for a long time, I had some thoughts a good while ago about a means to make the cooldown options more effective in terms of some areas where the current cooldowns failed, but, this is basically a core vanilla issue (but, somewhat expected due to how all this stuff works)
Glad we agree. To sum it up, the event works like it is expected to work (on every attempted push or pull for an inventory move). Sadly this does result in a spam of events for hopper minecarts due to their agressive pulling logic, however addressing this is definitely a different issue which requires a lot more work. I am closing the issue hence as this works as currently intended.
Thanks for the report nonetheless!
Alright, I agree that makes sense, but shouldn’t there be either a separate event or a way to check if it was actually able to move it or not? In the issue I mentioned this in, you can see it makes checking items or whatnot during a hopper move completely impossible due to the event spam. If there was a separate event or if the move event was fired after it actually decides to move it could fire pre cancelled or something, don’t know the best way that would be done. Thanks.