Page MenuHomeWildfire Games

Cleanup Timer.js and update documentation
ClosedPublic

Authored by Stan on Feb 8 2019, 11:31 AM.

Details

Reviewers
bb
Commits
rP23673: Cleanup Timer.js
Summary

For the record once we move to SM45 might be interesting https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap

Add and fix documentation. The current one was incorrect.

Test Plan

Ought to be unproblematic. Check for completeness

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Stan created this revision.Feb 8 2019, 11:31 AM
Vulcan added a subscriber: Vulcan.Feb 8 2019, 11:33 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/Timer.js
|  59| »   if·(typeof·repeattime·!=·"number"·||·!(repeattime·>=·0))
|    | [NORMAL] JSHintBear:
|    | Confusing use of '!'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1053/

Stan updated this revision to Diff 7471.EditedFeb 8 2019, 11:55 AM
  • Fix Vulkan warning
  • @returns -> @return
  • A little example in Vscode

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1054/

Imarok added a subscriber: Imarok.Feb 8 2019, 2:32 PM
Imarok added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
59 ↗(On Diff #7471)

Sure, you're not meaning repeattime <= 0?

Stan marked 2 inline comments as done.Feb 8 2019, 2:37 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
59 ↗(On Diff #7471)

Nope because else the simplification above would error out :D

Imarok added inline comments.Feb 8 2019, 2:44 PM
binaries/data/mods/public/simulation/components/Timer.js
59 ↗(On Diff #7471)

So the change in behaviour is intended.
Then it's ok.

Stan marked 3 inline comments as done.Feb 8 2019, 2:46 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
59 ↗(On Diff #7471)

Yep was wondering if I should have added a comment :) I should have :)

elexis added a subscriber: elexis.Feb 8 2019, 7:29 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
59 ↗(On Diff #7471)

When is a timer with a repeat-time of 0 useful?
Sounds like it would either do an infinite loop or update each time the simulation is updated, which in the worst case of client-dependent progressive updates would trigger an OOS? (I didn't actually check whether they are client dependent, but I suspect they might depend on the renderer framerate, or the rejoining client not running progressive updates, dunno)

Stan marked an inline comment as done.Feb 8 2019, 7:57 PM
Stan marked an inline comment as done.Mar 6 2019, 2:47 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
59 ↗(On Diff #7471)

A timer with a 0 repeat time is a timer that does not repeat itself. So SetInterval with repeattime = 0 = SetTimeout.

Stan marked an inline comment as done.Mar 6 2019, 2:47 PM
Stan updated this revision to Diff 7521.Mar 6 2019, 5:29 PM

Fix comment.

Vulcan added a comment.Mar 6 2019, 5:31 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1082/display/redirect

Stan updated this revision to Diff 7635.Mar 31 2019, 11:27 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1143/display/redirect

bb added a subscriber: bb.Apr 14 2019, 10:00 PM
bb added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
34 ↗(On Diff #7635)

I though we used hyphens and caps, however code convention page made in doubt...

35 ↗(On Diff #7635)

period

52 ↗(On Diff #7635)

period

55 ↗(On Diff #7635)

non-zero says my math teacher and a comma If non-zero, the ...

Stan updated this revision to Diff 7752.Apr 15 2019, 7:56 AM
Stan marked 4 inline comments as done.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1215/display/redirect

You forgot to put the full context to the diff.

Now that you have changed the check in SetInterval,
the two functions are almost similar, it isn't preferable to merge them into a setTimer() or something like that?
But it may be better to keep the 2 names separate for clearer use.

binaries/data/mods/public/simulation/components/Timer.js
34 ↗(On Diff #7752)

the entity the timer does not look correct. ^^

61 ↗(On Diff #7752)

If we check for repeattime parameter, we can also check for time, no?

Also this display an error but continues to be executed despite bad value.

80 ↗(On Diff #7752)

timer ID rather?

Stan marked 3 inline comments as done.Nov 6 2019, 6:10 PM

@Polakrity sorry I never saw your answer hence why you never got one.

binaries/data/mods/public/simulation/components/Timer.js
61 ↗(On Diff #7752)

I actually reduced the number of checks in this version to make things faster. This code is critical for performance. The function below more though though. If people want to do stupid things it will break :)

Stan updated this revision to Diff 10280.Nov 6 2019, 6:11 PM
Stan marked an inline comment as done.

Update.

Vulcan added a comment.Nov 6 2019, 6:13 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/542/display/redirect

Vulcan added a comment.Nov 6 2019, 6:20 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1056/display/redirect

Polakrity added inline comments.Dec 1 2019, 4:18 PM
binaries/data/mods/public/simulation/components/Timer.js
133 ↗(On Diff #10280)

timer.repeatTime == 0

Stan added inline comments.Dec 1 2019, 4:35 PM
binaries/data/mods/public/simulation/components/Timer.js
133 ↗(On Diff #10280)

Any reason to make it more specific?

Polakrity added inline comments.Dec 1 2019, 4:51 PM
binaries/data/mods/public/simulation/components/Timer.js
133 ↗(On Diff #10280)

In case where the repeattime has a string with the "0" value.
But don't care of this inline.
As you said :

@Stan wrote:

If people want to do stupid things it will break :)

Stan marked 2 inline comments as done.Dec 1 2019, 5:00 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
133 ↗(On Diff #10280)

Well in this case you'd need === 0 (three equals), else Js will to the conversion anyway :)
I'll mark the inline as done then :)

Freagarach added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
49 ↗(On Diff #10280)

lateness is within data?

99 ↗(On Diff #10280)

Might it be beneficial to run over the currently active timers and save the ones who need to be destroyed? (Instead of caching all of them beforehand.)
Then after all timers are checked stop those who need to be stopped?

109 ↗(On Diff #10280)

While at it, cmpTimer does not sound like what is meant here.

Stan marked 2 inline comments as done.Mar 22 2020, 5:28 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
49 ↗(On Diff #10280)

Nope. It is on of the arguments passed. Check the OnUpdate function. Data is user defined, lateness is provided.

  • Reusing the function is good.
  • Polishing documentation is nice.
  • No further improvements possible, AFAICS.

Just the question about the cmpTimer.

binaries/data/mods/public/simulation/components/Timer.js
55 ↗(On Diff #10280)

Then one wonders what happens if it *is* zero ;)

99 ↗(On Diff #10280)

Can't be done since timers may cancel eachother during execution.

118 ↗(On Diff #10280)

Ah, this is lateness ^^

61 ↗(On Diff #7752)

Just for the record, a negative repeat time works fine, it'll just keep executing untill something stops the timer.

Freagarach added inline comments.Apr 11 2020, 3:03 PM
binaries/data/mods/public/simulation/components/Timer.js
85 ↗(On Diff #10280)

-micro

Stan updated this revision to Diff 11659.Apr 11 2020, 3:24 PM
Stan marked 4 inline comments as done.
Stan edited the summary of this revision. (Show Details)

Fix some inlines

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/570/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1990/display/redirect

bb accepted this revision as: bb.Sat, May 16, 6:28 PM
bb removed a reviewer: Restricted Owners Package.

linter happy, bb happy

binaries/data/mods/public/simulation/components/Timer.js
41 ↗(On Diff #11659)

Per Freagarach: shouldn't time become lateness? Same for intervals

This revision is now accepted and ready to land.Sat, May 16, 6:28 PM
This revision was automatically updated to reflect the committed changes.