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

[Merged by Bors] - Add global time scaling #5752

Closed
wants to merge 5 commits into from

Conversation

maniwani
Copy link
Contributor

@maniwani maniwani commented Aug 20, 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 maniwani added A-Core Common functionality for all bevy apps 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 labels Aug 20, 2022
@maniwani maniwani requested a review from IceSentry August 20, 2022 20:39
@DJMcNab
Copy link
Member

DJMcNab commented Aug 20, 2022

I feel like being able to scale time is likely to be an attractive nuisance.

What use cases do you foresee for it?

@maniwani
Copy link
Contributor Author

maniwani commented Aug 20, 2022

What use cases do you foresee for it?

If you want to speed up or slow down a simulation running on a fixed timestep, the proper way to do it is to scale the time fed into the accumulator.

Simulating steps at a different frequency like this will not affect time integration whereas changing the step size itself would.

Edit: also see:
https://docs.godotengine.org/en/stable/classes/class_engine.html#class-engine-property-time-scale
https://docs.unity3d.com/ScriptReference/Time-timeScale.html
https://docs.unrealengine.com/5.0/en-US/BlueprintAPI/Utilities/Time/
https://www.o3de.org/docs/api/frameworks/azcore/class_a_z_1_1_i_time.html

crates/bevy_time/src/time.rs Outdated Show resolved Hide resolved
crates/bevy_time/src/time.rs Outdated Show resolved Hide resolved
/// # Panics
///
/// Panics if `ratio` is negative or not finite.
pub fn set_relative_speed(&mut self, ratio: f32) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about set_time_scale?
Unity and Godot call it time scale and Unreal calls it time dilation.

Copy link
Contributor Author

@maniwani maniwani Aug 20, 2022

Choose a reason for hiding this comment

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

I only referenced other engines to show the universal presence of the feature. IMO they've all named it incorrectly.

Increasing time dilation or scale would mean lengthening each unit of time (i.e. slowing things down), which is the inverse of how we actually apply the value (i.e. if dilation is 1.1x, time should pass 1.0/1.1 slower, but this PR and other engines just go 1.1x faster).

I think "relative speed" is the most literal term here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, I suppose you're right. Though I'm not sure being correct is more valuable than the option being recognizable.

Copy link
Contributor

@IceSentry IceSentry Aug 21, 2022

Choose a reason for hiding this comment

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

Might be worth adding a line saying something like "Sometimes referred to as time scaling or time dilation in other engines". To help discoverability of users coming from those engines. I do prefer relative speed personally.

crates/bevy_time/src/time.rs Outdated Show resolved Hide resolved
crates/bevy_time/src/time.rs Outdated Show resolved Hide resolved
@maniwani maniwani changed the title Add time scaling Add global time scaling Aug 20, 2022
crates/bevy_time/src/time.rs Show resolved Hide resolved
crates/bevy_time/src/time.rs Outdated Show resolved Hide resolved
crates/bevy_time/src/time.rs Outdated Show resolved Hide resolved
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.

LGTM. I think the uncontroversial parts were only the changes without any scaling but I think the scaling is worth adding either way.


/// Returns the [`Instant`] when [`update`](Self::update) was first called, if it exists.
#[inline]
pub fn first_update(&self) -> Option<Instant> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a fallible variant? A lot of bevy apis have a get_* variant that returns the option and the one without get unwraps it. I'm just curious if the expected usage will end up with people unwrapping it more often than not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, idk. I'll defer to the other reviewers.

