PlayerPickItemEvent should be an InventoryEvent

[codeHusky]

Expected behavior

The relevant packet for PlayerPickItemEvent (ServerboundPickItemPacket) is used by some mods to quick move items through the inventory to the player's hand. This event should extend InventoryEvent - it has all relevant Inventory context to provide the necessary information for an InventoryEvent.

Observed/Actual behavior

When sending a ServerboundPickItemPacket on slot 9, this event will fire and cause inventory manipulation without triggering any InventoryEvent (such as an InventoryClickEvent). This event also does not extend InventoryEvent, making it semi-invisible to Javadocs browsing for trying to handle Inventory-related exploits and bugs.

Steps/models to reproduce

1. Implement a system utilizing InventoryEvents to prevent players from moving items
2. Trigger a ServerboundPickItemPacket on a relevant slot
3. PlayerPickItemEvent will fire, but no InventoryEvents will fire. The inventory will, however, still be mutated.

Plugin and Datapack List

[22:05:57 INFO]: Server Plugins (21):
[22:05:57 INFO]: Paper Plugins:
[22:05:57 INFO]:  - BKCommonLib
[22:05:57 INFO]: Bukkit Plugins:
[22:05:57 INFO]:  - Essentials, FastAsyncWorldEdit, GSit, helper, helper-sql, HuskyScreen, LuckPerms, Multiverse-Core, Multiverse-NetherPortals, packetevents
[22:05:57 INFO]:  *PlasmoDynamicSources, PlasmoVoice, PlugManX, ProtocolLib, pv-addon-lavaplayer-lib, spark, SubserverAdditions, Train_Carts, ViaVersion, WorldGuard
[22:06:09 INFO]: There are 2 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)]
[22:06:09 INFO]: There are no more data packs available

Paper version

Unknown version
Previous version: git-Paper-280 (MC: 1.20.2)

Note: This paper fork only has small alterations for how Maps are updated and does not have any sweeping changes

Other

No response

[electronicboy]

The event is a PlayerEvent, so we can't really extend InventoryEvent; I'm also not really sure what mutations this event is supposed to be doing, all it really seems to do is change the selected slot?

oh, it moves items around in a players inventory, bleh...

[codeHusky]

Yeah, to be fully clear, if you have an item in (for example) Slot 9 and fire that packet for it, it will move into the player's held inventory slot. This caused me several hours of debugging headache until I just finally caved and used the reporting player's modpack and a packet capture and, sure enough, it was doing some bizarre crap when shift-right-clicking items in the inventory.

Logging all relevant Inventory events looked identical to my own when trying to repro, but I couldn't get the dupe to occur no matter what in Vanilla. It's some odd, unused vanilla behavior or something.

[AjayAntoIsDev]

I dont understand how this could cause a dupe it just looks like a desync

[AjayAntoIsDev]

also could you link the modpack that causes the issue?

[codeHusky]

I dont understand how this could cause a dupe it just looks like a desync

The event does not trigger a dupe of any kind on its own. This is a developer UX issue on one hand, and an oversight in the API on the other. PlayerPickItemEvent is not built with the consideration that it can modify non-creative inventories in mind, so it does not extend InventoryEvent or provide any important context. This makes it somewhat invisible to folks making custom GUIs. There are likely many plugins that don't consider this event.

also could you link the modpack that causes the issue?

This is the modpack. It's ~175mb. I believe the specific mod here may be MouseTweaks or a different inventory QOL mod - it's the behavior where it swaps an item into your hotbar when it's consumed