Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parsing block_states with version greater than 1.18 #17

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

suzakuwcx
Copy link

Fix relate issue: #10, cite: https://minecraft.wiki/w/Chunk_format

But in contrast, this pr will cause that deprecating the support for old versions. So maybe creating a new branch is a better idea?

image

@MestreLion
Copy link
Owner

Thank you, this is such a welcome contribution!

Btw, is the difference only in the capitalization of names (sections vs Sections) or were there other structural changes? If it is, it might be easy to create a unified get_blocks() that is compatible with both the old and the new standard. Even some minor structural changes (palette / data being under block_state or directly in Sections) might be handled without too complex logic.

If not, I'd prefer the old function to remain intact, possibly provisionally renamed as get_blocks_old(), remove any compatibility logic in get_blocks(), and add documentation (as docstrings or plain comments) on which game versions each one operates.

In the future, if desired, get_blocks() may become a wrapper that automatically chooses the appropriate function based on the actual save file version.

@MestreLion
Copy link
Owner

Also, would you mind adding a "Fix #10" suffix to the commit message so that bug is properly closed and linked to this fix?

@suzakuwcx
Copy link
Author

Thanks, I would try your suggestion to make backward compatible and update suffix.

About difference, it is not the only capitalization, base on my experiment on 1.21 and wiki description, the different is:

  • Name change: "section" -> "Section", "BlockStates" -> "block_states", "Palette" -> "palette"
  • Structure change: "palette" now is under "block_state" (before is under "section")
  • If only one block in "palette", the "data" field will be remove
  • sometime a strange "Y=-5" Section will appear and this is useless

@suzakuwcx suzakuwcx marked this pull request as draft October 9, 2024 09:20
@suzakuwcx suzakuwcx marked this pull request as ready for review October 10, 2024 02:55
@suzakuwcx
Copy link
Author

suzakuwcx commented Oct 10, 2024

Now I modify get_blocks() as a wrapper and it will choose correct function to parse

I test the test case and now they goes well

blocks()
image

pretty()
image

@SorkoPiko
Copy link

When is this getting merged? I would love to see this implemented, as there are currently no working MC world manipulation python libraries.

Because that the index of palette array begins from 0, so the maximux
index is 1 less than palette length
@MestreLion
Copy link
Owner

@SorkoPiko

When is this getting merged? I would love to see this implemented, as there are currently no working MC world manipulation python libraries.

I didn't have the time to properly review or test it yet. The new block state format changes more than just renaming/reorganizing keys in the NBT structure, and this PR also deals with backward-compatibility at run time, so not very trivial. If anyone can/wants to test this, or even step up as a co-maintainer of this project, I would appreciate!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants