Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix rollback for pre-predicted entities #788

Merged
merged 3 commits into from
Jan 9, 2025
Merged

Fix rollback for pre-predicted entities #788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3603b95
avoid log
cBournhonesque Jan 8, 2025
5c98a96
fix pre-prediction rollback
cBournhonesque Jan 9, 2025
2d8dceb
Merge branch 'main' into cb/avoid-error-log-preprediction
cBournhonesque Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lightyear/src/client/prediction/pre_prediction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,15 @@ impl PrePredictionPlugin {
#[cfg(test)]
mod tests {
use super::*;
use crate::client::prediction::predicted_history::PredictionHistory;
use crate::prelude::server;
use crate::prelude::server::AuthorityPeer;
use crate::prelude::{client, ClientId};
use crate::tests::protocol::{ComponentClientToServer, ComponentSyncModeFull};
use crate::tests::stepper::{BevyStepper, TEST_CLIENT_ID};

/// Simple preprediction case
/// Also check that the PredictionHistory is correctly added to the PrePredicted entity
#[test]
fn test_pre_prediction() {
// tracing_subscriber::FmtSubscriber::builder()
Expand Down Expand Up @@ -260,6 +262,11 @@ mod tests {
.0,
2.0
);
assert!(stepper
.client_app
.world()
.get::<PredictionHistory<ComponentSyncModeFull>>(predicted_entity)
.is_some());
}

// TODO: test that pre-predicted works in host-server mode
Expand Down
75 changes: 39 additions & 36 deletions lightyear/src/client/prediction/predicted_history.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ pub(crate) fn add_non_networked_component_history<C: Component + PartialEq + Clo

/// Add component history for entities that are predicted
/// There is extra complexity because the component could get added on the Confirmed entity (received from the server), or added to the Predicted entity directly
/// Also:
/// - handle PrePredicted entities (the Predicted entity might already have the component)
/// - handle entity that become Predicted after authority transfer (the Confirmed entity might already have the component)
#[allow(clippy::type_complexity)]
pub(crate) fn add_component_history<C: SyncComponent>(
component_registry: Res<ComponentRegistry>,
Expand All @@ -130,7 +133,7 @@ pub(crate) fn add_component_history<C: SyncComponent>(
With<Predicted>,
),
>,
confirmed_entities: Query<(Entity, &Confirmed, Option<Ref<C>>)>,
confirmed_entities: Query<(Entity, Ref<Confirmed>, Option<Ref<C>>)>,
) {
let kind = std::any::type_name::<C>();
let tick = tick_manager.tick();
Expand All @@ -146,48 +149,48 @@ pub(crate) fn add_component_history<C: SyncComponent>(
&mut commands,
);

// if component got added on confirmed side
// if component got added on confirmed side, or if confirmed itself got added (this is useful to handle cases
// where the confirmed entity already exists and had the component, for example when authority was transferred
// away from the client and the client needs to add prediction)
// - full: sync component and add history
// - simple/once: sync component
if let Some(confirmed_component) = confirmed_component {
// We check this instead of using confirmed_component.is_added()
// in case there were existing components on the confirmed entity
// when the predicted entity was spawned (for example in case of an authority transfer)
if predicted_component.is_some() {
// component already added on predicted side, no need to do anything
continue;
}
trace!(?kind, "Component added on confirmed side");
// safety: we know the entity exists
let mut predicted_entity_mut = commands.get_entity(predicted_entity).unwrap();
// map any entities from confirmed to predicted
let mut new_component = confirmed_component.deref().clone();
let _ = manager.map_entities(&mut new_component, component_registry.as_ref());
match component_registry.prediction_mode::<C>() {
ComponentSyncMode::Full => {
// insert history, 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());
predicted_entity_mut.insert((new_component, history));
}
ComponentSyncMode::Simple => {
debug!(
?kind,
"Component simple synced between confirmed and predicted"
);
// we only sync the components once, but we don't do rollback so no need for a component history
predicted_entity_mut.insert(new_component);
}
ComponentSyncMode::Once => {
// if this was a prespawned entity, don't override SyncMode::Once components!
if predicted_component.is_none() {
if confirmed_component.is_added() || confirmed.is_added() {
trace!(?kind, "Component added on confirmed side");
// safety: we know the entity exists
let mut predicted_entity_mut =
commands.get_entity(predicted_entity).unwrap();
// map any entities from confirmed to predicted
let mut new_component = confirmed_component.deref().clone();
let _ =
manager.map_entities(&mut new_component, component_registry.as_ref());
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)
// 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());
predicted_entity_mut.insert((new_component, history));
}
ComponentSyncMode::Simple => {
debug!(
?kind,
"Component simple synced between confirmed and predicted"
);
// we only sync the components once, but we don't do rollback so no need for a component history
predicted_entity_mut.insert(new_component);
}
ComponentSyncMode::Once => {
// if this was a prespawned entity, don't override SyncMode::Once components!
if predicted_component.is_none() {
// we only sync the components once, but we don't do rollback so no need for a component history
predicted_entity_mut.insert(new_component);
}
}
_ => {}
}
_ => {}
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions lightyear/src/client/prediction/rollback.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use bevy::prelude::{
use bevy::reflect::Reflect;
use bevy::time::{Fixed, Time};
use parking_lot::RwLock;
use tracing::{debug, error, trace, trace_span};
use tracing::{debug, error, trace, trace_span, warn};

use crate::client::components::{Confirmed, SyncComponent};
use crate::client::config::ClientConfig;
Expand Down Expand Up @@ -150,7 +150,7 @@ pub(crate) fn check_rollback<C: SyncComponent>(
continue;
};
let Ok(mut predicted_history) = predicted_query.get_mut(p) else {
debug!(
warn!(
"Predicted entity {:?} was not found when checking rollback for {:?}",
confirmed.predicted,
std::any::type_name::<C>()
Expand All @@ -164,7 +164,7 @@ pub(crate) fn check_rollback<C: SyncComponent>(
// get the tick that the confirmed entity is at
let tick = confirmed.tick;
if tick > current_tick {
debug!(
warn!(
"Confirmed entity {:?} is at a tick in the future: {:?} compared to client timeline. Current tick: {:?}",
confirmed_entity,
tick,
Expand Down
8 changes: 7 additions & 1 deletion lightyear/src/shared/replication/authority.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ impl MapEntities for AuthorityChange {

#[cfg(test)]
mod tests {
use crate::client::prediction::predicted_history::PredictionHistory;
use crate::prelude::client::{Confirmed, ConfirmedHistory};
use crate::prelude::server::{Replicate, SyncTarget};
use crate::prelude::{client, server, ClientId, NetworkTarget, Replicated};
Expand Down Expand Up @@ -1013,9 +1014,14 @@ mod tests {
.0,
2.0
);
assert!(stepper
.client_app_1
.world()
.get::<PredictionHistory<ComponentSyncModeFull>>(predicted_1)
.is_some());
assert_eq!(
stepper
.client_app_2
.client_app_1
.world()
.get::<ComponentSyncModeFull>(predicted_1)
.unwrap()
Expand Down
Loading