SmylerMC / litemapy

Litemapy's goal is to provide an easy to use way to read and edit Litematica's schematic file format
GNU General Public License v3.0
51 stars 5 forks source link

Added converter from Litematica Region to Sponge v2 schematic #14

Closed boscawinks closed 2 years ago

boscawinks commented 2 years ago
  1. Added converter from Litematica Region to Sponge v2 schematic (used by WorldEdit) and vice versa.
  2. Litematica removed the id tag from TileEntities, so I removed it and the RequiredKeyMissingException from those.
  3. Fixed a bug in the NBT structure converter which caused entity positions to be calculated incorrectly for Litematica Regions with negative dimensions.

Sorry for putting so much stuff into one PR, but it kind of accumulated since the previous PR.

boscawinks commented 2 years ago

Actually, the bug also affects Regions with positive dimensions, so this should be fixed ASAP! I thought I tested it before the last PR but I must have messed it up. Sorry for that!

SmylerMC commented 2 years ago

About the position calculation, it might be worth writing helper functions and unit tests to unsure everything keeps working in future versions.

SmylerMC commented 2 years ago

It might be a good idea to make BlockState#__repr__() use your new BlockState#to_block_state_identifier() method instead of the current implementation.

boscawinks commented 2 years ago

Fortunately, I have to learn how to do test in Python for work anyway, so I could look into this in the future. For this PR I would leave it as is.

boscawinks commented 2 years ago

It might be a good idea to make BlockState#__repr__() use your new BlockState#to_block_state_identifier() method instead of the current implementation.

Sure, but I don't have the overview to foresee what this might break.

SmylerMC commented 2 years ago

It might be a good idea to make BlockState#__repr__() use your new BlockState#to_block_state_identifier() method instead of the current implementation.

Sure, but I don't have the overview to foresee what this might break.

Hum, I hadn't though of that, do you think anyone relies on __repr__'s output in any way ? That would be a weird use case but it is possible.

boscawinks commented 2 years ago

It might be a good idea to make BlockState#__repr__() use your new BlockState#to_block_state_identifier() method instead of the current implementation.

Sure, but I don't have the overview to foresee what this might break.

Hum, I hadn't though of that, do you think anyone relies on __repr__'s output in any way ? That would be a weird use case but it is possible.

I was worried more about internal use, but after a quick search I couldn't find any

boscawinks commented 2 years ago

Shouldn't the program exit when the id of the entity isn't defined? Litematica only removed the id for TileEntities not Entities.

boscawinks commented 2 years ago

Ah, my bad. I wasn't sure if the exception would automatically terminate the program. Which it will. Will remove it.

SmylerMC commented 2 years ago

Shouldn't the program exit when the id of the entity isn't defined? Litematica only removed the id for TileEntities not Entities.

Calling exit() yourself is a rather violent way to deal with it, especially in a library. That would terminate the entire program without any chance to recover. Raising the exception is more than enough. It will interrupt the execution until an except block is reached in the stack, giving a chance to the code that called the method to deal with the exception by, for example, displaying a nice error message to the user. In fact, here exit is never called, as the raise instruction immediately stops the interruption of the program. If the exception is not caught by an except block, python will simply show the error message and stack trace and terminate.

Here is a good if you want to improve or refresh your understanding of exceptions.

boscawinks commented 2 years ago

Here is a good if you want to improve or refresh your understanding of exceptions.

Thanks for the pointers! I removed the call.