player.getInventory().getItem(EquipmentSlot.BODY) not implemented

StarTux commented 4 months ago

Expected behavior

Usually I can call

Player player = getPlayer();
for (EquipmentSlot slot : EquipmentSlot.values()) {
    player.getInventory().getItem(slot);
    // or
    player.getEquipment().getItem(slot);
}

and get all player equipment this way.

Observed/Actual behavior

However, the getter for new EquipmentSlot BODY is not implemented for CraftInventoryPlayer.

java.lang.IllegalArgumentException: Not implemented. This is a bug
        at org.bukkit.craftbukkit.inventory.CraftInventoryPlayer.getItem(CraftInventoryPlayer.java:166)

Steps/models to reproduce

Call

player.getInventory().getItem(EquipmentSlot.BODY);

This should simply return null because iterating over EquipmentSlot was a common shortcut to iterate over a player's inventory in Paper 1.20.4 and prior. Instead, an exception is thrown, breaking existing plugins.

Plugin and Datapack List

plugins Server Plugins (58): Bukkit Plugins:

AFK, Area, ArmorStandEditor, Auction, Bans, BlockClip, BlockTrigger, Chat, Connect, Core Countdown, Editor, Fam, Fly, Home, HotSwap, Inventory, ItemStore, Kit, MagicMap Mail, MemberList, Menu, Money, Mytems, OpenInv, Perm, PlayerCache, PlayerInfo, PlugInfo PlugManX, Protect, ResourcePack, Rules, Server, Shutdown, Sidebar, Spawn, Spike, SQL StarBook, StopRain, Streamer, Televator, Ticket, Tinfoil, Title, TPA, Tutor, Vault VoidGenerator, Vote, Wall, Wardrobe, Warp, WorldEdit, WorldMarker, Worlds

datapack list There are 3 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)], [paper (built-in)] There are no more data packs available

Paper version

This server is running Paper version 1.20.6-115-master@9d6f2cc (2024-05-28T14:55:16Z) (Implementing API version 1.20.6-R0.1-SNAPSHOT)

Other

No response

Machine-Maker commented 4 months ago

Well I'd much rather it return an empty stack instead of null, but moving past that, I wonder if it isn't better API to throw for invalid equipment slots. Certainly I think the setter for a certain slot should throw instead of just doing nothing and not informing the user at all that they are taking an invalid action. And then it seems weird that one interaction returns empty and the other throws.

electronicboy commented 4 months ago

if you're doing something invalid you should generally be told that you're doing something invalid; There are 0 promises that you can iterate over random enums inside of the API safely, and so I don't think that this is a concern vs us returning borked data

PaperMC / Paper