From 7132ef41361700d9f8210835163fb8ff7a150b04 Mon Sep 17 00:00:00 2001
From: Imbris <imbrisf@gmail.com>
Date: Sun, 8 Mar 2020 23:32:34 -0400
Subject: [PATCH] Rearrange some operations in the server tick to reduce
 clientside latency of ServerEvent mediated effects

---
 CHANGELOG.md                   |  1 +
 client/src/lib.rs              |  2 +-
 common/src/state.rs            | 51 ++++++++++++++++--------
 server/src/lib.rs              | 73 ++++++++++++++++++++++------------
 server/src/sys/mod.rs          | 35 +++++++++-------
 server/src/sys/terrain_sync.rs |  8 +---
 server/src/sys/waypoint.rs     | 10 ++++-
 7 files changed, 114 insertions(+), 66 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 37648ad8bf..8cdf3cf0c0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Removed highlighting of non-collectible sprites
 - Fixed /give_exp ignoring player argument
 - Extend run sfx to small animals to prevent sneak attacks by geese.
+- Decreased clientside latency of ServerEvent mediated effects (e.g. projectiles, inventory operations, etc)
 
 ### Removed
 
diff --git a/client/src/lib.rs b/client/src/lib.rs
index 09841e3fa7..f2bc898373 100644
--- a/client/src/lib.rs
+++ b/client/src/lib.rs
@@ -410,7 +410,7 @@ impl Client {
         // 3) Update client local data
 
         // 4) Tick the client's LocalState
-        self.state.tick(dt, add_foreign_systems);
+        self.state.tick(dt, add_foreign_systems, true);
 
         // 5) Terrain
         let pos = self
diff --git a/common/src/state.rs b/common/src/state.rs
index c657785b25..0fbd3202d0 100644
--- a/common/src/state.rs
+++ b/common/src/state.rs
@@ -286,8 +286,35 @@ impl State {
         }
     }
 
+    // Run RegionMap tick to update entity region occupancy
+    pub fn update_region_map(&self) {
+        self.ecs.write_resource::<RegionMap>().tick(
+            self.ecs.read_storage::<comp::Pos>(),
+            self.ecs.read_storage::<comp::Vel>(),
+            self.ecs.entities(),
+        );
+    }
+
+    // Apply terrain changes
+    pub fn apply_terrain_changes(&self) {
+        let mut terrain = self.ecs.write_resource::<TerrainGrid>();
+        let mut modified_blocks = std::mem::replace(
+            &mut self.ecs.write_resource::<BlockChange>().blocks,
+            Default::default(),
+        );
+        // Apply block modifications
+        // Only include in `TerrainChanges` if successful
+        modified_blocks.retain(|pos, block| terrain.set(*pos, *block).is_ok());
+        self.ecs.write_resource::<TerrainChanges>().modified_blocks = modified_blocks;
+    }
+
     /// Execute a single tick, simulating the game state by the given duration.
-    pub fn tick(&mut self, dt: Duration, add_foreign_systems: impl Fn(&mut DispatcherBuilder)) {
+    pub fn tick(
+        &mut self,
+        dt: Duration,
+        add_foreign_systems: impl Fn(&mut DispatcherBuilder),
+        update_terrain_and_regions: bool,
+    ) {
         // Change the time accordingly.
         self.ecs.write_resource::<TimeOfDay>().0 += dt.as_secs_f64() * DAY_CYCLE_FACTOR;
         self.ecs.write_resource::<Time>().0 += dt.as_secs_f64();
@@ -297,12 +324,9 @@ impl State {
         // important physics events.
         self.ecs.write_resource::<DeltaTime>().0 = dt.as_secs_f32().min(MAX_DELTA_TIME);
 
-        // Run RegionMap tick to update entity region occupancy
-        self.ecs.write_resource::<RegionMap>().tick(
-            self.ecs.read_storage::<comp::Pos>(),
-            self.ecs.read_storage::<comp::Vel>(),
-            self.ecs.entities(),
-        );
+        if update_terrain_and_regions {
+            self.update_region_map();
+        }
 
         // Run systems to update the world.
         // Create and run a dispatcher for ecs systems.
@@ -315,16 +339,9 @@ impl State {
 
         self.ecs.maintain();
 
-        // Apply terrain changes
-        let mut terrain = self.ecs.write_resource::<TerrainGrid>();
-        let mut modified_blocks = std::mem::replace(
-            &mut self.ecs.write_resource::<BlockChange>().blocks,
-            Default::default(),
-        );
-        // Apply block modifications
-        // Only include in `TerrainChanges` if successful
-        modified_blocks.retain(|pos, block| terrain.set(*pos, *block).is_ok());
-        self.ecs.write_resource::<TerrainChanges>().modified_blocks = modified_blocks;
+        if update_terrain_and_regions {
+            self.apply_terrain_changes();
+        }
 
         // Process local events
         let events = self.ecs.read_resource::<EventBus<LocalEvent>>().recv_all();
diff --git a/server/src/lib.rs b/server/src/lib.rs
index c3cc142a63..d8710d1b2c 100644
--- a/server/src/lib.rs
+++ b/server/src/lib.rs
@@ -98,6 +98,7 @@ impl Server {
         state.ecs_mut().insert(sys::SubscriptionTimer::default());
         state.ecs_mut().insert(sys::TerrainSyncTimer::default());
         state.ecs_mut().insert(sys::TerrainTimer::default());
+        state.ecs_mut().insert(sys::WaypointTimer::default());
         // Server-only components
         state.ecs_mut().register::<RegionSubscription>();
         state.ecs_mut().register::<Client>();
@@ -336,7 +337,6 @@ impl Server {
         // 8) Finish the tick, passing control of the main thread back
         //    to the frontend
 
-        let before_tick_1 = Instant::now();
         // 1) Build up a list of events for this frame, to be passed to the frontend.
         let mut frontend_events = Vec::new();
 
@@ -354,23 +354,38 @@ impl Server {
         // (e.g. run before controller system)
         sys::message::Sys.run_now(&self.state.ecs());
 
-        let before_tick_4 = Instant::now();
+        let before_state_tick = Instant::now();
 
         // 4) Tick the server's LocalState.
-        self.state.tick(dt, sys::add_server_systems);
+        // 5) Fetch any generated `TerrainChunk`s and insert them into the terrain.
+        // in sys/terrain.rs
+        self.state.tick(dt, sys::add_server_systems, false);
 
         let before_handle_events = Instant::now();
+
         // Handle game events
         frontend_events.append(&mut self.handle_events());
 
+        let before_update_terrain_and_regions = Instant::now();
+
+        // Apply terrain changes and update the region map after processing server
+        // events so that changes made by server events will be immediately
+        // visble to client synchronization systems, minimizing the latency of
+        // `ServerEvent` mediated effects
+        self.state.update_region_map();
+        self.state.apply_terrain_changes();
+
+        let before_sync = Instant::now();
+
+        // 6) Synchronise clients with the new state of the world.
+        sys::run_sync_systems(self.state.ecs_mut());
+
+        let before_world_tick = Instant::now();
+
         // Tick the world
         self.world.tick(dt);
 
-        // 5) Fetch any generated `TerrainChunk`s and insert them into the terrain.
-        // in sys/terrain.rs
-
-        let before_tick_6 = Instant::now();
-        // 6) Synchronise clients with the new state of the world.
+        let before_entity_cleanup = Instant::now();
 
         // Remove NPCs that are outside the view distances of all players
         // This is done by removing NPCs in unloaded chunks
@@ -392,7 +407,7 @@ impl Server {
             }
         }
 
-        let before_tick_7 = Instant::now();
+        let end_of_server_tick = Instant::now();
         // 7) Update Metrics
         let entity_sync_nanos = self
             .state
@@ -412,31 +427,31 @@ impl Server {
             .read_resource::<sys::TerrainSyncTimer>()
             .nanos as i64;
         let terrain_nanos = self.state.ecs().read_resource::<sys::TerrainTimer>().nanos as i64;
-        let total_sys_nanos = entity_sync_nanos
-            + message_nanos
-            + sentinel_nanos
-            + subscription_nanos
-            + terrain_sync_nanos
-            + terrain_nanos;
-        self.metrics
-            .tick_time
-            .with_label_values(&["input"])
-            .set((before_tick_4 - before_tick_1).as_nanos() as i64 - message_nanos);
+        let waypoint_nanos = self.state.ecs().read_resource::<sys::WaypointTimer>().nanos as i64;
+        let total_sys_ran_in_dispatcher_nanos = terrain_nanos + waypoint_nanos;
         self.metrics
             .tick_time
             .with_label_values(&["state tick"])
             .set(
-                (before_handle_events - before_tick_4).as_nanos() as i64
-                    - (total_sys_nanos - message_nanos),
+                (before_handle_events - before_state_tick).as_nanos() as i64
+                    - total_sys_ran_in_dispatcher_nanos,
             );
         self.metrics
             .tick_time
             .with_label_values(&["handle server events"])
-            .set((before_tick_6 - before_handle_events).as_nanos() as i64);
+            .set((before_update_terrain_and_regions - before_handle_events).as_nanos() as i64);
         self.metrics
             .tick_time
-            .with_label_values(&["entity deletion"])
-            .set((before_tick_7 - before_tick_6).as_nanos() as i64);
+            .with_label_values(&["update terrain and region map"])
+            .set((before_sync - before_update_terrain_and_regions).as_nanos() as i64);
+        self.metrics
+            .tick_time
+            .with_label_values(&["world tick"])
+            .set((before_entity_cleanup - before_world_tick).as_nanos() as i64);
+        self.metrics
+            .tick_time
+            .with_label_values(&["entity cleanup"])
+            .set((end_of_server_tick - before_entity_cleanup).as_nanos() as i64);
         self.metrics
             .tick_time
             .with_label_values(&["entity sync"])
@@ -445,6 +460,10 @@ impl Server {
             .tick_time
             .with_label_values(&["message"])
             .set(message_nanos);
+        self.metrics
+            .tick_time
+            .with_label_values(&["sentinel"])
+            .set(sentinel_nanos);
         self.metrics
             .tick_time
             .with_label_values(&["subscription"])
@@ -457,6 +476,10 @@ impl Server {
             .tick_time
             .with_label_values(&["terrain"])
             .set(terrain_nanos);
+        self.metrics
+            .tick_time
+            .with_label_values(&["waypoint"])
+            .set(waypoint_nanos);
         self.metrics
             .player_online
             .set(self.state.ecs().read_storage::<Client>().join().count() as i64);
@@ -476,7 +499,7 @@ impl Server {
         self.metrics
             .tick_time
             .with_label_values(&["metrics"])
-            .set(before_tick_7.elapsed().as_nanos() as i64);
+            .set(end_of_server_tick.elapsed().as_nanos() as i64);
 
         // 8) Finish the tick, pass control back to the frontend.
 
diff --git a/server/src/sys/mod.rs b/server/src/sys/mod.rs
index bead21501e..d622582e9b 100644
--- a/server/src/sys/mod.rs
+++ b/server/src/sys/mod.rs
@@ -15,30 +15,35 @@ pub type SentinelTimer = SysTimer<sentinel::Sys>;
 pub type SubscriptionTimer = SysTimer<subscription::Sys>;
 pub type TerrainTimer = SysTimer<terrain::Sys>;
 pub type TerrainSyncTimer = SysTimer<terrain_sync::Sys>;
+pub type WaypointTimer = SysTimer<waypoint::Sys>;
 
 // System names
-const ENTITY_SYNC_SYS: &str = "server_entity_sync_sys";
-const SENTINEL_SYS: &str = "sentinel_sys";
-const SUBSCRIPTION_SYS: &str = "server_subscription_sys";
-const TERRAIN_SYNC_SYS: &str = "server_terrain_sync_sys";
+// Note: commented names may be useful in the future
+//const ENTITY_SYNC_SYS: &str = "server_entity_sync_sys";
+//const SENTINEL_SYS: &str = "sentinel_sys";
+//const SUBSCRIPTION_SYS: &str = "server_subscription_sys";
+//const TERRAIN_SYNC_SYS: &str = "server_terrain_sync_sys";
 const TERRAIN_SYS: &str = "server_terrain_sys";
 const WAYPOINT_SYS: &str = "waypoint_sys";
 
 pub fn add_server_systems(dispatch_builder: &mut DispatcherBuilder) {
-    // TODO: makes some of these dependent on systems in common like the phys system
-    dispatch_builder.add(sentinel::Sys, SENTINEL_SYS, &[common::sys::PHYS_SYS]);
-    dispatch_builder.add(subscription::Sys, SUBSCRIPTION_SYS, &[
-        common::sys::PHYS_SYS,
-    ]);
-    dispatch_builder.add(entity_sync::Sys, ENTITY_SYNC_SYS, &[
-        SUBSCRIPTION_SYS,
-        SENTINEL_SYS,
-    ]);
-    dispatch_builder.add(terrain_sync::Sys, TERRAIN_SYS, &[]);
-    dispatch_builder.add(terrain::Sys, TERRAIN_SYNC_SYS, &[TERRAIN_SYS]);
+    dispatch_builder.add(terrain::Sys, TERRAIN_SYS, &[]);
     dispatch_builder.add(waypoint::Sys, WAYPOINT_SYS, &[]);
 }
 
+pub fn run_sync_systems(ecs: &mut specs::World) {
+    use specs::RunNow;
+
+    // Setup for entity sync
+    // If I'm not mistaken, these two could be ran in parallel
+    sentinel::Sys.run_now(ecs);
+    subscription::Sys.run_now(ecs);
+
+    // Sync
+    terrain_sync::Sys.run_now(ecs);
+    entity_sync::Sys.run_now(ecs);
+}
+
 /// Used to keep track of how much time each system takes
 pub struct SysTimer<S> {
     pub nanos: u64,
diff --git a/server/src/sys/terrain_sync.rs b/server/src/sys/terrain_sync.rs
index 426a062788..2300640120 100644
--- a/server/src/sys/terrain_sync.rs
+++ b/server/src/sys/terrain_sync.rs
@@ -8,12 +8,8 @@ use common::{
 };
 use specs::{Join, Read, ReadExpect, ReadStorage, System, Write, WriteStorage};
 
-/// This system will handle loading generated chunks and unloading
-/// uneeded chunks.
-///     1. Inserts newly generated chunks into the TerrainGrid
-///     2. Sends new chunks to neaby clients
-///     3. Handles the chunk's supplement (e.g. npcs)
-///     4. Removes chunks outside the range of players
+/// This systems sends new chunks to clients as well as changes to existing
+/// chunks
 pub struct Sys;
 impl<'a> System<'a> for Sys {
     type SystemData = (
diff --git a/server/src/sys/waypoint.rs b/server/src/sys/waypoint.rs
index a09ec4e92b..6234b95d22 100644
--- a/server/src/sys/waypoint.rs
+++ b/server/src/sys/waypoint.rs
@@ -1,5 +1,6 @@
+use super::SysTimer;
 use common::comp::{Player, Pos, Waypoint, WaypointArea};
-use specs::{Entities, Join, ReadStorage, System, WriteStorage};
+use specs::{Entities, Join, ReadStorage, System, Write, WriteStorage};
 
 /// This system updates player waypoints
 /// TODO: Make this faster by only considering local waypoints
@@ -11,12 +12,15 @@ impl<'a> System<'a> for Sys {
         ReadStorage<'a, Player>,
         ReadStorage<'a, WaypointArea>,
         WriteStorage<'a, Waypoint>,
+        Write<'a, SysTimer<Self>>,
     );
 
     fn run(
         &mut self,
-        (entities, positions, players, waypoint_areas, mut waypoints): Self::SystemData,
+        (entities, positions, players, waypoint_areas, mut waypoints, mut timer): Self::SystemData,
     ) {
+        timer.start();
+
         for (entity, player_pos, _) in (&entities, &positions, &players).join() {
             for (waypoint_pos, waypoint_area) in (&positions, &waypoint_areas).join() {
                 if player_pos.0.distance_squared(waypoint_pos.0) < waypoint_area.radius().powf(2.0)
@@ -25,5 +29,7 @@ impl<'a> System<'a> for Sys {
                 }
             }
         }
+
+        timer.end();
     }
 }