add metrics for inputs (#823)

lightyear/src/client/connection.rs

@@ -185,6 +185,11 @@ impl ConnectionManager {
         self.sync_manager.is_synced()
     }
 
+    /// Amount of input delay applied
+    pub(crate) fn input_delay_ticks(&self) -> u16 {
+        self.sync_manager.current_input_delay
+    }
+
     /// Returns true if we received a new server packet on this frame
     pub(crate) fn received_new_server_tick(&self) -> bool {
         self.sync_manager.duration_since_latest_received_server_tick == Duration::default()

lightyear/src/client/input/leafwing.rs

@@ -104,7 +104,6 @@ impl<A: LeafwingUserAction> Default for MessageBuffer<A> {
 impl<A> Default for LeafwingInputConfig<A> {
     fn default() -> Self {
         LeafwingInputConfig {
-            // input_delay_ticks: 0,
             packet_redundancy: 4,
             _marker: PhantomData,
         }
@@ -397,12 +396,7 @@ fn buffer_action_state<A: LeafwingUserAction>(
         With<InputMap<A>>,
     >,
 ) {
-    // TODO: if the input delay changes, this could override a previous tick's input in the InputBuffer
-    //  or leave gaps
-    let input_delay_ticks = config.prediction.input_delay_ticks(
-        connection_manager.ping_manager.rtt(),
-        config.shared.tick.tick_duration,
-    ) as i16;
+    let input_delay_ticks = connection_manager.input_delay_ticks() as i16;
     let tick = tick_manager.tick() + input_delay_ticks;
     for (entity, action_state, mut input_buffer) in action_state_query.iter_mut() {
         input_buffer.set(tick, action_state);
@@ -414,6 +408,15 @@ fn buffer_action_state<A: LeafwingUserAction>(
             "set action state in input buffer: {}",
             input_buffer.as_ref()
         );
+        #[cfg(feature = "metrics")]
+        {
+            metrics::gauge!(format!(
+                "inputs::{}::{}::buffer_size",
+                std::any::type_name::<A>(),
+                entity
+            ))
+            .set(input_buffer.len() as f64);
+        }
     }
     // if let Some(action_state) = global_action_state {
     //     global_input_buffer.set(tick, action_state.as_ref());
@@ -548,10 +551,7 @@ fn prepare_input_message<A: LeafwingUserAction>(
         With<InputMap<A>>,
     >,
 ) {
-    let input_delay_ticks = config.prediction.input_delay_ticks(
-        connection.ping_manager.rtt(),
-        config.shared.tick.tick_duration,
-    ) as i16;
+    let input_delay_ticks = connection.input_delay_ticks() as i16;
     let tick = tick_manager.tick() + input_delay_ticks;
     // TODO: the number of messages should be in SharedConfig
     trace!(tick = ?tick, "prepare_input_message");
@@ -771,6 +771,15 @@ fn receive_remote_player_input_messages<A: LeafwingUserAction>(
                                                 start,
                                                 diffs,
                                             );
+                                            #[cfg(feature = "metrics")]
+                                            {
+                                                metrics::gauge!(format!(
+                                                    "inputs::{}::remote_player::{}::buffer_size",
+                                                    std::any::type_name::<A>(),
+                                                    entity
+                                                ))
+                                                .set(input_buffer.len() as f64);
+                                            }
                                         } else {
                                             // add the ActionState or InputBuffer if they are missing
                                             let mut input_buffer = InputBuffer::<A>::default();

lightyear/src/client/plugin.rs

@@ -81,6 +81,27 @@ struct SetupPlugin {
 //  before the plugin is ready
 impl Plugin for SetupPlugin {
     fn build(&self, app: &mut App) {
+        #[cfg(feature = "metrics")]
+        {
+            metrics::describe_gauge!(
+                "sync::prediction_time::error_ms",
+                metrics::Unit::Milliseconds,
+                // Ideal client time is the time that the client should be at so that the client input
+                // packets arrive on time for the server to process them.
+                "Difference between the actual client time and the ideal client time"
+            );
+            metrics::describe_counter!(
+                "sync::resync_event",
+                metrics::Unit::Count,
+                "Resync events where the client time is resynced with the server time"
+            );
+            metrics::describe_gauge!(
+                "inputs::input_delay_ticks",
+                metrics::Unit::Count,
+                "Amount of input delay applied, in ticks"
+            );
+        }
+
         app
             // RESOURCES //
             .insert_resource(self.config.clone());

lightyear/src/client/prediction/despawn.rs

@@ -169,13 +169,12 @@ pub(crate) fn remove_despawn_marker(
 mod tests {
     use crate::client::prediction::despawn::PredictionDespawnMarker;
     use crate::client::prediction::resource::PredictionManager;
-    use crate::client::prediction::rollback::RollbackEvent;
     use crate::prelude::client::{Confirmed, PredictionDespawnCommandsExt};
     use crate::prelude::server::SyncTarget;
     use crate::prelude::{client, server, NetworkTarget};
     use crate::tests::protocol::{ComponentSyncModeFull, ComponentSyncModeSimple};
     use crate::tests::stepper::BevyStepper;
-    use bevy::prelude::{default, Component, Trigger};
+    use bevy::prelude::{default, Component};
 
     #[derive(Component, Debug, PartialEq)]
     struct TestComponent(usize);

lightyear/src/client/prediction/predicted_history.rs

@@ -37,8 +37,8 @@ pub(crate) fn add_non_networked_component_history<C: Component + PartialEq + Clo
 ) {
     let tick = tick_manager.tick();
     for (entity, predicted_component) in predicted_entities.iter() {
-        let mut history = PredictionHistory::<C>::default();
-        history.add_update(tick, predicted_component.clone());
+        // TODO: should we add a value to the history for this tick?
+        let history = PredictionHistory::<C>::default();
         commands.entity(entity).insert(history);
     }
 }
@@ -69,6 +69,8 @@ pub(crate) fn add_component_history<C: SyncComponent>(
     for (confirmed_entity, confirmed, confirmed_component) in confirmed_entities.iter() {
         if let Some(predicted_entity) = confirmed.predicted {
             if let Ok(predicted_component) = predicted_components.get(predicted_entity) {
+                // TODO: do we want to add a value to the history for this tick? in particular if the component was added during the Update schedule
+                //   the value will correspond to the previous tick!
                 // if component got added on predicted side, add history
                 add_history::<C>(
                     component_registry.as_ref(),
@@ -96,10 +98,8 @@ pub(crate) fn add_component_history<C: SyncComponent>(
                         match component_registry.prediction_mode::<C>() {
                             ComponentSyncMode::Full => {
                                 debug!("Adding history for {:?}", std::any::type_name::<C>());
-                                // insert history, no need to add any value to it because we run the UpdateHistory system set after the SpawnHistory
-                                // it will be quickly filled by a rollback (since it starts empty before the current client tick)
-                                // or will it? because the component just got spawned anyway..
-                                // TODO: then there's no need to add the component here, since it's going to get added during rollback anyway?
+                                // insert history, no need to add any value to it
+                                // because it will be filled by rollback anyway
                                 let history = PredictionHistory::<C>::default();
                                 predicted_entity_mut.insert((new_component, history));
                             }
@@ -128,7 +128,11 @@ pub(crate) fn add_component_history<C: SyncComponent>(
 }
 
 /// Add the history for prespawned entities.
-/// This must run on FixedUpdate (for entities spawned on FixedUpdate) and PreUpdate (for entities spawned on Update)
+///
+/// This must run on FixedPostUpdate (for entities spawned on FixedUpdate).
+/// We do not handle entities created in Update.
+///
+/// The history will be updated with a component value in UpdateHistory so there's no need to add a value here.
 #[allow(clippy::type_complexity)]
 pub(crate) fn add_prespawned_component_history<C: SyncComponent>(
     component_registry: Res<ComponentRegistry>,
@@ -232,7 +236,6 @@ pub(crate) fn apply_component_removal_predicted<C: Component + PartialEq + Clone
     rollback: Res<Rollback>,
     mut predicted_query: Query<&mut PredictionHistory<C>>,
 ) {
-    // TODO: do not run this if component-sync-mode != FULL
     // if the component was removed from the Predicted entity, add the Removal to the history
     if let Ok(mut history) = predicted_query.get_mut(trigger.entity()) {
         // tick for which we will record the history (either the current client tick or the current rollback tick)
@@ -311,7 +314,6 @@ mod tests {
     #[test]
     fn test_add_component_history() {
         let mut stepper = BevyStepper::default();
-
         let tick = stepper.client_tick();
         let confirmed = stepper
             .client_app
@@ -328,7 +330,6 @@ mod tests {
                 confirmed_entity: Some(confirmed),
             })
             .id();
-
         stepper
             .client_app
             .world_mut()
@@ -344,6 +345,7 @@ mod tests {
             .entity_mut(confirmed)
             .insert(ComponentSyncModeFull(1.0));
         stepper.frame_step();
+        let tick = stepper.client_tick();
         assert_eq!(
             stepper
                 .client_app
@@ -367,13 +369,13 @@ mod tests {
         );
 
         // 2. Add the history for ComponentSyncMode::Full that was added to the predicted entity
-        let tick = stepper.client_tick();
         stepper
             .client_app
             .world_mut()
             .entity_mut(predicted)
-            .insert(ComponentSyncModeFull2(1.0));
+            .insert(ComponentSyncModeFull2(2.0));
         stepper.frame_step();
+        let tick = stepper.client_tick();
         assert_eq!(
             stepper
                 .client_app
@@ -382,7 +384,7 @@ mod tests {
                 .get_mut::<PredictionHistory<ComponentSyncModeFull2>>()
                 .expect("Expected prediction history to be added")
                 .pop_until_tick(tick),
-            Some(HistoryState::Updated(ComponentSyncModeFull2(1.0))),
+            Some(HistoryState::Updated(ComponentSyncModeFull2(2.0))),
             "Expected component value to be added to prediction history"
         );
         assert_eq!(
@@ -392,12 +394,11 @@ mod tests {
                 .entity(predicted)
                 .get::<ComponentSyncModeFull2>()
                 .expect("Expected component to be added to predicted entity"),
-            &ComponentSyncModeFull2(1.0),
+            &ComponentSyncModeFull2(2.0),
             "Expected component to be added to predicted entity"
         );
 
         // 3. Don't add the history for ComponentSyncMode::Simple
-        let tick = stepper.client_tick();
         stepper
             .client_app
             .world_mut()
@@ -425,7 +426,6 @@ mod tests {
         );
 
         // 4. Don't add the history for ComponentSyncMode::Once
-        let tick = stepper.client_tick();
         stepper
             .client_app
             .world_mut()
@@ -435,7 +435,8 @@ mod tests {
         assert!(
             stepper
                 .client_app
-                .world()                .entity(predicted)
+                .world()
+                .entity(predicted)
                 .get::<PredictionHistory<ComponentSyncModeOnce>>()
                 .is_none(),
             "Expected component value to not be added to prediction history for ComponentSyncMode::Once"
@@ -452,7 +453,6 @@ mod tests {
         );
 
         // 5. Component with MapEntities get mapped from Confirmed to Predicted
-        let tick = stepper.client_tick();
         stepper
             .client_app
             .world_mut()

lightyear/src/client/prediction/prespawn.rs

@@ -498,15 +498,15 @@ mod tests {
     use crate::client::prediction::history::HistoryState;
     use crate::client::prediction::predicted_history::PredictionHistory;
     use crate::client::prediction::resource::PredictionManager;
-    use crate::client::prediction::rollback::RollbackEvent;
-    use crate::prelude::client::PredictionDespawnCommandsExt;
+    use crate::prelude::client::{is_in_rollback, PredictionDespawnCommandsExt, PredictionSet};
     use crate::prelude::client::{Confirmed, Predicted};
     use crate::prelude::server::{Replicate, SyncTarget};
     use crate::prelude::*;
     use crate::tests::protocol::*;
     use crate::tests::stepper::BevyStepper;
     use crate::utils::ready_buffer::ItemWithReadyKey;
-    use bevy::prelude::{default, Entity, Trigger, With};
+    use bevy::app::PreUpdate;
+    use bevy::prelude::{default, Entity, IntoSystemConfigs, With};
 
     #[test]
     fn test_compute_hash() {
@@ -798,7 +798,7 @@ mod tests {
             .is_err());
     }
 
-    fn panic_on_rollback(_: Trigger<RollbackEvent>) {
+    fn panic_on_rollback() {
         panic!("rollback triggered");
     }
 
@@ -809,7 +809,12 @@ mod tests {
     #[test]
     fn test_prespawn_local_despawn_match() {
         let mut stepper = BevyStepper::default();
-        stepper.client_app.add_observer(panic_on_rollback);
+        stepper.client_app.add_systems(
+            PreUpdate,
+            panic_on_rollback
+                .run_if(is_in_rollback)
+                .in_set(PredictionSet::PrepareRollback),
+        );
 
         let client_tick = stepper.client_tick().0 as usize;
         let server_tick = stepper.server_tick().0 as usize;

lightyear/src/client/prediction/rollback.rs

@@ -540,7 +540,6 @@ pub(crate) fn prepare_rollback_non_networked<C: Component + PartialEq + Clone>(
 
     // 0. If the entity didn't exist at the rollback tick, despawn it
     // TODO? or is it handled for us?
-
     for (entity, component, mut history) in predicted_query.iter_mut() {
         // 1. restore the component to the historical value
         match history.pop_until_tick(rollback_tick) {
@@ -1334,34 +1333,43 @@ mod integration_tests {
     /// - the rollback removes the component from the predicted entity
     #[test]
     fn test_added_predicted_component_rollback() {
-        let (mut stepper, confirmed, predicted) = setup(true);
+        let (mut stepper, confirmed, predicted) = setup(false);
 
         // add a new component to Predicted
         stepper
             .client_app
             .world_mut()
             .entity_mut(predicted)
             .insert(ComponentSyncModeFull(1.0));
+
         stepper.frame_step();
 
-        // create a rollback situation (confirmed doesn't have a component that predicted has)
-        let tick = stepper.client_tick();
-        received_confirmed_update(&mut stepper, confirmed, tick - 1);
+        // the prediction history is updated with this tick
+        let rollback_tick = stepper.client_tick();
+        stepper.frame_step();
 
         // add a non-networked component as well, which should be removed on the rollback
+        // since it did not exist at the rollback tick
         stepper
             .client_app
             .world_mut()
             .entity_mut(predicted)
             .insert(ComponentRollback(1.0));
+
+        // create a rollback situation to a tick where
+        // - confirmed_component missing
+        // - predicted component exists in history
+        received_confirmed_update(&mut stepper, confirmed, rollback_tick);
         stepper.frame_step();
 
-        // check that rollback happened: the component got removed from predicted
+        // check that rollback happened:
+        // the registered component got removed from predicted since it was not present on confirmed
         assert!(stepper
             .client_app
             .world()
             .get::<ComponentSyncModeFull>(predicted)
             .is_none());
+        // the non-networked component got removed from predicted as it wasn't present in the history
         assert!(stepper
             .client_app
             .world()

lightyear/src/client/sync.rs

@@ -520,6 +520,10 @@ impl SyncManager {
         let jitter = ping_manager.jitter();
         // recompute the input delay using the new rtt estimate
         self.current_input_delay = prediction_config.input_delay_ticks(rtt, tick_duration);
+        #[cfg(feature = "metrics")]
+        {
+            metrics::gauge!("inputs::input_delay_ticks").set(self.current_input_delay as f64);
+        }
         // recompute the server time estimate (using the rtt we just computed)
         self.update_server_time_estimate(tick_duration, rtt);
 

lightyear/src/inputs/leafwing/input_buffer.rs

@@ -91,6 +91,11 @@ impl<A: LeafwingUserAction> Default for InputBuffer<A> {
 }
 
 impl<T: LeafwingUserAction> InputBuffer<T> {
+    /// Number of elements in the buffer
+    pub(crate) fn len(&self) -> usize {
+        self.buffer.len()
+    }
+
     // Note: we expect this to be set every tick?
     //  i.e. there should be an ActionState for every tick, even if the action is None
     /// Set the ActionState for the given tick in the InputBuffer