From a01f75b38d4a88120dceff1b5debf9067dd9dc09 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sat, 3 Jun 2023 22:23:42 -0400 Subject: [PATCH 1/7] Add `sync_me` parameter to `Presence` that must be set to `true` for entities with the `Presence` component to be synced to other clients. --- common/src/comp/presence.rs | 9 ++++++++- common/src/region.rs | 36 +++++++++++++++++++++++++++--------- common/state/src/state.rs | 1 + server/src/events/player.rs | 1 + server/src/state_ext.rs | 10 ++++++++-- 5 files changed, 45 insertions(+), 12 deletions(-) diff --git a/common/src/comp/presence.rs b/common/src/comp/presence.rs index 7148d220a7..d08cba3cf6 100644 --- a/common/src/comp/presence.rs +++ b/common/src/comp/presence.rs @@ -13,16 +13,23 @@ pub struct Presence { /// updated! pub kind: PresenceKind, pub lossy_terrain_compression: bool, + /// Controls whether this entity is synced to other clients. + /// + /// Note, if it ends up being useful this could be generalized to an + /// independent component that is required for any entity to be synced + /// (as an independent component it could use NullStorage). + pub sync_me: bool, } impl Presence { - pub fn new(view_distances: ViewDistances, kind: PresenceKind) -> Self { + pub fn new(view_distances: ViewDistances, kind: PresenceKind, sync_me: bool) -> Self { let now = Instant::now(); Self { terrain_view_distance: ViewDistance::new(view_distances.terrain, now), entity_view_distance: ViewDistance::new(view_distances.entity, now), kind, lossy_terrain_compression: false, + sync_me, } } } diff --git a/common/src/region.rs b/common/src/region.rs index d01b026268..cd34173684 100644 --- a/common/src/region.rs +++ b/common/src/region.rs @@ -1,4 +1,4 @@ -use crate::comp::{Pos, Vel}; +use crate::comp::{Pos, Presence, Vel}; use common_base::span; use hashbrown::{hash_map::DefaultHashBuilder, HashSet}; use indexmap::IndexMap; @@ -69,7 +69,12 @@ const NEIGHBOR_OFFSETS: [Vec2; 8] = [ #[derive(Default)] // TODO generic region size (16x16 for now) // TODO compare to sweep and prune approach -/// A region system that tracks where entities are +/// A region system that tracks where entities are. +/// +/// Note, this structure is primarily intended for tracking which entities need +/// to be synchronized to which clients (and as part of that what entities are +/// already synchronized). If an entity is marked to not be synchronized to +/// other clients it may not appear here. pub struct RegionMap { // Tree? // Sorted Vec? (binary search lookup) @@ -92,7 +97,13 @@ impl RegionMap { // TODO maintain within a system? // TODO special case large entities - pub fn tick(&mut self, pos: ReadStorage, vel: ReadStorage, entities: Entities) { + pub fn tick( + &mut self, + pos: ReadStorage, + vel: ReadStorage, + presence: ReadStorage, + entities: Entities, + ) { span!(_guard, "tick", "Region::tick"); self.tick += 1; // Clear events within each region @@ -101,9 +112,10 @@ impl RegionMap { }); // Add any untracked entities - for (pos, id) in (&pos, &entities, !&self.tracked_entities) + for (pos, id) in (&pos, &entities, presence.maybe(), !&self.tracked_entities) .join() - .map(|(pos, e, _)| (pos, e.id())) + .filter(|(_, _, presence, _)| presence.map_or(true, |p| p.sync_me)) + .map(|(pos, e, _, _)| (pos, e.id())) .collect::>() { // Add entity @@ -123,15 +135,21 @@ impl RegionMap { .iter() .enumerate() .for_each(|(i, (¤t_region, region_data))| { - for (maybe_pos, _maybe_vel, id) in - (pos.maybe(), vel.maybe(), ®ion_data.bitset).join() + for (maybe_pos, _maybe_vel, maybe_presence, id) in ( + pos.maybe(), + vel.maybe(), + presence.maybe(), + ®ion_data.bitset, + ) + .join() { + let should_sync = maybe_presence.map_or(true, |p| p.sync_me); match maybe_pos { // Switch regions for entities which need switching // TODO don't check every tick (use velocity) (and use id to stagger) // Starting parameters at v = 0 check every 100 ticks // tether_length^2 / vel^2 (with a max of every tick) - Some(pos) => { + Some(pos) if should_sync => { let pos = pos.0.map(|e| e as i32); let key = Self::pos_key(pos); // Consider switching @@ -148,7 +166,7 @@ impl RegionMap { }, // Remove any non-existant entities (or just ones that lost their position // component) TODO: distribute this between ticks - None => { + None | Some(_) => { // TODO: shouldn't there be a way to extract the bitset of entities with // positions directly from specs? Yes, with `.mask()` on the component // storage. diff --git a/common/state/src/state.rs b/common/state/src/state.rs index 7771d54f00..6c1ee98404 100644 --- a/common/state/src/state.rs +++ b/common/state/src/state.rs @@ -534,6 +534,7 @@ impl State { self.ecs.write_resource::().tick( self.ecs.read_storage::(), self.ecs.read_storage::(), + self.ecs.read_storage::(), self.ecs.entities(), ); } diff --git a/server/src/events/player.rs b/server/src/events/player.rs index dff4d34704..f329c3bada 100644 --- a/server/src/events/player.rs +++ b/server/src/events/player.rs @@ -418,6 +418,7 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui // from overwriting original character info with stuff from the new character. kind: PresenceKind::Possessor, lossy_terrain_compression: presence.lossy_terrain_compression, + sync_me: presence.sync_me, }) } else { None diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index ae77bd5d53..eedded76f5 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -604,6 +604,7 @@ impl StateExt for State { entity: view_distance, }, PresenceKind::Spectator, + false, )) } @@ -644,7 +645,11 @@ impl StateExt for State { self.write_component_ignore_entity_dead( entity, - Presence::new(view_distances, PresenceKind::LoadingCharacter(character_id)), + Presence::new( + view_distances, + PresenceKind::LoadingCharacter(character_id), + false, + ), ); // Tell the client its request was successful. @@ -669,7 +674,7 @@ impl StateExt for State { self.write_component_ignore_entity_dead( entity, - Presence::new(view_distances, PresenceKind::Spectator), + Presence::new(view_distances, PresenceKind::Spectator, false), ); // Tell the client its request was successful. @@ -704,6 +709,7 @@ impl StateExt for State { self.ecs() .write_resource::() .add_character(id, entity); + presence.sync_me = true; Ok(()) } else { Err("PresenceKind is not LoadingCharacter") From ba7d7481d7c4ba8b7839123209791db891208434 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sun, 4 Jun 2023 12:32:09 -0400 Subject: [PATCH 2/7] Move RegionMap insertion into ecs and ticking from `common` into `server` Since it now needs Presence component to be available and we don't use the RegionMap on the client. --- common/state/src/state.rs | 28 +++++----------------------- server/src/lib.rs | 18 +++++++++++++++++- server/src/sys/entity_sync.rs | 8 ++++++++ 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/common/state/src/state.rs b/common/state/src/state.rs index 6c1ee98404..01a1e23b31 100644 --- a/common/state/src/state.rs +++ b/common/state/src/state.rs @@ -12,7 +12,6 @@ use common::{ link::Is, mounting::{Mount, Rider, VolumeRider, VolumeRiders}, outcome::Outcome, - region::RegionMap, resources::{ DeltaTime, EntitiesDiedLastTick, GameMode, PlayerEntity, PlayerPhysicsSettings, Time, TimeOfDay, TimeScale, @@ -294,7 +293,6 @@ impl State { // TODO: only register on the server ecs.insert(EventBus::::default()); ecs.insert(comp::group::GroupManager::default()); - ecs.insert(RegionMap::new()); ecs.insert(SysMetrics::default()); ecs.insert(PhysicsMetrics::default()); ecs.insert(Trades::default()); @@ -528,17 +526,6 @@ impl State { } } - // Run RegionMap tick to update entity region occupancy - pub fn update_region_map(&self) { - span!(_guard, "update_region_map", "State::update_region_map"); - self.ecs.write_resource::().tick( - self.ecs.read_storage::(), - self.ecs.read_storage::(), - self.ecs.read_storage::(), - self.ecs.entities(), - ); - } - // Apply terrain changes pub fn apply_terrain_changes(&self, block_update: impl FnMut(&specs::World, Vec)) { self.apply_terrain_changes_internal(false, block_update); @@ -548,10 +535,9 @@ impl State { /// [State::tick]. /// /// This only happens if [State::tick] is asked to update terrain itself - /// (using `update_terrain_and_regions: true`). [State::tick] is called - /// from within both the client and the server ticks, right after - /// handling terrain messages; currently, client sets it to true and - /// server to false. + /// (using `update_terrain: true`). [State::tick] is called from within + /// both the client and the server ticks, right after handling terrain + /// messages; currently, client sets it to true and server to false. fn apply_terrain_changes_internal( &self, during_tick: bool, @@ -629,7 +615,7 @@ impl State { &mut self, dt: Duration, add_systems: impl Fn(&mut DispatcherBuilder), - update_terrain_and_regions: bool, + update_terrain: bool, mut metrics: Option<&mut StateTickMetrics>, server_constants: &ServerConstants, block_update: impl FnMut(&specs::World, Vec), @@ -657,10 +643,6 @@ impl State { self.ecs.write_resource::().0 = (dt.as_secs_f32() * time_scale as f32).min(MAX_DELTA_TIME); - if update_terrain_and_regions { - self.update_region_map(); - } - section_span!(guard, "create dispatcher"); // Run systems to update the world. // Create and run a dispatcher for ecs systems. @@ -680,7 +662,7 @@ impl State { self.ecs.maintain(); drop(guard); - if update_terrain_and_regions { + if update_terrain { self.apply_terrain_changes_internal(true, block_update); } diff --git a/server/src/lib.rs b/server/src/lib.rs index 1392abcfe3..4b19c3af17 100644 --- a/server/src/lib.rs +++ b/server/src/lib.rs @@ -76,6 +76,7 @@ use common::{ cmd::ServerChatCommand, comp, event::{EventBus, ServerEvent}, + region::RegionMap, resources::{BattleMode, GameMode, Time, TimeOfDay}, rtsim::{RtSimEntity, RtSimVehicle}, shared_server_config::ServerConstants, @@ -377,6 +378,9 @@ impl Server { .ecs_mut() .insert(sys::PersistenceScheduler::every(Duration::from_secs(10))); + // Region map (spatial structure for entity synchronization) + state.ecs_mut().insert(RegionMap::new()); + // Server-only components state.ecs_mut().register::(); state.ecs_mut().register::(); @@ -754,7 +758,7 @@ impl Server { // events so that changes made by server events will be immediately // visible to client synchronization systems, minimizing the latency of // `ServerEvent` mediated effects - self.state.update_region_map(); + self.update_region_map(); // NOTE: apply_terrain_changes sends the *new* value since it is not being // synchronized during the tick. self.state.apply_terrain_changes(on_block_update); @@ -1092,6 +1096,18 @@ impl Server { .map(|mut t| t.maintain()); } + // Run RegionMap tick to update entity region occupancy + fn update_region_map(&mut self) { + prof_span!("Server::update_region_map"); + let ecs = self.state().ecs(); + ecs.write_resource::().tick( + ecs.read_storage::(), + ecs.read_storage::(), + ecs.read_storage::(), + ecs.entities(), + ); + } + fn initialize_client(&mut self, client: connection_handler::IncomingClient) -> Entity { let entity = self .state diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index 2d4458d01a..7d5feff755 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -372,6 +372,14 @@ impl<'a> System<'a> for Sys { )); } + // TODO: this seems like the ideal spot to sync other components for clients + // that don't have a position or otherwise aren't included in the regions that + // the client is subscribed to... + // + // Maybe we can pass a bool into `create_sync_from_client_package`... renamed it + // to create_sync_package_for_client_entity(?) + // create_client_sync_package(?) + // // Sync components that are only synced for the client's own entity. for (entity, client) in (&entities, &clients).join() { let comp_sync_package = From 997babca181732b3de065c6eafb08a4b4c3f8289 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sun, 4 Jun 2023 14:21:42 -0400 Subject: [PATCH 3/7] Always sync components for a client's entity even if it has no position or has sync_me false in `Presence`. --- common/src/region.rs | 22 ++++++++++++++++++++++ server/src/sys/entity_sync.rs | 25 ++++++++++++------------- server/src/sys/sentinel.rs | 12 ++++++++---- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/common/src/region.rs b/common/src/region.rs index cd34173684..dda545ab3a 100644 --- a/common/src/region.rs +++ b/common/src/region.rs @@ -269,6 +269,28 @@ impl RegionMap { None } + /// Checks if this entity is located in the `RegionMap` using the provided + /// position to limit which regions are checked. + /// + /// May produce false negatives (e.g. if for some reason the entity is not + /// in a region near the provided position). + pub fn in_region_map_relaxed(&self, entity: specs::Entity, pos: Vec3) -> bool { + let id = entity.id(); + let pos = pos.map(|e| e as i32); + + // Compute key for most likely region + let key = Self::pos_key(pos); + if let Some(region) = self.regions.get(&key) && region.entities().contains(id) { return true } + + // Get the base of the four nearest regions. + let quad_base = Self::pos_key(pos - (REGION_SIZE / 2) as i32); + [(0, 0), (0, 1), (1, 0), (1, 1)] + .iter() + .map(|&offset| Vec2::::from(offset) + quad_base) + // skip key we already checked + .any(|k| k != key && self.regions.get(&key).is_some_and(|r| r.entities().contains(id))) + } + fn key_index(&self, key: Vec2) -> Option { self.regions.get_full(&key).map(|(i, _, _)| i) } diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index 7d5feff755..c87df21e16 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -362,8 +362,6 @@ impl<'a> System<'a> for Sys { } } - // TODO: Sync clients that don't have a position? - // Sync inventories for (inventory, update, client) in (inventories, &mut inventory_updates, &clients).join() { client.send_fallible(ServerGeneral::InventoryUpdate( @@ -372,18 +370,19 @@ impl<'a> System<'a> for Sys { )); } - // TODO: this seems like the ideal spot to sync other components for clients - // that don't have a position or otherwise aren't included in the regions that - // the client is subscribed to... - // - // Maybe we can pass a bool into `create_sync_from_client_package`... renamed it - // to create_sync_package_for_client_entity(?) - // create_client_sync_package(?) - // // Sync components that are only synced for the client's own entity. - for (entity, client) in (&entities, &clients).join() { - let comp_sync_package = - trackers.create_sync_from_client_package(&tracked_storages, entity); + for (entity, client, maybe_pos) in (&entities, &clients, positions.maybe()).join() { + // Include additional components for clients that aren't in a region (e.g. due + // to having no position or have sync_me as `false`) since those + // won't be synced above. + let include_all_comps = + !maybe_pos.is_some_and(|pos| region_map.in_region_map_relaxed(entity, pos.0)); + + let comp_sync_package = trackers.create_sync_from_client_package( + &tracked_storages, + entity, + include_all_comps, + ); if !comp_sync_package.is_empty() { client.send_fallible(ServerGeneral::CompSync( comp_sync_package, diff --git a/server/src/sys/sentinel.rs b/server/src/sys/sentinel.rs index 2f699d8810..ecc844fff6 100644 --- a/server/src/sys/sentinel.rs +++ b/server/src/sys/sentinel.rs @@ -231,10 +231,14 @@ macro_rules! trackers { /// Create sync package for components that are only synced for the client's entity. + /// + /// This can optionally include components that are synced "for any entity" for cases + /// where other mechanisms don't sync those components. pub fn create_sync_from_client_package( &self, comps: &TrackedStorages, entity: specs::Entity, + include_all_comps: bool, ) -> CompSyncPackage { // TODO: this type repeats the entity uid for each component but // we know they will all be the same here, using it for now for @@ -249,10 +253,10 @@ macro_rules! trackers { }; $( - if matches!( - <$component_type as NetSync>::SYNC_FROM, - SyncFrom::ClientEntity, - ) { + if match <$component_type as NetSync>::SYNC_FROM { + SyncFrom::ClientEntity => true, + SyncFrom::AnyEntity => include_all_comps, + } { comp_sync_package.add_component_update( &self.$component_name, &comps.$component_name, From 396c08e7ee7818548637ab2d6a0eeb69987667b1 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sun, 4 Jun 2023 17:16:06 -0400 Subject: [PATCH 4/7] Also make sure physics components are synced --- server/src/sys/entity_sync.rs | 129 ++++++++++++++++++++++------------ 1 file changed, 84 insertions(+), 45 deletions(-) diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index c87df21e16..e53c5d9aae 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -8,6 +8,7 @@ use common::{ region::{Event as RegionEvent, RegionMap}, resources::{PlayerPhysicsSettings, Time, TimeOfDay, TimeScale}, terrain::TerrainChunkSize, + uid::Uid, vol::RectVolSize, }; use common_ecs::{Job, Origin, Phase, System}; @@ -233,14 +234,13 @@ impl<'a> System<'a> for Sys { for (client, _, client_entity, client_pos) in &mut subscribers { let mut comp_sync_package = CompSyncPackage::new(); - for (_, entity, &uid, (&pos, last_pos), vel, ori, force_update, collider) in ( + for (_, entity, &uid, (&pos, last_pos), vel, ori, collider) in ( region.entities(), &entities, uids, (&positions, last_pos.mask().maybe()), (&velocities, last_vel.mask().maybe()).maybe(), (&orientations, last_vel.mask().maybe()).maybe(), - force_updates.maybe(), colliders.maybe(), ) .join() @@ -256,7 +256,7 @@ impl<'a> System<'a> for Sys { // Don't send client physics updates about itself unless force update is // set or the client is subject to // server-authoritative physics - force_update.map_or(false, |f| f.is_forced()) + force_updates.get(entity).map_or(false, |f| f.is_forced()) || player_physics_setting.server_authoritative() || is_rider.get(entity).is_some() } else if matches!(collider, Some(Collider::Voxel { .. })) { @@ -288,27 +288,15 @@ impl<'a> System<'a> for Sys { } }; - if last_pos.is_none() { - comp_sync_package.comp_inserted(uid, pos); - } else if send_now { - comp_sync_package.comp_modified(uid, pos); - } - - if let Some((v, last_vel)) = vel { - if last_vel.is_none() { - comp_sync_package.comp_inserted(uid, *v); - } else if send_now { - comp_sync_package.comp_modified(uid, *v); - } - } - - if let Some((o, last_ori)) = ori { - if last_ori.is_none() { - comp_sync_package.comp_inserted(uid, *o); - } else if send_now { - comp_sync_package.comp_modified(uid, *o); - } - } + add_physics_components( + send_now, + &mut comp_sync_package, + uid, + pos, + last_pos, + ori, + vel, + ); } // TODO: force update counter only needs to be sent once per frame (and only if @@ -326,6 +314,41 @@ impl<'a> System<'a> for Sys { drop(guard); job.cpu_stats.measure(common_ecs::ParMode::Single); + // Sync components that are only synced for the client's own entity. + for (entity, client, &uid, (maybe_pos, last_pos), vel, ori) in ( + &entities, + &clients, + uids, + (positions.maybe(), last_pos.mask().maybe()), + (&velocities, last_vel.mask().maybe()).maybe(), + (&orientations, last_vel.mask().maybe()).maybe(), + ) + .join() + { + // Include additional components for clients that aren't in a region (e.g. due + // to having no position or have sync_me as `false`) since those + // won't be synced above. + let include_all_comps = + !maybe_pos.is_some_and(|pos| region_map.in_region_map_relaxed(entity, pos.0)); + + let mut comp_sync_package = trackers.create_sync_from_client_package( + &tracked_storages, + entity, + include_all_comps, + ); + + if include_all_comps && let Some(&pos) = maybe_pos { + add_physics_components(true, &mut comp_sync_package, uid, pos, last_pos, ori, vel); + } + + if !comp_sync_package.is_empty() { + client.send_fallible(ServerGeneral::CompSync( + comp_sync_package, + force_updates.get(entity).map_or(0, |f| f.counter()), + )); + } + } + // Update the last physics components for each entity for (_, &pos, vel, ori, last_pos, last_vel, last_ori) in ( &entities, @@ -370,27 +393,6 @@ impl<'a> System<'a> for Sys { )); } - // Sync components that are only synced for the client's own entity. - for (entity, client, maybe_pos) in (&entities, &clients, positions.maybe()).join() { - // Include additional components for clients that aren't in a region (e.g. due - // to having no position or have sync_me as `false`) since those - // won't be synced above. - let include_all_comps = - !maybe_pos.is_some_and(|pos| region_map.in_region_map_relaxed(entity, pos.0)); - - let comp_sync_package = trackers.create_sync_from_client_package( - &tracked_storages, - entity, - include_all_comps, - ); - if !comp_sync_package.is_empty() { - client.send_fallible(ServerGeneral::CompSync( - comp_sync_package, - force_updates.get(entity).map_or(0, |f| f.counter()), - )); - } - } - // Consume/clear the current outcomes and convert them to a vec let outcomes = outcomes.recv_all().collect::>(); @@ -445,3 +447,40 @@ impl<'a> System<'a> for Sys { } } } + +/// Adds physics components if `send_now` is true or `Option>` is +/// `None`. +/// +/// If `Last` isn't present, this is recorded as an insertion rather than a +/// modification. +fn add_physics_components( + send_now: bool, + comp_sync_package: &mut CompSyncPackage, + uid: Uid, + pos: Pos, + last_pos: Option, + ori: Option<(&Ori, Option)>, + vel: Option<(&Vel, Option)>, +) { + if last_pos.is_none() { + comp_sync_package.comp_inserted(uid, pos); + } else if send_now { + comp_sync_package.comp_modified(uid, pos); + } + + if let Some((v, last_vel)) = vel { + if last_vel.is_none() { + comp_sync_package.comp_inserted(uid, *v); + } else if send_now { + comp_sync_package.comp_modified(uid, *v); + } + } + + if let Some((o, last_ori)) = ori { + if last_ori.is_none() { + comp_sync_package.comp_inserted(uid, *o); + } else if send_now { + comp_sync_package.comp_modified(uid, *o); + } + } +} From cdca700297b3bcd6b1f386343b4529713aedf5d7 Mon Sep 17 00:00:00 2001 From: Imbris Date: Mon, 5 Jun 2023 18:23:07 -0400 Subject: [PATCH 5/7] Properly check conditions for when the client's physics components should be synced --- client/src/lib.rs | 7 ++++++ server/src/state_ext.rs | 2 +- server/src/sys/entity_sync.rs | 45 +++++++++++++++++++++++++---------- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index c19d73529b..5ccb65ff65 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -2257,10 +2257,17 @@ impl Client { .apply_entity_sync_package(entity_sync_package, uid); }, ServerGeneral::CompSync(comp_sync_package, force_counter) => { + // TODO: Test with and without sync_me = true commented out to see if results are + // as expected. (client shouldn't normally receive updates if client physics is + // enabled) + let b = self.position(); self.force_update_counter = force_counter; self.state .ecs_mut() .apply_comp_sync_package(comp_sync_package); + if b != self.position() { + println!("{b:?}"); + } }, ServerGeneral::CreateEntity(entity_package) => { self.state.ecs_mut().apply_entity_package(entity_package); diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index eedded76f5..30746a90c3 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -709,7 +709,7 @@ impl StateExt for State { self.ecs() .write_resource::() .add_character(id, entity); - presence.sync_me = true; + //presence.sync_me = true; Ok(()) } else { Err("PresenceKind is not LoadingCharacter") diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index e53c5d9aae..898731efd8 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -4,6 +4,8 @@ use common::{ calendar::Calendar, comp::{Collider, ForceUpdate, InventoryUpdate, Last, Ori, Player, Pos, Presence, Vel}, event::EventBus, + link::Is, + mounting::Rider, outcome::Outcome, region::{Event as RegionEvent, RegionMap}, resources::{PlayerPhysicsSettings, Time, TimeOfDay, TimeScale}, @@ -247,18 +249,13 @@ impl<'a> System<'a> for Sys { { // Decide how regularly to send physics updates. let send_now = if client_entity == &entity { - let player_physics_setting = players - .get(entity) - .and_then(|p| { - player_physics_settings.settings.get(&p.uuid()).copied() - }) - .unwrap_or_default(); - // Don't send client physics updates about itself unless force update is - // set or the client is subject to - // server-authoritative physics - force_updates.get(entity).map_or(false, |f| f.is_forced()) - || player_physics_setting.server_authoritative() - || is_rider.get(entity).is_some() + should_sync_client_physics( + entity, + &player_physics_settings, + &players, + &force_updates, + is_rider, + ) } else if matches!(collider, Some(Collider::Voxel { .. })) { // Things with a voxel collider (airships, etc.) need to have very // stable physics so we always send updated @@ -338,7 +335,8 @@ impl<'a> System<'a> for Sys { ); if include_all_comps && let Some(&pos) = maybe_pos { - add_physics_components(true, &mut comp_sync_package, uid, pos, last_pos, ori, vel); + let send_now = should_sync_client_physics(entity, &player_physics_settings, &players, &force_updates, is_rider); + add_physics_components(send_now, &mut comp_sync_package, uid, pos, last_pos, ori, vel); } if !comp_sync_package.is_empty() { @@ -448,6 +446,27 @@ impl<'a> System<'a> for Sys { } } +/// Determines whether a client should receive an update about its own physics +/// components. +fn should_sync_client_physics( + entity: specs::Entity, + player_physics_settings: &PlayerPhysicsSettings, + players: &ReadStorage<'_, Player>, + force_updates: &WriteStorage<'_, ForceUpdate>, + is_rider: &ReadStorage<'_, Is>, +) -> bool { + let player_physics_setting = players + .get(entity) + .and_then(|p| player_physics_settings.settings.get(&p.uuid()).copied()) + .unwrap_or_default(); + // Don't send client physics updates about itself unless force update is + // set or the client is subject to + // server-authoritative physics + force_updates.get(entity).map_or(false, |f| f.is_forced()) + || player_physics_setting.server_authoritative() + || is_rider.contains(entity) +} + /// Adds physics components if `send_now` is true or `Option>` is /// `None`. /// From ecb27deeae2d8a1f9e24fb800c8af436a861c36b Mon Sep 17 00:00:00 2001 From: Imbris Date: Tue, 6 Jun 2023 02:25:45 -0400 Subject: [PATCH 6/7] When sync_me is false avoid: * Logging a warning when deleting the entity and it is not in any Region. * Searching every single region for an entity that is in none of them. Also: * Add workaround for bug in specs where deleting any entity clears the components before checking that the generation is correct (surprised that we haven't encounted bugs from this yet). * Properly update `tracked_entities` inside `RegionMap` when deleting an entity. Previously, we just relied on this being updated in `RegionMap::tick` by the absence of the `Pos` component at that index. However, this means if a new entity is created at that index after deletion and before calling `RegionMap::tick`, then this can be interpreted as an entity moving between regions rather than one being deleted and another created. It seems like this could lead to synchronization bugs like not creating the new entity on the client (although I haven't noticed this before, I think maybe we use newly inserted `Uid`s to detect new entities rather than the region system?). I think it could at least lead to sending redundant messages to synchronize the new entity. --- client/src/lib.rs | 7 ------ common/src/region.rs | 32 +++++++++++----------------- server/src/events/player.rs | 11 ++++++---- server/src/state_ext.rs | 40 ++++++++++++++++++++++++----------- server/src/sys/entity_sync.rs | 3 +-- 5 files changed, 48 insertions(+), 45 deletions(-) diff --git a/client/src/lib.rs b/client/src/lib.rs index 5ccb65ff65..c19d73529b 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -2257,17 +2257,10 @@ impl Client { .apply_entity_sync_package(entity_sync_package, uid); }, ServerGeneral::CompSync(comp_sync_package, force_counter) => { - // TODO: Test with and without sync_me = true commented out to see if results are - // as expected. (client shouldn't normally receive updates if client physics is - // enabled) - let b = self.position(); self.force_update_counter = force_counter; self.state .ecs_mut() .apply_comp_sync_package(comp_sync_package); - if b != self.position() { - println!("{b:?}"); - } }, ServerGeneral::CreateEntity(entity_package) => { self.state.ecs_mut().apply_entity_package(entity_package); diff --git a/common/src/region.rs b/common/src/region.rs index dda545ab3a..d6905eb94f 100644 --- a/common/src/region.rs +++ b/common/src/region.rs @@ -209,6 +209,13 @@ impl RegionMap { } } + /// Must be called immediately after succesfully deleting an entity from the + /// ecs (i.e. when deleting the entity did not generate a WrongGeneration + /// error). + pub fn entity_deleted(&mut self, entity: specs::Entity) { + self.tracked_entities.remove(entity.id()); + } + fn add_entity(&mut self, id: u32, pos: Vec3, from: Option>) { let key = Self::pos_key(pos); if let Some(region) = self.regions.get_mut(&key) { @@ -247,6 +254,8 @@ impl RegionMap { } } } + } else if !self.tracked_entities.contains(id) { + return None; } else { // Check neighbors for o in &NEIGHBOR_OFFSETS { @@ -269,26 +278,9 @@ impl RegionMap { None } - /// Checks if this entity is located in the `RegionMap` using the provided - /// position to limit which regions are checked. - /// - /// May produce false negatives (e.g. if for some reason the entity is not - /// in a region near the provided position). - pub fn in_region_map_relaxed(&self, entity: specs::Entity, pos: Vec3) -> bool { - let id = entity.id(); - let pos = pos.map(|e| e as i32); - - // Compute key for most likely region - let key = Self::pos_key(pos); - if let Some(region) = self.regions.get(&key) && region.entities().contains(id) { return true } - - // Get the base of the four nearest regions. - let quad_base = Self::pos_key(pos - (REGION_SIZE / 2) as i32); - [(0, 0), (0, 1), (1, 0), (1, 1)] - .iter() - .map(|&offset| Vec2::::from(offset) + quad_base) - // skip key we already checked - .any(|k| k != key && self.regions.get(&key).is_some_and(|r| r.entities().contains(id))) + /// Checks if this entity is located in the `RegionMap`. + pub fn in_region_map(&self, entity: specs::Entity) -> bool { + self.tracked_entities.contains(entity.id()) } fn key_index(&self, key: Vec2) -> Option { diff --git a/server/src/events/player.rs b/server/src/events/player.rs index f329c3bada..7f7bdc2536 100644 --- a/server/src/events/player.rs +++ b/server/src/events/player.rs @@ -127,15 +127,16 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten error!("handle_exit_ingame called with entity that is missing expected components"); } - let maybe_character = state + let (maybe_character, sync_me) = state .read_storage::() .get(entity) - .and_then(|p| p.kind.character_id()); + .map(|p| (p.kind.character_id(), p.sync_me)) + .unzip(); let maybe_rtsim = state.read_component_copied::(entity); state.mut_resource::().remove_entity( Some(entity), None, // Uid re-mapped, we don't want to remove the mapping - maybe_character, + maybe_character.flatten(), maybe_rtsim, ); @@ -143,7 +144,9 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten // Uid to a new entity (and e.g. don't want it to be unmapped). // // Delete old entity - if let Err(e) = crate::state_ext::delete_entity_common(state, entity, maybe_uid) { + if let Err(e) = + crate::state_ext::delete_entity_common(state, entity, maybe_uid, sync_me.unwrap_or(true)) + { error!( ?e, ?entity, diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index 30746a90c3..2e499896d8 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -709,7 +709,7 @@ impl StateExt for State { self.ecs() .write_resource::() .add_character(id, entity); - //presence.sync_me = true; + presence.sync_me = true; Ok(()) } else { Err("PresenceKind is not LoadingCharacter") @@ -1196,20 +1196,21 @@ impl StateExt for State { // NOTE: We expect that these 3 components are never removed from an entity (nor // mutated) (at least not without updating the relevant mappings)! let maybe_uid = self.read_component_copied::(entity); - let maybe_character = self + let (maybe_character, sync_me) = self .read_storage::() .get(entity) - .and_then(|p| p.kind.character_id()); + .map(|p| (p.kind.character_id(), p.sync_me)) + .unzip(); let maybe_rtsim = self.read_component_copied::(entity); self.mut_resource::().remove_entity( Some(entity), maybe_uid, - maybe_character, + maybe_character.flatten(), maybe_rtsim, ); - delete_entity_common(self, entity, maybe_uid) + delete_entity_common(self, entity, maybe_uid, sync_me.unwrap_or(true)) } fn entity_as_actor(&self, entity: EcsEntity) -> Option { @@ -1247,6 +1248,7 @@ pub(crate) fn delete_entity_common( state: &mut State, entity: EcsEntity, maybe_uid: Option, + sync_me: bool, ) -> Result<(), specs::error::WrongGeneration> { if maybe_uid.is_none() { // For now we expect all entities have a Uid component. @@ -1254,8 +1256,24 @@ pub(crate) fn delete_entity_common( } let maybe_pos = state.read_component_copied::(entity); - let res = state.ecs_mut().delete_entity(entity); + // TODO: workaround for https://github.com/amethyst/specs/pull/766 + let actual_gen = state.ecs().entities().entity(entity.id()).gen(); + let res = if actual_gen == entity.gen() { + state.ecs_mut().delete_entity(entity) + } else { + Err(specs::error::WrongGeneration { + action: "delete", + actual_gen, + entity, + }) + }; + if res.is_ok() { + let region_map = state.mut_resource::(); + let uid_pos_region_key = maybe_uid + .zip(maybe_pos) + .map(|(uid, pos)| (uid, pos, region_map.find_region(entity, pos.0))); + region_map.entity_deleted(entity); // Note: Adding the `Uid` to the deleted list when exiting "in-game" relies on // the client not being able to immediately re-enter the game in the // same tick (since we could then mix up the ordering of things and @@ -1263,16 +1281,14 @@ pub(crate) fn delete_entity_common( // // The client will ignore requests to delete its own entity that are triggered // by this. - if let Some((uid, pos)) = maybe_uid.zip(maybe_pos) { - let region_key = state - .ecs() - .read_resource::() - .find_region(entity, pos.0); + if let Some((uid, pos, region_key)) = uid_pos_region_key { if let Some(region_key) = region_key { state .mut_resource::() .record_deleted_entity(uid, region_key); - } else { + // If there is a position and sync_me is true, but the entity is not + // in a region, something might be wrong. + } else if sync_me { // Don't panic if the entity wasn't found in a region, maybe it was just created // and then deleted before the region manager had a chance to assign it a region warn!( diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index 898731efd8..e3e85d6de7 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -325,8 +325,7 @@ impl<'a> System<'a> for Sys { // Include additional components for clients that aren't in a region (e.g. due // to having no position or have sync_me as `false`) since those // won't be synced above. - let include_all_comps = - !maybe_pos.is_some_and(|pos| region_map.in_region_map_relaxed(entity, pos.0)); + let include_all_comps = region_map.in_region_map(entity); let mut comp_sync_package = trackers.create_sync_from_client_package( &tracked_storages, From aba0c23bc7e86716f9834934528d3ebd57c46f7a Mon Sep 17 00:00:00 2001 From: Imbris Date: Fri, 11 Aug 2023 20:09:10 -0400 Subject: [PATCH 7/7] Make sync_me based of PresenceKind rather than being an independent field. --- common/src/comp/presence.rs | 21 +++++++++++++-------- common/src/region.rs | 4 ++-- server/src/events/player.rs | 3 +-- server/src/state_ext.rs | 12 +++--------- 4 files changed, 19 insertions(+), 21 deletions(-) diff --git a/common/src/comp/presence.rs b/common/src/comp/presence.rs index d08cba3cf6..578d5f91f6 100644 --- a/common/src/comp/presence.rs +++ b/common/src/comp/presence.rs @@ -13,23 +13,16 @@ pub struct Presence { /// updated! pub kind: PresenceKind, pub lossy_terrain_compression: bool, - /// Controls whether this entity is synced to other clients. - /// - /// Note, if it ends up being useful this could be generalized to an - /// independent component that is required for any entity to be synced - /// (as an independent component it could use NullStorage). - pub sync_me: bool, } impl Presence { - pub fn new(view_distances: ViewDistances, kind: PresenceKind, sync_me: bool) -> Self { + pub fn new(view_distances: ViewDistances, kind: PresenceKind) -> Self { let now = Instant::now(); Self { terrain_view_distance: ViewDistance::new(view_distances.terrain, now), entity_view_distance: ViewDistance::new(view_distances.entity, now), kind, lossy_terrain_compression: false, - sync_me, } } } @@ -62,6 +55,18 @@ impl PresenceKind { None } } + + /// Controls whether this entity is synced to other clients. + /// + /// Note, if it ends up being useful this could be generalized to an + /// independent component that is required for any entity to be synced + /// (as an independent component it could use NullStorage). + pub fn sync_me(&self) -> bool { + match self { + Self::Spectator | Self::LoadingCharacter(_) => false, + Self::Character(_) | Self::Possessor => true, + } + } } #[derive(PartialEq, Debug, Clone, Copy)] diff --git a/common/src/region.rs b/common/src/region.rs index d6905eb94f..33f4762c7c 100644 --- a/common/src/region.rs +++ b/common/src/region.rs @@ -114,7 +114,7 @@ impl RegionMap { // Add any untracked entities for (pos, id) in (&pos, &entities, presence.maybe(), !&self.tracked_entities) .join() - .filter(|(_, _, presence, _)| presence.map_or(true, |p| p.sync_me)) + .filter(|(_, _, presence, _)| presence.map_or(true, |p| p.kind.sync_me())) .map(|(pos, e, _, _)| (pos, e.id())) .collect::>() { @@ -143,7 +143,7 @@ impl RegionMap { ) .join() { - let should_sync = maybe_presence.map_or(true, |p| p.sync_me); + let should_sync = maybe_presence.map_or(true, |p| p.kind.sync_me()); match maybe_pos { // Switch regions for entities which need switching // TODO don't check every tick (use velocity) (and use id to stagger) diff --git a/server/src/events/player.rs b/server/src/events/player.rs index 7f7bdc2536..91f10f54d3 100644 --- a/server/src/events/player.rs +++ b/server/src/events/player.rs @@ -130,7 +130,7 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten let (maybe_character, sync_me) = state .read_storage::() .get(entity) - .map(|p| (p.kind.character_id(), p.sync_me)) + .map(|p| (p.kind.character_id(), p.kind.sync_me())) .unzip(); let maybe_rtsim = state.read_component_copied::(entity); state.mut_resource::().remove_entity( @@ -421,7 +421,6 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui // from overwriting original character info with stuff from the new character. kind: PresenceKind::Possessor, lossy_terrain_compression: presence.lossy_terrain_compression, - sync_me: presence.sync_me, }) } else { None diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index 2e499896d8..3b67a9a98e 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -604,7 +604,6 @@ impl StateExt for State { entity: view_distance, }, PresenceKind::Spectator, - false, )) } @@ -645,11 +644,7 @@ impl StateExt for State { self.write_component_ignore_entity_dead( entity, - Presence::new( - view_distances, - PresenceKind::LoadingCharacter(character_id), - false, - ), + Presence::new(view_distances, PresenceKind::LoadingCharacter(character_id)), ); // Tell the client its request was successful. @@ -674,7 +669,7 @@ impl StateExt for State { self.write_component_ignore_entity_dead( entity, - Presence::new(view_distances, PresenceKind::Spectator, false), + Presence::new(view_distances, PresenceKind::Spectator), ); // Tell the client its request was successful. @@ -709,7 +704,6 @@ impl StateExt for State { self.ecs() .write_resource::() .add_character(id, entity); - presence.sync_me = true; Ok(()) } else { Err("PresenceKind is not LoadingCharacter") @@ -1199,7 +1193,7 @@ impl StateExt for State { let (maybe_character, sync_me) = self .read_storage::() .get(entity) - .map(|p| (p.kind.character_id(), p.sync_me)) + .map(|p| (p.kind.character_id(), p.kind.sync_me())) .unzip(); let maybe_rtsim = self.read_component_copied::(entity);