[Merged by Bors] - ♻️ `Timer` refactor to duration.✨ Add `Stopwatch` struct. #1151

kokounet · 2020-12-25T22:44:35Z

This pull request is following the discussion on the issue #1127. Additionally, it integrates the change proposed by #1112.

The list of change of this pull request:

✨ Add Timer::times_finished method that counts the number of wraps for repeating timers.
♻️ Refactored Timer
🐛 Fix a bug where 2 successive calls to Timer::tick which makes a repeating timer to finish makes Timer::just_finished to return false where it should return true. Minimal failing example:

use bevy::prelude::*;
let mut timer: Timer<()> = Timer::from_seconds(1.0, true);
timer.tick(1.5);
assert!(timer.finished());
assert!(timer.just_finished());
timer.tick(1.5);
assert!(timer.finished());
assert!(timer.just_finished()); // <- This fails where it should not

📚 Add extensive documentation for Timer with doc examples.
✨ Add a Stopwatch struct similar to Timer with extensive doc and tests.

Even if the type specialization is not retained for bevy, the doc, bugfix and added method are worth salvaging 😅.
This is my first PR for bevy, please be kind to me ❤️ .

Refactor Timer to have Type parameter

DJMcNab · 2020-12-25T22:57:31Z

My initial thoughts are that it all looks good, apart from the type parameter to Timer, which seems downright bizarre

I think the rest of the changes would be very welcome, pending full code review

Edit: I appreciate your use of emoji too :)

kokounet · 2020-12-25T23:13:46Z

I'm missing the implementation of Reflect for std::marker::PhantomData<T>. It's all todo!() for now which is far from ideal, though I'm not sure how to implement Reflect for this bizarre type.

the type parameter to Timer, which seems downright bizarre

I totally agree, this is something wierd. However, there are good motivations to explore this. The idea behind it is to prevent user from having to do this:

pub MyTimer(pub Timer); // this could be avoided as well in many cases

fn main() {
    let mut timer = MyTimer(Timer::from_seconds(1.0, false));
    timer.0.tick(1.0); // <- this `.0` is a common boilerplate to all specialized timers
}

This way they can attach multiples timers to an entity without this NewType pattern, and they're all equally easy to use. It's a fairly common use, even the 0.4 blog post points that out:

Timer Components also no longer tick by default. Timer resources and newtyped Timer components couldn't tick by default, so it was a bit inconsistent to have the (relatively uncommon) "unwrapped component Timer" auto-tick.

Though it makes some things harder as pointed out in the issue:

Deriving traits do not work that well when there is a generic type param

Which is a totally understandable concern to be honest.
And it makes Reflect more difficult as well...

mockersf · 2020-12-25T23:28:43Z

Hey there!

I don't like all those Timer<()> in examples, but not sure of the best way forward, that's (one of) the reason I didn't continue working on this:

either we consider that the associated type should be () if the user doesn't care, and in this case there should be a way to set it as default so that they don't have to think about it
or we want the user to be explicit every time about the associated type, and then we shouldn't show examples with () but rework them all to have an explicit type used that has a meaningful name

Also the issue with registering the Timer<...>type means that timers become harder to use in a scene without the user doing something extra, which I'm not a fan of...

mockersf · 2020-12-25T23:32:19Z

crates/bevy_core/src/time/stopwatch.rs

+/// ```
+#[derive(Debug, Clone, Reflect)]
+#[reflect(Component)]
+pub struct Stopwatch<T: Send + Sync + 'static = ()> {


the = () should mean that the type can be () by default but I didn't get it to actually work in the short time I tried. I don't think it should be kept if it doesn't work or we don't want a default

Yes I'm not sure of what it does either. It seems that this feature is not intended for using the type but rather implementing it.
I'm going to remove them then.

mockersf · 2020-12-25T23:34:11Z

examples/ecs/timers.rs

