[Merged by Bors] - Make paused timers update `just_finished` on tick #4445

tim-blackbird · 2022-04-08T19:39:53Z

Objective

Make timers update just_finished on tick, even if paused.
Fixes #4436

Solution

just_finished() returns times_finished > 0. So I:

Renamed times_finished to times_finished_this_tick to reduce confusion.
Set times_finished_this_tick to 0 on tick when paused.
Additionally set finished to false if the timer is repeating.

Notably this change broke none of the existing tests, so I added a couple for this.

Files changed shows a lot of noise because of the rename. Check the first commit for the relevant changes.

Migration Guide

Timer::times_finished has been renamed to Timer::times_finished_this_tick for clarity.

crates/bevy_core/src/time/timer.rs

alice-i-cecile

Both of the tests look good, but the fix is a bit questionable to me.

alice-i-cecile

Aha, that makes way more sense. LGTM now.

tim-blackbird · 2022-04-11T10:09:41Z

It would be great if this could make it into 0.7.
If we decide we do want a times_finished that indicates the amount of times a timer has finished since it's last reset for 0.8. We'll end up with a breaking change that still compiles 😧

alice-i-cecile · 2022-04-11T15:40:52Z

It would be great if this could make it into 0.7.

Needs a second approval :) You should consider asking on Discord in #general-dev.

hymm · 2022-04-11T17:44:55Z

I'm not sure about adding this functionality to paused. Real stopwatches don't change the state when they're paused. Only when the reset button is pushed, so holding the finished state makes sense. To me it feels like this should be a separate function that can be called to reset finished.

tim-blackbird · 2022-04-12T10:18:17Z

The finished state will still be held even with this pr. Only just_finished will be updated.

Unless the the timer is repeating, in which case using finished() usually behaves like just_finished() as mentioned in the example.
in this pr I made it so it still behaves the same way, even when paused. (I'm not entirely sold on my decision here)

Having just_finished() return true for more than one tick, (Which contradicts it's doc comment!) is IMO suprising and in the usecases I can think of, undesired.

As an example:
let's say there is a system that spawns one enemy every few seconds using a repeating timer's just_finished().
And if the player pauses the game, this timer is paused.
Before this pr, if the player pauses the game on the frame the enemy is going to be spawned, an enemy will be spawned every-frame as long as the player stays in the pause menu.

See the linked issue(#4436) for an explanation of the problem by @Madadog.

hymm · 2022-04-12T17:45:00Z

Yeah my previous suggestion was a little confused. But setting times_finished to 0 feels a little surprising though. It should still report the number of times the timer finished even if it is paused.

I think I would prefer if we just decouple the state of just_finished from times_finished. i.e. add a separate bool for just_finished.

Also what is the reasoning to setting finished to false if it's paused and repeating?

tim-blackbird · 2022-04-12T21:18:25Z

Oh, no. Another victim of the confusingly named variable times_finished.
Sorry if that sounded snarky. I think I can speak for @alice-i-cecile and myself that we were very confused at first also.

I hope you did notice my latest commit renaming times_finished to times_finished_this_tick. If you're still having trouble, do read the doc comment on the times_finished_this_tick() method, and it's tests.

I think I would prefer if we just decouple the state of just_finished from times_finished. i.e. add a separate bool for just_finished.

When I started making this change this was exactly what I was going for, but as I was trying to do so I read the doc comment/tests for times_finished and realized that it wasn't what I though it was, and it's very clear to me now that I'm not the only one.

(Pinging @kokounet as they introduced times_finished)

Also what is the reasoning to setting finished to false if it's paused and repeating?

Purely because it would make this statement in the timers example, no longer true.
I am now leaning more to rephrasing the example and removing the special case I coded in for this.

hymm · 2022-04-12T21:34:46Z

yeah, definitely a bit confusing. So if we pause the timer the internal elapsed time might say evaluate to time_finished_this_tick to 2, but since it's paused it reports 0. But when we unpause the time it'll report 2 or more after the timer is unpaused and ticked, because the internal state hasn't been reset.

What we want is for a tick to be a noop to the public api. So is reporting the timer as not having finished correct then? It might be? Honestly not sure. The timer actually finished before you paused the timer...

Really need to see some use cases for this API to know for sure.

tim-blackbird · 2022-04-12T22:26:08Z

Sorry, I should have explained the purpose of times_finished_this_tick better.

Timers do not track how many times they have finished, period, this has not been added (yet).
Only how many times they finished within the latest tick.

So, times_finished_this_tick can be used to see whether a timer finished multiple times over in one tick.
finished and just_finished cannot do this.

But I am clearly failing terribly at explaining this with words. I hope these code examples do the trick.

let mut t = Timer::from_seconds(1.0, true);

if t.tick(Duration::from_secs_f32(3.0)).just_finished() {
    // only reached once!!!
}

for _ in 0..t.tick(Duration::from_secs_f32(3.0)).times_finished_this_tick() {
    // reached three times!!!
}

t.tick(Duration::from_secs_f32(0.0));
assert_eq!(t.times_finished_this_tick(), 0);

t.tick(Duration::from_secs_f32(3.0));
assert_eq!(t.times_finished_this_tick(), 3);

t.tick(Duration::from_secs_f32(69.0));
assert_eq!(t.times_finished_this_tick(), 69);

kokounet · 2022-04-13T16:57:34Z

I definitely agree, that name is confusing. At that time I did not think it could be interpreted as the total number of wraps for repeating timers 😅 as I didn't see any use case for it. It's definitely a bad name if it's required to read the doc to understand what it does.
Though I'm not a huge fan of the new name as it is a bit verbose, but still it's better than the current one !

hymm · 2022-04-14T20:16:35Z

I'm sorry to make you explain so much. This was definitely a me problem. I was getting my mental model confused with timers from other programs. Reseting times_finished_this_tick makes sense because this tick is the current/last call to tick, which is a no op if the timer is paused, so there definitely can't be any finishes on a paused tick.

hymm · 2022-04-23T06:11:06Z

Should this have a migration guide (label) for the rename?

alice-i-cecile · 2022-04-26T18:37:05Z

@devil-ira please edit your PR description to include the following migration guide section when you get a chance :)

Migration Guide

Timer::times_finished has been renamed to Timer::times_finished_this_tick for clarity.

alice-i-cecile · 2022-04-26T18:37:11Z

bors r+

# Objective Make timers update `just_finished` on tick, even if paused. Fixes #4436 ## Solution `just_finished()` returns `times_finished > 0`. So I: * Renamed `times_finished` to `times_finished_this_tick` to reduce confusion. * Set `times_finished_this_tick` to `0` on tick when paused. * Additionally set `finished` to `false` if the timer is repeating. Notably this change broke none of the existing tests, so I added a couple for this. Files changed shows a lot of noise because of the rename. Check the first commit for the relevant changes. Co-authored-by: devil-ira <[email protected]>

bors · 2022-04-26T19:20:24Z

Pull request successfully merged into main.

Build succeeded:

# Objective Make timers update `just_finished` on tick, even if paused. Fixes bevyengine#4436 ## Solution `just_finished()` returns `times_finished > 0`. So I: * Renamed `times_finished` to `times_finished_this_tick` to reduce confusion. * Set `times_finished_this_tick` to `0` on tick when paused. * Additionally set `finished` to `false` if the timer is repeating. Notably this change broke none of the existing tests, so I added a couple for this. Files changed shows a lot of noise because of the rename. Check the first commit for the relevant changes. Co-authored-by: devil-ira <[email protected]>

make timers update just_finished on tick. add tests.

4bdf272

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 8, 2022

use assert!

abeeeaa

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Core and removed S-Needs-Triage This issue needs to be labelled labels Apr 8, 2022

alice-i-cecile reviewed Apr 8, 2022

View reviewed changes

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

alice-i-cecile requested changes Apr 8, 2022

View reviewed changes

rename "times_finished" to "times_finished_this_tick"

aab0b65

tim-blackbird requested a review from alice-i-cecile April 8, 2022 21:44

alice-i-cecile approved these changes Apr 8, 2022

View reviewed changes

hymm approved these changes Apr 14, 2022

View reviewed changes

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 Apr 14, 2022

tim-blackbird changed the title ~~Make timers update just_finished on tick, even if paused.~~ Make paused timers update just_finished on tick Apr 14, 2022

alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 23, 2022

bors bot changed the title ~~Make paused timers update just_finished on tick~~ [Merged by Bors] - Make paused timers update just_finished on tick Apr 26, 2022

bors bot closed this Apr 26, 2022

tim-blackbird deleted the timer_changes branch November 8, 2022 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Make paused timers update `just_finished` on tick #4445

[Merged by Bors] - Make paused timers update `just_finished` on tick #4445

tim-blackbird commented Apr 8, 2022 •

edited

Loading

alice-i-cecile left a comment

alice-i-cecile left a comment

tim-blackbird commented Apr 11, 2022

alice-i-cecile commented Apr 11, 2022

hymm commented Apr 11, 2022

tim-blackbird commented Apr 12, 2022 •

edited

Loading

hymm commented Apr 12, 2022

tim-blackbird commented Apr 12, 2022 •

edited

Loading

hymm commented Apr 12, 2022 •

edited

Loading

tim-blackbird commented Apr 12, 2022

kokounet commented Apr 13, 2022

hymm commented Apr 14, 2022 •

edited

Loading

hymm commented Apr 23, 2022

alice-i-cecile commented Apr 26, 2022

alice-i-cecile commented Apr 26, 2022

bors bot commented Apr 26, 2022

[Merged by Bors] - Make paused timers update just_finished on tick #4445

[Merged by Bors] - Make paused timers update just_finished on tick #4445

Conversation

tim-blackbird commented Apr 8, 2022 • edited Loading

Objective

Solution

Migration Guide

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

tim-blackbird commented Apr 11, 2022

alice-i-cecile commented Apr 11, 2022

hymm commented Apr 11, 2022

tim-blackbird commented Apr 12, 2022 • edited Loading

hymm commented Apr 12, 2022

tim-blackbird commented Apr 12, 2022 • edited Loading

hymm commented Apr 12, 2022 • edited Loading

tim-blackbird commented Apr 12, 2022

kokounet commented Apr 13, 2022

hymm commented Apr 14, 2022 • edited Loading

hymm commented Apr 23, 2022

alice-i-cecile commented Apr 26, 2022

Migration Guide

alice-i-cecile commented Apr 26, 2022

bors bot commented Apr 26, 2022

[Merged by Bors] - Make paused timers update `just_finished` on tick #4445

[Merged by Bors] - Make paused timers update `just_finished` on tick #4445

tim-blackbird commented Apr 8, 2022 •

edited

Loading

tim-blackbird commented Apr 12, 2022 •

edited

Loading

tim-blackbird commented Apr 12, 2022 •

edited

Loading

hymm commented Apr 12, 2022 •

edited

Loading

hymm commented Apr 14, 2022 •

edited

Loading