Extend HumanEntity#dropItem API

[Strokkur424]

Closes #11656
Post-softspoon version of #11689

Motive

Developers currently have to either use NMS or copy-paste the already existing NMS implementation. This PR aims to fix that by exposing ways to call that NMS implementation more easily by extending onto the (Human)Player#dropItem method to provide more extensive ways of making the player drop items from their inventory.

What does it add?

The following overloads to the HumanPlayer#dropItem(...) method have been added:

• Item dropItem(int slot, int amount)
• Item dropItem(EquipmentSlot slot, int amount)

Additionally, the following methods were introduced:

• Item dropItemRandomly(int slot, int amount)
• Item dropItemRandomly(EquipmentSlot slot, int amount)

Furthermore, booleam dropItem(boolean) has been deprecated in favor of Item dropItem(EquipmentSlot.HAND, amount).

Implementation details

Internally, the implementation calls net.minecraft.world.entity.player.Player#dropItem(...), making it a fairly maintainable API.

JavaDocs status

JavaDocs have been added.

Usage example

/**
 * Strips the player of all currently wearing armor
 */
private void stripOfArmor(final Player player) {
    player.dropItem(EquipmentSlot.HEAD, 64);
    player.dropItem(EquipmentSlot.CHEST, 64);
    player.dropItem(EquipmentSlot.LEGS, 64);
    player.dropItem(EquipmentSlot.FEET, 64);
}

[kennytv]

All of the default methods that are single-line redirects probably make it harder to parse than they help, I'd reduce some or even all of them, not sure. If you do want to keep default helpers, they should be moved as proper default methods to the interface, so it's immediately clear what they do

[Strokkur424]

Oh that's smart, I hadn't though about that. Thanks for that suggestion

[kennytv]

And you can remove the Paper comments and add imports instead of fqn

[SoSeDiK]

Are this many overloads really needed?
Personally, I'd go only with just one Item dropItem(ItemStack itemStack) plus boolean throwRandomly and Consumer<? super Item> function to apply data before adding to the world, and would make that itemStack independent of anything, so that you could drop any item in any context you want. You can always easily manually remove/decrease item's amount in any equipment/inventory slot if needed.

[NonSwag]

I think removing the thrower parameter would already decrease the method amount by a fair bit
If you want to set the thrower just do it but by default it should use the player as the thrower

[Strokkur424]

Personally, I'd go only with just one Item dropItem(ItemStack itemStack) plus boolean throwRandomly and Consumer<? super Item> function

I feel like that is too complicated and would kinda defeat its purpose.

• The reason for the int slot overload is the fact that when having just an ItemStack itemStack overload, it always just chooses the first one that applies. With a slot you can choose the precise one to drop.
• The reason for the EquipmentSlot one is due to the the standard int slot having no way to select those in armor or offhand slots.

[I] would make that itemStack independent of anything

That feels weird, as dropping an item sounds like you actually don't have it in your inventory anymore. Giving the developer the task of removing it themselves feels like a fair bit of unneeded code in this context.

[Strokkur424]

I think removing the thrower parameter would already decrease the method amount by a fair bit If you want to set the thrower just do it but by default it should use the player as the thrower

I mean, it would not remove a single method, as there is also boolean throwRandomly, which definitely should be exposed as API, due to the complexity of that operation. Though we could get rid of the thrower parameter instead always have <itemselector>, boolean throwRandomly, which usually would just make the last param be set to false.

That sounds like a good middle-ground solution to me. If the developer wants to set the thrower they can do so.
I am not sure if it should flag the inventory owner of the Item as its thrower by default, but that can be put up to discussion.

That means it would turn to only these overloads:

• Item dropItem(ItemStack itemStack, boolean throwRandomly)
• Item dropItem(int slot, boolean throwRandomly)
• Item dropItem(EquipmentSlot slot, boolean throwRandomly)

[SoSeDiK]

I feel like that is too complicated and would kinda defeat its purpose.

It's similar to already existing methods (e.g. World#dropItem), entity spawning methods have the save overloads.

