[XItemStack] - Add functionality to edit ItemStack / ItemMeta

[bestlinuxgamers]

Description

To modify ItemStacks independently of the Minecraft version, it is necessary to reimplement the deserialization functionality of the XItemStack class. This often results in "ItemBuilder"s that either fail to support all features or do not work across most versions. However, the required code already kinda exists - it just cannot be directly used for this purpose.

I think diversifying the (de)serialization logic of XItemStack would be a great idea. Instead of storing an ItemStack directly in a YAML-config, it should be stored within a Java object, which can then be serialized into and deserialized from YAML. But can also be created from a Bukkit-ItemStack and create a Bukkit-ItemStack (with its properties). This approach would also provide greater flexibility for potential new serialization formats such as Gson/JSON. If we would add functionality for modifying the attributes of this intermediary object, we would essentially get the perfect ItemBuilder.

Of course I know that this would be a significant change, requiring a complete restructuring of the XItemStack code. However, I believe this functionality would be universally useful for almost every Minecraft developer.

I would be willing to implement these changes myself, but I first wanted to check for interest and see if such a modification would even be accepted.

If so, I would like to discuss the concrete implementation of this. I have thought of two possible implementation approaches:

1. The attributes of an ItemStack are stored as plain global variables. Maybe wrapped with Optional<> to identify if an attribute is set or uninitialized. While values for specific child classes of ItemMeta would have their own XItemMeta class to store, load, apply all relevant data (or ItemMeta-specific attributes are just being saved to the global variables of XItemStack too).
   (pros: Simple to implement, minimal overhead)
   (cons: Might become difficult to manage, potential clutter)

2. Each attribute receives its own class, providing two functions: void from(ItemStack item) and void to(ItemStack item). The XItemStack would then consist of a list of these objects, each responsible for storing the necessary values for a specific attribute.
   (pros: Easier to extend/edit with new Mc behaviour, clean separation of concerns)
   (cons: Potential performance overhead, more complex)

Would love to hear your thoughts on this!

[CryptoMorin]

Just to make sure, the summary of this post is: Add a way to create/edit hardcoded items that don't depend on YAML?

I personally never had any use for this feature since I let my users configure every single item, but if you're willing to work on this, I'll be happy to help you get this done :)

So first of all, I think we should just keep XItemStack how it is, and add a builder() method to XItemStack that returns a separate XItemBuilder class. XItemStack is already bloated as it is, I don't want to make it worse.
I don't think converting ItemStack -> XItemBuilder will help in serialization/deserialization. I recently implemented a ItemMeta hierarchy serializer which makes handling new versions easier. You can simply convert the ItemStack to XItemBuilder after the deserialization if you want.

As for the implementation... If by "Global" variables you mean public fields, they are just not going to be compatible with the builder pattern. We want to have something similar to ParticleDisplay where you can chain withParticle().withColor()... so e.g. withMaterial().withName()...
Can't we just make a builder class that delegates a real ItemStack?

public class XItemBuilder {
  private final ItemStack item;
  private final ItemMeta meta;

  public XItemBuilder withMaterial(XMaterial mat) { item.setType(mat.get()); ... }
  public XItemBuilder withCustomModelData(int data) { meta.setCustomModelData(data); }
  ...

  public ItemStack build() { return item; }
}

I would understand if you wanted some kind of template builder class that can be "applied" to multiple ItemStacks. In that case, yes I think we can just create a class for each "property"

public class XItemBuilder {
  private static final Map<Class<? extends Property>, Supplier<Property>> PROPERTIES_REGISTRY = new IdentityHashMap<>();
  private final Map<Class<? extends Property>, Property> properties = new IdentityHashMap<>();

  private XItemBuilder property(Property property) {
    properties.put(property.getClass(), property);
    return this;
  }

  public XItemBuilder withMaterial(XMaterial mat) { return property(new Material(mat)); }
  public XItemBuilder withCustomModelData(int data) { return property(new CustomModelData(data)); }

  public <T extends Property> Optional<T> get(Class<T> propertyType) {
    return Optional.ofNullable((T) properties.get(propertyType))
  }

  public ItemStack build() {
    ItemStack item = new ItemStack();
    for (Property prop : properties.values()) {
      prop.to(item);
    }
    return item;
  }

  // It wouldnt make sense to implement Property for this class, but we can use these methods
  public void to(ItemStack item) {simply loop properties and to()}
  public void from(ItemStack item) {register empty constructors to PROPERTIES_REGISTRY?}

