fix an oopsie in chunk caches with dynamic world height

[wagyourtail]

fixes #mine chest and some stuff not working properly

[Rorschach3533]

You guys gonna update this or are we going to change something ?

[leijurv]

I explained how it should be done here https://github.com/cabaletta/baritone/commit/05cc63f4ffc4d16d1024c8f7f4806f8679f6f929

I'd like to see something like LEGACY_REGION_MAGIC = 456022910; and CACHED_REGION_MAGIC = 456022911;. When loading, switch on that value and do something like boolean yCoordinateIsAByte = magic == LEGACY_REGION_MAGIC;, then int Y = yCoordinateIsAByte ? in.readByte() & 0xff : in.readInt() + dimension.minY();

Also I don't see why we need to add and subtract out dimension.minY(). It seems simpler to just directly write the actual value?

That should only happen on read. Writing a chunk should always do it the new way with the new magic.

[wagyourtail]

Also I don't see why we need to add and subtract out dimension.minY()

yeah, I suppose that doesn't need to be there as it'll be signed when read/writing, it will match the y value of the chunk data tho, since those are -minY to be able to shove into a long value

[wagyourtail]

I'd like to see something like LEGACY_REGION_MAGIC ...

I'm not sure if this is a good idea, this might cause issues with people who have old cache data that is wrong (incorrect y value currently from chunks parsed with worlds with -ymin values). it's good I guess for loading cachedata from 1.17/older worlds that didn't previously have the minY, but iirc we're now saving the cache data in a folder name based on the worldheight, so it'll always have been wrong

[leijurv]

we're now saving the cache data in a folder name based on the worldheight

mmmmmmmmmmmm I don't like that, with a patch to saving the Y coord I don't think that's a good idea anymore, want to undo that afterwards

this might cause issues with people who have old cache data that is wrong (incorrect y value currently from chunks parsed with worlds with -ymin values).

I actually don't think this would cause any issues. Imagining you do #goto chest or some such, and it's out of your render distance, the Y coordinate doesn't matter in practice (and if simplifyUnloadedYCoord is left at default, it doesn't matter at all). Once it comes into your render distance, incorrect Y coordinate data in the cache is overwritten with the true data from the actual chunks, so there is no issue.

Really, thinking on it now, the XYZ coordinate within the chunk of the interesting block doesn't matter lol. Saving it in RAM is good, but saving it on disk is a little pointless. I think we could get away just as well with the on-disk format just containing the quantity of blocks in the chunk, not their locations?

[leijurv]

tldr: given that the XYZ doesn't matter so much, my opinion is that it's much better to scrape together as much data as we can from older or incompatible caches, than to ignore it because its Y could be incorrect.

[wagyourtail]

Really, thinking on it now, the XYZ coordinate within the chunk of the interesting block doesn't matter lol. Saving it in RAM is good, but saving it on disk is a little pointless. I think we could get away just as well with the on-disk format just containing the quantity of blocks in the chunk, not their locations?

goto spawner/chest isn't working properly without cache, so might need to fix a few things to do that...

[leijurv]

goto spawner/chest isn't working properly without cache, so might need to fix a few things to do that...

Reread what I said - "Saving it in RAM is good, but saving it on disk is a little pointless."

If it's in the CachedChunk.specialBlockLocations, which it will be for a chunk in render distance that just came out of the ChunkPacker, then it will work just fine for goto spawner. If it's out of render distance, the Y coordinate will be unknown, as well as the X and Z within the chunk, but that's fine since if it's out of render distance we don't need to know where it is more precisely than what chunk it's in.

So I'm saying that actually I'm pretty sure goto spawner/chest would work perfectly without saving the XYZ of chest/spawner to disk. Just whether they're in the chunk or not (and maybe a quantity).

[leijurv]

Update: to be clear, packed CachedChunks SHOULD still store the XYZ list while in RAM. XYZ should only be discarded when written to disk, it should NOT be discarded when packed. The reason is scenarios like 2b2t spawn

[wagyourtail]

2b2t spawn is probably big enough it shouldnt be discarded on disk either... not like it takes that much space...