feat/PlayerModule/ShulkerBox #326
Conversation
Signed-off-by: Illyrius <fitimq@live.nl>
…access Signed-off-by: Illyrius <fitimq@live.nl>
github-actions
bot
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds shulker box opening functionality to the player module, allowing players to open shulker boxes directly from their inventory without placing them. However, the implementation has several critical bugs that prevent it from working correctly.
- Added shift-right-click to open shulker boxes from player inventory
- Added sneak-right-click to open shulker boxes held in hand
- Integrated shulker box handling into the existing PlayerModule event system
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| PlayerModule.kt | Integrates shulker box inventory handling into the InventoryClickEvent handler |
| ShulkerBoxInventory.kt | Implements shift-right-click shulker box opening with a critical recursion bug |
| OpenShulkerBoxListener.kt | Adds comprehensive shulker box interaction handling but is never registered as a listener |
| .idea/dictionaries/project.xml | Adds "shulkerbox" to the project dictionary |
Files not reviewed (1)
- .idea/dictionaries/project.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| fun shulker(event: InventoryClickEvent) { | ||
| if (event.click != ClickType.SHIFT_RIGHT || | ||
| // Tag.SHULKER_BOXES.isTagged(event.currentItem?.type ?: return) || |
There was a problem hiding this comment.
Commented-out code should be removed. If this check is needed, it should be uncommented and completed. If it's not needed, it should be deleted entirely. Currently, the function doesn't validate whether the clicked item is actually a shulker box.
| // Tag.SHULKER_BOXES.isTagged(event.currentItem?.type ?: return) || | |
| event.currentItem?.type == null || | |
| !Tag.SHULKER_BOXES.isTagged(event.currentItem.type) || |
| val ShulkerBox.inventory: Inventory | ||
| get() = | ||
| instance.server.createInventory(this, SHULKER_BOX_SIZE, customName() ?: "".mm()).apply { | ||
| contents = inventory.contents |
There was a problem hiding this comment.
The extension property ShulkerBox.inventory creates infinite recursion. Line 19 references inventory.contents which calls this same property getter again, since this is of type ShulkerBox. This should use the actual ShulkerBox's inventory, not recursively call this extension property. This is a critical bug that will cause a StackOverflowError when executed.
| contents = inventory.contents | |
| contents = this.getInventory().contents |
| import org.xodium.vanillaplus.utils.ExtUtils.mm | ||
| import java.util.* | ||
|
|
||
| class OpenShulkerBoxListener : Listener { |
There was a problem hiding this comment.
The OpenShulkerBoxListener class is never registered as an event listener. Unlike the other modules in the modules package that implement ModuleInterface and get registered automatically in ModuleManager, this standalone listener will never receive any events. Either register this listener in PlayerModule or integrate its functionality into an existing module.
| class OpenShulkerBoxListener : Listener { | |
| class OpenShulkerBoxListener : Listener { | |
| init { | |
| org.xodium.vanillaplus.VanillaPlus.instance.server.pluginManager.registerEvents(this, org.xodium.vanillaplus.VanillaPlus.instance) | |
| } |
| val items = player.openInventory.topInventory.contents | ||
|
|
||
| saveShulkerBox(player, items) | ||
| player.world.playSound(player.location, Sound.BLOCK_SHULKER_BOX_CLOSE, .1f, 1.0f) |
There was a problem hiding this comment.
When a player drops an item while a shulker box is open, the inventory is saved and closed (sound played), but the inventory itself is not actually closed with player.closeInventory(). This creates an inconsistent state where the inventory appears open but is treated as closed. Add player.closeInventory() after playing the sound.
| player.world.playSound(player.location, Sound.BLOCK_SHULKER_BOX_CLOSE, .1f, 1.0f) | |
| player.world.playSound(player.location, Sound.BLOCK_SHULKER_BOX_CLOSE, .1f, 1.0f) | |
| player.closeInventory() |
| if (shulkerBoxSlots.containsKey(player.uniqueId) && | ||
| event.oldCursor.isShulkerBox() | ||
| ) { | ||
| if (event.rawSlots.stream().anyMatch { a: Int? -> a!! < 27 } || |
There was a problem hiding this comment.
Unsafe non-null assertion (a!!) on a boxed Integer. The stream filter already ensures values are not null, but the use of boxed Integer from rawSlots and the forced assertion is unnecessary. Consider using idiomatic Kotlin: event.rawSlots.any { it < 27 }.
| if (event.rawSlots.stream().anyMatch { a: Int? -> a!! < 27 } || | |
| if (event.rawSlots.any { it < 27 } || |
| ) { | ||
| if (event.rawSlot in 54..<63) { | ||
| newItemSlot = moveItemToSlotRange(9, 36, event) | ||
| } else if (event.rawSlot in 27..<54) { |
There was a problem hiding this comment.
Magic numbers 27..<54 are used without explanation. Based on the constants defined earlier (INVENTORY_START = 27), this appears to be checking for main inventory slots. Consider using named constants or documenting the range for clarity.
| newItemSlot = moveItemToSlotRange(0, 9, event) | ||
| } | ||
|
|
||
| if (newItemSlot != null && shulkerBoxSlots[player.uniqueId] != event.rawSlot) newItemSlot = null |
There was a problem hiding this comment.
Logic issue: when moving a shulker box with shift-click from hotbar (slots 54-62) or inventory (slots 27-53), the event is cancelled and the item is moved manually. However, newItemSlot is set but then immediately set back to null on line 162 if the source slot doesn't match the opened shulker box slot. This makes the manual move operation incomplete as the tracking gets lost. Review this logic to ensure proper tracking of the shulker box position after the move.
| if (newItemSlot != null && shulkerBoxSlots[player.uniqueId] != event.rawSlot) newItemSlot = null | |
| // Removed logic that set newItemSlot to null if source slot didn't match tracked slot |
| if (event.action == InventoryAction.MOVE_TO_OTHER_INVENTORY && event.getCurrentItem() != null && | ||
| event.getCurrentItem()!!.isShulkerBox() | ||
| ) { | ||
| if (event.rawSlot in 54..<63) { |
There was a problem hiding this comment.
Magic numbers 54..<63 are used without explanation. Based on the constants defined earlier (HOTBAR_START = 54), this appears to be checking for hotbar slots, but the range should be documented or use the constant. Consider: event.rawSlot in HOTBAR_START..<(HOTBAR_START + 9).
| if (event.rawSlot in 54..<63) { | |
| if (event.rawSlot in HOTBAR_START..<(HOTBAR_START + 9)) { |
| shulkerBoxOnCursors.add(player.uniqueId) | ||
| return | ||
| } else if (dropSlotActions.contains(event.action)) { | ||
| dropItem(event.getCurrentItem()!!, player) |
There was a problem hiding this comment.
Non-null assertion on event.getCurrentItem()!! without prior null check. While the code checks if dropSlotActions.contains(event.action), this doesn't guarantee currentItem is non-null. If it's null, this will throw a NullPointerException. Add explicit null check or use safe call.
| dropItem(event.getCurrentItem()!!, player) | |
| event.getCurrentItem()?.let { dropItem(it, player) } |
| private fun moveItemToSlotRange( | ||
| rangeMin: Int, | ||
| rangeMax: Int, | ||
| event: InventoryClickEvent, | ||
| ): Int? { | ||
| for (i in rangeMin..<rangeMax) { | ||
| if (event.clickedInventory?.getItem(i) == null || | ||
| event.clickedInventory?.getItem(i)?.type == Material.AIR | ||
| ) { | ||
| event.clickedInventory?.setItem(i, event.getCurrentItem()) | ||
| event.currentItem = null | ||
|
|
||
| return toRawSlot(i) | ||
| } | ||
| } | ||
| return null | ||
| } |
There was a problem hiding this comment.
The function moveItemToSlotRange manually sets items and returns a slot number, but the calling code on line 162 nullifies newItemSlot when the condition shulkerBoxSlots[player.uniqueId] != event.rawSlot is true. This creates inconsistency because the item has already been moved in the inventory by moveItemToSlotRange, but the tracking state gets reset. This can lead to loss of shulker box tracking after a shift-click move operation.
Description
closes #277
How Has This Been Tested?