  public interface Property {
     void to(ItemStack item);
     void from(ItemStack item);
  }

  public static final class Material implements Property {
    private XMaterial material;
    public Material(XMaterial material) {this.material = material;}
    public void to(ItemStack item) {item.setType(...);}
    public void from(ItemStack item) {this.material = ...;}
  }
}

The only way to fix the overhead of the hashmap would be to spread these to their own fields which makes this much more messy or assigning a hardcoded ID to each property class and using a fixed array instead but that means more RAM usage. I don't think it's worth it.

To make this cleaner I would force IntelliJ's @formatter:off to use single lines for property classes (unless the method implementation is more than 1 line) and no new line between methods, otherwise it's going to be too cramped like XItemStack.

Thoughts?

[bestlinuxgamers]

Thank you for your quick response and valuable help! :)

Just to make sure, the summary of this post is: Add a way to create/edit hardcoded items that don't depend on YAML?

Yes, that description sums it up very well.

So first of all, I think we should just keep XItemStack how it is, and add a builder() method [...]. I don't think converting ItemStack -> XItemBuilder will help in serialization/deserialization. [...]

I completely agree that this change won’t be useful for serialization/deserialization. My main concern was that XItemStack and XItemBuilder would otherwise have a lot of duplicate code since both classes create and modify ItemStacks independent of the Minecraft version. I hadn’t considered that the YAML serializer could simply use XItemBuilder to avoid duplicated code.

I recently implemented a ItemMeta hierarchy serializer which makes handling new versions easier. You can simply convert the ItemStack to XItemBuilder after the deserialization if you want.

I saw that in the code (you're referring to XItemStack#recursiveMetaHandle, right?). However, I’m not entirely sure how to use it within XItemBuilder. In general, I’m unsure about what would be the best approach for handling all different ItemMeta types.

Can't we just make a builder class that delegates a real ItemStack?

Now that I think about it, that should be enough for my usecase. Originally, I structured it this way so that XItemBuilder could be serialized, but since that’s no longer necessary, a simpler approach should work just fine.

The remaining question is how to handle properties of different ItemMeta types.
Should we just add methods for each meta type directly in XItemBuilder? That might quickly become messy due to the number of different properties:

public class XItemBuilder {
  private final ItemStack item;
  private final ItemMeta meta;

  public XItemBuilder withMaterial(XMaterial mat) { item.setType(mat.get()); ... }
  [...]
  public XItemBuilder withBookTitle(String title) {
    if (!(meta instanceof BookMeta)) return;
    BookMeta bookMeta = (BookMeta) meta;
    
    bookMeta.setTitle(title);
  }
  [...]
  public XItemBuilder withSpawnerType(EntityType type) {...}
  [...]
}

Or should we create a separate builder/modifier class for each ItemMeta subtype? Maybe only for metas with multiple properties.

public class XItemBuilder {
  private final ItemStack item;
  private final ItemMeta meta;

  public XItemBuilder withMaterial(XMaterial mat) { item.setType(mat.get()); ... }
  [...]
  public XItemBuilder wthBookMeta(Consumer<XBookMetaModifier> bookAction) {
    if (!(meta instanceof BookMeta)) return;
    BookMeta bookMeta = (BookMeta) meta;
    
    bookAction.accept(new XBookMetaModifier(bookMeta));
    return this;
  }
  [...]
  public XItemBuilder withSpawnerType(EntityType type) {...}
  [...]
}

public class XBookMetaModifier {
  private final BookMeta meta;
  
  public XBookMetaModifier(BookMeta meta) { this.meta = meta; }
  
  public XBookMetaBuilder withBookTitle(String title) { meta.setTitle(title); ... }
  [...]

}

---

My second approach with dedicated Property (from, to) classes was mainly intended to improve maintainability and scalability. The question is whether the additional complexity is worth it. I really like your approach on how to implement this.

The only way to fix the overhead of the hashmap would be to spread these to their own fields which makes this much more messy or assigning a hardcoded ID to each property class and using a fixed array instead but that means more RAM usage. I don't think it's worth it.

Yes, I completely agree with you on that.

[bestlinuxgamers]

To support multiple ItemMetas with the second approach, we could also add the following:

    public interface MetaProperty<T extends ItemMeta> extends Property {
        T getItemMeta(ItemStack item);

        default void to(ItemStack item) {
            T meta = getItemMeta(item);
            if (meta == null) return;
            to(meta);
            item.setItemMeta(meta);
        }

        void to(T meta);

        default void from(ItemStack item) {
            T meta = getItemMeta(item);
            if (meta == null) return;

            from(meta);
        }

        void from(T meta);
    }

    public static final class BookAuthor implements MetaProperty<BookMeta> {
        private String bookAuthor;

        public BookAuthor(String bookAuthor) { this.bookAuthor = bookAuthor; }

        @Override
        public BookMeta getItemMeta(final ItemStack item) {
            ItemMeta meta = item.getItemMeta();
            if(!(meta instanceof BookMeta)) return null;
            return (BookMeta) item.getItemMeta();
        }

        @Override
        public void to(final BookMeta meta) {
            meta.setAuthor(bookAuthor);
        }

        @Override
        public void from(final BookMeta meta) {
            if(!meta.hasAuthor()) return;
            bookAuthor = meta.getAuthor();
        }
    }

We could also consider caching T getItemMeta(ItemStack item) somehow, so that we don’t have to perform a type cast for every property.

[CryptoMorin]

Duplicate code between XItemStack and XItemBuilder would be really rare and it'd be mostly because of compatibility if any. I just mentioned XItemStack#recursiveMetaHandle to point that even if duplicates due to incompatibility arise, this entire system is taking care of it, but yes it's not usable for XItemBuilder. I also want to keep both classes independent from each other so people could exclude it if they don't need to use both.

Anyway, I think we should just follow XSkull's Profileable system and keep the XItemBuilder properties independent objects that can perform transformations on real objects (ItemStack and ItemMeta) instead of storing ItemStack/ItemMeta. This way we basically have our own ItemStack/ItemMeta classes which has maximum level of abstraction. So basically going with the private final Map<Class<? extends Property>, Property> properties route.

That being said, the current Property class I wrote can only transform ItemStack. Now this will work even for ItemMeta properties, but the problem will be excessive overhead of getting/setting ItemMeta from/to an ItemStack which is what you're showing. To fix this we could simply change the signature to from/to(ItemStack,ItemMeta) and to abstract out setItemMeta()/getItemMeta() we can do this:

    interface Property {
        void to(ItemStack item, ItemMeta meta)
        void from(ItemStack item, ItemMeta meta)
        boolean affectsMeta()
    }
    abstract MetaProperty<T extends ItemMeta> extends Property {
        public final void to(ItemStack item, ItemMeta meta) { to(item, (T) meta) }
        public final void from(ItemStack item, ItemMeta meta) { from(item, (T) meta) }
        public final boolean affectsMeta() { return true }

        void to(T meta);
        void from(T meta);
    }
    class BookMeta implements MetaProperty<BookMeta> {
        void to(BookMeta meta) { ... }
        void from(BookMeta meta) { ... }
    }

Now in XItemBuilder's from/to method we simply check which property affectsMeta() and we store the ItemMeta for later iteration of properties on the first encounter of a property that requires it. And if it's a to(...) we simply setItemMeta() at the end. Almost zero-overhead. And to make things more simple for properties that don't affect ItemMeta:

  abstract class SimpleProperty implements Property {
        public final to(ItemStack item, ItemMeta meta) { to(item, null) }
        public final void from(ItemStack item, ItemMeta meta) { from(item, null) }
        public final boolean affectsMeta() { return false }

        void to(ItemStack item);
        void from(ItemStack item);
  }
  class Material implements SimpleProperty {
        private XMaterial material;
        public Material(XMaterial material) {this.material = material;}
        public void to(ItemStack item) {item.setType(...);}
        public void from(ItemStack item) {this.material = ...;}
  }

Pretty clean right?

Now how to handle compatibility, it's pretty simple:

interface Property {
    default boolean isSupported() { return true }
}

If this requires complex calculations like checking if a class exists using reflection, we implement isSupported() with a private static final boolean inside that specific property which calculates this once and returns the static boolean. We simply check isSupported() on ItemBuilder's to/from() method and skip if it's not supported.

[bestlinuxgamers]

I agree with most of this and will start implementing everything like this soon.

The only issue is that we can’t just cast the ItemMeta to T, as this would throw an exception if the user tries to set a property that isn't supported. We also can't simply check meta instanceof T because of Java limitations.
So we would need the children of MetaProperty to at least do the type check (maybe even the cast to tame Intellij's unchecked cast warning).

public interface MetaProperty<T extends ItemMeta> extends Property {
    T castMeta(ItemMeta item);

    default void to(ItemStack item, ItemMeta meta) {
        T specificMeta = castMeta(meta);
        if (specificMeta == null) return;

        to(specificMeta);
    }
    ...
}

Alternatively, we could use the class of the ItemMeta subtype to do the type check:

public interface MetaProperty<T extends ItemMeta> extends Property {
    Class<T> getMetaClass();

    default void to(ItemStack item, ItemMeta meta) {
        Class<T> metaClass = getMetaClass();
        if (!metaClass.isInstance(meta)) return;
        T specificMeta = metaClass.cast(meta);
        to(specificMeta);
    }
    ...
}

public static final class BookAuthor implements MetaProperty<BookMeta> {

    @Override
    public Class<BookMeta> getMetaClass() {
        return BookMeta.class;
    }
    ...
}

What do you prefer?

[bestlinuxgamers]

I created a very early version of this: bestlinuxgamers@45f41d2
It would be great if you could take a rough look at it. The TODOs are particularly important.

The biggest change is the LambdaProperty<T>, which should significantly reduce the size of the source file by moving from() and to() inside of the constructor:

public static final class Amount extends LambdaProperty<Integer> {
    public AmountAlternative(final Integer amount) {
        super(amount, ItemStack::setAmount, ItemStack::getAmount);
    }
}

I also tried to split the handling of a property across multiple versions into separate properties, which are then combined to one. With hindsight, I don't think that's a good idea.

public static class DurabilityAlternative extends PropertyCollection {
    public DurabilityAlternative(int durability) {
        super(new NewDurability(durability), new LegacyDurability(durability));
    }
}

public static class NewDurability implements MetaProperty<Damageable> {
    ...
    @Override
    public boolean isSupported() {
        return supports(13);
    }
    ...
}


public static class LegacyDurability implements MetaProperty<Damageable> {
    ...
    @Override
    public boolean isSupported() {
        return !supports(13);
    }
    ...
}

Furthermore I don't think we should treat the Material as a property. Rather it should be passed to the constructor, as it is always required.
This also requires making XItemBuilder#from(ItemStack) static and return an XItemBuilder instance.

[CryptoMorin]

I really like your LambdaProperty solution since it uses method references that makes this almost zero-overhead.

Furthermore I don't think we should treat the Material as a property. Rather it should be passed to the constructor, as it is always required.

Yes, but you can always change the material later. Also, these item builders should be able to merge together. So a material shouldn't be always required. We should have a simple:

class XItemBuilder {
    public void from(XItemBuilder builder, boolean override)
}

this simply just copies the properties. But the build() method should check if there is a material set and throw an exception.

---

Add private empty constructor to every Property

This makes the code a bit bulkier. Yes, developers shouldn't make instances of property classes in most cases, but we should keep it available just in case. I don't think we need to do this. But you should make a single method that handles PROPERTIES_REGISTRY.put calls

private static <T extends Property> void register(Class<T>, Supplier<T> ctor)

---

Maybe make static and return XItemBuilder instance with XMaterial passed to constructor.

I agree, we should add a separate static from method, but keep this one.

---

PROPERTIES_REGISTRY.forEach()

This is a bit less efficient than the traditional for-each loop. It's not that important, but I prefer to keep it that way.

---

Only add if currentProperty was set (maybe add boolean return to Property#from())

You should add an option for that.

class XItemBuilder {
    public XItemBuilder from(ItemStack item, boolean override)
}

---

Other things:

• metaClass.isInstance() should throw an exception instead of simply returning. This is error-prone and the responsibility of the developer to know which meta is for what item.
• We should make a copy() method for XItemBuilder, which means adding a copy method to the properties as well.
• You should make every method a single line when the method body is only a single line. This saves a lot of space. You can do this inside IntelliJ using // @formatter:off like I said. Add this at the beginning of the first property and close it at the last property class. And also don't put spaces between those methods. You should also make the @Override annotation inline. Also don't use final for the parameters, it's pointless in most cases. E.g. for Amount property:

      public static final class Amount implements SimpleProperty {
        private int amount;
        public Amount(int amount) {this.amount = amount;}
  
        @Override public void to(ItemStack item) {item.setAmount(amount); }
        @Override public void from(ItemStack item) {this.amount = item.getAmount();}
    }

• We shouldn't have two classes like LegacyDurability and NewDurability if the intended purpose of the developer is to literally set the durability of the item, we should merge this to a single class and choose the appropriate way to handle this based on the version.

You should make the next code a PR so we can talk on that instead.

[bestlinuxgamers]

Let's continue in #338 :)