#Paper Item Property API

1 messages · Page 1 of 1 (latest)

wind wasp
#

đź“ť

#

Because of how far future this is, generally all plans in here are subject to change. Please however feel free to give any feedback!

wind wasp
#

@bleak fjord The goal with this is that yes I want to allow people to register their own properties. Not supported but it would work.

bleak fjord
#

Ah okay. Doesn't solve the pita of writing invalid values into vanilla keys.

wind wasp
#

I mean that’s an issue that can’t be avoided, so if someone wants to do that they can.

#

This is the things that the property system won’t prevent, as in general you are accepting the fact that you won’t have any pretty validating for you.

bleak fjord
#

Regarding the discussion in contrib, does it just work cloning the nbt and after that modifying it in ItemMeta API using Property API. -> To keep the "legacy functionality of getItemMeta/SetItemMeta".

Just make ItemMeta lazy load and/or directly modify the thing instead of making a copy

bleak fjord
wind wasp
#

Yes item meta will always be cloned

wind wasp
bleak fjord
wind wasp
#

Yes

bleak fjord
wind wasp
#

?

bleak fjord
#

You said ItemMeta impl is pita and with the property way we are able to maintain Paper easier.

#

ItemMeta API will be like the BlockData API currently.

wind wasp
#

Yes exactly

eager swift
#

What do you plan for the itemstack api, there currently two impl and moving all the impl to a nms baked itemstack could be simpler

wooden sail
#

hard disagree on letting anyone create their own property keys

#

we don't want an nbt api

#

we already have the PDC for people storing stuff in places

bleak fjord
#

The big question is, how do we restrict that code wise.

wooden sail
#

just not having public ctors?

bleak fjord
#

package private static factory methods in interface isn't a thing, if I'm not wrong.

wind wasp
#

I mean, you won’t be able to register your own adapters.

wooden sail
#

still, why even make it public

#

there's like 0 reason it has to be, its not even easier

wind wasp
#

I mean doesn’t have to be that’s just how it was for the time being

#

Or well, actually, it’s cause certain item meta will create their own property keys for legacy purposes.

#

But I mean I can restrict it to server only sure

#

I just didn’t see the issue with having it built for custom keys in mind

wooden sail
#

why would there be any dynamically created property keys

#

they should all be static final fields, I feel like that's the only time any should ever be constructed

#

that also means record's aren't really of the most use for them (using the interface/impl pattern) cause reference equality should be used

#

if there needs to be some for some legacy values, they can just be kept private in the meta implementations

wind wasp
#

I see, I mean sure I don’t see an issue with that. Just I’m curious why ur so against custom keys like that.

wooden sail
#

because its a hint of an nbt api

#

we have custom keys already. pdc

#

what is the purpose of the Value interface?

#

it's required for PropertyKey, but also returns the property key?

#

that seems redundant

#

also, I don't think we want to support mutability there, for collections

wind wasp
#

I disagree, I think the the mutability is an important part of being able to “lazy” access the collection.

#

The concept of the value class is more inspired by sponge, and at least I took it to allow to have special collection values like that.

wooden sail
#

why does "set" exist? that doesn't exist in nbt

wind wasp
#

The Value can be throught of as representing the tag, where you can use getValue() to actually parse and get a value from it.

wind wasp
#

Where although it’s an integer it can be represented a set

wooden sail
#

I think Set is the wrong name for that then

#

I think Value is really overthinking it. If we want lazily transformed things, we can make a custom List implementation which delegates to the ListTag

#

PropertyKey<List<NamespacedKey>> is just simpler to deal with than PropertyKey<Value<List<NamespacedKey>>>

wind wasp
#

It wouldn’t be like that tho, it’d be PropertyKey<ListValue<NamespacedKey>>

#

I defo see your point but

wooden sail
#

well I think that's also bad. We should just use java's types, for lists and maps

#

still not sure about mutability tho

wind wasp
#

I mean that whole part is inspired for sponge and I can’t really fight hard because not even I fully know why it’s needed.

I figured however that having a sort of middle way of representing an “unparsed” tag however would be useful.

