Discrete timer and stopwatch #2683

alice-i-cecile · 2021-08-18T19:34:26Z

Objective

Fixes Native Tick Timers #1633.
As expressed in the linked issue, tick timers are a fundamental tool for controlling game-logic in certain game genres (factory builders, tower defenses) where we want to slow down gameplay when frame rate drops and ensure perfect synchronization.
Using absolute time can be too complex and too inexact for many gameplay uses.

Solution

Create a DiscreteStopwatch and DiscreteTimer structs to closely mirror the existing Timer and Stopwatch API.
This allows users to trivially swap between the APIs and reduces learning and maintenance burden.

Directly mirrors existing Stopwatch, but for discrete time step use cases

Mirrors API and impl of Timer

alice-i-cecile · 2021-08-18T19:37:48Z

Code duplication could be reduced somewhat by refactoring these types to use a shared trait. I decided not to for simplicity (there are only two variants, and no obvious extensions), but I'm happy to change that if others feel that I should.

I'm not sure where you would want to accept either a DiscreteTimer or a Timer, but permitting that would be another benefit of a trait-based impl.

DJMcNab · 2021-08-18T19:47:18Z

Is any of this code taken from @maplant's repo? If so, it would be good to make sure we record that properly

(I don't expect that we'll need to get in touch with all contributors again, but I don't want to miss any if we do)

afonsolage · 2021-08-18T19:52:46Z

crates/bevy_core/src/time/stopwatch.rs

+    /// assert_eq!(stopwatch.elapsed(), 1);
+    /// ```
+    #[inline]
+    pub fn unpause(&mut self) {


What about continue in favor of unpause?

I'd probably call that out-of-scope for this PR; I'm just following the existing API :)

(I also slightly prefer unpause, and think resume is also better than continue if we want to change.)

maniwani

A couple doctests weren't updated from the original timer, but other than that LGTM.

crates/bevy_core/src/time/timer.rs

alice-i-cecile · 2021-08-18T20:45:49Z

Is any of this code taken from @maplant's repo? If so, it would be good to make sure we record that properly

Nope, this is written completely from scratch.

mockersf · 2021-08-18T20:46:57Z

Is this really a fundamental feature? I couldn't find something similar in Unity or Godot after a quick look.

As you mentioned, if the goal is to keep the API close to Timer and Stopwatch, they should share a trait with those to ensure that. Otherwise this just adds maintenance complexity...

I would prefer the names (DiscreteStopwatch and DiscreteTimer) to not be related to time at all as it's not using time. Maybe Countdown and Ticker?

maplant · 2021-08-18T20:52:50Z

Is this really a fundamental feature? I couldn't find something similar in Unity or Godot after a quick look.

As you mentioned, if the goal is to keep the API close to Timer and Stopwatch, they should share a trait with those to ensure that. Otherwise this just adds maintenance complexity...

I would prefer the names (DiscreteStopwatch and DiscreteTimer) to not be related to time at all as it's not using time. Maybe Countdown and Ticker?

It is definitely an important, fundamental feature. Unity does not have this because components have an Update method and corresponding Time.deltaTime value. If you search "implement timer in Unity", you will find tick timers. Case in point: https://gamedevbeginner.com/how-to-make-countdown-timer-in-unity-minutes-seconds/

alice-i-cecile · 2021-08-18T20:56:24Z

Is this really a fundamental feature? I couldn't find something similar in Unity or Godot after a quick look.

I think this deserves to be in the engine to provide a consistent, useful API and point beginners in the right direction (rather than forcing everyone to write their own data structures for this constantly).

As you mentioned, if the goal is to keep the API close to Timer and Stopwatch, they should share a trait with those to ensure that. Otherwise this just adds maintenance complexity...

I can do that :) I think it's the right call too; I just didn't want to dive right in before I'd gotten some initial feedback.

I would prefer the names (DiscreteStopwatch and DiscreteTimer) to not be related to time at all as it's not using time. Maybe Countdown and Ticker?

IMO these will almost always be used as direct substitutes of the existing Timer and Countdown. We should try to reflect that in the API, ideally through both traits and naming.

mockersf · 2021-08-18T21:09:05Z

It is definitely an important, fundamental feature. Unity does not have this because components have an Update method and corresponding Time.deltaTime value. If you search "implement timer in Unity", you will find tick timers. Case in point: https://gamedevbeginner.com/how-to-make-countdown-timer-in-unity-minutes-seconds/

This link describes the Timer that already exists in Bevy I think?

IMO these will almost always be used as direct substitutes of the existing Timer and Countdown. We should try to reflect that in the API, ideally through both traits and naming.

I disagree, real time timers will always be needed, if only for any kind of animation or sound

maplant · 2021-08-18T21:33:47Z

