PaperMC / Paper

The most widely used, high performance Minecraft server that aims to fix gameplay and mechanics inconsistencies
https://papermc.io/
Other
9.54k stars 2.22k forks source link

Items do not have rarity and rarity disappears when set. #11040

Closed TauCubed closed 6 days ago

TauCubed commented 2 weeks ago

Expected behavior

ItemMeta#hasRarity to return true for items with rarity and for set rarity to persist.

Observed/Actual behavior

ItemMeta#hasRarity returns false for items with rarity and set rarity does not persist.

Steps/models to reproduce

var testStack = ItemType.PLAYER_HEAD.createItemStack();
var testMeta = testStack.getItemMeta();

// this should print true as PLAYER_HEAD has uncommon rarity. It prints false.
System.out.println(testMeta.hasRarity());

testMeta.setRarity(ItemRarity.UNCOMMON);

// this should (and does) print true.
System.out.println(testMeta.hasRarity());

testStack.setItemMeta(testMeta);

// this should print true as we have set the rarity to uncommon. It prints false.
System.out.println(testStack.getItemMeta().hasRarity());

Plugin and Datapack List

[23:10:54 INFO]: Server Plugins (7):
[23:10:54 INFO]: Bukkit Plugins:
[23:10:54 INFO]:  - client-crasher, LuckPerms, test-plugin, MoDispenserMechanics, PlugManX, spark, SystemChat
[23:10:59 INFO]: There are 3 data pack(s) enabled: [vanilla (built-in)], [file/bukkit (world)], [paper (built-in)]
[23:10:59 INFO]: There are no more data packs available

Paper version

[23:10:51 INFO]: Checking version, please wait...
[23:10:52 INFO]: This server is running Paper version 1.21-47-master@62ed302 (2024-07-07T17:34:15Z) (Implementing API version 1.21-R0.1-SNAPSHOT)
You are running the latest version
Previous version: 1.21-44-8d91b85 (MC: 1.21)

Other

No response

Lulu13022002 commented 1 week ago

I think this works as intended the itemmeta representation is flawed and outdated since the 1.20.5 and only represent explicit component set on the patch. Since the rarity of a player head is already uncommon on the item itself (its prototype). Setting it on the patch will just revert to the prototype value. In the future with the item data component api you could do: testStack.hasData(DataComponentTypes.RARITY) to check if the component is set on the patch or the prototype.

You can see that with the give command too with /give @s minecraft:player_head[minecraft:rarity=uncommon] the component doesn't appear when you do /paper dumpitem and the only way to see implicit component is via /paper dumpitem all

TauCubed commented 1 week ago

Yeah I'm aware that it doesn't have a data component but in it's current implementation it is a bug.

electronicboy commented 1 week ago

This appears to be conforming to mojangs expectations of how patches work?

    private void applyPatch(DataComponentType<?> type, Optional<?> optional) {
        Object object = this.prototype.get(type);
        if (optional.isPresent()) {
            if (optional.get().equals(object)) {
                this.patch.remove(type);
            } else ...
        } ...
    }
TauCubed commented 1 week ago

Yeah that is. But the return values aren't. Perhaps include the same prototype check when getRarity is called. hasRarity seems redundant. Unless reserved for explicitly set rarity but then you'd run into the issue of rarity not being set when it matches the rarity of the prototype so it doesn't really make sense to have that method return anything other than true.

For instance: say I'm making a chat-item plugin and I want to apply the color of the item's rarity to it's translation component/item name/display name depending on what is set. Right now it doesn't seem possible to get the default rarity of the item. And the deprecated method Material#getRarity fails entirely and always, as it calls getItemMeta#getRarity#name, and of course a newly created ItemStack doesn't have any explicitly set rarity, so the method literally blows up for all 1333 items in the game that you can call it on.

I understand a new API is being worked on but I think a simple patch to getRarity is all that is needed, at least for now.

electronicboy commented 1 week ago

ItemMeta does not have the patch to perform such operations, the entire thing is a copy of the data stored on the stacks component. ItemMeta would need a pretty hefty overhaul to be able to perform such an operation, one which is already planned down the line of the new API. I cannot see us trying to shim this into the current setup as stuff stands right now.

it would probably be viable to patch the method on ItemStack given that we make all ItemStacks backed by mojangs stacks now

TauCubed commented 1 week ago

Alright, thanks for your time. Should I leave this issue open until the new API is out (to prevent creating of redundant duplicates) or close it?