owengage / fastnbt

Fast serde serializer and deserializer for Minecraft's NBT and Anvil formats
MIT License
186 stars 34 forks source link

Add (de)serializer to/from Value #54

Closed RubixDev closed 2 years ago

RubixDev commented 2 years ago

Another feature ported from serde_json:

RubixDev commented 2 years ago

I was working on the deserializer now, but currently have issues with the endianness of the bytes per integer for the IntArray and LongArray types. My current test fails the following way:

thread 'test::value::de::int_array_types' panicked at 'assertion failed: `(left == right)`
  left: `V { bytes: ByteArray { data: [1, 2, 3, 4, 5] }, ints: IntArray { data: [1, 2, 3, 4, 5] }, longs: LongArray { data: [1, 2, 3, 4, 5] } }`,
 right: `V { bytes: ByteArray { data: [1, 2, 3, 4, 5] }, ints: IntArray { data: [16777216, 33554432, 50331648, 67108864, 83886080] }, longs: LongArray { data: [72057594037927936, 144115188075855872, 216172782113783808, 288230376151711744, 360287970189639680] } }`',

The first value of the new IntArray 16777216 is just a 1 with the bytes swapped.


In the Serialize code for both IntArray and LongArray you use the following to get the integers as a byte slice:

// Alignment of i32 is >= alignment of bytes so this should always work.
let (_, data, _) = unsafe { self.data.as_slice().align_to::<u8>() };

If I understand this correctly the resulting byte slice depends on the targets endianness and might not always produce the correct thing. I am not sure whether this is the cause for my problem as all other tests do pass on my machine, but I just wanted to point it out anyways in case that is an actual problem.

owengage commented 2 years ago

Hmm, that might be a legitimate issue. In the actual array serialization I have this:

            Tag::IntArray => {
                let stride = 4;
                let len = v.len() / stride;
                self.ser.writer.write_len(len)?;

                for chunk in v.chunks(stride) {
                    let el = NativeEndian::read_i32(chunk);
                    self.ser.writer.write_i32::<BigEndian>(el)?;
                }
            }

So endianness should be fine since I read native and write big endian. I'll have to dig into this when I find time.

RubixDev commented 2 years ago

Ok I didn't see you read it as native endian, your code should be fine then. I now just somehow have to get the data of of an IntArray and LongArray as BigEndian with a long enough lifetime so the Deserialize of the IntArray's correctly picks it up.

RubixDev commented 2 years ago

This PR is ready to be merged now. A lot of the code is again copied from serde_json, but this time I already added the license notices. One thing that might need to be added is a test with borrowed types.


I was working on the deserializer now, but currently have issues with the endianness of the bytes per integer for the IntArray and LongArray types.

I got around that issue by introducing a second token for deserializing from Values and then reading the bytes as either big endian or native endian in the Deserialize impl for IntArray and LongArray:

match token {
    INT_ARRAY_TOKEN => IntArray::from_bytes::<BigEndian>(data)
        .map_err(|_| serde::de::Error::custom("could not read i32 for int array")),
    INT_ARRAY_VALUE_TOKEN => IntArray::from_bytes::<NativeEndian>(data)
        .map_err(|_| serde::de::Error::custom("could not read i32 for int array")),
    _ => Err(serde::de::Error::custom("expected NBT int array token")),
}