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.

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

Add time scaling and simplify fixed timestep use #3002

Closed
wants to merge 3 commits into from

Conversation

maniwani
Copy link
Contributor

@maniwani maniwani commented Oct 21, 2021

Objective

  • Fill out Time API.
  • Make using FixedTimestep less prone to user error.
  • Implement time scaling.
  • Improve documentation.

Solution

  • Ensure the "delta" and "elapsed" methods on Time all have f32, f64, and Duration variants.
    • The f32 and f64 values are cached, but derived from the Duration value to minimize drift from rounding errors.
  • Add an explicit FixedTime counterpart to Time for people to use in their fixed timestep systems.
  • Remove the FixedTimesteps resource (HashMap<String, FixedTimestepState>).
    • There's only one FixedTimestepState now (only need to read it when you want to interpolate something).
    • Having more than one instance was a big footgun.
  • Implement time scaling.
    • Fixed time follows scaled time in fixed increments.

Setting the step rate isn't quite as ergonomic as before but it's still pretty easy to do.

Changelist

  • Renamed Time::time_since_startup to Time::elapsed_since_startup.
  • Added method to get the Instant of the first Time update (there's some delay after startup).
    • Time::first_update
  • Added methods to control time scaling.
    • Time::relative_speed
    • Time::set_relative_speed
  • Added methods that ignore time scaling (i.e. for diagnostics).
    • Time::raw_delta
    • Time::raw_elapsed_since_startup
  • Added FixedTime resource with similar API.
    • FixedTime::startup (should give same value as Time::startup)
    • FixedTime::first_update
    • FixedTime::last_update
    • FixedTime::delta
    • FixedTime::set_delta
    • FixedTime::steps_per_second
    • FixedTime::set_steps_per_second
    • FixedTime::elasped_since_startup
  • Changed FixedTimestepState to be step-size agnostic so it works seamlessly with time scaling.
  • Removed FixedTimesteps resource. (explained below)
    • The builtin run criteria is now just FixedTimestep::step. Set the base step size/rate through FixedTime.

Why only one built-in fixed timestep?

tl;dr

  • FixedTimestep is for repeatable, deterministic behavior
  • having two or more will mess up system ordering, which is non-deterministic and defeats the purpose
  • no other engine has this footgun and we don't need to either
  • users wanting precise time intervals need to use another thread, FixedTimestep won't work for that

A fixed timestep is "context" that wraps a block of systems and sort-of-but-not-really-decouples it from the main frame rate. (It's still inside the frame loop, so nothing is really decoupled.) The systems inside the block use your hardcoded dt value, and FixedTimestep just makes it so the average time between runs is dt. It'll regularly loop several times in a single frame to achieve that.

So your systems always see a constant dt while the actual dt between runs varies wildly. Great for getting consistent game physics, but useless if you wanted an actual fixed frequency (if you need that, your only option is a dedicated thread).

Anyway, FixedTimestep only works correctly when it wraps a single system/stage/sub-schedule. You can't use a bunch of them in different places without running into some subtle side effects.

For example, if you had two of FixedTimestep instances, their steps wouldn't run in a consistent order. E.g. if you had a 50ms timestep A and a 100ms timestep B, you'd probably expect them to interleave like this...

A AB A AB A AB...

...which might often be the case. However, if the app stutters even a little (e.g. one frame takes half a second), A and B would get several steps queued up, and then their order on the next frame would be completely screwed up (because of how they catch up):

AAAAAA BBB...

So a single long frame could cause very subtle bugs.

Note: You can subdivide one timestep into longer ones by counting. For example, you can get a 1 second timestep from a 100ms timestep just by counting to 10 and having run criteria detect that. That gives variable—but still coarse—rate limiting without messing up the system order.

I still left FixedTimestepState pub so people can build their own HashMap<Key, FixedTimestepState> resource if they need it for something exotic.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 21, 2021
@maniwani maniwani force-pushed the make_fixed_timestep_better branch 2 times, most recently from 754d57f to 7d77bb6 Compare October 21, 2021 23:10
@alice-i-cecile alice-i-cecile added A-Core C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Oct 22, 2021
@maniwani maniwani force-pushed the make_fixed_timestep_better branch from 6a57c25 to 2dddfc6 Compare October 22, 2021 20:58
Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

This change seem to introduce time scaling. I'd be against doing so, if not at all, at least mixed in this PR with other changes, and instead have a proper discussion about it (apology if I missed it). Time scaling is a very delicate and complex topic, that is likely to break a lot of things, especially any game physics and animation.

crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

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

I can't seem to find any accumulated time / fixed time. Is there a reason for that? Unity has Time.time and Time.fixedTime, which are helpful sometimes, as not all logic can reason in terms of deltas. Can we add time() (which is I think last_update() maybe?) and fixed_time() as the accumulated time since startup of those 2 clocks? I expect if not, users will use their own accumulator, and this will end up being duplicated in several systems.

@maniwani
Copy link
Contributor Author

maniwani commented Nov 4, 2021

I can't seem to find any accumulated time / fixed time. Is there a reason for that?

No, I just forgot.

Can we add time()?

That's time.time_since_startup(). It's the accumulated time since startup, equivalent to Unity's Time.time.

Btw, Time.time and Time.fixedTime aren't strictly decoupled. They both derive from Time.deltaTime. The former just adds it directly while the latter accumulates them to update in discrete increments. Just want to make sure that's clear.

@maniwani maniwani force-pushed the make_fixed_timestep_better branch from 7accde0 to f9da443 Compare November 6, 2021 21:58
@maniwani
Copy link
Contributor Author

maniwani commented Nov 6, 2021

Okay, I made edits to the fn comments, got rid of the "live" getters for the raw, unscaled time, and added a field to track the "current time" of the fixed loop.

Now I'm wondering if separating the fixed stuff into a FixedTime resource might be better. No matter what, it'd be functionally coupled to Time (because of the accumulator), but a system using FixedTime::delta might express intent more clearly than Time::fixed_delta.

AFAIK you wouldn't need both time resources in a single system, so having that split can still have clean ergonomics. Well, UI systems that produce stuff consumed by the fixed-step logic might need both, idk. Something to think about.

@jamesbeilby
Copy link
Contributor

Nice!
Probably FrameTimeDiagnosticsPlugin and LogDiagnosticsPlugin should use raw time?

crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
@Guvante
Copy link
Contributor

Guvante commented Jan 8, 2022

I think splitting makes sense, especially since accessing both is a code smell. The overall clock will be the latest but the fixed clock could be arbitrarily behind depending on the situation.

@maniwani maniwani force-pushed the make_fixed_timestep_better branch from 5e82ddf to a04d805 Compare January 9, 2022 21:49
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Migration-Guide labels Jan 20, 2022
@maniwani maniwani force-pushed the make_fixed_timestep_better branch from 4f0e7da to e0335c2 Compare March 4, 2022 03:44
@maniwani maniwani changed the title Reorganize Time and FixedTimestep and add runtime step rate control Add time scaling and simplify fixed timestep use Mar 4, 2022
@maniwani
Copy link
Contributor Author

maniwani commented Mar 4, 2022

Decided to go ahead and split things into a separate FixedTime resource, so this could probably use another review.

(Also I messed up merging changes from main and had to rebase. Sorry if anyone was depending on my branch.)

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Alright, this isn't a complete review. I only looked at the time.rs file, but here's a few things I noticed.

crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/time/time.rs Outdated Show resolved Hide resolved
@maniwani
Copy link
Contributor Author

maniwani commented Mar 7, 2022

bors try

bors bot added a commit that referenced this pull request Mar 7, 2022
@maniwani
Copy link
Contributor Author

maniwani commented Mar 14, 2022

The bpm example may have been a poor example, but it fits for syncing gameplay to music, even if not for coordinating talking to the audio middleware.

Ah, okay. Since you had mentioned bpm, I just assumed it was headed in the direction of "let's use FixedTimestep like a metronome", which just wouldn't work. But yeah, rhythm games and other audio-sensitive mechanics probably need their "time" driven by the audio hardware. Your understanding on how we might map game time to, like, track playback position is probably way better than mine.

In that case, I think that how the fixed timestep works is pretty orthogonal to all that, since regardless of how many there are, they would all derive from Time. Even if it turns out audio engines like Quartz use this same struct internally, I'd imagine we would expose it as a separate type/resource that has no relation to FixedTimestep so we don't confuse users about their respective roles.

Overgeneralizing, fixed updates are for physics/simulation updates, frame updates are for input/rendering.

Yes, this is my rule of thumb as well.

@maniwani maniwani force-pushed the make_fixed_timestep_better branch 3 times, most recently from e09970a to 1441afd Compare March 31, 2022 17:29
@maniwani maniwani mentioned this pull request Apr 1, 2022
7 tasks
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label May 16, 2022
@@ -13,7 +13,7 @@ fn main() {
}

#[derive(Component, Debug)]
struct MyComponent(f64);
struct MyComponent(f32);
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was either change this to be an f32 or use time.seconds_since_startup_f64() below. Since this is a user example, I went with the choice that looked better.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

@maniwani could you split out the relatively uncontroversial changes from this into a smaller PR?

I think the changes to Time (except for relative_speed) can stand alone and will be easy to merge.

bors bot pushed a commit that referenced this pull request May 26, 2022
# Objective

Reduce the catch-all grab-bag of functionality in bevy_core by minimally splitting off time functionality into bevy_time. Functionality like that provided by #3002 would increase the complexity of bevy_time, so this is a good candidate for pulling into its own unit.

A step in addressing #2931 and splitting bevy_core into more specific locations.

## Solution

Pull the time module of bevy_core into a new crate, bevy_time.

# Migration guide

- Time related types (e.g. `Time`, `Timer`, `Stopwatch`, `FixedTimestep`, etc.) should be imported from `bevy::time::*` rather than `bevy::core::*`.
- If you were adding `CorePlugin` manually, you'll also want to add `TimePlugin` from `bevy::time`.
- The `bevy::core::CorePlugin::Time` system label is replaced with `bevy::time::TimeSystem`.

Co-authored-by: Carter Anderson <[email protected]>
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# Objective

Reduce the catch-all grab-bag of functionality in bevy_core by minimally splitting off time functionality into bevy_time. Functionality like that provided by bevyengine#3002 would increase the complexity of bevy_time, so this is a good candidate for pulling into its own unit.

A step in addressing bevyengine#2931 and splitting bevy_core into more specific locations.

## Solution

Pull the time module of bevy_core into a new crate, bevy_time.

# Migration guide

- Time related types (e.g. `Time`, `Timer`, `Stopwatch`, `FixedTimestep`, etc.) should be imported from `bevy::time::*` rather than `bevy::core::*`.
- If you were adding `CorePlugin` manually, you'll also want to add `TimePlugin` from `bevy::time`.
- The `bevy::core::CorePlugin::Time` system label is replaced with `bevy::time::TimeSystem`.

Co-authored-by: Carter Anderson <[email protected]>
@maniwani maniwani force-pushed the make_fixed_timestep_better branch 3 times, most recently from 3f8cdda to 527da04 Compare July 29, 2022 02:37
@maniwani maniwani force-pushed the make_fixed_timestep_better branch from 527da04 to 0d83288 Compare July 29, 2022 02:51
@alice-i-cecile alice-i-cecile added A-Time Involves time keeping and reporting and removed A-Core labels Sep 14, 2022
bors bot pushed a commit that referenced this pull request Oct 22, 2022
# Objective

- Make `Time` API more consistent.
- Support time accel/decel/pause.

## Solution

This is just the `Time` half of #3002. I was told that part isn't controversial.

- Give the "delta time" and "total elapsed time" methods `f32`, `f64`, and `Duration` variants with consistent naming.
- Implement accelerating / decelerating the passage of time.
- Implement stopping time.

---

## Changelog

- Changed `time_since_startup` to `elapsed` because `time.time_*` is just silly.
- Added `relative_speed` and `set_relative_speed` methods.
- Added `is_paused`, `pause`, `unpause` , and methods. (I'd prefer `resume`, but `unpause` matches `Timer` API.)
- Added `raw_*` variants of the "delta time" and "total elapsed time" methods.
- Added `first_update` method because there's a non-zero duration between startup and the first update.

## Migration Guide

- `time.time_since_startup()` -> `time.elapsed()`
- `time.seconds_since_startup()` -> `time.elapsed_seconds_f64()`
- `time.seconds_since_startup_wrapped_f32()` -> `time.elapsed_seconds_wrapped()`

If you aren't sure which to use, most systems should continue to use "scaled" time (e.g. `time.delta_seconds()`). The realtime "unscaled" time measurements (e.g. `time.raw_delta_seconds()`) are mostly for debugging and profiling.
@maniwani
Copy link
Contributor Author

Time scaling added by #5752. I'll add FixedTime in another PR.

@maniwani maniwani closed this Oct 24, 2022
@maniwani maniwani deleted the make_fixed_timestep_better branch October 24, 2022 14:45
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Make `Time` API more consistent.
- Support time accel/decel/pause.

## Solution

This is just the `Time` half of bevyengine#3002. I was told that part isn't controversial.

- Give the "delta time" and "total elapsed time" methods `f32`, `f64`, and `Duration` variants with consistent naming.
- Implement accelerating / decelerating the passage of time.
- Implement stopping time.

---

## Changelog

- Changed `time_since_startup` to `elapsed` because `time.time_*` is just silly.
- Added `relative_speed` and `set_relative_speed` methods.
- Added `is_paused`, `pause`, `unpause` , and methods. (I'd prefer `resume`, but `unpause` matches `Timer` API.)
- Added `raw_*` variants of the "delta time" and "total elapsed time" methods.
- Added `first_update` method because there's a non-zero duration between startup and the first update.

## Migration Guide

- `time.time_since_startup()` -> `time.elapsed()`
- `time.seconds_since_startup()` -> `time.elapsed_seconds_f64()`
- `time.seconds_since_startup_wrapped_f32()` -> `time.elapsed_seconds_wrapped()`

If you aren't sure which to use, most systems should continue to use "scaled" time (e.g. `time.delta_seconds()`). The realtime "unscaled" time measurements (e.g. `time.raw_delta_seconds()`) are mostly for debugging and profiling.
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
# Objective

- Make `Time` API more consistent.
- Support time accel/decel/pause.

## Solution

This is just the `Time` half of bevyengine#3002. I was told that part isn't controversial.

- Give the "delta time" and "total elapsed time" methods `f32`, `f64`, and `Duration` variants with consistent naming.
- Implement accelerating / decelerating the passage of time.
- Implement stopping time.

---

## Changelog

- Changed `time_since_startup` to `elapsed` because `time.time_*` is just silly.
- Added `relative_speed` and `set_relative_speed` methods.
- Added `is_paused`, `pause`, `unpause` , and methods. (I'd prefer `resume`, but `unpause` matches `Timer` API.)
- Added `raw_*` variants of the "delta time" and "total elapsed time" methods.
- Added `first_update` method because there's a non-zero duration between startup and the first update.

## Migration Guide

- `time.time_since_startup()` -> `time.elapsed()`
- `time.seconds_since_startup()` -> `time.elapsed_seconds_f64()`
- `time.seconds_since_startup_wrapped_f32()` -> `time.elapsed_seconds_wrapped()`

If you aren't sure which to use, most systems should continue to use "scaled" time (e.g. `time.delta_seconds()`). The realtime "unscaled" time measurements (e.g. `time.raw_delta_seconds()`) are mostly for debugging and profiling.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Make `Time` API more consistent.
- Support time accel/decel/pause.

## Solution

This is just the `Time` half of bevyengine#3002. I was told that part isn't controversial.

- Give the "delta time" and "total elapsed time" methods `f32`, `f64`, and `Duration` variants with consistent naming.
- Implement accelerating / decelerating the passage of time.
- Implement stopping time.

---

## Changelog

- Changed `time_since_startup` to `elapsed` because `time.time_*` is just silly.
- Added `relative_speed` and `set_relative_speed` methods.
- Added `is_paused`, `pause`, `unpause` , and methods. (I'd prefer `resume`, but `unpause` matches `Timer` API.)
- Added `raw_*` variants of the "delta time" and "total elapsed time" methods.
- Added `first_update` method because there's a non-zero duration between startup and the first update.

## Migration Guide

- `time.time_since_startup()` -> `time.elapsed()`
- `time.seconds_since_startup()` -> `time.elapsed_seconds_f64()`
- `time.seconds_since_startup_wrapped_f32()` -> `time.elapsed_seconds_wrapped()`

If you aren't sure which to use, most systems should continue to use "scaled" time (e.g. `time.delta_seconds()`). The realtime "unscaled" time measurements (e.g. `time.raw_delta_seconds()`) are mostly for debugging and profiling.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Reduce the catch-all grab-bag of functionality in bevy_core by minimally splitting off time functionality into bevy_time. Functionality like that provided by bevyengine#3002 would increase the complexity of bevy_time, so this is a good candidate for pulling into its own unit.

A step in addressing bevyengine#2931 and splitting bevy_core into more specific locations.

## Solution

Pull the time module of bevy_core into a new crate, bevy_time.

# Migration guide

- Time related types (e.g. `Time`, `Timer`, `Stopwatch`, `FixedTimestep`, etc.) should be imported from `bevy::time::*` rather than `bevy::core::*`.
- If you were adding `CorePlugin` manually, you'll also want to add `TimePlugin` from `bevy::time`.
- The `bevy::core::CorePlugin::Time` system label is replaced with `bevy::time::TimeSystem`.

Co-authored-by: Carter Anderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Time Involves time keeping and reporting C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants