Periodically changing 'LastExportTime' item data results in items not matching

A user of one of my plugins (Shopkeepers) reported an issue with items created by this plugin not being accepted in Shopkeeper trades after a while. The issue seems to be caused by this plugin periodically changing the custom items in player inventories to store a new different timestamp in their LastExportTime tag (this might involve a plugin reload, not sure). This difference in item data causes items to no longer match, resulting in the items no longer being accepted in Shopkeeper trades.

Would it be possible to avoid using this mechanism of storing periodically changing data inside of items?

Note that this issue is not limited to the context of Shopkeeper trades: Mismatching items can cause issues in any context in which Minecraft or some other plugin compares items by their NBT data. For instance, in vanilla Minecraft, for stackable types of items, players may encounter item stacking issues when they try to stack an item that had its internal timestamp changed with some items that did not have their timestamp updated (for example because they are stored inside a chest or other type of container, which is one of potentially many cases which the item updater of this plugin does not acocunt for). Another not covered case are custom (non-Shopkeeper) villager trades. And also the item on the cursor of an open inventory is currently ignored. There are probably countless more cases in which Minecraft stores items which this plugin does not account for currently. I also doubt that a plugin will ever be able to cover all of these possible cases, nor that this periodic scanning could be implemented efficiently. And even if this plugin would account for all vanilla cases, I doubt that it will be able to account for all cases in which some other plugin might store or reference an item stack and then under some circumstances compare it with item stacks of players that went through this periodic updating.

Hence the suggestion to somehow find a way for this plugin to work without this periodic changing item data (or ideally even without this periodic scanning and replacing of items in player inventories).

I haven't checked in detail why this item updating is even required, or what is stored in this BooleanRepresentation. But maybe it would be enough to store only the CustomItem name and then dynamically lookup any data for the CustomItem with this name, instead of encoding this data inside the items themselves.

Periodic updating

I will start by explaining what the periodic updating is for. Consider the case that an admin creates a custom item, for instance Ruby sword and gives it 12 attack damage. Some players play on the server and craft a Ruby sword. Later, the admin finds out that 12 attack damage is a bit too much and decreases it to 10 attack damage. This will cause all newly crafted Ruby swords to have 10 attack damage, but the players who crafted it before the 'patch' will still have a Ruby sword with 12 attack damage. Periodic updating ensures that the attack damage of existing swords will be decreased as well.

BooleanRepresentation

Periodic updating sounds easy, but has some complications. The easiest way would be to replace the entire ItemStack, but that would also erase some player-given properties like enchantments, lost durability, custom name, etc... In order to have nice periodic updating, the plug-in needs to know the properties of the custom item represented by the ItemStack at the time the ItemStack was created. The BooleanRepresentation stores the binary serialization of the custom item at creation time. The periodic updater will deserialize it and compare it with the current properties of the custom item, and uses that to determine which properties of the ItemStack should be altered, and how. (Handling enchantments is even more annoying because I need to distinguish the enchantments that belong to the custom item from the enchantments that were added by the player.)

Stacking

Since all custom items are actually tools, minecraft doesn't stack them anyway. The plug-in needs a lot of InventoryClickEvent work to make this happen.

Performance

