search

NavidK0 / Carbon

Carbon is a Spigot plugin which turns a spigot protocol hacked server (on 1.7.10) into a 1.8 server.
GNU Lesser General Public License v3.0
35 stars 19 forks source link

Fix enchanting level passed to EnchantItemEvent #139

Closed Jikoo closed 10 years ago

Jikoo commented 10 years ago

Addresses #138, didn't realize how simple this was to fix when I opened the issue.

Jikoo commented 10 years ago

Issue: The number returned now by EnchantItemEvent.getExpLevelCost() is the explicit level cost. While I agree that this makes sense because of the method name, it does not provide good compatibility with the existing system and breaks plugins that use the API to modify enchantments. It should instead provide the level being enchanted at, the shelf-based number displayed on the right of the button.

The only way to obtain the effective enchanting level right now is to A. preserve it from the PrepareItemEnchantEvent B. recalculate it, which requires finding and counting shelf blocks, then recalculating random values based on the number of shelves These aren't great options, it would be much better for Carbon to handle it.

Shevchik commented 10 years ago

But now we don't care about plugin modifying the required levels. Not sure if we should do it or not.

Jikoo commented 10 years ago

No, the issue is that plugins modifying enchantments being applied cannot fetch the actual level being enchanted at. It returns lvl 3 because button 3 is pressed. Who knows how many shelves there are or what level the button represents? The actual level cost is 3, yes, but button 3 can be a lvl 1-30 enchantment based on the number of shelves present.

CB handles the level reduction, plugins should not be doing that.

For example, I have a plugin that allows you to enchant furnaces. I uncancel the PrepareItemEnchantEvent and set the levels of the table, then I fetch them and calculate the enchants to apply in the ItemEnchantEvent. With the current system, I can either use NMS to fetch costs from the table directly, or I can use the event which previously returned the correct value, resulting in all enchants just being the level of exp that they cost despite the actual level the item should be enchanted at.

See https://gist.github.com/andrepl/5522053 for a very basic example of proper usage of this event that is broken by the change to the ItemEnchantEvent.

NavidK0 commented 10 years ago

Merging, plugin compatibility resolved with this pull request.