Add ValidatedBlockPosition (#483)

* Add `ValidBlockPosition`

* Update feather internals to use `ValidBlockPosition`

* Apply the automated cargo fixes because I'm lazy as HECK

* Use new functions in place of old hack

* Add tests for good measure

* Remove wildcard dependency on `bytemuck`

Author: ambeeeeee

Cargo.lock

@@ -32,6 +32,7 @@ smallvec = "1"
 thiserror = "1"
 uuid = { version = "0.8", features = [ "serde" ] }
 vek = "0.14"
+bytemuck = { version = "1", features = ["derive"] }
 
 [dev-dependencies]
 rand = "0.8"

feather/base/Cargo.toml

@@ -0,0 +1,163 @@
+use bytemuck::{Pod, Zeroable};
+use serde::{Deserialize, Serialize};
+
+use thiserror::Error;
+
+use libcraft_core::{BlockPosition, ChunkPosition, Position};
+use std::convert::TryFrom;
+
+/// Validated position of a block.
+///
+/// This structure is immutable.
+/// All operations that change a [`ValidBlockPosition`] must be done by
+/// turning it into a [`BlockPosition`], performing said operations,
+/// then using [`ValidBlockPosition`]'s [`TryFrom`] impl to get a [`ValidBlockPosition`].
+///
+/// The definition of a valid block position is defined by [`BlockPosition::valid`].
+///
+/// # Examples
+///
+/// Converting a [`BlockPosition`] to a [`ValidBlockPosition`], unwrapping any errors that
+/// occur.
+/// ```
+/// # use feather_base::BlockPosition;
+/// # use feather_base::ValidBlockPosition;
+/// # use std::convert::TryInto;
+/// // Create an unvalidated block position
+/// let block_position = BlockPosition::new(727, 32, 727);
+///
+/// // Validate the block position and unwrap any errors
+/// let valid_block_position: ValidBlockPosition = block_position.try_into().unwrap();
+/// ```
+///
+/// Performing operations on a [`ValidBlockPosition`], then re-validating it.
+/// ```
+/// # use feather_base::BlockPosition;
+/// # use feather_base::ValidBlockPosition;
+/// # use std::convert::TryInto;
+/// # let mut valid_block_position: ValidBlockPosition = BlockPosition::new(727, 32, 727).try_into().unwrap();
+/// // Convert the ValidBlockPosition into an unvalidated one to perform math
+/// let mut block_position: BlockPosition = valid_block_position.into();
+///
+/// block_position.x = 821;
+/// block_position.z += 32;
+///
+/// assert!(block_position.valid());
+///
+/// valid_block_position = block_position.try_into().unwrap();
+/// ```
+#[derive(
+    Clone,
+    Copy,
+    Debug,
+    PartialEq,
+    Eq,
+    Hash,
+    PartialOrd,
+    Ord,
+    Default,
+    Serialize,
+    Deserialize,
+    Zeroable,
+    Pod,
+)]
+#[repr(C)]
+pub struct ValidBlockPosition {
+    x: i32,
+    y: i32,
+    z: i32,
+}
+
+impl ValidBlockPosition {
+    pub fn x(&self) -> i32 {
+        self.x
+    }
+
+    pub fn y(&self) -> i32 {
+        self.y
+    }
+
+    pub fn z(&self) -> i32 {
+        self.z
+    }
+
+    pub fn chunk(self) -> ChunkPosition {
+        self.into()
+    }
+
+    pub fn position(self) -> Position {
+        self.into()
+    }
+}
+
+impl TryFrom<BlockPosition> for ValidBlockPosition {
+    type Error = BlockPositionValidationError;
+
+    fn try_from(value: BlockPosition) -> Result<Self, Self::Error> {
+        if value.valid() {
+            Ok(ValidBlockPosition {
+                x: value.x,
+                y: value.y,
+                z: value.z,
+            })
+        } else {
+            Err(BlockPositionValidationError::OutOfRange(value))
+        }
+    }
+}
+
+impl From<ValidBlockPosition> for BlockPosition {
+    fn from(position: ValidBlockPosition) -> Self {
+        BlockPosition {
+            x: position.x,
+            y: position.y,
+            z: position.z,
+        }
+    }
+}
+
+impl From<ValidBlockPosition> for ChunkPosition {
+    fn from(position: ValidBlockPosition) -> Self {
+        let position: BlockPosition = position.into();
+        position.into()
+    }
+}
+
+impl From<ValidBlockPosition> for Position {
+    fn from(position: ValidBlockPosition) -> Self {
+        let position: BlockPosition = position.into();
+        position.into()
+    }
+}
+
+#[derive(Error, Debug)]
+pub enum BlockPositionValidationError {
+    #[error("coordinate {0:?} out of range")]
+    OutOfRange(BlockPosition),
+}
+
+#[cfg(test)]
+mod tests {
+
+    use std::convert::TryInto;
+
+    use libcraft_core::BlockPosition;
+
+    use crate::ValidBlockPosition;
+
+    #[test]
+    #[should_panic]
+    fn check_out_of_bounds_up() {
+        let block_position = BlockPosition::new(0, 39483298, 0);
+
+        <BlockPosition as TryInto<ValidBlockPosition>>::try_into(block_position).unwrap();
+    }
+
+    #[test]
+    #[should_panic]
+    fn check_out_of_bounds_down() {
+        let block_position = BlockPosition::new(0, -39483298, 0);
+
+        <BlockPosition as TryInto<ValidBlockPosition>>::try_into(block_position).unwrap();
+    }
+}

