Anvil file support (blocks and biomes)

[TerminatorNL]

This is a starting point for the valence_anvil crate.

It currently supports loading blocks and biomes.

Note on negative Y values:
Since UnloadedChunk does not support negative Y values, all parsed Y values are raised to Y=0.

Before merging, make sure the changes from 03e89ad are uploaded to crates.io and the version number is set in Cargo.toml

[TerminatorNL]

Something worth mentioning: When loading chunks, especially with larger view distances, I notice (significant) stuttering while flying around in the java_region.rs example.

This is room for improvement and should be looked into. This PR is a start, but at least we have more functionality than yesterday.

[rj00a]

When loading chunks, especially with larger view distances, I notice (significant) stuttering while flying around in the java_region.rs example.

Is the stuttering client side? I recall someone discovering their game was lagging because valence is not sending any light data. If the light data is filled with zeroes instead, the lag was eliminated.

[rj00a]

Game crashed while flying around with this.

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NbtFormatError(MissingKey("Status"))', valence_anvil/examples/java_region.rs:152:69
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[rj00a]

Flying out of bounds, I found this. All the lava blocks are in the corner of the chunk, at index 0 of some array I assume. Expected behavior

And there was also this. That row of stone blocks is not supposed to be there.

Quite possible that this is entirely my fault.

[rj00a]

• Doc tests are failing. CI should have caught that.
• I feel that the program can be restructured to remove the Mutexes. It might make more sense to use channels. In other words, the API is structured where you can supply the chunks you want to be loaded and the program returns them through a channel as they're ready. This way, the example can be written where chunks are loaded into the valence server as they're ready without blocking the main server thread.
• Don't think there's much parallelism here? Looks like the load_chunks function is processing chunks sequentially.
• It is possible to have multiple file handles to the same file in read mode. Not sure if this should be taken advantage of.
• Using lava blocks to mark empty chunks is a bit confusing for the example (send a message to the client explaining this?).
• Might be good moving the example to the rest of examples and adding valence_anvil as a dev-dependency.

Not sure there's enough File IO happening to justify the use of tokio. Need to investigate how fastanvil is doing things.

[rj00a]

In this module you can use #[derive(Error)] from the thiserror crate. That would save you some boilerplate.

anyhow::Error is boxed, meaning it is the size of only one pointer. Since errors are rare, this means anyhow::Result<T> is usually as small as possible which can have a meaningful performance impact in some cases. Perhaps we can do the same without being too dynamic/loose with errors. There are bigger fish to fry but something to consider.

[TerminatorNL]

I have implemented the #[derive(Error)] in 8db39c7

This needs to be revisited when the parse_chunk_nbt function is being updated.
For future reference: #145 (comment)

[rj00a]

I think this function can be simplified by parsing the chunk NBT data (like what serde would typically do) separately from the block states, biomes, etc. rather than interleaving the work.

Also I realized the From traits we added to valence_nbt::Compound might be unidiomatic because From/Into conversions are not supposed to be lossy. Not too worried about that right now though.

[TerminatorNL]

I have removed the From traits from valence_nbt for now just to be on the safe side. A good implementation might be to return an error type instead of None as shown in rust-lang/rfcs#2484.

[rj00a]

Rather than just speculating about performance as I've done, I think it would be a good idea to set up a benchmark in performance_tests/. The benchmark could involve loading a large anvil world in one go and measuring how long it takes.

[rj00a]

Here are some things I would like to see before we merge:

• The structure of biome.json should match the items in the biome registry. https://github.com/valence-rs/valence/blob/main/src/biome.rs#L59-L123
• tokio does not need to be used and can be removed.

There are some API things we should figure out and some issues related to loading partially generated chunks, but let's not worry about that until after we merge.

[rj00a]

Now that I've had a chance to take a closer look at this, I have a lot to say. I've rewritten quite a few things.

Architectural Changes

Loading a Valence chunk is now done in a two-step process.

1. Load the chunk NBT data from the filesystem.
2. Convert the NBT data to a Valence chunk.

AnvilWorld still exists, but its only purpose is to complete step 1. Step 2 is done by a dedicated function named to_valence (in the future, we can add a from_valence to go the other way). This design has a number of advantages.

• All chunk data is accessible to users because the raw chunk NBT can be inspected after step 1.
• valence_anvil no longer has a required dependency on valence. If you only need step 1, there is no dependency. Step 2 is behind a feature flag.

Some details about to_valence:

• Rather than return an UnloadedChunk, to_valence accepts a mutable reference to any type implementing the Chunk trait. This is a lot more flexible. I've also made a few changes to Chunk to make it more efficient.
• to_valence accepts a section offset. This lets us load the chunk at any height we need.
• to_valence accepts a function to map biome resource identifiers to BiomeId. This is convenient and means we don't need to store a map somewhere.

The vanilla biome generator has been removed for the time being. As far as Anvil and the protocol is concerned, vanilla biomes are not necessary. I still think it would be nice to have the vanilla biomes available so that we can implement the example correctly. This would go in the valence_protocol crate. We would probably also want to unify the Biome type in valence with the one in valence_protocol, but I didn't want to worry about all that in this PR.

The test for valence_anvil has been removed because I didn't want CI to have to download the world every time it runs. The benchmark does basically the same thing as the test, so we can still run that locally.

Speaking of benchmarks, the new code appears to be about 15% faster.

Code Feedback

• The main issue I have is that your code seems to be designed around object-oriented classes rather than the procedures that are actually needed to solve the problem. This results in a lot of unnecessary complexity. Most of those classes don't add much value and I think you're better off without them. This includes types like CompressionScheme, AnvilHeader, ChunkSeekLocation, ChunkTimestamp, RegionPos, WebAsset, etc. Your code is spread out across these classes' methods which makes things more difficult to follow in my opinion.

  In other words, if something is only used in one place, it's usually not worth making an abstraction for it. Don't add functionality until you actually need it.

• To improve readability and decrease nesting, consider returning from functions as early as possible so that the "happy path" can continue with as little nesting as possible. The new let else feature helps with this. You can see how I've used it in to_valence.

• By convention, error messages start with a lowercase letter and do not end with punctuation.

• Complicated trait bounds are better expressed using a where clause.

---

I'll go ahead and merge this now. If I missed something, feel free to open more PRs.

[TerminatorNL]

Thanks for the feedback! I see you made a lot of changes and it does look a lot better now.