Paper
Paper copied to clipboard
PaperMC
Fixed CraftItemStack/CraftItemMeta enchantment level inconsistency
CraftItemStack#getEnchantmentLevel and CraftItemMeta#getEnchantLevel behave differently, which breaks compatibility when the Bukkit Enchantment registry doesn't match the internal NMS enchantment registry.
I've discussed this in the discord, but I will elaborate here. I understand an objection made by mbaxter, that quote: "You are deliberately ignoring that this would be implicit acceptance of maintaining support for a previously unsupported custom enchantments setup, in perpetuity."
However, I posit three counter-arguments to this:
First is the argument of maintenance of a previously unsupported setup. While it's true that paper doesn't officially support registering custom enchantments to the API, it's also well known that many plugin developers do this. That, in itself, doesn't constitute an argument - after all, it was an unsupported method, however, the problem is the indefinite maintenance of supporting this. I argue that there is essentially zero maintenance required if this PR is merged. In order for this pull request to require maintenance in any subsequent version, Mojang would have to completely rewrite how enchantments are stored in item NBT, breaking every enchanted item that has ever been made. It would be the first change to this system since enchantments were added to the game, and I would argue that it's fair to say that this will not happen. It's not worth worrying about future changes to a system that in all likelihood will not change.
Next, I want to mention the net benefit this would give to server owners and plugin developers. There are many plugins that register custom enchantments to the API, whether it be to apply a glow effect to items, or to have full custom enchantment systems. While I understand that the natural reply to this would be 'Just use PDC', this wouldn't add a glow to items, and it would break cross-plugin compatibility where plugins can naturally integrate with each other's custom enchantments simply by doing Enchantment.getByKey(NamespacedKey.minecraft(String)); (as they have done since 1.13, over 4 years ago). It would break years of existing compatibility between plugins, and would (and currently does) cause a headache to server owners.
And on a more API-centric note, because of how it is written currently, ItemStack#getEnchantmentLevel does not function the same as ItemMeta#getEnchantLevel. Calling the method on ItemMeta works as intended, returning the level of whatever enchantment is on the item. However, calling it on ItemStack (or specifically CraftItemStack) returns the level of whatever enchantment is on the item, if and only if the enchantment is also registered via NMS into the Registry, and if the enchantment is either an instance of EnchantmentWrapper or CraftEnchantment. I understand a reasonable objection to this would be 'Why not register into NMS if it's such a problem?' but this causes many problems with natural generation, client incompatibility, et cetera.
I completely understand why you may object to this on the grounds of principle. However, the maintenance requirements are slim to none, it would improve the developer experience and fix plugin incompatibilities that have been generated by updating the CraftItemStack code, it would fix API inconsistency, and most importantly, it would ease the lives of thousands of server owners that have seemingly bizarre bugs caused by this.
PS: In order to completely remove the chance of there any maintenance obligation whatsoever, this change could simply be
return getEnchantments(this.handle).getOrDefault(ench, 0);
however you could argue that this would have (albeit microscopic) performance implications where the loop isn't returned from as soon as the enchantment is found. In order to sidestep this potential issue, I've submitted a more optimised version in this PR.
Too late for me to comment on the politics of this pr, but it would certainly help you if your patch would follow basic paper contribution guidelines.
Minimise the diff using fully qualified imports and don't delete imports nor edit code you don't even touch.
Too late for me to comment on the politics of this pr, but it would certainly help you if your patch would follow basic paper contribution guidelines.
Minimise the diff using fully qualified imports and don't delete imports nor edit code you don't even touch.
Ah! Really sorry about that, must have been touched by my auto-reformater, i'll fix that
Patches shouldn't modify older patches without explict cause, this should geneally just go into the patch which made those changes
In the long term, this is 100% unsupported behavior, relying on the broken behavior of craftbukkits ItemMeta system in order to even work, this WILL break in the future, however, there is a level of hope that we are able to support this properly in the future
I'm inclined to accept this on the basis that this EXPLICTLY does not constitute the support of this behavior, I intend to push 0 effort towards this behavior as the codebase evolves, but, given the nature of how long this system has been broken for, I have some, albiet reserved, inclination to accept this just for the basis of the community derived behaviors here and to have some level of consistency in terms of expectations of results.
It's already iffy on how this will exist post the properties API, given that we NEED to start moving towards handing on server internals more, rather than relying on craftbukkits dated representations of stuff. I heavily suggest that plugin developers need to find a more supported way of handling this which doesn't rely on the broken behavior of CB.
Yeah, I completely agree - it sucks to have functionality that relies on the questionable nature of CraftBukkit. What would be nice is to have an officially supported way of registering enchantments the way people currently do, that has all the compatibility benefits without losing any of the customisability that current solutions have (e.g. enchantment weights). I'd love to be able to (down the line) contribute an official custom enchantment API that works like the current one, but actually supported without any of the jank.
The real problem is how it's literally impossible to have a custom enchantment plugin with a fluent API that works like how vanilla enchantments do (which is the ultimate goal, ideally) without doing some janky workarounds.
Paper has PRs in the works to allow proper modifications of the servers registries before startup, which would hopefully then enable registering custom enchantments down the line. (https://github.com/PaperMC/Paper/pull/8108 and https://github.com/PaperMC/Paper/pull/8789)
I am interested in your argumentation however, specifically point three. I personally do not see any merit whatsoever in actually having a desync between any api registry and the internal ones it mirrors. That only leads to inconsistent state down the line (e.g. exactly what is happening here). "natural generation, client incompatibility, et cetera." do not particularly convince me, can you go into more details here ?
Natural generation should not be affected whatsoever by an addition to the registry ? Lootables etc specify which entries they use, they don't just use all registry contents. I can see a potential issue with clients, as most of these registries are not synced with the client, however that would be the case in either way ? whether or not you only mutate the api registry mirror/view or the server one itself, at some point you'd have to translate to something the client speaks.
Beyond my few questions in regards to point three, I would not mind pulling this, however I would, as cat already mentioned, very very clearly state that this behaviour (unfreezing and modifying the bukkit mirror of the enchantment registry) is not at all supported and my stop working at any point in time (e.g. when we provide proper API to register custom enchantments). If we do so through strong additions to javadocs or for all I care a warning on the server when a plugin attempts to register these enchantments to the Enchantment class is certainly up for debate, but I hope that everyone can at least agree that the current way is beyond hacky and not a healthy way forward for the API to evolve.
I hope from that angle you can understand the partially negative feedback to this suggestion (both on this PR and the discord). I am sure plugins like your EcoEnchants are aware of the risk they are taking when using this hack, but developers are quick to interpret bug fixes as some form of support/endorsement for this "feature", which is not good for the API going forward as we want people off this hack as fast as possible once we enable registry support.
If you register with registries, you have to specify an enchantment weight out of an enum, and then that's all handled by NMS. The problem is that people (in my experience, including me) much prefer having more granular control, for example controlling enchanting table probabilities independent of villagers, independent of fishing, independent of loot generation, etc. People also love having things that aren't usually available in with vanilla enchants, like making them available from villagers but not from loot, or having them only be obtainable from an enchanting table, or having them not be obtainable through vanilla mechanics at all. Another issue is with vanilla clients - if enchantments are allowed to generate before PrepareEnchantEvent (I think thats what it's called) it leads to the client being led to decode lang keys which don't exist, which makes people think there's some kind of bug. Obviously that's outside the scope of paper, though.
But more importantly, the real hitch is with enchantment targets. Vanilla enchantments (and those implemented via the registry) have to choose pre-set items which enchantments can be applied to. This is a real problem for people that want them to only be applied to items that previously weren't enchantable and especially those which want enchantments to only be applied to custom items, which is especially popular now that custom items are a common feature on a lot of modern servers. The current vanilla API just feels extremely limited and feels more like a hinderance to server owners.
Also - being only able to register on startup is actually more of a pain to server owners than you might think. If you're making a bunch of custom enchantments, the last thing you want is have to restart the whole server every time you make a change. Currently, you can reload the plugin as many times as you want to register and unregister whatever you like - and that's a massive productivity bonus, especially for large servers.
In an ideal world (or at least my ideal world), the way vanilla handles enchants would have the kind of granular control that things like EcoEnchants provide, with the ability to specify exotic obtaining methods, the ability to specify custom targets, specify custom colours and formatting (including gradients), the ability to really get into the nitty-gritty as it were. If you have a look at a lot of servers that use EcoEnchants, you can really see how much extra customisability it provides, which is really useful especially in the current era where servers use Minecraft more as a platform to create custom content rather than the base game with additions.
That doesn't even bring up the way custom enchantments are currently having to be shown to the player. In order to apply all of these custom configurations and designs, every item has to be modified in packets to apply the new rules while still working with things like hide enchants (which is why the server lore approach doesn't work by the way, hide enchants would have to be a custom meta tag rather than just working naturally with the hide_enchants flag) - it's the hackiest of hacks. And while I don't mind doing this kind of thing, I actually find it quite fun, it really would be nice to have an API (and of course a client) that allows for this kind of thing.
I completely agree that this is extremely hacky, but unfortunately it's the only way to provide the best of both worlds. If a custom enchantment API that provides proper cross-compatibility is ever developed, I'd love to help and provide my own opinions (and the opinions of others) in it's design to have it be as flexible as possible, in line with how EcoEnchants (and other plugins, I'm sure) are able to provide customisability now.
It's a shame to have to rely on a hack, but that's the only possible way currently.
This is why custom enchants like this should be done via the PDC and Lore with the enchantment plugin providing it's own API to access those "enchants". You can control every aspect of it and you don't have to do any hacky things to try and register enchantments. You already generally rely on an enchantment plugin's api if you want to interact with those enchantments in another plugin, you need the enchantment instance it creates.
I really understand your point here, but it has some really critical flaws that seem to fall on deaf ears:
- The client has no idea to show a glow on PDC enchantments, so you'd have to register a fake 'glow' enchantment anyway, which takes us back to where we started
- It breaks API compatibility. Each plugin would have to manually integrate with the custom enchantments plugin (which is a massive pain for large, deeply-interconnected servers), and Enchantment.getByKey() wouldn't work at all, which again breaks integrations and the entire seamless nature of the current setup.
It really sounds good at a surface level, but being someone who's tried both designs, the tradeoffs just aren't worth it. And at the end of the day, you're not making an enchantment plugin at that point. You're making something that pretends to be enchantments.
While this would be a helpful change, it just sucks that we have to rely at all on craftbukkit to have custom enchants. While ideally we would have native custom enchant registering, this works for now until that does happen.
You're making something that pretends to be enchantments.
That is already what you are doing tho. You are hijacking a system (the vanilla enchantment system/the bukkit) to pretend what you are creating and applying are enchantments when they really are not, at least not in the vanilla sense of an enchantment, which is what the API aims to expose. The API (in its current state) is not there to allow for custom enchantments.
Nonetheless, and as I stated earlier, I don't mind pulling this as long as everyone understands, this system is far from great and will die hopefully soner than later when we can register stuff to the registries. How granular paper can mutate vanilla enchantments to be at that point is up for the implementation, I am sure you'll have plenty of input (I am looking forward to it!) when the time comes.
In regards to the comment
being only able to register on startup is actually more of a pain to server owners than you might think. this is not the direction mojang seems to be taking it, seeing how they freeze registries. At least I, I am unsure about the rest of the team, don't see much merit in breaking their decisions, reloading is a pain already. If you are changing your plugin code on your production server so frequently that you need fast reload times, you are doing something wrong.
But to sum this up, as I feel like we are at least partially on the same page, at least to this specific issue: The current system is horrible for custom enchantments and what your or other libs are doing is hacky af. Pulling this would at least allow the hack on its verge of death to make it past the finish line until we can properly allow custom enchantments through registry manipulation, at which point, hacky modifications to an API registry may very well not work anymore by design. How customisable properly registered enchantments will end up being is a question/task for the future when we have the groundwork in place. Hopefully with a lot of community input. But once we have something usable, this specific hack may/should imo stop being supported by the server whatsoever to ensure the API and the ecosystem around it move forward instead of sticking to the current hack.
I think that's an excellent compromise and I'd be very happy allowing the current 'system' to limp onwards until something better can be done
That is already what you are doing tho. You are hijacking a system (the vanilla enchantment system/the bukkit) to pretend what you are creating and applying are enchantments when they really are not, at least not in the vanilla sense of an enchantment, which is what the API aims to expose. The API (in its current state) is not there to allow for custom enchantments.
I completely understand what you mean - however, it's about what the client understands. Conceptually, an enchantment is really just any ID and level that sits in the enchantments part of NBT; the client will glow items if there is something there, for example.
I also look forward to the implementation details on a future API for this when the time comes!