@@ -32,12 +32,12 @@ impl Default for Countdown {

 fn setup_system(commands: &mut Commands) {
    // Add an entity to the world with a timer
-    commands.spawn((Timer::from_seconds(5.0, false),));
+    commands.spawn((Timer::<Entity>::from_seconds(5.0, false),));


I especially don't like using Entity as the associated type, I find it confusing

kokounet · 2020-12-26T00:08:44Z

I don't like all those Timer<()> in examples.

Yeah me neither, the thing is that with such small scoped examples the Timer was already be used without the NewType pattern.
In both case I think the example should show an idiomatic use of this struct...

Also the issue with registering the Timer<...>type means that timers become harder to use in a scene without the user doing something extra, which I'm not a fan of...

Oh yeah, I didn't have that in mind at all! It makes this solution far less desirable indeed

alice-i-cecile · 2020-12-26T20:45:21Z

As I'm working with system-local resources (i.e. Local) I'm increasingly wishing that Timer was just a trait that I could derive or implement for each separate system. Would that be a feasible direction to take this redesign?

Here's the motivating example: https://github.com/alice-i-cecile/understanding-bevy/blob/main/src/ecs/communication/resources_code/examples/system_local_resources.rs

alice-i-cecile · 2020-12-26T21:08:14Z

Our whole Time and Timer API and related should also very much be using a Duration type, rather than an f32. f32s lose precision as they increase, resulting in progressively worse time resolution as the game runs for longer. After half an hour of game time, you only have millisecond precision.

Duration also seems to support other useful edge cases and have nice utility functions (and provides for a better interface with other libraries).

kokounet · 2020-12-26T21:30:39Z

As I'm working with system-local resources (i.e. Local) I'm increasingly wishing that Timer was just a trait that I could derive or implement for each separate system. Would that be a feasible direction to take this redesign?

It seems confusing to make Timer a trait, much more than having it with a type parameter. Especially since systems in bevy are simple functions for the user. What I find bothering for local timers for systems is that there is no way of initializing them (or I didn't find how). A simple untyped timer would be enough for the use-case of a local system resource. It also makes the input and dependency of the system clearer. Prehaps the solution is just a simple macro for implementing Deref for NewTyped timers?

Our whole Time and Timer API and related should also very much be using a Duration type.

I agree, it's a minor change though.

Edit: I made this PR a draft because it needs more discussion clearly 😅

alice-i-cecile · 2020-12-27T02:14:40Z

It seems confusing to make Timer a trait, much more than having it with a type parameter

You may very well be right here. I was thinking an API like:

#[derive(Timer)]
struct SlowTimer;

which gives you access to the various timer methods on SlowTimer , but that is a bit magic. The Deref impl for NewTyped timers is pretty easy, even without a macro: I suppose I'm more worried that it throws a relatively advanced (and very implicit) Rust feature right in the faces of beginners: Timers are one of the very first things most examples will introduce.

Upon reflection, a parametrized Timer type is definitely my preferred solution from an ergonomic perspective: what are the blockers on that? I read through the thread and commits but couldn't really grok it.

Lythenas · 2020-12-27T12:40:41Z

Note: This is maybe only slightly related to this PR but might be worth keeping in mind.

The most common use case for timers I have is to run systems every x seconds. As I see it there are two ways to do this:

Take Time and ResMut<Wrapped(Timer)> as parameter and initialize it correctly as a resource. Then tick it in the system and check if it just_finished. If not return.
Use the SystemStage.with_run_criteria with FixedTimestep to add a system to it's own stage.

What I would like to do is something like:

#[bevy::once_every(5.0)]
fn my_system() {
  // this system is run every 5 seconds
}

With the current Timer implementation this would need to generate a new wrapper type for every system. But I think this is unavoidable until const generics with f32 work. It could desugar to something like this:

struct Wrapper(Timer);
impl Default for Wrapper {
  fn default() -> Self {
    Wrapper(Timer::from_seconds(5.0 /* the parameter */, true)
  }
}

fn my_system(time: Res<Time>, mut timer: Local<Wrapper>, /* original parameters */) {
  if !timer.0.tick(time.delta_seconds()).just_finished() {
    return;
  }
  /* original body */
  // this system is run every 5 seconds
}

The first part of the desugaring (the wrapper and default impl) could also be another macro. That would probably solve a lot of other use cases.

multun · 2020-12-27T13:20:01Z

I'm not sure a type parameter is much better. I've seen this idiom in C++ (I've heard people call these type tags), but the newtype idiom is really the rust standard. Neither of those are great, but until language support comes in to solve the issue, I'd rather stick to the most common option

Ratysz · 2020-12-27T13:39:51Z

[...] two ways to do this [...]

There will be a third once #1144 lands: system sets. You'll be able to put my_system into its own SystemSet with a FixedTimestep run criterion - it'll be seamlessly scheduled to run together with other systems of the stage the set belongs to.

I'm also opposed to plumbing in a type parameter into the stock timer. Local resource is the intended solution for system-local timers, and marker components alongside component timers (like in the contributors example) is the preferred pattern for this kind of differentiation in entities. For complex cases where you need multiple timers in a single entity you'd be better off building a bespoke solution out of stock timers - a monolithic struct, or a set of newtypes, with the help any one of the multitude of crates that provide #[derive(Deref, DerefMut)] if need be.

kokounet · 2020-12-27T14:12:58Z

The most common use case for timers I have is to run systems every x seconds.

Is that really the case ? I find that timers as components are also a pretty common use case.

marker components alongside component timers (like in the contributors example) is the preferred pattern for this kind of differentiation in entities.

I would agree, but this doesn't allow for an entity to have multiple timers for different usage.
The solution I prefer currently is just to have some sugar added for NewTyped timers, as it is the most flexible and the most transparent.

Lythenas · 2020-12-27T15:00:16Z

The most common use case for timers I have is to run systems every x seconds.

Is that really the case ? I find that timers as components are also a pretty common use case.

You're right. It's probably not the most common case. In my mind other cases are more "custom" and I think this case should definitely be made as easy as possible. But maybe there are other (more efficient at runtime) ways than my example with a proc macro. But I think this is getting a bit off topic.

List of changes: * Change `tick` now takes a duration as `delta` * Add `tick_f32`, `elapsed_f32` and `duration_f32` methods. * Update examples

kokounet · 2020-12-27T23:01:14Z

So, I changed the Timer internals as well as the Stopwatch and Cooldown to use duration as suggested by @alice-i-cecile.
It makes the code cleaner to use with Time:

// previously
timer.tick(time.delta_seconds());

// with the refactor
timer.tick(time.delta());

This is a breaking change though.

For cases where we want to interact with f32 instead of Duration, I added the *_f32 methods (for tick, elapsed, duration).

I realize now that the code for Cooldown and Timer is so similar it's painful, these 2 structs can be factored out into one common slightly more general internal struct.

For complex cases where you need multiple timers in a single entity you'd be better off building a bespoke solution out of stock timers - a monolithic struct, or a set of newtypes, with the help any one of the multitude of crates that provide #[derive(Deref, DerefMut)] if need be.

That's a very reasonable proposal! I'm wondering if we want the user to use such libraries if they want, or do we want to include a helper in bevy only for timers in order to smooth things out for the user.

Okay so next thing I'll do will be removing the type parameter. I would be happy to hear what you all think about the other changes in this PR, to see if they are welcome in the engine or not 😅.

Lythenas · 2020-12-28T11:58:10Z

I think the change to Duration is good. But I don't like the *_f32 names. *_seconds would be better but is maybe a little long. So maybe *_secs (Duration has from_secs)? At the very least I think it should be documented what the f32 means (even if it is the current behavior).

cart · 2020-12-28T21:23:47Z

Some high level thoughts:

Love the move to Duration
While the Timer<T> generic cuts down on boilerplate for "raw timers", for the reasons everyone has already discussed I don't think its worth it. I'd rather roll with the simpler approach. Its friendlier for scenes, makes the api easier to consume, and prevents the need for Timer<()> everywhere. I expect the more complicated cases to require more than a newtype anyway.
It seems like Timer can be used in place of Cooldown in almost every case. I think Cooldown should be removed and Timer should be extended if its missing features (but I don't think its missing anything critical).
Stopwatch is a nice addition. I also dig that Timer uses it internally
Not a huge fan of the *_f32 methods on Timer. I'd prefer to just use Duration in most cases, then add things like timer.elapsed_secs() for convenience. I think elapsed_secs is definitely useful, but I might want to hold off on things like tick_secs until it becomes clear that its actually useful in practice.

cart · 2021-02-06T22:01:13Z

Any updates on this? I'm down to include the changes in Bevy 0.5 if someone is willing to make the adjustments above.

kokounet · 2021-02-07T00:12:48Z

Yeah surely! I will continue on this, probably in the following days

rename *_f32 methods to *_secs methods

cart

Looks good to me. Just a couple of nits

cart · 2021-03-03T23:05:00Z

crates/bevy_core/src/time/stopwatch.rs

+use bevy_utils::Duration;
+
+/// A Stopwatch is a struct that track elapsed time when started.
+/// It requires a type `T` in order to be specialized for your systems and components.


This is no longer true

cart · 2021-03-03T23:07:34Z

crates/bevy_core/src/time/timer.rs

-    pub fn finished(&self) -> bool {
-        self.finished
+    pub fn set_elapsed(&mut self, time: Duration) {
+        self.stopwatch.set(time);


These method names should probably be consistent. I think set_elapsed is clearer

kokounet · 2021-03-04T18:15:07Z

Yup! I fixed that, if there is anything more let me know 👍

cart · 2021-03-05T18:51:05Z

bors r+

This pull request is following the discussion on the issue #1127. Additionally, it integrates the change proposed by #1112. The list of change of this pull request: * 💥 Change `Timer` to `Timer<T>` in order to make specialization very explicit to the user, while removing the boilerplated NewType pattern for Timers. This is a breaking change * ✨ Add `Timer::times_finished` method that counts the number of wraps for repeating timers. * ♻️ Refactored `Timer` * 🐛 Fix a bug where 2 successive calls to `Timer::tick` which makes a repeating timer to finish makes `Timer::just_finished` to return `false` where it should return `true`. Minimal failing example: ```rust use bevy::prelude::*; let mut timer: Timer<()> = Timer::from_seconds(1.0, true); timer.tick(1.5); assert!(timer.finished()); assert!(timer.just_finished()); timer.tick(1.5); assert!(timer.finished()); assert!(timer.just_finished()); // <- This fails where it should not ``` * 📚 Add extensive documentation for Timer with doc examples. * ✨ Add `Cooldown` and `Stopwatch` structs similar to `Timer` with extensive doc and tests. Even if the type specialization is not retained for bevy, the doc, bugfix and added method are worth salvaging 😅. This is my first PR for bevy, please be kind to me ❤️ . Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2021-03-05T18:53:41Z

Build failed:

build-wasm (stable, ubuntu-latest)

kokounet · 2021-03-05T19:37:56Z

Hum... What should I do about that?

mockersf · 2021-03-05T19:42:21Z

bors is doing a rebase and rerun tests before merging... to reproduce, could you rebase your pr?

cart · 2021-03-05T19:43:30Z

Sorry I resolved the merge conflict, but a path change on main broke another file. I'm pushing the fix now.

DJMcNab · 2021-03-05T19:43:40Z

Or you should be able to check out 6de8c03, somehow

cart · 2021-03-05T19:44:27Z

bors r+

This pull request is following the discussion on the issue #1127. Additionally, it integrates the change proposed by #1112. The list of change of this pull request: * ✨ Add `Timer::times_finished` method that counts the number of wraps for repeating timers. * ♻️ Refactored `Timer` * 🐛 Fix a bug where 2 successive calls to `Timer::tick` which makes a repeating timer to finish makes `Timer::just_finished` to return `false` where it should return `true`. Minimal failing example: ```rust use bevy::prelude::*; let mut timer: Timer<()> = Timer::from_seconds(1.0, true); timer.tick(1.5); assert!(timer.finished()); assert!(timer.just_finished()); timer.tick(1.5); assert!(timer.finished()); assert!(timer.just_finished()); // <- This fails where it should not ``` * 📚 Add extensive documentation for Timer with doc examples. * ✨ Add a `Stopwatch` struct similar to `Timer` with extensive doc and tests. Even if the type specialization is not retained for bevy, the doc, bugfix and added method are worth salvaging 😅. This is my first PR for bevy, please be kind to me ❤️ . Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2021-03-05T19:46:22Z

Build failed:

build-wasm (stable, ubuntu-latest)

DJMcNab · 2021-03-05T19:47:44Z

Fairlure seems to be spurious.

@bors retry

(that might be not-@cart blocked, although it probably is)

cart · 2021-03-05T19:49:13Z

hmm maybe you need to write bors retry instead of @bors retry?

DJMcNab · 2021-03-05T19:51:21Z

bors retry

bors · 2021-03-05T19:51:23Z

🔒 Permission denied

Existing reviewers: click here to make DJMcNab a reviewer

cart · 2021-03-05T19:58:52Z

haha oh well

cart · 2021-03-05T19:58:59Z

bors r+

This pull request is following the discussion on the issue #1127. Additionally, it integrates the change proposed by #1112. The list of change of this pull request: * ✨ Add `Timer::times_finished` method that counts the number of wraps for repeating timers. * ♻️ Refactored `Timer` * 🐛 Fix a bug where 2 successive calls to `Timer::tick` which makes a repeating timer to finish makes `Timer::just_finished` to return `false` where it should return `true`. Minimal failing example: ```rust use bevy::prelude::*; let mut timer: Timer<()> = Timer::from_seconds(1.0, true); timer.tick(1.5); assert!(timer.finished()); assert!(timer.just_finished()); timer.tick(1.5); assert!(timer.finished()); assert!(timer.just_finished()); // <- This fails where it should not ``` * 📚 Add extensive documentation for Timer with doc examples. * ✨ Add a `Stopwatch` struct similar to `Timer` with extensive doc and tests. Even if the type specialization is not retained for bevy, the doc, bugfix and added method are worth salvaging 😅. This is my first PR for bevy, please be kind to me ❤️ . Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2021-03-05T20:15:39Z

Pull request successfully merged into main.

Build succeeded:

Add Cooldown and Stopwatch struct

a6d7361

Refactor Timer to have Type parameter

Implement Reflect for PhantomData

9ff36b8

mockersf reviewed Dec 25, 2020

View reviewed changes

kokounet added 2 commits December 26, 2020 00:47

Fix doctest

f03f1ad

Remove unused default type () for Timer

7f606c3

kokounet added 2 commits December 26, 2020 01:09

Revert timer example to the previous one

168e542

Fix timer example

c1ab350

Moxinilian added A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible labels Dec 26, 2020

kokounet marked this pull request as draft December 26, 2020 21:35

Change Timer internals to use Duration

79714f1

List of changes: * Change `tick` now takes a duration as `delta` * Add `tick_f32`, `elapsed_f32` and `duration_f32` methods. * Update examples

Revert type parameter for Timer

d313ff9

rename *_f32 methods to *_secs methods

kokounet marked this pull request as ready for review February 7, 2021 12:15

alice-i-cecile added help wanted S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 17, 2021

Base automatically changed from master to main February 19, 2021 20:44

cart reviewed Mar 3, 2021

View reviewed changes

rename Stopwatch::set to set_elapsed + fix doc

e7448cd

Merge branch 'main' into master

6ed9116

fix import

b467f85

bors bot changed the title ~~♻️ Timer refactor to duration.✨ Add Stopwatch struct.~~ [Merged by Bors] - ♻️ Timer refactor to duration.✨ Add Stopwatch struct. Mar 5, 2021

bors bot closed this Mar 5, 2021

alice-i-cecile mentioned this pull request Mar 9, 2021

0.4 -> 0.5 Migration Guide #1601

Closed

[Merged by Bors] - ♻️ Timer refactor to duration.✨ Add Stopwatch struct. #1151

[Merged by Bors] - ♻️ Timer refactor to duration.✨ Add Stopwatch struct. #1151

Conversation

kokounet commented Dec 25, 2020 • edited Loading

DJMcNab commented Dec 25, 2020 • edited Loading

kokounet commented Dec 25, 2020 • edited Loading

mockersf commented Dec 25, 2020

mockersf Dec 25, 2020

Choose a reason for hiding this comment

kokounet Dec 25, 2020

Choose a reason for hiding this comment

mockersf Dec 25, 2020

Choose a reason for hiding this comment

kokounet commented Dec 26, 2020

alice-i-cecile commented Dec 26, 2020 • edited Loading

alice-i-cecile commented Dec 26, 2020

kokounet commented Dec 26, 2020 • edited Loading

alice-i-cecile commented Dec 27, 2020

Lythenas commented Dec 27, 2020

multun commented Dec 27, 2020

Ratysz commented Dec 27, 2020

kokounet commented Dec 27, 2020

Lythenas commented Dec 27, 2020

kokounet commented Dec 27, 2020

Lythenas commented Dec 28, 2020

cart commented Dec 28, 2020

cart commented Feb 6, 2021

kokounet commented Feb 7, 2021

cart left a comment

Choose a reason for hiding this comment

cart Mar 3, 2021

Choose a reason for hiding this comment

cart Mar 3, 2021

Choose a reason for hiding this comment

kokounet commented Mar 4, 2021

cart commented Mar 5, 2021

bors bot commented Mar 5, 2021

kokounet commented Mar 5, 2021

mockersf commented Mar 5, 2021

cart commented Mar 5, 2021 • edited Loading

DJMcNab commented Mar 5, 2021

cart commented Mar 5, 2021

bors bot commented Mar 5, 2021

DJMcNab commented Mar 5, 2021

cart commented Mar 5, 2021

DJMcNab commented Mar 5, 2021

bors bot commented Mar 5, 2021

cart commented Mar 5, 2021

cart commented Mar 5, 2021

bors bot commented Mar 5, 2021

[Merged by Bors] - ♻️ `Timer` refactor to duration.✨ Add `Stopwatch` struct. #1151

[Merged by Bors] - ♻️ `Timer` refactor to duration.✨ Add `Stopwatch` struct. #1151

kokounet commented Dec 25, 2020 •

edited

Loading

DJMcNab commented Dec 25, 2020 •

edited

Loading

kokounet commented Dec 25, 2020 •

edited

Loading

alice-i-cecile commented Dec 26, 2020 •

edited

Loading

kokounet commented Dec 26, 2020 •

edited

Loading

cart commented Mar 5, 2021 •

edited

Loading