Discussion for the "Better 1.18.x support" PR

[JorelAli]

PR #354 is getting very cluttered with arbitrary comments, I'd like to bring all of those to here so we don't pollute the PR with ongoing development ideas and bug fixes - I'd rather keep the PR specific to "does it compile", and code reviews.

@AgileGhost @Ravendwyr

I'm currently busy for the next couple hours, but in that time, it would be nice to know if my dev branch actually fixed the inventory saving issue or not. I've made an unstable release here - if you've got time and are willing to try it out, getting feedback on whether this fixes inventory issues would be very helpful!

[AgileGhost]

I was able to confirm that inventories are now able to save and I was able to see the info messages now confirming they saved. But item frames are now broken? They didn't appear in the save. Oh well. Anyways, best of luck! I need to sleep XD

[JorelAli]

I was able to confirm that inventories are now able to save

Lovely! It's almost certain that the additional chunk updating is necessary then. Will make sure to carry that over into my PR.

But item frames are now broken?

It's possible that this is due to overwriting an existing item frame and it's something that I haven't figured out how to fix. When you were downloading this world, did you delete any previous copies (e.g. before you ran the new world downloader, did you delete any worlds which may have been saved by running previous world downloaders) first? If not, I think there's a case where the world downloader tries to write an entity alongside an existing entity, and Minecraft can't handle duplicate entity UUIDs, so (I presume) it decides to delete them both.

In simpler terms, running the world downloader sees and item frame with id A and saves it to the downloaded world. Running the world downloader again sees the item frame with id A again, so writes it to the downloaded world, but it doesn't delete the old A that it saw before, so the world now has two item frames with id A. This derps and Minecraft deletes both of them.

[AgileGhost]

When you were downloading this world, did you delete any previous copies (e.g. before you ran the new world downloader, did you delete any worlds which may have been saved by running previous world downloaders) first?

