Fix extra rollbacks on pre-spawned entities

lightyear/src/client/prediction/history.rs

@@ -2,6 +2,8 @@ use crate::prelude::Tick;
 use bevy::prelude::{Component, Reflect, Resource};
 use bevy::prelude::{ReflectComponent, ReflectResource};
 use std::collections::VecDeque;
+use std::fmt::Debug;
+use tracing::{error, info};
 
 /// Stores a past value in the history buffer
 #[derive(Debug, PartialEq, Clone, Default, Reflect)]
@@ -66,7 +68,19 @@ impl<R> HistoryBuffer<R> {
     }
 
     /// Add a value to the history buffer
+    /// The tick must be strictly more recent than the most recent update in the buffer
     pub(crate) fn add(&mut self, tick: Tick, value: Option<R>) {
+        if let Some(last_tick) = self.peek().map(|(tick, _)| tick) {
+            assert!(
+                tick >= *last_tick,
+                "Tick must be more recent than the last update in the buffer"
+            );
+            if *last_tick == tick {
+                error!("Adding update to history buffer for tick: {:?} but it already had a value for that tick!", tick);
+                // in this case, let's pop back the update to replace it with the new value
+                self.buffer.pop_back();
+            }
+        }
         self.buffer.push_back((
             tick,
             match value {
@@ -97,8 +111,9 @@ impl<R: Clone> HistoryBuffer<R> {
     /// the history will only contain the ticks where the value got updated, and otherwise
     /// contains gaps. Therefore, we need to always leave a value in the history buffer so that we can
     /// get the values for the future ticks.
-    /// (i.e. if the buffer contains values at tick 4 and 8. If we pop_until_tick(6),
-    /// we still need to include for tick 4 in the buffer, so that if we call pop_until_tick(7) we still have the correct value
+    /// (i.e. if the buffer contains values at tick 4 and 8. If we pop_until_tick(6), we cannot delete the value for tick 4
+    /// because we still need in case we call pop_until_tick(7). What we'll do is remove the value for tick 4 and re-insert it
+    /// for tick 6)
     pub(crate) fn pop_until_tick(&mut self, tick: Tick) -> Option<HistoryState<R>> {
         // self.buffer[partition] is the first element where the buffer_tick > tick
         let partition = self
@@ -137,54 +152,51 @@ mod tests {
     /// Test adding and removing updates to the resource history
     #[test]
     fn test_add_remove_history() {
-        let mut resource_history = HistoryBuffer::<TestValue>::default();
+        let mut history = HistoryBuffer::<TestValue>::default();
 
         // check when we try to access a value when the buffer is empty
-        assert_eq!(resource_history.pop_until_tick(Tick(0)), None);
+        assert_eq!(history.pop_until_tick(Tick(0)), None);
 
         // check when we try to access an exact tick
-        resource_history.add_update(Tick(1), TestValue(1.0));
-        resource_history.add_update(Tick(2), TestValue(2.0));
+        history.add_update(Tick(1), TestValue(1.0));
+        history.add_update(Tick(2), TestValue(2.0));
         assert_eq!(
-            resource_history.pop_until_tick(Tick(2)),
+            history.pop_until_tick(Tick(2)),
             Some(HistoryState::Updated(TestValue(2.0)))
         );
         // check that we cleared older ticks, and that the most recent value still remains
-        assert_eq!(resource_history.buffer.len(), 1);
+        assert_eq!(history.buffer.len(), 1);
         assert_eq!(
-            resource_history.buffer,
+            history.buffer,
             VecDeque::from(vec![(Tick(2), HistoryState::Updated(TestValue(2.0)))])
         );
 
         // check when we try to access a value in-between ticks
-        resource_history.add_update(Tick(4), TestValue(4.0));
+        history.add_update(Tick(4), TestValue(4.0));
         // we retrieve the most recent value older or equal to Tick(3)
         assert_eq!(
-            resource_history.pop_until_tick(Tick(3)),
+            history.pop_until_tick(Tick(3)),
             Some(HistoryState::Updated(TestValue(2.0)))
         );
-        assert_eq!(resource_history.buffer.len(), 2);
+        assert_eq!(history.buffer.len(), 2);
         // check that the most recent value got added back to the buffer at the popped tick
         assert_eq!(
-            resource_history.buffer,
+            history.buffer,
             VecDeque::from(vec![
                 (Tick(3), HistoryState::Updated(TestValue(2.0))),
                 (Tick(4), HistoryState::Updated(TestValue(4.0)))
             ])
         );
 
         // check that nothing happens when we try to access a value before any ticks
-        assert_eq!(resource_history.pop_until_tick(Tick(0)), None);
-        assert_eq!(resource_history.buffer.len(), 2);
+        assert_eq!(history.pop_until_tick(Tick(0)), None);
+        assert_eq!(history.buffer.len(), 2);
 
-        resource_history.add_remove(Tick(5));
-        assert_eq!(resource_history.buffer.len(), 3);
-        assert_eq!(
-            resource_history.peek(),
-            Some(&(Tick(5), HistoryState::Removed))
-        );
+        history.add_remove(Tick(5));
+        assert_eq!(history.buffer.len(), 3);
+        assert_eq!(history.peek(), Some(&(Tick(5), HistoryState::Removed)));
 
-        resource_history.clear();
-        assert_eq!(resource_history.buffer.len(), 0);
+        history.clear();
+        assert_eq!(history.buffer.len(), 0);
     }
 }

lightyear/src/client/prediction/predicted_history.rs

@@ -6,7 +6,7 @@ use bevy::prelude::{
     Added, Commands, Component, DetectChanges, Entity, OnRemove, Or, Query, Ref, Res, Trigger,
     With, Without,
 };
-use tracing::{debug, trace};
+use tracing::{debug, info, trace};
 
 use crate::client::components::{ComponentSyncMode, Confirmed, SyncComponent};
 use crate::client::prediction::history::HistoryBuffer;
@@ -96,11 +96,11 @@ 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, it will be quickly filled by a rollback (since it starts empty before the current client tick)
+                                // 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?
-                                let mut history = PredictionHistory::<C>::default();
-                                history.add_update(tick, confirmed_component.deref().clone());
+                                let history = PredictionHistory::<C>::default();
                                 predicted_entity_mut.insert((new_component, history));
                             }
                             ComponentSyncMode::Simple => {
@@ -135,7 +135,7 @@ pub(crate) fn add_prespawned_component_history<C: SyncComponent>(
     mut commands: Commands,
     tick_manager: Res<TickManager>,
     prespawned_query: Query<
-        (Entity, Option<Ref<C>>),
+        (Entity, Ref<C>),
         (
             Without<PredictionHistory<C>>,
             Without<Confirmed>,
@@ -144,13 +144,20 @@ pub(crate) fn add_prespawned_component_history<C: SyncComponent>(
         ),
     >,
 ) {
+    let tick = tick_manager.tick();
     // add component history for pre-spawned entities right away
     for (predicted_entity, predicted_component) in prespawned_query.iter() {
+        trace!(
+            ?tick,
+            "Potentially adding prediction history for component {:?} for pre-spawned entity {:?}",
+            std::any::type_name::<C>(),
+            predicted_entity
+        );
         add_history::<C>(
             component_registry.as_ref(),
-            tick_manager.tick(),
+            tick,
             predicted_entity,
-            &predicted_component,
+            &Some(predicted_component),
             &mut commands,
         );
     }
@@ -170,9 +177,9 @@ fn add_history<C: SyncComponent>(
             // component got added on predicted side, add history
             if predicted_component.is_added() {
                 debug!(?kind, ?tick, ?predicted_entity, "Adding prediction history");
-                // insert history, it will be quickly filled by a rollback (since it starts empty before the current client tick)
-                let mut history = PredictionHistory::<C>::default();
-                history.add_update(tick, predicted_component.deref().clone());
+                // insert history component
+                // no need to add any value to it because we run the UpdateHistory system set after the SpawnHistory
+                let history = PredictionHistory::<C>::default();
                 commands.entity(predicted_entity).insert(history);
             }
         }

lightyear/src/client/prediction/rollback.rs

@@ -185,6 +185,7 @@ pub(crate) fn check_rollback<C: SyncComponent>(
             // info!(
             //     ?tick,
             //     ?current_tick,
+            //     ?p,
             //     "History before popping until tick. {:?}",
             //     predicted_history
             // );
@@ -210,10 +211,6 @@ pub(crate) fn check_rollback<C: SyncComponent>(
                 // confirm exist. rollback if history value is different
                 Some(c) => history_value.map_or_else(
                     || {
-                        // info!(
-                        //     ?tick, ?current_tick,
-                        //     "Component {} present on confirmed but missing on predicted! History: {:?}", std::any::type_name::<C>(), predicted_history
-                        // );
                         #[cfg(feature = "metrics")]
                         metrics::counter!(format!(
                             "prediction::rollbacks::causes::{}::missing_on_predicted",
@@ -250,9 +247,9 @@ pub(crate) fn check_rollback<C: SyncComponent>(
             if should_rollback {
                 debug!(
                    ?predicted_exist, ?confirmed_exist,
-                   "Rollback check: mismatch for component between predicted and confirmed {:?} on tick {:?} for component {:?}. Current tick: {:?}",
-                   confirmed_entity, tick, kind, current_tick
-                   );
+                   "Rollback check: mismatch for component between predicted {:?} and confirmed {:?} on tick {:?} for component {:?}. Current tick: {:?}",
+                   p, confirmed_entity, tick, kind, current_tick
+                );
                 // in `prepare_rollback`, we will reset the state to what the server sends us for the confirmed.tick
                 // The server sends the packet in PostUpdate after the confirmed.tick is done.
                 // When we do rollback we need to start reading inputs from the tick after that!

lightyear/src/utils/avian2d.rs

@@ -31,8 +31,12 @@ impl Plugin for Avian2dPlugin {
         // }
         app.configure_sets(
             FixedPostUpdate,
-            // run physics before updating the prediction history
             (
+                // run physics before spawning the prediction history for prespawned entities
+                // we want all avian-added components (Rotation, etc.) to be inserted before we try
+                // to spawn the history, so that the history is spawned at the correct time for all components
+                PredictionSet::SpawnHistory,
+                // run physics before updating the prediction history
                 PredictionSet::UpdateHistory,
                 PredictionSet::IncrementRollbackTick,
                 InterpolationSet::UpdateVisualInterpolationState,

lightyear/src/utils/avian3d.rs

@@ -30,8 +30,12 @@ impl Plugin for Avian3dPlugin {
         // }
         app.configure_sets(
             FixedPostUpdate,
-            // run physics before updating the prediction history
             (
+                // run physics before spawning the prediction history for prespawned entities
+                // we want all avian-added components (Rotation, etc.) to be inserted before we try
+                // to spawn the history, so that the history is spawned at the correct time for all components
+                PredictionSet::SpawnHistory,
+                // run physics before updating the prediction history
                 PredictionSet::UpdateHistory,
                 PredictionSet::IncrementRollbackTick,
                 InterpolationSet::UpdateVisualInterpolationState,