RemoteEntityMap::insert is not called on the client for client-replicated entities

[philpax]

An entity that is spawned on the client and replicated up to the server with Replicate::default will not be added to the client's RemoteEntityMap.

This means that, for example, an interest management despawn will fail: the server will map the entity to the client's local entity, but RemoteEntityMap::remove_by_remote will fail to find the entity, resulting in the entity not being despawned.

Applying the following diff:

diff --git a/lightyear/src/server/connection.rs b/lightyear/src/server/connection.rs
index 246da77e..6d015be4 100644
--- a/lightyear/src/server/connection.rs
+++ b/lightyear/src/server/connection.rs
@@ -841,11 +841,13 @@ impl ConnectionManager {
             // );
 
             // convert the entity to a network entity (possibly mapped)
+            info!("preparing entity despawn for pre-map {entity}");
             entity = self
                 .connection_mut(client_id)?
                 .replication_receiver
                 .remote_entity_map
                 .to_remote(entity);
+            info!("preparing entity despawn for post-map {entity}");
 
             self.connection_mut(client_id)?
                 .replication_sender
diff --git a/lightyear/src/server/replication.rs b/lightyear/src/server/replication.rs
index d6b734a9..3564759d 100644
--- a/lightyear/src/server/replication.rs
+++ b/lightyear/src/server/replication.rs
@@ -790,6 +790,7 @@ pub(crate) mod send {
         }
 
         if !target.is_empty() {
+            info!("Replicate entity despawn: {entity} {group_id:?} {target:?}");
             let _ = sender
                 .prepare_entity_despawn(entity, group_id, target)
                 .inspect_err(|e| {
diff --git a/lightyear/src/shared/replication/entity_map.rs b/lightyear/src/shared/replication/entity_map.rs
index 69552b2b..1959f69f 100644
--- a/lightyear/src/shared/replication/entity_map.rs
+++ b/lightyear/src/shared/replication/entity_map.rs
@@ -72,6 +72,7 @@ impl RemoteEntityMap {
     pub fn insert(&mut self, remote_entity: Entity, local_entity: Entity) {
         self.remote_to_local.insert(remote_entity, local_entity);
         self.local_to_remote.insert(local_entity, remote_entity);
+        tracing::info!("Inserted mapped entity: local {local_entity} remote {remote_entity}");
     }
 
     // pub(crate) fn get_to_remote_mapper(&self) -> Box<dyn EntityMapper + '_> {
@@ -153,15 +154,20 @@ impl RemoteEntityMap {
     pub(super) fn remove_by_remote(&mut self, remote_entity: Entity) -> Option<Entity> {
         // the entity is actually local, because it has already been mapped!
         if Self::is_mapped(remote_entity) {
+            tracing::info!("Removing mapped entity: {remote_entity}");
             let local = Self::mark_unmapped(remote_entity);
+            tracing::info!("Unmapped entity: {local}");
             if let Some(remote) = self.local_to_remote.remove(&local) {
+                tracing::info!("Removing entity: local {local} remote {remote}");
                 self.remote_to_local.remove(&remote);
                 return Some(local);
             }
         } else if let Some(local) = self.remote_to_local.remove(&remote_entity) {
+            tracing::info!("Removing unmapped entity: local {local} remote {remote_entity}");
             self.local_to_remote.remove(&local);
             return Some(local);
         }
+        tracing::info!("Entity not found: {remote_entity} {self:?}");
         None
     }

as well as changing the log levels of some other logs, due to environment issues, produces the following logs on the server and client upon an interest-management despawn:
Server:

2024-09-16T03:56:53.125254Z  INFO lightyear::server::replication::send: Replicate entity despawn: 11v1 ReplicationGroupId(4294967307) Single(Netcode(59838886875065533))
2024-09-16T03:56:53.125331Z  INFO lightyear::server::connection: preparing entity despawn for pre-map 11v1
2024-09-16T03:56:53.125384Z  INFO lightyear::server::connection: preparing entity despawn for post-map 88v1073741825

Client:

2024-09-16T03:56:53.130311Z  INFO lightyear::shared::replication::receive: Received entity actions remote_entity=Entity { index: 88, generation: 1073741825 }
2024-09-16T03:56:53.130621Z  INFO lightyear::shared::replication::receive: Received entity despawn remote_entity=Entity { index: 88, generation: 1073741825 }
2024-09-16T03:56:53.130846Z  INFO lightyear::shared::replication::entity_map: Removing mapped entity: 88v1073741825
2024-09-16T03:56:53.131059Z  INFO lightyear::shared::replication::entity_map: Unmapped entity: 88v1
2024-09-16T03:56:53.131292Z  INFO lightyear::shared::replication::entity_map: Entity not found: 88v1073741825 RemoteEntityMap { remote_to_local: ReceiveEntityMap({Entity { index: 9, generation: 1 }: Entity { index: 85, generation: 1 }}), local_to_remote: SendEntityMap({Entity { index: 85, generation: 1 }: Entity { index: 9, generation: 1 }}) }
2024-09-16T03:56:53.131592Z ERROR lightyear::shared::replication::receive: Received despawn for an entity that does not exist

[philpax]

I think I'm seeing this issue the other way around as well: a server-spawned entity that the client then assumes authority of will result in mapping failures on the server. The scenario in which I'm seeing this is quite noisy, so I think I'll have to collect more information at a later stage.

[cBournhonesque]

Yes I think it's similar to what I identified here: #642

Basically I want to move away from relying on the fact that the receiver always has the entity in the entity map. So it's expected that an entity that is replicated from client to server will not be present in the client's RemoteEntityMap. Normally everything should work despite this, but apparently not.

It looks like the problem is that

    /// Remove the entity from our mapping and return the local entity
    pub(super) fn remove_by_remote(&mut self, remote_entity: Entity) -> Option<Entity> {
        // the entity is actually local, because it has already been mapped!
        if Self::is_mapped(remote_entity) {
            let local = Self::mark_unmapped(remote_entity);
            if let Some(remote) = self.local_to_remote.remove(&local) {
                self.remote_to_local.remove(&remote);
                return Some(local);
            }
        } else if let Some(local) = self.remote_to_local.remove(&remote_entity) {
            self.local_to_remote.remove(&local);
            return Some(local);
        }
        None
    }

should always return Some(local) even if we don't find local in the self.local_to_remote mapping?

The flow is:

• client creates 88v1 and sends it to the server.
• server spawns 11v1 and maps (client:88v1 to server:11v1)
• server sends a message to despawn 11v1. The server has the entity-mapping, so it maps 11v1 to 88v1 + it flips a high bit to indicate that a mapping was done: 88v1073741825
• the client receives the 88v1073741825, it knows that mapping was done because of the high bit; it unflips the bit to recover 88v1
• BUG: in remove_by_remote, we don't find 88v1 in the local mapping so we return None. But instead we should just return Some(local), because the entity has already been mapped!!

TODO:

• implement the easy fix (fix the remove_by_remote function)
• add a unit test for this case

[philpax]

Makes sense! There's no rush on our part; we have other things we can work on. Take as much time as you need 🙂

[cBournhonesque]

I haven't had the time to create a unit test; but does #656 fix the issue?

[philpax]

Will test at my soonest convenience!

[philpax]

Apologies for the delay on this. I've tested #656; the behaviour I'm seeing now is that the entity correctly despawns when the player teleports away, but this despawn is replicated to the server, resulting in the server despawning the entity. The ControlledBy target has a target of NetworkTarget::None, so I suspect that the client isn't checking ControlledBy when issuing that despawn.

I think that PR can probably be merged and the behaviour I'm seeing can be fixed in a separate commit/PR - I think it's a separate issue that this PR is revealing.