Items usually don't appear on the client

[caelunshun]

When an item is spawned with the Legion rewrite, it usually doesn't appear on the client. Occasionally, the entity counter in the client debug menu will increment and then rapidly go back to 0, but the item stack still cannot be seen.

On the pre-Legion code, spawning items worked fine, so this is a bug confined to the new implementation.

Some debugging revealed that:

• The correct packets are being sent: SpawnObject followed by EntityMetadata, all with identical fields as before the rewrite.
• Very rarely, spawning will work correctly, and the client will observe the item correctly.
• No DestroyEntities packet is being sent which could delete the item.

[samlich]

It seems SpawnObject and EntityMetadata need to be sent in one go. I made it so the codec can send a Chained packet type which will send two packets at once in the code below. I think a good solution would be to change network.send from sending packets to the worker, to writing them to a buffer for the worker using the codec logic. It can have a vectored option to ensure nobody stuffs packets in between. This pushes more work onto the game logic threads, but perhaps makes up for it by removing all the boxes, I don't know. Does that sound good?

diff --git a/core/src/network/codec.rs b/core/src/network/codec.rs
index 41fd091..d880d57 100644
--- a/core/src/network/codec.rs
+++ b/core/src/network/codec.rs
@@ -179,7 +179,14 @@ impl Encoder for MinecraftCodec {
             crypter.encrypt(dst);
         }

-        Ok(())
+        if let Some(next) = packet.chained_next() {
+            let mut dst2 = dst.split_off(dst.len());
+            let ret = self.encode(next.box_clone(), &mut dst2);
+            dst.unsplit(dst2);
+            ret
+        } else {
+            Ok(())
+        }
     }
 }

--- a/core/src/network/packet/mod.rs
+++ b/core/src/network/packet/mod.rs
@@ -33,6 +33,86 @@ pub trait Packet: AsAny + IntoAny + Send + Sync + Any {

     /// Returns a clone of this packet in a dynamic box.
     fn box_clone(&self) -> Box<dyn Packet>;
+
+    fn chained_next(&self) -> Option<&dyn Packet> {
+        None
+    }
+}
+
+#[derive(Debug, Clone)]
+pub struct Chained<A, B: Clone + Packet> {
+    pub first: A,
+    pub second: B,
+}
+impl<A: Packet, B: Clone + Packet> AsAny for Chained<A, B> {
+    fn as_any(&self) -> &dyn Any {
+        self.first.as_any()
+    }
+}
+impl<B: Clone + Packet> AsAny for Chained<Box<dyn Packet>, B> {
+    fn as_any(&self) -> &dyn Any {
+        self.first.as_any()
+    }
+}
+impl<A: Packet, B: Clone + Packet> Packet for Chained<A, B> {
+    fn read_from(&mut self, buf: &mut Cursor<&[u8]>) -> Result<(), failure::Error> {
+        self.first.read_from(buf)
+    }
+    fn write_to(&self, buf: &mut BytesMut) {
+        self.first.write_to(buf)
+    }
+    fn ty(&self) -> PacketType {
+        self.first.ty()
+    }
+    fn ty_sized() -> PacketType
+    where
+        Self: Sized,
+    {
+        A::ty_sized()
+    }
+
+    /// Returns a clone of this packet in a dynamic box.
+    fn box_clone(&self) -> Box<dyn Packet> {
+        let b: Box<dyn Packet> = Box::new(Chained {
+            first: self.first.box_clone(),
+            second: self.second.clone(),
+        });
+        b
+    }
+
+    fn chained_next(&self) -> Option<&dyn Packet> {
+        Some(&self.second)
+    }
+}
+impl<B: Clone + Packet> Packet for Chained<Box<dyn Packet>, B> {
+    fn read_from(&mut self, buf: &mut Cursor<&[u8]>) -> Result<(), failure::Error> {
+        self.first.read_from(buf)
+    }
+    fn write_to(&self, buf: &mut BytesMut) {
+        self.first.write_to(buf)
+    }
+    fn ty(&self) -> PacketType {
+        self.first.ty()
+    }
+    fn ty_sized() -> PacketType
+    where
+        Self: Sized,
+    {
+        panic!()
+    }
+
+    /// Returns a clone of this packet in a dynamic box.
+    fn box_clone(&self) -> Box<dyn Packet> {
+        let b: Box<dyn Packet> = Box::new(Chained {
+            first: self.first.box_clone(),
+            second: self.second.clone(),
+        });
+        b
+    }
+
+    fn chained_next(&self) -> Option<&dyn Packet> {
+        Some(&self.second)
+    }
 }
