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

[nfagerlund]

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; 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!!

[nfagerlund]

I should also mention, I'm happy to implement solutions 3 or 4b! But I'd want a maintainer to say that makes sense, first, rather than doing it on spec.