Because at least it would allow things such as setting the tag to another tag without doing any parsing or what not.

#

Yeah mutability is a whole other concept sponge had a crazy amount of work on that I kinda scaled down.

#

Each collection had a mutable/immutable type

#

You can see this inspiration in the property reader/property holder

wooden sail
#

the whole "parsing vs not parsing" thing, like I think the important part is just not parsing it always. And that still can happen with immutable collections

#

nbt isn't that complicated, its parsing some strings and what not into things, this is hardly performance heavy stuff

#

the most complicated thing will probably end up being the adventure CanDestroy and CanPlaceOn bits

wind wasp
#

Yeah it’s mostly supposed to be so we can move the value without reading it as an adventure component or whatever.

So I can copy the Value of a display name from one item to another and it’ll just copy the underlying string tag.

#

But if many people disagree with the whole value concept I will back down, but, I think that it may be useful if I can fully understand how sponge utilizes this.

wooden sail
#

I just don't see that as useful enough to warrant all this extra complexity from Value

#

if we wanted some copying thing, we could maybe have a way to copy a value just using a specific key and a property holder

#

PropertyHolder#copyTo(PropertyHolder, PropertyKey...)

#

that could skip all parsing and just copy the underlying nbt tag

wind wasp
#

ic

wooden sail
#

with this, specific to item meta, are you planning to do any item type checks? or can you just use a key on an itemstack of any type?

wind wasp
#

Any type, it’s basically just NBT manipulation really.

#

I am planning on using this system for entities as well

#

Just a way to edit the NBT entity blobs

wooden sail
#

then why does it matter if you change the material of the itemstack?

#

you put @Deprecated on that

wind wasp
#

Ahh okay so that deprecation I’ll prolly remove but

#

Basically vanilla doesn’t really support changing the material, at least I remember seeing some deprecation in the NMs ItemStack class

#

I made the whole clear meta logic in each meta class so that deprecation isn’t needed anymore tho.

wooden sail
#

well its final on nms.ItemStack

wind wasp
#

Yeah that’s prolly where I got that from

wooden sail
#

I guess I didn't realize that setType just isn't gonna work in places where asCraftMirror is used

wind wasp
#

I thought bukkit made it not final, is that really the case? 🥶

wooden sail
#

oh they do

#

I didnt realize I was looking at vanilla source

wind wasp
#

Just I think that it’s something we shouldn’t encourage but not gonna fight that

wooden sail
#

yeah, I can see that as something we don't want to support

#

I thought it was fully supported by vanilla

wind wasp
#

I mean idk how many people actually do that but yeah it mostly became an apparent problem with item meta

#

As I now need to manually delete old tags

wooden sail
wind wasp
# wooden sail .

So it's mostly the fact that Value allows for a light "wrapping" around nbt tags. So for example, getting the Value<Component> from a tag won't parse the underlying tag until needed.

wooden sail
#

yeah ik, but why is that useful? we discussed that above. you said it was for copying, and I said you don't need Value to copy from one to another without parsing

#

you just have a copyTo(Key, Holder) method on a holder

wind wasp
#

So it can also apply to copying, yeah.
But I think it's just a sort of middle ground.

Because at least, for collections, the reason why there are re-implementions of "Values" is so that you don't need to parse the entire collection.

If I have a list of item stacks, I can use ListValue<ItemStack>.get(0) in order to get the 1st slot without caring about the other items.
If we want to parse the entire list (such as wanting to do calculations off thread or something), we would .getValue() the ListValue to return just a List<ItemStack>, which is pretty much just the entire value compiled.

wooden sail
#

you can pretty much just do that with a transforming list

#

and that partial parsing benefit you cite for collections doesn't apply to single values, so is the only benefit the copying mechanic (for single value types)?

wind wasp
#

Yeah

wooden sail
#

I don't think that's enough to warrant introducing a very common wrapper type. We already had a discussion about Reference and how with the registry modification API it should be used as little as possible (in that case, only in static final fields). Value is all over a frequently used API and I don't think it has to be that involved.

#

