Extend HumanEntity#dropItem API #11810

Strokkur424 · 2024-12-24T12:03:29Z

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 method have been added:

Item dropItem(int slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItem(int slot, int amount)
Item dropItem(@NotNull EquipmentSlot slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItem(@NotNull EquipmentSlot slot, int amount)
Item dropItemRandomly(int slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItemRandomly(int slot, int amount)
Item dropItemRandomly(@NotNull EquipmentSlot slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItemRandomly(@NotNull EquipmentSlot slot, int amount)
Item dropItems(int slot, @Nullable Consumer<Item> entityOperation)
Item dropItems(int slot)
Item dropItems(@NotNull EquipmentSlot slot, @Nullable Consumer<Item> entityOperation)
Item dropItems(@NotNull EquipmentSlot slot)
Item dropItemsRandomly(int slot, @Nullable Consumer<Item> entityOperation)
Item dropItemsRandomly(int slot)
Item dropItemsRandomly(@NotNull EquipmentSlot slot, @Nullable Consumer<Item> entityOperation)
Item dropItemsRandomly(@NotNull EquipmentSlot slot)
Item dropItem(@Nullable ItemStack itemStack, @Nullable Consumer<Item> entityOperation)
Item dropItem(@Nullable ItemStack itemStack)
Item dropItemRandomly(@Nullable ItemStack itemStack, @Nullable Consumer<Item> entityOperation)
Item dropItemRandomly(@Nullable ItemStack itemStack)

This may look like a lot at first glance (and it kind of is), but in reality, most of these are just default methods on the HumanEntity interface. The only, actually implemented methods are:

Item dropItem(int slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItem(@NotNull EquipmentSlot slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItemRandomly(int slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItemRandomly(@NotNull EquipmentSlot slot, int amount, @Nullable Consumer<Item> entityOperation)
Item dropItem(@Nullable ItemStack itemStack, @Nullable Consumer<Item> entityOperation)
Item dropItemRandomly(@Nullable ItemStack itemStack, @Nullable Consumer<Item> entityOperation)

And even among these neither have more than 5 lines in their implementation in CraftHumanEntity.

All logic goes through the added methods private CraftHumanEntity#dropItemRaw(...) and private CraftHumandEntity#dropAnyItemRaw(...), where the logic is fairly simple due to the pre-implemented NMS method Player.dropItem(...).

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

Implementation details

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

I have added an additional overload to the the Player#dropItem(...) method, which takes in a nullable Consumer<org.bukkit.entity.Item> to allow for entity operations before the Entity has been added to the world.

JavaDocs status

JavaDocs have been added.

Usage example

// Simple command to test some of the added API
this.getLifecycleManager().registerEventHandler(LifecycleEvents.COMMANDS, event -> {
    event.registrar().register(Commands.literal("drop")
        .then(Commands.literal("one")
            .executes(ctx -> {
                ((Player) ctx.getSource().getExecutor()).dropItemRandomly(EquipmentSlot.HAND, 1);
                return 1;
            }))

        .then(Commands.literal("all")
            .executes(ctx -> {
                ((Player) ctx.getSource().getExecutor()).dropItems(EquipmentSlot.HAND);
                return 1;
            }))

        .then(Commands.literal("any")
            .then(Commands.argument("item", ArgumentTypes.itemStack())
                .executes(ctx -> {
                    ((Player) ctx.getSource().getExecutor()).dropItem(ctx.getArgument("item", ItemStack.class), item -> {
                        item.setGlowing(true);
                    });
                    return 1;
                })))

        .build());
});

kennytv · 2024-12-24T12:06:22Z

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 · 2024-12-24T12:08:21Z

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

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java

SoSeDiK · 2024-12-24T12:53:56Z

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 · 2024-12-24T13:15:24Z

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 · 2024-12-24T13:17:37Z

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 · 2024-12-24T13:21:11Z

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 · 2024-12-24T13:39:48Z

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 · 2024-12-24T14:00:03Z

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 · 2024-12-24T14:04:59Z

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 · 2024-12-24T15:15:02Z

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

SoSeDiK · 2024-12-24T15:24:03Z

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 · 2024-12-24T22:10:19Z

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

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java

paper-api/src/main/java/org/bukkit/entity/HumanEntity.java

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java

Strokkur424 · 2024-12-24T23:33:31Z

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

Strokkur424 · 2024-12-26T11:56:36Z

Very well, I will go to work doing the changes!

NonSwag · 2024-12-26T12:49:58Z

dropAnyItem(ItemStack itemStack, @Nullable Consumer<Item>)
dropAnyItemRandomly(ItemStack itemStack, @Nullable Consumer<Item>)

I would call those methods dropItem and dropItemRandomly

dropItemAll(int slot, @Nullable Consumer<Item>)
dropItemAll(EquipmentSlot slot, @Nullable Consumer<Item>)
dropItemAllRandomly(int slot, @Nullable Consumer<Item>)
dropItemAllRandomly(EquipmentSlot slot, @Nullable Consumer<Item>)

I would call those methods dropItems instead of dropItemAll

Strokkur424 · 2024-12-26T12:56:14Z

Just for testing, I setup the following command:

event.registrar().register(Commands.literal("drop")
        .then(Commands.literal("one")
            .executes(ctx -> {
                ((Player) ctx.getSource().getExecutor()).dropItemRandomly(EquipmentSlot.HAND, 1);
                return 1;
            }))

        .then(Commands.literal("all")
            .executes(ctx -> {
                ((Player) ctx.getSource().getExecutor()).dropItemAll(EquipmentSlot.HAND);
                return 1;
            }))

        .then(Commands.literal("any")
            .then(Commands.argument("item", ArgumentTypes.itemStack())
                .executes(ctx -> {
                    ((Player) ctx.getSource().getExecutor()).dropAnyItem(ctx.getArgument("item", ItemStack.class), item -> {
                        item.setGlowing(true);
                    });
                    return 1;
                })))

    .build());

You can see the result here:

reduced_5.mp4

NonSwag · 2024-12-26T12:58:22Z

Also, I don't really like that all methods require an item consumer (or null)
I'd propose adding overload methods that pass null for you
But on the other hand, those are way too many methods already even without overloads

Strokkur424 · 2024-12-26T12:58:34Z

I would call those methods dropItem and dropItemRandomly

I feel like new API users could get confused on what those methods do, even when reading the JavaDoc (which I am yet to add).
I want to make it fairly clear which methods drop actual ItemStack from the player's inventory and which methods could cause a dupe glitch 😂.

I would call those methods dropItems instead of dropItemAll

Yeah, sounds like a good idea.

Strokkur424 · 2024-12-26T12:58:55Z

Also, I don't really like that all methods require an item consumer (or null) I'd propose adding overload methods that pass null for you But on the other hand, those are way too many methods already even without overloads

I already have overload methods for that

Strokkur424 · 2024-12-26T12:59:40Z

Most of these are just default methods on the interface

@Nullable
public Item dropItemRandomly(@NotNull EquipmentSlot slot, int amount, @Nullable Consumer<Item> entityOperation);

@Nullable
public default Item dropItemRandomly(@NotNull EquipmentSlot slot, int amount) {
    return this.dropItemRandomly(slot, amount, null);
}


@Nullable
public default Item dropItemAll(int slot, @Nullable Consumer<Item> entityOperation) {
    return this.dropItem(slot, Integer.MAX_VALUE, entityOperation);
}

@Nullable
public default Item dropItemAll(int slot) {
    return this.dropItemAll(slot, null);
}

@Nullable
public default Item dropItemAll(@NotNull EquipmentSlot slot, @Nullable Consumer<Item> entityOperation) {
    return this.dropItem(slot, Integer.MAX_VALUE, entityOperation);
}

@Nullable
public default Item dropItemAll(@NotNull EquipmentSlot slot) {
    return this.dropItemAll(slot, null);
}

NonSwag · 2024-12-26T13:02:54Z

I would call those methods dropItem and dropItemRandomly

I feel like new API users could get confused on what those methods do, even when reading the JavaDoc (which I am yet to add). I want to make it fairly clear which methods drop actual ItemStack from the player's inventory and which methods could cause a dupe glitch 😂.

dropAnyItem and dropAnyItemRandomly sounds even more confusing in my opinion
also the Any does not add any information to what the method does

Strokkur424 · 2024-12-26T13:04:10Z

Do you have any idea on what I could rename dropAnyItem to so that it is clear it is not tied to the Player's inventory in any way?

NonSwag · 2024-12-26T13:06:58Z

Do you have any idea on what I could rename dropAnyItem to so that it is clear it is not tied to the Player's inventory in any way?

Potentially somthing like dropItemAt

Strokkur424 · 2024-12-26T13:08:07Z

Potentially somthing like dropItemAt

Not sure I like it. dropItemAt(ItemStack) sounds weird, since I'd assume at to have some sort of Location parameter

NonSwag · 2024-12-26T13:08:21Z

but i think just mentioning in the docs that the item is not tied to the players inventory at all and will spawn a new item is better
in any case dropItem would be a better fit

Strokkur424 · 2024-12-26T13:08:49Z

Yeah probably. I am not having any ideas either

SoSeDiK · 2024-12-26T13:14:12Z

I would call those methods dropItem and dropItemRandomly

but i think just mentioning in the docs that the item is not tied to the players inventory at all and will spawn a new item is better
in any case dropItem would be a better fit

Clashing with other methods isn't ideal either, since the behavior is different enough. Even with javadocs it'd be quite easy to miss.

I don't like Any too, but couldn't think of anything better yet.

Stale

willkroboth · 2024-12-26T15:11:02Z

I don't like Any too, but couldn't think of anything better yet.

Would createDroppedItem and createDroppedItemRandomly work? I think create indicates that the item is new and doesn't already exist in the player's inventory.

SoSeDiK · 2024-12-26T15:35:47Z

As another suggestion, dropItemStack might work?

Machine-Maker · 2024-12-26T18:00:15Z

Maybe dropItemFrom(int/slot) and dropItemStack()?

Strokkur424 · 2024-12-26T22:24:17Z

Maybe dropItemFrom(int/slot) and dropItemStack()?

Yeah I like that one.

Machine-Maker · 2024-12-26T23:14:22Z

Ok, now its just soo many methods. What was the original issue with a randomly boolean parameter instead? I kinda think just adding that to the methods and reducing the number of methods by half is better.

SoSeDiK · 2024-12-26T13:41:11Z

paper-server/patches/sources/net/minecraft/server/level/ServerPlayer.java.patch

@@ -1376,6 +1383,11 @@
 +                    return null;
 +                }
 +            }
+            // Paper start - Extend HumanEntity#dropItem API
+            if (entityOperation != null) {
+                entityOperation.accept((org.bukkit.entity.Item) itemEntity.getBukkitEntity());


Might make sense to apply it before PlayerDropItemEvent?

SoSeDiK · 2024-12-26T13:46:07Z

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java

+
+        final net.minecraft.world.item.ItemStack nmsItemStack = CraftItemStack.asNMSCopy(itemStack);
+
+        final ItemEntity droppedEntity = this.getHandle().drop(nmsItemStack, throwRandomly, true, true, entityOperation);


drop should have a separate handling for cancelled PlayerDropItemEvent in case of arbitrary items. This is a special case, the item's source might not be player's inventory, so the item should not be neccesarily added to the player's inventory, but rather let the dev handle it.

Edit: also, as an example, if the item was from the inventory, we will be adding back its copy, essentially duplicating the item, which might not be the desired behavior. It is way easier to manually add the item to inventory if this is wanted than to prevent this if this would be the default behavior.

Strokkur424 · 2024-12-27T01:03:53Z

What was the original issue with a randomly boolean parameter instead?

Too many overloads for a single method, I guess.
But yeah, here is my plan to reduce methods (once again):

Remove the non-entityOperation overloads (api users can just pass null)
Instead of the ...Randomly methods, let's just add a boolean dropRandomly parameter back in and explain in the JD what it does (again)

That should cut it down to like a quarter of the methods. And that's gonna be it.

SoSeDiK · 2024-12-27T06:24:54Z

I think non-entityOperation ones could stay (@Machine-Maker?), given that's the only overload left; while this would be 3 methods * 2 for item consumption overloads, it would be on pair with other such APIs, and requiring users to pass null each time is annoying

Extend HumanEntity#dropItem API

53ff19f

Strokkur424 requested a review from a team as a code owner December 24, 2024 12:03

kennytv added the type: feature Request for a new Feature. label Dec 24, 2024

kennytv reviewed Dec 24, 2024

View reviewed changes

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java Outdated Show resolved Hide resolved

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java Outdated Show resolved Hide resolved

Strokkur424 added 5 commits December 24, 2024 13:15

Reduce and replace implemented methods with defaults

5361df8

Remove fully qualified imports and comments. Small cleanup

11884ca

Cleanup imports

26f826d

Further cleanup imports

a7b4433

Rename parameter

63a5240

Strokkur424 requested a review from kennytv December 24, 2024 12:23

Remove thrower and 'empty' overloads

2d9e357

kennytv previously approved these changes Dec 24, 2024

View reviewed changes

Machine-Maker reviewed Dec 24, 2024

View reviewed changes

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java Outdated Show resolved Hide resolved

Machine-Maker approved these changes Dec 24, 2024

View reviewed changes

Strokkur424 marked this pull request as draft December 24, 2024 23:11

powercasgamer suggested changes Dec 24, 2024

View reviewed changes

paper-api/src/main/java/org/bukkit/entity/HumanEntity.java Show resolved Hide resolved

paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftHumanEntity.java Show resolved Hide resolved

Refactor methods

02afac7

Strokkur424 added 2 commits December 26, 2024 13:18

Add a ton more overloads

00202b6

Implement Item consumer

daeaafa

Rename dropItemAll methods to dropItems

0ea85a3

Rename dropAnyItem methods to dropItem

9918deb

Add Javadocs

65b8763

Strokkur424 requested review from Machine-Maker and kennytv December 26, 2024 13:33

SoSeDiK suggested changes Dec 26, 2024

View reviewed changes


		final net.minecraft.world.item.ItemStack nmsItemStack = CraftItemStack.asNMSCopy(itemStack);

		final ItemEntity droppedEntity = this.getHandle().drop(nmsItemStack, throwRandomly, true, true, entityOperation);

Extend HumanEntity#dropItem API #11810

Are you sure you want to change the base?

Extend HumanEntity#dropItem API #11810

Conversation

Strokkur424 commented Dec 24, 2024 • edited Loading

Motive

What does it add?

Implementation details

JavaDocs status

Usage example

kennytv commented Dec 24, 2024

Strokkur424 commented Dec 24, 2024

kennytv left a comment

Choose a reason for hiding this comment

SoSeDiK commented Dec 24, 2024

NonSwag commented Dec 24, 2024

Strokkur424 commented Dec 24, 2024

Strokkur424 commented Dec 24, 2024 • edited Loading

SoSeDiK commented Dec 24, 2024

Strokkur424 commented Dec 24, 2024

SoSeDiK commented Dec 24, 2024 • edited Loading

Strokkur424 commented Dec 24, 2024

SoSeDiK commented Dec 24, 2024

Strokkur424 commented Dec 24, 2024

Strokkur424 commented Dec 24, 2024

Strokkur424 commented Dec 26, 2024 • edited Loading

NonSwag commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024

NonSwag commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024

NonSwag commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024 • edited Loading

NonSwag commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024

NonSwag commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024

SoSeDiK commented Dec 26, 2024

willkroboth commented Dec 26, 2024

SoSeDiK commented Dec 26, 2024

Machine-Maker commented Dec 26, 2024

Strokkur424 commented Dec 26, 2024

Machine-Maker commented Dec 26, 2024

SoSeDiK Dec 26, 2024

Choose a reason for hiding this comment

SoSeDiK Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Strokkur424 commented Dec 27, 2024

SoSeDiK commented Dec 27, 2024

Strokkur424 commented Dec 24, 2024 •

edited

Loading

Strokkur424 commented Dec 24, 2024 •

edited

Loading

SoSeDiK commented Dec 24, 2024 •

edited

Loading

Strokkur424 commented Dec 26, 2024 •

edited

Loading

Strokkur424 commented Dec 26, 2024 •

edited

Loading

SoSeDiK Dec 26, 2024 •

edited

Loading