Roll over any extra frame time in timers

[8bit-pudding]

The current timer implementation resets elapsed time back to 0.0, even though the exact moment the finished check is done might already be some time after the duration has elapsed. This means that each time the timer is reset, it drifts a little bit apart from it's actual timing. This is especially noticeable when using e.g. a one second timer and a two second timer. Both timers will drift at different rates because the second timer is reset less often.

To fix it, upon resetting the timer any overshoot should roll over into the elapsed time after resetting.

See the table below for some of the timings I measured with and without the fix.

Every row corresponds to one finish of a timer, the second column being the difference in time between finishes. Note how without the fix every finish is always higher than the duration by about single frame's worth of time, never lower.

1 sec timer, no rollover, with vsync (60fps)		2 sec timer, no rollover, with vsync (60fps)		1 sec timer, with rollover, with vsync (60fps)		2 sec timer, with rollover, with vsync (60fps)
1,7202934		2,7202607		1,7455829		2,7456175
2,736951	1,0166576	4,7368873	2,0166266	2,7456175	1,0000346	4,7455369	1,9999194
3,7536238	1,0166728	6,753461	2,0165737	3,7455841	0,9999666	6,7454953	1,9999584
4,7702786	1,0166548	8,7701082	2,0166472	4,7455369	0,9999528	8,7455382	2,0000429
5,7868416	1,016563	10,7866728	2,0165646	5,7454703	0,9999334	10,7453447	1,9998065
6,8035022	1,0166606	12,7866846	2,0000118	6,7454953	1,000025	12,7452798	1,9999351
7,8200845	1,0165823	14,8032508	2,0165662	7,7454417	0,9999464	14,745219	1,9999392
8,8203511	1,0002666	16,8198416	2,0165908	8,7455382	1,0000965	16,7451496	1,9999306
9,8367805	1,0164294	18,83645	2,0166084	9,7453596	0,9998214	18,7451992	2,0000496
10,8533805	1,0166	20,8367235	2,0002735	10,7453447	0,9999851	20,7450517	1,9998525

I had two timers running, printing the time elapsed since startup for both upon finishing, in case anyone would want to reproduce the tests:

fn timer_one(time: Res<Time>, mut timer: ResMut<DefaultTimerOneSec>) {
    timer.0.tick(time.delta_seconds);

    if timer.0.finished {
        println!("One second timer finished, elapsed:\t{}", time.seconds_since_startup);
        timer.0.reset();
    }
}

[jakerr]

I think this changes the semantics around reset in a way that would affect other valid use cases. Consider when the intention is to reset a timer that has not finished yet. In that case I think you would want it to return to zero.

[jakerr]

Maybe adding a new method is called for with a name like: resume, continue or rollover ?

[8bit-pudding]

Tested it with a resettable timer and you're totally right! Fixed in the next commit.

[8bit-pudding]

Not sure what the added value would be of an extra method. Both cases can be covered by a single method as far as I can tell.

[jakerr]

I suggested a new method because reset to me, sounds like it should always zero out the timer.

The additional change you added to handle reset before a timer is finished improves the situation but I think there are still situations where this automatic carry over would be surprising.

What about a timer that is only reset in order to measure an infrequent event (not looping all the time). Imagine a timer used to leave an informational message on the screen for a specified interval. It would be surprising if the timer carried over the previous elapsed time when it was reset. I suppose if it were an infrequent timer like that, it wouldn't have a significant effect if the timer was only carried forward by a single run loop's worth of elapsed time. But as it's currently implemented it could go along ticking the elapsed variable for a long duration and by the time reset is called it would be at some arbitrary value.

That made me think a new method or a new type like LoopingTimer might be nice.

If we want to make this support both looping with carry over, as well as infrequent event time measurements I think a little bit more logic in tick() might get us there.

How about something like this:

//...
    pub fn tick(&mut self, delta: f32) {
        if self.finished {
            // A tick after already finished before `reset` was called indicates that carry over
            // of elapsed time for a looping timer was not intended.
            self.elapsed = self.duration;
            return;
        }
        self.elapsed = self.elapsed + delta;
        if self.elapsed >= self.duration {
            self.finished = true;
        }
    }

    pub fn reset(&mut self) {
        self.elapsed = if self.finished {
            self.elapsed % self.duration
        } else {
            0.0
        };
        self.finished = false;
    }
//...

[jakerr]

Here are some tests for the above:

