Custom Item Data

Discussion in 'Plugin Development' started by NathanWolf, May 6, 2014.

Thread Status:
Not open for further replies.
  1. Offline

    NathanWolf

    I'm hoping this is the right forum.

    Are any plugin developers doing anything with "custom" item data?

    By this I mean using NMS methods to store custom NBT tags on items. The main use case would be to create special items with custom abilities, without using the display name or lore to store and parse "data".

    For instance you could have a fire charge item that has a "bomb" NBT tag- when dropped, it explodes after a set timer. It would be difficult to do this without using special name hacks (like looking for an item named "Bomb"), since you can't really track an item instance through its entire lifecycle.

    You could also expand on this concept to have bombs with bigger yields, incendiary, longer fuses, etc- without having to embed all of that data in the lore for parsing. The NBT format provides a rich, structured data storage mechanism, but I am wondering if anyone has any thoughts or input on how dangerous it is to be using it for custom data.

    I suppose there's a chance in the future that Mojang would, for instance, add an item that uses that special "bomb" tag and now your plugin causes issues. This seems like a rare collision case, but also maybe a good reason to do something like store our data under a common "custom" root tag. There's also a chance that metadata will get further locked down and this will all simply cease to function.

    I'm not sure if there are any plugins out there that do this, but if there are, I would really love to hear from you. And if there is any official word from the Bukkit team about this (I know we generally don't discuss out-of-API issues), I'd really love to hear from you :)
     
  2. Offline

    NathanWolf

    I am going to be a bit sad, for a variety of reasons, if no one else is doing anything like this.

    Can anyone think of any plugins they've used that seem to have custom items where it's not plainly transparent what the item does by the name or lore? Something with some hidden special properties... anything like that?
     
  3. Offline

    raGan.

    You might want to take a look at this post.
    You will need to work with NMS ItemStack to change its tag. There should be a class to easily unwrap bukkit's objects. I only found this after quick googling.
     
  4. Offline

    NathanWolf

    Thank you, that was hugely helpful! Though perhaps not in the way you intended :)

    It looks like lots of people are actually doing this, or would like to, or at least see the potential in storing persistent custom item tags. I'm not so sure about metalhedd 's experiments storing 300KB of data though!

    Ok, so, sorry in advance for this everyone (particularly the few who I'm tagging due to a year-old post), but ...

    desht CaptainBern Comphenix Ultimate_n00b Zhro

    I'm trying to see if there are more people out there using custom NBT item tags, it appears that there are- I'm curious if you've faced the same issues I have and how you've worked around them. Specifically I have a few major problems that all sum up to "When Bukkit makes a copy of the item, it loses any custom data":
    • Using the creative mode inventory at all (I have to do my best to prevent this, eject items on mode switch, prevent pickup, etc)
    • When the ItemDragEvent triggers- I have to cancel it and fake my way around it
    • A few other inventory-related events that send me copies or drop copies into the world
    • Any other plugin that tries to copy or serialize an item
    All in all it's been a pretty big challenge, I'd say 90% of my plugin's reported issues amount to "something made the wand stop glowing and it doesn't work anymore", the root cause being something from the above list.

    But, I am not willing to let go of the potential of custom item data. I'd appreciate any thoughts, comments, et cetera any of you might have on the topic!
     
  5. Offline

    Ultimate_n00b

    I haven't had a need for this since my original need. I'm not really using it any more.

    However, a good way is to just add a unique name/lore to the item. If it has a bigger radius, say that in lore then check when it's placed if it has that lore.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 8, 2016
    NathanWolf likes this.
  6. Offline

    NathanWolf


    What if it has 120 unique spells on it? A couple dozen other properties that scale up independently of one another? Something that can grow and change over months of enchanting, that kinda thing.

    I could do something hacky like pack some data structure into base64 and put it in the lore.. maybe get fancy and use that weird ChatColor that turns it into gobbledly gook. But, you know, it's not ideal :)
     
  7. Offline

    xTigerRebornx

    NathanWolf likes this.
  8. Offline

    Relicum


    What he means is you store a ID key on the item/enity which links to any kind of data storage what holds the effects/abilities you wish to preform.. Simple. Plus Storing 120 unique spells on the item would be more data that make up the block.
     
    NathanWolf likes this.
  9. Like xTigerRebornx said, Portable Horses is doing this the best way I can imagine. He stores the data inside the lore of the item, then uses protocol lib to hide the lore for the client. But because, when in creative, the client sends the item data back to the server, all data will be whiped. So you'll have to set an NBT-tag with the data, then detect the incoming itemstack packet, get the nbt-tag before bukkit filters it out and set the lore back.

    I'm now looking for better ways to do this though. I've been thinking to make use of the Attribute-API but I have no idea if it will persist trough reloads etc. So I can't really tell.
     
    NathanWolf likes this.
  10. Offline

    desht

    NathanWolf I've been using Comphenix 's AttributeStorage classes for this - storing custom data in a dummy attribute's name rather than the attribute's value. Nice thing about this is you can store up to 64K of data, and it doesn't get sent to the client. More info here: https://forums.bukkit.org/threads/l...-tags-with-a-compact-class-no-obc-nms.178464/

    My code is here: https://github.com/desht/sensibletoolbox - in particular, https://github.com/desht/sensibleto.../desht/sensibletoolbox/items/BaseSTBItem.java. Two primary methods in that class of interest:
    • toItemStack() serialises one of my BaseSTBItem objects (of which there are many subclasses) onto an ItemStack (the attribute name is basically the frozen object data converted to a YAML string)
    • getItemAttributes() extracts the saved attribute data from an ItemStack into a Bukkit Configuration object, suitable for constructing a BaseSTBItem


    It does :) Also seems to behave itself properly when you're in creative mode.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 8, 2016
    Comphenix and NathanWolf like this.
  11. desht Oh! Thanks for the info :)
     
  12. Offline

    NathanWolf

    Relicum Ah, ok- I feel like a bit of an idiot for not thinking of just storing a unique key instead of all the data :)

    My plugin currently doesn't track or store any data at all, and I like it that way- so much so that I sometimes forget there are other ways.

    xTigerRebornx CaptainBern That looks like a pretty nice solution, actually. I do use and love my lore, so I wouldn't want it empty- Though it looks like I could use the packet approach and send fake lore, rather than just empty lore? The trick with avoiding getting data wiped sounds like I wouldn't be much better off than I am now, though.

    desht That looks like the best approach I've seen so far, though it feels a bit hacky, eh (mainly the bit where you have to put the data in a name)? And general support for attributes seems to be left to plugins in some cases- like I noticed Shopkeepers handles it as a separate set of data to serialize. Not sure if this is still the case- sounds like you're saying attributes get automatically serialized now? Or did you mean "persisted" just in that Bukkit doesn't wipe the data?

    Ok, well let me try and be more direct. I wasn't coming here, necessarily, to try and find an alternate approach. Though I guess if I find one that has no issues and works the way I want, maybe I'll re-work my whole plugin :p

    I'm actually very happy with how my plugin works, minus the issue of Bukkit losing my data. If I were, say, implementing this as some kind of direct Minecraft server mod, I'm pretty confident that the way I'm doing it is the "right" way. In my mind, I'm creating a new type of item that stores a new type of data- and the precedent for that internally seems to be "add a new NBT root tag, store data under that in a structured way". Do you all disagree, or are we just doing it differently because the "right" way doesn't work?

    So, I guess what I'm here to find out, is there anyone else who wants custom NBT data functionality? Not even necessarily (though ideally) as an API feature, but at the very least for Bukkit to be respectful with our custom data.

    If you could just use the NBT tree to store structured data, and it worked as you'd expect, would you use that instead of protocol hacks, attribute abuse, or lorefuscation?

    Since I Tagh'd a BukkitDev staff, I want to be clear I'm not trying to be antagonistic or form some kind of angry mob here :)

    I'm just honestly curious if I have any support for this, in terms of pushing for a fix.

    I'm probably not the only one to have done this, but I have a CB patch that seems to resolve all of these issues. It's not a hack (er, well, IMO at least), I made an effort to properly implement official storage of custom data. So far it seems to fix all the issues with my plugin, creative mode, interactions with other plugins (Shopkeepers selling wands without additional work, a beautiful thing).

    If I'm the only one that would care for such a fix, or use such an API should an API eventually exist, then I'm not going to have much traction in trying to get this fixed (or changed, depending on perspective) internally.

    I have not submitted a PR (yet), I want to do quite a bit of internal/personal testing, and maybe clean up code... but I probably will not bother unless there's actual support for it, otherwise it looks like I'm just trying to bully my way into Bukkit's code, which is really not my intention.

    I am also hoping to hear an official stance on why this is not allowed (I tried to theorize a bit in my OP) - if there is a real reason why storing custom data in there is a bad thing to do, I will give up and be more motivated to change my ways, as it were.

    Ok, thanks for hearing me out, and I appreciate the alternative approaches (and would love to hear more of them!)

    Well, since I have gathered some massive brainpower together here, does anyone want to check my work? :)

    The main commit that fixes all of my problems and brings Christmas in May:
    https://github.com/NathanWolf/CraftBukkit-Bleeding/commit/67617ff88a213bc0e74e05e84368409d362bce68

    The branch I've been testing with also has this:
    https://github.com/NathanWolf/CraftBukkit-Bleeding/commit/f2daef9ae07fb23f7b82637a103da8f03894556d

    Which is a minor change to preserve empty enchantment lists- something that could be the basis of a "isGlowing/setGlowing" API if desired, but is at least necessary to preserve items that have been made to artificially glow. This bit is not so important to me, just an aesthetic thing, but it's nice that if I force an item to glow it stays that way.

    So far I've encountered no issues with the first commit. The second one did cause some trouble when using my plugin in conjunction with dtlTraders, which now gets back an empty enchant list in a case where it never would have before, and it kind of freaked out. So I'm not so sure about that one- it was an easy fix in the dtlTraders code, and only happens if some other plugin (mine) is forcing glow and you add that forced-glowing item to a Trader. IMO it's a worthwhile change given the risk, but the first commit is really the important one.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: Jun 8, 2016
    CaptainBern likes this.
  13. Offline

    Kainzo

    Seems interesting :D
     
    NathanWolf likes this.
  14. Offline

    xTigerRebornx

    NathanWolf Do you think that if a way to interface w/ NBT through the BukkitAPI was added, that it could ever have a similar functionality to the current Metadata system in Bukkit (where you can attach FixedMetataValues w/ Objects in them), since currently, I only see some things like Integers and Lists and Strings, but not a generic Object for NBT (Unless I am just blind :p), so would there ever be a way to attach Objects that I would create (Say, a class called Stats or something like that) to ItemStacks through NBT?
    Either way, the idea is an amazing one, would love to have this type of functionality
     
  15. Offline

    NathanWolf


    Well, not exactly- the Metadatable classes rely on having something (an Entity, Block) you can track- Items don't really have that (which is sort of the whole problem, really). So we can't make a map of items to data, or otherwise tag an item instance - the moment a player drops the item, it becomes an entity for a while, then an item again, etc.

    Also, Metadatable data is not persistent- which is how you can store anything you want in there without having to worry about if its Serializeable or not.

    I would say that this is more akin to storing Configuration data than it is Metadata. So much so that, if I were to propose an API for this, it would likely be something like this (probably attached to the ItemMeta class):

    Code:
    public ConfigurationSection getExtraData();
    public void setExtraData(ConfigurationSection extraData);
    public boolean isGlowing();
    public void setGlowing(boolean glowing);
    I would basically wrap the item's internal map of extra data in a Configuration, so you could use all the helpful type casting methods. The basic structure (tree of arbitrary variant data) is the same, so I think it'd be a good fit.

    I threw in the glowing methods just for fun :)

    Anyway, it does relate to the Metadatable stuff in that it's storing metadata on items in a similar way to how you'd store metadata on an Entity or Block. But this is persistent data, so it must be serializeable, and it will be there on reload (or even if you move your data files elsewhere, etc- it's baked into the chunk or player data)

    It probably wouldn't (without a good bit of planning and work) be able to track Plugin ownership like metadata does- but it could have the same potential for cross-plugin sharing.

    For instance, if you knew my NBT structure (wand.*) you could create one of my wand items without any API integration with my plugin. The item wouldn't do anything unless my plugin was present, since otherwise it's just ignored extra data.
     
  16. Offline

    xTigerRebornx

    NathanWolf Its still an amazing idea, and I'd love to see it implemented.
    Some side thoughts, such as having my "custom" Objects having the ability to serialize as a String, then attach it, and do it that.
    Either way, some form of non-lore/non-name storage would be nice.
    And I wasn't really comparing it to the Metadata system, only using that as an example of how you could have all types of Objects stored on Entities.
    Well, good luck on working on this (and hopefully you'll get support and have a PR sent in to try and get it implemented in some way), Ill look forward to a way of doing this w/ BukkitAPI someday :p
     
    NathanWolf likes this.
  17. Offline

    NathanWolf


    Thanks! I think if your object implements ConfigurationSerializeable, you could just toss it in the item data. Just realize you can't serialize object references and such (er, easily).
     
  18. Offline

    xTigerRebornx

    NathanWolf Yeah, having it based off of the ConfigurationAPI would be a good idea, but it might be better to implement a different API based on the ConfigAPI, as all people might not intent for their objects to be ConfigurationSerializable :p
    They could probably still use the base of the ConfigAPI (like the MemorySection) to implement similar structure.
     
  19. Offline

    NathanWolf


    Well that's what I love about ConfigurationSection - it's just an interface :) You don't have to use serialization at all, and in fact I tend not to - I prefer to have objects with save/load methods that write to a config section, though I can't really attest that it's any cleaner.

    As an example, here's my Wand.load method:

    https://github.com/elBukkit/MagicPl.../com/elmakers/mine/bukkit/wand/Wand.java#L866

    It just takes a ConfigurationSection and loads what it needs out of there. This actually gets called via two different paths- one loading a section from actual config file (creating a new wand from a template)- and one that gets called with data that's been passed through a conversion layer. I pull the NBT data out of the item, stuff it into a ConfigurationSection and then pass it to the same load method. I used to have two separate methods for loading from NBT or a config file, and it got a bit messy.

    So I may be a bit biased in wanting a ConfigurationSection wrapper, it would mean an easy transition for me to an API :)

    Saving is the same way, in reverse- you could pull an item's existing extraData ConfigurationSection out, modify it (via methods or serialization), then put it back into the item. I'm thinking if you call getExtraData on an item with no metadata, it'd just return an empty ConfigurationSection (it'd be using MemorySections to do all of this). This would make it similar to getItemMeta in usage.
     
  20. Offline

    xTigerRebornx

    NathanWolf When ConfigurationAPI was mentioned, I was thinking more towards the FileConfiguration, which is why I got mixed up.
    A wrapper for ConfigurationSection would probably be something a large majority of people would be familiar with, since a lot of people tend to use plugins :p
    Either way, when you do decide to PR it,it would be better to plan out an approach for both the implementation and interfacing through BukkitAPI. There are many ideas of how this could be done, but you should take precaution to what would be the best approach for implementation and the API to get it accepted (though I think you have the support of some people who are interested in this). Would be nice to have some opinions from the people who would actually implement this past a PR, and the people who would decide on whether or not its a "good" idea.
     
  21. Offline

    NathanWolf


    I agree, I'm hoping for such feedback. I don't claim to be an expert in MC internals, not in the way the BukkitDev team is. If there's a reason this is bad, I really do want to hear it- that's not some empty challenge. I don't want to do evil :)

    Admittedly, I also don't want to do a lot of work unless there's a chance it'll get accepted in some form. Though at this point I'm halfway there anyway.

    I've thought (quite a bit) about implementing the API side of this - but, and maybe this an invalid perception on my part, I feel like there would be more resistance to a PR for "Custom Item Data API", versus one that is more about making sure we don't lose data that may or may not be valuable to something (a Plugin, a newer version of Minecraft, etc).

    However, I was already shot down on this before:

    https://bukkit.atlassian.net/browse/BUKKIT-4864

    (EDIT: Though, strangely, the one that would solve all my problems remains open: https://bukkit.atlassian.net/browse/BUKKIT-3221)

    I've tried very hard to come up with a use case for why this needs to be fixed that does not involve going outside of the API, and the best I've come up with is Minecraft updates. The recent Skull item update is actually a great example... I think you could find some builds of 1.7.9 that would effectively wipe your custom (player) skull heads if you had some in your inventory and dragged them around a bit, or went into creative mode. But .. yeah this is kind of an edge case.

    If I were to do an API, I would basically do just what I described above. This would be reasonably easy to do at this point since the internal storage is a Map<String, Object>, which also happens to be the internal storage of a MemorySection. So basically I'd be implementing a couple of methods to shuffle Maps and MemorySections around, and I'd like to implement is/set Glowing while I was at it. getGlowing is just return hasEnchants() - setGlowing would add an empty enchant list if !isGlowing() ... both one-liners, more or less :)

    EDIT#2: Since I'm happily linking issues, here's one for the Glow API:

    https://bukkit.atlassian.net/browse/BUKKIT-4767

    Vote early, vote often! ;)
     
  22. Offline

    xTigerRebornx

    NathanWolf I agree on believing that it'd be a better approach to decide "What would this fix" vs "I want custom ItemStack Data API". Submitting this as a fix, rather then a suggestion, will probably increase its chances on getting accepted (as some things that may be implemented in the future that use NBT would be fixed, and anything now that has problems would be fixed), but based on the fact that they aren't supporting hooking into CB, it'd be hard to use "Support for Custom NBT tags" as a reason. Though, I believe that the one that "would solve all your problems" is still opened due to the fact that they are asking for an interfacing/new feature to be added to the API, rather then directly stating that they want support for hooking into CB.
    Taking an approach to submit this as a "fix" and "feature", with the right precautions, would probably be a good route to take. Either that, or submitting this as a "fix", then later submitting a request for a new feature to add implementation of the possibilities that have arisen as a result of a fix, and using that as an approach.
    Another thing to add on the list of reasons, would be that it would help enforce an idea of the BukkitAPI not being Version dependant, since it would add support to future NBT data added in, allowing those 'new' features to be implemented into plugins without having to rely on the specific version of CB that they were added in (or some wording of this idea)
     
  23. Offline

    NathanWolf

    I've been giving the API a lot of thought, if I find time this weekend I may try to put a Bukkit branch together and basically prepare for a PR. I'd appreciate some peer review at that point before I take it any farther, though!

    Currently I'm waffling on two main things:

    1.
    Whether or not to allow "direct" access to the ItemMeta "extra data". Normally in the Bukkit API you call get to get a copy of some data, modify it, then call set to put it back. I worry this will be a bit inefficient here, given this is an arbitrary large tree of data and you may only want to touch one value.
    So if I were to use get/set, I'd probably also want to add some utility methods that don't do any wrapping - like "Object get(String key)" .. but then I feel like I'm just re-implementing ConfigurationSection.
    Which leads me to think maybe I should just have "getExtraData()", and it returns a live reference that you modify directly. Any thoughts?
    Either way I'm thinking of taking what I have now and better encapsulating it. I'd like to create a new "ItemMeta extra data" class that holds the extra data and takes care of all the NMS conversions. So what you'd get would either be that object directly, or a copy of it. This object would implement ConfigurationSection.
    2.
    Naming. I called the data Map "extraData" in my current branch- but I think that's a little vague. I mean it could be like "(class ItemMeta): ItemMetaData getData();" or "getExtraData()" .. but that's kind of vague and confusing (I'd like to avoid "MetaData") either way. Any suggestions there?
     
  24. Offline

    xTigerRebornx

    NathanWolf I also questioned the idea that was in https://bukkit.atlassian.net/browse/BUKKIT-3221, where it suggested that ItemMeta extend/implement something. I like the approach of creating a new class for the 'ItemMeta extra class', you could call it something like ItemData, and it would store that extra data in an implementation of ConfigurationSection.
    If you do take an approach of structuring it how you have it now, I'd go with the live reference, as it'd make more sense to have it live.
    You'll be able to get a good ammount of peer review by posting the idea in a place where it will get the proper attention (and the correct section) on the forums. I'd also get the attention of some people who have messed around with the NBT editing (like Comphenix) and see if they would help with forming a good PR and branch of Bukkit to prepare for the PR.
     
    NathanWolf likes this.
  25. Offline

    NathanWolf


    What do you think is the proper section? I wasn't really sure, but I'd love to move it if there's a better one.

    I heavily refactored my original commit, I'm still testing it for any functional changes but the code itself is much cleaner. I ended up pulling everything I had done out and encapsulating it, so if you'd like you can see it all here now:

    https://github.com/NathanWolf/Craft.../craftbukkit/inventory/CraftMetaItemData.java

    I think you can see where I'm going with this- it extends MemorySection so you can use all the ConfigurationSection methods on it. Each CraftMetaItem object has an optional CraftMetaItemData object to deal with its custom data.

    I don't have an any accessors yet, I'm still weighing the pros and cons of just throwing in an ItemMeta.getData() method and allowing direct access as a ConfigurationSection.

    This branch also adds setGlowEffect and hasGlowEffect for BUKKIT-4767, mainly because it was the easiest way for me to fix the behavioral change I'd made to hasEnchants().

    I could probably pull that out into a separate branch and clean this all up, I guess I'll see what kind of feedback I get first though. Thanks for all your input!
     
  26. Offline

    xTigerRebornx

    NathanWolf While I am not sure of the exact location, the WIP section may be the proper place for this. It may not be a plugin, but its a WIP feature/idea for Plugins to use.
    The class in its current state looks awesome :p
    Perhaps you could tahg some people who would be able to contribute (I'd hope they wouldn't mind being tahg'd to contribute)?
     
  27. Offline

    metalhedd

    Wow, talk about TL;DR :) Just popped by because I was tagged here. For what it's worth, The technique used in PortableHorses has been extremely reliable. CaptainBern did a good job of explaining the process, but here is a slightly more in-depth explanation:

    1) the data is base64 encoded and spread across several lines of lore, each of which is given a 'magic' sequence of color codes as a prefix.
    2) when a packet is going from server->client, I check for any portablehorses item data in the lore, and if there is any, its removed and re-encoded as a custom NBT Tag. This way a client in creative mode will not lose the data when it sends it back in a client->server packet.
    3) when a packet comes in which contains the PortableHorses NBT tag, its removed and placed in the lore once again.

    To my knowledge, no-one has ever lost data from a PortableHorse, though it is theoretically possible to crash a client with too-large of a packet, in practice this hasn't been an issue at all.

    desht, I investigated the attribute storage system a while back and found that the data is indeed send to the client, and is equally capable of crashing the client if the packet is too large, Have you found some trick to avoid sending to the client at all?
     
    CaptainBern likes this.
  28. Offline

    NathanWolf

    metalhedd Direct NBT storage will potentially have the same issue. Do you have a feeling for what a sane string length limit would be for keys and/or values? Is it even a problem for values?

    The API I'm proposing would otherwise be limited to fixed length values, though I'm not sure if then have to worry about array or list sizes, too.
     
  29. Offline

    desht

    I'm guessing you're referring to this conversation here: https://forums.bukkit.org/threads/l...compact-class-no-obc-nms.178464/#post-2122925 ?

    In which case, yeah, you're right - the attribute data (being part of the item's NBT) will get sent :( In my case, probably not a big deal, since the data isn't large. However, I should probably still strip it with ProtocolLib to reduce network traffic.
     
  30. Offline

    NathanWolf


    If CraftBukkit allowed custom data storage internally, would it be worthwhile to try and strip it out before sending to the client?

    I'm trying to put together a nice clean PR so I wouldn't want to to introduce anything really hacky, but that sounds like it would be tremendously safer. If there was a clean "sanitization" point on sending to the client, it'd be easy enough to provide a copy of the item with the extra data stripped out. If it doesn't already make a copy for sending, then this could have a performance impact and I wouldn't want to mess with it.

    At this point I'm also pretty sure I should enforce some sane limits on data. I don't really intend for this to be a bulk data storage mechanism. If you need to store like megabytes of data in an item, you should probably store a key in the NBT data and keep the data elsewhere. I'd recommend the same thing using a real DB though.... :)

    I'm thinking like 255 chars for a key, maybe 4k for a String value? I'm not sure if I'd want to similarly limit List and Array entries, though. These will be constants that Bukkit could tweak if they wanted to (assuming this ever got accepted), but I'd like to aim for some good defaults.
     
Thread Status:
Not open for further replies.

Share This Page