Custom Item Data

NathanWolf - In PortableHorses I trim my string values to 32767 chars. I dont think there is a limit on individual string lengths, only on the overall packet size, which I'm not entirely sure of.

For what it's worth, the reason I couldn't just store a Primary Key and keep my data elsewhere is because its actually impossible to completely keep track of all the ways an ItemStack can disappear from the game. You could toss your PortableHorse saddle into the void and the plugin would have no way to know about that, and no way to delete the associated database entry.

 

I know, all too well- that's exactly what this thread is about- finding a good way to store data in an item that will persist throughout its lifecycle.

I know I type a lot, and this thread has gotten lengthy, but if you want to scan this post:

http://forums.bukkit.org/threads/custom-item-data.265410/#post-2480520

And maybe check out my code, it may be something you're interested in. I'm trying to put together a PR for an official solution, and probably API, for custom item data storage, and would love any feedback you have.

 

I love the idea, but you should run it by a few of the higher-up people with commit privileges, I feel like this has been suggested before and rejected, anything that writes or reads custom NBT tags afaik won't be accepted into bukkit. (something to do with NBT being an implementation detail, which is subject to change or disappear with any new minecraft release). I could be completely wrong, and I hope that I am because I'd like to see something like this make its way into bukkit, but I have doubts it will be accepted.

 

I feel that way, too. But what hasn't been rejected is the general idea of custom data storage.

So I'm approaching it two separate ways:

1. I want to internally make it so that any unknown NBT data is still stored, persisted, and serialized.
2. At an API level, I want to add a custom data storage mechanism based on ConfigurationSection. The implementation details should not be important to Plugins, but the underlying storage mechanism will be NBT until a future date when it goes away, and we have to migrate everyone's data....

I'm nearly ready to try for a PR. I'd appreciate feedback from anyone willing to look over my code. I have two branches now, I think I will combine them for the PR, it'll make testing easier and they don't merge well.

Implementation for BUKKIT-3221
https://github.com/NathanWolf/CraftBukkit-Bleeding/commit/ec0685b97fd916b0f67638d72794282cee30159f
https://github.com/NathanWolf/CraftBukkit-Bleeding/commit/7f682a6643495c2cb4cbd0f576674be9020c3bf3
https://github.com/NathanWolf/Bukkit/commit/c2c9c65961d2618e7d7391879952e145022c36e3

Implementation for BUKKIT-4767, item glow effect API:
https://github.com/NathanWolf/CraftBukkit-Bleeding/commit/e73f0556c098b4106bf13a7947d64f994b565d17
https://github.com/NathanWolf/Bukkit/commit/f0b5fd3f1cb8aee0f19ccfa7057034474a398b26

And if you just want a high-level view of using the new API, here's my test plugin:

https://github.com/NathanWolf/Test-...ukkit/plugins/test/TestItemMetaData.java#L156

EDIT by Moderator: merged posts, please use the edit button instead of double posting.

 

NathanWolf Nice - this would certainly make my life simpler. Couple of comments based on a quick initial view:

1. hasCustomData() and getCustomData() might be better API names? More descriptive.
2. I know the dev team prefer a model of "get data" -> "modify data" -> "re-apply data" when changing the properties of objects in the world (see the ItemMeta system in general). You might meet some opposition to having changes in the ConfigurationSection immediately reflected in the item's internal data.
3. I wonder if ItemStack stack should implement Metadatable (not saying it definitely should, but have you considered it?) - since Metadatable is already used for storing custom data on other things.
4. Has the glow system been comprehensively tested with adding/removing enchantments? E.g. if you set an item glowing, add enchantments, then remove all enchantments, does it stay glowing?
5. Make sure your Javadoc lines are not more than 72 chars wide. Use <p> markers between paragraphs in Javadocs.

 

NathanWolf, how about raising an exception in the event that someone attempts to store a non-nbt-serializable object? It seems to just silently fail in that case.

 

Wow, thanks for the feedback, guys!

