Page MenuHomeWildfire Games

Random function cleanup
ClosedPublic

Authored by bb on Feb 3 2017, 8:30 PM.

Details

Summary

Merge and cleanup all random number generating functions.

It is meant that this patch does not change the code, just clean it up. They are a few places where the code is changed a tiny bit (off by one errors). Also this patch only cleans up style errors in the random function, not other style errors in code.

Test Plan

Test generation of random maps.

Check for typo's or other style errors.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Feb 20 2017, 3:55 AM
binaries/data/mods/public/maps/random/unknown.js
651 ↗(On Diff #488)

refs randbool

667 ↗(On Diff #488)

refs randbool

676 ↗(On Diff #488)

refs randbool

685 ↗(On Diff #488)

randbool

691 ↗(On Diff #488)

spaces, ++i

718 ↗(On Diff #488)

spaces, ++i

730 ↗(On Diff #488)

refs randbool

737 ↗(On Diff #488)

refs randbool

755 ↗(On Diff #488)

refs randbool

766 ↗(On Diff #488)

refs randbool

768 ↗(On Diff #488)

refs randbool

825 ↗(On Diff #488)

randbool

919 ↗(On Diff #488)

k, refs randbool

948 ↗(On Diff #488)

randbool

967 ↗(On Diff #488)

refs randbool

992 ↗(On Diff #488)

refs randbool

1034 ↗(On Diff #488)

fine, could be > 1 too, whatever

1076 ↗(On Diff #488)

not sure why this isnt a >= 2 as well, dont care

1154 ↗(On Diff #488)

1159 ↗(On Diff #488)

spaces, newlines

1176 ↗(On Diff #488)

same

1739 ↗(On Diff #488)

bergh ✓

1771 ↗(On Diff #488)

1786 ↗(On Diff #488)

1836 ↗(On Diff #488)

1892 ↗(On Diff #488)

✓ spaces

1900 ↗(On Diff #488)

✓ spaces

binaries/data/mods/public/maps/random/unknown_land.js
249 ↗(On Diff #488)

0, 1

265 ↗(On Diff #488)

refs randbool

489 ↗(On Diff #488)

refs randbool

505 ↗(On Diff #488)

refs randbool

552 ↗(On Diff #488)

☑ spaces unneeded parenthesis, ++i

660 ↗(On Diff #488)

randbool

728 ↗(On Diff #488)

randbool

801 ↗(On Diff #488)

good

1118 ↗(On Diff #488)

As there is some stuff going on with lakeAreaLen below, cant nuke that var on first sight

1509 ↗(On Diff #488)

✓ :-X

1541 ↗(On Diff #488)

1556 ↗(On Diff #488)

1669 ↗(On Diff #488)

✓ spaces, above too

1733 ↗(On Diff #488)

✓ spaces

1814 ↗(On Diff #488)

binaries/data/mods/public/maps/random/unknown_nomad.js
109 ↗(On Diff #488)

good! 0.80 -> 0.8

126 ↗(On Diff #488)

✓ spaces, 0.8, 0

143 ↗(On Diff #488)

✓ spaces

187 ↗(On Diff #488)

214 ↗(On Diff #488)

✓, spaces 0.80, .0.0

231 ↗(On Diff #488)

0.80 ✓

259 ↗(On Diff #488)

0, 1

363 ↗(On Diff #488)

unneeded parenthesis, ✓

402 ↗(On Diff #488)

0.80, ✓

475 ↗(On Diff #488)

randbool

772 ↗(On Diff #488)

elexis accepted this revision.Feb 20 2017, 4:05 AM

Notice review so big that you have to click "Show older changes" 4 times to see it entirely!
Applied fixes to all mistakes found and proposed style changes locally, sent it to bb on saturday and got a quick review of them.
Still contains unreviewed whitespace and parenthesis changes as maps/random/ is ugly as sin.

This revision is now accepted and ready to land.Feb 20 2017, 4:05 AM
fatherbushido added inline comments.Feb 20 2017, 1:24 PM
binaries/data/mods/public/globalscripts/random.js
44 ↗(On Diff #439)

Here is for example documentation of analog functions in scilab:

uniform (unf)

Y=grand(m,n,'unf',Low,High) generates random reals uniformly distributed in [Low, High).

uniform (uin)

Y=grand(m,n,'uin',Low,High) generates random integers uniformly distributed between Low and High (included). High and Low must be integers such that (High-Low+1) < 2,147,483,561.

You can also look at numpy.random.randint and numpy.random.random_integers.

Or to other such functions.

(else using Math.random() everytime works too as in this case we know what we get ;-))

elexis added inline comments.Feb 20 2017, 3:42 PM
binaries/data/mods/public/globalscripts/random.js
44 ↗(On Diff #439)

randIntInclusive(+this.template.FeedTimeMin, +this.template.FeedTimeMax) seems more readable though than

Math.floor(+this.template.FeedTimeMin + Math.random(+this.template.FeedTimeMax - +this.template.FeedTimeMin + 1))
in particular if we have hundreds of occurances of such things.

Also using Math.round here still doesn't mean that we don't have that non-uniform distribution issue, right? It would just be moved to each occurance, unless we'd add 3 more rounds afaics.

The function of the documentation you have quoted here avoid that issue by enforcing integer arguments. So if we take this as a serious problem, then we could add those rounds here, but not convinced it is.

fatherbushido added inline comments.Feb 20 2017, 4:10 PM
binaries/data/mods/public/globalscripts/random.js
44 ↗(On Diff #439)

I expect (but that's subjective) that randIntInclusive(1.6, 5.2) and randintExclusive(1.6, 5.2) return the same thing (if we want to use that kind of stuff) ie random integers numbers uniformly distributed in {2, 3, 4, 5}.

bb added inline comments.Feb 21 2017, 4:49 PM
binaries/data/mods/public/maps/random/caledonian_meadows.js
444 ↗(On Diff #488)

rly, foo[foo.length] does not exists afaiks, ([a,b][2]...) and foodEntities.length - 1 was included right

binaries/data/mods/public/maps/random/rmgen2/gaia.js
393 ↗(On Diff #488)

If elevation var allows Floats, we should use randFloat instead of randInt*.

binaries/data/mods/public/maps/random/sahel.js
46 ↗(On Diff #488)

the sin/cos are multiplied, don't you see the 0* sin/cos and 1*sin/cos?

binaries/data/mods/public/maps/random/sahel_watering_holes.js
172 ↗(On Diff #488)

whoop whoop

binaries/data/mods/public/maps/random/unknown.js
919 ↗(On Diff #488)

isn't randbool here, since 1/3 chance of true and 2/3 of false

1076 ↗(On Diff #488)

Because of cases

elexis added a subscriber: mimo.Feb 23 2017, 1:44 PM
elexis added inline comments.
binaries/data/mods/public/globalscripts/random.js
44 ↗(On Diff #439)

@mimo do you have an opinion on this issue?

Does the "Random numbers is a function which should be as fast as possible" argument from http://trac.wildfiregames.com/ticket/4326#comment:11 apply here?

I kind of share that view with that regards.
If a uniform distribution is important (which doesn't seem to be the case with random map scripts or the nr of garrisoned arrows being shot in the current round), then people should lookup the function they use to ensure that, no?

If not, we could change the function to return Math.floor(min) + Math.floor(Math.random() * (Math.floor(max) + 1 - Math.floor(min)));, or the equivalent with Math.round.

binaries/data/mods/public/maps/random/gulf_of_bothnia.js
263 ↗(On Diff #488)

It happened after staging about 700 hunks in maps/random/ x.x

mimo added inline comments.Feb 24 2017, 6:52 PM
binaries/data/mods/public/globalscripts/random.js
44 ↗(On Diff #439)

I've no strong opinions: i would expect that it is rather the caller which should replace randIntInclusive(x1,x2) by randIntInclusive(Math.floor(x1), Math.floor(x2)) so that he controls what he will get.

But i'm fine with the latest version you wrote, possibly with an intermediate variable for Math.floor(min).

elexis added a comment.Mar 3 2017, 6:11 PM

This will be committed in two parts: one diff containing all sim and gui changes, the other one all RMS changes. We can't really split it into more logicalyl unrelated fragments afaics.
As mentioned above I had applied some style mistakes, spaces and the like to a branch which was reviewed and tested by bb again today.

binaries/data/mods/public/globalscripts/random.js
44 ↗(On Diff #439)

We keep it as is for now, added an example and removed the "Math.random library" comment.
We can still change it in a follow up commit in a separate proposal which is likely read by more people as this proposal is really crowded.

26 ↗(On Diff #488)

JSdoc comment

binaries/data/mods/public/simulation/components/Formation.js
724 ↗(On Diff #488)

Was changed to (-1,1) * factor to be inline with UnitAI jitter thing

elexis added a comment.Mar 3 2017, 8:16 PM

The megareview only contained the correctness check.

But it's also complete as all RandomInt occurances all gone.
For completeness also changing the remaining five occurances in the three AI files from Math.Random to randFloat(0, 1).

binaries/data/mods/public/gui/common/functions_utility.js
9 ↗(On Diff #488)

Notice our JS Math.random function comes from ScriptInterface.cpp and is defined from ReplaceNondeterministicRNG.
The function using that uses http://en.cppreference.com/w/cpp/numeric/random/uniform_real_distribution whose seed was initialized in rP18604.

Not sure how this should be unified, likely not.

Some places that are not synced like GameSetup.cpp (for autostarted games) use std::rand(). Unifying not worthy.

In case someone finds something worthy do change in C++ -> other proposal

elexis added a comment.Mar 3 2017, 9:39 PM

Final test concludes that the patch doesn't change the simstate besides the discussed UnitAI and BuildingAI change.

Here a 1v1 on the polar sea map between borg adnd Hannibal that proves that the simstate is not changed if those 2 changes are reverted

.

binaries/data/mods/public/simulation/components/UnitAI.js
3233 ↗(On Diff #488)

RandomInt was exclusive, so this changes simstate, but the change is legitimate.
Those are the only 2 occurances of the old randomInt function!

Thanks for the second of three big patches!
The reviewed cleanup of the 60+ random map script files will be part of a separate commit.
The duplicate helper functions with slight variations have been annoying and I'm glad they're gone!