feather/base/src/block.rs

@@ -10,11 +10,13 @@ use num_derive::{FromPrimitive, ToPrimitive};
 use serde::{Deserialize, Serialize};
 
 pub mod anvil;
+mod block;
 pub mod chunk;
 pub mod chunk_lock;
 pub mod inventory;
 pub mod metadata;
 
+pub use block::{BlockPositionValidationError, ValidBlockPosition};
 pub use blocks::*;
 pub use chunk::{Chunk, ChunkSection, CHUNK_HEIGHT, CHUNK_WIDTH};
 pub use chunk_lock::*;

feather/base/src/lib.rs

@@ -2,7 +2,7 @@
 //! metadata format. See <https://wiki.vg/Entity_metadata>
 //! for the specification.
 
-use crate::{BlockPosition, Direction};
+use crate::{Direction, ValidBlockPosition};
 use bitflags::bitflags;
 use generated::ItemStack;
 use std::collections::BTreeMap;
@@ -47,8 +47,8 @@ pub enum MetaEntry {
     Slot(Option<ItemStack>),
     Boolean(bool),
     Rotation(f32, f32, f32),
-    Position(BlockPosition),
-    OptPosition(Option<BlockPosition>),
+    Position(ValidBlockPosition),
+    OptPosition(Option<ValidBlockPosition>),
     Direction(Direction),
     OptUuid(OptUuid),
     OptBlockId(Option<i32>),
@@ -131,7 +131,7 @@ impl ToMetaEntry for bool {
     }
 }
 
