Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Prevent windup with FixedTimestep #5692

Closed
wants to merge 1 commit into from
Closed

Prevent windup with FixedTimestep #5692

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 20 additions & 1 deletion crates/bevy_time/src/fixed_timestep.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,18 @@ impl FixedTimestep {
}
}

/// Creates a [`FixedTimestep`] that ticks once every `step` seconds, running up to `max_loops` times to catch up to the clock.
pub fn step_with_max_loops(step: f64, max_loops: usize) -> Self {
Self {
state: LocalFixedTimestepState {
step,
max_time_delta: step * max_loops as f64,
..Default::default()
},
..Default::default()
}
}

/// Creates a [`FixedTimestep`] that ticks once every `rate` times per second.
pub fn steps_per_second(rate: f64) -> Self {
Self {
Expand Down Expand Up @@ -137,6 +149,7 @@ impl FixedTimestep {
struct LocalFixedTimestepState {
label: Option<String>, // TODO: consider making this a TypedLabel
step: f64,
max_time_delta: f64,
accumulator: f64,
looping: bool,
}
Expand All @@ -145,6 +158,7 @@ impl Default for LocalFixedTimestepState {
fn default() -> Self {
Self {
step: 1.0 / 60.0,
max_time_delta: std::f64::INFINITY,
accumulator: 0.0,
label: None,
looping: false,
Expand All @@ -155,7 +169,12 @@ impl Default for LocalFixedTimestepState {
impl LocalFixedTimestepState {
fn update(&mut self, time: &Time) -> ShouldRun {
if !self.looping {
self.accumulator += time.delta_seconds_f64();
let time_delta = time.delta_seconds_f64();
self.accumulator += if time_delta > self.max_time_delta {
self.max_time_delta
} else {
time_delta
};
Comment on lines +172 to +177
Copy link
Contributor

@maniwani maniwani Aug 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the fixed timestep itself, it would be better to allow limiting the number of steps that can be run per frame rather than limit the amount of time that can be accrued.

The current change in this PR would prevent us from keeping the fixed timestep in sync with Time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wouldn't achieve the desired goal, because the system would run the maximum number of steps (up to the limit) until it has caught up to the accumulator.

Copy link
Contributor

@maniwani maniwani Aug 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that we should enable limiting the number of steps per frame as a supplement to pausing Time.

I think it's important that the fixed timestep be kept in sync with Time. Your change would break that connection.

}

if self.accumulator >= self.step {
Expand Down