Changes on EntityPickupItemEvent not reflected

[rainbowdashlabs]

Expected behavior

Changes done in the event to the item stack associated with the event should be reflected on the actual picked up item.

Observed/Actual behavior

The changes are not reflected and are ignored.

Steps/models to reproduce

Example code: https://code.lewds.de/BQ7OpU

Output showing that even though the item was changed it didnt end up in the inventory

The whole process is:

• I drop a diamond to a zombie
• When picking up an item I set a key inside the pdc.
• I check whether the key is actually there. It is.
• I check after two ticks if the item in the hand. It is not.
• The zombie gets killed after 20 ticks
• The zombie drops a diamond without any pdc keys set.
• I pick the diamond up. The key is added again and present in my inventory.
• I drop the now tagged item to a zombie again.
• The zombie picks up my item and the key is now actually there

Plugin and Datapack List

Only the example plugin above.

Paper version

1This server is running Paper version git-Paper-318 (MC: 1.20.2) (Implementing API version 1.20.2-R0.1-SNAPSHOT) (Git: 9271ee7) You are running the latest version Previous version: git-Paper-196 (MC: 1.20.1)

Other

No response

[lynxplay]

https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/nms-patches/net/minecraft/world/entity/EntityInsentient.patch#159-188

While upstream passes the itementity, which intern is used for the event, the item stack passed to the method is used instead of the "updated" one from the item entity.

The method also receives a copy of the item entities itemstack, so we don't magically update it with a craft mirror rn either.

[Machine-Maker]

The EntityPickupItemEvent is just wrong right now. It is incorrect to pass the Item (entity) as the thing being picked up because in vanilla, the stack actually being kept by the mob isn't always the full stack in the entity. Piglins, for example, can pickup 1 item at a time from an item entity and the event doesn't give you that context.

I think the solution here, is to add an ItemStack field to that event, deprecate getItem() (just to rename it to getSourceItem) and tell people to check the stack instead.