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

Block editing #150

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

Block editing #150

wants to merge 32 commits into from

Conversation

ReCore-sys
Copy link
Collaborator

@ReCore-sys ReCore-sys commented Dec 31, 2024

This PR implements block editing and some basic terrain generation.

Description

This is a fairly large PR that implements a lot of different things, the main ones being the set_block() function as well as some simple terrain generation.
The set_block() method uses some bitwise manipulation to modify the block in a chunk. It has been tested relatively well and is unlikely to break under normal circumstances. It is currently unable to use the direct palette format but that shouldn't really be an issue for now.
There is also some very basic terrain gen implemented using OpenSimplex noise, but should absolutely be expanded on later.
There is certainly room for optimizations, but these were largely ignored for the sake of getting this core feature implemented as soon as possible.
One important note is that breaking blocks works for the client, but placing blocks is very half-baked. Inventories will need to be implemented for placing different blocks and currently the default placing of stone blocks with right click is exceptionally buggy, so this should be sorted out when inventories get done.

Motivation and Context

Block editing is a fundamental part of minecraft, both for player interactions with the world and for terrain generation. This PR is vital for minecraft being at all playable.

How has this been tested?

Tested manually and with unit tests

Screenshots (if appropriate):

TICahXd

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (restructuring code, without changing its behavior)

Checklist:

  • My code follows the code style of this project.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • Clippy passes with no warnings.

.get_entities_with::<ChunkReceiver>()
.get_entities_with::<PlayerIdentity>()
Copy link
Member

Choose a reason for hiding this comment

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

Keep this as a ChunkReceiver, since you don't want to send play packets to someone in login state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There has to be a better solution that that. Also why would a player during login have a player identity?

Copy link
Member

Choose a reason for hiding this comment

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

You're more than welcome to find a better solution, but, player receives the PlayerIdentity component during login start packet.
Oh wait, you also add ChunkReceiver during login start? Add it when login process is completed, and keep this as is. So 'everyone' would be players that could receive chunks I suppose?