You can just have a transforming list backed by a ListTag with functions to convert one way and the other

wind wasp
#

Fair

wooden sail
#

oh lol ListTag actually is a java List anyways

#

so you can just use the one we already have that jmp made for 2-way transformations

wind wasp
#

And how exactly should I handle stuff like multi map

#

I think what I do for stuff like contains I iterate until I find it.

wooden sail
#

where is there a multimap in the nbt structure?

wind wasp
#

I mean, there is a MultiMapValue\

#

It wraps around attributes

wooden sail
#

right, but like what item uses it

#

oh attributes. cause each attribute type can have multiple isntances?

wind wasp
#

Something like that, I think

wooden sail
#

well a MultiMap isn't really the direct representation of what it is in the NBT

#

its a list of compounds

#

if this API is supposed to match up with nbt stucture, it should be that

wind wasp
#

Yeah, I am just not sure how "far" I should go when it comes to abstracting away from NBT.

wooden sail
#

well I thought this API was supposed to not do as much of that as ItemMeta does

#

ItemMeta can have methods that abstract away from it

wind wasp
#

I agree, I think I just got a bit carried away for that lol.

wooden sail
#

well, the API doesn't have a type for representing a modifier with its attribute type

#

so maybe it'd just be a Pair<Attribute, AttributeModifier> for modifiers

#

or it could have some other special case, idk

#

either way, 1 instance of special casing isn't worth it to have Value all throughout this API imo

wind wasp
#

Oh nvm, legit just found it, ugh.

wind wasp
#

@wooden sail Implemented

#

It actually cleans up alot of stuff, thankfully.

wooden sail
#

talk be through PropertyReader vs PropertyHolder? It seems like PropertyHolder is the exact same, in which case why differentiate?

#

also, are you sticking with Property? I think kash agreed that we can be more specific

#

while entity and item properties share a lot in common, I feel like they still should be called different things

#

also, for the sake of making reviewing this easier, I might do separate PRs. One for the property system and then a different PR for all the implementions of ItemMeta using it.

wind wasp
wooden sail
#

well I'm saying the keys should be different types, they can extend a common base (if there is any value for them do so).
but like it doesn't make sense to have the property-related methods on Entity also accept keys meant for itemstacks right?

#

ItemPropertyKey, EntityPropertyKey

#

probably a lot of the logic can be shared between them, but they should be separated somehow

#

maybe the Holder interface can take a generic to determine the key type

bleak fjord
#

@wind wasp for the whole config serialization part I would rather prefer to have a Jackson SNBT module.
It has the advantage, that a developer can decide which serialization (s)he prefers.
I personally hate how the configuration api is build and using snakeyaml.

Just passing an instance of the following:

import org.bukkit.Color;
import org.bukkit.FireworkEffect;
import org.bukkit.Location;
import org.bukkit.NamespacedKey;
import org.bukkit.attribute.AttributeModifier;
import org.bukkit.block.banner.Pattern;
import org.bukkit.potion.PotionEffect;
import org.bukkit.util.BlockVector;
import org.bukkit.util.BoundingBox;
import org.bukkit.util.Vector;

public record ComplexPojo(
        AttributeModifier strength,
        BlockVector skyIslandOrigin,
        BoundingBox boundingBox,
        Color color,
        FireworkEffect fireworkEffect,
        Location location,
        NamespacedKey namespacedKey,
        Pattern pattern,
        PotionEffect potionEffect,
        Vector heavyWeightedPressurePlateJump
) {
}

Should result in a readable yaml, toml, json, SNBT. https://gist.github.com/yannicklamprecht/e6f6e9fd0f5441fd9547672ddbee4fd3

wooden sail
#

The problem with that is, it brings back requiring maintaining the serialization system. For each meta type, we’d need to say what needs to be serialized as what.

#

Owen was trying to remove that requirement all together, by just having it serialize (in whatever form) the raw nbt

#

I think I am pro having serialization stuff for those types, but it’d probably not be Jackson, but configurate as we already use it (it also supports whatever type, hocon, toml, etc).

pallid kiln
#

