Added abstraction for BlockId

`i32` is really ambiguous and it is not immediately obvious what it is supposed to represent.
`BlockId` is much more readable.

Changed internal type to u16 because we currently have ~30k/64k block states.
(and i32 doesn't make a lot of sense for an Id, except when replicating mojang behavior)

pumpkin-protocol/src/client/play/c_chunk_data.rs

@@ -3,7 +3,7 @@ use std::collections::HashMap;
 use crate::{bytebuf::ByteBuffer, BitSet, ClientPacket, VarInt};
 use itertools::Itertools;
 use pumpkin_macros::packet;
-use pumpkin_world::{chunk::ChunkData, DIRECT_PALETTE_BITS};
+use pumpkin_world::{block::block_registry::BlockId, chunk::ChunkData, DIRECT_PALETTE_BITS};
 
 #[packet(0x27)]
 pub struct CChunkData<'a>(pub &'a ChunkData);
@@ -26,7 +26,11 @@ impl<'a> ClientPacket for CChunkData<'a> {
             let block_count = chunk
                 .iter()
                 .dedup()
-                .filter(|block| **block != 0 && **block != 12959 && **block != 12958)
+                .filter(|block| {
+                    !block.is_air()
+                        && **block != BlockId::from_id(12959)
+                        && **block != BlockId::from_id(12958)
+                })
                 .count() as i16;
             // Block count
             data_buf.put_i16(block_count);
@@ -63,7 +67,7 @@ impl<'a> ClientPacket for CChunkData<'a> {
                     palette.iter().enumerate().for_each(|(i, id)| {
                         palette_map.insert(*id, i);
                         // Palette
-                        data_buf.put_var_int(&VarInt(**id));
+                        data_buf.put_var_int(&VarInt(id.get_id_mojang_repr()));
                     });
                     for block_clump in chunk.chunks(64 / block_size as usize) {
                         let mut out_long: i64 = 0;
@@ -83,7 +87,7 @@ impl<'a> ClientPacket for CChunkData<'a> {
                         let mut out_long: i64 = 0;
                         let mut shift = 0;
                         for block in block_clump {
-                            out_long |= (*block as i64) << shift;
+                            out_long |= (block.get_id() as i64) << shift;
                             shift += DIRECT_PALETTE_BITS;
                         }
                         block_data_array.push(out_long);

pumpkin-world/src/block/block_registry.rs

@@ -7,6 +7,56 @@ use crate::level::WorldError;
 
 const BLOCKS_JSON: &str = include_str!("../../assets/blocks.json");
 
+// 0 is air -> reasonable default
+#[derive(Default, Deserialize, Debug, Hash, Clone, Copy, PartialEq, Eq)]
+#[serde(transparent)]
+pub struct BlockId {
+    data: u16,
+}
+
+impl BlockId {
+    pub fn new(
+        block_id: &str,
+        properties: Option<&HashMap<String, String>>,
+    ) -> Result<Self, WorldError> {
+        let mut block_states = BLOCKS
+            .get(block_id)
+            .ok_or(WorldError::BlockStateIdNotFound)?
+            .states
+            .iter();
+
+        let block_state = match properties {
+            Some(properties) => match block_states.find(|state| &state.properties == properties) {
+                Some(state) => state,
+                None => return Err(WorldError::BlockStateIdNotFound),
+            },
+            None => block_states
+                .find(|state| state.is_default)
+                .expect("Every Block should have at least 1 default state"),
+        };
+
+        Ok(block_state.id)
+    }
+
+    pub fn from_id(id: u16) -> Self {
+        // TODO: add check if the id is actually valid
+        Self { data: id }
+    }
+
+    pub fn is_air(&self) -> bool {
+        self.data == 0
+    }
+
+    pub fn get_id(&self) -> u16 {
+        self.data
+    }
+
+    /// An i32 is the way mojang internally represents their Blocks
+    pub fn get_id_mojang_repr(&self) -> i32 {
+        self.data as i32
+    }
+}
+
 #[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
 pub struct BlockDefinition {
     /// e.g. minecraft:door or minecraft:button
@@ -23,7 +73,7 @@ pub struct BlockDefinition {
 /// This could e.g. be an extended piston facing left.
 #[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
 pub struct BlockState {
-    id: i64,
+    id: BlockId,
 
     /// Whether this is the default state of the Block
     #[serde(default, rename = "default")]
@@ -51,28 +101,3 @@ lazy_static! {
     pub static ref BLOCKS: HashMap<String, BlockType> =
         serde_json::from_str(BLOCKS_JSON).expect("Could not parse block.json registry.");
 }
-
-pub fn block_id_and_properties_to_block_state_id(
-    block_id: &str,
-    properties: Option<&HashMap<String, String>>,
-) -> Result<i64, WorldError> {
-    let block = match BLOCKS.get(block_id) {
-        Some(block) => block,
-        None => return Err(WorldError::BlockStateIdNotFound),
-    };
-    let block_state_id = match properties {
-        None => Ok(block
-            .states
-            .iter()
-            .find(|state| state.is_default)
-            .expect("Each block should have at least one default state")
-            .id),
-        Some(properties) => block
-            .states
-            .iter()
-            .find(|state| &state.properties == properties)
-            .map(|state| state.id)
-            .ok_or(WorldError::BlockStateIdNotFound),
-    };
-    block_state_id
-}

pumpkin-world/src/chunk.rs

@@ -4,13 +4,14 @@ use fastnbt::LongArray;
 use serde::{Deserialize, Serialize};
 
 use crate::{
+    block::block_registry::BlockId,
     coordinates::{ChunkCoordinates, ChunkRelativeBlockCoordinates},
     level::WorldError,
     WORLD_HEIGHT,
 };
 
 pub struct ChunkData {
-    pub blocks: Box<[i32; 16 * 16 * WORLD_HEIGHT]>,
+    pub blocks: Box<[BlockId; 16 * 16 * WORLD_HEIGHT]>,
     pub position: ChunkCoordinates,
     pub heightmaps: ChunkHeightmaps,
 }
@@ -61,23 +62,18 @@ impl ChunkData {
         };
 
         // this needs to be boxed, otherwise it will cause a stack-overflow
-        let mut blocks = Box::new([0; 16 * 16 * WORLD_HEIGHT]);
+        let mut blocks = Box::new([BlockId::default(); 16 * 16 * WORLD_HEIGHT]);
 
         for (k, section) in chunk_data.sections.into_iter().enumerate() {
             let block_states = match section.block_states {
                 Some(states) => states,
                 None => continue, // this should instead fill all blocks with the only element of the palette
             };
+
             let palette = block_states
                 .palette
                 .iter()
-                .map(|entry| {
+                .map(|entry| BlockId::new(&entry.name, entry.properties.as_ref()))
-                    crate::block::block_registry::block_id_and_properties_to_block_state_id(
-                        &entry.name,
-                        entry.properties.as_ref(),
-                    )
-                    .map(|v| v as i32)
-                })
                 .collect::<Result<Vec<_>, _>>()?;
             let block_data = match block_states.data {
                 None => continue,
@@ -119,8 +115,8 @@ impl ChunkData {
     pub fn set_block(
         &mut self,
         at: ChunkRelativeBlockCoordinates,
-        block_id: i32,
+        block_id: BlockId,
-    ) -> Result<i32, WorldError> {
+    ) -> Result<BlockId, WorldError> {
         Ok(std::mem::replace(
             &mut self.blocks
                 [(at.y.get_absolute() * 16 * 16 + *at.z as u16 * 16 + *at.x as u16) as usize],

pumpkin/src/client/player_packet.rs

@@ -25,7 +25,7 @@ use pumpkin_protocol::{
         SUseItemOn, Status,
     },
 };
-use pumpkin_world::block::BlockFace;
+use pumpkin_world::block::{block_registry::BlockId, BlockFace};
 use pumpkin_world::global_registry;
 
 use super::PlayerConfig;
@@ -400,21 +400,16 @@ impl Player {
             let minecraft_id =
                 global_registry::find_minecraft_id(global_registry::ITEM_REGISTRY, item.item_id)
                     .expect("All item ids are in the global registry");
-            if let Ok(block_state_id) =
+            if let Ok(block_state_id) = BlockId::new(minecraft_id, None) {
-                pumpkin_world::block::block_registry::block_id_and_properties_to_block_state_id(
-                    minecraft_id,
-                    None,
-                )
-            {
                 server.broadcast_packet(
                     self,
-                    &CBlockUpdate::new(&location, (block_state_id as i32).into()),
+                    &CBlockUpdate::new(&location, block_state_id.get_id_mojang_repr().into()),
                 );
                 server.broadcast_packet(
                     self,
                     &CBlockUpdate::new(
                         &WorldPosition(location.0 + face.to_offset()),
-                        (block_state_id as i32).into(),
+                        block_state_id.get_id_mojang_repr().into(),
                     ),
                 );
             }