Timer::set_elapsed, set_duration, and set_mode can cause inconsistent Timer state
- Dominant language
- Rust
- Stars
- 48.2k
- Forks
- 4.8k
- Avg merge
- 3d 16h
- Merged PRs (30d)
- 171
Description
## Bevy version
Main, and also 0.9.1.
## What you did
While cleaning up my personal implementation of a `TimerMode::Countup` for eventual PR, I noticed that `Timer::set_elapsed`, `set_duration`, and `set_mode` can put a `TimerMode::Once` timer into a persistently inconsistent state.
[Here's some new tests from my WIP branch that illustrate the problem](https://github.com/nfagerlund/bevy/blob/1b98f0a9d1666b836c2887a18c697d44109f5efd/crates/bevy_time/src/timer.rs#L599-L707); look for the assertions preceded by `FAILS`.
## What went wrong
Short story is that there's an early return in `Timer::tick()` for `Once` timers that have already `finished`, and if you use one of those three public methods to manipulate the internal state (instead of keeping to `tick` and `reset` like a good puppy), the timer can get recalcitrant:
- Elapsed is less than duration, but registers as finished... and can't be ticked forward anymore.
- Elapsed is greater than duration, which breaks the promise that `elapsed` gets clamped for `Once` timers.
You can get out of problem states with `Timer::reset`.
**More fundamentally, though:** The semantics of these three methods are very underdefined (and there's no tests for em), and it was not obvious to me how they actually _should_ work. They effectively re-write the timer's history, so what effect should that have on the methods (`finished`, `just_finished`, etc.) that report on what occurred between the last tick and the one before? Both of those ticks arguably got obliterated from the timeline. 🤷🏽
I figure there's about four competing reasonable expectations for how those methods might behave:
1. They can cause inconsistent state, but the next `tick` will settle it down and recover (in a potentially lossy way).
2. Don't even touch them unless you're also gonna call `reset`; garbage in, garbage out.
3. Those methods should be private. Just replace your timer when you need a radically different one.
4. Those methods need rewrites so they always leave the timer in a consistent state. (But... _which_ consistent state? 🤔 Like a new timer that just got ticked by `elapsed` in one go? (i.e. we'll call `reset` for you.) A complicated matrix of possibilities based on the state immediately before the new value? If the latter, then how do multiple `set_thing` calls in a row behave?)
My initial theory was option 1, as shown in those new tests linked above, but the result is way too confusing. Currently I'm in favor of either 3 (make the methods private) or 4a (make them reset the timer to just-born status prior to hard-setting the state). 2 is too rude, and 4b is too masochistic.
## Additional information
When I raised this in the discord, multiple people immediately responded that they consider `Timer` to be overcomplicated. Maybe so!!
Contributor guide
Research direction
Read crates/bevy_time/src/timer.rs and inspect the FAILS assertions in the linked WIP tests around lines 599-707. Reproduce the inconsistent Once-timer states, then resolve and document the intended semantics of set_elapsed, set_duration, and set_mode; done means the setters leave Timer behavior consistent and the regression tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- game-dev
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100