That could work- I originally had "ExtraData". I like "Custom" - but I also had originally split this change into "save the data" versus "allow custom data", which is why I made it so generic. I guess that's a moot point though.

I know, I had a pretty big debate about this myself. Here's the reasoning I ended up at, please let me know what you think>

• This is a sub-object of ItemMeta, which itself doesn't get applied until you call setItemMeta on the ItemStack. So all you're really modifying "live" is a copy of a data object that does nothing on its own.
• It would be very expensive to copy the data structure back and forth since it's an arbitrarily large tree. Think of your plugin wanting to just check for a specific key to see if an item is special in your eyes- but maybe my plugin has stored 1,000 keys in that item (for whatever reason)- you now have to make a copy of all that data just so the caller can check for the presence of one key.

I originally was going to go the get/set route. Then on thinking of the performance implications, I thought "well what if we only allow access to one root node at a time?" or "what if we also provide a simple set of basic root operations like "hasKey", "getInt", etc. And then I realized, "Hey, that's just the ConfigurationSection interface all over again..."
So hopefully you can kind of see how I got here, and hopefully the dev team will, too. Unless there's a better way that's not so wildly inefficient

Interesting...I could do this (and use the custom data store to back it), but you can't store just anything in here like you can with other Metadatables, so I'd probably want to throw an exception on something non-serializeable (like metalhedd suggested- though I guess I thought MemorySection should be handling that for me?)

I have so far only been testing with the basic native type accessors, I suppose I should add Serializeable and non-serliaeable Object storage to my test case.

Anyway, my main hesitation here would be confusion. Normally you can throw any Object you want in a Metadatable store, and that wouldn't work out here.

I don't know about "comprehensively" yet, but it will be before I hand it over. My test plugin lets you do some basic stuff like glow/unglow/unenchant items.

That said, there is some strange behavior here with enchants that I'm not sure how to get away from-other than maybe using my own custom data system to store a "glowing" flag (I'm not currently doing that).

The glow thing was kind of an add-on, and its admittedly very simple. In fact, this case you mentioned, I'm now worried this represents a behavior change. The answer to your question is "yes"- it will stay glowing. But unfortunately that's also the answer without the first part of your question. If you add enchantments to an item, and then remove the enchantments- it stays glowing. I think I need to fix it, but not sure how without also losing the glow in the case you provided, or making this all a bit more complex. Hmmm.. derp. Well thanks for bringing this up!

Thank you! I missed the "<p>" thing and I need to go back and clean that up- I'll check comment line widths while I'm in there, too. I think the docs need some cleanup in general, though it doesn't seem like javadocs in the CB code are the norm anyway

If the underlying MemoryConfiguration doesn't do this for me already, then I think that's a great idea. I'm also still debating throwing some hard size limits in, which would trigger exceptions. In general there are a few places where it silently does nothing and probably should not.

For the size limits thing, I want to spend some time getting familiar with how PortableHorses does its thing (or, rather, how ProtocolLib is internally doing it). I do think if there's an easy and efficient way I could sanitize the item data (remove all custom tags) before sending to the client, that'd be really ideal.

I revised the code and the answer to this question- which is now "no".

The "glow" stuff is secondary to me- I thought about it, and don't want to do anything complex to bog this overall change down. I like having the glow API as part of this because it touches similar code and testing overlaps (you'd want to test the two changes in conjunction anyway).

But I also understand Bukkit's hesitation about the glow API- it truly does rely on an implementation detail out of our control. If Mojang ever updated the client to ignore empty enchant lists when adding a glow effect to an item, this API would break irreparably.

That said, as long as we have the functionality, it'd be nice to use it. But my preference is to give the Enchantment API priority, Glow being secondary. So here's the behavior I've decided on:

• You can add and remove glow to an item with no enchantments
• You can not remove glow from an item with enchantments
• If you remove all enchantments from an item, the glow is also removed (regardless of if a glow was added artificially)
• There is still no removeAllEnchantments or anything, so the above would have to be a pretty deliberate action.

