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

Anvil Chunk saving #401

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Anvil Chunk saving #401

wants to merge 4 commits into from

Conversation

Snowiiii
Copy link
Owner

@Snowiiii Snowiiii commented Dec 18, 2024

Description

Add Chunk saving via the Anvil format.
More details on how the formats works can be found at https://minecraft.fandom.com/wiki/Region_file_format

Testing

  • Test Saving Chunks
  • Test Loading Saved Chunks
  • Test Loading Invalid Chunks

Checklist

Things need to be done before this Pull Request can be merged.

  • Write header (size & compression scheme)

  • Code is well-formatted and adheres to project style guidelines: cargo fmt

  • Code does not produce any clippy warnings: cargo clippy

  • All unit tests pass: cargo test

  • I added new unit tests, so other people don't accidentally break my code by changing other parts of the codebase. How?

Copy link
Contributor

@kralverde kralverde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some reviews to the code. Would like to see some unit tests too, basically create a fake region with (1, 2 ... all) chunks getting serialized. Then immediately de-serialize and verify all data is the same. With all compressions also.

@@ -141,41 +186,300 @@ impl ChunkReader for AnvilChunkReader {
out
};

// TODO: check checksum to make sure chunk is not corrupted
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was a checksum implemented?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not that i know, this was lucas comment

)
});

region_file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect, the pad should make the whole file a multiple of 4096, not just add 4096. It should be len(header) + len(bytes) + len(pad) % 4096 == 0

}

sections.push(ChunkSection {
y: i as i8,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There can be -y heights, but i is a usize. Should there be a start_y passed in?


data_version: i32,
status: ChunkStatus,
// #[serde(rename = "xPos")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What were these? can they be removed?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All things in there are NBT data stored in the chunk

@@ -189,16 +176,22 @@ impl Level {
self.chunk_watchers.shrink_to_fit();
}

pub fn write_chunk(&self, _chunk_to_write: (Vector2<i32>, Arc<RwLock<ChunkData>>)) {
//TODO
pub async fn write_chunk(&self, chunk_to_write: (Vector2<i32>, Arc<RwLock<ChunkData>>)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some kind of file lock so multiple threads don't try to write to the same file at the same time/read while the file is being written

@Snowiiii
Copy link
Owner Author

For some reason currently loading vanilla chunks does not work when writing, when early return in the write_chunk then it works

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.

2 participants