bevyengine / bevyengine/bevy

Timer::set_elapsed, set_duration, and set_mode can cause inconsistent Timer state

Open
#8,411 1 comment 1 reaction 0 assignees View on GitHub
A-Time C-Bug
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.