tbf configurate isn't the serialisation system, it's just a layer on top of [serialiser] to treat values as a graph of vector or scalar nodes, in my experience Jackson is extremely versatile for many reasons, one of them being the support for other formats is pretty much just "replace new ObjectMapper() with new SomethingElseMapper()", but for this purpose, so is the configurate layer, just gotta include the right module

wooden sail
pallid kiln
#

sorry, that was an unnecessary 🤓 moment lol

proud night
wooden sail
#

the thing is... a lot of the numbers can be any number type

#

vanilla accepts short, byte, int, long for a lot of things

#

and that has to be preserved if you are serializing and deserializing

proud night
#

Yeah but like, lets say the property height takes a short; when serialized, it'll take that short value and string-ify it into 5. Then later on when de-serialized, it'll get the height property by name baed on the key, see that it's type is short, and convert the string value "5" to that

#

Or am I missing/misunderstanding something?

pallid kiln
#

thing is, snbt makes a difference between {hello: 5} vs {hello: 5s} and {hello: 5b}, and when checking for a specific value, you do contains("key", TagType.SHORT) (or whatever the exact names are), so it's not "read as", it's when actually querying for the value after it was deserialised, so if it was serialised as hello: 5, after deserialising that contains check will evaluate to false (as it is deserialised as a full-width integer)

#

the value is deserialised from snbt as a whole, then the values and types are checked for whatever they are used

proud night
#

An item with {hello: 5b} will get serialized into

hello: 5

Then later when de-serializing it, it'll turn the 5 into a byte because it knows it's meant to be one (as that's the hello property's type), and a method like contains("hello", TagType.BYTE) would return true as it is a byte now, in it's de-serialized form

pallid kiln
#

you don't know it's a byte

wooden sail
#

but hello might also work fine with a short, int or long

#

that's what I'm saying. In a lot of places, ANY number type works fine

pallid kiln
#

you first deserialize the entire snbt into CompoundTag

proud night
#

Ahhh I see, that's what I was missing lol - thought it always know what number type is it meant to get

wooden sail
#

for some properties, yes, its only gonna be one type

#

but there are plenty where any number type can work, and then if you just serialize it as the number, that type info is lost

proud night
#

I suppose you could serialize it into 5b, 5s etc. and use that when de-serializing it, but at that point it's not much better then just SNBT, other then being a little more readable / easier to work with for users ig

pallid kiln
#

i don't think the point is to be "better than snbt"

#

i'm still with the idea that it should align with the way the game already offers, the game already accepts and prints snbt in many commands, there is no reason to invent and maintain another format (not to mention it is what isn't desired)

narrow meadow
#

just make a new format!

#

hello: "yes hello I am a byte of value '3'“
thing: "yes hello I am a string of value 'I like cats'“

#

runs away

pallid kiln
#

no no you come back here

#

you are onto something

narrow meadow
wooden sail
#

we can make snbt more readable in the yaml format. Just make it a multiline string, and format it like json is. newlines for new properties etc.

#

so in yaml its

# 
hello: |
  {
    value: 5b,
    other_value: true
  }
#

so its not a huge one line thing

pallid kiln
#

wait so you mean, simply put the snbt in a single yaml string? like what pop suggested last night but multi-line i guess

wooden sail
#

yeah, I think that works to maintain readability as much as possible, while using a format that is thoroughly documented via the mc wiki

#

it keeps number types, I feel like it addresses all the issues

pallid kiln
#

does the "vanilla" nbt parser skip newlines? lol I know it skips regular spaces

wooden sail
#

well we can remove newlines manually when we parse it if not

pallid kiln
#

yea

wooden sail
#

we might have to write our own thing to spit out multiline snbt, but that doesn't sound too hard

#

although, the paper dumpitem command does take an indent width

wind wasp
wooden sail
#

but I'm still not sure about the whole breaking spigot compat with stack/meta deserialization pre hard fork

#

I thought I've brought it up in the past, and people have disagreed.

pallid kiln
#

hard fork then?

wooden sail
#

like with changing the ItemFlag name

pallid kiln
#

