[SPIGOT-2731] Fire InventoryMoveItemEvent cancelled without move Created: 14/Oct/16 Updated: 14/Jul/22 |
|
Status: | Open |
Project: | Spigot |
Component/s: | None |
Affects Version/s: | None |
Fix Version/s: | None |
Type: | New Feature | Priority: | Minor |
Reporter: | Phoenix616 | Assignee: | Unassigned |
Resolution: | Unresolved | Votes: | 3 |
Labels: | Craftbukkit, Spigot |
Description |
Currently the InventoryMoveItemEvent is fired every time a Hopper or Dropper tries to move an item into another inventory, even when the destination inventory is full and the Vanilla action would be to not transfer items at all. There is no way for a plugin to know if the move actually occurred or if it was only an attempt of a move. This also poses performance issues with protection plugins like LWC as they need to check every move for block protections. E.g. in storage systems where hoppers point into full chests. Every hopper-transfer-rate tick the event is fired. Two possible solutions:
|
Comments |
Comment by Paul Palmer [ 14/Jul/22 ] |
Bump. This is a frequent source of lag for us, to the point that we have coded into one of our plugins logic to cancel these events automatically when a hopper is trying to push into a container that has no room for the item. It has proven effective for many scenarios without any noticeable negative side-effects. We have also encountered situations where hoppers pointing into composters get non-compostable items in them. The hopper will generate one move event for every non-compostable ItemStack every tick until the items are removed manually. In my experience, there is little risk to changing the semantics of the InventoryItemMove event to match its name (that is only fire it when a move is actually occurring). What is clear is that the current implementation is broken in a way that is causing real problems and working around it is unnecessarily difficult/expensive. At a minimum, adding a boolean to the event to indicate whether the move would fail even if not canceled. It would at least allow those of us looking for a solution to the problem to craft inexpensive fixes with 0 risk of changing the behavior of existing plugins. If you wanted to go further, you could create an InventoryItemMovePrecheck event that fires with the current semantics for InventoryItemMove and then change the behavior for InventoryItemMove to only fire when an actual move could take place. This would be more aggressive, but would provide relief to many server admins without them having to do anything other than update Spigot and those few (seriously, any?) plugins that depend on the current semantics could easily switch to using the new Precheck event. |
Comment by Syd Montague [ 11/Apr/20 ] |
I've been looking into hoppers performance and noticed that the way Bukkit (and thus Spigot) does the InventoryItemMove event puts a major load on the server even without plugins actually dealing with it. When the hopper tries to pull items from a container above will it fire an InventoryMoveItemEvent for every non-empty slot of the container until it successfully moves an item or no non-empty slot remains. That means in the worst case a single hopper can fire up to 54 InventoryMoveItemEvents every single tick, without any item actually being moved. In the same vain, when a hopper tries to push items to a container it's connected to while that container has at least one non-full stack it can fire up to 5 InventoryMoveItemEvents per tick. Restoring Vanilla code reduced the performance impact of those hoppers in my (very simple and not accurate) test setup by around 80%, not accounting for plugins. Checking the plugins I use on my server that listen to the event it didn't seem like any was actually caring (or even aware) about unsuccessful InventoryItemMoveEvents, making me wonder whether this behavior is actually used by plugins or if it's used whether the performance impact is actually worth it. I'd suggest changing the InventoryItemMoveEvent from firing at attempts to move items to only firing if an item has actually been moved.
|
Comment by BillyGalbreath [ 23/Nov/16 ] |
New event InventoryMoveItemAttemptEvent would be ideal here. It would fire in all cases for logging purposes, etc. InventoryMoveItemEvent would then only fire if something actually moves. Or something like that. |
Comment by Thomas van den Bulk [ 22/Nov/16 ] |
Uncancelling PlayerInteractEvent should definitely do something. ;3 |
Comment by Phoenix616 [ 22/Nov/16 ] |
Yeah the cancelling would be kinda awkward, you are right. But it isn't a big problem with the PlayerInteractEvent (which just doesn't do anything if you uncancel it) as far as I'm aware of and would follow the same design principle. Another field would definitely make it easier but would mean that you couldn't filter via the ignoreCancelled setting of the EventHanlder annotation. |
Comment by Thomas van den Bulk [ 22/Nov/16 ] |
Both options wouldn't be optimal: Not firing the event at all would mean that it would look like there was no attempt made at all. This could cause issues when you want, for example, to do something else with the items that won't fit. Making a separate variable containing a boolean if the destination is full, or cannot accept the item would not solve the initial problem, as it will still fire the event. Although it does make it easier to detect if the attempt will be succesful on a not-cancelled event. |