Review of region.py about nbt

[twoolie]

I'm not familiar with the region code, though I have been meaning to do some code cleanup for a while. It may be a good opportunity to revisit this code.

[macfreek]

I just pushed some test code to branch write-nbt-todo in my own repository. @Fenixin, @twoolie, (and any other listeners) could you do me a favour? Run the examples/region_util.py script on all your saved files.

I.e.:

git remote add macfreek https://github.com/macfreek/NBT.git
git checkout macfreek/write-nbt-todo
examples/region_util.py ~/Library/Application\ Support/minecraft/saves/*/region/*.mc*

(or wherever your saved files are located; I'm obviously using a Mac)

I'm interested in all notes about the size of the chunk versus the number of allocated sectors.

E.g.:

chunk 21,12 is 4096 bytes (4+1+4091) and requires 1 sectors, but 2 sectors are allocated
chunk 31,30 is 8192 bytes (4+1+8187) and requires 2 sectors, but 3 sectors are allocated
chunk 11,21 is 4096 bytes (4+1+4091) and requires 1 sectors, but 2 sectors are allocated
chunk 15,15 is 4094 bytes (4+1+4089) and requires 1 sector(s), and indeed 1 sector(s) is/are allocated
chunk 4,3 is 4095 bytes (4+1+4090) and requires 1 sector(s), and indeed 1 sector(s) is/are allocated
chunk 8,24 is 4096 bytes (4+1+4091) and requires 1 sectors, but 2 sectors are allocated

As expected, a chunk of 4090 bytes (thus reported "length" value in the chunk block header of 4091, and a block of 4095 bytes) uses 1 sector. And a chunk of 4092 bytes (thus reported "length" value in the chunk block header of 4093, and a block size of 4097) uses 2 sectors. No surprises there.

However, a chunk of 4091 bytes (thus reported "length" value in the chunk block header of 4092, and a block of 4096 bytes) uses 2 sectors, while I would have expected 1 sector.

Am I missing something here (is there a compulsory \0 byte at the end of each compressed chunk perhaps?) Or is there a off-by-one error in the Minecraft code? Or is there a bug in my code somewhere?

Note that I see this behaviour consistently for region files written by Minecraft and at least WorldDowloader (a mod that stores chunks received from a server)

[macfreek]

Just added 52 unit tests for region.py 9aeb394. The good news is: 18 tests already succeed, and it is fairly easy to increase that to 29. The bed new is: the remain 23 unit tests are harder to fix.

[macfreek]

Question: if there is a corrupt chunk in a region file, it is now almost arbitrary what kind of exception is raised: nbt.region.ChunkDataError, nbt.nbt.MalformedFileError, or zlib.error.

There are 3 types or errors here:

• unknown compression
• decompression error in zlib or gzip
• data format error in nbt.nbt.NBTFile

I'm almost inclined to turn this into: nbt.region.UnkownCompression, native zlib/gzip exception, and nbt.nbt.MalformedDataError. (with ChunkDataError an alias of UnkownCompression, and MalformedFileError an alias of MalformedDataError for backward compatibility).

An alternative to the native zlib/gzip exceptions is to catch these exception and reraise them as a nbt.region.CompressionError. I personally like that, since most users don't care what's exactly wrong while decompression, just that the chunk is corrupt. However, the process of catching an exception and reraising as something different seems very uncommon in Python. (The Ruby solution would be to open the zlib.error and gzip.error classes and make subclasses of CompressionError, but I don't think this is possible or recommended in Python).

Suggestions?

[macfreek]

Review completed.

Rewrite completed. See #68.

[Fenixin]

I'm sorry I didn't show up before, I was more or less out for a month and after that I've been catching up with everything slowly.

It seems this thread is pretty outdated now and that lot's of changes have been done, so I guess I'm pretty late to answer/do anything. Anyway, anything I can do/answer right now?

[macfreek]

Hi Fenixin, welcome back! :)

Have a look at #69 and see if the changes are useful to you, in particular if the backward compatible changes are OK.