feather-rs / feather

A Minecraft server implementation in Rust
Apache License 2.0
2.58k stars 143 forks source link

Change the representation of BlockPosition. #476

Closed Miro-Andrin closed 2 years ago

Miro-Andrin commented 2 years ago

Currently the BlockPosition struct is defined like this:

#[repr(C)]
pub struct BlockPosition {
     x: i32,
     y: i32,
     z: i32.
}

This is representation has some issue. First of all the y value can never exceed the build height. And a i32, is overkill for that. It should probably be a i16, allowing for -64 to 4096 that is to be introduced in minecraft 1.17+ -. However its nice to have a simple representation, so i don't think it's necessarily a bad idea to keep it like this. But maybe something to think about.

Another issue with the current representation , happens when Block Positions are sent to the client. wiki.vg defines a position like this:

> An integer/block position:
    x (-33554432 to 33554431),
    y (-2048 to 2047), 
    z (-33554432 to 33554431) 
    x as a 26-bit integer, followed by y as a 12-bit integer, followed  by z as a 26-bit integer (all signed, two's complement).

You can therefore create a BlockPosition strut that can never be sent from the server to the client. Because the values exceed what is actually written to the client. When block position is written to the client its done like this:

impl Writeable for BlockPosition {
    fn write(&self, buffer: &mut Vec<u8>, version: ProtocolVersion) -> anyhow::Result<()> {
        let val = ((self.x as u64 & 0x3FFFFFF) << 38)
            | ((self.z as u64 & 0x3FFFFFF) << 12)
            | (self.y as u64 & 0xFFF);
        val.write(buffer, version)?;

        Ok(())
    }
}

This code silently converts from the internal representation to how its sent on the wire. Potentially sending incoherent positions to the client.

Suggested solutions.

Either change BlockPosition to how wiki.vg specifies it, or create a new position type. Something like ProtocolBlockPosision using wiki.vg's specification, and implement try_into from BlockPosition to ProtocolBlockPosition, and Into the other way around.

ambeeeeee commented 2 years ago

You can therefore create a BlockPosition strut that can never be sent from the server to the client. Because the values exceed what is actually written to the client. When block position is written to the client its done like this:

Not if we make the creation method fallible, the members are private.

Miro-Andrin commented 2 years ago

Had not considered that elegant solution. However then any operation that modifies/creates BlockPositions would become fallible. Thinking a bit i have come to the conclusion that giving the errors as early as possible might be annoying, but having errors at those points might make them actionable.

ambeeeeee commented 2 years ago

Could also make all functions that take BlockPosition unsafe with the safety clause that a BlockPosition is safe if BlockPosition::valid() returns true. I'd say operators aren't that bad to replace with methods, but technically using unsafe would be correct.

ambeeeeee commented 2 years ago

"but noo" the Rust dev cries, "how dare you ever mention the idea of using unsafe anywhere in my precious code," they say.