Skip to content

Fix Items with a negative pickup delay being able to be picked up #13008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+14 −9
Open

Fix Items with a negative pickup delay being able to be picked up #13008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36e84e2
Fix Items with a negative pickup delay being able to be picked up.
456dev Aug 22, 2025
ddddd76
Update Javadocs of API
456dev Aug 22, 2025
2b0557d
clamp minimum in api setter
456dev Aug 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion paper-api/src/main/java/org/bukkit/entity/Item.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,22 @@ public interface Item extends Entity, io.papermc.paper.entity.Frictional { // Pa

/**
* Gets the delay before this Item is available to be picked up by players
* <p>
* If the value is 0, the item can be picked up immediately.
* If the value is negative or {@link Short#MAX_VALUE} (32767), the item cannot be picked up at all.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this part is needed.
Not sure about explicitly show the magic value when setCanPlayerPickup exists, maybe instead just use a Range/IntRange annotation since it's an integer on 16 bits. The negative value doesn't make sense in that context too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to confirm, i should annotate this param to 0 to Short.MAX_VALUE? or 1 to short.max_value -1 (just actually functional "delay")? or smth else?

Copy link
Contributor Author

@456dev 456dev Aug 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, if we prevent players from passing a negative number here, should the get method translate negative numbers to NO_PICKUP_TIME, so api users dont get a value they cannot restore later?

* Otherwise, the value is the number of ticks before the item can be picked up.
*
* @return Remaining delay
*/
public int getPickupDelay();

/**
* Sets the delay before this Item is available to be picked up by players
*
* <p>
* If the value is 0, the item can be picked up immediately.
* If the value is negative or {@link Short#MAX_VALUE} (32767), the item cannot be picked up at all.
* Otherwise, the value is the number of ticks before the item can be picked up.
*
* @param delay New delay
*/
public void setPickupDelay(int delay);
Expand Down
11 changes: 4 additions & 7 deletions paper-server/patches/sources/net/minecraft/world/entity/item/ItemEntity.java.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@
}
}

@@ -334,10 +_,73 @@
@@ -334,10 +_,70 @@
ItemStack item = this.getItem();
Item item1 = item.getItem();
int count = item.getCount();
Expand All @@ -190,7 +190,7 @@
+ boolean flyAtPlayer = false; // Paper
+
+ // Paper start - PlayerAttemptPickupItemEvent
+ if (this.pickupDelay <= 0) {
+ if (this.pickupDelay == 0) {
+ org.bukkit.event.player.PlayerAttemptPickupItemEvent attemptEvent = new org.bukkit.event.player.PlayerAttemptPickupItemEvent((org.bukkit.entity.Player) entity.getBukkitEntity(), (org.bukkit.entity.Item) this.getBukkitEntity(), remaining);
+ this.level().getCraftServer().getPluginManager().callEvent(attemptEvent);
+
Expand All @@ -204,7 +204,7 @@
+ }
+ }
+
+ if (this.pickupDelay <= 0 && canHold > 0) {
+ if (this.pickupDelay == 0 && canHold > 0) {
+ item.setCount(canHold);
+ // Call legacy event
+ org.bukkit.event.player.PlayerPickupItemEvent playerEvent = new org.bukkit.event.player.PlayerPickupItemEvent((org.bukkit.entity.Player) entity.getBukkitEntity(), (org.bukkit.entity.Item) this.getBukkitEntity(), remaining);
Expand Down Expand Up @@ -237,12 +237,9 @@
+ } else {
+ item.setCount(canHold + remaining); // = i
+ }
+
+ // Possibly < 0; fix here so we do not have to modify code below
+ this.pickupDelay = 0;
+ } else if (this.pickupDelay == 0) {
+ // ensure that the code below isn't triggered if canHold says we can't pick the items up
+ this.pickupDelay = -1;
+ return;
+ }
+ // CraftBukkit end
+ // Paper end - PlayerAttemptPickupItemEvent
Expand Down
2 changes: 1 addition & 1 deletion paper-server/src/main/java/org/bukkit/craftbukkit/entity/CraftItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public int getPickupDelay() {

@Override
public void setPickupDelay(int delay) {
this.getHandle().pickupDelay = Math.min(delay, Short.MAX_VALUE);
this.getHandle().pickupDelay = Math.clamp(Short.MIN_VALUE, delay, Short.MAX_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the right way to clamp, did you look at the parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, not sure what i was thinking here, i must have misread this locally.
funny this, this still works for values -inf to Short.MAX_VALUE, but nothing greater

}

@Override
Expand Down
Loading