Added on itemPickup event

[yajatkaul]

I was working on a mod recently when i came across this and realized there is no event for it, so i thought i might as well try to contribute it in there for others to use :)

[Fuzss]

Hey, thanks for your contribution!
Could you please outline some use cases?

I'm honestly not quite sure what this event is for in the current form. 😅
While NeoForge has something similar which I've been using myself, that does allow for two things:

• preventing the item pickup (useful for backpack mods, so they can add to their own inventory and skip vanilla afterward)
• forcing the pickup regardless of pick up delay or the item "owner" (not sure if we need to account for that, use cases seem pretty niche)

It would be nice to allow your implementation to be used at least in the first scenario outlined above.

[yajatkaul]

Okay so basically i came across this while i was making a tasks mod so to keep a track of when the player gets and item and stuff, there is a bit more to it but yea that's why i needed this,
And yes since u mentioned the cancellable thing in your first point, sounds like a good idea and i have hence added that.

[Fuzss]

Great, thanks.

Please also address the review. Ideally to make sure no vanilla checks are skipped, moving the injection point to where the item is added to the player inventory (Inventory::add) might be ideal. That will also allow to minimize the injected code.

[yajatkaul]

I'm not sure about the Inventory::add i think then i cant make the event cancellable, or maybe i just didn't get to the right method.

[Fuzss]

Don't seem to be able to add any code suggestions as part of the review, so here are some snippets for changes:

This is how the Mixin should look:

	@Inject(method = "onPlayerCollision", at = @At(value = "INVOKE", target = "Lnet/minecraft/entity/player/PlayerInventory;insertStack(Lnet/minecraft/item/ItemStack;)Z"), cancellable = true)
	private void onPlayerPickup(PlayerEntity playerEntity, CallbackInfo ci) {
		ItemEntity itemEntity = (ItemEntity) (Object) this;
		boolean allowPickup = ServerEntityEvents.ITEM_PICKUP.invoker().onPickup(playerEntity, itemEntity, itemEntity.getStack());

		if (!allowPickup) {
			ci.cancel();
		}
	}

Also Javadoc could be slightly expanded:

	/**
	 * Called during {@link ItemEntity#tick()} on the logical server if an entity tries to pick up any {@link ItemEntity}.
	 *
	 * <p>Picking up of an item is determined by {@link ItemEntity#onPlayerCollision(PlayerEntity)}.
	 *
	 * <p>Returning {@code false} prevents vanilla from handling the pick-up.
	 */
	public static final Event<ItemPickup> ITEM_PICKUP = EventFactory.createArrayBacked(<...>

[maityyy]

I have a strong belief that this should be in ServerLivingEntityEvents in the fabric-entity-events-v1 module.

[maityyy]

(mojmap)

I think we need a more general solution that supports not only player but also mobs. I.e. the event should accept LivingEntity instead of Player and be invoked also in Mob::pickUpItem or before Mob::pickUpItem (Mob::aiStep and ObtainRaidLeaderBannerGoal::tick).

edit: But the problem is that Mob::pickupItem has overrides and can also be called by other mods (but can't remember if this is a problem for FAPI tbh)