/// Reads an n-bit integer from a packed `i64`.
pub fn read_nbit_i32(
word: &i64,
bit_size: usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

you state earlier that this must be 32 or less. so why not just u8?

@ReCore-sys ReCore-sys changed the title WIP: Block editing Block editing Mar 8, 2025
@ReCore-sys ReCore-sys marked this pull request as ready for review March 8, 2025 11:17
Comment on lines 72 to 102
if !global_state.world.chunk_exists(0, 0, "overworld").await? {
info!("No overworld spawn chunk found, generating spawn chunks...");
// Generate a 12x12 chunk area around the spawn point
let mut chunks = Vec::new();
for x in -12..12 {
for z in -12..12 {
chunks.push((x, z));
}
}
let generated_chunks: Vec<Result<Chunk, WorldGenError>> = chunks
.chunks(72)
.par_bridge()
.map(|chunk_coord_arr| {
let mut generated_chunks = Vec::new();
for (x, z) in chunk_coord_arr {
let state = global_state.clone();
generated_chunks.push(state.terrain_generator.generate_chunk(*x, *z));
}
generated_chunks
})
.flatten()
.collect();
for chunk in generated_chunks {
let chunk = chunk.map_err(|e| {
error!("Error generating chunk: {:?}", e);
BinaryError::Custom("Error generating chunk".to_string())
})?;
global_state.world.save_chunk(chunk).await?;
}
info!("Finished generating spawn chunks...");
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not make this into a function, why just put it directly in the main function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally it was a lot smaller but yeah you raise a good point

Comment on lines 19 to 43
fn generate_chunk(x: i32, z: i32) -> Chunk {
let mut new_chunk = Chunk::new(x, z, "overworld".to_string());
for x in 0..16 {
for z in 0..16 {
let mut base_y = (x as f64).sin() / 32.0;
base_y += (z as f64).cos() / 32.0;
let scaled_y = ((base_y + 1.0) * 64.0) as i32;
for y in 0..scaled_y {
new_chunk
.set_block(
x,
y,
z,
BlockData {
name: "minecraft:stone".to_string(),
properties: None,
},
)
.unwrap();
}
}
}
new_chunk
}

Copy link
Member

Choose a reason for hiding this comment

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

magic numbers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's kinda just how terrain gen is. You kinda just gotta play around with numbers until it looks how you want. This function will be removed though and should use the proper terrain gen algorithms

@@ -41,27 +69,42 @@ impl System for ChunkFetcher {
let mut copied_chunks = HashMap::new();
for chunk in chunk_recv.needed_chunks.iter() {
let (key, chunk) = chunk;
if chunk.is_none() {
if let Fetching = chunk {
Copy link
Member

Choose a reason for hiding this comment

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

is that a capital F, and what is that even doing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an enum variant, and yeah its pretty messy. Will be redoing this entire thing.

@@ -79,7 +122,7 @@ impl System for ChunkFetcher {
}
}
}
tokio::time::sleep(std::time::Duration::from_millis(5)).await;
tokio::time::sleep(std::time::Duration::from_millis(45)).await;
Copy link
Member

Choose a reason for hiding this comment

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

We need to rework the polling chunk system

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

100%. Was going to leave it to a later PR but you are right, this is terrible.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's a good idea. merge this pr for now. let's fix it in a different pr. otherwise it'll get too much for a single pr

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nah, I did it anyway. Leaving it until later is a a great way for us to have an unusable codebase.

@@ -9,7 +9,7 @@ pub struct BitSet(Vec<u64>);

impl BitSet {
pub fn new(size: usize) -> Self {
let num_blocks = (size + 63) / 64;
let num_blocks = size.div_ceil(64);
Copy link
Member

Choose a reason for hiding this comment

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

magic number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a 64 bit unsigned int. I wouldn't call 64 a magic number in this context.

Comment on lines +9 to +95
impl BiomeGenerator for PlainsBiome {
fn _biome_id(&self) -> u8 {
0
}

fn _biome_name(&self) -> String {
"plains".to_string()
}

fn generate_chunk(
&self,
x: i32,
z: i32,
noise: &NoiseGenerator,
) -> Result<Chunk, WorldGenError> {
let mut chunk = Chunk::new(x, z, "overworld".to_string());
let mut heights = vec![];
let stone = BlockData {
name: "minecraft:stone".to_string(),
properties: None,
};

// Fill with water first
for section_y in -4..4 {
chunk.set_section(
section_y as i8,
BlockData {
name: "minecraft:water".to_string(),
properties: Some(BTreeMap::from([("level".to_string(), "0".to_string())])),
},
)?;
}

// Then generate some heights
for chunk_x in 0..16i64 {
for chunk_z in 0..16i64 {
let global_x = i64::from(x) * 16 + chunk_x;
let global_z = i64::from(z) * 16 + chunk_z;
let height = noise.get_noise(global_x as f64, global_z as f64);
let height = (height * 64.0) as i32 + 64;
heights.push((global_x, global_z, height));
}
}

// Fill in the sections that consist of only stone first with the set_section method since
// it's faster than set_block
let y_min = heights.iter().min_by(|a, b| a.2.cmp(&b.2)).unwrap().2;
let heighst_full_section = y_min / 16;
for section_y in -4..heighst_full_section {
chunk.set_section(section_y as i8, stone.clone())?;
}
let above_filled_sections = (heighst_full_section * 16) - 1;
for (global_x, global_z, height) in heights {
if height > above_filled_sections {
let height = height - above_filled_sections;
for y in 0..height {
if y + above_filled_sections <= 64 {
chunk.set_block(
global_x as i32 & 0xF,
y + above_filled_sections,
global_z as i32 & 0xF,
BlockData {
name: "minecraft:sand".to_string(),
properties: None,
},
)?;
} else {
chunk.set_block(
global_x as i32 & 0xF,
y + above_filled_sections,
global_z as i32 & 0xF,
BlockData {
name: "minecraft:grass_block".to_string(),
properties: Some(BTreeMap::from([(
"snowy".to_string(),
"false".to_string(),
)])),
},
)?;
}
}
}
}

Ok(chunk)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

underscored function names? not used?
Also would be nice to make this function into smaller functions, right now, it's a huge function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Underscored because they are currently not used and clippy tries to kill me if i don't underscore them. And yeah should probably break it down, but since this is just a very simple proof of concept terrain gen system, it'll need to be ripped up and replaced soon anyway.

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