@DJMcNab DJMcNab added the X-Controversial There is active debate or serious implications around merging this PR label Aug 21, 2022
#[inline]
pub fn startup(&self) -> Instant {
self.startup
pub fn elapsed_seconds(&self) -> f32 {
Copy link
Contributor

@tim-blackbird tim-blackbird Aug 22, 2022

Choose a reason for hiding this comment

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

On Timer/Stopwatch this method is called elapsed_secs.
We should choose one or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Yeah, elapsed_secs would better match the existing API.

Can other reviewers chime in here?
We'd also have to change delta_seconds to delta_secs, so that's more API breakage.

Also, are we trying to follow std::time as convention?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer using seconds but I do think there's value in following what the rust std does.

@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 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 labels Sep 1, 2022
@brandon-reinhart
Copy link
Contributor

This looks good.

I feel like being able to scale time is likely to be an attractive nuisance.

What use cases do you foresee for it?

It's great for observing small changes in motion re: physics, looking at misbehavior in particle effects and animation. It's a helpful tool when you see something is wrong, but you can't pin it down because the problem passes too quickly. It can also be helpful to fast forward time when debugging time based scripts (conversations, events, etc) when you don't want to inject system specific skips or what not.

@DJMcNab
Copy link
Member

DJMcNab commented Sep 1, 2022

Sure, I can accept its utility for debugging purposes.

But you appreciate that wasn't my concern. I do see the use case of "during conversations, stop the world"

However, my concern there is what time does e.g. the conversation text animation use then? Because you might want it to scale for debugging as well, but obviously at a different rate than the global 0. (you could do one letter every 3 frames or whatever if you don't care about framerate independence, but then you'd get really bad UX on the outliers)

My concern is that making this a global setting (again, for non-debugging scenarios) means that every situation needs to decide whether it will use unscaled or scaled time, without any context about what the scaling is used for in the actual game.

Could we expose an easy way for people to make their own 'context specific' scaled times? I suppose that would make the FixedTimeStep API more complicated to select which time to use. I'm not sure if there are any downsides to that I'm not seeing.

@maniwani
Copy link
Contributor Author

maniwani commented Sep 1, 2022

Sure, I can accept its utility for debugging purposes.

I didn't make this PR to just help people write tests though. Global time scaling can be used for actual gameplay.

Every situation needs to decide whether it will use unscaled or scaled time.

I think you're blowing this out of proportion.

My concern there is what time does e.g. the conversation text animation use?

Let's rename scaled time to "game time" and unscaled time to "wall time" for a second. Everything "in-game" should follow the game clock. Anything else can follow the wall clock.

Dialogue is an in-game thing, so animations for conversation text should be driven by game time. If pausing your app stops the game clock (which isn't always the case), any animations for pause menu widgets and text should use wall time since you're not "in-game" anymore.

Because you might want it to scale for debugging as well, but obviously at a different rate than the global 0.

I don't think you'd ever be juggling a bunch of different relative speeds unless you're full-on building some localized time dilation mechanic (like in this game) using components.

[Can we do time scaling non-globally?]

Yes, it's possible to impl a form of time scaling local to the fixed timestep itself.

But if you want them decoupled like that, we can't use Time to know what time it is inside the fixed timestep context, which we need to know so we can play animations that are tied to that data.

If time fed into the fixed timestep accumulator can be scaled independently of Time, we need a synonymous FixedTime clock (like in #3002) to track how much time has advanced.

I'd actually like to impl that in addition to global time scaling. I do think there will be times when you'd broadly want the gameplay to speed up and not the in-game UI. I think we should have both mechanisms to cover all uses (but if we can only have one, I'm inclined to follow the other engines and do it globally).


Does anyone have an example usage of time scaling they can share that doesn't involve fixed timestep logic or draw from that data? Snippets from something made with another engine would be good. Would help resolve this argument.

@alice-i-cecile
Copy link
Member

Global time scaling can be used for actual gameplay.

I see this used aggressively in both tower defenses and simulation genres: in both cases there's commonly a "double speed" setting.

I'd actually like to impl that in addition to global time scaling. I do think there will be times when you'd broadly want the gameplay to speed up and not the in-game UI. I think we should have both mechanisms to cover all uses (but if we can only have one, I'm inclined to follow the other engines and do it globally).

Agreed, but I don't think that needs to be merged at the same time.

@alice-i-cecile alice-i-cecile added A-Time Involves time keeping and reporting and removed A-Core Common functionality for all bevy apps labels Sep 14, 2022
@IceSentry
Copy link
Contributor

IceSentry commented Sep 14, 2022

So, after working on #5982 a bit. I think all the f32 variants of elapsed seconds should be removed in favor of the wrapped variants if they need an f32 version of elapsed time.

@maniwani
Copy link
Contributor Author

maniwani commented Sep 14, 2022

So, after working on #5982 a bit. I think all the f32 variants of elapsed seconds should be removed in favor of the wrapped variants if they need an f32 version of elapsed time.

Can you share some context? I don't really know what you're saying.

@IceSentry
Copy link
Contributor

IceSentry commented Sep 14, 2022

Sorry, I should have been clearer. f32 aren't precise enough to store time over long periods of time. I don't remember the exact numbers right now but it starts losing precision in a matter of a few hours or maybe a day. It's definitely in a timescale that will be just long enough to be hard to detect while still being at a human timescale. f64 would last long enough that it doesn't matter. This isn't a massive issue, but people will leave their app open for a long time and it will cause issues if an unwrapped f32 is used anywhere. By wrapped I just mean that wraps to 0 after a certain period of time. For example. godot wraps back to 0 after an hour.

Which is why I believe it's simply safer if we don't have elapsed_seconds api that returns f32 that don't wrap.

@alice-i-cecile
Copy link
Member

Which is why I believe it's simply safer if we don't have elapsed_seconds api that returns f32 that don't wrap.

Agreed, I don't think we should expose these. elapsed_seconds should return a Duration, and we should explicitly warn against converting it to an f32.

@maniwani
Copy link
Contributor Author

maniwani commented Sep 14, 2022

Overall, I think this topic is orthogonal to this PR. We simply extend the current behavior.

elapsed_seconds should return a Duration

@alice-i-cecile, the elapsed method already returns a Duration.

f32 precision loss

So if I understand correctly, the problem is that after about 9 hours, f32 can no longer represent elapsed time to millisecond precision, and it gets worse from there.

You can see it in this table showing f32 and f64 fractional precision over multiple ranges.

table

source

Godot

Could you share a link where Godot posted about or its contributors talk about making elapsed time wrap around?

I agree we should at least document this, but just removing the method that returns an f32 won't do anything. Users will just take the f64 or Duration and convert it.

Again, I don't think it's relevant to this PR, but I don't like the suggestion about having elapsed time wrap around. It's unintuitive and seems like it would burden all users with having to consider this edge case when only some will run into it.

I know many applications like shaders are stuck with 32-bit math, but in those cases, couldn't you do one of these?

  • Perform the modulo yourself on the Duration or f64 value, then convert the result into an f32.
  • Use a Timer to count from a closer reference point instead of the app's startup time.

@IceSentry
Copy link
Contributor

IceSentry commented Sep 14, 2022

My PR (#5982) adds the method to do the modulo conversion directly on Time because shaders aren't necessarily the only place where this is required.

My point is just that I think the non-wrapped f32 version are pretty much just footguns waiting to happen. If people need an f32 for whatever reason, gpu math or otherwise, using a cast will cause issue eventually and wrapping won't. I guess the api could still exist, but I'd rather point users in the direction of doing the right thing if they look for an f32 variant of elapsed_seconds. They don't need to think about it if we offer clearly documented apis to deal with it.

Here's what I would propose, I think the default should be f64 and the f32 variant should either be removed entirely or very clearly labeled as not recommended if not wrapped. Removed or not, a wrapped variant should exist and it should be clearly marked as such. The wrapped version doesn't need to be implemented in this PR, but that's what I believe the api should end up being. My goal is mostly to make sure that people not wrapping are aware of the potential issues as much as possible.

Edit: Thinking about it more, I think my main issue is actually making f32 the default instead of f64. At least if the default is f64 it will hint at the fact that an f32 representation isn't ideal. The wrap or not to wrap issue isn't really related to this PR, it was just the context I had when thinking about this.

@maniwani
Copy link
Contributor Author

A value that is wrong due to precision error isn't really it's true value either.

Unless you're suggesting we never let anyone observe a non-wrapping elapsed time (which again is outside the scope of this PR), IMO it's better to have the f32 method so that we can formally document its precision loss and have it log a warning when the value exceeds some threshold.

Like, the hypothetically illiterate users you speak of wouldn't read about a wrapped value method either. They'd just make an f32 by converting the f64 or Duration and file issues complaining about the lack of API symmetry.

I think it's pointless to have an inconsistent, confusing API when it doing so would, at best, slightly reduce the frequency that users have issues with precision loss. If mistakes can be made regardless, in the end we can only highlight and direct users to the correct methods (wherever they end up).

@IceSentry
Copy link
Contributor

I feel like we are talking past each other a little, but either way. I'm completely fine with just adding clear docs about precision loss issues and I'll leave my approval of this PR because I approve of every other change too. (Assuming conflicts are fixed and docs updated as discussed of course)

I just wish it was possible to make it even more obvious that f32 should probably not be used in most cases, but it doesn't seem possible without weird apis 😢

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 19, 2022
@alice-i-cecile
Copy link
Member

@maniwani, I'm comfortable merging this in approximately this form; are you able to rebase?

@alice-i-cecile
Copy link
Member

bors r+

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.
@bors bors bot changed the title Add global time scaling [Merged by Bors] - Add global time scaling Oct 22, 2022
@bors bors bot closed this Oct 22, 2022
@maniwani maniwani deleted the time-scaling branch October 22, 2022 19:25
bors bot pushed a commit that referenced this pull request Oct 24, 2022
Quick follow-up to #5752. I think this is a slightly better wording.
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.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
Quick follow-up to bevyengine#5752. I think this is a slightly better wording.
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.
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
Quick follow-up to bevyengine#5752. I think this is a slightly better wording.
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
Quick follow-up to bevyengine#5752. I think this is a slightly better wording.
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants