Fix position syncronization issues for entities and fixes loading and saving of playerdata to use the correct inventory slot indexes (#498)

* Add a previousOnGround component to track changes.

* Add OnGround check for entity movement.

* Check if OnGround has changed and send a different packet.

* remove clone

* Fixes saving and loading playerdata to use correct indexes.

* Add doc linking to from_network_index and from_inventory_index.

* Fix comment links.

* Explain panic.

* Add more comments and links.

* fix typo

* Add documentation for set_item on window.

* Add comment about why the unwraps are safe.

* Add comment explaining why unwrap is safe.

Author: Tracreed

feather/base/src/anvil/player.rs

@@ -70,8 +70,8 @@ pub struct InventorySlot {
 }
 
 impl InventorySlot {
-    /// Converts an `ItemStack` and network protocol index into an `InventorySlot`.
-    pub fn from_network_index(network: usize, stack: ItemStack) -> Option<Self> {
+    /// Converts an [`ItemStack`] and network protocol index into an [`InventorySlot`].
+    pub fn from_network_index(network: usize, stack: &ItemStack) -> Option<Self> {
         let slot = if SLOT_HOTBAR_OFFSET <= network && network < SLOT_HOTBAR_OFFSET + HOTBAR_SIZE {
             // Hotbar
             (network - SLOT_HOTBAR_OFFSET) as i8
@@ -90,8 +90,8 @@ impl InventorySlot {
         Some(Self::from_inventory_index(slot, stack))
     }
 
-    /// Converts an `ItemStack` and inventory position index into an `InventorySlot`.
-    pub fn from_inventory_index(slot: i8, stack: ItemStack) -> Self {
+    /// Converts an [`ItemStack`] and inventory position index into an [`InventorySlot`].
+    pub fn from_inventory_index(slot: i8, stack: &ItemStack) -> Self {
         let nbt = stack.clone().into();
         let nbt = if nbt == Default::default() {
             None
@@ -287,7 +287,7 @@ mod tests {
             assert_eq!(
                 InventorySlot::from_network_index(
                     expected,
-                    ItemStack::new(Item::Stone, 1).unwrap()
+                    &ItemStack::new(Item::Stone, 1).unwrap()
                 ),
                 Some(slot),
             );

feather/common/src/game.rs

@@ -104,7 +104,7 @@ impl Game {
         self.entity_spawn_callbacks.push(Box::new(callback));
     }
 
-    /// Creates an emtpy entity builder to create entities in
+    /// Creates an empty entity builder to create entities in
     /// the ecs world.
     pub fn create_empty_entity_builder(&mut self) -> EntityBuilder {
         mem::take(&mut self.entity_builder)

feather/common/src/window.rs

@@ -353,6 +353,9 @@ impl Window {
         self.inner.item(index)
     }
 
+    /// Sets an [`InventorySlot`] at the index.
+    /// # Error
+    /// Returns an error if the index is [`WindowError::OutOfBounds`]
     pub fn set_item(&self, index: usize, item: InventorySlot) -> Result<(), WindowError> {
         self.inner.set_item(index, item)
     }

feather/server/src/client.rs

@@ -20,7 +20,7 @@ use common::{
 use libcraft_items::InventorySlot;
 use packets::server::{Particle, SetSlot, SpawnLivingEntity, UpdateLight, WindowConfirmation};
 use protocol::packets::server::{
-    EntityPosition, EntityPositionAndRotation, HeldItemChange, PlayerAbilities,
+    EntityPosition, EntityPositionAndRotation, EntityTeleport, HeldItemChange, PlayerAbilities,
 };
 use protocol::{
     packets::{
@@ -37,7 +37,10 @@ use protocol::{
 use quill_common::components::{OnGround, PreviousGamemode};
 
 use crate::{
-    entities::PreviousPosition, initial_handler::NewPlayer, network_id_registry::NetworkId, Options,
+    entities::{PreviousOnGround, PreviousPosition},
+    initial_handler::NewPlayer,
+    network_id_registry::NetworkId,
+    Options,
 };
 use slab::Slab;
 
@@ -386,6 +389,7 @@ impl Client {
         position: Position,
         prev_position: PreviousPosition,
         on_ground: OnGround,
+        prev_on_ground: PreviousOnGround,
     ) {
         if self.network_id == Some(network_id) {
             // This entity is the client. Only update
@@ -400,6 +404,21 @@ impl Client {
         let no_change_yaw = (position.yaw - prev_position.0.yaw).abs() < 0.001;
         let no_change_pitch = (position.pitch - prev_position.0.pitch).abs() < 0.001;
 
+        // If the entity jumps or falls we should send a teleport packet instead to keep relative movement in sync.
+        if on_ground != prev_on_ground.0 {
+            self.send_packet(EntityTeleport {
+                entity_id: network_id.0,
+                x: position.x,
+                y: position.y,
+                z: position.z,
+                yaw: position.yaw,
+                pitch: position.pitch,
+                on_ground: *on_ground,
+            });
+
+            return;
+        }
+
         if no_change_yaw && no_change_pitch {
             self.send_packet(EntityPosition {
                 entity_id: network_id.0,

feather/server/src/entities.rs

@@ -1,6 +1,6 @@
 use base::{EntityKind, Position};
 use ecs::{EntityBuilder, EntityRef, SysResult};
-use quill_common::entity_init::EntityInit;
+use quill_common::{components::OnGround, entity_init::EntityInit};
 use uuid::Uuid;
 
 use crate::{Client, NetworkId};
@@ -15,17 +15,31 @@ impl SpawnPacketSender {
     }
 }
 
-/// Stores the position of an entity on
+/// Stores the [`Position`] of an entity on
 /// the previous tick. Used to determine
 /// when to send movement updates.
 #[derive(Copy, Clone, Debug)]
 pub struct PreviousPosition(pub Position);
+/// Stores the [`OnGround`] status of an entity on
+/// the previous tick. Used to determine
+/// what movement packet to send.
+#[derive(Copy, Clone, Debug)]
+pub struct PreviousOnGround(pub OnGround);
 
 pub fn add_entity_components(builder: &mut EntityBuilder, init: &EntityInit) {
     if !builder.has::<NetworkId>() {
         builder.add(NetworkId::new());
     }
-    builder.add(PreviousPosition(*builder.get::<Position>().unwrap()));
+
+    // can't panic because this is only called after both position and onground is added to all entities.
+    // Position is added in the caller of this function and on_ground is added in the
+    // build default function. All entity builder functions call the build default function.
+    let prev_position = *builder.get::<Position>().unwrap();
+    let on_ground = *builder.get::<OnGround>().unwrap();
+
+    builder
+        .add(PreviousPosition(prev_position))
+        .add(PreviousOnGround(on_ground));
     add_spawn_packet(builder, init);
 }
 

feather/server/src/systems/entity.rs

@@ -12,7 +12,10 @@ use quill_common::{
     events::{SneakEvent, SprintEvent},
 };
 
-use crate::{entities::PreviousPosition, NetworkId, Server};
+use crate::{
+    entities::{PreviousOnGround, PreviousPosition},
+    NetworkId, Server,
+};
 
 mod spawn_packet;
 
@@ -27,17 +30,32 @@ pub fn register(game: &mut Game, systems: &mut SystemExecutor<Game>) {
 
 /// Sends entity movement packets.
 fn send_entity_movement(game: &mut Game, server: &mut Server) -> SysResult {
-    for (_, (&position, prev_position, &on_ground, &network_id)) in game
+    for (_, (&position, prev_position, &on_ground, &network_id, prev_on_ground)) in game
         .ecs
-        .query::<(&Position, &mut PreviousPosition, &OnGround, &NetworkId)>()
+        .query::<(
+            &Position,
+            &mut PreviousPosition,
+            &OnGround,
+            &NetworkId,
+            &mut PreviousOnGround,
+        )>()
         .iter()
     {
         if position != prev_position.0 {
             server.broadcast_nearby_with(position, |client| {
-                client.update_entity_position(network_id, position, *prev_position, on_ground);
+                client.update_entity_position(
+                    network_id,
+                    position,
+                    *prev_position,
+                    on_ground,
+                    *prev_on_ground,
+                );
             });
             prev_position.0 = position;
         }
+        if on_ground != prev_on_ground.0 {
+            prev_on_ground.0 = on_ground;
+        }
     }
     Ok(())
 }

feather/server/src/systems/player_join.rs

@@ -83,12 +83,19 @@ fn accept_new_player(game: &mut Game, server: &mut Server, client_id: ClientId)
         player: inventory.new_handle(),
     });
     if let Ok(data) = player_data.as_ref() {
-        for slot in data.inventory.iter() {
+        for inventory_slot in data.inventory.iter() {
+            let net_slot = inventory_slot.convert_index();
+            let slot = match net_slot {
+                Some(slot) => slot,
+                None => {
+                    log::error!("Failed to convert saved slot into network slot");
+                    continue;
+                }
+            };
+
+            // This can't fail since the earlier match filters out all incorrect indexes.
             window
-                .set_item(
-                    slot.slot as usize,
-                    InventorySlot::Filled(ItemStack::from(slot)),
-                )
+                .set_item(slot, InventorySlot::Filled(ItemStack::from(inventory_slot)))
                 .unwrap();
         }
     }

feather/server/src/systems/player_leave.rs

@@ -1,6 +1,6 @@
 use num_traits::cast::ToPrimitive;
 
-use base::anvil::entity::{AnimalData, BaseEntityData, ItemNbt};
+use base::anvil::entity::{AnimalData, BaseEntityData};
 use base::anvil::player::{InventorySlot, PlayerAbilities, PlayerData};
 use base::{Gamemode, Inventory, Position, Text};
 use common::entities::player::HotbarSlot;
@@ -130,21 +130,16 @@ fn create_player_data(
             // Here we filter out all empty slots.
             .filter_map(|(slot, item)| {
                 match item {
-                    libcraft_items::InventorySlot::Filled(item) => Some(InventorySlot {
-                        count: item.count() as i8,
-                        slot: slot as i8,
-                        item: item.item().name().to_owned(),
-                        nbt: {
-                            let nbt = ItemNbt {
-                                damage: item.damage_taken().map(|damage| damage as i32),
-                            };
-                            if nbt.damage.is_none() {
+                    libcraft_items::InventorySlot::Filled(item) => {
+                        let res = InventorySlot::from_network_index(slot, item);
+                        match res {
+                            Some(i) => Some(i),
+                            None => {
+                                log::error!("Failed to convert the slot into anvil format.");
                                 None
-                            } else {
-                                Some(nbt)
                             }
-                        },
-                    }),
+                        }
+                    }
                     libcraft_items::InventorySlot::Empty => {
                         // Empty items are filtered out.
                         None