Page MenuHomeWildfire Games

Cleanup Timer.js and update documentation
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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

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
60–61

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
60–61

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
60–61

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
60–61

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
60–61

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
60–61

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–39

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

35

period

52

period

55

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

the entity the timer does not look correct. ^^

61

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

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

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.Sun, Dec 1, 4:18 PM
binaries/data/mods/public/simulation/components/Timer.js
133

timer.repeatTime == 0

Stan added inline comments.Sun, Dec 1, 4:35 PM
binaries/data/mods/public/simulation/components/Timer.js
133

Any reason to make it more specific?

Polakrity added inline comments.Sun, Dec 1, 4:51 PM
binaries/data/mods/public/simulation/components/Timer.js
133

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.Sun, Dec 1, 5:00 PM
Stan added inline comments.
binaries/data/mods/public/simulation/components/Timer.js
133

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 :)