It is definitely an important, fundamental feature. Unity does not have this because components have an Update method and corresponding Time.deltaTime value. If you search "implement timer in Unity", you will find tick timers. Case in point: https://gamedevbeginner.com/how-to-make-countdown-timer-in-unity-minutes-seconds/

This link describes the Timer that already exists in Bevy I think?

My mistake, what I am looking for is a FixedUpdate, found here: https://docs.unity3d.com/ScriptReference/MonoBehaviour.FixedUpdate.html This is a tick timer.

IMO these will almost always be used as direct substitutes of the existing Timer and Countdown. We should try to reflect that in the API, ideally through both traits and naming.

I disagree, real time timers will always be needed, if only for any kind of animation or sound

They may be necessary for display and sound given the way your update loop is structured (although I believe http://gameprogrammingpatterns.com/game-loop.html shows that you can do it in one place, and then have a fixed conversion between ticks and real time), however it's not very good to use system timers for gameplay.

If you have a fixed time step, then tick timers are absolutely a direct substitute for system timers. You're never going to update faster than a tick anywhere, so you don't get higher resolution without them.

maniwani · 2021-08-18T21:37:53Z

IMO these will almost always be used as direct substitutes of the existing Timer and Stopwatch. We should try to reflect that in the API, ideally through both traits and naming.

I disagree, real time timers will always be needed, if only for any kind of animation or sound.

I wouldn't say Timer and Stopwatch can be replaced in most instances by these generic/unitless variants, but I think keeping those names in the suffix would help users more easily understand their use. In my mind, these discrete primitives fill the niche of "I want a generic interval" (e.g. run every 3 fixed update steps, every 10 frames, every 4 events, etc.). But I would also be okay with Countdown and Counter/Ticker.

(edit)

My mistake, what I am looking for is a FixedUpdate, found here: docs.unity3d.com/ScriptReference/MonoBehaviour.FixedUpdate.html This is a tick timer.

Also just wanted to say that a fixed timestep is not a timer. They're technically opposites. (I suppose this means that there could also be an integer variant of FixedTimestep, but that seems pretty esoteric.)

FixedTimestep

runs N steps per second
each step represents a fixed amount of time, but the actual time between steps varies

Timer

runs steps with at least N seconds in between
actual steps per second varies, because it does not "catch up" like FixedTimestep does

alice-i-cecile · 2021-08-18T22:24:11Z

IMO these will almost always be used as direct substitutes of the existing Timer and Countdown. We should try to reflect that in the API, ideally through both traits and naming.

I disagree, real time timers will always be needed, if only for any kind of animation or sound

Sorry, my sentence structure was not clear there. The point I was trying to make is that "users will often want to swap back and forth between these types directly", not "one is strictly superior": they both have their niches but fundamentally fill a similar role which their API should reflect.

alice-i-cecile · 2021-08-19T16:48:29Z

So @mockersf I've now refactored this PR to use traits :) I'm not thrilled with the end result, so I'd appreciate some mentorship. Here's my thought process:

There's still a great deal of near-duplicated, non-trivial code.
Some of this is due to getter-setter patterns, and the inability to directly store data on traits in Rust.
We can't store most of the complex, tricky logic within the shared impl of the methods, because it requires APIs such as set_finished that we don't want to expose to the end user (because they'll end up putting their timers in invalid states).
We can't just make these methods internal only, since this trait needs to be pub in order for others to add their own timers and stopwatches. See this discussion or this SO post.
With the addition of some more trait bounds / clever work, I suspect we could use a derive macro for Stopwatch and Timer. This would reduce code duplication, but further increase complexity and make maintenance and optimization harder in the future. And again, I'm not sure that there's a large number of end users clamoring to make their own timers (that still behave fundamentally similarly and would benefit from the macro) once these two exist.

I've also had to rename Timer and Stopwatch to DurationTimer and DurationStopwatch, as Timer and Stopwatch are the obviously correct trait names. Which are clearer, but very heavy on boilerplate.

Upon writing this up this morning, I'm tempted to:

Make the Timer and Stopwatch trait private.
Change the names to something less ergonomic, and keep Timer and Stopwatch as they are on 0.5.
Add more private methods that can break the promises of the public API.
Use those private methods to reduce code duplication further.

Allowing end users to be able to extend the trait and refer to it in their code is nice, but it comes at a high cost.

alice-i-cecile · 2022-01-15T22:41:56Z

Closing out for now. The whole Timer API needs a cohesive redesign.

As mentioned in #2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from #2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes #2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.

) As mentioned in bevyengine#2926, it's better to have an explicit type that clearly communicates the intent of the timer mode rather than an opaque boolean, which can be only understood when knowing the signature or having to look up the documentation. This also opens up a way to merge different timers, such as `Stopwatch`, and possibly future ones, such as `DiscreteStopwatch` and `DiscreteTimer` from bevyengine#2683, into one struct. Signed-off-by: Lena Milizé <me@lvmn.org> # Objective Fixes bevyengine#2926. ## Solution Introduce `TimerMode` which replaces the `bool` argument of `Timer` constructors. A `Default` value for `TimerMode` is `Once`. --- ## Changelog ### Added - `TimerMode` enum, along with variants `TimerMode::Once` and `TimerMode::Repeating` ### Changed - Replace `bool` argument of `Timer::new` and `Timer::from_seconds` with `TimerMode` - Change `repeating: bool` field of `Timer` with `mode: TimerMode` ## Migration Guide - Replace `Timer::new(duration, false)` with `Timer::new(duration, TimerMode::Once)`. - Replace `Timer::new(duration, true)` with `Timer::new(duration, TimerMode::Repeating)`. - Replace `Timer::from_seconds(seconds, false)` with `Timer::from_seconds(seconds, TimerMode::Once)`. - Replace `Timer::from_seconds(seconds, true)` with `Timer::from_seconds(seconds, TimerMode::Repeating)`. - Change `timer.repeating()` to `timer.mode() == TimerMode::Repeating`.

alice-i-cecile added 2 commits August 18, 2021 14:58

Added DiscreteStopwatch

29a6690

Directly mirrors existing Stopwatch, but for discrete time step use cases

Added DiscreteTimer

ac222fd

Mirrors API and impl of Timer

alice-i-cecile added A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible labels Aug 18, 2021

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 18, 2021

afonsolage reviewed Aug 18, 2021

View reviewed changes

maniwani reviewed Aug 18, 2021

View reviewed changes

crates/bevy_core/src/time/timer.rs Outdated Show resolved Hide resolved

crates/bevy_core/src/time/timer.rs Outdated Show resolved Hide resolved

Fixed overlooked doc tests

a0f4cf7

alice-i-cecile force-pushed the discrete-timer branch from 7f3d886 to a0f4cf7 Compare August 18, 2021 20:50

Refactor stopwatches to use a shared trait

c0e39d5

mockersf removed the S-Needs-Triage This issue needs to be labelled label Aug 19, 2021

Refactored timers to use shared trait

5027bb3

alice-i-cecile added the S-Needs-Design This issue requires design work to think about how it would best be accomplished label Sep 22, 2021

arialpew mentioned this pull request Oct 7, 2021

Refactor away the bool argument of Timer #2926

Closed

maniwani mentioned this pull request Oct 22, 2021

Add time scaling and simplify fixed timestep use #3002

Closed

alice-i-cecile closed this Jan 15, 2022

lovelymono mentioned this pull request Oct 13, 2022

[Merged by Bors] - Replace the bool argument of Timer with TimerMode #6247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discrete timer and stopwatch #2683

Discrete timer and stopwatch #2683

alice-i-cecile commented Aug 18, 2021

alice-i-cecile commented Aug 18, 2021

DJMcNab commented Aug 18, 2021

afonsolage Aug 18, 2021

alice-i-cecile Aug 18, 2021

maniwani left a comment

alice-i-cecile commented Aug 18, 2021

mockersf commented Aug 18, 2021

maplant commented Aug 18, 2021

alice-i-cecile commented Aug 18, 2021

mockersf commented Aug 18, 2021 •

edited

Loading

maplant commented Aug 18, 2021 •

edited

Loading

maniwani commented Aug 18, 2021 •

edited

Loading

alice-i-cecile commented Aug 18, 2021

alice-i-cecile commented Aug 19, 2021

alice-i-cecile commented Jan 15, 2022

Discrete timer and stopwatch #2683

Discrete timer and stopwatch #2683

Conversation

alice-i-cecile commented Aug 18, 2021

Objective

Solution

alice-i-cecile commented Aug 18, 2021

DJMcNab commented Aug 18, 2021

afonsolage Aug 18, 2021

Choose a reason for hiding this comment

alice-i-cecile Aug 18, 2021

Choose a reason for hiding this comment

maniwani left a comment

Choose a reason for hiding this comment

alice-i-cecile commented Aug 18, 2021

mockersf commented Aug 18, 2021

maplant commented Aug 18, 2021

alice-i-cecile commented Aug 18, 2021

mockersf commented Aug 18, 2021 • edited Loading

maplant commented Aug 18, 2021 • edited Loading

maniwani commented Aug 18, 2021 • edited Loading

alice-i-cecile commented Aug 18, 2021

alice-i-cecile commented Aug 19, 2021

alice-i-cecile commented Jan 15, 2022

mockersf commented Aug 18, 2021 •

edited

Loading

maplant commented Aug 18, 2021 •

edited

Loading

maniwani commented Aug 18, 2021 •

edited

Loading