fix(protocol): fix multi-version client protocol compatibility

README.md

@@ -37,6 +37,7 @@ and customizable experience. It prioritizes performance and player enjoyment whi
   - [x] Encryption
   - [x] Packet Compression
   - [x] Java Edition
+    - 1.7 ~ 26.2
   - [x] Bedrock Edition (W.I.P)
   - ...
 - [Tracking: World](https://github.com/Pumpkin-MC/Pumpkin/issues/1403)

crates/pumpkin-protocol/src/java/client/play/multi_block_update.rs

@@ -58,18 +58,164 @@ impl ClientPacket for CMultiBlockUpdate {
         version: &JavaMinecraftVersion,
     ) -> Result<(), WritingError> {
         let mut write = write;
-        write.write_i64_be(self.chunk_section)?;
-        write.write_var_int(&VarInt(self.updates.len() as i32))?;
-
-        for update in &self.updates {
-            let packed_update = update.0 as u64;
-            let local_pos = packed_update & 0xFFF;
-            let state_id = (packed_update >> 12) as u16;
-            let remapped_state_id = remap_block_state_for_version(state_id, *version);
-            let remapped_packed = (u64::from(remapped_state_id) << 12) | local_pos;
-            write.write_var_long(&VarLong(remapped_packed as i64))?;
+
+        if *version >= JavaMinecraftVersion::V_1_13 {
+            // 1.13+ packs the chunk section position into a single i64 and each
+            // record into a VarLong: (block state id << 12) | local position.
+            write.write_i64_be(self.chunk_section)?;
+            write.write_var_int(&VarInt(self.updates.len() as i32))?;
+
+            for update in &self.updates {
+                let (state_id, local_pos) = unpack_update(*update);
+                let remapped_state_id = remap_block_state_for_version(state_id, *version);
+                let remapped_packed = (u64::from(remapped_state_id) << 12) | local_pos;
+                write.write_var_long(&VarLong(remapped_packed as i64))?;
+            }
+        } else {
+            // Pre-1.13 the chunk position is sent as two separate i32s.
+            let chunk_x = (self.chunk_section >> 42) & 0x3F_FFFF;
+            let chunk_z = (self.chunk_section >> 20) & 0x3F_FFFF;
+            write.write_i32_be(((chunk_x << 42) >> 42) as i32)?;
+            write.write_i32_be(((chunk_z << 42) >> 42) as i32)?;
+
+            if *version >= JavaMinecraftVersion::V_1_9 {
+                // 1.9 - 1.12: each record is a u16 packed position followed by
+                // a VarInt block state id.
+                write.write_var_int(&VarInt(self.updates.len() as i32))?;
+                for update in &self.updates {
+                    let (state_id, local_pos) = unpack_update(*update);
+                    let remapped_state_id = remap_block_state_for_version(state_id, *version);
+                    let (x, z, y) = local_coords(local_pos);
+                    let packed_pos = ((x & 0xF) << 12) | ((z & 0xF) << 8) | (y & 0xF);
+                    write.write_u16_be(packed_pos as u16)?;
+                    write.write_var_int(&VarInt(remapped_state_id as i32))?;
+                }
+            } else if *version >= JavaMinecraftVersion::V_1_8 {
+                // 1.8: each record is horizontal position, y and a block state id.
+                write.write_var_int(&VarInt(self.updates.len() as i32))?;
+                for update in &self.updates {
+                    let (state_id, local_pos) = unpack_update(*update);
+                    let remapped_state_id = remap_block_state_for_version(state_id, *version);
+                    let (x, z, y) = local_coords(local_pos);
+                    write.write_u8(((x & 0xF) << 4 | (z & 0xF)) as u8)?;
+                    write.write_u8(y as u8)?;
+                    write.write_var_int(&VarInt(remapped_state_id as i32))?;
+                }
+            } else {
+                // 1.7.x: a short record count, then an i32 byte length, then
+                // each record is a packed position short followed by a packed
+                // block-state short ((block id << 4) | metadata).
+                let count = i16::try_from(self.updates.len())
+                    .map_err(|_| WritingError::Message("Too many block updates".into()))?;
+                write.write_i16_be(count)?;
+                write.write_i32_be(i32::from(count) * 4)?;
+                for update in &self.updates {
+                    let (state_id, local_pos) = unpack_update(*update);
+                    let remapped_state_id = remap_block_state_for_version(state_id, *version);
+                    let (x, z, y) = local_coords(local_pos);
+                    let packed_pos = ((x & 0xF) << 12) | ((z & 0xF) << 8) | (y & 0xF);
+                    write.write_u16_be(packed_pos as u16)?;
+                    write.write_u16_be(remapped_state_id)?;
+                }
+            }
         }
 
         Ok(())
     }
 }
+
+/// Splits a stored packed update into its block state id and 12-bit local position.
+const fn unpack_update(update: VarLong) -> (u16, u64) {
+    let packed = update.0 as u64;
+    ((packed >> 12) as u16, packed & 0xFFF)
+}
+
+/// Decodes the `(x, z, y)` coordinates from a 12-bit packed local position.
+const fn local_coords(local_pos: u64) -> (u64, u64, u64) {
+    (
+        (local_pos >> 8) & 0xF,
+        (local_pos >> 4) & 0xF,
+        local_pos & 0xF,
+    )
+}
+
+#[cfg(test)]
+mod tests {
+    use super::CMultiBlockUpdate;
+    use crate::ClientPacket;
+    use crate::codec::var_long::VarLong;
+    use pumpkin_util::math::vector3::{self, Vector3};
+    use pumpkin_util::version::JavaMinecraftVersion;
+
+    fn sample() -> CMultiBlockUpdate {
+        // state id 5 at local position (x=1, z=2, y=3), section (x=1, y=2, z=3)
+        let local_pos = (1u64 << 8) | (2u64 << 4) | 3u64;
+        CMultiBlockUpdate {
+            chunk_section: vector3::packed_chunk_pos(&Vector3::new(1, 2, 3)),
+            updates: vec![VarLong(((5u64 << 12) | local_pos) as i64)],
+        }
+    }
+
+    #[test]
+    fn pre_1_9_uses_horizontal_and_y_record() {
+        let packet = sample();
+        let mut out = Vec::new();
+        packet
+            .write_packet_data(&mut out, &JavaMinecraftVersion::V_1_8)
+            .unwrap();
+
+        assert_eq!(&out[0..4], &1i32.to_be_bytes());
+        assert_eq!(&out[4..8], &3i32.to_be_bytes());
+        assert_eq!(out[8], 1, "record count should be a 1-byte VarInt");
+        assert_eq!(out[9], 0x12, "horizontal position packs x << 4 | z");
+        assert_eq!(out[10], 3, "y coordinate");
+        assert!(out.len() >= 12);
+    }
+
+    #[test]
+    fn v1_7_uses_short_count_and_packed_records() {
+        let packet = sample();
+        let mut out = Vec::new();
+        packet
+            .write_packet_data(&mut out, &JavaMinecraftVersion::V_1_7_6)
+            .unwrap();
+
+        assert_eq!(&out[0..4], &1i32.to_be_bytes());
+        assert_eq!(&out[4..8], &3i32.to_be_bytes());
+        assert_eq!(&out[8..10], &1i16.to_be_bytes(), "count is a short in 1.7");
+        assert_eq!(&out[10..14], &4i32.to_be_bytes(), "data length = count * 4");
+        // one record: packed position short + packed block-state short
+        assert_eq!(&out[14..16], &4611u16.to_be_bytes());
+        assert_eq!(out.len(), 4 + 4 + 2 + 4 + 4);
+    }
+
+    #[test]
+    fn pre_1_13_uses_chunk_x_z_and_short_record() {
+        let packet = sample();
+        let mut out = Vec::new();
+        packet
+            .write_packet_data(&mut out, &JavaMinecraftVersion::V_1_9)
+            .unwrap();
+
+        assert_eq!(&out[0..4], &1i32.to_be_bytes());
+        assert_eq!(&out[4..8], &3i32.to_be_bytes());
+        assert_eq!(out[8], 1, "record count should be a 1-byte VarInt");
+        // local position packed for 1.9-1.12: (x << 12) | (z << 8) | y
+        assert_eq!(&out[9..11], &4611u16.to_be_bytes());
+        // the block state id VarInt follows (at least one byte, no extra data)
+        assert!(out.len() >= 12);
+    }
+
+    #[test]
+    fn post_1_13_uses_section_and_var_long_record() {
+        let packet = sample();
+        let mut out = Vec::new();
+        packet
+            .write_packet_data(&mut out, &JavaMinecraftVersion::V_1_13)
+            .unwrap();
+
+        assert_eq!(&out[0..8], &packet.chunk_section.to_be_bytes());
+        assert_eq!(out[8], 1, "record count should be a 1-byte VarInt");
+        assert!(out.len() >= 10);
+    }
+}

crates/pumpkin-protocol/src/java/client/play/set_container_content.rs

@@ -45,24 +45,31 @@ impl ClientPacket for CSetContainerContent<'_> {
         if *version >= JavaMinecraftVersion::V_1_17_1 {
             write.write_var_int(&self.state_id)?;
         }
+
+        // The player inventory screen handler (window id 0) always includes the
+        // offhand slot at index 45. Offhand was introduced in 1.9, so clients
+        // older than that only expect 45 slots and would reject a 46-slot window
+        // 0 payload with `IndexOutOfBoundsException: Index: 45, Size: 45`.
+        let strip_offhand = *version < JavaMinecraftVersion::V_1_9
+            && self.window_id.0 == 0
+            && self.slot_data.len() == 46;
+        let slot_count = self.slot_data.len() - usize::from(strip_offhand);
+
         if *version >= JavaMinecraftVersion::V_1_17_1 {
-            let slot_count = i32::try_from(self.slot_data.len()).map_err(|_| {
-                WritingError::Message(format!(
-                    "{} slot entries do not fit in VarInt",
-                    self.slot_data.len()
-                ))
+            let slot_count = i32::try_from(slot_count).map_err(|_| {
+                WritingError::Message(format!("{slot_count} slot entries do not fit in VarInt"))
             })?;
             write.write_var_int(&VarInt(slot_count))?;
         } else {
-            let slot_count = i16::try_from(self.slot_data.len()).map_err(|_| {
-                WritingError::Message(format!(
-                    "{} slot entries do not fit in Short",
-                    self.slot_data.len()
-                ))
+            let slot_count = i16::try_from(slot_count).map_err(|_| {
+                WritingError::Message(format!("{slot_count} slot entries do not fit in Short"))
             })?;
             write.write_i16_be(slot_count)?;
         }
-        for stack in self.slot_data {
+        for (index, stack) in self.slot_data.iter().enumerate() {
+            if strip_offhand && index == 45 {
+                continue;
+            }
             stack.write_with_version(&mut write, version)?;
         }
         if *version >= JavaMinecraftVersion::V_1_17_1 {
@@ -72,3 +79,97 @@ impl ClientPacket for CSetContainerContent<'_> {
         Ok(())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::CSetContainerContent;
+    use crate::codec::item_stack_seralizer::ItemStackSerializer;
+    use crate::{ClientPacket, VarInt};
+    use pumpkin_data::item::Item;
+    use pumpkin_data::item_stack::ItemStack;
+    use pumpkin_util::version::JavaMinecraftVersion;
+
+    /// A 46-slot player inventory where the offhand slot (index 45) holds a
+    /// non-empty stone stack and every other slot is empty.
+    fn player_inventory_packet_with_offhand() -> (
+        Vec<ItemStackSerializer<'static>>,
+        ItemStackSerializer<'static>,
+    ) {
+        let stone = ItemStack::new(1, Item::from_id(1).expect("stone"));
+        let mut slots: Vec<ItemStackSerializer> = (0..45)
+            .map(|_| ItemStackSerializer::from(ItemStack::EMPTY.clone()))
+            .collect();
+        slots.push(ItemStackSerializer::from(stone));
+        let carried = ItemStackSerializer::from(ItemStack::EMPTY.clone());
+        (slots, carried)
+    }
+
+    #[test]
+    fn pre_1_9_player_inventory_omits_offhand_slot() {
+        let (slots, carried) = player_inventory_packet_with_offhand();
+        let packet = CSetContainerContent::new(VarInt(0), VarInt(0), &slots, &carried);
+
+        let mut out = Vec::new();
+        packet
+            .write_packet_data(&mut out, &JavaMinecraftVersion::V_1_8)
+            .unwrap();
+
+        // window id (u8) + slot count (i16) + 45 empty slots (i16 each).
+        // The non-empty offhand stack must be omitted entirely.
+        assert_eq!(out.len(), 1 + 2 + 45 * 2);
+        let count = i16::from_be_bytes([out[1], out[2]]);
+        assert_eq!(count, 45);
+    }
+
+    #[test]
+    fn post_1_9_player_inventory_keeps_offhand_slot() {
+        let (slots, carried) = player_inventory_packet_with_offhand();
+        let packet = CSetContainerContent::new(VarInt(0), VarInt(0), &slots, &carried);
+
+        let mut out = Vec::new();
+        packet
+            .write_packet_data(&mut out, &JavaMinecraftVersion::V_1_9)
+            .unwrap();
+
+        // window id (u8) + slot count (i16) + 45 empty slots (i16 each) + the
+        // non-empty offhand stack (i16 item id, i8 count, i16 damage, u8 NBT).
+        assert_eq!(out.len(), 1 + 2 + 45 * 2 + 6);
+        let count = i16::from_be_bytes([out[1], out[2]]);
+        assert_eq!(count, 46);
+        // The last slot is the offhand and must not be the empty-item marker (-1).
+        assert_ne!(&out[out.len() - 6..out.len() - 4], &(-1i16).to_be_bytes());
+    }
+
+    #[test]
+    fn offhand_survives_alternating_legacy_and_modern_joins() {
+        let (slots, carried) = player_inventory_packet_with_offhand();
+        let packet = CSetContainerContent::new(VarInt(0), VarInt(0), &slots, &carried);
+
+        // Sequence: 1.8 -> 26.2 -> 1.8 -> 26.2. Serialization is read-only, so
+        // the server-side offhand stack must never be mutated, and every modern
+        // (26.2) join must still encode the offhand item normally.
+        for _ in 0..2 {
+            // 1.8 join: the offhand slot is stripped from the wire.
+            let mut legacy = Vec::new();
+            packet
+                .write_packet_data(&mut legacy, &JavaMinecraftVersion::V_1_8)
+                .unwrap();
+            assert_eq!(legacy.len(), 1 + 2 + 45 * 2);
+            assert_eq!(i16::from_be_bytes([legacy[1], legacy[2]]), 45);
+            assert!(!slots[45].0.as_ref().is_empty());
+
+            // 26.2 join: the offhand item must still be present and encoded.
+            let mut modern = Vec::new();
+            packet
+                .write_packet_data(&mut modern, &JavaMinecraftVersion::V_26_2)
+                .unwrap();
+            // window id (VarInt) + state id (VarInt) + slot count (VarInt) +
+            // 45 empty slots (VarInt 0) + non-empty offhand (count, item id, add
+            // count, remove count as VarInts) + empty carried item (VarInt 0).
+            assert_eq!(modern.len(), 1 + 1 + 1 + 45 + 4 + 1);
+            // offhand: count 1, stone id 1, 0 components added, 0 removed.
+            assert_eq!(&modern[48..52], &[0x01, 0x01, 0x00, 0x00]);
+            assert!(!slots[45].0.as_ref().is_empty());
+        }
+    }
+}