Fix NBT enchantment issue on 1.13 clients

[embeddedt]

This patches a regression introduced by #22.

[IdanHo]

this is not an appropriate fix, it was missing an undefined check, not adding in the old implementation, if

nbt.simplify(boots.nbt).Enchantments

is undefined,

boots.nbt.value.Enchantments

will be as well

[embeddedt]

@IdanHo Sorry about that. I hadn't investigated the code in detail and had assumed that the two were different. I've changed it to just check whether enchantments is undefined.

[IdanHo]

that will indeed fix the crash, but im wondering why on 1.14+ enchantments are always there, even if there are none applied, and on 1.13- there are not, i wonder if its called "Enchantments" on older versions, or if it was changed to it on 1.14. do you mind console.loging the output of simplify on the version youre on? (i think you said 1.8.9)

[embeddedt]

This is the output right before the crash happens:

{ Unbreakable: 1, display: { color: 65535 } }

The boots aren't enchanted. To test the NBT data when they are enchanted I added Protection 4 to them.

{
  ench: [ { lvl: 4, id: 0 } ],
  Unbreakable: 1,
  display: { color: 65535 }
}

So it looks like it was called ench in 1.8, and (according to the wiki) in 1.13 it was renamed to Enchantments. What this suggests is that neither of the two versions ever worked the way they were supposed to on 1.12- servers, but the old code checked whether the tag existed before accessing it, preventing an outright crash.

I pushed another fix; I think this should cover all the cases.

[embeddedt]

I may have found another case where enchantments won't work as expected in mineflayer-pathfinder.

Do you think it would make more sense for prismarine-nbt to normalize 1.8's ench property instead of checking for it in every package?

[Karang]

prismarine-nbt no, but mineflayer yes ! When it receives the item from the server it should make sure the api is consistent accross versions.

[embeddedt]

If the check isn't done inside prismarine-nbt, doesn't that mean that we need duplicate Enchantments || ench checks in a lot of different PrismarineJS packages?

[Karang]

prismarine-nbt is a package that only knows how to encode / decode nbt. The nbt data are used for a lot of things, not only for items. mineflayer is the package in charge of interpreting what the server is sending, IMO it should be the one that does the check. If you do it in mineflayer you'll not have to do it in the other packages that take their data from mineflayer (like mineflayer-pathfinder or prismarine-physics)

Edit: or it could be made in https://github.com/PrismarineJS/prismarine-item

[embeddedt]

prismarine-item looks like a good spot; I assume the check should be placed here?

[Karang]

Yes, but also in the fromNotch/toNotch methods