PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
10.04k stars 2.34k forks source link

InventoryClickEvent does not detect middle clicks (Survival mode) #11465

Open Klyser8 opened 1 month ago

Klyser8 commented 1 month ago

Expected behavior

When printing out the result of event.getClick() during the InventoryClickEvent, I was expecting ClickType.MIDDLE to get printed upon clicking with the mousewheel on an item.

Observed/Actual behavior

Instead, what I've found out is that no matter what one tries doing, ClickType.MIDDLE can never be detected. Upon middle clicking an item in an inventory (could be any inventory from what I saw), the only click being detected is ClickType.UNKNOWN. On discord I was initially told that detecting middle clicks isn't supported, though I find it really strange that the only way to get an UNKNOWN click type is through middle-clicking an item in Survival Mode.

Steps/models to reproduce

  1. Create a barebones plugin, one which simply listens to the InventoryClickEvent
  2. Print out event.getClick()
  3. In survival mode, try all types of clicks that are meant to be available (Left, right, shift-left, shift-right, double left, switch hand, number key, drop, ctrl drop, middle click)
  4. All click types get printed like expected, except for ClickType.MIDDLE

Plugin and Datapack List

None, simply the above stated barebones plugin.

Paper version

This server is running Paper version 1.21.1-99-master@1bc02e6 (2024-09-25T02:41:02Z) (Implementing API version 1.21.1-R0.1-SNAPSHOT)

Other

No response

Klyser8 commented 1 month ago

Update: Turns out that the behavior breaks when rebinding the middle mouse button to another button (As I have, since I develop on my macbook and there is no middle mouse button to use). It seems that rebinding the middle mouse button to a keyboard key (in my case, R) breaks the way that the middle button is detected to behave like I described above.

SoSeDiK commented 1 month ago

Was able to reproduce this. Remapping MMB (to M on keyboard in my case) leads to invalid(?) actions being sent from the client:

Both of these are unmapped/unknown actions, so it falls back to UNKNOWN.

Machine-Maker commented 1 month ago

Hmm, the javadocs for ClickType.MIDDLE kinda specifically state that it's the middle mouse button being used, not "whatever action the middle mouse button does by default". I think, because of that, we have to add another enum value and then maybe a helper method to check if it's either MIDDLE or whatever the new one is, like isCloneAction() or something.

Klyser8 commented 1 month ago

Hmm, the javadocs for ClickType.MIDDLE kinda specifically state that it's the middle mouse button being used, not "whatever action the middle mouse button does by default". I think, because of that, we have to add another enum value and then maybe a helper method to check if it's either MIDDLE or whatever the new one is, like isCloneAction() or something.

That's fair enough. Either way though, I think it's important to be able to pick up whether a user rebound the key or not as especially those on laptops may have, and plugins which rely on middle click functionality would break completely.

Also, while I'm at it, from what I can tell there might be a possibility to detect a player's middle-mouse click (or whatever the button was rebound to) while they are on survival, with no item on cursor yet an item in the slot beneath it. As it servers no functionality in survival it might be a handy way to provide plugin makers with a way to implement functionality in InventoryClickEvents.

notTamion commented 1 month ago

Is there a way to produce a ClickType MIDDLE with a vanilla client though? as far as i can tell the vanilla client doesn't even send a packet when using a middle click in survival, because of that i think this is actually a vanilla bug that the client sends a packet for the rebound middle click at all

ryantheleach commented 1 month ago

Vanilla bug or not, it would be dangerously fragile API to tie the 'middle click' action to only the middle click press, despite how its currently named.

In fact, the current naming scheme of the entire event is kinda bad, given the ability to rebind other controls, but I don't think it's necessarily appropriate to change that now, 10 years in, what I don't want to see though, is a mix of "the name reflects the default controls so people understand the action easier" and "the name reflects the actual button being used" side by side.

If you want to start renaming and deprecating, do the lot or none.

notTamion commented 1 month ago

yeah in my opinion a mojira issue should be created and for the time being the event should either not be called at all for this or the button check from the CLONE case should be removed