Stop the timer when the game is not visible #478
Conversation
| requestAnimationFrame(() => { | ||
| this.base!.textContent = minSec(Date.now() - this._start!); | ||
| this.base!.textContent = minSec(++this._secondsSinceStart! * 1000); |
There was a problem hiding this comment.
This assumes that setInterval is accurate, but it isn't.
The correct way to do this (afaik):
_startbecomes_anchorTime.- On
_startTimer, set_anchorTimetoDate.now()& start the interval. - When visibility becomes hidden, clear the interval, and set
_hiddenTimetoDate.now(). - When visibility becomes visible, increase
_anchorTimebyDate.now() - _hiddenTime, and restart the interval.
There was a problem hiding this comment.
Thanks for the review, @jakearchibald. In my initial tests, I noticed that Chrome does account for the discrepancies in the setInterval by offsetting the next "tick" by enough milliseconds to compensate for the previous ones, but indeed the users of other browsers would get ~1s of unfair advantage every ~16m.
I applied the fixes as you requested.
LeonardoBraga
force-pushed
the
issue-477
branch
2 times, most recently
from
1dcf970
to
5489b3d
Compare
There was a problem hiding this comment.
I'm happy with all the code here, however pausing is only fixed for the timer across the top, and isn't reflected in the final time when you win.
- here's where the real game timer is.Having timer logic split across two places isn't great. We were trying to avoid going through DOM diffing every second, which was a bit slow on lower end phones.
I'm open to better ideas, but here's the best I've got right now:
- Make
anchorTimepart ofGame's state. - Make
anchorTimea prop onTopBar. - Move the
visibilityChangedlogic intoGame, and use it to set thetimerRunningprop onTopBar.
@LeonardoBraga we really appreciate the time you've spent on this so far, and totally understand if you want us to take on the rest. Let us know.
|
Hey @jakearchibald, are you kidding? :-) We should definitely do it in the best way we possibly can! I had noticed the main timer when I was analyzing the code but I just wasn't sure if I should have gone for a PR what would touch multiple parts of the app, this is why I went for the low hanging fruit. I'm glad you're open to a more thorough change and will take a deeper look at your suggestion, the code, and work on the next iteration. |
|
We've changed the build for this project which has involved moving a lot of files around. Don't worry about the conflicts, we'll take care of it when we come to merge this. |
|
Sure, thanks for the update. I apologize for the radio silence over the last days, things ended up piling up on my end. I plan on wrapping the modifications you requested this week and send for your review. |
LeonardoBraga
force-pushed
the
issue-477
branch
from
5489b3d
to
19b0306
Compare
|
@jakearchibald I tried to incorporate the points you raised before, and besides any other feedback you may have, I wanted to call your attention to the following items:
|
|
Hey @jakearchibald, just wanted to know if you had time to check out the latest changes and have any additional feedback. Thanks! :) |
|
Thanks for continuing with this, and sorry for the slow response. This is now doing the DOM-diffing every second, which we were trying to avoid due to performance issues on low-end devices. I'll try and think of a way around it. |
Use the Page Visibility API to allow for pausing/resuming the game timer.
Closes #477