--- a/server/src/broadcasters/entity_creation.rs
+++ b/server/src/broadcasters/entity_creation.rs
@@ -16,7 +16,12 @@ fn broadcast_entity_creation(
     state: &State,
     accessor1: &QueryAccessor<Read<CreationPacketCreator>>,
     accessor2: &QueryAccessor<Read<SpawnPacketCreator>>,
-    _query: &mut Query<Read<Position>>,
+    _query: &mut Query<(
+        Read<Position>,
+        Read<crate::metadata::Metadata>,
+        Read<crate::entity::EntityId>,
+        Read<crate::network::Network>,
+    )>,
     world: &mut PreparedWorld,
     holders: &ChunkHolders,
 ) {
@@ -31,7 +36,40 @@ fn broadcast_entity_creation(
         if let Some(accessor) = accessor2.find(event.entity) {
             if let Some(packet_creator) = accessor.get_component::<SpawnPacketCreator>(world) {
                 let packet = packet_creator.get(&accessor, world);
-                state.broadcast_entity_update_boxed(event.entity, packet, Some(event.entity));
+                // state.broadcast_entity_update_boxed(event.entity, packet, Some(event.entity));
+                if let Some(meta) = world.get_component::<crate::metadata::Metadata>(event.entity) {
+                    let chunk = world
+                        .get_component::<Position>(event.entity)
+                        .unwrap()
+                        .chunk_pos();
+                    for entity in holders.holders_for(chunk).unwrap_or(&[]) {
+                        if let Some(network) =
+                            world.get_component::<crate::network::Network>(*entity)
+                        {
+                            use feather_core::network::packet::implementation::PacketEntityMetadata;
+                            // network.send_boxed(packet.box_clone());
+
+                            let entity_id = world
+                                .get_component::<crate::entity::EntityId>(event.entity)
+                                .unwrap()
+                                .0;
+                            let packet = feather_core::network::packet::Chained {
+                                first: packet.box_clone(),
+                                second: PacketEntityMetadata {
+                                    entity_id,
+                                    metadata: meta.to_full_raw_metadata(),
+                                },
+                            };
+
+                            /*let packet = PacketEntityMetadata {
+                                entity_id,
+                                metadata: meta.to_full_raw_metadata(),
+                            };*/
+                            //dbg!(&packet);
+                            network.send(packet);
+                        }
+                    }
+                }
             }
         }

I tried this beforehand which doesn't require changing the codec logic. It worked much more of the time, but still failed often, so changes to how packets are sent are necessary.

diff --git a/server/src/broadcasters/entity_creation.rs b/server/src/broadcasters/entity_creation.rs
index 88751b5..baaf3c7 100644
--- a/server/src/broadcasters/entity_creation.rs
+++ b/server/src/broadcasters/entity_creation.rs
@@ -16,7 +16,12 @@ fn broadcast_entity_creation(
     state: &State,
     accessor1: &QueryAccessor<Read<CreationPacketCreator>>,
     accessor2: &QueryAccessor<Read<SpawnPacketCreator>>,
-    _query: &mut Query<Read<Position>>,
+    _query: &mut Query<(
+        Read<Position>,
+        Read<crate::metadata::Metadata>,
+        Read<crate::entity::EntityId>,
+        Read<crate::network::Network>,
+    )>,
     world: &mut PreparedWorld,
     holders: &ChunkHolders,
 ) {
@@ -31,7 +36,31 @@ fn broadcast_entity_creation(
         if let Some(accessor) = accessor2.find(event.entity) {
             if let Some(packet_creator) = accessor.get_component::<SpawnPacketCreator>(world) {
                 let packet = packet_creator.get(&accessor, world);
-                state.broadcast_entity_update_boxed(event.entity, packet, Some(event.entity));
+                // state.broadcast_entity_update_boxed(event.entity, packet, Some(event.entity));
+                if let Some(meta) = world.get_component::<crate::metadata::Metadata>(event.entity) {
+                    let chunk = world
+                        .get_component::<Position>(event.entity)
+                        .unwrap()
+                        .chunk_pos();
+                    for entity in holders.holders_for(chunk).unwrap_or(&[]) {
+                        if let Some(network) =
+                            world.get_component::<crate::network::Network>(*entity)
+                        {
+                            use feather_core::network::packet::implementation::PacketEntityMetadata;
+                            network.send_boxed(packet.box_clone());
+                            let entity_id = world
+                                .get_component::<crate::entity::EntityId>(event.entity)
+                                .unwrap()
+                                .0;
+                            let packet = PacketEntityMetadata {
+                                entity_id,
+                                metadata: meta.to_full_raw_metadata(),
+                            };
+                            dbg!(&packet);
+                            network.send(packet);
+                        }
+                    }
+                }
             }
         }

Also, when I try to decode SpawnObject, all of the fields after uuid are read from the wrong position, am I doing something wrong?

SpawnObject {
    entity_id: 1,
    object_uuid: 236878dc-0c35-4731-9bd6-7ff83e104c30,
    ty: 2,
    x: 0.0,
    y: 64.32000000000001,
    z: 0.0,
    pitch: 14,
    yaw: 232,
    data: 1,
    velocity_x: 1292,
    velocity_y: 30,
    velocity_z: 1817,
}
[1, 35, 104, 120, 220, 12, 53, 71, 49, 155, 214, 127, 248, 62, 16, 76, 48, 2, 0, 0, 0, 0, 0, 0, 0, 0, 64, 80, 20, 122, 225, 71, 174, 21, 0, 0, 0, 0, 0, 0, 0, 0, 14, 232, 0, 0, 0, 1, 5, 12, 0, 30, 7, 25]
SpawnObject { entity_id: 1,
 object_uuid: 236878dc-0c35-4731-9bd6-7ff83e104c30,
 ty: 35,
 x: 8147243540018832000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000.0,
y:-4692637175077517000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.0,
z: 0.0, pitch: 64, yaw: 80, data: 343597383, velocity_x: -20971, velocity_y: 0, velocity_z: 0 }

I'm writing then reading back the packet just before it's sent.

diff --git a/server/src/io/worker.rs b/server/src/io/worker.rs
index c6d9405..f17695a 100644
--- a/server/src/io/worker.rs
+++ b/server/src/io/worker.rs
@@ -96,7 +96,41 @@ async fn _run_worker(
+                        if packet.ty() == feather_core::network::packet::PacketType::SpawnObject {
+                            use feather_core::Packet;
+                            let mut bytes = bytes::BytesMut::new();
+                            packet.write_to(&mut bytes);
+                            let mut p =
+                                feather_core::network::packet::implementation::SpawnObject::default(
+                                );
+                            error!("{:?}", bytes.as_ref());
+                            match p.read_from(&mut std::io::Cursor::new(&bytes)) {
+                                Ok(()) => error!("Read {:?}", p),
+                                Err(p) => error!("Error reading {:?}", p),
+                            }
+                        }

[caelunshun]

Interesting, thanks! As I understand it, SpawnObject and EntityMetadata have to be sent within a certain timeframe of each other—is this correct? I took a look at the old codebase and it does indeed send both packets in the same function so that they are sent together. I find it odd that your attempt at doing this still failed on occasion—it worked invariably on the old codebase, so there is likely another bug hiding somewhere.

Your idea of writing packets to a buffer on the game logic threads and then only writing them out on the IO threads is worth a try; I'll implement this tonight.

As for SpawnObject not being decoded correctly, that seems to be a bug with our implementation of Buf::get_uuid—it doesn't advance the current reader index. I'll get that fixed today.

[caelunshun]

Something else I found out: if the item doesn't appear when it is dropped, it does show on the client upon relog.

[samlich]

Seems like this is fixed by 6895f44.

[caelunshun]

Seems like this is fixed by 6895f44.

I've still observed it a few times; are you sure?

I have the feeling this is caused by dtolnay/inventory#7—some systems don't seem to be registered properly. The refactor in the hopefully-last-refactor branch should fix this definitively.