huh, TagParser uses the brigadier StringReader, which does skip newlines on skip whitespace

#

that is uuh neat

#

and the TextComponentTagVisitor can take care of indentation stuff too, well that's all that formatting issue resolved lol

viscid widget
#

will this come out with the 1.19.4 release ?

pallid kiln
#

.4 already released lol

viscid widget
#

it's experimental

#

i meant the stable release

flint oyster
#

So are you going to break the inventory plugin I use?

  '1':
    ==: org.bukkit.inventory.ItemStack
    v: 2586
    type: NETHERITE_PICKAXE
    meta:
      ==: ItemMeta
      meta-type: UNSPECIFIC
      display-name: '{"extra":[{"text":"Drillmaster 5000"}],"text":""}'
      enchants:
        DIG_SPEED: 5
        DURABILITY: 3
        LOOT_BONUS_BLOCKS: 3
      repair-cost: 63
      Damage: 40
wooden sail
#

Not planning on breaking anything. Deserialization from that format will be supported ofc to support upgrading. It might be serialized in a different format but there won’t be any info lost

wind wasp
#

@blazing obsidian

test:
  ==: org.bukkit.inventory.ItemStack
  v: 3337
  type: PLAYER_HEAD
  meta:
    ==: ItemMeta
    meta-type: PAPER_META
    snbt: |-
      {
          Count: 1b,
          id: "minecraft:player_head",
          tag: {
              SkullOwner: {
                  Id: [I; 1065279268, -1351007849, -1353122767, -455512511],
                  Name: "Owen1212055",
                  Properties: {
                      textures: [
                          {
                              Signature: "GYMzsf268bMs9LnAXB/f3VwQHpRQh4V9/a2aSicb6dgauZXbAy1OSYQci4AGR2DfvINHdJaBe7m47dn70J8ENqBDsjamnEAlG+q1GCWz3P2h+7Wc5VNSVKHnJmTBIEpzsXAvr/bc51i9X0wA4m285Nhe2NWsESQz9hieVQ47sYmdJMzGROoJwAdlEXqfsY3zAe/P+VEWOC3iMjGlt9Az7ubjIi4yW6pjUgT7iMAAp2YLas91XalhsMQwOy001ycxNe4eebbt1IPDBvKSiTu6jWRBeajz7fTVP7WI+DRW6HnJnlVDSzkcMx8vTE8W5x2PTQoDxa5ahg8NrhcX9aA6divYHQkJqOs5mJ2wss5Fdl73FcnRM5ueWMbRSoxm6A3OafOjfBhjC8Lj0FYrMDNUlgnl/Yx+aT9+FdaxEW7vqamIwqR+78y8O30TIczGXvG8iryzBmFJWSFINRlfn9XMG98LAlZe8R73ZnN0dZ7q3Ib5+usICsERellKRKVMfaDufGvsXVmDnoTD+2t8KJ/C4kwO5CVfaHa6SzVNBfaVGpx0sZLixKNwspbzzE9YuzkHcMRjTq9Lex7NgoGD1DxPORAohYZKsombmNa0Fon36UFvuUqo2dTFx9+AT/usgZqhW1Z4Vq01xeZR8RgEvktIRXufFZ9Lh1rNdtLsLN/UV6Y=",
                              Value: "ewogICJ0aW1lc3RhbXAiIDogMTY3OTk1MDI4NjQ3OSwKICAicHJvZmlsZUlkIiA6ICIzZjdlZGYyNGFmNzk0MTk3YWY1OGZjMzFlNGQ5NmU0MSIsCiAgInByb2ZpbGVOYW1lIiA6ICJPd2VuMTIxMjA1NSIsCiAgInNpZ25hdHVyZVJlcXVpcmVkIiA6IHRydWUsCiAgInRleHR1cmVzIiA6IHsKICAgICJTS0lOIiA6IHsKICAgICAgInVybCIgOiAiaHR0cDovL3RleHR1cmVzLm1pbmVjcmFmdC5uZXQvdGV4dHVyZS83OWY2OTc5NmU3N2U2ZmM0YWM0MWU0YTEyZmE1Mjc1YjcyNzlkMzZlM2I5NDI1NWVlYmI0NzgzNGZhNjJjYTZlIgogICAgfSwKICAgICJDQVBFIiA6IHsKICAgICAgInVybCIgOiAiaHR0cDovL3RleHR1cmVzLm1pbmVjcmFmdC5uZXQvdGV4dHVyZS9lN2RmZWExNmRjODNjOTdkZjAxYTEyZmFiYmQxMjE2MzU5YzBjZDBlYTQyZjk5OTliNmU5N2M1ODQ5NjNlOTgwIgogICAgfQogIH0KfQ=="
                          }
                      ]
                  }
              },
              display: {
                  Name: '{"text":"GET REAL!"}'
              }
          }
      }

