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

Allow the use of Timer as a Cooldown / Create a Cooldown struct #1127

Closed
kokounet opened this issue Dec 21, 2020 · 6 comments
Closed

Allow the use of Timer as a Cooldown / Create a Cooldown struct #1127

kokounet opened this issue Dec 21, 2020 · 6 comments
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible

Comments

@kokounet
Copy link
Contributor

kokounet commented Dec 21, 2020

What problem does this solve or what need does it fill?

A Cooldown is like a timer except it's available when constructed and a reset make it available again.

The Timer struct currently can't be used to make a "cooldown" behavior because of the following minor differences between the two:

Behavior Timer Cooldown
Creation Timer::finished() == false Cooldown::available() == true
Reset Timer::finished() == false Cooldown::available() == true
Start N.A. Cooldown::available() goes from true to false and the cooldown start ticking (only for non looping Cooldowns).

I found the need for this behavior when implementing my sprite sheet animation system for my game. I need that a change of animation (from Idle to Attack for example) occur directly and not on the next animation frame in order to increase responsiveness (I'm sure there are other needs for this particular behavior).

Describe the solution would you like?

Because of the similarity of behavior of both things, my idea would be to have them share part of the code. But it could also be a new struct with a very similar API.

A cooldown could be made with a timer by doing the following:

  • On creation: tick the duration of the timer to make it available.
  • On reset: reset the timer and tick the duration of the timer to make it available.

However, implementing it this way is fragile because it doesn't support looping cooldown, and is a bit messy to use.

Describe the alternative(s) you've considered?

The more I think of it, the more I think that having a different struct for this job is more appropriate for the reason that it's not a complicated behavior and also that the ticking mechanism, although similar, is quite different.
Maybe the ticking, pausing, elapsed times could be factored out in a Chronometer struct that both those two things could use?

Additional context

An example of what a Cooldown struct could look like:

pub struct Cooldown {
    elapsed: f32,
    duration: f32,
    available: bool,
    just_available: bool,
    paused: bool,
    repeating: bool,
}

impl Cooldown {
    // Same as Timer for constructors, pausing and elapsed times... Maybe that's worth factoring

    pub fn start(&mut self) {
        self.available = false;
    }

    pub fn reset(&mut self) {
        self.elapsed = 0.0;
        self.available = true;
        self.just_available = true;
    }

    pub fn tick(&mut self, delta: f32) -> &Self {
        // ticking mechanism if the cooldown is not available
    }
}

I'm sorry if this is a bit messy, It's my first issue. I love your work on bevy, I think it's really great! I would be happy to help integrate this feature in bevy if you find it belongs there!

@kokounet kokounet changed the title Allow the use of Timer as a Cooldown Allow the use of Timer as a Cooldown / Create a Cooldown struct Dec 21, 2020
@memoryruins memoryruins added A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible labels Dec 22, 2020
@alice-i-cecile
Copy link
Member

This is an interesting proposal. I think the Timer and FixedTimeStep APIs could both use some love to be a bit less full of boilerplate, multiple similar approaches and boilerplateful my_timer.0 methods, which I think fits in well with making sure we can support this use case properly.

To make sure I'm understanding you correctly, the core issues are:

  1. One timestep offset between the timer being created and the first time that it can be finished, which can cause avoidable lag in animation and input.
  2. Some nicer sugar for cooldowns, with inverted booleans for code clarity.

We might be able to get away with 1 through some specialty methods. The latter seems like it might be a better fit for a thin wrapper around Timer, either in the individual game / crate or within Bevy itself. I'm a bit worried about redundancy if we put it in Bevy, but making it a thin wrapper would reduce the maintenance burden substantially.

On FixedTimeStep, I think that allowing systems and system sets to work with run_criteria (via the proposed system set and explicit system ordering features) would go a long way to eliminating most of the common boilerplate found with Timers, and make FixedTimeStep itself a more natural feature to use.

To get rid of the timer.0 ugliness (and confusion for beginners), I think the issue is mostly in sloppy examples. Really, these custom timer structs should be deriving Deref (or From, if you want to be explicit about things) to convert them seamlessly into timers.

struct HelloTimer(Timer);

// These Deref impls are optional sugar to make using these timers more pleasant.
// Rather than having to use `my_hello_timer.0` every time we want to use one of these objects
// we can just call `my_hello_timer` directly
impl Deref for HelloTimer{
	type Target = Timer;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl DerefMut for HelloTimer{
    fn deref_mut(&mut self) -> &mut Timer {
        &mut self.0
    }
}

Not thrilled with the amount of space that takes up; maybe a simple derive trait or proc macro is called for?

@kokounet
Copy link
Contributor Author

The Timer API is also concerned with this PR #1112, which also propose more sugar for timers, and the feature could be used for the FixedTimestep thingy. I'm not very familiar with the FixedTimestep struct though...

One timestep offset between the timer being created and the first time that it can be finished, which can cause avoidable lag in animation and input.

Yes, that is correct, there are other usecases like auto-fire for weapons and stuff like that (cooldowns for spells).

Some nicer sugar for cooldowns, with inverted booleans for code clarity.

Yeah, this is right, although it's a bit more subtle than simply inverted booleans because when a cooldown is ticking it's not available, and the same goes for the Timer. Although looking similar, Cooldowns and Timers are close but not really for the same use-cases. The first allow doing things before the ticking start, while the latter allow doing things once finished (tracking lifetimes for example).

I'm a bit worried about redundancy if we put it in Bevy [...].

My take on that is that Timers, Cooldowns, and just a simple Stopwatch are very frequent features in games, and it's a small amount of code completely decoupled from the rest of the engine.
Plus, the Stopwatch could be the base to construct both Timer and Cooldown. This would factor out the ticking / starting / resetting mechanism.


On FixedTimeStep, I think that allowing systems and system sets to work with run_criteria (via the proposed system set and explicit system ordering features) [...].

I'm not familiar with those features, but I think it will be clearer once the Timer is sorted out.

To get rid of the timer.0 ugliness (and confusion for beginners).

I don't know if that's part of my issue, but I agree that the NewType pattern for this is not very adequate. Maybe enforce that the timer is used with a type like Timer<T> to hint that this feature is designed for specialization as it removes the boilerplate? And if one really must use a simple timer then making an alias for Timer<()> ? But maybe that's worth making a whole new issue 😅

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Dec 22, 2020

Thanks for the explanations; I think I'm on board with making Cooldowns a standard bit of kit.

I really like the Timer<T> approach to specialization: that makes the specialization dramatically more clear and cuts down on boilerplate.

I think this calls for a new issue (or PR) to overhaul the Timer-related API at this point :) I'll try to work on this in the next few hours, so don't rush to file a new issue for that.

@mockersf
Copy link
Member

mockersf commented Dec 22, 2020

I tried Timer<T>, just pushed if you want to take a look: mockersf@36f9579

there are two issues:

  • deriving traits do not work that well when there is a generic type param, that can mostly be worked around (this one)
  • it becomes impossible to register all Timer<T> type registry here

@kokounet
Copy link
Contributor Author

kokounet commented Dec 22, 2020

Oh woaw that's quite a lot of change I wasn't expecting as much!

deriving traits do not work that well when there is a generic type param, that can mostly be worked around

Yes, that is indeed a hassle. Are there still problems with the work around you propose ?

it becomes impossible to register all Timer type registry here

Can you elaborate a bit more on why this is a problem? User defined timers will be components, resources, or locals in any cases, just like other user defined structs?

Regarding the initial Cooldown matter I will (tomorrow) make a minimal example of an implementation here, so you can use the code if you find it useful (contrary to what the name suggest, it is not intended to directly fit in bevy as I feel I don't have enough experience with the bevy codebase).

bors bot pushed a commit that referenced this issue Mar 5, 2021
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 <[email protected]>
bors bot pushed a commit that referenced this issue Mar 5, 2021
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 <[email protected]>
bors bot pushed a commit that referenced this issue Mar 5, 2021
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 <[email protected]>
@kokounet
Copy link
Contributor Author

kokounet commented Feb 3, 2022

We should probably close this issue since it was merged to bevy 0.5

@kokounet kokounet closed this as completed Feb 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Feature A new feature, making something new possible
Projects
None yet
Development

No branches or pull requests

4 participants