Fix replication with visibility policy != VisibilityPolicy::All

Visibility was coupled with a check if a client was just connected.
This resulted in a passed test that checks replication of previously
spawned entities (it was running with `VisibilityPolicy::All`). But for
all other policies the logic was incorrect and led to a crash with
"entity should be present after adding component".

I decoupled the logic. We now determine if the entity is new to a
client by checking if it has a tick.

I thinking that we need to rename "change limit" into "init tick" because
we use this terminology on client. But it will be a breaking change, so
I will leave it for the next release.

Author: Shatur

CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [0.25.1] - 2024-05-16
 
+- Fix replicating previously spawned entities to a newly connected client with visibility policy different from `VisibilityPolicy::All`.
+
 ### Fixed
 
 - Fix possible overflow in `Confirmed::contains_any`.

src/server.rs

@@ -368,21 +368,12 @@ fn collect_changes(
                         continue;
                     }
 
-                    let new_entity = marker_added || visibility == Visibility::Gained;
-                    if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run())
+                    if let Some(tick) = client
+                        .get_change_limit(entity.id())
+                        .filter(|_| !marker_added)
+                        .filter(|_| visibility != Visibility::Gained)
+                        .filter(|_| !ticks.is_added(change_tick.last_run(), change_tick.this_run()))
                     {
-                        init_message.write_component(
-                            &mut shared_bytes,
-                            rule_fns,
-                            component_fns,
-                            &ctx,
-                            replicated_component.fns_id,
-                            component,
-                        )?;
-                    } else {
-                        let tick = client
-                            .get_change_limit(entity.id())
-                            .expect("entity should be present after adding component");
                         if ticks.is_changed(tick, change_tick.this_run()) {
                             update_message.write_component(
                                 &mut shared_bytes,
@@ -393,6 +384,15 @@ fn collect_changes(
                                 component,
                             )?;
                         }
+                    } else {
+                        init_message.write_component(
+                            &mut shared_bytes,
+                            rule_fns,
+                            component_fns,
+                            &ctx,
+                            replicated_component.fns_id,
+                            component,
+                        )?;
                     }
                 }
             }

src/server/connected_clients/client_visibility.rs

@@ -20,9 +20,7 @@ impl ClientVisibility {
     /// Creates a new instance based on the preconfigured policy.
     pub(super) fn new(policy: VisibilityPolicy) -> Self {
         match policy {
-            VisibilityPolicy::All => Self::with_filter(VisibilityFilter::All {
-                just_connected: true,
-            }),
+            VisibilityPolicy::All => Self::with_filter(VisibilityFilter::All),
             VisibilityPolicy::Blacklist => Self::with_filter(VisibilityFilter::Blacklist {
                 list: Default::default(),
                 added: Default::default(),
@@ -49,7 +47,7 @@ impl ClientVisibility {
     /// `cached_visibility` remains untouched.
     pub(super) fn clear(&mut self) {
         match &mut self.filter {
-            VisibilityFilter::All { just_connected } => *just_connected = true,
+            VisibilityFilter::All => (),
             VisibilityFilter::Blacklist {
                 list,
                 added,
@@ -76,7 +74,7 @@ impl ClientVisibility {
     /// Should be called after each tick.
     pub(crate) fn update(&mut self) {
         match &mut self.filter {
-            VisibilityFilter::All { just_connected } => *just_connected = false,
+            VisibilityFilter::All => (),
             VisibilityFilter::Blacklist {
                 list,
                 added,
@@ -250,13 +248,7 @@ impl ClientVisibility {
     /// Returns visibility of a specific entity.
     fn get_visibility_state(&self, entity: Entity) -> Visibility {
         match &self.filter {
-            VisibilityFilter::All { just_connected } => {
-                if *just_connected {
-                    Visibility::Gained
-                } else {
-                    Visibility::Visible
-                }
-            }
+            VisibilityFilter::All => Visibility::Visible,
             VisibilityFilter::Blacklist { list, .. } => match list.get(&entity) {
                 Some(BlacklistInfo::QueuedForRemoval) => Visibility::Gained,
                 Some(BlacklistInfo::Hidden) => Visibility::Hidden,
@@ -273,12 +265,7 @@ impl ClientVisibility {
 
 /// Filter for [`ClientVisibility`] based on [`VisibilityPolicy`].
 enum VisibilityFilter {
-    All {
-        /// Indicates that the client has just connected to the server.
-        ///
-        /// If true, then visibility of all entities has been gained.
-        just_connected: bool,
-    },
+    All,
     Blacklist {
         /// All blacklisted entities and an indicator of whether they are in the queue for deletion
         /// at the end of this tick.

tests/visibility.rs

@@ -131,7 +131,7 @@ fn blacklist() {
 }
 
 #[test]
-fn blacklist_despawn() {
+fn blacklist_with_despawn() {
     let mut server_app = App::new();
     let mut client_app = App::new();
     for app in [&mut server_app, &mut client_app] {
@@ -250,7 +250,7 @@ fn whitelist() {
 }
 
 #[test]
-fn whitelist_despawn() {
+fn whitelist_with_despawn() {
     let mut server_app = App::new();
     let mut client_app = App::new();
     for app in [&mut server_app, &mut client_app] {