I downloaded the world with a fresh start, but I did fly away and come back to double up the chunk saving. (idk if it actually does since I've hardly decompiled the code in my head but I just can't be bothered XD. Also simple terms aren't needed, I can understand)

[JorelAli]

Flying away and flying back should work as intended (Implemented via loadPreviousInventoriesAt())

[Ravendwyr]

Inventories are working here as well in 1.18.2 with the unstable build, and glowing item frames appear okay too (I didn't notice non-glowing ones). I did notice this error in the console window though but I wasn't paying attention to when it happened sadly.

java.lang.NullPointerException: Cannot invoke "String.length()" because "str" is null
        at java.base/java.io.DataOutputStream.writeUTF(DataOutputStream.java:359)
        at java.base/java.io.DataOutputStream.writeUTF(DataOutputStream.java:333)
        at se.llbit.nbt.StringTag.write(StringTag.java:57)
        at se.llbit.nbt.StringTag.write(StringTag.java:53)
        at se.llbit.nbt.NamedTag.write(NamedTag.java:70)
        at se.llbit.nbt.CompoundTag.write(CompoundTag.java:68)
        at se.llbit.nbt.NamedTag.write(NamedTag.java:70)
        at se.llbit.nbt.CompoundTag.write(CompoundTag.java:68)
        at se.llbit.nbt.ListTag.write(ListTag.java:71)
        at se.llbit.nbt.NamedTag.write(NamedTag.java:70)
        at se.llbit.nbt.CompoundTag.write(CompoundTag.java:68)
        at se.llbit.nbt.NamedTag.write(NamedTag.java:70)
        at game.data.chunk.ChunkBinary.fromChunk(ChunkBinary.java:63)
        at game.data.chunk.ChunkBinary.entitiesFromChunk(ChunkBinary.java:97)
        at game.data.region.Region.lambda$toFile$0(Region.java:150)
        at java.base/java.util.concurrent.ConcurrentHashMap$KeySetView.forEach(ConcurrentHashMap.java:4706)
        at game.data.region.Region.toFile(Region.java:127)
        at game.data.WorldManager.save(WorldManager.java:433)
        at util.ExceptionHandling.attempt(ExceptionHandling.java:12)
        at util.ExceptionHandling.attempt(ExceptionHandling.java:22)
        at game.data.WorldManager.lambda$start$2(WorldManager.java:410)
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
        at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
        at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)

[JorelAli]

Inventories are working here as well in 1.18.2 with the unstable build

Yay! Will make sure to carry over the fix into my PR.

I did notice this error in the console window though but I wasn't paying attention to when it happened sadly.

Unfortunately, I can't figure out what caused it - all I know is that some entity has some incorrect NBT data. Is it possible that there are modded entities (e.g. entities with custom type names, not like minecraft:zombie)?

[Ravendwyr]

It's very possible yes. I saw a Wither Skeleton named "Skeletal Knight" wearing iron armour so maybe it was that? I never noticed this with the stable build though.

[Ravendwyr]

When you were downloading this world, did you delete any previous copies (e.g. before you ran the new world downloader, did you delete any worlds which may have been saved by running previous world downloaders) first?

I'll admit, I've never done this. I've always used the same downloaded world with different downloader jars. I make a copy of these to muck about with in single player though.

[JorelAli]

I saw a Wither Skeleton named "Skeletal Knight" wearing iron armour so maybe it was that?

It shouldn't be that. Who knows?

[JorelAli]

Released stable build 1.14.13 on my fork which should fix the inventory issue.

[Ravendwyr]

No luck yet I'm afraid. :(

[JorelAli]

Welp. Guess I'll have to actually test this one manually

[Ravendwyr]

Take your time, I can wait. I'm supposed to be at work right now anyway. >_>;

[JorelAli]

@Ravendwyr Fixed now, see here!

[JorelAli]

Released build 1.14.14 on my fork which should fix the inventory issue. The issue was caused by doing packet reading in the wrong section (chest opening packets are from client to the server, and I was putting that code in a place that only reads packets from the server to the client).

Also fixed the bug by @AgileGhost mentioned in the PR revolving around the following error:

java.lang.ArrayIndexOutOfBoundsException: arraycopy: last source index 10 out of bounds for byte[2]
        at java.base/java.lang.System.arraycopy(Native Method)
        at packets.DataTypeProvider.readByteArray(DataTypeProvider.java:90)
        ...

This was due to the use item packet not having any information about coordinates of what you interacted with. This may break support for inventory saving for previous versions of Minecraft.

[Ravendwyr]

We're making progress! Double chests don't save the top three rows though (or I need to open the chest twice; one on each side?) Player inventory also doesn't save but that can be worked around with chests.

Beds are invisible again, and chests don't appear to keep their inventory when restarting the downloader. Not sure if the latter is deliberate though. Chest inventories and mob entities on 1.16.5 are broken as suspected, but that could be worked around by using previous versions.

[JorelAli]

Double chests don't save the top three rows though (or I need to open the chest twice; one on each side?)

Double chests should save all chest contents when opening any side of the double chest. I've tested this and can confirm that it does work (sometimes). If the double chest is on the border of two chunks (i.e. one side of the chest is in one chunk and the other side of the chest is in another chunk), it's very likely that it won't work. This can be rectified by opening the chest twice, one on each side.

Player inventory also doesn't save but that can be worked around with chests.

I didn't add support for player inventory saving in this PR. I know it's been requested a lot (#115, #317), but I don't plan to look into this for this PR (maybe in the future?).

Beds are invisible again

I'm aware. I don't know how to fix this. I've tried.

chests don't appear to keep their inventory when restarting the downloader. Not sure if the latter is deliberate though.

It's not really "it's deliberate", but more "it's really difficult not to do this". I'm aware of this and I believe it's been broken since forever. This downloader tool appears to work along the lines of:

1. Read incoming server packets
2. (Interpret them and then) write those packets to a world file

Except in order to support saving inventories when restarting it would have to do something along the lines of:

1. Read the entire existing locally downloaded world into memory
2. Read incoming server packets
3. (Interpret them and then) write those packets to a world file (as well as the existing locally downloaded world)

Of course, there are alternatives (e.g. having a local file keeping track of these or something), but it sounds like a pain and would need time and thought to do it properly.

Chest inventories and mob entities on 1.16.5 are broken as suspected, but that could be worked around by using previous versions.

I don't plan to test 1.16.5 support for this PR.

[JorelAli]

Beds are invisible again

@Ravendwyr I think I just figured out how to solve beds.

[JorelAli]

Beds fixed in 1.14.16

[Ravendwyr]

Whoops, sorry, I fell asleep. I'll play with the new tags later today.

[JorelAli]

There are some "low hanging fruit" left that I think I can tackle and shove into this PR while I'm at it, and I want to give a shot at doing those:

• Dropped items doesn't saving #316 - It's actually important that this is implemented properly because by default, dropped items have an ID of 0 (air), and Spigot servers complain (you can't drop "air" onto the ground) when you try to start up a world with dropped items (I think)

• Program doesn't download books stored in lecterns #298 - I've already added some lectern support, but there's some bugs left that I've seen (not sure how reproducible, but I know they exist) and think it's worth ironing those bugs out if I can

• Make it so that command block commands are stored #245 - Assuming this is similar to handling containers (on interact, store command block data), I should implement this. Command blocks are also block entities:

  "minecraft:command_block": {
      "protocol_id": 21
  },

  It shouldn't be too difficult to implement this.

• Save Villagers #234 - This is actually a lot of work to implement! I've implemented villager professions, types and AI, but implementing their actual trades will take a bit of time. I'm not sure if I'm going to make that a goal for this PR or not yet.

[JorelAli]

@AgileGhost Also worth noting, I added support for dropped items in some patch at some point. For some reason, I still get complaints by Spigot saying "cannot drop air" but I think it may be referring to other items in the world. Dropped item support should make dropped items persist forever (i.e. they shouldn't be under the 5 minute timer, so should stay dropped forever in the downloaded world)

[AgileGhost]

Okay, I played for a few hours and did all sorts of stuff. But I did notice a few things.

Chests saved most of the time but sometimes didn't save, I had a single chest that never saved for some reason and ender chests never saved even though they said it was recorded. I also made sure the chests that didn't save were fully in the same chunk as me to be sure, if that's important. But every other container seemed to work perfectly. Inventory didn't save too.

Command blocks worked perfectly, which is good. Didn't get to test lecterns but I could try if you wish. I also had a few chunks that saved funky, shown here but is likely due to something else. (The bottom img had no sand there originally.) I also saw dropped items saved. Villagers kept their profession fine and everything else seemed to work fine.

Out of the four fully vanilla servers, I tried it on. I only got these two errors, one (top one) was caused as I was wandering around a certain chunk and was unable to determine which chunk it was to see what could have been causing it, but it only seemed to happen in that one chunk. The other error message is unknown how I even got it, I was AFK at the time and came back to it. I believe that was all that happened, if I remember anything new I'll let you know.

java.lang.RuntimeException: Invalid VarInt found! Packet structure may have changed.
        at packets.DataReader.readVarInt(DataReader.java:70)
        at packets.DataTypeProvider.readVarInt(DataTypeProvider.java:111)
        at game.data.chunk.version.Chunk_1_18.readChunkColumn(Chunk_1_18.java:90)
        at game.data.chunk.version.Chunk_1_18.parse(Chunk_1_18.java:56)
        at game.data.chunk.ChunkFactory.parseChunk(ChunkFactory.java:104)
        at game.data.chunk.ChunkFactory.readChunkDataPacket(ChunkFactory.java:121)
        at game.data.chunk.ChunkFactory.parse(ChunkFactory.java:81)
        at game.data.chunk.ChunkFactory.lambda$addChunk$1(ChunkFactory.java:68)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)
Chunk could not be parsed!

and 

java.lang.ArrayIndexOutOfBoundsException: Index 74 out of bounds for length 74
        at packets.DataTypeProvider.readNext(DataTypeProvider.java:76)
        at packets.DataTypeProvider$1.read(DataTypeProvider.java:191)
        at java.base/java.io.DataInputStream.readUnsignedShort(DataInputStream.java:344)
        at java.base/java.io.DataInputStream.readUTF(DataInputStream.java:595)
        at java.base/java.io.DataInputStream.readUTF(DataInputStream.java:570)
        at se.llbit.nbt.StringTag.read(StringTag.java:46)
        at se.llbit.nbt.NamedTag.read(NamedTag.java:58)
        at packets.DataTypeProvider.readNbtTag(DataTypeProvider.java:188)
        at game.data.entity.metadata.MetaData.parse(MetaData.java:32)
        at game.data.entity.specific.Villager.parseMetadata(Villager.java:38)
        at game.data.entity.EntityRegistry.lambda$addMetadata$7(EntityRegistry.java:107)
        at util.ExceptionHandling.attempt(ExceptionHandling.java:12)
        at util.ExceptionHandling.attempt(ExceptionHandling.java:22)
        at game.data.entity.EntityRegistry.lambda$addMetadata$8(EntityRegistry.java:103)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)
        ```

[JorelAli]

Thanks for the update!

ender chests never saved even though they said it was recorded.

Yeah... we don't support enderchest storage because that requires storing player data and we don't do that (yet?). I don't plan to support that for this PR (probably something for the future). I'm aware that the popup is misleading - it counts as a container and that popup appears when recording containers.

I had a single chest that never saved for some reason

Same... I'm not too sure what the issue is :/

Inventory didn't save too.

Not supported in this PR, due to player data storage not being supported. Can be mitigated by shoving your inventory into a chest for the time being.

I also had a few chunks that saved funky

I've noticed this too, but don't know what the cause is. It may be related to the SectionBlocksUpdate packet not working on 1.18, but I don't know and will have to get more info from @mircokroon and hopefully we can figure it out.

Command blocks worked perfectly

Glad to know that they work "well enough". I'm mildly aware that command block custom names don't save properly (if you put a command block in an anvil and rename it, then get it to run say ..., it is supposed to say [whatever_you_named_it_to] ..., but that doesn't get saved for some reason). I don't think it's a big deal at this stage, but it's worth noting down that this is an issue that exists.

[mircokroon]

SectionBlocksUpdate isn't used much on vanilla servers AFAIK, I think only for /fill and maybe some large redstone/liquid updates. Not sure what the cause might be.

[JorelAli]

@AgileGhost No need to test lecterns - I found the bug. When storing lecterns, I'm using written books which doesn't work for books that haven't been signed yet (i.e. writable books). Will fix this when I'm next available.

[JorelAli]

Released 1.14.17 which adds command block support for #245. (@AgileGhost)

[JorelAli]

@mircokroon I had a brief look into SectionBlocksUpdate, I think the cause is to do with rendered chunks. What happens when a block update occurs outside of rendered chunks? Staring at some logging for Hypixel joining, it appears it's trying to set a block in a chunk outside of render distance. I was able to reproduce this bug once using WorldEdit to generate stuff on the border of a visibly rendered chunk (can't reproduce it again, will keep looking into it).

In Chunk.java, there's a check in updateBlock() that checks if the section is null - what if the section isn't null, but instead is in some other invalid state?

SectionBlocksUpdate is used a lot in redstone updates by the looks of things - a very simple example is passing redstone to 3 pistons and activating it

[mircokroon]

Ahh, I think it's supposed to ignore changes outside of render distance anyway so maybe this is easy to fix, I'll look around a bit more

[mircokroon]

Seems to be another issue for parsing chunks with blocks above Y 192, thats 256 blocks above the bottom of the world so I imagine related to that (maybe some Y value stored as a byte somewhere?). Not sure why this would cause parsing issues though.

[JorelAli]

There are a lot of Y values being stored and casted to bytes all over the place:

• Chunk_1_18.createNewChunkSection(byte y, Palette palette)
• section = (ChunkSection_1_18) createNewChunkSection((byte) (sectionY & 0xFF), blockPalette); from Chunk_1_18.readChunkColumn
• updateBlock() in Chunk.java casts sectionY to a byte as well.

Changing these to int on its own doesn't seem to solve the issue - there's probably other things going on...

[mircokroon]

Yeah but none of these should have any affect on how many bytes are read or anything like that. Also if I add more blocks in the next chunk section up seems to work again? Kinda lost on this one, maybe I'll just add a try/catch and leave it for now, still seems to save everything just fine as far as I can tell.

[JorelAli]

The best part about development is the stuff you find during development that nobody else sees:

[mircokroon]

Capitalism gone too far, they're charging money for air now

[JorelAli]

@AgileGhost Adds villager trade saving in 1.14.19

[JorelAli]

@AgileGhost Adds horse, axolotl and sheep support in 1.14.20

[mircokroon]

Merged that, I'll make a release a little bit later today. Thanks again for helping out.

[JorelAli]

No problem, and thanks for all of your hard work on both making this whole thing in the first place and sorting out this PR.

[mircokroon]

Made a release. Commandblocks and villager trades are only on 1.18 right? Not sure if this page is 100% up to date now.

https://github.com/mircokroon/minecraft-world-downloader/wiki/Features

[JorelAli]

For now, it's safe to say that it's for 1.18 only. I've not tested 1.17 and below, so it wouldn't be safe to say that support for them have been implemented.