[B+C] Add ItemMeta.hasCustomData/getCustomData. Adds BUKKIT-3221 #1064
Conversation
|
This looks interesting... I'll look it over further when I'm not on the Android app. :) |
|
I'm a little confused about this being a ConfigurationSection. Shouldn't we On Wed, May 14, 2014 at 4:42 PM, Travis Ralston notifications@github.comwrote:
|
|
My rationale was that this is akin to how we might use a YamlConfiguration to store simple data. It provides a nice structured interface for navigating and safely converting typed data, and the general structure (holding variant data that can be Maps or Lists) closely mirrors what is available in the underlying NBT storage. It's a familiar interface to most Bukkit devs, and I think it's a good fit. My original implementation was simply a Map<String, Object>, but I feel that puts a lot of unnecessary burden on the end user in terms of type checking and tree traversal. Using the ConfigurationSection interface also makes it slightly clearer that this cannot hold any arbitrary Object, and that it can handle ConfigurationSerializeable Objects (it occurs to me I should make that fact more prominent in the docs). Open to debate, of course! |
|
I've been experimenting with a few different plugins that serialize items (StarterKit, Shopkeepers) and I've found an issue I need to fix. The issue involves Lists, basically, and the fact that type information about what they contain is lost. I apologize for not noticing this sooner, I now have a test case for the specific problem but I'm not entirely sure how to fix it yet. |
|
Alright, now that I've had a decent look at this... The ConfigurationSection object is a bit weird. That could lead to potential issues regarding plugins writing and reading data over eachother or colliding on a key. A system more similar to Metadata may be more optimal to avoid plugin collisions. That being said, I don't really agree with storing the data to NBT itself. Although nice, I don't really see a use case for making an item a configuration object. This also currently implies that I can save ANY object to NBT so long as it's a ConfigurationSerializable object, but I don't see that serialization on the CraftBukkit side (if it's there, please point me to it). With the upcoming 1.8 ability to store custom tags in NBT, I think this PR should hold off for a bit. Once that capability is presented then I think this PR could be revisted for storing to the disk the information given. From the perspective of someone that could really use this, I almost feel as though a ConfigurationSection as well as the storing to disk is inappropriate for this application. |
|
See my comments above about the ConfigurationSection interface. You can't write an object to the root, so plugin collisions are no more likely than if, say, this was just a Map. There is no Plugin ownership like in Metadatable, I think that would be difficult to do in a persistent way, though I'm sure possible if desired. You may have missed the ConfigurizationSerialization implementation because I did it in a second commit: NathanWolf/CraftBukkit-Bleeding@5d1212a#diff-3ec13e0118854fb5a82d450c81f69a75R246 I was under the mistaken impression that MemoryConfiguration would do this automagically. :) EDIT: I am currently fixing an issue with a test case for this involving Lists, though, so it's not yet fully functional. That all said, I used the interface because it seemed like a good fit. I could certainly make a new interface, but my feeling is it would end up looking very, very much like ConfigurationSection. The note about 1.8 custom tags is extremely interesting- could you please point me to some official docs on this? The other half of the intention of this commit is actually to make sure Bukkit stores and preserves unknown or custom NBT tags. Otherwise I would just have thrown all of this under a "custom" tag and not have had to deal with the auto-registry and tag filtering. This could actually be a valuable addition if there is really going to be official support for custom tags, though I agree it may be worth waiting for 1.8 at that point. (I'm not sure I want to wait that long, but I can try and be patient). Storing to disk is a must for me, though. The whole point is to store data that is somehow directly associated with an item. If it's not persistent, it's useless to me. |
|
(correcting myself on the 1.8 NBT tags, misread the changelog) In 1.8 the ability to define 'custom' NBT tags onto an item will be passed down to the block entity (tile) once placed. I'm not so sure if this applies to all items, or if it's even possible on all items. The only notes I could find are in the ongoing 1.8 changelog and nothing more :( As for storing to disk, there's been many discussions and comments about permitting Bukkit plugins to actually support saving data to NBT (in fact it's mentioned in the contributing guidelines). This however does abstract it out a little bit so that if NBT takes a dive for whatever reason, the API could still be supported. I personally disagree with storing data to NBT on items, and I would really like to see a per-plugin possibility (very much like metadata) on this data regardless of saving to disk or not. The ability to flag an item in any sense can be incredibly powerful and very useful. With that being said (and my re-read of the 1.8 current changes), I'm not sure if it's best to wait until 1.8 and see what shows up or to go forward with this and potentially have a better solution after 1.8 (if the capability arises). I'd also like to hear feedback from other people on storing to disk; specifically in NBT format. I'm almost certain that a ConfigurationSection for this application is not ideal by any means, nor would a Map. At the bare minimum getters and setters should be present (see ItemMeta). |
|
Awww, @turt2live , you had me excited! Well, yes, to be clear I'm being very careful not to call this an "NBT access API". I go through great lengths to make sure you can't use it as that. NBT is just the implementation choice, and could be swapped out for something else later if needed. I think it's a good choice for this if we want it to be persistent (I do)- though if not something more like Metadatable would be a better fit. I could see an API that's more like a bunch of getters and setters, but if you start to allow Lists, Maps, etc, it starts to get very complex. And without then reimplementing all the serialization functionality of the Configuration classes, you lose a bit of flexibility. But honestly, I'd be happy with "String getCustomData(String)" and "void setCustomData(String, String)" .. I was just trying to make this is as complete and flexible as possible. |
|
Nothing can ever happen automagically
|
This replaces the ConfigurationSection-based getCustomData() method. The metadata store will only accept a new PersistentMetadataValue class, which encapsulates and hopefully clarifies the notion that only "persistable" data can be stored here. This includes Lists, Maps, most basic types, and ConfigurationSerializable objects.
|
I've re-worked this in a way that I hope will be less of a head-scratcher :) ItemMeta now extends Metadatable. See the updated description for some details about how I chose to handle this, I'm hoping to avoid confusion since Metadatable is usually non-persistent data. |
|
The only issue I see now is that people would assume that a persistent metadata value will work for other metadatables, like players. I don't think I explained my perspective very clearly in the previous posts: a metadata-like system may be more optimal. Basically I personally don't feel as though there is an existing interface in Bukkit that would facilitate this functionality. This is a very complicated and involving change, so I'm waiting to hear from more of the people who would need to maintain this implementation. (Pardon any English failures, the GitHub app is kinda... Bad) |
|
Overall a pretty decent job thus far though :) |
|
What if this expanded to cover everything that was currently metadatable? |
|
The original intent of metadata was a nonpersistent store. So I don't think sent from mobile
|
|
@NathanWolf @turt2live What's the downside for adding an |
|
Well thanks! I maybe jumped the gun on this re-work, but to be clear it wasn't just your feedback. I've been arguing against using Metadatable for this (same reasons of confusion) since before submitted a PR. I had made a forum thread to feel out the idea, most people's reaction seemed to be "this should be Metadatable, not ConfigurationSection". I agree there is not a really great fit in the API so far- I originally chose ConfigurationSection because it kinda fit, but I do think Metadatable is a better-kinda-fit. Really it'd be a pretty perfect fit, if not for the other existing use cases... I wish it was all just persistent myself :) I also decided to re-work this PR because I felt the secondary goals of storing existing unknown or custom data were seeming more and more selfish and not really within Bukkit's goals. |
|
I suppose you could adapt the other stores to scan for PersistentMetadataValue objects (or add a flag) and then persist then somehow.. but how? I guess for Entities you could use a similar NBT store, but what about Blocks? Assuming you don't want to create a TileEntity for each one. I guess what I'm getting at is it seems like at that point we'd want a consistent data storage mechanism in Bukkit, like a built-in database you could configure and then it's able to track metadata in a persistent way on anything. That seems out of scope for Bukkit, though, and definitely for this PR :) |
|
That is out of scope of Bukkit, as that is what per-plugin databases are sent from mobile
|
|
I agree, though after some thought I figured just in case anyone ever wanted to do the same thing for Entities, I should make the item store a more generalized NBT data store. So I did: |
|
@turt2live We already have the metadata system—why is making it persistent out of scope? (Not really trying to start an argument: I'm fine with whatever the answer is. I just want to understand the reasoning a bit more) |
|
This has been answered sent from mobile
|
This provides an efficient mechanism for the common use case of wanting to check for the presence of a specific metadata key without unpacking all of the metadata.
I have to admit I'm not sure what specific use cases will result in the base ItemStack methods getting called, but this is generally the correct thing to do, and let descending classes override for optimization purposes.
I think this is a good fit here as an extension to Metadatable, the more common use case is for a Plugin to want to retrieve its own data.
|
Sorry for all of the extra pushes. I've been working on putting this API into real practice so I can test plugin interactions in a live server environment, and I've seen some patterns that weren't well accounted for in the Metadatable interface, or which would be potentially inefficient. This may indicate that Metadatable isn't a good fit for this. I still think it works, though the more common use case here will probably working with data for your own plugin versus all registered data. |
There is no longer an efficient way to perform this check, and I don't believe there was a valid use case for its existence in the API in the first place. If a Plugin wants to interrogate for data, it should be doing so with a specific key.
Extend MetadataValueAdapter directly for performance, and remove all the variant constructor types. They were meant to serve as a compile-time suggestion for what types are ok, but in practice it feels cluttered.
| @@ -600,4 +601,40 @@ private boolean setItemMeta0(ItemMeta itemMeta, Material material) { | |||
|
|
|||
| return true; | |||
| } | |||
|
|
|||
| /** | |||
| * This can be overridden to provide an efficient quick check to | |||
There was a problem hiding this comment.
We don't actually support subclassing this class so mentioning you can override it to make it faster doesn't make sense. Same this for #hasMetadata(String, Plugin). I'm not sure what you mean about checking for it without unpacking it, can you explain this better? If it means what I think it does that shouldn't be here either since, again, the only supported implementation is the one that you've written here.
There was a problem hiding this comment.
I may have worded this poorly- by "subclass" I just meant CraftItemStack, though that really doesn't belong in the comments here.
Basically, the way it's implemented here isn't really useful- but the overridden implementation on CraftItemStack (which I think you will get at least some of, if not the majority of, the time?) provides much quicker access.
One of my main concerns with implementing this as a "get a copy of the data" system is that the data could be quite large, and copying it expensive. I wanted to ensure devs could at least do a quick "is there data here I care about?" check before having to make a copy of all data on the item (even data they don't care about) to inspect it.
So, that said, the implementations of these methods on CraftItemStack do not call the meta methods, but rather call static NBTDatastore methods to do direct inspection of the data without making a copy.
Restore type-specific constructors, and add asList and asMap accessors. I did not add array accessors.
|
@amaranth - I put the specific-type constructors back in PersistentMetadataValue, and hid the "Object" constructor. I didn't add in anything to recurse through Lists or Maps, though- do you think it needs to check that far in advance? |
|
There is only so much we can do to protect people from doing the wrong thing here. I don't think we should pay the cost of deep inspection, they'll figure it out when things fail. That's why I mentioned type erasure though, we could have done Map and List generic variants for each primitive and a generic Map<? implements ConfigurationSerializable, ? implements ConfigurationSerializable> one for everything else. Pretty sure Java won't allow that though since foo(Map<Long,Long>) and foo(Map<Integer,Integer>) will count as the same method. |
|
So, looking at this again, I think we should at least do a little more validation of Lists and Maps. I think we should use an iterator to get the first entry and check if it is a valid type. This isn't perfect but people don't usually use mixed types. We'd want to do this recursively as well so if you have Map<String, Map<String, List>> we check all three. I think we should also get rid of the glow stuff, it was a useful proof of concept to show how the system would work but should be handled in a separate PR. Aside from that I think we just need someone to review the API and javadoc, most likely @Wolvereness would be the one to get for this. I know the javadoc needs work but I'm not sure how to improve it. We also need to ensure this is compatible with what you can do with ItemStack NBT in 1.8 just to make sure we aren't doing this for nothing. |
|
@amaranth - I agree, I'm up for adding additional validation when I get a chance. I'm also open to pulling out the glow API - I'll admit it's a bit selfish of me to have it in there. It seemed like a good fit, and in the overall spirit of "customizable items", but I can certainly let it go. At this point, given the direction 1.8 is going in with custom item data- it seems most important to me to decide whether Bukkit is going to support plugin devs using custom NBT data on items, and if so if there will be a standardized approach. The "bukkit.plugins.[plugin].[key]" structure I've set up here is one example, but there may be differing opinions. If this was going to be "a thing" in the future, it'd be nice to know what the structure might look like so we (plugin devs) can try to build in some forward-compatibility. That said, I'm looking forward to what others have to say about this PR! |
|
Thinking about it more, I would like to make a final plea to keep the glow API in here: Besides just being a good test case for "standard" custom Bukkit data, I also think this is a better implementation of the API than could be done without the custom data API backing it, due to the strange interactions between glow and enchantments. It's also nice to be able to test them all together. That said, I can certainly pull it out- but I do think it's a nice fit/add here. |
|
I agree it can act as a first implementation of this new API. Maybe it's not exactly what the PR was built for, but it gives an integrated example. |
The Issue
There is currently no API method to track an ItemStack through its lifecycle for the purpose of creating a custom item.
Justification for this PR
The ability to create custom items by tagging them with arbitrary data could add lot of depth to what Plugin developers are able to offer.
There are various methods employed to accomplish this (protocol hacking, direct NBT editing, name/lore data storage) but they all have downsides and don't interact well with each other or the API.
The item "glow" API requested by BUKKIT-4767 is also added here, it was a very clean and easy fit into the metadata framework provided by the bulk of this PR, and will hopefully make testing all of this in conjunction easier.
This PR provides also provides a mechanism to store unknown or custom NBT data on items, which may be important given the addition of custom item data in 1.8.
PR Breakdown
Bukkit
The ItemMeta interface now extends Metadatable. This is a different kind of persistent metadata store that can not store arbitrary Object instances. I wanted to make that clear and hopefully avoid confusion, so I made a new PersistentMetadataValue type that must be used when storing custom item data.
Basic types, Lists and Maps can be stored, as can ConfigurationSerializeable objects.
ItemStack also contains two "hasMetadata" methods which can be used to do efficient first-pass checks for custom data without the expense of unpacking the ItemMeta structure.
CraftBukkit
Custom data will be stored in a "bukkit" NBT tag on the NMS ItemStack. This is an implementation detail that could change in the future, but at present is the easiest and safest method for storing relatively small amounts of structured data on an item.
The "bukkit" tag itself is reserved for internal Bukkit use. This PR also implements the item "glow" API requested by BUKKIT-4767, using a custom "bukkit.glow" flag to maintain state independently of enchantments.
Custom Plugin metadata is stored in "bukkit.plugins..". No effort is made to preserve data attached to items that is registered to an unloaded plugin.
Testing Results and Materials
A JUnit test is provided in the CB PR which attempts to exercise the API as much as possible. There is also a plugin that can be used for testing, available here:
https://github.com/NathanWolf/Test-ItemMetaData/tree/Metadatable
http://mine.elmakers.com/Test-ItemMetaData.jar
This plugin has several commands available for adding custom data, testing with enchants, glow effect, and checking for data.
Plugin Commands
All of the following must be used in-game.
invpush: Push your entire inventory into a serialized stack. Useful for testing item serialization.
invpop: Pop your stored inventory off the stack. Will drop any items you have to the ground.
All of the following must be used while holding an item
itemcheck: Display information about an item, including any custom data
itemclone: Make a copy of an item
itemunenchant: Remove all enchantments from an item
itemfly: Tag an item with some special data that also makes you fly when you swing it
itemunfly: Remove the special testing/fly data.
itemglow: Make an item glow
itemunglow: Make an item not glow, unless it's enchanted
After you've used /itemfly, the item will have its custom data updated each time you swing it. Use /itemcheck to verify the data is getting updated properly.
Relevant PR(s)
B-1053 : #1053 - @Ribesg 's glow API
CB-1376: Bukkit/CraftBukkit#1376 - Associated CB PR
JIRA Ticket
https://bukkit.atlassian.net/browse/BUKKIT-3221
Also resolves these tickets:
https://bukkit.atlassian.net/browse/BUKKIT-4864
https://bukkit.atlassian.net/browse/BUKKIT-4767