add ItemFrameMeta API #11271
add ItemFrameMeta API #11271
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| + Material.GLOW_ITEM_FRAME | ||
| + ); | ||
| + | ||
| + static final ItemMetaKeyType<CustomData> ENTITY_TAG = new ItemMetaKeyType<>(DataComponents.ENTITY_DATA, "EntityTag", "entity-tag"); |
There was a problem hiding this comment.
Is NBT supposed to be entity-tag instead of EntityTag?
There was a problem hiding this comment.
entity-tag for both, so you can remove that param
TehBrian
force-pushed
the
feat/item-frame-meta
branch
from
7d0dac7
to
435b242
Compare
| +@DelegateDeserialization(SerializableMeta.class) | ||
| +public class CraftMetaItemFrame extends CraftMetaItem implements ItemFrameMeta { | ||
| + | ||
| + private static final Set<Material> ENTITY_TAGGABLE_MATERIALS = Sets.newHashSet( |
| + @Override | ||
| + void serializeInternal(Map<String, Tag> internalTags) { | ||
| + // correctly serialised as entity tag. | ||
| + } |
There was a problem hiding this comment.
Just don't override this at all.
| + SerializableMeta.getObjectOptionally(Boolean.class, map, FIXED.BUKKIT, true).ifPresent((value) -> { | ||
| + populateTagIfNull(); | ||
| + this.entityTag.putBoolean(FIXED.NBT, value); | ||
| + }); | ||
| + SerializableMeta.getObjectOptionally(Boolean.class, map, INVISIBLE.BUKKIT, true).ifPresent((value) -> { | ||
| + populateTagIfNull(); | ||
| + this.entityTag.putBoolean(INVISIBLE.NBT, value); | ||
| + }); |
There was a problem hiding this comment.
None of this is needed. We are always serializing it as a full nbt tag instead of splitting it up. The only reason ArmorStand has similar logic is were weren't always doing that. But since this is brand new, we can do this.
| + try { | ||
| + this.entityTag = NbtIo.readCompressed(buf, NbtAccounter.unlimitedHeap()); | ||
| + } catch (IOException ex) { | ||
| + Logger.getLogger(CraftMetaItem.class.getName()).log(Level.SEVERE, null, ex); |
There was a problem hiding this comment.
Lets make this an slf4j logger in a private static field like LogUtils.getClassLogger()
| + | ||
| +import org.bukkit.inventory.meta.ItemMeta; | ||
| + | ||
| +public interface ItemFrameMeta extends ItemMeta { |
There was a problem hiding this comment.
Are we making the same mistake here lots of other ItemMetas have done? technically there are 3 states for every boolean. It could be true, false, or not present in the custom data.
| + if (this.entityTag != null) { | ||
| + tag.put(CraftMetaItemFrame.ENTITY_TAG, CustomData.of(this.entityTag)); | ||
| + } |
There was a problem hiding this comment.
We probably need to do a sanity check here to make sure the custom data compound, if its not empty, that it has the "id" key/value in it. If it doesn't and its not empty, it'll throw errors all over the place as the codec requires it.
Draft PR; CraftMetaItemFrame is (probably) currently non-functional, but I wanted to share progress so far.The second commit is a rewrite based on #11107,
and I'm waiting on that PR to finalize changes here.Concerns:
Please let me know your thoughts :D