Fix Items with a negative pickup delay being able to be picked up #13008
Conversation
This doesn't match current vanilla behaviour, and vanilla overflows short values
ie `/summon item ~ ~1 ~5 {Item:{id:"minecraft:mace",count:1},PickupDelay:60000}`
wraps to a pickup delay of `-5536`, which vanilla treats as infinitely un-pickupable.
|
Can be good mention this behaviour in docs for the method in Item? |
|
Should be fine as paper moved off of wall-time for items. CB commit 029ca3e introduced this change initially to compensate for wall time potentially jumping into negative values. |
| @@ -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); | |||
There was a problem hiding this comment.
That's not the right way to clamp, did you look at the parameters?
There was a problem hiding this comment.
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
| @@ -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. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
|
Item#canPlayerPickup should be updated too |
Fixes a bug where Items with a negative pickup delay can be picked up, which doesn't match vanilla behaviour, where any value != 0 is unable to be picked up
this is most noticable with summon commands, where high values (exceeding max short) of pickupdelay can overflow into the negatives:
ie
/summon item ~ ~1 ~5 {Item:{id:"minecraft:mace",count:1},PickupDelay:60000}(test item with
/data get entity @e[type=minecraft:item, distance=..4, limit=1] PickupDelay)->
-5536svanilla: unable to be picked up
paper main: able to be picked up
paper after patch: unable to be picked up
as all the changes were within existing bukkit / paper code, no new comments were added.
its possibly worth discussing the last change
the behaviour is effectively the same, skipping the if statement after, but I'm wondering if modifying pickupdelay had any deeper meaning / avoiding that early return, in-case behaviour changes in the future.