Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Timer ergonomics #923

Merged
merged 1 commit into from
Nov 26, 2020
Merged

Improve Timer ergonomics #923

merged 1 commit into from
Nov 26, 2020

Conversation

CleanCut
Copy link
Member

I often use timers to drive animation by converting it to a percentage. The fact that elapsed could exceed duration for non-repeating timers means that I had to check for that everywhere I used them, or the percentage could go negative (which can do various unexpected things, such as flip vector direction). This is a paper cut for me, so I made this PR which:

  • Clamps elapsed to the range [0.0, duration]
  • Adds tests
  • Adds some documentation
  • Adds two convenience methods: (percent and percent_left)
    • I was on the fence whether to include these methods in the PR due to the fact that it assumes keeping the size of Timer small is more important than the speed of the calculation. A more speed-optimized approach would store the inverse of duration and do multiplication. Is there a general principle we're following that could inform the approach? Or should we leave the methods out altogether?

Copy link
Contributor

@ambeeeeee ambeeeeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how it replaces the fancy logic with easier to understand at-a-glance logic

@Moxinilian Moxinilian added core C-Feature A new feature, making something new possible labels Nov 25, 2020
@CleanCut
Copy link
Member Author

CleanCut commented Nov 25, 2020

Here's an example from a showcase I'm making that demonstrates using these changes to directly power a shrinking death animation and a punching boxing glove animation. You need to have at least one gamepad connected to your computer to try it out (controls are gamepad-only).

punchball-0 1 0

@cart
Copy link
Member

cart commented Nov 26, 2020

Ooh I love the look of that boxing game.

I was on the fence whether to include these methods in the PR due to the fact that it assumes keeping the size of Timer small is more important than the speed of the calculation. A more speed-optimized approach would store the inverse of duration and do multiplication. Is there a general principle we're following that could inform the approach? Or should we leave the methods out altogether?

In this case I prefer the clarity of division over storing / multiplying by the inverse.

Using percentages seems like a common enough requirement for timers that I think its worth including, largely because the function name helps encode intent.

I think this is good to go!

@cart
Copy link
Member

cart commented Nov 26, 2020

Semi-relevant idea that I'd love to discuss with yall (I'm pulling in @amberkowalski because they're also working on timers):

I think it might be a good idea to remove the "auto ticking" timer system.

  • Resource-based timers don't auto-tick
  • Component-based timers only auto-tick if they aren't wrapped. But I think having multiple timers with different "names" will be common (see @CleanCut's boxing example).

Its a bit inconsistent that only raw Timer components auto-tick and theres no good way to force all timers to tick. I think I'd rather just make ticking explicit everywhere for simplicity.

@cart
Copy link
Member

cart commented Nov 26, 2020

@amberkowalski if we make ticking explicit everywhere, maybe we don't need built in pausing? users could "pause" by not ticking where relevant according to their own context-specific logic.

@cart cart merged commit 2f408cf into bevyengine:master Nov 26, 2020
@CleanCut
Copy link
Member Author

My two cents:

  • Making ticking of timers always explicit seems like the sanest option. I didn't even think to look for auto-ticking, and would have been surprised had I run into it. Including an example of how to tick your timer in the docs for Timer should be sufficient.
  • I would welcome pause functionality -- it is a useful and typical feature to have built-in to the timers. In its absence, a user who needs to pause is faced with either tightly coupling the pause logic to the ticking logic, or wrapping/reimplementing the Timer with pause logic.

In this case I prefer the clarity of division over storing / multiplying by the inverse.

Using percentages seems like a common enough requirement for timers that I think its worth including, largely because the function name helps encode intent.

🥳 🎉

@cart
Copy link
Member

cart commented Nov 26, 2020

I would welcome pause functionality -- it is a useful and typical feature to have built-in to the timers. In its absence, a user who needs to pause is faced with either tightly coupling the pause logic to the ticking logic, or wrapping/reimplementing the Timer with pause logic.

Good enough for me. @amberkowalski, if you update your pr to account for the changes in this pr I think it'll be merge-ready!

@CleanCut
Copy link
Member Author

* Making ticking of timers always explicit seems like the sanest option. I didn't even think to look for auto-ticking, and would have been surprised had I run into it.  Including an example of how to tick your timer in the docs for `Timer` should be sufficient.

Having said that, having a way to opt-in to auto-ticking could be very convenient. 🤔 But I think I'm just feature creeping now. 😆

@smokku
Copy link
Member

smokku commented Nov 26, 2020

Having said that, having a way to opt-in to auto-ticking could be very convenient. But I think I'm just feature creeping now.

Would providing a system that auto-ticks all timers and an example how to include it into the App, count as opt-in?

@cart
Copy link
Member

cart commented Nov 26, 2020

Haha I'm very curious how you would do this across wrapper types without adding some sort of internal shared state

@smokku
Copy link
Member

smokku commented Nov 26, 2020

Nyah. Just for the not wrapped ones - like it works now.

We could use the example to put a note reminding users that it ticks only not wrapped Timers and if you want to tick a wrapped one, you need to do it manually.

@fopsdev fopsdev mentioned this pull request Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants