Item updater

[Huskydog9988]

Reopening #754 in preparation of custom item rework

[okx-code]

How is this class initialised?

[Huskydog9988]

Its really weird. The class is meant to be extended, by a migrator for an item, which then calls

Civ/plugins/civmodcore-paper/src/main/java/vg/civcraft/mc/civmodcore/inventory/items/custom/CustomItemsUpdater.java

Line 123 in d101628

public static <T extends CustomItemsUpdater> @NotNull T init(

itself, or something similar to that.

[Protonull]

Yup. Give #754 a read through. It started off as a really simple predicate-with-side-effects: it takes an item and returns a boolean of whether the item was modified or not. You'd do all your checks within that predicate, returning early wherever possible.

Then I added event handler classes which handle specific events and updating their respective items. For example, UpdatePlayerItemsOnJoin listens for player joins and then invokes the predicate for each item in the player's inventory and ender chest. UpdateInventoryItemsOnOpen listens for players opening chests (and other contains), updating its items. UpdateContainerItemsOnLoad listens for chunk loads and updates all the items in its container blocks.

package uk.protonull.civ.example;

import org.bukkit.inventory.ItemStack;
import org.jetbrains.annotations.NotNull;
import vg.civcraft.mc.civmodcore.ACivMod;
import vg.civcraft.mc.civmodcore.inventory.items.updater.listeners.UpdatePlayerItemsOnJoin;

public final class ExamplePlugin extends ACivMod {
    @Override
    public void onEnable() {
        super.onEnable();
        registerListener(new ItemUpdateListener());
    }

    private static final class ItemUpdateListener implements UpdatePlayerItemsOnJoin /** , UpdateInventoryItemsOnOpen */ {
        @Override
        public void updateItem(final @NotNull ItemStack item) {
            // if (item.needsUpdating()) {
            //     item.updateSomehow();
            // }
        }
    }
}

If you wanted your item-updater to run whenever a chest was opened, all you needed to do was uncomment that implements part. All the work was done for you.

However, Okx wanted more of a DataFixer-esque system. Take a look at net.minecraft.util.datafix.DataFixers for what that looks like. As mentioned here: instead of entirely replacing what was already built, I instead created the CustomItemUpdater as a layer of abstraction on-top of it. This meant there was a simpler system if you wanted it, and a complexer system if you needed it.

However, I do admit that the type shenanigans were a mistake. It seemed clever at the time but it's definitely a struggle to read, even for me now and I wrote it. Basically, since UpdatePlayerItemsOnJoin extends ItemUpdater but does not implement it, you can effectively combine/union them with an implementation of ItemUpdater via casting, eg:

package uk.protonull.civ.example;

import org.bukkit.inventory.ItemStack;
import org.jetbrains.annotations.NotNull;
import vg.civcraft.mc.civmodcore.ACivMod;
import vg.civcraft.mc.civmodcore.inventory.items.updater.listeners.UpdatePlayerItemsOnJoin;

public final class ExamplePlugin extends ACivMod {
    @Override
    public void onEnable() {
        super.onEnable();
        registerListener((UpdatePlayerItemsOnJoin) ExamplePlugin::updateItem);
    }

    private static void updateItem(final @NotNull ItemStack item) {
        // if (item.needsUpdating()) {
        //     item.updateSomehow();
        // }
    }
}

[okx-code]

is a first migration necessary?

[Huskydog9988]

My understanding that items like fossils are registered custom items, so they would need a migration like this to be put into the system essentially. The problem is I have no idea how they are targeted by a migration if they lack a custom item key. IIRC the migration code just looks for items with a custom item key?

[Protonull]

There are two purposes to the item updater system:

1. Provide a means to update legacy custom items (those that are "custom" by virtue of having names and/or lore) to be registered custom items.

2. Provide a means to update registered custom items through migrations.

The 0th migration just ensures that EVERY registered custom item has both a custom-item key and migration version, as stated explicitly in the comments. It establishes a baseline of what data can be accessed from the item without concern for NPEs. It also prevents stacking issues where one person's item has an explicit 0th migration-version set, and another whose 0th migration-version is implied by its absence (related: null != emptyList()).

The 0th migration is not strictly necessary but it does offer a benefit, "speculative" as that may be. That said, I maintain that the vast majority of this migration system would be utterly unnecessary if Okx simply agreed to permit ephemeral display data (ie, setting names and lore to items at network-time): this whole migration system exists to fix typos in lore, or to remove outdated parts of lore, etc. It's silly.

[Huskydog9988]

To clean up the api, and make the usage clearer, I made a demo implementation Huskydog9988#1

[Protonull]

I've been regularly coming back to this issue and honestly, the more I think about it, the more it seems like an over-engineered solution to a non-problem. Hear me out.

As mentioned in an earlier comment, there are two purposes to this item-updater system:

1. Update legacy custom items (those that are "custom" by virtue of having particular names and/or lore, eg: Player Essence) into registered CustomItems.

2. Update CustomItem data via migrations (eg: fixing a typo in the lore).

Though this also includes an unmentioned third:

3. Update items more generally when Minecraft/Bukkit internal behaviour changes causing existing/old items to stop stacking with newly-created items (Named/Lored items don't stack properly #510), which is why the first target of this system was the ItemMetaConverterHack.

But any system that officially supports CustomItems does not care about misspelt lore, nor new and old items not stacking: a FactoryMod recipe that accepts 32 Crude Oil as an input will accept 21 old items and 11 new items without complaint, because it doesn't care about the lore, only the CustomItem key. The only custom items that have this issue are legacy custom items, which can be immediately resolved by just updating them to CustomItems, which you don't need a full-blown migration system for.

You basically just need the original ItemUpdater interface with two if-statements, eg:

(ItemUpdater) (item) -> {
    boolean updated = false;
    if (isLegacyPlayerEssence(item)) {
        CustomItem.setKey("player_essence");
        updated = true;
    }
    if (CustomItem.isCustomItem(item, "player_essence")) {
        item.setItemName("Player Essence");
        item.setLore(List.of("Activity reward used to fuel pearls"));
        updated = true;
    }
    return updated;
}

It's that easy. Anything more complicated than this is dead on arrival, and probably why this PR (and it's predecessor) have a combined 56 weeks of being left open.

Embrace simplicity.

[Huskydog9988]

How does this compare to my demo implementation? From what I can gather, it handles them very similarly. The only difference I can surmise is that this doesn't explicitly call it a migration?

[Protonull]

How does this compare to my demo implementation?

Your demo still uses the same migration system: you have to register migrations to a corresponding number, and it'll run each migration in-turn depending on a custom version number placed on the item. My "embrace simplicity" suggestion is to ditch all that nonsense and just re-run the proverbial latest migration anytime you encounter a custom item or its legacy.

[Huskydog9988]

This doesn't handle items that aren't on the latest migration though right? Presumably there will be items missed at some point by a "latest" migration no?

[Protonull]

With my "embrace simplicity" solution, there's no concept of a migration: if your item matches either the legacy predicate or the custom-item predicate, your item will be updated. Finale has this system: there is no "migration" for the modified axe damage, it's just obsessively re-applied on predefined events (eg: opening a container), so each axe processed by Finale is, as it were, on the "latest migration".

[Huskydog9988]

I guess my worry is that the items will fall behind in updates and miss some critical upgrade like a change to the damage they do. I also slightly worried about overriding a custom name set on say a meteoric sword.

In terms of code quality I'm worried that the implicit upgrade path will be too detached that it's harder to reason about even if it's more ergonomic in the short term. I'm also slightly worried the upgrade function will balloon in size, but I think that would happen on either implementation.

[Protonull]

For the sake of argument, let's assume a maximalist example:

1. Legacy Player Essence needs to be updated to custom item player_essence
2. Set item name to "Player Essence"
3. The 0th line of lore needs to be changed from "Activity reward used to fuel pearls" to "Activity reward used to fuel pearls and factories"
4. Add glint
5. Add RARE rarity
6. Change RARE rarity to UNCOMMON
7. Change max-stack size to 99
8. Change max-stack size back to 64

This would look something like the following with migrations (pseudocode):

// This is being called within ExamplePlugin.onEnable()
final var migrations = new CustomItemUpdater("player_essence");
// It's actually not possible to do (1) because by definition of doing a custom-item migration, you must already be handling a custom item... which legacy custom items aren't.
migrations.registerMigration(2, (item) -> {
	item.setData(DataComponentTypes.ITEM_NAME, Component.text("Player Essence"));
});
migrations.registerMigration(3, (item) -> {
	ItemUtils.editMeta((lore) -> { // Some presumed edit-meta helper method
		lore.set(0, Component.text("Activity reward used to fuel pearls and factories"));
	});
});
migrations.registerMigration(4, (item) -> {
	item.setData(DataComponentTypes.ENCHANTMENT_GLINT_OVERRIDE, true);
});
migrations.registerMigration(5, (item) -> {
	item.setData(DataComponentTypes.RARITY, ItemRarity.RARE);
});
migrations.registerMigration(6, (item) -> {
	item.setData(DataComponentTypes.RARITY, ItemRarity.UNCOMMON);
});
migrations.registerMigration(7, (item) -> {
	item.setData(DataComponentTypes.MAX_STACK_SIZE, 99);
});
migrations.registerMigration(8, (item) -> {
	item.resetData(DataComponentTypes.MAX_STACK_SIZE);
});
CustomItemUpdater.init(this, migrations);

Whereas my "embrace simplicity" solution would look like this:

// This is being called within ExamplePlugin.onEnable()
registerListener((DefaultItemUpdaterListeners) ExamplePlugin::updateItem);

// This is a static method on ExamplePlugin
public static boolean updateItem(ItemStack item) {
	boolean updated = false;
	if (isLegacyPlayerEssence(item)) { // Some presumed predicate
		CustomItem.setKey("player_essence");
		ItemUtils.removeMatchingCustomName(item, "Player Essence"); // Some presumed helper
		updated = true;
	}
	if (updated || CustomItem.isCustomItem(item, "player_essence")) {
		item.setData(DataComponentTypes.ITEM_NAME, Component.text("Player Essence"));
		item.setData(DataComponentTypes.LORE, ItemLore.lore(List.of(
			Component.text("Activity reward used to fuel pearls and factories")
		)));
		item.setData(DataComponentTypes.ENCHANTMENT_GLINT_OVERRIDE, true);
		// Also overrides any pre-existing item with RARE rarity.
		item.setData(DataComponentTypes.RARITY, ItemRarity.UNCOMMON);
		// Resets any previous item that may still have 99 max-stack-size.
		item.resetData(DataComponentTypes.MAX_STACK_SIZE);
		updated = true;
	}
	return updated;
}

The migration solution may show intent more clearly, but it's also less powerful (such as by not being able to handle legacy custom items). It's also very tempting for people to write un-idiomatic migrations. For example, your demo migration (see embed)...

...doesn't do the minimum change necessary (only the 0th lore-line being changed), but instead replaces the entire lore. I'd wager that most migrations would end up doing that, because it's how people are used to editing items. Now, to be fair, my "embrace simplicity" solution also replaces the entire lore, but my solution is not pretending to be a series of small and incremental changes.

That said, both your demo migration and my "embrace simplicity" solution have unfortunate conflicts with compacted items (give that compacted items are solely marked as compacted via the presence of that lore line), which our item-updater code would remove. This is why I, yet again, call for item display data to be moved to ephemeral network items: the "Activity reward used to fuel pearls and factories" and "Compacted Item" lore lines are added to a cloned item at network time based on the presence of PDC tags, so that we can avoid this problem entirely.

[Huskydog9988]

Based on your provided example, I do think this is probably a better path forward. I'd prefer those static methods be attached to the custom item classes I gave in my demo, just to consolidate everything related to the items in one place. Aside from that I do think you make a good case for migrations not being necessary.

On the topic of compacted items, I'm torn as I can't think of any solution that doesn't have serious tradeoffs. Networked items require a massive jump in complexity, alongside possible bottlenecks in packet transmission. (I assume we'd need some hook in the network stack, and I'd prefer we avoid playing with the fire that is civ code next to an oil soaked rag.) If we try to wrap compacted items in a bundle or something to make it a separate custom item, the contents are not as clearly visible as they are rn. (Say there is compacted cobble, the item players would see is a bundle, and would require them to hover their mouse to see its actual value. This isn't ideal obviously.) My best thought is to just add a pdc to denoted a compacted item. Its close to the existing system so it shouldn't be that horrible? It would also easily allow for the compacted item to be upgraded without removing that compacted item status.

[Protonull]

I'd prefer those static methods be attached to the custom item classes

Valid

Networked items require a massive jump in complexity

To be fair, Civ already does meddle with network items (eg: AttrHider) but the issue is more, as you suggest, trying to ensure that every type of item transmission is intercepted, which can change from version to version, and also that every kind of item with constituent items (eg: bundles / shulkers) are also handled correctly. This is not insurmountable, but it would be tedious.

I think, perhaps, we should do a performance test of ephemeral network-item data before ruling it out: I've been a serial fretter of how decadently inefficient Civ's item-editing code is, and yet I don't believe it's ever been a problem in spark (or the other profilers).

My best thought is to just add a pdc to denoted a compacted item

That was my proposed solution with #398, but you still have the problem of how you then convey to the player that it's a compacted item, ie, you still need to keep the lore there. The rub is that, when I posted the PR, I had moved the compact-item code to CivModCore because of how fundamental it is (like CustomItem), but SoundTech instructed me to move it back to FactoryMod under the assertion that that's its domain. This means you can't really have CustomItems be considerate of compacted items without: a) creating a bunch of [imo unnecessary] abstraction, or b) having CMC depend on FM. Perhaps the code can be moved back to CMC now that the composition of the admin team has changed? Either way, it's something to keep in mind.

Assuming that compacted items would remain in FactoryMod, we could take advantage of events (or a similar system), eg:

1. Open-inventory event is fired.
2. The inventory is iterated over.
3. Every non-air item fires a ResolveCivItem event.
4. CustomItem and CompactedItem register handlers for this event and add the relevant PDC tag if they detect a legacy item of their type.
5. Every non-air item then fires a UpdateCivItem event.
6. CustomItem and CompactedItem register handlers for this event too, which fiddle with the item's data.

This effectively reimplements my "embrace simplicity" solution, but through indirection via events. I think any solution we come up for this will be necessarily more complex, less readable, etc, than ephemeral network-item data, though perhaps you may have a better idea than me :P

[Huskydog9988]

Firstly, I'm just gonna put my foot down and say compacted items do need to be defined in cmc. Other plugins need to be able to understand what that is, and requiring a dependency on fm for that is a stretch imo. I'd avoid making the creation of compacted items in fm, but the definition of what one is a no brainier imo.

Second, I do think network testing is a must, especially if networked items is to be actually considered. Personally I'd rather we try your suggested migration implementation first though before we head down the networked route. (Though this is all still pending @okx-code's opinion of course.)

[Protonull]

Yeah, Okx didn't seem to have an issue with it being in CivModCore, though iirc the conversation about FactoryMod's ownership of compacted items took place in #dev-nook, so I cannot say for certain either way. We could try two competing strategies and test them for their ergonomics and performance? Though how would you propose to test the network-item approach? Spin it up on CivTest and ask people to stress test?

[Protonull]

Another approach could be an official data pack 👉👈