Use prismarine-nbt to cleanly handle nbt + fix post-flattening enchants

[IdanHo]

This change uses prismarine-nbt to simplify the object down to a common format. I've also found that on mc 1.14+ the "id" property of the enchant is now a string in the format of "minecraft:enchant_name", so ive added support for that as well. Unfortunately this is still crashing, as it seems like no enchantments data was specified post 1.13.2 in minecraft-data, so enchantmentsByName is null on those versions. This can be fixed for versions 1.14-1.15.2 just by adding the datapath to the 1.13.2 enchantments data (as no enchantments were added in those versions), and can be fixed for 1.16+ by PrismarineJS/minecraft-data#325, as new enchantments were added in that version. Once minecraft-data is fixed and this is merged, this should fix PrismarineJS/mineflayer#1270

[nickelpro]

I'll prioritize working on the mcd half of this. I got derailed by the same discovery of the string-based identifiers, since I was hoping to get away with a simple extractor.

[IdanHo]

not sure what the best way to do that in prismarine-physics? if this was in the mineflayer side we could have launched a minecraft server, made it connect, and gave it boots

[rom1504]

Look at the existing test. The idea is to add a similar one with different player state. For example in this case you want to test with a player state with boot and check it doesn't crash. Will run much faster than a end-to-end test like in mineflayer too

On Sat, Aug 22, 2020, 19:34 Idan Horowitz notifications@github.com wrote:

not sure what the best way to do that in prismarine-physics? if this was in the mineflayer side we could have launched a minecraft server, made it connect, and gave it boots

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/PrismarineJS/prismarine-physics/pull/22#issuecomment-678669278, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR437RZPZMCYGXIWJNLTWLSB76RHANCNFSM4QIC2OIA .

[Karang]

Look at the existing test. The idea is to add a similar one with different player state. For example in this case you want to test with a player state with boot and check it doesn't crash. Will run much faster than a end-to-end test like in mineflayer too

While adding this kind of tests is good, in this case, it would not have caught the issue. The boot issue is comming from the fact that the enchantment format changed from one version to an other in mineflayer. Because the test would have mocked up this part, the issue would not have been caught.

[rom1504]

What I'm saying is we should add a regression test specifically for this case: having or not enchantment on boots, with an array or not. That's a simple thing to add for now, to improve the tests.

In a second step (not included in this PR) we could think how to add more generic tests for this package to add tests cases for all the player state that is used to catch all possible incorrect state handling (that I'm sure will happen in non vanilla even if vanilla doesn't have the problems)

On Sat, Aug 22, 2020, 21:16 Karang notifications@github.com wrote:

Look at the existing test. The idea is to add a similar one with different player state. For example in this case you want to test with a player state with boot and check it doesn't crash. Will run much faster than a end-to-end test like in mineflayer too

While adding this kind of tests is good, in this case, it would not have caught the issue. The boot issue is comming from the fact that the enchantment format changed from one version to an other in mineflayer. Because the test would have mocked up this part, the issue would not have been caught.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/PrismarineJS/prismarine-physics/pull/22#issuecomment-678680860, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR437QOJ4SIH7SURAXAGY3SCAKQVANCNFSM4QIC2OIA .

[Karang]

Ok, I understand your point. The way I see it, there are 2 approach to this problem:

• prismarine-physics should not crash whatever we throw at it (in that case we keep adding special cases and regression test each time mojang change the format)
• we decide on an interface (eg enchantments should be an array) and adapt mineflayer, flyingsquid etc.. to format the data correctly. In that case, the regression test should be added in those packaged not in p-physics

I'm more for the second solution, to keep prismarine-physics as simple as possible. In my original design, I wanted to have PlayerState https://github.com/PrismarineJS/prismarine-physics/blob/master/index.js#L559 be that interface. For now, the only user of p-physics is mineflayer so PlayerState assume the parameter is a mineflayer bot but this was only for convenience. When we'll use pphysics also in other packages, PlayerState would have to be reworked or have an other implementation. I'm thinking it would make more sense to move the PlayerState class out of pphysics.

[rom1504]

Good points. I answered in advanced-coders room

[rom1504]

https://github.com/PrismarineJS/prismarine-physics/issues/23 conclusion here Tests for handling translation issues will go to mineflayer

[embeddedt]

This is crashing when connecting to Spigot 1.8.9 using the current version of mineflayer on npm. The crash occurs because nbt.simplify(boots.nbt).Enchantments is undefined. Reverting to the previous code appears to fix the issue.

[rom1504]

@embeddedt can you fix it by adding some if there (and PR) ?

[embeddedt]

@rom1504 I've opened #26 to address this.

[rom1504]

@embeddedt thanks, merged and released