#[cfg(test)]
mod tests {
    use super::{Timer};

    #[test]
    fn test_carry_over() {
        let mut timer = Timer::from_seconds(3.0);
        timer.tick(3.5);
        assert!(timer.finished == true);
        timer.reset();
        // Carry over because the timer was finished during the most recent tick.
        assert_eq!(timer.elapsed, 0.5);
    }

    #[test]
    fn test_carry_over_not_applied_to_unfinished_timer() {
        let mut timer = Timer::from_seconds(3.0);
        timer.tick(2.5);
        assert!(timer.finished == false);
        timer.reset();
        // No carry over since timer wasn't finished.
        assert_eq!(timer.elapsed, 0.0);
    }

    #[test]
    fn test_carry_over_not_applied_to_rarely_reset_timer() {
        let mut timer = Timer::from_seconds(3.0);
        timer.tick(2.5);
        assert!(timer.finished == false);
        timer.tick(2.5);
        assert!(timer.finished == true);
        assert_eq!(timer.elapsed, 5.0);

        // Tick again after finished without reseting means the timer is set to `duration`
        timer.tick(2.5);
        assert_eq!(timer.elapsed, 3.0);

        timer.reset();
        // No carry over since timer wasn't finished after the most recent tick.
        assert_eq!(timer.elapsed, 0.0);
    }
}

running 3 tests
test tests::test_carry_over_not_applied_to_unfinished_timer ... ok
test tests::test_carry_over_not_applied_to_rarely_reset_timer ... ok
test tests::test_carry_over ... ok

[8bit-pudding]

Thanks for the explanation! That does clear it up a lot for me.

Given the example used in the introduction (https://bevyengine.org/learn/book/getting-started/resources/) I had the impression Timer was supposed to be a repeating construct from the get go, but the case you've pointed out makes a lot of sense too.

Maybe adding a repeating flag in the constructors would give the most intuitive behavior. In non repeating mode the timer stops after finishing once, then reset() should be called to re-use the timer, setting the elapsed time back to 0.0. In repeating mode the timer sets the finished state during the update in which the duration was reached and automatically resets it's elapsed time carrying over any excess. In the next update finished should then be false again. This also seems more ergonimical since for repeating timers you don't have to keep calling a reset/resume function manually; you just set it to go and keep check of whether or not it finished.

[8bit-pudding]

As an example of the above, I've yet to actually test this:

    pub fn tick(&mut self, delta: f32) {
        match (self.repeating, self.elapsed >= self.duration) {
            // non repeating timer
            (false, false) => {
                self.elapsed = self.elapsed + delta;
            },
            (false, true) => {
                self.finished = true;
            },

            // repeating timer
            (true, false) => {
                self.elapsed = self.elapsed + delta;
                self.finished = false;
            }
            (true, true) => {
                self.elapsed = self.elapsed % self.duration;
                self.finished = true;
            }
        }
    }

    pub fn reset(&mut self) {
        self.finished = false;
        self.elapsed = 0.0;
    }

Edit: Added an implementation for this.

[AmionSky]

By removing this if check, elapsed could be used to calculate how long ago did the timer finish + a minor perf boost.
Or is there a concern for the float to overflow?

[8bit-pudding]

I think it's more about expected behavior; if I set a timer for 10 seconds I expect it to notify me after 10 seconds and stop doing anything after that, but maybe that's just me.

I did have overflow in mind adding the check but I didn't check how long you'd actually have to leave it running. Since it's going to run out at some point anyways you're probably better off storing an Instant externally and comparing that later on.

[multun]

what this field doesn't isn't immediately obvious, could you document it?

[8bit-pudding]

Added some comments to prev_finished and just_finished() to clarify.

[multun]

Can you rebase this on master? Format checking was enabled, and I believe the CI doesn't yet check for it at the commit you're at

[cart]

I'm happy with how this turned out. Thanks!

[chaosteil]

Sorry to bust in on this discussion, but would it have made more sense to add a new method for the repeatable variants?

An additional bool on the timer function signatures, while still ergonomic, looks a bit off. It's not entirely clear what the boolean means on first look, and maybe some of the "why is my timer not repeating" off-by-bool-swap errors could be avoided? Something like repeat_from_seconds or even RepeatTimer or something similar might work better? Just a suggestion of course.

[AmionSky]

While I had the same concern as you, personally I like that it forces you to make the decision whether or not you want your timer to repeat.