Both these ways have their issues. The first one is just overkill for what should be applying NBT data, simply due to the fact of DecentHolograms using the old NBT structure, and the second is simply risky as we allow any arbitrary NBT values to be thrown onto the Item, which just isn't a safe thing to do here.
So with this issue am I now proposing to rework the NBT handling in multiple ways:
Reduce the NBT to only a collection of supported values.
Use pre-existing API from the server to apply the values.
I'll adress both now.
Reduce the NBT to only a collection of supported values.
There is no general need for supporting any NBT value possible, simply due to the fact that barely any NBT outside of a few can have a notable impact on the item.
Right now, the following options come to mind:
CustomModelData:<int> tag for applying a custom model data.
display:{color:<int>} tag for applying a color for a colorable item (leather armor, potion).
Item Glow effect (Not the enchant glint. The glow potion effect).
There isn't really any other NBT used here that I'm aware of, so reducing the workload here by only handling specific cases would be overall better and simpler in the end, especially with the next point:
Use pre-existing API from the server to apply the values.
Currently relying on NBT API for getting and setting values may work, but will eventually become tedious.
Not only does it add another dependency DecentHolograms needs to keep track of to stay updated, but with its planned move away from allowing shading to only be allowed as a plugin, would mean that DecentHolograms would eventually be hard-depending NBTAPI as a plugin, making it lose its dependency-free state.
By switching to using pre-existing methods (i.e. ItemMeta#setCustomModelData(<int>)) we could start dropping or dependency on NBTAPI and make things somewhat simpler to do.
Only issue that could be seen here is, that some methods may not exist in older versions (from what I gathered CMD only got added in 1.14+?), but one workaround could be for the most part to maybe extend the ItemStack in a own class to then modify accordingly...
Other benefits/improvements
Looking for specific keywords to handle could help simplifying the structure itself.
Like instead of {display:{color:1234567}} to apply a color could a user just have {color:#ff0000} and it would apply the proper color value.
Also, maybe to keep it separate would/should the new system use [] as identifiers to separate them from the old NBT setup, which could be supported for a while to allow a more seamless transition.
The tl;dr
Item NBT handling should be simplified to only handling specific keywords and values and applying them with pre-existing, long established API, to reduce maintenance work in the long run for when another major change to Items could happen.
Your input is welcome
Please share if you use any other NBT outside the above mentioned one and why, and I may add it to the list.
Also, feel free to share your view on this and if it would be a good or bad change.
The current aproach of applying NBT data to holograms items is... tedious to say the least, speaking from a coding perspective that is.
Having to fix the issues introduced in 1.20.5+ was a huge pain and still isn't fully resolved yet (see PR #237).
Right now is DecentHolograms doing the following when NBT is set for an Item:
tagcompoundCounttag to avoid conversion skippingBukkit.getUnsafe().modifyItemStack(ItemStack, String)Both these ways have their issues. The first one is just overkill for what should be applying NBT data, simply due to the fact of DecentHolograms using the old NBT structure, and the second is simply risky as we allow any arbitrary NBT values to be thrown onto the Item, which just isn't a safe thing to do here.
So with this issue am I now proposing to rework the NBT handling in multiple ways:
I'll adress both now.
Reduce the NBT to only a collection of supported values.
There is no general need for supporting any NBT value possible, simply due to the fact that barely any NBT outside of a few can have a notable impact on the item. Right now, the following options come to mind:
CustomModelData:<int>tag for applying a custom model data.display:{color:<int>}tag for applying a color for a colorable item (leather armor, potion).There isn't really any other NBT used here that I'm aware of, so reducing the workload here by only handling specific cases would be overall better and simpler in the end, especially with the next point:
Use pre-existing API from the server to apply the values.
Currently relying on NBT API for getting and setting values may work, but will eventually become tedious. Not only does it add another dependency DecentHolograms needs to keep track of to stay updated, but with its planned move away from allowing shading to only be allowed as a plugin, would mean that DecentHolograms would eventually be hard-depending NBTAPI as a plugin, making it lose its dependency-free state.
By switching to using pre-existing methods (i.e.
ItemMeta#setCustomModelData(<int>)) we could start dropping or dependency on NBTAPI and make things somewhat simpler to do. Only issue that could be seen here is, that some methods may not exist in older versions (from what I gathered CMD only got added in 1.14+?), but one workaround could be for the most part to maybe extend the ItemStack in a own class to then modify accordingly...Other benefits/improvements
Looking for specific keywords to handle could help simplifying the structure itself. Like instead of
{display:{color:1234567}}to apply a color could a user just have{color:#ff0000}and it would apply the proper color value.Also, maybe to keep it separate would/should the new system use
[]as identifiers to separate them from the old NBT setup, which could be supported for a while to allow a more seamless transition.The tl;dr
Item NBT handling should be simplified to only handling specific keywords and values and applying them with pre-existing, long established API, to reduce maintenance work in the long run for when another major change to Items could happen.
Your input is welcome
Please share if you use any other NBT outside the above mentioned one and why, and I may add it to the list. Also, feel free to share your view on this and if it would be a good or bad change.