What do you think of this? đź‘€

blazing obsidian
#

for yaml ? oh yea sure, I think SNBT would serve really well for us down the line

pallid kiln
#

oh god, base64 spam

wooden sail
#

isn't this what we already decided on, or did you just actually implement it now

wind wasp
#

Just implemented it

weak knoll
#

Guess ItemStack equality is related to this too:
if its changed, the isSimilar method definitely should be changed too and behave the same. Checking equality while ignoring the count is pretty important and it would be weird if it checks the rest of the NBT in a different way than equals()

wind wasp
#

I was thinking isSimilar would reflect the old equality behavior.

weak knoll
#

Maybe something like equals(boolean ignoreCount), that uses the new behaviour for the rest? Unless vanilla itemstack comparison ignores count, but I don't think it did last time I tried the /item command

wind wasp
#

I’ll have to check, there isn’t actually a default equality in item stacks, it’s just tag/count/type when needed in certain cases.

bleak fjord
#

Equality checker based on properties incoming. 🥳
It wouldn't be embedded into the itemstack's equals check instead be external.

formal wyvern
#

I'm playing around with this PR and one thing I noticed is that items created with new ItemStack(...) does not seem to support property operations. Is that something known/intentional? (seeing it's still wip)

wooden sail
#

yeah, I think it just hasn't been added to that itemstack. I think you use a method on UnsafeValues to create the stack for now

formal wyvern
#

Yeah, found Bukkit.getUnsafe().newItem(...) working, didn't took too long thanks to test plugin attached to this PR.

formal wyvern
# bleak fjord Equality checker based on properties incoming. 🥳 It wouldn't be embedded into t...

That's definitely something that current API is missing. How suitable something like that would be?

PropertyView.class

    // Objects are strictly compared by all properties
    boolean equals(final @Nullable PropertyView other)
    
    // Objects are compared only by specified properties (anything unspecified is simply ignored)
    boolean equals(final @Nullable PropertyView other, final @NotNull PropertyKey<?>... properties)

ItemStack.class

    // Objects are strictly compared by ItemStack#getType, ItemStack#getAmont and PropertyView#equals
    boolean equals(final @Nullable ItemStack other)
    
    // Objects are compared by ItemStack#getType, ItemStack#getAmount and PropertyView#equals(___, properties)
    boolean equals(final @Nullable ItemStack other, final @NotNull PropertyKey<?>... properties)

I'm not too sure if something like that should be defined as equals but can't think of a better name right now.

wooden sail
#

Yeah, I think we’ve talked about that. About being able to list properties to want equal on each.

#

Current api uses “isSimilar” for some comparison, maybe that’s a better name but idk the best name.

formal wyvern
#

I think using isSimilar would only make sense if on par with current Bukkit implementation. I don't remember what it looks like but I think it might not be as strict as comparison by all properties and their presence.

formal wyvern
wooden sail
#

So with the transition to each property mapping to a specific property, it would be good to also have a way to dynamically create properties specifically for stuff in the PDC. So if you wanted to access/change something in the PDC, you would just supply the namespaced key and type and get an item property back that could be used to access what you want

#

ofc you could still access the PersistentDataContainer itself with a separate property

#

that would also help with the custom equality checks, so you could just pass in several PDC PropertyKeys just like any other property check