Thanks for the work on this.

For chunk generation, I would recommend a WorldGenerator trait:

pub trait WorldGenerator: Send + Sync {
    /// Generates the chunk at the given position.
    fn generate_chunk(&self, position: ChunkPosition) -> Chunk;
}

The chunk worker would then have a Arc<dyn WorldGenerator> field which it would invoke in a Rayon task for generating chunks.

If you have any questions, feel free to ask.

Codecov Report

Merging #108 into develop will increase coverage by 0.18%.
The diff coverage is 40%.

@@             Coverage Diff             @@
##           develop     #108      +/-   ##
===========================================
+ Coverage    57.94%   58.13%   +0.18%     
===========================================
  Files           58       59       +1     
  Lines         7718     7901     +183     
===========================================
+ Hits          4472     4593     +121     
- Misses        3246     3308      +62

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 015c053...33f5983. Read the comment docs.

@caelunshun Thanks, this is a bit ambitious for my Rust experience so I'm sure I'll have a few questions regarding rayon :)

Got it to work, it generates the new chunks using the layers. Just gotta make it asynchronous and write some unit tests.

Also I noticed that the Block enum entries that have property maps, like GrassBlock, will return an empty result if a property is missing (note the first ?):
https://github.com/caelunshun/feather/blob/fc6818582144e9f3902e0e85fc20b40aadec8942/blocks/src/blocks.rs#L5620

Which makes the from_name_and_props return a None when no properties are provided. Since the LevelData's generatorOptions layers doesn't contain any block data, it would have to get the default property map for the ID, or replace the ? with .unwrap_or and some default state for each property.

Also I noticed that the Block enum entries that have property maps, like GrassBlock, will return an empty result if a property is missing (note the first ?):
https://github.com/caelunshun/feather/blob/fc6818582144e9f3902e0e85fc20b40aadec8942/blocks/src/blocks.rs#L5620

Which makes the from_name_and_props return a None when no properties are provided. Since the LevelData's generatorOptions layers doesn't contain any block data, it would have to get the default property map for the ID, or replace the ? with .unwrap_or and some default state for each property.

Yes, I guess from_name_and_props should default to the default state when no properties are provided. As a quick fix, I could just change the generated ? to .unwrap_or_default(). The block data report also contains a default state for each block, so maybe that could be used instead.

To do the generation asynchronously, just use Rayon like a thread pool through rayon::spawn. Give the task a handle to the chunk worker channel so it can send the chunk once it is generated. For example:

fn generate_chunk(chunk_worker: &ChunkWorker, chunk_pos: ChunkPosition) {
    let sender = chunk_worker.sender.clone();
    let generator = Arc::clone(&chunk_worker.generator);
    rayon::spawn(move || {
        let chunk = generator.generate_chunk(chunk_pos);
        sender.send((chunk_pos, Ok(chunk))).unwrap();
    }
}

This will cause each chunk to be generated on a thread pool, potentially in parallel.

Test test_worldgen_flat will fail as long as the above issue with the property map is resolved.

Right, I'll get to implementing that then.

Sounds good. Also, I'm not super satisfied with the design of my changes in chunkworker, it seems a bit hacky to use Option/None when loading the chunk is delegated to the rayon task. It's also pretty annoying to test. Let me know if you have some ideas.

Well, you could pass the channel to load_chunk and make sending the reply a side-effect of the function... but this might not be ideal either.

Anyway, in the Tokio rewrite (#42), the chunk worker will be rewritten to use Tokio tasks for IO and Rayon tasks for generation, which will remove the need to return an Option. I think this will be fine as it is.

Alright, I'll leave it as is then.

@Momothereal I implemented Block::from_name_and_default_props.

test_spawn_item is failing because ChunkCrossSystem depends on ChunkWorkerHandle but the handle is not initialized. Try inserting a chunk worker handle in testframework::Builder::build().

Ready for merge?

Ah, hold on, there's still an issue with this. When a region file containing the chunk is missing, the chunk is not generated and instead an error is returned:

2019-09-06 19:27:45,806 WARN  [feather_server::chunk_logic] Failed to load chunk at ChunkPosition { x: 67, z: 21 }: Error loading chunk: No such file or directory (os error 2)

Consider generating the chunk if Err is returned here: https://github.com/caelunshun/feather/blob/aab3ebe0deb7a3341e7b0f1acc2f9a6e474a81b9/server/src/chunkworker.rs#L135

Ready to merge, unless there's something I can do about coverage.

Thanks!