The scan just scans player inventories, entity equipment, and open inventories; it does not scan every single chest in the world. Also, it happens only once every 5 seconds, not 20 times per second (if I really did this 20 times per second, it might cause a performance problem, but I'm not even sure about that).

Comparison problems

Comparisons (like for your plug-in ShopKeepers) is indeed a drawback of this system. If you have ideas on how to solve this problem, I would like to hear them! Currently, the best solution I can come up with, is keeping track of when the admin actually changes a custom item, and only updating these items in the world. This already happens to some extent, but not entirely (it also forces an update whenever the items are exported with a newer version of the configuration tool, which the admin can only avoid by not updating, which is suboptimal). You could also add special integration with this plug-in by treating my custom items differently: An ItemStack is a custom item from my plug-in if it has the nbt KnokkosCustomItems: { Name: player_given_name }. If two item stacks have the same player_given_name, you should consider them equal. If shop trades need to keep working after the admin actually changes one of the items, this is even the only solution I can think off.

Okay, so the plugin uses the BooleanRepresentation to figure out which vanilla item properties it is 'responsible' for. Another idea could be to store this history of past item versions somewhere else, instead of duplicating that data in every item instance ever created. Users of the plugin could either be asked to manage this 'item version history' themselves, or you could manage it for them. For example, the plugin could create an 'item-history' file inside its plugin folder that is not meant to be edited by users manually (this doesn't mean you need to make it unneccessarily complicated to manually read or even edit it: there might very well be situations in which a user might want to debug issues with this item updating, and therefore wants to investigate the content of this item history, or even edit it). On every plugin startup, the plugin would compare the current item data configured by users with the last version saved inside this history file: If the past item data is missing or does not match the currently configured item data, the plugin would copy the current item data to the history file and assign it an incremented item version number (which is stored in this history file alongside the item data).

The items of players would then only need to contain the item name and an item version. When checking if an item needs to be updated, instead of comparing timestamps, you could compare the stored item versions. You wouldn't even have to compare any item data (boolean representations) anymore, because you already did that during plugin startup when updating the history file. So this approach would:

Avoid the data duplication by no longer storing past item data in every item instance. Instead, only one copy of each item version is stored inside the history file.
Performance: Avoid item data comparisons for individual item instances. You only need to do these comparisons once during plugin startup, and can then limit it to compare version numbers for individual item instances.
Only modify items when it is required, i.e. when the item version number actually changed. The currently used timestamp is also incremented in cases in which the item data configured by the user did not actually change.
Make it a lot less obscure and easier to debug what is being stored inside the item instances: Tags with knokko-item-name and knokko-item-version are a lot easier to understand compared to arbitrary binary encoded item data.

Regarding performance: Even iterating over all the entities on the server is already relatively costly. And every forced inventory update slightly increases the network overhead, because the data of all the items in the player's inventory need to be encoded and resent. The impacts might not be big, but these things add up, especially with more plugins on the server, and higher player counts.

Another possible optimzation might be to disable the item updater if no item data changes have been detected during plugin startup.

But I have no good suggestion regarding how else a plugin could implement automatic item updating. If a plugin could hook into the server's ItemStack constructor / load-method logic, it would be possible to place all item migration logic there (similar to how for example the paper server performs some of its custom item migrations). I.e. each item stack would then only run through this migration once, instead of the plugin having to repeatedly scan player inventories etc. However, I have no idea if there is an easy way for a plugin to inject its item migration code there (other than maybe using some runtime bytecode modification library to inject the code needed for that.. which is not a good solution either). Also, the server might already be running when the plugin is loaded or enabled, so it might not be possible this way to catch the items that were already loaded before.

The only other possibilty I can currently think of would be to not automatically update items, but offer players the means to manually exchange or convert their items. For example via a command, or by using some shop plugin and trading the old item version for the newest item version. However, this would certainly be less convenient, and it would not resolve the use case in which a server admin wants to tweak item attributes that might previously have been to overpowered. But maybe this is sufficient in some cases, so a config option to allow server admins to disable the automatic item updating (maybe even disable it by default) would be nice. I.e. the goal would be to reduce the number of occurences / servers on which this automatic item updating would cause problems by default. And a config option to disable this would also allow server admins to come up and implement their own solutions to deal with item updates (like the ones mentioned above). You could still track the item history, even when the automatic item updating is enabled. Maybe prompt server admins with a notification when your plugin detected a change in item data, and then inform them about the option to enable automatic item updating. Server admins would then be able to consciously decide to enable the automatic item updating only when they actually want to use it, and only after they have been informed about the risks related to it ("not all items on the server might get reliably updated, one may end up with multiple incompatible item variants on the server, may result in incompatibilities with certain shop plugins that compare items by their exact data, etc."). Currently, server admins may unconsciously run into item mismatching issues due to this feature running in the background without them being aware of it, or its risks.

What I suggested here would still not fully resolve the item mismatching issue, because items can still end up with different data in some cases. However, by being able to disable this item updating (maybe even disable it by default), and by only modifying items when the configured item data has actually changed, this would already reduce the risk and number of cases in which servers would encounter this issue.

You could also add special integration with this plug-in by treating my custom items differently

Any kind of special integration is not an option for me. I also want to stick to Minecraft's own item comparison rules. (If I remember correctly, Minecraft even does some client-side predication on villager trades, which would result in hard to avoid glitches if the client uses different item comparison rules compared to the server).

Thank you for all the inspiration! I especially like the idea of letting the server admin choose whether he wants automatic item updating and I agree that it would be better if ItemStacks are only updated when the corresponding custom item actually changed.

But, I don't see why it matters whether I compare item version numbers or the timestamp when the item was last modified, since they come down to the same thing (both are numbers that only change when the item is modified).

Keeping track of the history of all custom items solves the BooleanRepresentation problem (like you said, storing it per ItemStack is not optimal), but it causes another problem: the history will grow each time the admin changes items, but it will never shrink (unless the admin does it manually). If many change are made, the history will also remember all the old custom item versions, even if no single ItemStack still uses it. Storing it per ItemStack avoids this problem since the data will be deleted whenever the ItemStack is destroyed. This results in more predictable memory usage: exactly 1 version of each custom item is stored per custom ItemStack. (I guess the history will still use less memory in most cases, but it is much harder to predict.)

Also, I don't believe bytecode modifications are better than periodic updating checks. Profiling data from running servers shows that this takes ~0.01% of the server tick time. And even if it does get out of hand, I could make the updating period longer. And how would iterating over all entities every 5 seconds be expensive? The server has to do this 20 times per second anyway to update them (and these updates are more expensive than just checking their equipment). Inventory updating only happens when the item updater actually changes an item, which is quite rare.

Finally, I respect your choice of not adding special integration (it's not nice indeed and could cause client-sync problems in your case).

I don't see why it matters whether I compare item version numbers or the timestamp when the item was last modified, since they come down to the same thing

Sure, as long as the timestamp only changes when the item data has actually changed, your timestamp would essentially act as 'version number' and would work fine for that purpose. But currently, the timestamp seems to be able to change even when the item data has not actually changed.

the history will grow each time the admin changes items, but it will never shrink (unless the admin does it manually) I wouldn't expect this to be much of a problem. I would expect that number of custom items on the server (regardless of its version) will usually (on the average server) be much greater than the number of custom item edits.

I expect this whole usecases of updating past versions of the same custom item to ideally be a rare special case (especially compared to the number of new item creations): At some point the custom item should be relatively stable. And for the reasons/issues outlined in this ticket, server admins should ideally be very conscious about making changes to already existing items on the server (at least if they don't want to run into item matching issues, and don't want to freshly setup their shopkeeper trades, and any other server setup that involves these items, everytime they have made a change to one of their custom items).

This results in more predictable memory usage

I would expect that the number of items on the server is actually a lot less predictable compared to the number of item edits: Custom items are often created dynamically on the server (eg. via loottables, triggers by players or entity events, etc.). These events may occur automatically and quite frequently. Whereas item edits are explicit actions taken by a server admin. So server admins are always very well aware of how often they edit their items.

Also, server admins can always check the file size of the history file to determine if this amount of data is something to be concerned about. Whereas the data stored inside items is hidden away in the rest of player data files and the world data, with no way for server admins to know how much of this data is due to duplicated item data.

But if this is a concern, it would be quite easy to add an option to automatically remove item versions that are older than some predefined time span (eg. older than a year or so). Or when the history file has reached a certain maximum size or number of entries. This cleanup could also be a manual action (eg. a command) that is proposed to server admins once one of these conditions is reached (old data, lots of data, etc.)

If the size of this history file really becomes a problem (I doubt it), there are even ways to reduce its size. You could for example optimize it to only store deltas from version to version. Or use a regular file compression algorithm to minimize its data. Or only load the item data into memory that is actually required (eg. only when you encounter an item with a specific name and version on the server).

Anyways, the main concern of my ticket was not memory and storage space optimizations, but avoiding item modifications as much as possible. This could be improved by these means:

Timestamp/item versions updates only when the item data has actually changed.
Automatic item updates being optional (config option to disable them, and ideally even have them disabled by default so that admins have to explicitly decide that they want to use this feature).

And how would iterating over all entities every 5 seconds be expensive?

I can only comment on what I have observed in the past while working on the Shopkeepers plugins: The plugin needs to periodically (every few ticks) execute actions for each shopkeeper, similar to Minecraft's own ticking. However, reducing the update rate and then instead doing more work per update has actually improved performance quite a bit. Also, I think iterating the loaded chunks was something else that turned out to be quite heavy. As well as reading the item data via the Bukkit API (by converting from Minecraft ItemStacks to Bukkit ItemStack and MetaData). But I haven't checked whether you are using the Bukkit API or direct NBT access to read the item NBT data.

But all of this might indeed not be a big issue for something that runs every few seconds, instead of every few ticks.

The important stuff is this:

 - Timestamp/item versions updates only when the item data has actually changed.
 - Automatic item updates being optional. I think I will just force admins to make a decision in the configuration tool.

Creating a controllable item history rather than storing boolean representation sounds like it could indeed improve storage (if that ever becomes an issue). But, this is a lower priority plan. (As long as the BooleanRepresentation doesn't change, its presence should not be a problem.)

Thanks for the time you put into giving this feedback!

knokko / custom-items-gradle