Fix GUI concurrent access and autocrafter deadlock#35
Closed
Distolfix wants to merge 7 commits into
Closed
Conversation
Distolfix
force-pushed
the
development
branch
5 times, most recently
from
b67be41 to
98e8758
Compare
The suction module wasn't properly handling items stacked by stacker plugins (WildStacker, UltimateStacker, RoseStacker). This caused large stacks to disappear or be counted incorrectly. Changes made: - Bypass PickupDelay check for stacker items since we handle them through their APIs - Read item amount BEFORE emitting InventoryPickupItemEvent (WildStacker modifies the stack during event processing at HIGHEST priority, which was causing incorrect counts) - Add protected item check to respect shop items and custom plugin items (checks for display name, lore, or PDC data) - Only bypass event cancellation for stacker items that aren't protected - this lets suction work with stacker plugins while still respecting protection from shop plugins like EzChestShopReborn - Fix premature loop termination by changing return to continue for shulker boxes, shop items, and blacklisted items - Add null checks in updateAmount() for WildStacker and RoseStacker APIs with fallback to vanilla handling This ensures stacked items (like 128 diamonds from WildStacker) get collected properly while still respecting protected items from other plugins.
The suction module was only emitting InventoryPickupItemEvent when the EMIT_INVENTORYPICKUPITEMEVENT setting was enabled. This prevented other plugins from cancelling item pickup for their custom items. This change makes the suction module always emit the event, allowing any plugin to cancel pickup by listening to InventoryPickupItemEvent, maintaining consistency with standard hopper behavior. This is important because suction is an aggressive collection mechanism that bypasses normal hopper behavior, and other plugins should have control over what items can be collected. Benefits: - Universal plugin compatibility - Plugins can block their custom items from being collected - Uses standard Bukkit event system - Backwards compatible with existing plugins
Distolfix
force-pushed
the
development
branch
2 times, most recently
from
0bf145f to
6ec78d0
Compare
Distolfix
changed the title
Added explicit NMS dependency and fixed ClassCastException crashes that occurred when chunks unloaded or blocks were removed while hoppers were processing items. Changes: - Added SongodaCore-NMS-v1_21_R5 dependency for Minecraft 1.21.8 support - Disabled minimizeJar to preserve dynamically loaded NMS implementations - Added instanceof checks before casting to InventoryHolder in StorageContainerCache and ModuleSuction - Prevents crashes when blocks are no longer valid inventory holders
Bug fix Songoda-Plugins#1 - GUI Concurrent Access: Multiple players can now simultaneously access the same hopper GUI without closing each other's views. Changed activePlayer tracking from single Player to Set<Player>, and implemented proper cleanup when players close their GUIs individually. Bug fix Songoda-Plugins#2 - Autocrafter Deadlock Prevention: Fixed issue where hoppers with active autocrafters would fill all slots when thousands of items were on the ground, preventing the autocrafter from functioning. The suction module now reserves slots when an autocrafter is detected, maintaining at least 2 free slots (one for craft output, one for the next craft). Additionally, the autocrafter module will now automatically eject items when completely full of ingredients to prevent deadlocks.
Distolfix
force-pushed
the
development
branch
from
6ec78d0 to
d69886a
Compare
Prevent item loss by avoiding event emission when hopper is full. When suction module tried to pick up items but the hopper was full (added == 0), it was still emitting InventoryPickupItemEvent. This caused WildStacker and UltimateStacker to modify the entity stack amount even though we didn't actually pick up any items, resulting in hundreds of items disappearing. The fix moves addAny() call before the event emission, and only emits the event when we successfully added items (added > 0). This prevents stacker plugins from touching entities we can't pick up anyway.
Improve slot counting when autocrafter is active to prevent item pickup deadlock. Previous logic was too conservative when reserving slots for the autocrafter. With 2-3 free slots, it would refuse to add ANY items, leaving thousands of items stuck on the ground. The new logic correctly counts usable slots: if we have 3 free slots and need to reserve 1 for autocrafter, we can use 2 slots for pickup. Partial stack filling also doesn't consume free slots, so it's always allowed regardless of reservation. This fixes situations where items remain on ground even with multiple empty slots available.
Add timestamp-based caching to remember last output slot used, preventing inventory fragmentation and improving performance. Previous behavior iterated through all slots every craft to find where to place the output, often resulting in the same item type scattered across multiple slots (e.g., diamond blocks in slots 0, 1, and 4). Now caches the last used slot per hopper with a 60-second TTL. On subsequent crafts, it checks the cached slot first (O(1) instead of O(n)). If valid, uses it directly. If not, falls back to two-pass search: first looking for existing stacks of the same type, then for empty slots. This keeps inventory organized and reduces iteration overhead on every craft operation.
Author
|
Closing duplicate - PR #38 contains all the same fixes with better branch organization |
Author
|
Closing - splitting into separate focused PRs |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes two critical bugs that were affecting hopper functionality:
Bug #1: GUI Concurrent Access Issue
Previously, when one player opened a hopper GUI, it would force-close the GUI for any other player who had it open. This was caused by the
prepareForOpeningOverviewGui()method callingcloseInventory()on the previous player.Solution:
activePlayerfrom a single Player to aSet<Player>to track multiple viewersremoveActivePlayer(Player)method for granular player removalBug #2: Autocrafter Deadlock
When thousands of items were on the ground near a hopper with an active autocrafter, the suction module would fill all 5 inventory slots. This prevented the autocrafter from functioning since it needs at least one empty slot to place crafted items.
Solution:
StorageContainerCache.addAny()to accept an optionalreserveOneSlotparameterModuleAutoCraftingto automatically eject items when the hopper is completely full of ingredientsTesting
Tested with:
Compatibility
These changes are fully backwards compatible and only affect behavior when: