Rollback time resource during rollback #623
Conversation
nick-e
force-pushed
the
time-rollback
branch
from
382c89b
to
0a9b948
Compare
|
I tested the fix using the replication process found in #618 |
| @@ -1108,10 +1107,10 @@ impl AppComponentExt for App { | |||
| } | |||
| } | |||
|
|
|||
| /// Do not use `Time<Fixed>` for `R`. `Time<Fixed>` is already rollbacked. | |||
There was a problem hiding this comment.
This comment probably shouldn't be there, right? This is for users who want to use add_resource_rollback
There was a problem hiding this comment.
Updated the doc comments to add clarity. Let me know what you think!
|
Sorry I didn't have time to review in detail, but apparently just using the replication rollback to rollback Time and Time was not enough? We still need to have some ad-hoc logic?
What were the results? There's no rollbacks anymore? |
|
This noticeably improved FPS drops I was experiencing during rollback. However there are still rollbacks for me, although those might be related to other things in my project. |
nick-e
force-pushed
the
time-rollback
branch
from
0a9b948
to
2cb0e58
Compare
|
I updated the PR to correctly advance the elapsed time of the fixed time resource during a rollback. We can't use On top of that, the only way to progress As a rule of thumb, TestingI tested this PR by adding the following to if let Some(ref rb) = rollback {
if let Some(tick) = rb.get_rollback_tick() {
info!(
" rb-{:?}, delta: {}, elapsed: {}",
tick,
time.delta_seconds(),
time.elapsed_seconds()
);
} else {
info!(
"not-rb-{:?}, delta: {}, elapsed: {}",
tick_manager.tick(),
time.delta_seconds(),
time.elapsed_seconds()
);
}
} else {
info!(
"{:?}, delta: {}, elapsed: {}",
tick_manager.tick(),
time.delta_seconds(),
time.elapsed_seconds()
);
}This function is only run within a system within the I set the latency to 100ms and a packet loss of 0.1 in order to incur multi-tick rollbacks. Here are some logs of the Here are some logs with the PR: There are still constant rollbacks. Still looking into that. |
nick-e
force-pushed
the
time-rollback
branch
from
2cb0e58
to
09acf26
Compare
|
I added a test for verifying that the time resource is accurate during a rollback. I can confirm that the test fails without this PR. |
nick-e
force-pushed
the
time-rollback
branch
from
3f5e59a
to
46d7606
Compare
|
|
||
| // Set the rollback tick's generic time resource to the fixed time | ||
| // resource that was just advanced. | ||
| *world.resource_mut::<Time>() = world.resource::<Time<Fixed>>().as_generic(); |
There was a problem hiding this comment.
Why do we want to also rollback the non-fixed-time Time?
There was a problem hiding this comment.
Time is not a non-fixed time but instead a time without a "context". It could represent fixed time or some other kind of time like Time<Virtual>. It's my understanding that systems within the FixedMain schedule should be safe to assume that the value of the generic Time resource they retrieve via Res<Time> represents the Time<Fixed> resource.
|
Thanks for the explanation on why rollbacking Time + running RunFixedMainLoop wouldn't work. I'm convinced by this PR, making sure that Time was handled correctly during rollback was something I had completely overlooked! I left a couple comments but otherwise I think it's ready to merge. Thanks for the awesome work! |
nick-e
force-pushed
the
time-rollback
branch
from
46d7606
to
01aac47
Compare
nick-e
force-pushed
the
time-rollback
branch
from
01aac47
to
50e4629
Compare
Ensure that the
Fixed<Time>resource is accurate during a rollback by rollbacking the time resource when a rollback starts and advancing the time after each rollback tick. This is a continuation of #619. This PR takes into accountTime<Fixed>'s overstep.I originally thought that using
add_resource_rollback::<Time<Fixed>>()and running theRunFixedMainLoopschedule would be the solution butRunFixedMainLooprequires that the real time clock also progress in order to correctly advance theTime<Fixed>resource.Fixes #618