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

Allow timers to be paused #914

Merged
merged 9 commits into from
Nov 26, 2020
Merged

Allow timers to be paused #914

merged 9 commits into from
Nov 26, 2020

Conversation

ambeeeeee
Copy link
Contributor

This allows timers to be paused.

(Note: the git history for this is horrible but its getting squashed so that's okay :) )

@karroffel karroffel added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible core and removed A-ECS Entities, components, systems, and events labels Nov 22, 2020
@cart
Copy link
Member

cart commented Nov 22, 2020

I dig it. I generally like having "one way" to do things where possible. If we're going to expose the pause field publicly, then i dont think we need a pause() function. And if we're going to have a pause() function, then we should make pause private and add a resume() function.

I think encapsulation is good in this case, so can we make pause private and add resume?

Haha and then while you're at it, can you fix my "visibility mistakes" by encapsulating everything else? Ex: elapsed / finished / just_finished have no right being publicly editable. And at that point we might as well encapsulate everything for consistency.

@ambeeeeee
Copy link
Contributor Author

Got it, sounds like a good idea

@ambeeeeee
Copy link
Contributor Author

ambeeeeee commented Nov 23, 2020

I do want feedback on the function names, and this is a breaking change that would break a lot of code so that sucks :(. At least we're getting it out of the way now :)

Sorry that this PR's comment history and stuff is all messed up, I had to rewrite git history after realizing I messed up :(

@mockersf
Copy link
Member

could you add setters for the fields repeating and duration? There are a few occasions where I want to change those without resetting the current timer, which is not possible without that

case could be also made for field elapsed, to allow for a repeating timer to have its first loop start at half time for example, or a game event that would need to remove a few seconds of a running timer

@ambeeeeee
Copy link
Contributor Author

could you add setters for the fields repeating and duration? There are a few occasions where I want to change those without resetting the current timer, which is not possible without that

case could be also made for field elapsed, to allow for a repeating timer to have its first loop start at half time for example, or a game event that would need to remove a few seconds of a running timer

Seems reasonable, I'll add them in. Thank you!

@ambeeeeee
Copy link
Contributor Author

I merged my code with the timer changes on master, I haven't written an example yet because I just want the final sign-off on the changes before I write an example :D

@ambeeeeee ambeeeeee requested a review from cart November 26, 2020 07:42
@fopsdev
Copy link

fopsdev commented Nov 26, 2020 via email

@ambeeeeee
Copy link
Contributor Author

Floating point operations appear to be accelerated by the CPU so I doubt its horrible performance wise. Probably still worth looking into.

@cart
Copy link
Member

cart commented Nov 26, 2020

and here we use f64 for the seconds_since_startup

I'm pretty sure I chose f64 for seconds_since_startup because f32 will start losing valuable precision if your app runs for days. Thats a bit of an exception to the rule. In general we should be using f32

@cart
Copy link
Member

cart commented Nov 26, 2020

I'm going to merge this now. Feel free to follow up with an example if you want 😄

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

@amberkowalski @cart Wait! Is this really done? Shouldn't pausing the timer...actually pause the timer?

@CleanCut
Copy link
Member

I'm going to open up a PR. There were a couple other inconsistencies I noticed as well.

BTW, thanks for doing this, @amberkowalski -- when I initially opened up my other Timer PR I had no idea this was ongoing, or I would have brought up that issue here.

@CleanCut CleanCut mentioned this pull request Nov 26, 2020
@cart
Copy link
Member

cart commented Nov 26, 2020

Haha oops. Sounds like I shotgunned this one in too early.

@CleanCut
Copy link
Member

Should be fixed in #931

@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.

6 participants