-impl ToMetaEntry for BlockPosition {
+impl ToMetaEntry for ValidBlockPosition {
     fn to_meta_entry(&self) -> MetaEntry {
         MetaEntry::Position(*self)
     }

feather/base/src/metadata.rs

@@ -1,8 +1,8 @@
-use std::iter;
+use std::{convert::TryInto, iter};
 
 use base::{
     chunk::{SECTION_HEIGHT, SECTION_VOLUME},
-    BlockPosition, ChunkPosition,
+    BlockPosition, ChunkPosition, ValidBlockPosition,
 };
 use itertools::Either;
 
@@ -18,7 +18,7 @@ pub struct BlockChangeEvent {
 
 impl BlockChangeEvent {
     /// Creates an event affecting a single block.
-    pub fn single(pos: BlockPosition) -> Self {
+    pub fn single(pos: ValidBlockPosition) -> Self {
         Self {
             changes: BlockChanges::Single { pos },
         }
@@ -42,7 +42,7 @@ impl BlockChangeEvent {
     }
 
     /// Returns an iterator over block positions affected by this block change.
-    pub fn iter_changed_blocks(&self) -> impl Iterator<Item = BlockPosition> + '_ {
+    pub fn iter_changed_blocks(&self) -> impl Iterator<Item = ValidBlockPosition> + '_ {
         match &self.changes {
             BlockChanges::Single { pos } => Either::Left(iter::once(*pos)),
             BlockChanges::FillChunkSection { chunk, section } => {
@@ -60,7 +60,7 @@ impl BlockChangeEvent {
     ) -> impl Iterator<Item = (ChunkPosition, usize, usize)> + '_ {
         match &self.changes {
             BlockChanges::Single { pos } => {
-                iter::once((pos.chunk(), pos.y as usize / SECTION_HEIGHT, 1))
+                iter::once((pos.chunk(), pos.y() as usize / SECTION_HEIGHT, 1))
             }
             BlockChanges::FillChunkSection { chunk, section } => {
                 iter::once((*chunk, *section as usize, SECTION_VOLUME))
@@ -69,22 +69,27 @@ impl BlockChangeEvent {
     }
 }
 
-fn iter_section_blocks(chunk: ChunkPosition, section: u32) -> impl Iterator<Item = BlockPosition> {
+fn iter_section_blocks(
+    chunk: ChunkPosition,
+    section: u32,
+) -> impl Iterator<Item = ValidBlockPosition> {
     (0..16)
         .flat_map(|x| (0..16).map(move |y| (x, y)))
         .flat_map(|(x, y)| (0..16).map(move |z| (x, y, z)))
         .map(move |(dx, dy, dz)| {
             let x = dx + chunk.x * 16;
             let y = dy + section as i32 * 16;
             let z = dz + chunk.z * 16;
-            BlockPosition::new(x, y, z)
+
+            // It's safe to unwrap because we are working from a valid source of block positions
+            BlockPosition::new(x, y, z).try_into().unwrap()
         })
 }
 
 #[derive(Debug, Clone)]
 enum BlockChanges {
     /// A single block change.
-    Single { pos: BlockPosition },
+    Single { pos: ValidBlockPosition },
     /// A whole chunk section was filled with the same block.
     FillChunkSection { chunk: ChunkPosition, section: u32 },
 }
@@ -98,7 +103,7 @@ mod tests {
 
     #[test]
     fn create_single() {
-        let pos = BlockPosition::new(5, 64, 9);
+        let pos = BlockPosition::new(5, 64, 9).try_into().unwrap();
         let event = BlockChangeEvent::single(pos);
         assert_eq!(event.count(), 1);
         assert_eq!(event.iter_changed_blocks().collect::<Vec<_>>(), vec![pos]);
@@ -123,9 +128,9 @@ mod tests {
 
     #[test]
     fn test_iter_section_blocks() {
-        let blocks: Vec<BlockPosition> =
+        let blocks: Vec<ValidBlockPosition> =
             iter_section_blocks(ChunkPosition::new(-1, -2), 5).collect();
-        let unique_blocks: AHashSet<BlockPosition> = blocks.iter().copied().collect();
+        let unique_blocks: AHashSet<ValidBlockPosition> = blocks.iter().copied().collect();
 
         assert_eq!(blocks.len(), unique_blocks.len());
         assert_eq!(blocks.len(), SECTION_VOLUME);
@@ -134,7 +139,7 @@ mod tests {
             for y in 80..96 {
                 for z in -32..-16 {
                     assert!(
-                        unique_blocks.contains(&BlockPosition::new(x, y, z)),
+                        unique_blocks.contains(&BlockPosition::new(x, y, z).try_into().unwrap()),
                         "{}, {}, {}",
                         x,
                         y,

feather/common/src/events/block_change.rs

@@ -1,6 +1,6 @@
 use std::{cell::RefCell, mem, rc::Rc, sync::Arc};
 
-use base::{BlockId, BlockPosition, ChunkPosition, Position, Text, Title};
+use base::{BlockId, ChunkPosition, Position, Text, Title, ValidBlockPosition};
 use ecs::{
     Ecs, Entity, EntityBuilder, HasEcs, HasResources, NoSuchEntity, Resources, SysResult,
     SystemExecutor,
@@ -181,14 +181,14 @@ impl Game {
     }
 
     /// Gets the block at the given position.
-    pub fn block(&self, pos: BlockPosition) -> Option<BlockId> {
+    pub fn block(&self, pos: ValidBlockPosition) -> Option<BlockId> {
         self.world.block_at(pos)
     }
 
     /// Sets the block at the given position.
     ///
     /// Triggers necessary `BlockChangeEvent`s.
-    pub fn set_block(&mut self, pos: BlockPosition, block: BlockId) -> bool {
+    pub fn set_block(&mut self, pos: ValidBlockPosition, block: BlockId) -> bool {
         let was_successful = self.world.set_block_at(pos, block);
         if was_successful {
             self.ecs.insert_event(BlockChangeEvent::single(pos));
@@ -226,7 +226,7 @@ impl Game {
 
     /// Breaks the block at the given position, propagating any
     /// necessary block updates.
-    pub fn break_block(&mut self, pos: BlockPosition) -> bool {
+    pub fn break_block(&mut self, pos: ValidBlockPosition) -> bool {
         self.set_block(pos, BlockId::air())
     }
 }