From 3b28b5bb09c7da2fe5cea3a65c321aca06ca4943 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sat, 13 Jan 2024 17:54:08 +0200 Subject: [PATCH 01/48] Initial prototype --- src/network_event/server_event.rs | 5 +- src/server.rs | 37 ++++- src/server/clients_info.rs | 81 +++++++--- src/server/clients_info/client_visibility.rs | 157 +++++++++++++++++++ src/server/replication_messages.rs | 1 + 5 files changed, 249 insertions(+), 32 deletions(-) create mode 100644 src/server/clients_info/client_visibility.rs diff --git a/src/network_event/server_event.rs b/src/network_event/server_event.rs index 34e5e6ba..2c64b13c 100644 --- a/src/network_event/server_event.rs +++ b/src/network_event/server_event.rs @@ -323,10 +323,7 @@ pub fn send_with( } SendMode::Direct(client_id) => { if client_id != SERVER_ID { - if let Some(client_info) = clients_info - .iter() - .find(|client_info| client_info.id() == client_id) - { + if let Some(client_info) = clients_info.get(client_id) { let message = serialize_with(client_info, None, &serialize_fn)?; server.send_message(client_info.id(), channel, message.bytes); } diff --git a/src/server.rs b/src/server.rs index d41055c5..a40ae67b 100644 --- a/src/server.rs +++ b/src/server.rs @@ -27,7 +27,7 @@ use bevy_renet::{ use crate::replicon_core::{ replication_rules::ReplicationRules, replicon_tick::RepliconTick, ReplicationChannel, }; -use clients_info::{ClientBuffers, ClientInfo, ClientsInfo}; +use clients_info::{client_visibility::EntityVisibility, ClientBuffers, ClientInfo, ClientsInfo}; use despawn_buffer::{DespawnBuffer, DespawnBufferPlugin}; use removal_buffer::{RemovalBuffer, RemovalBufferPlugin}; use replicated_archetypes_info::ReplicatedArchetypesInfo; @@ -39,6 +39,9 @@ pub struct ServerPlugin { /// Tick configuration. pub tick_policy: TickPolicy, + /// Visibility configuration. + pub visibility_policy: VisibilityPolicy, + /// The time after which updates will be considered lost if an acknowledgment is not received for them. /// /// In practice updates will live at least `update_timeout`, and at most `2*update_timeout`. @@ -49,6 +52,7 @@ impl Default for ServerPlugin { fn default() -> Self { Self { tick_policy: TickPolicy::MaxTickRate(30), + visibility_policy: Default::default(), update_timeout: Duration::from_secs(10), } } @@ -62,9 +66,9 @@ impl Plugin for ServerPlugin { RenetServerPlugin, NetcodeServerPlugin, )) - .init_resource::() .init_resource::() .init_resource::() + .insert_resource(ClientsInfo::new(self.visibility_policy)) .configure_sets(PreUpdate, ServerSet::Receive.after(RenetReceive)) .configure_sets(PostUpdate, ServerSet::Send.before(RenetSend)) .add_systems( @@ -326,7 +330,8 @@ fn collect_changes( let mut shared_bytes = None; for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let new_entity = marker_added || client_info.just_connected; + let visibility = client_info.visibility().get(entity.entity()); + let new_entity = marker_added || visibility == EntityVisibility::JustVisible; if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run()) { init_message.write_component( @@ -335,7 +340,7 @@ fn collect_changes( component_info.replication_id, component, )?; - } else { + } else if visibility == EntityVisibility::Visible { let tick = client_info .get_change_limit(entity.entity()) .expect("entity should be present after adding component"); @@ -352,7 +357,8 @@ fn collect_changes( } for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let new_entity = marker_added || client_info.just_connected; + let visibility = client_info.visibility().get(entity.entity()); + let new_entity = marker_added || visibility == EntityVisibility::JustVisible; if new_entity || init_message.entity_data_size() != 0 { // If there is any insertion or we must initialize, include all updates into init message // and bump the last acknowledged tick to keep entity updates atomic. @@ -367,8 +373,7 @@ fn collect_changes( } } - for (init_message, _, client_info) in messages.iter_mut_with_info() { - client_info.just_connected = false; + for (init_message, _) in messages.iter_mut() { init_message.end_array()?; } @@ -422,6 +427,12 @@ fn collect_despawns( } } + for (message, _, client_info) in messages.iter_mut_with_info() { + for entity in client_info.remove_hidden() { + message.write_entity(&mut None, entity)?; + } + } + for (message, _) in messages.iter_mut() { message.end_array()?; } @@ -496,6 +507,18 @@ pub enum TickPolicy { Manual, } +/// Controls how visibility will be controlled via [`EntitiesVisibility`](entities_visibility::EntitiesVisibility). +#[derive(Default, Debug, Clone, Copy)] +pub enum VisibilityPolicy { + /// All entities are visible by default and visibility can't be changed. + #[default] + All, + /// All entities are hidden by default and should be explicitly marked to be visible. + Blacklist, + /// All entities are visible by default and should be explicitly marked to be hidden. + Whitelist, +} + /** A resource that exists on the server for mapping server entities to entities that clients have already spawned. The mappings are sent to clients as part of replication diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 833bbb28..5092680e 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -1,3 +1,5 @@ +pub mod client_visibility; + use std::mem; use bevy::{ @@ -7,11 +9,15 @@ use bevy::{ }; use bevy_renet::renet::ClientId; -use crate::replicon_core::replicon_tick::RepliconTick; +use crate::{replicon_core::replicon_tick::RepliconTick, server::VisibilityPolicy}; +use client_visibility::ClientVisibility; /// Stores meta-information about connected clients. -#[derive(Default, Resource)] -pub struct ClientsInfo(Vec); +#[derive(Resource, Default)] +pub struct ClientsInfo { + info: Vec, + policy: VisibilityPolicy, +} /// Reusable buffers for [`ClientsInfo`] and [`ClientInfo`]. #[derive(Default, Resource)] @@ -28,19 +34,38 @@ pub(crate) struct ClientBuffers { } impl ClientsInfo { + pub(super) fn new(policy: VisibilityPolicy) -> Self { + Self { + info: Default::default(), + policy, + } + } + + pub fn get(&self, client_id: ClientId) -> Option<&ClientInfo> { + self.info.iter().find(|info| info.id == client_id) + } + + pub fn get_mut(&mut self, client_id: ClientId) -> Option<&mut ClientInfo> { + self.info.iter_mut().find(|info| info.id == client_id) + } + /// Returns an iterator over clients information. - pub(crate) fn iter(&self) -> impl Iterator { - self.0.iter() + pub fn iter(&self) -> impl Iterator { + self.info.iter() } /// Returns a mutable iterator over clients information. - pub(super) fn iter_mut(&mut self) -> impl Iterator { - self.0.iter_mut() + pub fn iter_mut(&mut self) -> impl Iterator { + self.info.iter_mut() } /// Returns number of connected clients. - pub(super) fn len(&self) -> usize { - self.0.len() + pub fn len(&self) -> usize { + self.info.len() + } + + pub fn is_empty(&self) -> bool { + self.info.is_empty() } /// Initializes a new [`ClientInfo`] for this client. @@ -51,10 +76,10 @@ impl ClientsInfo { client_info.reset(client_id); client_info } else { - ClientInfo::new(client_id) + ClientInfo::new(client_id, self.policy) }; - self.0.push(client_info); + self.info.push(client_info); } /// Removes info for the client. @@ -62,11 +87,11 @@ impl ClientsInfo { /// Keeps allocated memory in the buffers for reuse. pub(super) fn remove(&mut self, client_buffers: &mut ClientBuffers, client_id: ClientId) { let index = self - .0 + .info .iter() .position(|info| info.id == client_id) .expect("clients info should contain all connected clients"); - let mut client_info = self.0.remove(index); + let mut client_info = self.info.remove(index); client_buffers.entities.extend(client_info.drain_entities()); client_buffers.info.push(client_info); } @@ -75,23 +100,22 @@ impl ClientsInfo { /// /// Keeps allocated memory in the buffers for reuse. pub(super) fn clear(&mut self, client_buffers: &mut ClientBuffers) { - for mut client_info in self.0.drain(..) { + for mut client_info in self.info.drain(..) { client_buffers.entities.extend(client_info.drain_entities()); client_buffers.info.push(client_info); } } } -pub(crate) struct ClientInfo { +pub struct ClientInfo { /// Client's ID. id: ClientId, - /// Indicates whether the client connected in this tick. - pub(super) just_connected: bool, - /// Lowest tick for use in change detection for each entity. ticks: EntityHashMap, + visibility: ClientVisibility, + /// The last tick in which a replicated entity was spawned, despawned, or gained/lost a component from the perspective /// of the client. /// @@ -108,11 +132,11 @@ pub(crate) struct ClientInfo { } impl ClientInfo { - fn new(id: ClientId) -> Self { + fn new(id: ClientId, policy: VisibilityPolicy) -> Self { Self { id, - just_connected: true, ticks: Default::default(), + visibility: ClientVisibility::new(policy), change_tick: Default::default(), updates: Default::default(), next_update_index: Default::default(), @@ -124,6 +148,14 @@ impl ClientInfo { self.id } + pub fn visibility_mut(&mut self) -> &mut ClientVisibility { + &mut self.visibility + } + + pub fn visibility(&self) -> &ClientVisibility { + &self.visibility + } + /// Clears all entities for unacknowledged updates, returning them as an iterator. /// /// Keeps the allocated memory for reuse. @@ -138,7 +170,7 @@ impl ClientInfo { /// Keeps the allocated memory for reuse. fn reset(&mut self, id: ClientId) { self.id = id; - self.just_connected = true; + self.visibility.clear(); self.ticks.clear(); self.updates.clear(); self.next_update_index = 0; @@ -229,10 +261,17 @@ impl ClientInfo { /// Removes a despawned entity tracked by this client. pub fn remove_despawned(&mut self, entity: Entity) { self.ticks.remove(&entity); + self.visibility.remove_despawned(entity); // We don't clean up `self.updates` for efficiency reasons. // `Self::acknowledge()` will properly ignore despawned entities. } + pub(super) fn remove_hidden(&mut self) -> impl Iterator + '_ { + self.visibility.iter_hidden().inspect(|entity| { + self.ticks.remove(entity); + }) + } + /// Removes all updates older then `min_timestamp`. /// /// Keeps allocated memory in the buffers for reuse. diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs new file mode 100644 index 00000000..1f6fea1f --- /dev/null +++ b/src/server/clients_info/client_visibility.rs @@ -0,0 +1,157 @@ +use std::iter; + +use bevy::{ + prelude::*, + utils::{EntityHashMap, EntityHashSet}, +}; + +use super::VisibilityPolicy; + +pub enum ClientVisibility { + All { + just_connected: bool, + }, + Blacklist { + list: EntityHashMap, + removed: EntityHashSet, + }, + Whitelist { + list: EntityHashMap, + removed: EntityHashSet, + }, +} + +impl ClientVisibility { + /// Creates a new instance based on the preconfigured policy. + pub(super) fn new(policy: VisibilityPolicy) -> Self { + match policy { + VisibilityPolicy::All => Self::All { + just_connected: true, + }, + VisibilityPolicy::Blacklist => Self::Blacklist { + list: Default::default(), + removed: Default::default(), + }, + VisibilityPolicy::Whitelist => Self::Whitelist { + list: Default::default(), + removed: Default::default(), + }, + } + } + + /// Resets the state to as it was after [`Self::new`]. + pub(super) fn clear(&mut self) { + match self { + ClientVisibility::All { just_connected } => *just_connected = true, + ClientVisibility::Blacklist { list, removed } + | ClientVisibility::Whitelist { list, removed } => { + list.clear(); + removed.clear(); + } + } + } + + /// Marks all entities as not "just added" and clears the removed. + /// + /// Should be called after each tick. + pub(crate) fn update(&mut self) { + match self { + ClientVisibility::All { just_connected } => *just_connected = false, + ClientVisibility::Blacklist { list, removed } + | ClientVisibility::Whitelist { list, removed } => { + for just_added in list.values_mut() { + *just_added = false; + } + removed.clear(); + } + } + } + + pub(super) fn remove_despawned(&mut self, entity: Entity) { + match self { + ClientVisibility::All { .. } => (), + ClientVisibility::Blacklist { list, removed } + | ClientVisibility::Whitelist { list, removed } => { + list.remove(&entity); + removed.remove(&entity); + } + } + } + + pub(super) fn iter_hidden(&self) -> Box + '_> { + match self { + ClientVisibility::All { .. } => Box::new(iter::empty()), + ClientVisibility::Blacklist { list, .. } => Box::new( + list.iter() + .filter_map(|(&entity, just_added)| just_added.then_some(entity)), + ), + ClientVisibility::Whitelist { removed, .. } => Box::new(removed.iter().copied()), + } + } + + /// Sets visibility for specific entity. + /// + /// # Panics + /// + /// Panics if the server plugin was previously configured with [`VisibilityPolicy::All`]. + pub fn set(&mut self, entity: Entity, visibile: bool) { + match self { + ClientVisibility::All { .. } => { + panic!( + "visibility shouldn't be set with {:?}", + VisibilityPolicy::All + ) + } + ClientVisibility::Blacklist { list, removed } => { + if !visibile { + list.insert(entity, true); + removed.remove(&entity); + } else if list.remove(&entity).is_some() { + removed.insert(entity); + } + } + ClientVisibility::Whitelist { list, removed } => { + if visibile { + list.insert(entity, true); + removed.remove(&entity); + } else if list.remove(&entity).is_some() { + removed.insert(entity); + } + } + } + } + + /// Gets visibility for specific entity. + pub fn get(&self, entity: Entity) -> EntityVisibility { + match self { + ClientVisibility::All { just_connected } => { + if *just_connected { + EntityVisibility::JustVisible + } else { + EntityVisibility::Visible + } + } + ClientVisibility::Blacklist { list, removed } => { + if list.contains_key(&entity) { + EntityVisibility::Hidden + } else if removed.contains(&entity) { + EntityVisibility::JustVisible + } else { + EntityVisibility::Visible + } + } + ClientVisibility::Whitelist { list, .. } => match list.get(&entity) { + Some(true) => EntityVisibility::JustVisible, + Some(false) => EntityVisibility::Visible, + None => EntityVisibility::Hidden, + }, + } + } +} + +#[derive(PartialEq)] +pub enum EntityVisibility { + JustVisible, + Visible, + Hidden, +} diff --git a/src/server/replication_messages.rs b/src/server/replication_messages.rs index 2af2dbad..5b91883b 100644 --- a/src/server/replication_messages.rs +++ b/src/server/replication_messages.rs @@ -94,6 +94,7 @@ impl ReplicationMessages { tick, timestamp, )?; + client_info.visibility_mut().update(); } let clients_info = mem::take(&mut self.clients_info); From 5767abeed9e2b13b901c18b707996fdd8f3493d2 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sat, 13 Jan 2024 18:18:02 +0200 Subject: [PATCH 02/48] Fix tests compilation --- tests/replication.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/replication.rs b/tests/replication.rs index bab93f45..80911bd9 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -677,6 +677,7 @@ fn update_replication_cleanup() { ReplicationPlugins.set(ServerPlugin { tick_policy: TickPolicy::EveryFrame, update_timeout: Duration::ZERO, // Will cause dropping updates after each frame. + ..Default::default() }), )) .replicate::(); From c1eea503de02660898fbfb7fc0dc0594ef035be9 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 12:33:45 +0200 Subject: [PATCH 03/48] Do not panic on visibility change with `ClientVisibility::All` --- src/server/clients_info/client_visibility.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 1f6fea1f..9809659c 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -91,16 +91,21 @@ impl ClientVisibility { /// Sets visibility for specific entity. /// - /// # Panics - /// - /// Panics if the server plugin was previously configured with [`VisibilityPolicy::All`]. + /// Does nothing if visibility policy for the server plugin is set to [`VisibilityPolicy::All`]. pub fn set(&mut self, entity: Entity, visibile: bool) { match self { ClientVisibility::All { .. } => { - panic!( - "visibility shouldn't be set with {:?}", - VisibilityPolicy::All - ) + if visibile { + debug!( + "ignoring visibility enable due to {:?}", + VisibilityPolicy::All + ); + } else { + warn!( + "ignoring visibility disable due to {:?}", + VisibilityPolicy::All + ); + } } ClientVisibility::Blacklist { list, removed } => { if !visibile { From 0b655e1c5215897aa65fd6c410a92a3e116f1346 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 12:37:05 +0200 Subject: [PATCH 04/48] Use proposed naming --- src/server.rs | 8 +++---- src/server/clients_info.rs | 4 ++-- src/server/clients_info/client_visibility.rs | 24 ++++++++++---------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/server.rs b/src/server.rs index a40ae67b..0d964dd6 100644 --- a/src/server.rs +++ b/src/server.rs @@ -331,7 +331,7 @@ fn collect_changes( let mut shared_bytes = None; for (init_message, update_message, client_info) in messages.iter_mut_with_info() { let visibility = client_info.visibility().get(entity.entity()); - let new_entity = marker_added || visibility == EntityVisibility::JustVisible; + let new_entity = marker_added || visibility == EntityVisibility::Gained; if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run()) { init_message.write_component( @@ -340,7 +340,7 @@ fn collect_changes( component_info.replication_id, component, )?; - } else if visibility == EntityVisibility::Visible { + } else if visibility == EntityVisibility::Maintained { let tick = client_info .get_change_limit(entity.entity()) .expect("entity should be present after adding component"); @@ -358,7 +358,7 @@ fn collect_changes( for (init_message, update_message, client_info) in messages.iter_mut_with_info() { let visibility = client_info.visibility().get(entity.entity()); - let new_entity = marker_added || visibility == EntityVisibility::JustVisible; + let new_entity = marker_added || visibility == EntityVisibility::Gained; if new_entity || init_message.entity_data_size() != 0 { // If there is any insertion or we must initialize, include all updates into init message // and bump the last acknowledged tick to keep entity updates atomic. @@ -428,7 +428,7 @@ fn collect_despawns( } for (message, _, client_info) in messages.iter_mut_with_info() { - for entity in client_info.remove_hidden() { + for entity in client_info.remove_lost_visibility() { message.write_entity(&mut None, entity)?; } } diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 5092680e..f2f7240e 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -266,8 +266,8 @@ impl ClientInfo { // `Self::acknowledge()` will properly ignore despawned entities. } - pub(super) fn remove_hidden(&mut self) -> impl Iterator + '_ { - self.visibility.iter_hidden().inspect(|entity| { + pub(super) fn remove_lost_visibility(&mut self) -> impl Iterator + '_ { + self.visibility.iter_lost_visibility().inspect(|entity| { self.ticks.remove(entity); }) } diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 9809659c..da12314c 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -78,7 +78,7 @@ impl ClientVisibility { } } - pub(super) fn iter_hidden(&self) -> Box + '_> { + pub(super) fn iter_lost_visibility(&self) -> Box + '_> { match self { ClientVisibility::All { .. } => Box::new(iter::empty()), ClientVisibility::Blacklist { list, .. } => Box::new( @@ -131,24 +131,24 @@ impl ClientVisibility { match self { ClientVisibility::All { just_connected } => { if *just_connected { - EntityVisibility::JustVisible + EntityVisibility::Gained } else { - EntityVisibility::Visible + EntityVisibility::Maintained } } ClientVisibility::Blacklist { list, removed } => { if list.contains_key(&entity) { - EntityVisibility::Hidden + EntityVisibility::None } else if removed.contains(&entity) { - EntityVisibility::JustVisible + EntityVisibility::Gained } else { - EntityVisibility::Visible + EntityVisibility::Maintained } } ClientVisibility::Whitelist { list, .. } => match list.get(&entity) { - Some(true) => EntityVisibility::JustVisible, - Some(false) => EntityVisibility::Visible, - None => EntityVisibility::Hidden, + Some(true) => EntityVisibility::Gained, + Some(false) => EntityVisibility::Maintained, + None => EntityVisibility::None, }, } } @@ -156,7 +156,7 @@ impl ClientVisibility { #[derive(PartialEq)] pub enum EntityVisibility { - JustVisible, - Visible, - Hidden, + Gained, + Maintained, + None, } From 05cc66a5785e2f00a21309b309606f921315db52 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 13:51:30 +0200 Subject: [PATCH 05/48] Use more efficient iteration approach --- src/server/clients_info/client_visibility.rs | 117 ++++++++++++++----- 1 file changed, 88 insertions(+), 29 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index da12314c..700b367e 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -7,16 +7,26 @@ use bevy::{ use super::VisibilityPolicy; +/// Entity visibility settings for a client. pub enum ClientVisibility { All { + /// Indicates that the client has just connected to the server. just_connected: bool, }, Blacklist { + /// All blacklisted entities and an indicator of whether it is in the queue for deletion at this tick. list: EntityHashMap, + /// All entities that were removed from the list on this tick. + added: EntityHashSet, + /// All entities that were added to the list on this tick. removed: EntityHashSet, }, Whitelist { + /// All whitelisted entities and an indicator whether it was added to the list on this tick. list: EntityHashMap, + /// All entities that were added to the list on this tick. + added: EntityHashSet, + /// All entities that were removed from the list on this tick. removed: EntityHashSet, }, } @@ -30,10 +40,12 @@ impl ClientVisibility { }, VisibilityPolicy::Blacklist => Self::Blacklist { list: Default::default(), + added: Default::default(), removed: Default::default(), }, VisibilityPolicy::Whitelist => Self::Whitelist { list: Default::default(), + added: Default::default(), removed: Default::default(), }, } @@ -43,9 +55,18 @@ impl ClientVisibility { pub(super) fn clear(&mut self) { match self { ClientVisibility::All { just_connected } => *just_connected = true, - ClientVisibility::Blacklist { list, removed } - | ClientVisibility::Whitelist { list, removed } => { + ClientVisibility::Blacklist { + list, + added, + removed, + } + | ClientVisibility::Whitelist { + list, + added, + removed, + } => { list.clear(); + added.clear(); removed.clear(); } } @@ -57,34 +78,62 @@ impl ClientVisibility { pub(crate) fn update(&mut self) { match self { ClientVisibility::All { just_connected } => *just_connected = false, - ClientVisibility::Blacklist { list, removed } - | ClientVisibility::Whitelist { list, removed } => { - for just_added in list.values_mut() { - *just_added = false; + ClientVisibility::Blacklist { + list, + added, + removed, + } => { + for entity in removed.drain() { + list.remove(&entity); + } + added.clear(); + } + ClientVisibility::Whitelist { + list, + added, + removed, + } => { + for entity in added.drain() { + list.insert(entity, false); } removed.clear(); } } } + /// Removes a despawned entity tracked by this client. pub(super) fn remove_despawned(&mut self, entity: Entity) { match self { ClientVisibility::All { .. } => (), - ClientVisibility::Blacklist { list, removed } - | ClientVisibility::Whitelist { list, removed } => { - list.remove(&entity); - removed.remove(&entity); + ClientVisibility::Blacklist { + list, + added, + removed, + } => { + if let Some(just_removed) = list.get_mut(&entity) { + *just_removed = true; + removed.remove(&entity); + added.remove(&entity); + } + } + ClientVisibility::Whitelist { + list, + added, + removed, + } => { + if list.remove(&entity).is_some() { + added.remove(&entity); + removed.remove(&entity); + } } } } + /// Returns an iterator of entities that lost visibility at this tick. pub(super) fn iter_lost_visibility(&self) -> Box + '_> { match self { ClientVisibility::All { .. } => Box::new(iter::empty()), - ClientVisibility::Blacklist { list, .. } => Box::new( - list.iter() - .filter_map(|(&entity, just_added)| just_added.then_some(entity)), - ), + ClientVisibility::Blacklist { added, .. } => Box::new(added.iter().copied()), ClientVisibility::Whitelist { removed, .. } => Box::new(removed.iter().copied()), } } @@ -107,20 +156,34 @@ impl ClientVisibility { ); } } - ClientVisibility::Blacklist { list, removed } => { + ClientVisibility::Blacklist { + list, + added, + removed, + } => { if !visibile { - list.insert(entity, true); - removed.remove(&entity); + if list.insert(entity, false).is_none() { + removed.remove(&entity); + added.insert(entity); + } } else if list.remove(&entity).is_some() { removed.insert(entity); + added.remove(&entity); } } - ClientVisibility::Whitelist { list, removed } => { + ClientVisibility::Whitelist { + list, + added, + removed, + } => { if visibile { - list.insert(entity, true); - removed.remove(&entity); + if list.insert(entity, true).is_none() { + removed.remove(&entity); + added.insert(entity); + } } else if list.remove(&entity).is_some() { removed.insert(entity); + added.remove(&entity); } } } @@ -136,15 +199,11 @@ impl ClientVisibility { EntityVisibility::Maintained } } - ClientVisibility::Blacklist { list, removed } => { - if list.contains_key(&entity) { - EntityVisibility::None - } else if removed.contains(&entity) { - EntityVisibility::Gained - } else { - EntityVisibility::Maintained - } - } + ClientVisibility::Blacklist { list, .. } => match list.get(&entity) { + Some(true) => EntityVisibility::Gained, + Some(false) => EntityVisibility::None, + None => EntityVisibility::Maintained, + }, ClientVisibility::Whitelist { list, .. } => match list.get(&entity) { Some(true) => EntityVisibility::Gained, Some(false) => EntityVisibility::Maintained, From f3b5692f4443ea31a4cbb252b6e437ce6f96fef6 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 13:54:55 +0200 Subject: [PATCH 06/48] Fix docs and add to prelude --- src/lib.rs | 5 +++-- src/server.rs | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index bd8eb885..10b0a82a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -432,8 +432,9 @@ pub mod prelude { NetworkChannels, ReplicationChannel, RepliconCorePlugin, }, server::{ - clients_info::ClientsInfo, has_authority, ClientEntityMap, ClientMapping, ServerPlugin, - ServerSet, TickPolicy, SERVER_ID, + clients_info::{client_visibility::ClientVisibility, ClientInfo, ClientsInfo}, + has_authority, ClientEntityMap, ClientMapping, ServerPlugin, ServerSet, TickPolicy, + SERVER_ID, }, ReplicationPlugins, }; diff --git a/src/server.rs b/src/server.rs index 0d964dd6..bdf5d695 100644 --- a/src/server.rs +++ b/src/server.rs @@ -507,7 +507,7 @@ pub enum TickPolicy { Manual, } -/// Controls how visibility will be controlled via [`EntitiesVisibility`](entities_visibility::EntitiesVisibility). +/// Controls how visibility will be controlled via [`ClientVisibility`](clients_info::client_visibility::ClientVisibility). #[derive(Default, Debug, Clone, Copy)] pub enum VisibilityPolicy { /// All entities are visible by default and visibility can't be changed. From 56c7880d0d429cb21e79e820a76cff788d968748 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 14:25:09 +0200 Subject: [PATCH 07/48] Remove `Box` from iterator --- src/server/clients_info/client_visibility.rs | 39 +++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 700b367e..b2e66cfd 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -1,5 +1,3 @@ -use std::iter; - use bevy::{ prelude::*, utils::{EntityHashMap, EntityHashSet}, @@ -130,11 +128,15 @@ impl ClientVisibility { } /// Returns an iterator of entities that lost visibility at this tick. - pub(super) fn iter_lost_visibility(&self) -> Box + '_> { + pub(super) fn iter_lost_visibility(&self) -> impl Iterator + '_ { match self { - ClientVisibility::All { .. } => Box::new(iter::empty()), - ClientVisibility::Blacklist { added, .. } => Box::new(added.iter().copied()), - ClientVisibility::Whitelist { removed, .. } => Box::new(removed.iter().copied()), + ClientVisibility::All { .. } => VisibilityLostIter::AllVisible, + ClientVisibility::Blacklist { added, .. } => { + VisibilityLostIter::Lost(added.iter().copied()) + } + ClientVisibility::Whitelist { removed, .. } => { + VisibilityLostIter::Lost(removed.iter().copied()) + } } } @@ -219,3 +221,28 @@ pub enum EntityVisibility { Maintained, None, } + +enum VisibilityLostIter { + AllVisible, + Lost(T), +} + +impl Iterator for VisibilityLostIter { + type Item = T::Item; + + fn next(&mut self) -> Option { + match self { + VisibilityLostIter::AllVisible => None, + VisibilityLostIter::Lost(entities) => entities.next(), + } + } + + fn size_hint(&self) -> (usize, Option) { + match self { + VisibilityLostIter::AllVisible => (0, Some(0)), + VisibilityLostIter::Lost(entities) => entities.size_hint(), + } + } +} + +impl ExactSizeIterator for VisibilityLostIter {} From 02a820ee57636d80882b7ef6299a3dbabf40748e Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 14:45:44 +0200 Subject: [PATCH 08/48] Export simpler visibility getter to users --- src/server.rs | 12 ++++---- src/server/clients_info/client_visibility.rs | 31 +++++++++++++------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/server.rs b/src/server.rs index bdf5d695..8bef84d6 100644 --- a/src/server.rs +++ b/src/server.rs @@ -27,7 +27,7 @@ use bevy_renet::{ use crate::replicon_core::{ replication_rules::ReplicationRules, replicon_tick::RepliconTick, ReplicationChannel, }; -use clients_info::{client_visibility::EntityVisibility, ClientBuffers, ClientInfo, ClientsInfo}; +use clients_info::{client_visibility::VisibilityInfo, ClientBuffers, ClientInfo, ClientsInfo}; use despawn_buffer::{DespawnBuffer, DespawnBufferPlugin}; use removal_buffer::{RemovalBuffer, RemovalBufferPlugin}; use replicated_archetypes_info::ReplicatedArchetypesInfo; @@ -330,8 +330,8 @@ fn collect_changes( let mut shared_bytes = None; for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let visibility = client_info.visibility().get(entity.entity()); - let new_entity = marker_added || visibility == EntityVisibility::Gained; + let visibility = client_info.visibility().get_info(entity.entity()); + let new_entity = marker_added || visibility == VisibilityInfo::Gained; if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run()) { init_message.write_component( @@ -340,7 +340,7 @@ fn collect_changes( component_info.replication_id, component, )?; - } else if visibility == EntityVisibility::Maintained { + } else if visibility == VisibilityInfo::Maintained { let tick = client_info .get_change_limit(entity.entity()) .expect("entity should be present after adding component"); @@ -357,8 +357,8 @@ fn collect_changes( } for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let visibility = client_info.visibility().get(entity.entity()); - let new_entity = marker_added || visibility == EntityVisibility::Gained; + let visibility = client_info.visibility().get_info(entity.entity()); + let new_entity = marker_added || visibility == VisibilityInfo::Gained; if new_entity || init_message.entity_data_size() != 0 { // If there is any insertion or we must initialize, include all updates into init message // and bump the last acknowledged tick to keep entity updates atomic. diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index b2e66cfd..9efda5fa 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -143,7 +143,7 @@ impl ClientVisibility { /// Sets visibility for specific entity. /// /// Does nothing if visibility policy for the server plugin is set to [`VisibilityPolicy::All`]. - pub fn set(&mut self, entity: Entity, visibile: bool) { + pub fn set_visible(&mut self, entity: Entity, visibile: bool) { match self { ClientVisibility::All { .. } => { if visibile { @@ -192,31 +192,40 @@ impl ClientVisibility { } /// Gets visibility for specific entity. - pub fn get(&self, entity: Entity) -> EntityVisibility { + pub fn is_visible(&self, entity: Entity) -> bool { + match self { + ClientVisibility::All { .. } => true, + ClientVisibility::Blacklist { list, .. } => !list.contains_key(&entity), + ClientVisibility::Whitelist { list, .. } => list.contains_key(&entity), + } + } + + /// Gets visibility with change information included for specific entity. + pub(crate) fn get_info(&self, entity: Entity) -> VisibilityInfo { match self { ClientVisibility::All { just_connected } => { if *just_connected { - EntityVisibility::Gained + VisibilityInfo::Gained } else { - EntityVisibility::Maintained + VisibilityInfo::Maintained } } ClientVisibility::Blacklist { list, .. } => match list.get(&entity) { - Some(true) => EntityVisibility::Gained, - Some(false) => EntityVisibility::None, - None => EntityVisibility::Maintained, + Some(true) => VisibilityInfo::Gained, + Some(false) => VisibilityInfo::None, + None => VisibilityInfo::Maintained, }, ClientVisibility::Whitelist { list, .. } => match list.get(&entity) { - Some(true) => EntityVisibility::Gained, - Some(false) => EntityVisibility::Maintained, - None => EntityVisibility::None, + Some(true) => VisibilityInfo::Gained, + Some(false) => VisibilityInfo::Maintained, + None => VisibilityInfo::None, }, } } } #[derive(PartialEq)] -pub enum EntityVisibility { +pub(crate) enum VisibilityInfo { Gained, Maintained, None, From 222f63e07aafe6504020cbd7a88c49f7621c33cf Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 14:49:18 +0200 Subject: [PATCH 09/48] Combine loops --- src/server.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/server.rs b/src/server.rs index 8bef84d6..7a768172 100644 --- a/src/server.rs +++ b/src/server.rs @@ -431,9 +431,7 @@ fn collect_despawns( for entity in client_info.remove_lost_visibility() { message.write_entity(&mut None, entity)?; } - } - for (message, _) in messages.iter_mut() { message.end_array()?; } From 53cb84c448b24ca5f0a0110328af7f3a734a4e50 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 16:22:05 +0200 Subject: [PATCH 10/48] Turn `ClientVisibility` into a struct with the inner enum Necessary to make fields private. --- src/server/clients_info/client_visibility.rs | 115 ++++++++++--------- 1 file changed, 59 insertions(+), 56 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 9efda5fa..18168e92 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -6,59 +6,38 @@ use bevy::{ use super::VisibilityPolicy; /// Entity visibility settings for a client. -pub enum ClientVisibility { - All { - /// Indicates that the client has just connected to the server. - just_connected: bool, - }, - Blacklist { - /// All blacklisted entities and an indicator of whether it is in the queue for deletion at this tick. - list: EntityHashMap, - /// All entities that were removed from the list on this tick. - added: EntityHashSet, - /// All entities that were added to the list on this tick. - removed: EntityHashSet, - }, - Whitelist { - /// All whitelisted entities and an indicator whether it was added to the list on this tick. - list: EntityHashMap, - /// All entities that were added to the list on this tick. - added: EntityHashSet, - /// All entities that were removed from the list on this tick. - removed: EntityHashSet, - }, -} +pub struct ClientVisibility(VisibilityFilter); impl ClientVisibility { /// Creates a new instance based on the preconfigured policy. pub(super) fn new(policy: VisibilityPolicy) -> Self { match policy { - VisibilityPolicy::All => Self::All { + VisibilityPolicy::All => Self(VisibilityFilter::All { just_connected: true, - }, - VisibilityPolicy::Blacklist => Self::Blacklist { + }), + VisibilityPolicy::Blacklist => Self(VisibilityFilter::Blacklist { list: Default::default(), added: Default::default(), removed: Default::default(), - }, - VisibilityPolicy::Whitelist => Self::Whitelist { + }), + VisibilityPolicy::Whitelist => Self(VisibilityFilter::Whitelist { list: Default::default(), added: Default::default(), removed: Default::default(), - }, + }), } } /// Resets the state to as it was after [`Self::new`]. pub(super) fn clear(&mut self) { - match self { - ClientVisibility::All { just_connected } => *just_connected = true, - ClientVisibility::Blacklist { + match &mut self.0 { + VisibilityFilter::All { just_connected } => *just_connected = true, + VisibilityFilter::Blacklist { list, added, removed, } - | ClientVisibility::Whitelist { + | VisibilityFilter::Whitelist { list, added, removed, @@ -74,9 +53,9 @@ impl ClientVisibility { /// /// Should be called after each tick. pub(crate) fn update(&mut self) { - match self { - ClientVisibility::All { just_connected } => *just_connected = false, - ClientVisibility::Blacklist { + match &mut self.0 { + VisibilityFilter::All { just_connected } => *just_connected = false, + VisibilityFilter::Blacklist { list, added, removed, @@ -86,7 +65,7 @@ impl ClientVisibility { } added.clear(); } - ClientVisibility::Whitelist { + VisibilityFilter::Whitelist { list, added, removed, @@ -101,9 +80,9 @@ impl ClientVisibility { /// Removes a despawned entity tracked by this client. pub(super) fn remove_despawned(&mut self, entity: Entity) { - match self { - ClientVisibility::All { .. } => (), - ClientVisibility::Blacklist { + match &mut self.0 { + VisibilityFilter::All { .. } => (), + VisibilityFilter::Blacklist { list, added, removed, @@ -114,7 +93,7 @@ impl ClientVisibility { added.remove(&entity); } } - ClientVisibility::Whitelist { + VisibilityFilter::Whitelist { list, added, removed, @@ -129,12 +108,12 @@ impl ClientVisibility { /// Returns an iterator of entities that lost visibility at this tick. pub(super) fn iter_lost_visibility(&self) -> impl Iterator + '_ { - match self { - ClientVisibility::All { .. } => VisibilityLostIter::AllVisible, - ClientVisibility::Blacklist { added, .. } => { + match &self.0 { + VisibilityFilter::All { .. } => VisibilityLostIter::AllVisible, + VisibilityFilter::Blacklist { added, .. } => { VisibilityLostIter::Lost(added.iter().copied()) } - ClientVisibility::Whitelist { removed, .. } => { + VisibilityFilter::Whitelist { removed, .. } => { VisibilityLostIter::Lost(removed.iter().copied()) } } @@ -144,8 +123,8 @@ impl ClientVisibility { /// /// Does nothing if visibility policy for the server plugin is set to [`VisibilityPolicy::All`]. pub fn set_visible(&mut self, entity: Entity, visibile: bool) { - match self { - ClientVisibility::All { .. } => { + match &mut self.0 { + VisibilityFilter::All { .. } => { if visibile { debug!( "ignoring visibility enable due to {:?}", @@ -158,7 +137,7 @@ impl ClientVisibility { ); } } - ClientVisibility::Blacklist { + VisibilityFilter::Blacklist { list, added, removed, @@ -173,7 +152,7 @@ impl ClientVisibility { added.remove(&entity); } } - ClientVisibility::Whitelist { + VisibilityFilter::Whitelist { list, added, removed, @@ -193,29 +172,29 @@ impl ClientVisibility { /// Gets visibility for specific entity. pub fn is_visible(&self, entity: Entity) -> bool { - match self { - ClientVisibility::All { .. } => true, - ClientVisibility::Blacklist { list, .. } => !list.contains_key(&entity), - ClientVisibility::Whitelist { list, .. } => list.contains_key(&entity), + match &self.0 { + VisibilityFilter::All { .. } => true, + VisibilityFilter::Blacklist { list, .. } => !list.contains_key(&entity), + VisibilityFilter::Whitelist { list, .. } => list.contains_key(&entity), } } /// Gets visibility with change information included for specific entity. pub(crate) fn get_info(&self, entity: Entity) -> VisibilityInfo { - match self { - ClientVisibility::All { just_connected } => { + match &self.0 { + VisibilityFilter::All { just_connected } => { if *just_connected { VisibilityInfo::Gained } else { VisibilityInfo::Maintained } } - ClientVisibility::Blacklist { list, .. } => match list.get(&entity) { + VisibilityFilter::Blacklist { list, .. } => match list.get(&entity) { Some(true) => VisibilityInfo::Gained, Some(false) => VisibilityInfo::None, None => VisibilityInfo::Maintained, }, - ClientVisibility::Whitelist { list, .. } => match list.get(&entity) { + VisibilityFilter::Whitelist { list, .. } => match list.get(&entity) { Some(true) => VisibilityInfo::Gained, Some(false) => VisibilityInfo::Maintained, None => VisibilityInfo::None, @@ -224,6 +203,30 @@ impl ClientVisibility { } } +/// Since enums can't have private fields, it's wrapped into [`ClientVisibility`] to make fields private, while keeping the struct public. +enum VisibilityFilter { + All { + /// Indicates that the client has just connected to the server. + just_connected: bool, + }, + Blacklist { + /// All blacklisted entities and an indicator of whether it is in the queue for deletion at this tick. + list: EntityHashMap, + /// All entities that were removed from the list on this tick. + added: EntityHashSet, + /// All entities that were added to the list on this tick. + removed: EntityHashSet, + }, + Whitelist { + /// All whitelisted entities and an indicator whether it was added to the list on this tick. + list: EntityHashMap, + /// All entities that were added to the list on this tick. + added: EntityHashSet, + /// All entities that were removed from the list on this tick. + removed: EntityHashSet, + }, +} + #[derive(PartialEq)] pub(crate) enum VisibilityInfo { Gained, From ebd93410f9deddf65ddcc59203ce47f69dea66b7 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 17:52:49 +0200 Subject: [PATCH 11/48] Refactor API to avoid extra lookup --- src/server.rs | 17 +++-- src/server/clients_info/client_visibility.rs | 77 +++++++++++++------- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/src/server.rs b/src/server.rs index 7a768172..8ea94e27 100644 --- a/src/server.rs +++ b/src/server.rs @@ -27,7 +27,7 @@ use bevy_renet::{ use crate::replicon_core::{ replication_rules::ReplicationRules, replicon_tick::RepliconTick, ReplicationChannel, }; -use clients_info::{client_visibility::VisibilityInfo, ClientBuffers, ClientInfo, ClientsInfo}; +use clients_info::{client_visibility::EntityState, ClientBuffers, ClientInfo, ClientsInfo}; use despawn_buffer::{DespawnBuffer, DespawnBufferPlugin}; use removal_buffer::{RemovalBuffer, RemovalBufferPlugin}; use replicated_archetypes_info::ReplicatedArchetypesInfo; @@ -295,9 +295,12 @@ fn collect_changes( }; for entity in archetype.entities() { - for (init_message, update_message) in messages.iter_mut() { + for (init_message, update_message, client_info) in messages.iter_mut_with_info() { init_message.start_entity_data(entity.entity()); update_message.start_entity_data(entity.entity()); + client_info + .visibility_mut() + .read_entity_state(entity.entity()); } // SAFETY: all replicated archetypes have marker component with table storage. @@ -330,8 +333,8 @@ fn collect_changes( let mut shared_bytes = None; for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let visibility = client_info.visibility().get_info(entity.entity()); - let new_entity = marker_added || visibility == VisibilityInfo::Gained; + let entity_state = client_info.visibility().entity_state(); + let new_entity = marker_added || entity_state == EntityState::JustVisible; if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run()) { init_message.write_component( @@ -340,7 +343,7 @@ fn collect_changes( component_info.replication_id, component, )?; - } else if visibility == VisibilityInfo::Maintained { + } else if entity_state == EntityState::Visible { let tick = client_info .get_change_limit(entity.entity()) .expect("entity should be present after adding component"); @@ -357,8 +360,8 @@ fn collect_changes( } for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let visibility = client_info.visibility().get_info(entity.entity()); - let new_entity = marker_added || visibility == VisibilityInfo::Gained; + let entity_state = client_info.visibility().entity_state(); + let new_entity = marker_added || entity_state == EntityState::JustVisible; if new_entity || init_message.entity_data_size() != 0 { // If there is any insertion or we must initialize, include all updates into init message // and bump the last acknowledged tick to keep entity updates atomic. diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 18168e92..7336f8f9 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -6,21 +6,24 @@ use bevy::{ use super::VisibilityPolicy; /// Entity visibility settings for a client. -pub struct ClientVisibility(VisibilityFilter); +pub struct ClientVisibility { + filter: VisibilityFilter, + entity_state: EntityState, +} impl ClientVisibility { /// Creates a new instance based on the preconfigured policy. pub(super) fn new(policy: VisibilityPolicy) -> Self { match policy { - VisibilityPolicy::All => Self(VisibilityFilter::All { + VisibilityPolicy::All => Self::with_filter(VisibilityFilter::All { just_connected: true, }), - VisibilityPolicy::Blacklist => Self(VisibilityFilter::Blacklist { + VisibilityPolicy::Blacklist => Self::with_filter(VisibilityFilter::Blacklist { list: Default::default(), added: Default::default(), removed: Default::default(), }), - VisibilityPolicy::Whitelist => Self(VisibilityFilter::Whitelist { + VisibilityPolicy::Whitelist => Self::with_filter(VisibilityFilter::Whitelist { list: Default::default(), added: Default::default(), removed: Default::default(), @@ -28,9 +31,19 @@ impl ClientVisibility { } } - /// Resets the state to as it was after [`Self::new`]. + /// Creates a new instance with specific filter. + fn with_filter(filter: VisibilityFilter) -> Self { + Self { + filter, + entity_state: Default::default(), + } + } + + /// Resets the filter state to as it was after [`Self::new`]. + /// + /// `entity_state` remains untouched. pub(super) fn clear(&mut self) { - match &mut self.0 { + match &mut self.filter { VisibilityFilter::All { just_connected } => *just_connected = true, VisibilityFilter::Blacklist { list, @@ -53,7 +66,7 @@ impl ClientVisibility { /// /// Should be called after each tick. pub(crate) fn update(&mut self) { - match &mut self.0 { + match &mut self.filter { VisibilityFilter::All { just_connected } => *just_connected = false, VisibilityFilter::Blacklist { list, @@ -80,7 +93,7 @@ impl ClientVisibility { /// Removes a despawned entity tracked by this client. pub(super) fn remove_despawned(&mut self, entity: Entity) { - match &mut self.0 { + match &mut self.filter { VisibilityFilter::All { .. } => (), VisibilityFilter::Blacklist { list, @@ -108,7 +121,7 @@ impl ClientVisibility { /// Returns an iterator of entities that lost visibility at this tick. pub(super) fn iter_lost_visibility(&self) -> impl Iterator + '_ { - match &self.0 { + match &self.filter { VisibilityFilter::All { .. } => VisibilityLostIter::AllVisible, VisibilityFilter::Blacklist { added, .. } => { VisibilityLostIter::Lost(added.iter().copied()) @@ -123,7 +136,7 @@ impl ClientVisibility { /// /// Does nothing if visibility policy for the server plugin is set to [`VisibilityPolicy::All`]. pub fn set_visible(&mut self, entity: Entity, visibile: bool) { - match &mut self.0 { + match &mut self.filter { VisibilityFilter::All { .. } => { if visibile { debug!( @@ -172,38 +185,45 @@ impl ClientVisibility { /// Gets visibility for specific entity. pub fn is_visible(&self, entity: Entity) -> bool { - match &self.0 { + match &self.filter { VisibilityFilter::All { .. } => true, VisibilityFilter::Blacklist { list, .. } => !list.contains_key(&entity), VisibilityFilter::Whitelist { list, .. } => list.contains_key(&entity), } } - /// Gets visibility with change information included for specific entity. - pub(crate) fn get_info(&self, entity: Entity) -> VisibilityInfo { - match &self.0 { + /// Reads entity visibility state for specific entity. + /// + /// Can be obtained later from [`Self::entity_state`]. + pub(crate) fn read_entity_state(&mut self, entity: Entity) { + match &mut self.filter { VisibilityFilter::All { just_connected } => { if *just_connected { - VisibilityInfo::Gained + self.entity_state = EntityState::JustVisible } else { - VisibilityInfo::Maintained + self.entity_state = EntityState::Visible } } VisibilityFilter::Blacklist { list, .. } => match list.get(&entity) { - Some(true) => VisibilityInfo::Gained, - Some(false) => VisibilityInfo::None, - None => VisibilityInfo::Maintained, + Some(true) => self.entity_state = EntityState::JustVisible, + Some(false) => self.entity_state = EntityState::Hidden, + None => self.entity_state = EntityState::Visible, }, VisibilityFilter::Whitelist { list, .. } => match list.get(&entity) { - Some(true) => VisibilityInfo::Gained, - Some(false) => VisibilityInfo::Maintained, - None => VisibilityInfo::None, + Some(true) => self.entity_state = EntityState::JustVisible, + Some(false) => self.entity_state = EntityState::Visible, + None => self.entity_state = EntityState::Hidden, }, } } + + /// Returns state obtained from last call of [`Self::read_entity_state`]. + pub(crate) fn entity_state(&self) -> EntityState { + self.entity_state + } } -/// Since enums can't have private fields, it's wrapped into [`ClientVisibility`] to make fields private, while keeping the struct public. +/// Filter for [`ClientVisibility`] based on [`VisibilityPolicy`]. enum VisibilityFilter { All { /// Indicates that the client has just connected to the server. @@ -227,11 +247,14 @@ enum VisibilityFilter { }, } -#[derive(PartialEq)] -pub(crate) enum VisibilityInfo { - Gained, - Maintained, +/// Visibility state for an entity. +#[derive(PartialEq, Default, Clone, Copy)] +pub(crate) enum EntityState { + #[default] None, + Hidden, + Visible, + JustVisible, } enum VisibilityLostIter { From a24fb9c306246a10e43f841abb84f0e8a0c0f993 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 18:08:48 +0200 Subject: [PATCH 12/48] Update docs --- src/server/clients_info.rs | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index f2f7240e..074c16dc 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -41,10 +41,16 @@ impl ClientsInfo { } } + /// Returns reference to connected client info. + /// + /// This operation is *O*(*n*). pub fn get(&self, client_id: ClientId) -> Option<&ClientInfo> { self.info.iter().find(|info| info.id == client_id) } + /// Returns mutable reference to connected client info. + /// + /// This operation is *O*(*n*). pub fn get_mut(&mut self, client_id: ClientId) -> Option<&mut ClientInfo> { self.info.iter_mut().find(|info| info.id == client_id) } @@ -64,6 +70,7 @@ impl ClientsInfo { self.info.len() } + /// Returns `true` if no clients connected. pub fn is_empty(&self) -> bool { self.info.is_empty() } @@ -148,14 +155,16 @@ impl ClientInfo { self.id } - pub fn visibility_mut(&mut self) -> &mut ClientVisibility { - &mut self.visibility - } - + /// Returns reference to client visibility settings. pub fn visibility(&self) -> &ClientVisibility { &self.visibility } + /// Returns mutable reference to client visibility settings. + pub fn visibility_mut(&mut self) -> &mut ClientVisibility { + &mut self.visibility + } + /// Clears all entities for unacknowledged updates, returning them as an iterator. /// /// Keeps the allocated memory for reuse. @@ -266,6 +275,9 @@ impl ClientInfo { // `Self::acknowledge()` will properly ignore despawned entities. } + /// Iterates over all entities for which visibility was lost during this tick and removes them. + /// + /// Removal happens lazily during the iteration. pub(super) fn remove_lost_visibility(&mut self) -> impl Iterator + '_ { self.visibility.iter_lost_visibility().inspect(|entity| { self.ticks.remove(entity); From 2e9e791b29cc253fc8592697162eb523024c2bb9 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 19:16:27 +0200 Subject: [PATCH 13/48] Fix visibility check logic --- src/server.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/server.rs b/src/server.rs index 8ea94e27..de08c3ea 100644 --- a/src/server.rs +++ b/src/server.rs @@ -334,6 +334,10 @@ fn collect_changes( let mut shared_bytes = None; for (init_message, update_message, client_info) in messages.iter_mut_with_info() { let entity_state = client_info.visibility().entity_state(); + if entity_state == EntityState::Hidden { + continue; + } + let new_entity = marker_added || entity_state == EntityState::JustVisible; if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run()) { @@ -343,7 +347,7 @@ fn collect_changes( component_info.replication_id, component, )?; - } else if entity_state == EntityState::Visible { + } else { let tick = client_info .get_change_limit(entity.entity()) .expect("entity should be present after adding component"); @@ -361,6 +365,10 @@ fn collect_changes( for (init_message, update_message, client_info) in messages.iter_mut_with_info() { let entity_state = client_info.visibility().entity_state(); + if entity_state == EntityState::Hidden { + continue; + } + let new_entity = marker_added || entity_state == EntityState::JustVisible; if new_entity || init_message.entity_data_size() != 0 { // If there is any insertion or we must initialize, include all updates into init message From e34fa97177821e98c2e101535f31a03dbdcd2aba Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 21:37:09 +0200 Subject: [PATCH 14/48] Fix unhiding logic for blacklist --- src/server/clients_info/client_visibility.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 7336f8f9..e425398a 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -160,7 +160,8 @@ impl ClientVisibility { removed.remove(&entity); added.insert(entity); } - } else if list.remove(&entity).is_some() { + } else if let Some(just_removed) = list.get_mut(&entity) { + *just_removed = true; removed.insert(entity); added.remove(&entity); } From ecc9f1e668716b6454b1b6eac532f66f065c1dcb Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 21:48:35 +0200 Subject: [PATCH 15/48] Put `VisibilityPolicy` to prelude --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 10b0a82a..09655dde 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -434,7 +434,7 @@ pub mod prelude { server::{ clients_info::{client_visibility::ClientVisibility, ClientInfo, ClientsInfo}, has_authority, ClientEntityMap, ClientMapping, ServerPlugin, ServerSet, TickPolicy, - SERVER_ID, + VisibilityPolicy, SERVER_ID, }, ReplicationPlugins, }; From f10d0a1e37b680ed3b2638819aa8210d47ad59a3 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 21:49:03 +0200 Subject: [PATCH 16/48] Add tests --- tests/replication.rs | 172 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 172 insertions(+) diff --git a/tests/replication.rs b/tests/replication.rs index 80911bd9..2b61050b 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -752,6 +752,178 @@ fn update_replication_cleanup() { ); } +#[test] +fn all_replication() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + ..Default::default() + }), + )) + .replicate::(); + } + + connect::single_client(&mut server_app, &mut client_app); + + let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + + let client_transport = client_app.world.resource::(); + let client_id = ClientId::from_raw(client_transport.client_id()); + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + visibility.set_visible(server_entity, true); + assert!(visibility.is_visible(server_entity)); + + server_app.update(); + client_app.update(); + + client_app + .world + .query_filtered::<(), (With, With)>() + .single(&client_app.world); + + // Reverse visibility. + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + visibility.set_visible(server_entity, true); + assert!( + visibility.is_visible(server_entity), + "shouldn't have any effect with {:?}", + VisibilityPolicy::All + ); + + server_app.update(); + client_app.update(); + + client_app + .world + .query_filtered::<(), (With, With)>() + .single(&client_app.world); +} + +#[test] +fn blacklist_replication() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + visibility_policy: VisibilityPolicy::Blacklist, + ..Default::default() + }), + )) + .replicate::(); + } + + connect::single_client(&mut server_app, &mut client_app); + + let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + let hidden_entity = server_app.world.spawn((Replication, TableComponent)).id(); + + let client_transport = client_app.world.resource::(); + let client_id = ClientId::from_raw(client_transport.client_id()); + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + visibility.set_visible(hidden_entity, false); + assert!(visibility.is_visible(server_entity)); + assert!(!visibility.is_visible(hidden_entity)); + + server_app.update(); + client_app.update(); + + let client_entity = client_app + .world + .query_filtered::, With)>() + .single(&client_app.world); + + let entity_map = client_app.world.resource::(); + assert_eq!( + entity_map.to_client().get(&server_entity), + Some(&client_entity), + ); + + // Reverse visibility. + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + visibility.set_visible(hidden_entity, true); + + server_app.update(); + client_app.update(); + + let entities_count = client_app + .world + .query_filtered::, With)>() + .iter(&client_app.world) + .count(); + assert_eq!( + entities_count, 2, + "hidden entity should be spawned on client after removing from blacklist" + ); +} + +#[test] +fn whitelist_replication() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + visibility_policy: VisibilityPolicy::Whitelist, + ..Default::default() + }), + )) + .replicate::(); + } + + connect::single_client(&mut server_app, &mut client_app); + + let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + let hidden_entity = server_app.world.spawn((Replication, TableComponent)).id(); + + let client_transport = client_app.world.resource::(); + let client_id = ClientId::from_raw(client_transport.client_id()); + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + visibility.set_visible(server_entity, true); + assert!(visibility.is_visible(server_entity)); + assert!(!visibility.is_visible(hidden_entity)); + + server_app.update(); + client_app.update(); + + let client_entity = client_app + .world + .query_filtered::, With)>() + .single(&client_app.world); + + let entity_map = client_app.world.resource::(); + assert_eq!( + entity_map.to_client().get(&server_entity), + Some(&client_entity), + ); + + // Reverse visibility. + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + visibility.set_visible(server_entity, false); + + server_app.update(); + client_app.update(); + + assert!( + client_app.world.entities().is_empty(), + "server entity should be despawned after removing from whitelist" + ); +} + #[test] fn replication_into_scene() { let mut app = App::new(); From f9e91b78c382a4788af4f9965eaa000efda353d5 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 21:58:15 +0200 Subject: [PATCH 17/48] Improve docs --- src/server.rs | 2 +- src/server/clients_info.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/server.rs b/src/server.rs index de08c3ea..6f475e53 100644 --- a/src/server.rs +++ b/src/server.rs @@ -516,7 +516,7 @@ pub enum TickPolicy { Manual, } -/// Controls how visibility will be controlled via [`ClientVisibility`](clients_info::client_visibility::ClientVisibility). +/// Controls how visibility will be managed via [`ClientVisibility`](clients_info::client_visibility::ClientVisibility). #[derive(Default, Debug, Clone, Copy)] pub enum VisibilityPolicy { /// All entities are visible by default and visibility can't be changed. diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 074c16dc..c6946cf5 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -121,6 +121,7 @@ pub struct ClientInfo { /// Lowest tick for use in change detection for each entity. ticks: EntityHashMap, + /// Entity visibility settings. visibility: ClientVisibility, /// The last tick in which a replicated entity was spawned, despawned, or gained/lost a component from the perspective From a936335232ed06c57c2e1e3c445058b0e23710a0 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Sun, 14 Jan 2024 23:20:54 +0200 Subject: [PATCH 18/48] Fix typo in the test --- tests/replication.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/replication.rs b/tests/replication.rs index 2b61050b..57da3dd6 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -789,7 +789,7 @@ fn all_replication() { // Reverse visibility. let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(server_entity, true); + visibility.set_visible(server_entity, false); assert!( visibility.is_visible(server_entity), "shouldn't have any effect with {:?}", From c8996ab6db61b88189564b28325390d571fb8593 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 00:58:39 +0200 Subject: [PATCH 19/48] Refactor tests Split into simpler tests. --- tests/replication.rs | 129 ++++++++++++++++++++++++++----------------- 1 file changed, 78 insertions(+), 51 deletions(-) diff --git a/tests/replication.rs b/tests/replication.rs index 57da3dd6..92a41593 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -753,7 +753,7 @@ fn update_replication_cleanup() { } #[test] -fn all_replication() { +fn all_visibility() { let mut server_app = App::new(); let mut client_app = App::new(); for app in [&mut server_app, &mut client_app] { @@ -775,20 +775,6 @@ fn all_replication() { let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(server_entity, true); - assert!(visibility.is_visible(server_entity)); - - server_app.update(); - client_app.update(); - - client_app - .world - .query_filtered::<(), (With, With)>() - .single(&client_app.world); - - // Reverse visibility. - let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); visibility.set_visible(server_entity, false); assert!( visibility.is_visible(server_entity), @@ -796,6 +782,36 @@ fn all_replication() { VisibilityPolicy::All ); + visibility.set_visible(server_entity, true); + assert!(visibility.is_visible(server_entity)); +} + +#[test] +fn blacklist_visibility() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + visibility_policy: VisibilityPolicy::Blacklist, + ..Default::default() + }), + )) + .replicate::(); + } + + connect::single_client(&mut server_app, &mut client_app); + + let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + + let client_transport = client_app.world.resource::(); + let client_id = ClientId::from_raw(client_transport.client_id()); + let clients_info = server_app.world.resource::(); + let visibility = clients_info.get(client_id).unwrap().visibility(); + assert!(visibility.is_visible(server_entity)); + server_app.update(); client_app.update(); @@ -806,7 +822,7 @@ fn all_replication() { } #[test] -fn blacklist_replication() { +fn blacklist_visibility_add_remove() { let mut server_app = App::new(); let mut client_app = App::new(); for app in [&mut server_app, &mut client_app] { @@ -824,51 +840,70 @@ fn blacklist_replication() { connect::single_client(&mut server_app, &mut client_app); let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); - let hidden_entity = server_app.world.spawn((Replication, TableComponent)).id(); let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(hidden_entity, false); - assert!(visibility.is_visible(server_entity)); - assert!(!visibility.is_visible(hidden_entity)); + visibility.set_visible(server_entity, false); + assert!(!visibility.is_visible(server_entity)); server_app.update(); client_app.update(); - let client_entity = client_app - .world - .query_filtered::, With)>() - .single(&client_app.world); - - let entity_map = client_app.world.resource::(); - assert_eq!( - entity_map.to_client().get(&server_entity), - Some(&client_entity), - ); + assert!(client_app.world.entities().is_empty()); - // Reverse visibility. + // Reverse visibility back. let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(hidden_entity, true); + visibility.set_visible(server_entity, true); server_app.update(); client_app.update(); - let entities_count = client_app + client_app .world - .query_filtered::, With)>() - .iter(&client_app.world) - .count(); - assert_eq!( - entities_count, 2, - "hidden entity should be spawned on client after removing from blacklist" + .query_filtered::<(), (With, With)>() + .single(&client_app.world); +} + +#[test] +fn whitelist_visibility() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + visibility_policy: VisibilityPolicy::Whitelist, + ..Default::default() + }), + )) + .replicate::(); + } + + connect::single_client(&mut server_app, &mut client_app); + + let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + + let client_transport = client_app.world.resource::(); + let client_id = ClientId::from_raw(client_transport.client_id()); + let clients_info = server_app.world.resource::(); + let visibility = clients_info.get(client_id).unwrap().visibility(); + assert!(!visibility.is_visible(server_entity)); + + server_app.update(); + client_app.update(); + + assert!( + client_app.world.entities().is_empty(), + "no entities should be replicated without adding to whitelist" ); } #[test] -fn whitelist_replication() { +fn whitelist_visibility_add_remove() { let mut server_app = App::new(); let mut client_app = App::new(); for app in [&mut server_app, &mut client_app] { @@ -886,7 +921,6 @@ fn whitelist_replication() { connect::single_client(&mut server_app, &mut client_app); let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); - let hidden_entity = server_app.world.spawn((Replication, TableComponent)).id(); let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); @@ -894,22 +928,15 @@ fn whitelist_replication() { let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); visibility.set_visible(server_entity, true); assert!(visibility.is_visible(server_entity)); - assert!(!visibility.is_visible(hidden_entity)); server_app.update(); client_app.update(); - let client_entity = client_app + client_app .world - .query_filtered::, With)>() + .query_filtered::<(), (With, With)>() .single(&client_app.world); - let entity_map = client_app.world.resource::(); - assert_eq!( - entity_map.to_client().get(&server_entity), - Some(&client_entity), - ); - // Reverse visibility. let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); @@ -920,7 +947,7 @@ fn whitelist_replication() { assert!( client_app.world.entities().is_empty(), - "server entity should be despawned after removing from whitelist" + "entity should be despawned after removing from whitelist" ); } From 64839be7702808769e4c43fc99f1b3dd9a7a377b Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 01:15:59 +0200 Subject: [PATCH 20/48] Apply suggestions from code review [skip ci] Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com> --- src/server.rs | 4 ++-- src/server/clients_info.rs | 14 +++++++------- src/server/clients_info/client_visibility.rs | 10 +++++----- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/server.rs b/src/server.rs index 6f475e53..c0bac81b 100644 --- a/src/server.rs +++ b/src/server.rs @@ -522,9 +522,9 @@ pub enum VisibilityPolicy { /// All entities are visible by default and visibility can't be changed. #[default] All, - /// All entities are hidden by default and should be explicitly marked to be visible. + /// All entities are hidden by default and should be explicitly registered to be visible. Blacklist, - /// All entities are visible by default and should be explicitly marked to be hidden. + /// All entities are visible by default and should be explicitly registered to be hidden. Whitelist, } diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index c6946cf5..835427f6 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -48,29 +48,29 @@ impl ClientsInfo { self.info.iter().find(|info| info.id == client_id) } - /// Returns mutable reference to connected client info. + /// Returns a mutable reference to a connected client's info. /// /// This operation is *O*(*n*). pub fn get_mut(&mut self, client_id: ClientId) -> Option<&mut ClientInfo> { self.info.iter_mut().find(|info| info.id == client_id) } - /// Returns an iterator over clients information. + /// Returns an iterator over client information. pub fn iter(&self) -> impl Iterator { self.info.iter() } - /// Returns a mutable iterator over clients information. + /// Returns a mutable iterator over client information. pub fn iter_mut(&mut self) -> impl Iterator { self.info.iter_mut() } - /// Returns number of connected clients. + /// Returns the number of connected clients. pub fn len(&self) -> usize { self.info.len() } - /// Returns `true` if no clients connected. + /// Returns `true` if no clients are connected. pub fn is_empty(&self) -> bool { self.info.is_empty() } @@ -156,12 +156,12 @@ impl ClientInfo { self.id } - /// Returns reference to client visibility settings. + /// Returns a reference to the client's visibility settings. pub fn visibility(&self) -> &ClientVisibility { &self.visibility } - /// Returns mutable reference to client visibility settings. + /// Returns a mutable reference to the client's visibility settings. pub fn visibility_mut(&mut self) -> &mut ClientVisibility { &mut self.visibility } diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index e425398a..cf96801a 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -31,7 +31,7 @@ impl ClientVisibility { } } - /// Creates a new instance with specific filter. + /// Creates a new instance with a specific filter. fn with_filter(filter: VisibilityFilter) -> Self { Self { filter, @@ -119,7 +119,7 @@ impl ClientVisibility { } } - /// Returns an iterator of entities that lost visibility at this tick. + /// Returns an iterator of entities the client lost visibility of this tick. pub(super) fn iter_lost_visibility(&self) -> impl Iterator + '_ { match &self.filter { VisibilityFilter::All { .. } => VisibilityLostIter::AllVisible, @@ -132,9 +132,9 @@ impl ClientVisibility { } } - /// Sets visibility for specific entity. + /// Sets visibility for a specific entity. /// - /// Does nothing if visibility policy for the server plugin is set to [`VisibilityPolicy::All`]. + /// Does nothing if the visibility policy for the server plugin is set to [`VisibilityPolicy::All`]. pub fn set_visible(&mut self, entity: Entity, visibile: bool) { match &mut self.filter { VisibilityFilter::All { .. } => { @@ -184,7 +184,7 @@ impl ClientVisibility { } } - /// Gets visibility for specific entity. + /// Gets visibility for a specific entity. pub fn is_visible(&self, entity: Entity) -> bool { match &self.filter { VisibilityFilter::All { .. } => true, From ac64583f350ac90ad8f9e4bf4dce6a51faeddc61 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 01:19:29 +0200 Subject: [PATCH 21/48] Apply more docs [skip ci] Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com> --- src/server/clients_info.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 835427f6..11f23025 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -41,7 +41,7 @@ impl ClientsInfo { } } - /// Returns reference to connected client info. + /// Returns a reference to a connected client's info. /// /// This operation is *O*(*n*). pub fn get(&self, client_id: ClientId) -> Option<&ClientInfo> { From 156d235f59804810ff4bef560eac3f7a7a7a65c3 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 01:24:36 +0200 Subject: [PATCH 22/48] Rename `set_visible` into `set_visibility` --- src/server/clients_info/client_visibility.rs | 2 +- tests/replication.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index cf96801a..4a9b493f 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -135,7 +135,7 @@ impl ClientVisibility { /// Sets visibility for a specific entity. /// /// Does nothing if the visibility policy for the server plugin is set to [`VisibilityPolicy::All`]. - pub fn set_visible(&mut self, entity: Entity, visibile: bool) { + pub fn set_visibility(&mut self, entity: Entity, visibile: bool) { match &mut self.filter { VisibilityFilter::All { .. } => { if visibile { diff --git a/tests/replication.rs b/tests/replication.rs index 92a41593..9b68494f 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -775,14 +775,14 @@ fn all_visibility() { let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(server_entity, false); + visibility.set_visibility(server_entity, false); assert!( visibility.is_visible(server_entity), "shouldn't have any effect with {:?}", VisibilityPolicy::All ); - visibility.set_visible(server_entity, true); + visibility.set_visibility(server_entity, true); assert!(visibility.is_visible(server_entity)); } @@ -845,7 +845,7 @@ fn blacklist_visibility_add_remove() { let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(server_entity, false); + visibility.set_visibility(server_entity, false); assert!(!visibility.is_visible(server_entity)); server_app.update(); @@ -856,7 +856,7 @@ fn blacklist_visibility_add_remove() { // Reverse visibility back. let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(server_entity, true); + visibility.set_visibility(server_entity, true); server_app.update(); client_app.update(); @@ -926,7 +926,7 @@ fn whitelist_visibility_add_remove() { let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(server_entity, true); + visibility.set_visibility(server_entity, true); assert!(visibility.is_visible(server_entity)); server_app.update(); @@ -940,7 +940,7 @@ fn whitelist_visibility_add_remove() { // Reverse visibility. let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); - visibility.set_visible(server_entity, false); + visibility.set_visibility(server_entity, false); server_app.update(); client_app.update(); From b63c8e1804b77a1a93206d193a58f4aaa610b875 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 01:27:02 +0200 Subject: [PATCH 23/48] Apply `VisibilityFilter` docs suggestions --- src/server/clients_info/client_visibility.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 4a9b493f..fce0bbc9 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -231,19 +231,20 @@ enum VisibilityFilter { just_connected: bool, }, Blacklist { - /// All blacklisted entities and an indicator of whether it is in the queue for deletion at this tick. + /// All blacklisted entities and an indicator of whether it is in the queue for deletion + /// at the end of this tick. list: EntityHashMap, - /// All entities that were removed from the list on this tick. + /// All entities that were removed from the list in this tick. added: EntityHashSet, - /// All entities that were added to the list on this tick. + /// All entities that were added to the list in this tick. removed: EntityHashSet, }, Whitelist { - /// All whitelisted entities and an indicator whether it was added to the list on this tick. + /// All whitelisted entities and an indicator whether it was added to the list in this tick. list: EntityHashMap, - /// All entities that were added to the list on this tick. + /// All entities that were added to the list in this tick. added: EntityHashSet, - /// All entities that were removed from the list on this tick. + /// All entities that were removed from the list in this tick. removed: EntityHashSet, }, } From d7813f1badff6ef2dfb3bb3bbbbd65971299a8bb Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 02:04:03 +0200 Subject: [PATCH 24/48] Apply suggested change to `EntityState` enum --- src/server.rs | 10 +++--- src/server/clients_info/client_visibility.rs | 35 ++++++++++++-------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/src/server.rs b/src/server.rs index c0bac81b..ee1c6ebd 100644 --- a/src/server.rs +++ b/src/server.rs @@ -27,7 +27,7 @@ use bevy_renet::{ use crate::replicon_core::{ replication_rules::ReplicationRules, replicon_tick::RepliconTick, ReplicationChannel, }; -use clients_info::{client_visibility::EntityState, ClientBuffers, ClientInfo, ClientsInfo}; +use clients_info::{client_visibility::Visibility, ClientBuffers, ClientInfo, ClientsInfo}; use despawn_buffer::{DespawnBuffer, DespawnBufferPlugin}; use removal_buffer::{RemovalBuffer, RemovalBufferPlugin}; use replicated_archetypes_info::ReplicatedArchetypesInfo; @@ -334,11 +334,11 @@ fn collect_changes( let mut shared_bytes = None; for (init_message, update_message, client_info) in messages.iter_mut_with_info() { let entity_state = client_info.visibility().entity_state(); - if entity_state == EntityState::Hidden { + if entity_state == Visibility::Hidden { continue; } - let new_entity = marker_added || entity_state == EntityState::JustVisible; + let new_entity = marker_added || entity_state == Visibility::Gained; if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run()) { init_message.write_component( @@ -365,11 +365,11 @@ fn collect_changes( for (init_message, update_message, client_info) in messages.iter_mut_with_info() { let entity_state = client_info.visibility().entity_state(); - if entity_state == EntityState::Hidden { + if entity_state == Visibility::Hidden { continue; } - let new_entity = marker_added || entity_state == EntityState::JustVisible; + let new_entity = marker_added || entity_state == Visibility::Gained; if new_entity || init_message.entity_data_size() != 0 { // If there is any insertion or we must initialize, include all updates into init message // and bump the last acknowledged tick to keep entity updates atomic. diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index fce0bbc9..f3b030de 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -8,7 +8,7 @@ use super::VisibilityPolicy; /// Entity visibility settings for a client. pub struct ClientVisibility { filter: VisibilityFilter, - entity_state: EntityState, + entity_state: Visibility, } impl ClientVisibility { @@ -200,26 +200,26 @@ impl ClientVisibility { match &mut self.filter { VisibilityFilter::All { just_connected } => { if *just_connected { - self.entity_state = EntityState::JustVisible + self.entity_state = Visibility::Gained } else { - self.entity_state = EntityState::Visible + self.entity_state = Visibility::Visible } } VisibilityFilter::Blacklist { list, .. } => match list.get(&entity) { - Some(true) => self.entity_state = EntityState::JustVisible, - Some(false) => self.entity_state = EntityState::Hidden, - None => self.entity_state = EntityState::Visible, + Some(true) => self.entity_state = Visibility::Gained, + Some(false) => self.entity_state = Visibility::Hidden, + None => self.entity_state = Visibility::Visible, }, VisibilityFilter::Whitelist { list, .. } => match list.get(&entity) { - Some(true) => self.entity_state = EntityState::JustVisible, - Some(false) => self.entity_state = EntityState::Visible, - None => self.entity_state = EntityState::Hidden, + Some(true) => self.entity_state = Visibility::Gained, + Some(false) => self.entity_state = Visibility::Visible, + None => self.entity_state = Visibility::Hidden, }, } } /// Returns state obtained from last call of [`Self::read_entity_state`]. - pub(crate) fn entity_state(&self) -> EntityState { + pub(crate) fn entity_state(&self) -> Visibility { self.entity_state } } @@ -249,14 +249,21 @@ enum VisibilityFilter { }, } -/// Visibility state for an entity. +/// Visibility state for an entity in the current tick, from the perspective of one client. +/// +/// Note that the distinction between 'lost visibility' and 'don't have visibility' is not exposed here. +/// There is only `Visibility::Hidden` to encompass both variants. +/// +/// Lost visibility is handled separately with [`ClientVisibility::iter_lost_visibility`]. #[derive(PartialEq, Default, Clone, Copy)] -pub(crate) enum EntityState { +pub(crate) enum Visibility { + /// The client does not have visibility of the entity in this tick. #[default] - None, Hidden, + /// The client gained visibility of the entity in this tick (it was not visible in the previous tick). + Gained, + /// The entity is visible to the client (and was visible in the previous tick). Visible, - JustVisible, } enum VisibilityLostIter { From fc1765198139798a122c6bb75d887aa2d71de5e3 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 02:05:51 +0200 Subject: [PATCH 25/48] Apply suggested naming about entity_state --- src/server.rs | 14 ++++---- src/server/clients_info/client_visibility.rs | 36 +++++++++++--------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/server.rs b/src/server.rs index ee1c6ebd..7c334cbd 100644 --- a/src/server.rs +++ b/src/server.rs @@ -300,7 +300,7 @@ fn collect_changes( update_message.start_entity_data(entity.entity()); client_info .visibility_mut() - .read_entity_state(entity.entity()); + .cache_visibility(entity.entity()); } // SAFETY: all replicated archetypes have marker component with table storage. @@ -333,12 +333,12 @@ fn collect_changes( let mut shared_bytes = None; for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let entity_state = client_info.visibility().entity_state(); - if entity_state == Visibility::Hidden { + let visibility = client_info.visibility().cached_visibility(); + if visibility == Visibility::Hidden { continue; } - let new_entity = marker_added || entity_state == Visibility::Gained; + let new_entity = marker_added || visibility == Visibility::Gained; if new_entity || ticks.is_added(change_tick.last_run(), change_tick.this_run()) { init_message.write_component( @@ -364,12 +364,12 @@ fn collect_changes( } for (init_message, update_message, client_info) in messages.iter_mut_with_info() { - let entity_state = client_info.visibility().entity_state(); - if entity_state == Visibility::Hidden { + let visibility = client_info.visibility().cached_visibility(); + if visibility == Visibility::Hidden { continue; } - let new_entity = marker_added || entity_state == Visibility::Gained; + let new_entity = marker_added || visibility == Visibility::Gained; if new_entity || init_message.entity_data_size() != 0 { // If there is any insertion or we must initialize, include all updates into init message // and bump the last acknowledged tick to keep entity updates atomic. diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index f3b030de..fa8a8137 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -8,7 +8,11 @@ use super::VisibilityPolicy; /// Entity visibility settings for a client. pub struct ClientVisibility { filter: VisibilityFilter, - entity_state: Visibility, + + /// Visibility for a specific entity that has been cached for re-referencing. + /// + /// Used as an optimization by server replication. + cached_visibility: Visibility, } impl ClientVisibility { @@ -35,13 +39,13 @@ impl ClientVisibility { fn with_filter(filter: VisibilityFilter) -> Self { Self { filter, - entity_state: Default::default(), + cached_visibility: Default::default(), } } /// Resets the filter state to as it was after [`Self::new`]. /// - /// `entity_state` remains untouched. + /// `visibility` remains untouched. pub(super) fn clear(&mut self) { match &mut self.filter { VisibilityFilter::All { just_connected } => *just_connected = true, @@ -195,32 +199,32 @@ impl ClientVisibility { /// Reads entity visibility state for specific entity. /// - /// Can be obtained later from [`Self::entity_state`]. - pub(crate) fn read_entity_state(&mut self, entity: Entity) { + /// Can be obtained later from [`Self::cached_visibility`]. + pub(crate) fn cache_visibility(&mut self, entity: Entity) { match &mut self.filter { VisibilityFilter::All { just_connected } => { if *just_connected { - self.entity_state = Visibility::Gained + self.cached_visibility = Visibility::Gained } else { - self.entity_state = Visibility::Visible + self.cached_visibility = Visibility::Visible } } VisibilityFilter::Blacklist { list, .. } => match list.get(&entity) { - Some(true) => self.entity_state = Visibility::Gained, - Some(false) => self.entity_state = Visibility::Hidden, - None => self.entity_state = Visibility::Visible, + Some(true) => self.cached_visibility = Visibility::Gained, + Some(false) => self.cached_visibility = Visibility::Hidden, + None => self.cached_visibility = Visibility::Visible, }, VisibilityFilter::Whitelist { list, .. } => match list.get(&entity) { - Some(true) => self.entity_state = Visibility::Gained, - Some(false) => self.entity_state = Visibility::Visible, - None => self.entity_state = Visibility::Hidden, + Some(true) => self.cached_visibility = Visibility::Gained, + Some(false) => self.cached_visibility = Visibility::Visible, + None => self.cached_visibility = Visibility::Hidden, }, } } - /// Returns state obtained from last call of [`Self::read_entity_state`]. - pub(crate) fn entity_state(&self) -> Visibility { - self.entity_state + /// Returns state obtained from last call of [`Self::cache_visibility`]. + pub(crate) fn cached_visibility(&self) -> Visibility { + self.cached_visibility } } From 751531243e74aa79c9fac47df908f07152e65af5 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 02:16:15 +0200 Subject: [PATCH 26/48] Fix remove_despawned logic --- src/server/clients_info/client_visibility.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index fa8a8137..c99d13d5 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -103,14 +103,8 @@ impl ClientVisibility { list, added, removed, - } => { - if let Some(just_removed) = list.get_mut(&entity) { - *just_removed = true; - removed.remove(&entity); - added.remove(&entity); - } } - VisibilityFilter::Whitelist { + | VisibilityFilter::Whitelist { list, added, removed, From e6bac4b23b9628af602cbe9ec179f18c44c271a1 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 02:42:54 +0200 Subject: [PATCH 27/48] Add panicking versions for getting `ClientInfo` --- src/network_event/server_event.rs | 2 +- src/server/clients_info.rs | 32 +++++++++++++++++++++++++++++-- tests/replication.rs | 14 +++++++------- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/src/network_event/server_event.rs b/src/network_event/server_event.rs index 2c64b13c..e4a7ed07 100644 --- a/src/network_event/server_event.rs +++ b/src/network_event/server_event.rs @@ -323,7 +323,7 @@ pub fn send_with( } SendMode::Direct(client_id) => { if client_id != SERVER_ID { - if let Some(client_info) = clients_info.get(client_id) { + if let Some(client_info) = clients_info.get_client(client_id) { let message = serialize_with(client_info, None, &serialize_fn)?; server.send_message(client_info.id(), channel, message.bytes); } diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 11f23025..1f7302ba 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -44,14 +44,42 @@ impl ClientsInfo { /// Returns a reference to a connected client's info. /// /// This operation is *O*(*n*). - pub fn get(&self, client_id: ClientId) -> Option<&ClientInfo> { + /// See also [`Self::get_client`] for the fallible version. + /// + /// # Panics + /// + /// Panics if passed client ID is not connected. + pub fn client(&self, client_id: ClientId) -> &ClientInfo { + self.get_client(client_id) + .unwrap_or_else(|| panic!("{client_id:?} should be connected")) + } + + /// Returns a mutable reference to a connected client's info. + /// + /// This operation is *O*(*n*). + /// See also [`Self::get_client_mut`] for the fallible version. + /// + /// # Panics + /// + /// Panics if passed client ID is not connected. + pub fn client_mut(&mut self, client_id: ClientId) -> &mut ClientInfo { + self.get_client_mut(client_id) + .unwrap_or_else(|| panic!("{client_id:?} should be connected")) + } + + /// Returns a reference to a connected client's info. + /// + /// This operation is *O*(*n*). + /// See also [`Self::client`] for the panicking version. + pub fn get_client(&self, client_id: ClientId) -> Option<&ClientInfo> { self.info.iter().find(|info| info.id == client_id) } /// Returns a mutable reference to a connected client's info. /// /// This operation is *O*(*n*). - pub fn get_mut(&mut self, client_id: ClientId) -> Option<&mut ClientInfo> { + /// See also [`Self::client`] for the panicking version. + pub fn get_client_mut(&mut self, client_id: ClientId) -> Option<&mut ClientInfo> { self.info.iter_mut().find(|info| info.id == client_id) } diff --git a/tests/replication.rs b/tests/replication.rs index 9b68494f..df5af72b 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -774,7 +774,7 @@ fn all_visibility() { let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, false); assert!( visibility.is_visible(server_entity), @@ -809,7 +809,7 @@ fn blacklist_visibility() { let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); let clients_info = server_app.world.resource::(); - let visibility = clients_info.get(client_id).unwrap().visibility(); + let visibility = clients_info.client(client_id).visibility(); assert!(visibility.is_visible(server_entity)); server_app.update(); @@ -844,7 +844,7 @@ fn blacklist_visibility_add_remove() { let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, false); assert!(!visibility.is_visible(server_entity)); @@ -855,7 +855,7 @@ fn blacklist_visibility_add_remove() { // Reverse visibility back. let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, true); server_app.update(); @@ -890,7 +890,7 @@ fn whitelist_visibility() { let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); let clients_info = server_app.world.resource::(); - let visibility = clients_info.get(client_id).unwrap().visibility(); + let visibility = clients_info.client(client_id).visibility(); assert!(!visibility.is_visible(server_entity)); server_app.update(); @@ -925,7 +925,7 @@ fn whitelist_visibility_add_remove() { let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, true); assert!(visibility.is_visible(server_entity)); @@ -939,7 +939,7 @@ fn whitelist_visibility_add_remove() { // Reverse visibility. let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.get_mut(client_id).unwrap().visibility_mut(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, false); server_app.update(); From b5071ab22dedb1dce5576913bbca5b3d40fe0711 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 03:02:43 +0200 Subject: [PATCH 28/48] Use enums instead of bools --- src/server/clients_info/client_visibility.rs | 61 ++++++++++++++------ 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index c99d13d5..5db7e85f 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -53,8 +53,12 @@ impl ClientVisibility { list, added, removed, + } => { + list.clear(); + added.clear(); + removed.clear(); } - | VisibilityFilter::Whitelist { + VisibilityFilter::Whitelist { list, added, removed, @@ -88,7 +92,7 @@ impl ClientVisibility { removed, } => { for entity in added.drain() { - list.insert(entity, false); + list.insert(entity, WhitelistInfo::Visible); } removed.clear(); } @@ -103,8 +107,13 @@ impl ClientVisibility { list, added, removed, + } => { + if list.remove(&entity).is_some() { + added.remove(&entity); + removed.remove(&entity); + } } - | VisibilityFilter::Whitelist { + VisibilityFilter::Whitelist { list, added, removed, @@ -153,15 +162,19 @@ impl ClientVisibility { added, removed, } => { - if !visibile { - if list.insert(entity, false).is_none() { - removed.remove(&entity); - added.insert(entity); + if visibile { + // For blacklist we don't remove the entity right away. + // Instead we mark it as queued for removal and remove it + // later in `Self::update`. This allows us to avoid extra lookup + // into `removed` inside `cache_visibility`. + if let Some(info) = list.get_mut(&entity) { + *info = BlacklistInfo::QueuedForRemoval; + removed.insert(entity); + added.remove(&entity); } - } else if let Some(just_removed) = list.get_mut(&entity) { - *just_removed = true; - removed.insert(entity); - added.remove(&entity); + } else if list.insert(entity, BlacklistInfo::Hidden).is_none() { + removed.remove(&entity); + added.insert(entity); } } VisibilityFilter::Whitelist { @@ -170,7 +183,7 @@ impl ClientVisibility { removed, } => { if visibile { - if list.insert(entity, true).is_none() { + if list.insert(entity, WhitelistInfo::JustAdded).is_none() { removed.remove(&entity); added.insert(entity); } @@ -204,13 +217,15 @@ impl ClientVisibility { } } VisibilityFilter::Blacklist { list, .. } => match list.get(&entity) { - Some(true) => self.cached_visibility = Visibility::Gained, - Some(false) => self.cached_visibility = Visibility::Hidden, + Some(BlacklistInfo::QueuedForRemoval) => { + self.cached_visibility = Visibility::Gained + } + Some(BlacklistInfo::Hidden) => self.cached_visibility = Visibility::Hidden, None => self.cached_visibility = Visibility::Visible, }, VisibilityFilter::Whitelist { list, .. } => match list.get(&entity) { - Some(true) => self.cached_visibility = Visibility::Gained, - Some(false) => self.cached_visibility = Visibility::Visible, + Some(WhitelistInfo::JustAdded) => self.cached_visibility = Visibility::Gained, + Some(WhitelistInfo::Visible) => self.cached_visibility = Visibility::Visible, None => self.cached_visibility = Visibility::Hidden, }, } @@ -231,7 +246,7 @@ enum VisibilityFilter { Blacklist { /// All blacklisted entities and an indicator of whether it is in the queue for deletion /// at the end of this tick. - list: EntityHashMap, + list: EntityHashMap, /// All entities that were removed from the list in this tick. added: EntityHashSet, /// All entities that were added to the list in this tick. @@ -239,7 +254,7 @@ enum VisibilityFilter { }, Whitelist { /// All whitelisted entities and an indicator whether it was added to the list in this tick. - list: EntityHashMap, + list: EntityHashMap, /// All entities that were added to the list in this tick. added: EntityHashSet, /// All entities that were removed from the list in this tick. @@ -247,6 +262,16 @@ enum VisibilityFilter { }, } +enum WhitelistInfo { + Visible, + JustAdded, +} + +enum BlacklistInfo { + Hidden, + QueuedForRemoval, +} + /// Visibility state for an entity in the current tick, from the perspective of one client. /// /// Note that the distinction between 'lost visibility' and 'don't have visibility' is not exposed here. From 2df564be7c4faadbf2f008c785d3abd977eb87b3 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 03:09:19 +0200 Subject: [PATCH 29/48] Write more comments about the logic --- src/server/clients_info/client_visibility.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 5db7e85f..ae3f3df6 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -172,9 +172,12 @@ impl ClientVisibility { removed.insert(entity); added.remove(&entity); } - } else if list.insert(entity, BlacklistInfo::Hidden).is_none() { + } else { + if list.insert(entity, BlacklistInfo::Hidden).is_none() { + // Do not mark an entry as newly added if the entry was already in the list. + added.insert(entity); + } removed.remove(&entity); - added.insert(entity); } } VisibilityFilter::Whitelist { @@ -184,9 +187,10 @@ impl ClientVisibility { } => { if visibile { if list.insert(entity, WhitelistInfo::JustAdded).is_none() { - removed.remove(&entity); + // Do not mark an entry as newly added if the entry was already in the list. added.insert(entity); } + removed.remove(&entity); } else if list.remove(&entity).is_some() { removed.insert(entity); added.remove(&entity); From 1f00634e27286d6abea48e883c40a93ad78b7996 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 11:54:28 +0200 Subject: [PATCH 30/48] Apply suggestions from code review [skip ci] Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com> --- src/server/clients_info.rs | 4 ++-- src/server/clients_info/client_visibility.rs | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 1f7302ba..90b88a2a 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -48,7 +48,7 @@ impl ClientsInfo { /// /// # Panics /// - /// Panics if passed client ID is not connected. + /// Panics if the passed client ID is not connected. pub fn client(&self, client_id: ClientId) -> &ClientInfo { self.get_client(client_id) .unwrap_or_else(|| panic!("{client_id:?} should be connected")) @@ -61,7 +61,7 @@ impl ClientsInfo { /// /// # Panics /// - /// Panics if passed client ID is not connected. + /// Panics if the passed client ID is not connected. pub fn client_mut(&mut self, client_id: ClientId) -> &mut ClientInfo { self.get_client_mut(client_id) .unwrap_or_else(|| panic!("{client_id:?} should be connected")) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index ae3f3df6..dd1f433f 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -45,7 +45,7 @@ impl ClientVisibility { /// Resets the filter state to as it was after [`Self::new`]. /// - /// `visibility` remains untouched. + /// `cached_visibility` remains untouched. pub(super) fn clear(&mut self) { match &mut self.filter { VisibilityFilter::All { just_connected } => *just_connected = true, @@ -165,8 +165,8 @@ impl ClientVisibility { if visibile { // For blacklist we don't remove the entity right away. // Instead we mark it as queued for removal and remove it - // later in `Self::update`. This allows us to avoid extra lookup - // into `removed` inside `cache_visibility`. + // later in `Self::update`. This allows us to avoid accessing + // the blacklist's `removed` field in `Self::cache_visibility`. if let Some(info) = list.get_mut(&entity) { *info = BlacklistInfo::QueuedForRemoval; removed.insert(entity); @@ -186,6 +186,10 @@ impl ClientVisibility { removed, } => { if visibile { + /// Similar to Blacklist removal, we don't just add the entity to the list. + /// Instead we mark it as 'just added' and then set it to 'visible' in `Self::update`. + /// This allows us to avoid accessing the whitelist's `added` field in + /// `Self::cache_visibility`. if list.insert(entity, WhitelistInfo::JustAdded).is_none() { // Do not mark an entry as newly added if the entry was already in the list. added.insert(entity); @@ -208,7 +212,7 @@ impl ClientVisibility { } } - /// Reads entity visibility state for specific entity. + /// Caches visibility for a specific entity. /// /// Can be obtained later from [`Self::cached_visibility`]. pub(crate) fn cache_visibility(&mut self, entity: Entity) { @@ -235,7 +239,7 @@ impl ClientVisibility { } } - /// Returns state obtained from last call of [`Self::cache_visibility`]. + /// Returns visibility cached by the last call of [`Self::cache_visibility`]. pub(crate) fn cached_visibility(&self) -> Visibility { self.cached_visibility } @@ -248,7 +252,7 @@ enum VisibilityFilter { just_connected: bool, }, Blacklist { - /// All blacklisted entities and an indicator of whether it is in the queue for deletion + /// All blacklisted entities and an indicator of whether they are in the queue for deletion /// at the end of this tick. list: EntityHashMap, /// All entities that were removed from the list in this tick. @@ -257,7 +261,8 @@ enum VisibilityFilter { removed: EntityHashSet, }, Whitelist { - /// All whitelisted entities and an indicator whether it was added to the list in this tick. + /// All whitelisted entities and an indicator of whether they were added to the list in + /// this tick. list: EntityHashMap, /// All entities that were added to the list in this tick. added: EntityHashSet, From 643f2fc858ae7f5a1b63d73ddae0a53a26fe97a1 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 11:27:11 +0200 Subject: [PATCH 31/48] Remove size_hint Unused. --- src/server/clients_info/client_visibility.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index dd1f433f..88d00980 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -312,13 +312,4 @@ impl Iterator for VisibilityLostIter { VisibilityLostIter::Lost(entities) => entities.next(), } } - - fn size_hint(&self) -> (usize, Option) { - match self { - VisibilityLostIter::AllVisible => (0, Some(0)), - VisibilityLostIter::Lost(entities) => entities.size_hint(), - } - } } - -impl ExactSizeIterator for VisibilityLostIter {} From 9641dea2071a009f3c69eac6ff2567a1d846cdf5 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 11:43:34 +0200 Subject: [PATCH 32/48] Use drain instead of iteration --- src/server.rs | 2 +- src/server/clients_info.rs | 8 ++--- src/server/clients_info/client_visibility.rs | 33 ++++++++------------ 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/src/server.rs b/src/server.rs index 7c334cbd..59a61dd5 100644 --- a/src/server.rs +++ b/src/server.rs @@ -439,7 +439,7 @@ fn collect_despawns( } for (message, _, client_info) in messages.iter_mut_with_info() { - for entity in client_info.remove_lost_visibility() { + for entity in client_info.drain_lost_visibility() { message.write_entity(&mut None, entity)?; } diff --git a/src/server/clients_info.rs b/src/server/clients_info.rs index 90b88a2a..34b7c7a9 100644 --- a/src/server/clients_info.rs +++ b/src/server/clients_info.rs @@ -304,11 +304,11 @@ impl ClientInfo { // `Self::acknowledge()` will properly ignore despawned entities. } - /// Iterates over all entities for which visibility was lost during this tick and removes them. + /// Drains all entities for which visibility was lost during this tick. /// - /// Removal happens lazily during the iteration. - pub(super) fn remove_lost_visibility(&mut self) -> impl Iterator + '_ { - self.visibility.iter_lost_visibility().inspect(|entity| { + /// Internal cleanup happens lazily during the iteration. + pub(super) fn drain_lost_visibility(&mut self) -> impl Iterator + '_ { + self.visibility.drain_lost_visibility().inspect(|entity| { self.ticks.remove(entity); }) } diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 88d00980..6c36bb77 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -70,31 +70,26 @@ impl ClientVisibility { } } - /// Marks all entities as not "just added" and clears the removed. + /// Updates list information and its sets based on the filter. /// /// Should be called after each tick. pub(crate) fn update(&mut self) { match &mut self.filter { VisibilityFilter::All { just_connected } => *just_connected = false, - VisibilityFilter::Blacklist { - list, - added, - removed, - } => { + VisibilityFilter::Blacklist { list, removed, .. } => { + // Remove all entities queued for removal. for entity in removed.drain() { list.remove(&entity); } - added.clear(); + // `added` is drained in `Self::drain_lost_visibility`. } - VisibilityFilter::Whitelist { - list, - added, - removed, - } => { + VisibilityFilter::Whitelist { list, added, .. } => { + // Change all recently added entities to `WhitelistInfo::Visible` + // from `WhitelistInfo::JustVisible`. for entity in added.drain() { list.insert(entity, WhitelistInfo::Visible); } - removed.clear(); + // `removed` is drained in `Self::drain_lost_visibility`. } } } @@ -126,15 +121,13 @@ impl ClientVisibility { } } - /// Returns an iterator of entities the client lost visibility of this tick. - pub(super) fn iter_lost_visibility(&self) -> impl Iterator + '_ { - match &self.filter { + /// Drains all entities for which visibility was lost during this tick. + pub(super) fn drain_lost_visibility(&mut self) -> impl Iterator + '_ { + match &mut self.filter { VisibilityFilter::All { .. } => VisibilityLostIter::AllVisible, - VisibilityFilter::Blacklist { added, .. } => { - VisibilityLostIter::Lost(added.iter().copied()) - } + VisibilityFilter::Blacklist { added, .. } => VisibilityLostIter::Lost(added.drain()), VisibilityFilter::Whitelist { removed, .. } => { - VisibilityLostIter::Lost(removed.iter().copied()) + VisibilityLostIter::Lost(removed.drain()) } } } From 832bbca24f409777a0698cadad489bf57e9da456 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 11:57:24 +0200 Subject: [PATCH 33/48] Apply docs suggestions --- src/server/clients_info/client_visibility.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 6c36bb77..a46482c9 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -242,24 +242,38 @@ impl ClientVisibility { 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, }, Blacklist { /// All blacklisted entities and an indicator of whether they are in the queue for deletion /// at the end of this tick. list: EntityHashMap, + /// All entities that were removed from the list in this tick. + /// + /// Visibility of these entities has been lost. added: EntityHashSet, + /// All entities that were added to the list in this tick. + /// + /// Visibility of these entities has been gained. removed: EntityHashSet, }, Whitelist { /// All whitelisted entities and an indicator of whether they were added to the list in /// this tick. list: EntityHashMap, - /// All entities that were added to the list in this tick. + + /// All entities that were added to the list in tVisibility of these entities has been gained.his tick. + /// + /// Visibility of these entities has been gained. added: EntityHashSet, + /// All entities that were removed from the list in this tick. + /// + /// Visibility of these entities has been lost. removed: EntityHashSet, }, } From 761254fa3745c67bed9b9917891188663f728d87 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 11:57:40 +0200 Subject: [PATCH 34/48] Fix warning about doc comment --- src/server/clients_info/client_visibility.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index a46482c9..de25cb1a 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -179,10 +179,10 @@ impl ClientVisibility { removed, } => { if visibile { - /// Similar to Blacklist removal, we don't just add the entity to the list. - /// Instead we mark it as 'just added' and then set it to 'visible' in `Self::update`. - /// This allows us to avoid accessing the whitelist's `added` field in - /// `Self::cache_visibility`. + // Similar to blacklist removal, we don't just add the entity to the list. + // Instead we mark it as 'just added' and then set it to 'visible' in `Self::update`. + // This allows us to avoid accessing the whitelist's `added` field in + // `Self::cache_visibility`. if list.insert(entity, WhitelistInfo::JustAdded).is_none() { // Do not mark an entry as newly added if the entry was already in the list. added.insert(entity); From 8497b87afac78d6f2e385380b6788f5abb2b3c14 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 12:08:05 +0200 Subject: [PATCH 35/48] Fix bug about re-adding in the list --- src/server/clients_info/client_visibility.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index de25cb1a..0ae61eb0 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -166,7 +166,8 @@ impl ClientVisibility { added.remove(&entity); } } else { - if list.insert(entity, BlacklistInfo::Hidden).is_none() { + if *list.entry(entity).or_insert(BlacklistInfo::Hidden) == BlacklistInfo::Hidden + { // Do not mark an entry as newly added if the entry was already in the list. added.insert(entity); } @@ -183,7 +184,9 @@ impl ClientVisibility { // Instead we mark it as 'just added' and then set it to 'visible' in `Self::update`. // This allows us to avoid accessing the whitelist's `added` field in // `Self::cache_visibility`. - if list.insert(entity, WhitelistInfo::JustAdded).is_none() { + if *list.entry(entity).or_insert(WhitelistInfo::JustAdded) + == WhitelistInfo::JustAdded + { // Do not mark an entry as newly added if the entry was already in the list. added.insert(entity); } @@ -278,11 +281,13 @@ enum VisibilityFilter { }, } +#[derive(PartialEq, Clone, Copy)] enum WhitelistInfo { Visible, JustAdded, } +#[derive(PartialEq, Clone, Copy)] enum BlacklistInfo { Hidden, QueuedForRemoval, From ed3881b452e284ef778401dca89999dbab8848cc Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 13:44:38 +0200 Subject: [PATCH 36/48] Refactor visibility logic as suggested --- src/server/clients_info/client_visibility.rs | 42 ++++++++++---------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 0ae61eb0..e3bf99a2 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -201,10 +201,9 @@ impl ClientVisibility { /// Gets visibility for a specific entity. pub fn is_visible(&self, entity: Entity) -> bool { - match &self.filter { - VisibilityFilter::All { .. } => true, - VisibilityFilter::Blacklist { list, .. } => !list.contains_key(&entity), - VisibilityFilter::Whitelist { list, .. } => list.contains_key(&entity), + match self.get_visibility_state(entity) { + Visibility::Hidden => false, + Visibility::Gained | Visibility::Visible => true, } } @@ -212,33 +211,36 @@ impl ClientVisibility { /// /// Can be obtained later from [`Self::cached_visibility`]. pub(crate) fn cache_visibility(&mut self, entity: Entity) { - match &mut self.filter { + self.cached_visibility = self.get_visibility_state(entity); + } + + /// Returns visibility cached by the last call of [`Self::cache_visibility`]. + pub(crate) fn cached_visibility(&self) -> Visibility { + self.cached_visibility + } + + /// Returns visibility including + fn get_visibility_state(&self, entity: Entity) -> Visibility { + match &self.filter { VisibilityFilter::All { just_connected } => { if *just_connected { - self.cached_visibility = Visibility::Gained + Visibility::Gained } else { - self.cached_visibility = Visibility::Visible + Visibility::Visible } } VisibilityFilter::Blacklist { list, .. } => match list.get(&entity) { - Some(BlacklistInfo::QueuedForRemoval) => { - self.cached_visibility = Visibility::Gained - } - Some(BlacklistInfo::Hidden) => self.cached_visibility = Visibility::Hidden, - None => self.cached_visibility = Visibility::Visible, + Some(BlacklistInfo::QueuedForRemoval) => Visibility::Gained, + Some(BlacklistInfo::Hidden) => Visibility::Hidden, + None => Visibility::Visible, }, VisibilityFilter::Whitelist { list, .. } => match list.get(&entity) { - Some(WhitelistInfo::JustAdded) => self.cached_visibility = Visibility::Gained, - Some(WhitelistInfo::Visible) => self.cached_visibility = Visibility::Visible, - None => self.cached_visibility = Visibility::Hidden, + Some(WhitelistInfo::JustAdded) => Visibility::Gained, + Some(WhitelistInfo::Visible) => Visibility::Visible, + None => Visibility::Hidden, }, } } - - /// Returns visibility cached by the last call of [`Self::cache_visibility`]. - pub(crate) fn cached_visibility(&self) -> Visibility { - self.cached_visibility - } } /// Filter for [`ClientVisibility`] based on [`VisibilityPolicy`]. From f4403125008b28abb9e435e6a4b6badbe1808edd Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 13:53:43 +0200 Subject: [PATCH 37/48] Clear removed and added in update too to avoid confusion --- src/server/clients_info/client_visibility.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index e3bf99a2..284f9be3 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -76,20 +76,28 @@ impl ClientVisibility { pub(crate) fn update(&mut self) { match &mut self.filter { VisibilityFilter::All { just_connected } => *just_connected = false, - VisibilityFilter::Blacklist { list, removed, .. } => { + VisibilityFilter::Blacklist { + list, + added, + removed, + } => { // Remove all entities queued for removal. for entity in removed.drain() { list.remove(&entity); } - // `added` is drained in `Self::drain_lost_visibility`. + added.clear(); } - VisibilityFilter::Whitelist { list, added, .. } => { + VisibilityFilter::Whitelist { + list, + added, + removed, + } => { // Change all recently added entities to `WhitelistInfo::Visible` // from `WhitelistInfo::JustVisible`. for entity in added.drain() { list.insert(entity, WhitelistInfo::Visible); } - // `removed` is drained in `Self::drain_lost_visibility`. + removed.clear(); } } } From bda6ac7b9967ba497d62ca82335cf7e3b62b4bed Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 14:09:35 +0200 Subject: [PATCH 38/48] Add unit tests for many combinations and simplify integration tests --- src/server/clients_info/client_visibility.rs | 208 +++++++++++++++++++ tests/replication.rs | 53 +++-- 2 files changed, 234 insertions(+), 27 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 284f9be3..ae128edf 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -335,3 +335,211 @@ impl Iterator for VisibilityLostIter { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn all() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::All); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); + + visibility.set_visibility(Entity::PLACEHOLDER, true); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); + + visibility.set_visibility(Entity::PLACEHOLDER, false); + assert!( + visibility.is_visible(Entity::PLACEHOLDER), + "shouldn't have any effect for this policy" + ); + } + + #[test] + fn blacklist_insertion() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Blacklist); + visibility.set_visibility(Entity::PLACEHOLDER, false); + assert!(!visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Blacklist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(list.contains_key(&Entity::PLACEHOLDER)); + assert!(added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + + visibility.update(); + + let VisibilityFilter::Blacklist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } + + #[test] + fn blacklist_emtpy_removal() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Blacklist); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); + + visibility.set_visibility(Entity::PLACEHOLDER, true); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Blacklist { + list, + added, + removed, + } = visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(!list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } + + #[test] + fn blacklist_removal() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Blacklist); + visibility.set_visibility(Entity::PLACEHOLDER, false); + visibility.update(); + visibility.set_visibility(Entity::PLACEHOLDER, true); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Blacklist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(removed.contains(&Entity::PLACEHOLDER)); + + visibility.update(); + + let VisibilityFilter::Blacklist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(!list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } + + #[test] + fn whitelist_insertion() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); + visibility.set_visibility(Entity::PLACEHOLDER, true); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Whitelist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a whitelist"); + }; + + assert!(list.contains_key(&Entity::PLACEHOLDER)); + assert!(added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + + visibility.update(); + + let VisibilityFilter::Whitelist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } + + #[test] + fn whitelist_emtpy_removal() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); + assert!(!visibility.is_visible(Entity::PLACEHOLDER)); + + visibility.set_visibility(Entity::PLACEHOLDER, false); + assert!(!visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Whitelist { + list, + added, + removed, + } = visibility.filter + else { + panic!("filter should be a whitelist"); + }; + + assert!(!list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } + + #[test] + fn whitelist_removal() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); + visibility.set_visibility(Entity::PLACEHOLDER, true); + visibility.update(); + visibility.set_visibility(Entity::PLACEHOLDER, false); + assert!(!visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Whitelist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a whitelist"); + }; + + assert!(!list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(removed.contains(&Entity::PLACEHOLDER)); + + visibility.update(); + + let VisibilityFilter::Whitelist { + list, + added, + removed, + } = &visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(!list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } +} diff --git a/tests/replication.rs b/tests/replication.rs index df5af72b..c352ee6a 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -775,19 +775,32 @@ fn all_visibility() { let client_id = ClientId::from_raw(client_transport.client_id()); let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.client_mut(client_id).visibility_mut(); - visibility.set_visibility(server_entity, false); - assert!( - visibility.is_visible(server_entity), - "shouldn't have any effect with {:?}", - VisibilityPolicy::All - ); + visibility.set_visibility(server_entity, false); // Shouldn't have any effect for this policy. + + server_app.update(); + client_app.update(); + + client_app + .world + .query_filtered::<(), (With, With)>() + .single(&client_app.world); + // Reverse visibility back. + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, true); - assert!(visibility.is_visible(server_entity)); + + server_app.update(); + client_app.update(); + + client_app + .world + .query_filtered::<(), (With, With)>() + .single(&client_app.world); } #[test] -fn blacklist_visibility() { +fn empty_blacklist_visibility() { let mut server_app = App::new(); let mut client_app = App::new(); for app in [&mut server_app, &mut client_app] { @@ -804,13 +817,7 @@ fn blacklist_visibility() { connect::single_client(&mut server_app, &mut client_app); - let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); - - let client_transport = client_app.world.resource::(); - let client_id = ClientId::from_raw(client_transport.client_id()); - let clients_info = server_app.world.resource::(); - let visibility = clients_info.client(client_id).visibility(); - assert!(visibility.is_visible(server_entity)); + server_app.world.spawn((Replication, TableComponent)); server_app.update(); client_app.update(); @@ -822,7 +829,7 @@ fn blacklist_visibility() { } #[test] -fn blacklist_visibility_add_remove() { +fn blacklist_visibility() { let mut server_app = App::new(); let mut client_app = App::new(); for app in [&mut server_app, &mut client_app] { @@ -846,7 +853,6 @@ fn blacklist_visibility_add_remove() { let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, false); - assert!(!visibility.is_visible(server_entity)); server_app.update(); client_app.update(); @@ -868,7 +874,7 @@ fn blacklist_visibility_add_remove() { } #[test] -fn whitelist_visibility() { +fn empty_whitelist_visibility() { let mut server_app = App::new(); let mut client_app = App::new(); for app in [&mut server_app, &mut client_app] { @@ -885,13 +891,7 @@ fn whitelist_visibility() { connect::single_client(&mut server_app, &mut client_app); - let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); - - let client_transport = client_app.world.resource::(); - let client_id = ClientId::from_raw(client_transport.client_id()); - let clients_info = server_app.world.resource::(); - let visibility = clients_info.client(client_id).visibility(); - assert!(!visibility.is_visible(server_entity)); + server_app.world.spawn((Replication, TableComponent)); server_app.update(); client_app.update(); @@ -903,7 +903,7 @@ fn whitelist_visibility() { } #[test] -fn whitelist_visibility_add_remove() { +fn whitelist_visibility() { let mut server_app = App::new(); let mut client_app = App::new(); for app in [&mut server_app, &mut client_app] { @@ -927,7 +927,6 @@ fn whitelist_visibility_add_remove() { let mut clients_info = server_app.world.resource_mut::(); let visibility = clients_info.client_mut(client_id).visibility_mut(); visibility.set_visibility(server_entity, true); - assert!(visibility.is_visible(server_entity)); server_app.update(); client_app.update(); From a991a20a09ceed8bd45d31435571dc20acbca857 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 20:28:55 +0200 Subject: [PATCH 39/48] Fix removal and insertion on the same tick --- src/server/clients_info/client_visibility.rs | 73 ++++++++++++++++---- 1 file changed, 61 insertions(+), 12 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index ae128edf..2dc47adb 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -1,6 +1,6 @@ use bevy::{ prelude::*, - utils::{EntityHashMap, EntityHashSet}, + utils::{hashbrown::hash_map::Entry, EntityHashMap, EntityHashSet}, }; use super::VisibilityPolicy; @@ -164,14 +164,18 @@ impl ClientVisibility { removed, } => { if visibile { - // For blacklist we don't remove the entity right away. - // Instead we mark it as queued for removal and remove it - // later in `Self::update`. This allows us to avoid accessing - // the blacklist's `removed` field in `Self::cache_visibility`. - if let Some(info) = list.get_mut(&entity) { - *info = BlacklistInfo::QueuedForRemoval; - removed.insert(entity); - added.remove(&entity); + if let Entry::Occupied(mut entry) = list.entry(entity) { + if !added.remove(&entity) { + removed.insert(entity); + // For blacklist we don't remove the entity right away. + // Instead we mark it as queued for removal and remove it + // later in `Self::update`. This allows us to avoid accessing + // the blacklist's `removed` field in `Self::cache_visibility`. + entry.insert(BlacklistInfo::QueuedForRemoval); + } else { + // If the entity was previously added in this tick, then do not consider it removed. + entry.remove(); + } } } else { if *list.entry(entity).or_insert(BlacklistInfo::Hidden) == BlacklistInfo::Hidden @@ -189,7 +193,8 @@ impl ClientVisibility { } => { if visibile { // Similar to blacklist removal, we don't just add the entity to the list. - // Instead we mark it as 'just added' and then set it to 'visible' in `Self::update`. + // Instead we mark it as `WhitelistInfo::JustAdded` and then set it to + // 'WhitelistInfo::Visible' in `Self::update`. // This allows us to avoid accessing the whitelist's `added` field in // `Self::cache_visibility`. if *list.entry(entity).or_insert(WhitelistInfo::JustAdded) @@ -200,8 +205,10 @@ impl ClientVisibility { } removed.remove(&entity); } else if list.remove(&entity).is_some() { - removed.insert(entity); - added.remove(&entity); + if !added.remove(&entity) { + // If the entity wasn't previously added in this tick, then consider it removed. + removed.insert(entity); + } } } } @@ -449,6 +456,27 @@ mod tests { assert!(!removed.contains(&Entity::PLACEHOLDER)); } + #[test] + fn blacklist_insertion_removal() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Blacklist); + visibility.set_visibility(Entity::PLACEHOLDER, false); + visibility.set_visibility(Entity::PLACEHOLDER, true); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Blacklist { + list, + added, + removed, + } = visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(!list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } + #[test] fn whitelist_insertion() { let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); @@ -542,4 +570,25 @@ mod tests { assert!(!added.contains(&Entity::PLACEHOLDER)); assert!(!removed.contains(&Entity::PLACEHOLDER)); } + + #[test] + fn whitelist_insertion_removal() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); + visibility.set_visibility(Entity::PLACEHOLDER, true); + visibility.set_visibility(Entity::PLACEHOLDER, false); + assert!(!visibility.is_visible(Entity::PLACEHOLDER)); + + let VisibilityFilter::Whitelist { + list, + added, + removed, + } = visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(!list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } } From 7696b539d8822aff18ab3231c9efd8b3e8214b8e Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 20:31:48 +0200 Subject: [PATCH 40/48] Add more comments --- src/server/clients_info/client_visibility.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index 2dc47adb..b3e27faf 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -459,6 +459,8 @@ mod tests { #[test] fn blacklist_insertion_removal() { let mut visibility = ClientVisibility::new(VisibilityPolicy::Blacklist); + + // Insert and remove from the list. visibility.set_visibility(Entity::PLACEHOLDER, false); visibility.set_visibility(Entity::PLACEHOLDER, true); assert!(visibility.is_visible(Entity::PLACEHOLDER)); @@ -574,6 +576,8 @@ mod tests { #[test] fn whitelist_insertion_removal() { let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); + + // Insert and remove from the list. visibility.set_visibility(Entity::PLACEHOLDER, true); visibility.set_visibility(Entity::PLACEHOLDER, false); assert!(!visibility.is_visible(Entity::PLACEHOLDER)); From 6b70f9cc30b8541fd43bfcd499dfc32275a4883c Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 21:05:49 +0200 Subject: [PATCH 41/48] Apply clippy suggestion --- src/server/clients_info/client_visibility.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index b3e27faf..ea45c0b5 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -204,11 +204,9 @@ impl ClientVisibility { added.insert(entity); } removed.remove(&entity); - } else if list.remove(&entity).is_some() { - if !added.remove(&entity) { - // If the entity wasn't previously added in this tick, then consider it removed. - removed.insert(entity); - } + } else if list.remove(&entity).is_some() && !added.remove(&entity) { + // If the entity wasn't previously added in this tick, then consider it removed. + removed.insert(entity); } } } From 2f9eefb9e1ba153c136499f83771021c0cf734f9 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 21:21:20 +0200 Subject: [PATCH 42/48] Add despawn tests --- tests/replication.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/replication.rs b/tests/replication.rs index c352ee6a..6b830558 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -873,6 +873,43 @@ fn blacklist_visibility() { .single(&client_app.world); } +#[test] +fn blacklist_despawn_visibility() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + visibility_policy: VisibilityPolicy::Blacklist, + ..Default::default() + }), + )) + .replicate::(); + } + + connect::single_client(&mut server_app, &mut client_app); + + let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + + let client_transport = client_app.world.resource::(); + let client_id = ClientId::from_raw(client_transport.client_id()); + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); + visibility.set_visibility(server_entity, false); + server_app.world.despawn(server_entity); + + server_app.update(); + client_app.update(); + + assert!(client_app.world.entities().is_empty()); + + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); + assert!(visibility.is_visible(server_entity)); // The missing entity must be removed from the list, so this should return `true`. +} + #[test] fn empty_whitelist_visibility() { let mut server_app = App::new(); From 042097d9f1968ee00df12b0a989e72bcdac846d9 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 23:33:20 +0200 Subject: [PATCH 43/48] Apply suggestions from code review Co-authored-by: UkoeHB <37489173+UkoeHB@users.noreply.github.com> --- src/server/clients_info/client_visibility.rs | 55 ++++++++++++-------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index ea45c0b5..df5283b6 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -164,19 +164,23 @@ impl ClientVisibility { removed, } => { if visibile { - if let Entry::Occupied(mut entry) = list.entry(entity) { - if !added.remove(&entity) { - removed.insert(entity); - // For blacklist we don't remove the entity right away. - // Instead we mark it as queued for removal and remove it - // later in `Self::update`. This allows us to avoid accessing - // the blacklist's `removed` field in `Self::cache_visibility`. - entry.insert(BlacklistInfo::QueuedForRemoval); - } else { - // If the entity was previously added in this tick, then do not consider it removed. - entry.remove(); - } + // If the entity is already visibile, do nothing. + let Entry::Occupied(mut entry) = list.entry(entity) else { + return; + }; + + // If the entity was previously added in this tick, then undo it. + if added.remove(&entity) { + entry.remove(); + return; } + + // For blacklisting an entity we don't remove the entity right away. + // Instead we mark it as queued for removal and remove it + // later in `Self::update`. This allows us to avoid accessing + // the blacklist's `removed` field in `Self::get_visibility_state`. + entry.insert(BlacklistInfo::QueuedForRemoval); + removed.insert(entity); } else { if *list.entry(entity).or_insert(BlacklistInfo::Hidden) == BlacklistInfo::Hidden { @@ -196,7 +200,7 @@ impl ClientVisibility { // Instead we mark it as `WhitelistInfo::JustAdded` and then set it to // 'WhitelistInfo::Visible' in `Self::update`. // This allows us to avoid accessing the whitelist's `added` field in - // `Self::cache_visibility`. + // `Self::get_visibility_state`. if *list.entry(entity).or_insert(WhitelistInfo::JustAdded) == WhitelistInfo::JustAdded { @@ -204,15 +208,24 @@ impl ClientVisibility { added.insert(entity); } removed.remove(&entity); - } else if list.remove(&entity).is_some() && !added.remove(&entity) { - // If the entity wasn't previously added in this tick, then consider it removed. + } else { + // If the entity is not in the whitelist, do nothing. + if list.remove(&entity).is_none() { + return; + } + + // If the entity was added in this tick, then undo it. + if added.remove(&entity) { + return; + } + removed.insert(entity); } } } } - /// Gets visibility for a specific entity. + /// Checks if a specific entity is visible. pub fn is_visible(&self, entity: Entity) -> bool { match self.get_visibility_state(entity) { Visibility::Hidden => false, @@ -232,7 +245,7 @@ impl ClientVisibility { self.cached_visibility } - /// Returns visibility including + /// Returns visibility of a specific entity. fn get_visibility_state(&self, entity: Entity) -> Visibility { match &self.filter { VisibilityFilter::All { just_connected } => { @@ -284,7 +297,7 @@ enum VisibilityFilter { /// this tick. list: EntityHashMap, - /// All entities that were added to the list in tVisibility of these entities has been gained.his tick. + /// All entities that were added to the list in this tick. /// /// Visibility of these entities has been gained. added: EntityHashSet, @@ -313,7 +326,7 @@ enum BlacklistInfo { /// Note that the distinction between 'lost visibility' and 'don't have visibility' is not exposed here. /// There is only `Visibility::Hidden` to encompass both variants. /// -/// Lost visibility is handled separately with [`ClientVisibility::iter_lost_visibility`]. +/// Lost visibility is handled separately with [`ClientVisibility::drain_lost_visibility`]. #[derive(PartialEq, Default, Clone, Copy)] pub(crate) enum Visibility { /// The client does not have visibility of the entity in this tick. @@ -396,7 +409,7 @@ mod tests { } #[test] - fn blacklist_emtpy_removal() { + fn blacklist_empty_removal() { let mut visibility = ClientVisibility::new(VisibilityPolicy::Blacklist); assert!(visibility.is_visible(Entity::PLACEHOLDER)); @@ -513,7 +526,7 @@ mod tests { } #[test] - fn whitelist_emtpy_removal() { + fn whitelist_empty_removal() { let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); assert!(!visibility.is_visible(Entity::PLACEHOLDER)); From e35ed31ef5ca857d14fcc298b8ed882cadd5e879 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 23:49:46 +0200 Subject: [PATCH 44/48] Fix duplicate insertion for blacklist --- src/server/clients_info/client_visibility.rs | 59 ++++++++++++++++++-- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index df5283b6..b2de2614 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -182,12 +182,13 @@ impl ClientVisibility { entry.insert(BlacklistInfo::QueuedForRemoval); removed.insert(entity); } else { - if *list.entry(entity).or_insert(BlacklistInfo::Hidden) == BlacklistInfo::Hidden - { - // Do not mark an entry as newly added if the entry was already in the list. - added.insert(entity); - } - removed.remove(&entity); + // If the entity is already registered, reset its removal status. + if list.insert(entity, BlacklistInfo::Hidden).is_some() { + removed.remove(&entity); + return; + }; + + added.insert(entity); } } VisibilityFilter::Whitelist { @@ -490,6 +491,29 @@ mod tests { assert!(!removed.contains(&Entity::PLACEHOLDER)); } + #[test] + fn blacklist_duplicate_insertion() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Blacklist); + visibility.set_visibility(Entity::PLACEHOLDER, false); + visibility.update(); + + // Duplicate insertion. + visibility.set_visibility(Entity::PLACEHOLDER, false); + + let VisibilityFilter::Blacklist { + list, + added, + removed, + } = visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } + #[test] fn whitelist_insertion() { let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); @@ -606,4 +630,27 @@ mod tests { assert!(!added.contains(&Entity::PLACEHOLDER)); assert!(!removed.contains(&Entity::PLACEHOLDER)); } + + #[test] + fn whitelist_duplicate_insertion() { + let mut visibility = ClientVisibility::new(VisibilityPolicy::Whitelist); + visibility.set_visibility(Entity::PLACEHOLDER, true); + visibility.update(); + + // Duplicate insertion. + visibility.set_visibility(Entity::PLACEHOLDER, true); + + let VisibilityFilter::Whitelist { + list, + added, + removed, + } = visibility.filter + else { + panic!("filter should be a blacklist"); + }; + + assert!(list.contains_key(&Entity::PLACEHOLDER)); + assert!(!added.contains(&Entity::PLACEHOLDER)); + assert!(!removed.contains(&Entity::PLACEHOLDER)); + } } From c0b2d8af2167141d7f6b35e3053a970c8030263a Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 23:54:43 +0200 Subject: [PATCH 45/48] Add whitelist despawn test --- tests/replication.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/replication.rs b/tests/replication.rs index 6b830558..74fae763 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -987,6 +987,43 @@ fn whitelist_visibility() { ); } +#[test] +fn whitelist_despawn_visibility() { + let mut server_app = App::new(); + let mut client_app = App::new(); + for app in [&mut server_app, &mut client_app] { + app.add_plugins(( + MinimalPlugins, + ReplicationPlugins.set(ServerPlugin { + tick_policy: TickPolicy::EveryFrame, + visibility_policy: VisibilityPolicy::Whitelist, + ..Default::default() + }), + )) + .replicate::(); + } + + connect::single_client(&mut server_app, &mut client_app); + + let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + + let client_transport = client_app.world.resource::(); + let client_id = ClientId::from_raw(client_transport.client_id()); + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); + visibility.set_visibility(server_entity, true); + server_app.world.despawn(server_entity); + + server_app.update(); + client_app.update(); + + assert!(client_app.world.entities().is_empty()); + + let mut clients_info = server_app.world.resource_mut::(); + let visibility = clients_info.client_mut(client_id).visibility_mut(); + assert!(!visibility.is_visible(server_entity)); +} + #[test] fn replication_into_scene() { let mut app = App::new(); From b3ed333cec9c312eb87ef331f545f7dda49ad053 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Mon, 15 Jan 2024 23:55:47 +0200 Subject: [PATCH 46/48] Simplify despawn tests --- tests/replication.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/replication.rs b/tests/replication.rs index 74fae763..ba720dfe 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -891,7 +891,7 @@ fn blacklist_despawn_visibility() { connect::single_client(&mut server_app, &mut client_app); - let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + let server_entity = server_app.world.spawn(Replication).id(); let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); @@ -1005,7 +1005,7 @@ fn whitelist_despawn_visibility() { connect::single_client(&mut server_app, &mut client_app); - let server_entity = server_app.world.spawn((Replication, TableComponent)).id(); + let server_entity = server_app.world.spawn(Replication).id(); let client_transport = client_app.world.resource::(); let client_id = ClientId::from_raw(client_transport.client_id()); From 60213468ac7c982a7ff0c7606a7112af743a69ca Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Tue, 16 Jan 2024 00:17:23 +0200 Subject: [PATCH 47/48] Add two more asserts --- src/server/clients_info/client_visibility.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/server/clients_info/client_visibility.rs b/src/server/clients_info/client_visibility.rs index b2de2614..ebd6bd5f 100644 --- a/src/server/clients_info/client_visibility.rs +++ b/src/server/clients_info/client_visibility.rs @@ -499,6 +499,7 @@ mod tests { // Duplicate insertion. visibility.set_visibility(Entity::PLACEHOLDER, false); + assert!(!visibility.is_visible(Entity::PLACEHOLDER)); let VisibilityFilter::Blacklist { list, @@ -639,6 +640,7 @@ mod tests { // Duplicate insertion. visibility.set_visibility(Entity::PLACEHOLDER, true); + assert!(visibility.is_visible(Entity::PLACEHOLDER)); let VisibilityFilter::Whitelist { list, From 6c87d2b9224ab68fc2c0f9a513c1e19bd8ed1e56 Mon Sep 17 00:00:00 2001 From: Hennadii Chernyshchyk Date: Tue, 16 Jan 2024 00:28:44 +0200 Subject: [PATCH 48/48] Use non-mut method --- tests/replication.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/replication.rs b/tests/replication.rs index ba720dfe..d6a2b257 100644 --- a/tests/replication.rs +++ b/tests/replication.rs @@ -905,8 +905,8 @@ fn blacklist_despawn_visibility() { assert!(client_app.world.entities().is_empty()); - let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.client_mut(client_id).visibility_mut(); + let clients_info = server_app.world.resource::(); + let visibility = clients_info.client(client_id).visibility(); assert!(visibility.is_visible(server_entity)); // The missing entity must be removed from the list, so this should return `true`. } @@ -1019,8 +1019,8 @@ fn whitelist_despawn_visibility() { assert!(client_app.world.entities().is_empty()); - let mut clients_info = server_app.world.resource_mut::(); - let visibility = clients_info.client_mut(client_id).visibility_mut(); + let clients_info = server_app.world.resource::(); + let visibility = clients_info.client(client_id).visibility(); assert!(!visibility.is_visible(server_entity)); }