The reason for the int slot overload is the fact that when having just an ItemStack itemStack overload, it always just chooses the first one that applies. With a slot you can choose the precise one to drop.

No, I mean to literally not link the dropped itemstack to anything, i.e. the drop would simply drop the cloned item no matter its source, not remove it from inventory or anything else. Then you just get the item in that slot and decrease its amount.

[Strokkur424]

It's not entirely the same though. World#dropItem(Location, ItemStack, Consumer<? extends Item>) is defines as follows:

@Override
public org.bukkit.entity.Item dropItem(Location loc, ItemStack item, Consumer<? super org.bukkit.entity.Item> function) {
    Preconditions.checkArgument(loc != null, "Location cannot be null");
    Preconditions.checkArgument(item != null, "ItemStack cannot be null");

    ItemEntity entity = new ItemEntity(this.world, loc.getX(), loc.getY(), loc.getZ(), CraftItemStack.asNMSCopy(item));
    org.bukkit.entity.Item itemEntity = (org.bukkit.entity.Item) entity.getBukkitEntity();
    entity.pickupDelay = 10;
    if (function != null) {
        function.accept(itemEntity);
    }
    this.world.addFreshEntity(entity, SpawnReason.CUSTOM);
    return itemEntity;
}

Notice the new ItemEntity(...). This is what actually allows for the pre-spawn function to make sense, as it runs before it gets added to the world.

In my implementation, I directly call net.minecraft.world.entity.player.Player#drop(...), which does not allow for such an operation. It would run the Consumer after the Item has already been added to the world, defeating its purpose.

[SoSeDiK]

You can add overload to net.minecraft.world.entity.player.Player#drop, it (*well, ServerPlayer) creates ItemEntity right at the beginning of the method

Edit: by the way, since it calls Player#drop, might be useful to mention in javadoc that it'll cause PlayerDropItemEvent?

[Strokkur424]

It'd honestly rather not change NMS code without a valid reason. Could add the JavaDocs information though

[SoSeDiK]

Should be fine, it's done all the time and is a small change. I'd argue the ability to drop any item, not linked to inventory would be more useful and cover more cases. You can already get an item by int/equipment slot and decrease its amount alongside if you need that.

[Strokkur424]

I suggest that it is not yet merged. After some discussion, I'd like to change it one last time

[Strokkur424]

Alright, I've done the following changes now:

• Removed throwRandomly parameter in favor of new methods: dropItemRandomly(...)
• Deprecated boolean dropItem(boolean) in favor of Item dropItem(EquipmentSlot.HAND, int)
• Removed dropItem(ItemStack, ...) overloads. They just felt out-of-place and not really particularly. The operations done in that method can be easily simulated by a dev using that API.
• The methods now look like this:
  • Item dropItem(int slot, int amount)
  • Item dropItem(@NotNull EquipmentSlot slot, int amount)
  • Item dropItemRandomly(int slot, int amount)
  • Item dropItemRandomly(@NotNull EquipmentSlot slot, int amount)
• The amount can be any arbitrary number. If it is 0 or below, the method just returns null, if it is too big, it will only drop as much as there is amount in that ItemStack from the selected slot.
• (Hopefully) improved the JavaDocs

[SoSeDiK]

I genuinely do not see the benefit of doing this, as opposed to:

Code sample

// Get any item you want
ItemStack item = player.getInventory().getItem(EquipmentSlot.HAND);
ItemStack item = ItemStack.of(Material.GOWSTONE);
ItemStack item = // some other source

// Drop it, provide any amount with `item.asQuantity(amount)`
// Additionally with optional overload to apply data before drop event & item spawn occurred
player.dropItem(item, item -> {
    item.setGlowing(true);
});

// Subtract its amount if needed (this will decrease item's amount in inventory)
item.subtract(item.getAmount());

The difference would literally be a one-liner (subtract is done behind the scenes instead of manually at the cost of additional clutter), but with more control and wouldn't need deprecating dropItem.