Internally, some details that tie this together:

• hasEnchants still looks for a non-empty or null enchant list
• hasGlowEffect checks for only a non-null enchant list
• setGlowEffect(false) checks for a non-empty enchant list before nulling it out
• removeEnchantment will null out the enchant list if it actually removed an enchant and the list is now empty

I think that gives us access to everything we can easily/unhackily provide given the way Minecraft currently handles item glow, and without changing any of the current enchantment functionality.

If this glow stuff gets too complex I will pull it out and make a separate PR, but I do think that adds some additional testing burden, assuming the dev team wants to accept both changes. My main focus is the custom data API.

Updated branches with some changes from desht's suggestions .. I still haven't added exceptions or other errors, but I need to add test casts that require them first.

Bukkit:
https://github.com/NathanWolf/Bukkit/commit/19de121fba1431aa3f6d8b0b4169d79e2dfe3f07

CraftBukkit:
https://github.com/NathanWolf/CraftBukkit-Bleeding/commit/c1ca27dab01e668b81aafa3d359832b04b0318ec

EDIT by Moderator: merged posts, please use the edit button instead of double posting.

 

I rebased those branches to completely remove the glow API changes. There is already a PR under consideration for that:

https://github.com/Bukkit/Bukkit/pull/1053

So I'm just going to keep this as clean as possible. I'm working on the JUnit test case now, which I think will help a lot. Hoping to have PR ready soon, maybe today - still thinking about looking into protocol hackery.

 

This would also be pretty nice on entities, just gonna throw that out there.

 

Baby steps....

I think the argument there would be

1. Entities have UUID's, so you can track and persist data for them already
2. Entities are Metadatable, so you can already tag them as needed

I agree it would be handy to have a consolidated "metadata" interface that is persistent, but I also worry that ship has sailed
I got really excited about Metadatable until I realized it wasn't persistent.

EDIT: I don't mean to be hard on Metadatable, it's great for cross-plugin communication.

Allrighty, here we go! Wish me luck

https://github.com/Bukkit/Bukkit/pull/1064
https://github.com/Bukkit/CraftBukkit/pull/1376

I decided that, at this point, the best way to get more expert feedback is to just go ahead and submit it.

I've taken in all the suggestions I've gotten here (thank you!), the most recent being metalhedd 's error checking suggestions. It will now throw IllegalArgumentExceptions if you give it something that can't be serialized.

I also added ConfigurationSerialization object support, since that's not actually built into MemoryConfiguration (thought it was). This means you can do something like:

Code:

ItemStack item = getItemFromSomething();
MyConfigurationSerializeableClass foo = new foo("Something", 12345);
ItemMeta itemMeta = item.getMeta();
itemMeta.getCustomData().set("awesomesauce", foo);

Then, later, if you call get("awesomesauce"), you'll get your MyConfigurationSerializeable instance back out automagically. Pretty handy!

EDIT by Moderator: merged posts, please use the edit button instead of double posting.

 

1. But I'm lazy Also, there are poor, innocent noobs everywhere who are missing out
2. I was considering suggesting making a "persistent" metadata via NBT instead, however I realized that other things (blocks) are metadatable and don't store NBT (well, in most cases).

 

Can't Blocks effectively have NBT data by having an associated TileEntity? Or is that what you meant by "in most cases"?

Metadatable is nice for tagging things you're too lazy to track yourself, it is true. I use it for this, some Entities are really hard to track in a way that guarantees you don't end up with a messy map of dead references.

I suppose one could argue for Bukkit having an internal database and storing a consistent structure about anything we want to track. For Items and Entities, it could internally use a custom NBT tag to track the PK of the data store. For blocks it could create a PK from the location, etc.

This would make it implementation-agnostic.. though I do question if we'll ever see an implementation of Bukkit that's not based on Minecraft. Tempting to go write one with a nethack UI or something... er, anyway.

This seems like kind of a big thing to ask Bukkit to do, more like a stand-alone game engine feature.