Enchantments implementation

[Tonodus]

This adds basic enchantment functionality.

This PR includes:

• Serialization (map and nbt) of enchantments (see #77 )
• GlowMetaEnchantedBook (implements EnchantmentStorageMeta) (see #77)
• Saving and loading of the xpSeed tag (in GlowHumanEntity)
• Enchantment and level calculation for 1.8
• Enchantment Table GUI (see #1)
• EnchantItemEvent and PrepareItemEnchantEvent (see #28)

This PR does NOT include:

• Enchantments created by anvils or commands
• the enchantment's functionality (a tool enchanted with efficiency will never lost this enchantment again, but it behaves like a 'normal' tool)

https://github.com/GlowstoneMC/Glowkit/pull/63

[dequis]

Noice. Thanks! Might be a bit too big though.

[Tonodus]

@dequis Sadly... However, the diff grew due to the new classes, which might be easily to review.

[turt2live]

I don't think you need a new class for every enchantment. Please consider a different system.

[Tonodus]

@turt2live IMO it's cleaner so, since vanilla does it either and enchantments might be extended in later versions. However, you can check out the branch without c90727a to see the diff without the new classes.

[dequis]

Sooo that would be this: https://github.com/GlowstoneMC/Glowstone/compare/1a283e6

Which is basically the same but with inline classes, and a diffstat of +954 -51 instead. I guess the functions are needed because that behavior seems arbitrary.

[turt2live]

I'd be inclined to accept the system in this diff if the classes actually did something. Currently they act as property containers which is not ideal at all. There are other systems, such as simple variables, that would better suit the excessive duplication and lack of behaviour.

[turt2live]

Also, the methods described by @dequis is just as bad. I do not think that enchantments need a large diff in either case...

[Tonodus]

@turt2live I'd be glad to know a better method, but if Glowstone should have nearly vanilla behaviour, the methods are needed and so are the (inner) classes.

[turt2live]

The methods are currently only needed due to the direction taken to have them there. I honestly do not feel as though the system proposed is adequate to solve the problem at hand.

About half of the enchantments use constant values, which makes them easily solvable. The other half use a formula, which is also easily solvable. Both can be solved with a simple mapping and the existing enum. There is no need for 2 methods for every enchantment, you should be able to get by with 2 formulas (although some would be "constant") for every enchantment that can be defined outside of the enum, but be accessed from the enum.

[dequis]

Function bodies with all the crap around them removed:

10 * modifier;
getMinRange(modifier) + 30;
1;
41;
10 + 20 * (modifier - 1);
super.getMinRange(modifier) + 50;
modifier * 10;
getMinRange(modifier) + 15;
5 + 20 * (modifier - 1);
super.getMinRange(modifier) + 50;
10 + 20 * (modifier - 1);
super.getMinRange(modifier) + 50;
15 + (modifier - 1) * 9;
super.getMinRange(modifier) + 50;
1 + 10 * (modifier - 1);
super.getMinRange(modifier) + 50;
15;
super.getMinRange(modifier) + 50;
5 + (modifier - 1) * 8;
super.getMaxRange(modifier) + 50;
15 + (modifier - 1) * 9;
super.getMinRange(modifier) + 50;
1 + (modifier - 1) * 10;
getMinRange(modifier) + 15;
12 + (modifier - 1) * 20;
getMinRange(modifier) + 25;
20;
50;
20;
50;
15 + (modifier - 1) * 9;
super.getMinRange(modifier) + 50;
15 + (modifier - 1) * 9;
super.getMinRange(modifier) + 50;

Sounds like all lineal mx+b stuff. The getMinRange/getMaxRange ones can be replaced by the corresponding constants of the other implementation.

Only ones that don't seem to fit are the ones that do 10 * modifier (instead of modifier - 1)

[Tonodus]

Actually you're right. Seems easily possible with 4 different formulas: https://github.com/Tonodus/Glowstone/commit/3c97799d311622ce0a4a03b8574fca93c1ba9800 Looking at it into detail tomorrow.

[Tonodus]

I end up with two "formulas" and a minimzed diff.

[turt2live]

After lots of testing and a few hours I came up with a slightly smaller diff with a bit less complexity as well. I haven't tested the pull request though as I'd like you to review my changes that I've PR'd to your branch before I do so. If you have any questions, please ask.

[Tonodus]

Seems, like you did strange things in your PR: Why did you remove all the maxLevel values in the constructor calls, so that the previous weight value is now the maxLevel? That causes the PR to fail in the test (maxLevel > 5). Additionally, you added these removed values as last parameter (maxInc). I think you didn't do this with this in mind, so I tried to revert your changes, please review them.

I also think you mixed up values for Smite and Bane of Arthropods, I think I fixed them, not sure either.

So, after some investigation into getMaxRange there are some differences: f.e. Thorns:

• your's: getMinRange(m) + 50 <=> (10 + (m -1) * 20) + 50 <=> 60 + (m -1) * 20
• vanilla: super.getMinRange(m) + 50 <=> (1 + m * 10) + 50 <=> 51 + m*10 <=> 61 + (m - 1) * 10

So it seems, that you cannot re-use getMinRange everytime.

See: https://github.com/Tonodus/Glowstone/compare/Turt2Live-DEV:GS591-1...ttl_impl

[turt2live]

I did screw up the ordering, and I've fixed that now.

As for the formula, I use the modified enchanting levels table as described in the enchanting mechanics. The output table from Glowstone is this with the test program output being this. They both match each other and the wiki article. The test code used was this added to the main method of Glowstone with the workaround of fields and classes being made public where required (this would be the GlowEnchantment class after those modifications).

I looked at the code flow and the modified enchantment level is passed down through the methods, meaning that the formulas I use are correct in being used.

[Tonodus]

Well, I can only guess that the wiki page is outdated (source is mc 1.4.6 allegedly) or simply false. See f.e. the values for level 1 for Knockback: 5 - 55. The used formula

• min: 5 + 20 (i - 1) where i = 1 for level 1.
• max: min + 50

Now compare to mc-source (see https://github.com/Bukkit/mc-dev/blob/master/net/minecraft/server/EnchantmentKnockback.java)

-min: public int a(int i) {...} (correct) -max: public int b(int i) { super.a(i) + 50; }

==> false, because super.a(int) refers to 1 + i * 10 (see Enchantment.java), therefore the correct range would be 5-61 instead of 5-51.

I don't know whether this deviation might be appreciably (open for suggestions), however it's not 100% correct.

[Tonodus]

@turt2live After a long respite I merged your PR and added a parameter simpleRange to cover the different maxRange options. The behaviour should be identical with vanilla now.

[turt2live]

Thanks @Tonodus. I've been meaning to test whether or not that pull request was/is valid but haven't had a chance to actually pull the values from vanilla yet. I'll do that as soon as I can.

[Tonodus]

@turt2live The PR is still tagged with 'Need Changes', any updates from your side?