search

twoolie / NBT

Python Parser/Writer for the NBT file format, and it's container the RegionFile.
MIT License
362 stars 74 forks source link

More support of "flattened" Anvil format #103

Closed mfld-fr closed 5 years ago

mfld-fr commented 5 years ago
mfld-fr commented 5 years ago

Well, actually, I am not satisfied by this first change and I made more changes that I will push after more testing, so I put a WIP: in the MR title to hold it and to get that upcoming changes reviewed.

Not only I have to fix the circular dependency as you said, but I also have to work on the right way to handle block identifiers in a generic manner: current MC version uses alpha identifiers, while old versions used numeric ones. So I am trying to make a comprehensive API with alpha identifiers, and to hide the old numeric ones in case we are reading an old version, with a switch on the "DataVersion" tag.

Stay tuned !

mfld-fr commented 5 years ago

As expected, the Travis build failed because the test world is still an old version, and I have not yet put back the decoding of the legacy block data. Scheduled for a next commit.

macfreek commented 5 years ago

I'm happy to accept anything that passes all tests, including a change to remove outdated tests ;) The chunk code is very old, and I've seen few (if any) issues on it last years, so I suspect that most people use the examples. Any change should not break other code such as Minecraft-Region-Fixer, but I'm fairly sure it does not use the chunk part.

But I'm very happy to see this code, as it may make the library more useful, so just let me know when you think it is ready, and I try to merge after a review.

macfreek commented 5 years ago

A bit of clarification: this library is maintained for over 8 years now, and the core parts: nbt.nbt and nbt.region are fairly solid. I would write it differently if I were to write it today, but the code is fairly mature. nbt.world and nbt.chunk are nice, but have not matured that much. Some of it's functionality was gradually moved to examples, to note to users that that part was provided 'as is'.

I would be thrilled to see more functionality in the core, especially logic when it comes to undefined sections, as well as some helper functions for all the extra files (like scoreboard.dat). However, if you want to add a lot of very Minecraft-specific code, be sure to discuss it, to avoid adding maintenance code.

It's a pity the NBT format gained no traction outside the Minecraft world. I think it's fairly elegant. However, other 'dynamic binary data' file format have prevailed. The one am actively using at work is Google protobuf.

mfld-fr commented 5 years ago

The latest commit fixes the regression in the map unit test. Well... looks like we can now consider this latest version as a merge candidate. Here is the rendered McRegion test world:

nbtmcregionovr4kt8u

mfld-fr commented 5 years ago

The Travis build job 93.7 failed not because of the map unit test, but after the 10 mins timeout. See https://travis-ci.org/twoolie/NBT/jobs/493402436.

mfld-fr commented 5 years ago

Again the same world, but after commenting out the block_ignore array:

world_region 1

Hey, this project begins to be a real cheat tool :smirk: !

macfreek commented 5 years ago

At first glance, this seems great. Of course, I was sold at seeing these pictures 👍

I particularly like how to seem to have simplified the math code in examples/anvil_blockdata.py.

There are two (very) minor issues I see at first glance:

macfreek commented 5 years ago

Errrm, I need sleep. Travis did pass. I guess the (object) frase now really is redundant. Good, I learned something! With the small PEP8 changes, I would vote to merge this.

mfld-fr commented 5 years ago

According to Python documentation, object inheritance is required in 2.x only when using some modern class features (see https://docs.python.org/2.7/reference/datamodel.html#newstyle). As we should follow the life cycle of the official Python releases (today 2.7, 3.5, 3.6 and 3.7), we should keep that point in mind, even if the Travis build passed. So thanks for the tip, I was not aware of that.

mfld-fr commented 5 years ago

There is no "good" reason to diverge from PEP8, beyond some personal habits, so I will push a corrective commit on the spacing. Sorry for the waste.

mfld-fr commented 5 years ago

Fixed the spacing and object inheritance in the last commit.

mfld-fr commented 5 years ago

Again a Travis failure in only one job (95.5), while downloading the sample file, without any link with the latest commit: https://travis-ci.org/twoolie/NBT/jobs/493852303

ERROR    Download https://github.com/downloads/twoolie/NBT/Sample_World.tar.gz failed: [Errno 104] Connection reset by peer

No failure on my side with alltests and Python 3.6.5.

macfreek commented 5 years ago

@mfld-fr please report if you think your commits are good enough to push to master, and I'm happy to merge. I'm less responsive next four days. Feel free to ping using a '@macfreek' comment, and I'll pick it up within a few days.

mfld-fr commented 5 years ago

@macfreek : yes, I think the proposed code is now in a good state after the last commit related to performance, and that you could merge.

mfld-fr commented 5 years ago

Thanks for the merge !