From c36d6e873f813236ea023dcbd9f0d019f1fe0e53 Mon Sep 17 00:00:00 2001 From: Imbris Date: Sun, 21 Aug 2022 00:48:51 -0400 Subject: [PATCH] Fix issue with the region subscription system not updating on view distance changes (until the player crossed a chunk boundary and triggered the normal update). This introduces a `ViewDistance` struct that provides an abstraction around limiting the rate the view distance can be cycled up and down. This helps avoid unnecessary sending, deleting, and then resending of synced things like entities (the client will still delete its terrain locally and re-request it though). The second part of this fix is storing the last view distance in the `RegionSubscription` struct and then updating region subscriptions if this doesn't match the current view distance in the `Presence` component. --- server/src/presence.rs | 101 ++++++++++++++++++++++++++++++--- server/src/state_ext.rs | 2 +- server/src/sys/entity_sync.rs | 3 +- server/src/sys/msg/in_game.rs | 31 ++++++---- server/src/sys/msg/terrain.rs | 2 +- server/src/sys/subscription.rs | 19 +++++-- server/src/sys/terrain.rs | 4 +- server/src/sys/terrain_sync.rs | 8 ++- 8 files changed, 139 insertions(+), 31 deletions(-) diff --git a/server/src/presence.rs b/server/src/presence.rs index fea7730441..c6559d1925 100644 --- a/server/src/presence.rs +++ b/server/src/presence.rs @@ -1,12 +1,14 @@ use common_net::msg::PresenceKind; use hashbrown::HashSet; use serde::{Deserialize, Serialize}; -use specs::{Component, DenseVecStorage, DerefFlaggedStorage, NullStorage, VecStorage}; +use specs::{Component, DerefFlaggedStorage, NullStorage}; +use std::time::{Duration, Instant}; use vek::*; -#[derive(Clone, Debug, Serialize, Deserialize)] +#[derive(Debug)] pub struct Presence { - pub view_distance: u32, + // TODO: separate view distance for syncing entities? + pub view_distance: ViewDistance, pub kind: PresenceKind, pub lossy_terrain_compression: bool, } @@ -14,7 +16,7 @@ pub struct Presence { impl Presence { pub fn new(view_distance: u32, kind: PresenceKind) -> Self { Self { - view_distance, + view_distance: ViewDistance::new(view_distance, Instant::now()), kind, lossy_terrain_compression: false, } @@ -22,8 +24,7 @@ impl Presence { } impl Component for Presence { - // Presence seems <= 64 bits, so it isn't worth using DenseVecStorage. - type Storage = DerefFlaggedStorage>; + type Storage = DerefFlaggedStorage>; } // Distance from fuzzy_chunk before snapping to current chunk @@ -34,11 +35,12 @@ pub const REGION_FUZZ: u32 = 16; #[derive(Clone, Debug)] pub struct RegionSubscription { pub fuzzy_chunk: Vec2, + pub last_view_distance: u32, pub regions: HashSet>, } impl Component for RegionSubscription { - type Storage = DerefFlaggedStorage>; + type Storage = DerefFlaggedStorage>; } #[derive(Copy, Clone, Debug, Default, Serialize, Deserialize)] @@ -47,3 +49,88 @@ pub struct RepositionOnChunkLoad; impl Component for RepositionOnChunkLoad { type Storage = NullStorage; } + +#[derive(PartialEq, Debug, Clone, Copy)] +enum Direction { + Up, + Down, +} + +/// Distance from the [Presence] from which the world is loaded and information +/// is synced to clients. +/// +/// We limit the frequency that changes in the view distance change direction +/// (e.g. shifting from increasing the value to decreasing it). This is useful +/// since we want to avoid rapid cycles of shrinking and expanding of the view +/// distance. +#[derive(Debug)] +pub struct ViewDistance { + direction: Direction, + last_direction_change_time: Instant, + target: Option, + current: u32, +} + +impl ViewDistance { + /// Minimum time allowed between changes in direction of value adjustments. + const TIME_PER_DIR_CHANGE: Duration = Duration::from_millis(300); + + pub fn new(start_value: u32, now: Instant) -> Self { + Self { + direction: Direction::Up, + last_direction_change_time: now - Self::TIME_PER_DIR_CHANGE, + target: None, + current: start_value, + } + } + + /// Returns the current value. + pub fn current(&self) -> u32 { self.current } + + /// Applies deferred change based on the whether the time to apply it has + /// been reached. + pub fn update(&mut self, now: Instant) { + if let Some(target_val) = self.target { + if now.saturating_duration_since(self.last_direction_change_time) + > Self::TIME_PER_DIR_CHANGE + { + self.last_direction_change_time = now; + self.current = target_val; + self.target = None; + } + } + } + + /// Sets the target value. + /// + /// If this hasn't been changed recently or it is in the same direction as + /// the previous change it will be applied immediately. Otherwise, it + /// will be deferred to a later time (limiting the frequency of changes + /// in the change direction). + pub fn set_target(&mut self, new_target: u32, now: Instant) { + use core::cmp::Ordering; + let new_direction = match new_target.cmp(&self.current) { + Ordering::Equal => return, // No change needed. + Ordering::Less => Direction::Down, + Ordering::Greater => Direction::Up, + }; + + // Change is in the same direction as before so we can just apply it. + if new_direction == self.direction { + self.current = new_target; + self.target = None; + // If it has already been a while since the last direction change we can + // directly apply the request and switch the direction. + } else if now.saturating_duration_since(self.last_direction_change_time) + > Self::TIME_PER_DIR_CHANGE + { + self.direction = new_direction; + self.last_direction_change_time = now; + self.current = new_target; + self.target = None; + // Otherwise, we need to defer the request. + } else { + self.target = Some(new_target); + } + } +} diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index dd1241f5b8..d3c1bb94e7 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -519,7 +519,7 @@ impl StateExt for State { // Make sure physics components are updated self.write_component_ignore_entity_dead(entity, comp::ForceUpdate::forced()); - const INITIAL_VD: u32 = 5; //will be changed after login + const INITIAL_VD: u32 = 5; // will be changed after login self.write_component_ignore_entity_dead( entity, Presence::new(INITIAL_VD, PresenceKind::Character(character_id)), diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index ceff297429..a2e5407322 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -384,7 +384,8 @@ impl<'a> System<'a> for Sys { let is_near = |o_pos: Vec3| { pos.zip_with(presence, |pos, presence| { pos.0.xy().distance_squared(o_pos.xy()) - < (presence.view_distance as f32 * TerrainChunkSize::RECT_SIZE.x as f32) + < (presence.view_distance.current() as f32 + * TerrainChunkSize::RECT_SIZE.x as f32) .powi(2) }) }; diff --git a/server/src/sys/msg/in_game.rs b/server/src/sys/msg/in_game.rs index 474d16a902..a47a784a89 100644 --- a/server/src/sys/msg/in_game.rs +++ b/server/src/sys/msg/in_game.rs @@ -17,6 +17,7 @@ use common_ecs::{Job, Origin, Phase, System}; use common_net::msg::{ClientGeneral, PresenceKind, ServerGeneral}; use common_state::{BlockChange, BuildAreas}; use specs::{Entities, Join, Read, ReadExpect, ReadStorage, Write, WriteStorage}; +use std::time::Instant; use tracing::{debug, trace, warn}; use vek::*; @@ -49,6 +50,7 @@ impl Sys { _terrain_persistence: &mut TerrainPersistenceData<'_>, maybe_player: &Option<&Player>, maybe_admin: &Option<&Admin>, + time_for_vd_changes: Instant, msg: ClientGeneral, ) -> Result<(), crate::error::Error> { let presence = match maybe_presence { @@ -67,20 +69,15 @@ impl Sys { *maybe_presence = None; }, ClientGeneral::SetViewDistance(view_distance) => { - presence.view_distance = settings + let clamped_view_distance = settings .max_view_distance .map(|max| view_distance.min(max)) .unwrap_or(view_distance); + presence.view_distance.set_target(clamped_view_distance, time_for_vd_changes); - //correct client if its VD is to high - if settings - .max_view_distance - .map(|max| view_distance > max) - .unwrap_or(false) - { - client.send(ServerGeneral::SetViewDistance( - settings.max_view_distance.unwrap_or(0), - ))?; + // Correct client if its requested VD is too high. + if view_distance != clamped_view_distance { + client.send(ServerGeneral::SetViewDistance(clamped_view_distance))?; } }, ClientGeneral::ControllerInputs(inputs) => { @@ -378,6 +375,8 @@ impl<'a> System<'a> for Sys { ) { let mut server_emitter = server_event_bus.emitter(); + let time_for_vd_changes = Instant::now(); + for (entity, client, mut maybe_presence, player, maybe_admin) in ( &entities, &mut clients, @@ -387,12 +386,15 @@ impl<'a> System<'a> for Sys { ) .join() { + // If an `ExitInGame` message is received this is set to `None` allowing further + // ingame messages to be ignored. + let mut clearable_maybe_presence = maybe_presence.as_deref_mut(); let _ = super::try_recv_all(client, 2, |client, msg| { Self::handle_client_in_game_msg( &mut server_emitter, entity, client, - &mut maybe_presence.as_deref_mut(), + &mut clearable_maybe_presence, &terrain, &can_build, &is_rider, @@ -410,9 +412,16 @@ impl<'a> System<'a> for Sys { &mut terrain_persistence, &player, &maybe_admin, + time_for_vd_changes, msg, ) }); + + // Ensure deferred view distance changes are applied (if the + // requsite time has elapsed). + if let Some(mut presence) = maybe_presence { + presence.view_distance.update(time_for_vd_changes); + } } } } diff --git a/server/src/sys/msg/terrain.rs b/server/src/sys/msg/terrain.rs index af91135db6..f129ce5912 100644 --- a/server/src/sys/msg/terrain.rs +++ b/server/src/sys/msg/terrain.rs @@ -76,7 +76,7 @@ impl<'a> System<'a> for Sys { pos.0.xy().map(|e| e as f64).distance_squared( key.map(|e| e as f64 + 0.5) * TerrainChunkSize::RECT_SIZE.map(|e| e as f64), - ) < ((presence.view_distance as f64 - 1.0 + ) < ((presence.view_distance.current() as f64 - 1.0 + 2.5 * 2.0_f64.sqrt()) * TerrainChunkSize::RECT_SIZE.x as f64) .powi(2) diff --git a/server/src/sys/subscription.rs b/server/src/sys/subscription.rs index a2748ac8ea..5136f44c57 100644 --- a/server/src/sys/subscription.rs +++ b/server/src/sys/subscription.rs @@ -60,7 +60,8 @@ impl<'a> System<'a> for Sys { // To update subscriptions // 1. Iterate through clients // 2. Calculate current chunk position - // 3. If chunk is the same return, otherwise continue (use fuzziness) + // 3. If chunk is different (use fuzziness) or the client view distance + // has changed continue, otherwise return // 4. Iterate through subscribed regions // 5. Check if region is still in range (use fuzziness) // 6. If not in range @@ -78,13 +79,15 @@ impl<'a> System<'a> for Sys { ) .join() { - let vd = presence.view_distance; + let vd = presence.view_distance.current(); // Calculate current chunk let chunk = (Vec2::::from(pos.0)) .map2(TerrainChunkSize::RECT_SIZE, |e, sz| e as i32 / sz as i32); - // Only update regions when moving to a new chunk - // uses a fuzzy border to prevent rapid triggering when moving along chunk - // boundaries + // Only update regions when moving to a new chunk or if view distance has + // changed. + // + // Uses a fuzzy border to prevent rapid triggering when moving along chunk + // boundaries. if chunk != subscription.fuzzy_chunk && (subscription .fuzzy_chunk @@ -96,7 +99,10 @@ impl<'a> System<'a> for Sys { e.abs() > (sz / 2 + presence::CHUNK_FUZZ) as f32 }) .reduce_or() + || subscription.last_view_distance != vd { + // Update the view distance + subscription.last_view_distance = vd; // Update current chunk subscription.fuzzy_chunk = Vec2::::from(pos.0) .map2(TerrainChunkSize::RECT_SIZE, |e, sz| e as i32 / sz as i32); @@ -216,7 +222,7 @@ pub fn initialize_region_subscription(world: &World, entity: specs::Entity) { let chunk_size = TerrainChunkSize::RECT_SIZE.reduce_max() as f32; let regions = regions_in_vd( client_pos.0, - (presence.view_distance as f32 * chunk_size) as f32 + (presence.view_distance.current() as f32 * chunk_size) as f32 + (presence::CHUNK_FUZZ as f32 + chunk_size) * 2.0f32.sqrt(), ); @@ -261,6 +267,7 @@ pub fn initialize_region_subscription(world: &World, entity: specs::Entity) { if let Err(e) = world.write_storage().insert(entity, RegionSubscription { fuzzy_chunk, + last_view_distance: presence.view_distance.current(), regions, }) { error!(?e, "Failed to insert region subscription component"); diff --git a/server/src/sys/terrain.rs b/server/src/sys/terrain.rs index 9bf2ecf288..a7243e9f39 100644 --- a/server/src/sys/terrain.rs +++ b/server/src/sys/terrain.rs @@ -269,7 +269,7 @@ impl<'a> System<'a> for Sys { .map(|e: i32| (e.unsigned_abs()).saturating_sub(2)) .magnitude_squared(); - if adjusted_dist_sqr <= presence.view_distance.pow(2) { + if adjusted_dist_sqr <= presence.view_distance.current().pow(2) { chunk_send_emitter.emit(ChunkSendEntry { entity, chunk_key: key, @@ -293,7 +293,7 @@ impl<'a> System<'a> for Sys { // For each player with a position, calculate the distance. for (presence, pos) in (&presences, &positions).join() { - if chunk_in_vd(pos.0, chunk_key, &terrain, presence.view_distance) { + if chunk_in_vd(pos.0, chunk_key, &terrain, presence.view_distance.current()) { should_drop = false; break; } diff --git a/server/src/sys/terrain_sync.rs b/server/src/sys/terrain_sync.rs index a12302b393..010502910e 100644 --- a/server/src/sys/terrain_sync.rs +++ b/server/src/sys/terrain_sync.rs @@ -33,8 +33,12 @@ impl<'a> System<'a> for Sys { // Sync changed chunks for chunk_key in &terrain_changes.modified_chunks { for (entity, presence, pos) in (&entities, &presences, &positions).join() { - if super::terrain::chunk_in_vd(pos.0, *chunk_key, &terrain, presence.view_distance) - { + if super::terrain::chunk_in_vd( + pos.0, + *chunk_key, + &terrain, + presence.view_distance.current(), + ) { chunk_send_emitter.emit(ChunkSendEntry { entity, chunk_key: *chunk_key,