Skip to content

Conversation

@AlbeMiglio
Copy link

  • Added unsafeCopy() method in GuiItem to deep copy the item even if AIR (use it at your own risk!);
  • Added GuiItem.air(Consumer<InventoryClickEvent>) to quickly create an air button performing an action;
  • Added addUnsafeItem() method in InventoryComponent to retrieve the unsafe copies instead of the normal ones;

These methods have been added to create a new StaticNullablePane (extending StaticPane), which:

  • overrides StaticPane's display() method using new addUnsafeItem() methods
  • overrides StaticPane's click() method retrieving the action to perform from the slot (with O(1) complexity) instead of the associated ItemStack, allowing users to create AIR-clicking GUIs (useful for custom textured panels)
  • adds some addItem(GuiItem, Slot, int, int) methods to quickly add larger rectangular areas of buttons which basically do the exact same thing (instead of adding them manually by slots)

Copy link
Owner

@stefvanschie stefvanschie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your pull request. The main difficulty with having air items with an associated action is that empty slots (i.e., the ones with air) are not moved like regular items. For example, when dragging an ItemStack over multiple slots, the slots that were previously empty are now filled with items, essentially erasing the air item that might have been there. Because of this, the air item with its associated action is completely deleted and cannot be recovered, even when the item is removed later. In contrast, when an item occupies a slot that is not air, the items are swapped, so the item is not deleted.

This issue can be addressed in multiple ways, but the simplest is to just not allow having air items with actions. From my understanding of your code, it seems like this issue is not addressed (which I'm guessing is why the related methods are called unsafe). In my opinion, this implementation is not entirely ideal for a couple of reasons.
Firstly, the unsafe nature means the API is hard to trust and therefore puts burden on the user to hope that it works as expected. Secondly, the extension of the static pane works, but is limited to just that pane. A pane like outline pane does not beneftir from this. While we can make extensions for each of the panes, this is quite cumbersome and a large amount of API to maintain with relatively little benefit.

To my understanding however, it seems like you just want to listen to clicks for a specific slot, rather than a specific item. This also seems to be how the extended static pane functions - look at the slot, rather than the item. To me, it seems then that it is a lot easier, to have a method on MergedGui, InventoryComponent, and Pane called setOnClick(Slot, Consumer<InventoryClickEvent>). This would fire when a specific slot is being clicked, rather than a specific item. This click event could then fire after the Gui-specific handlers (GlobalClick, OutsideClick, TopClick, and BottomClick) and before the GuiItem handler. I think this approach is favourable to your proposal, as it significantly reduces the amount of additional API needed for, to my knowledge, similar results.

As for your addItem(GuiItem, Slot, int, int) suggestion, an OutlinePane set to repeat seems to achieve the same goal. So unless there is a compelling reason for this variant, I do not think this additional method is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants