Use the randBool function in public mod code.
Change this function that it takes 1 argument which is the chance on true. We have a Bernoulli distribution now.
Also one forgotten instance of pickRandom is added in this patch.
refs D121
Details
Details
- Reviewers
elexis - Commits
- rP19355: Replace many deprecated randInt calls with randBool.
- Trac Tickets
- #4326
Load changed random maps in atlas.
Notice that the changes are logicically equivalent to the previous code.
Diff Detail
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
Comment Actions
0.5 should be the default IMO. In general its good to avoid default values, but here we only have one value and the average use of the function will be 0.5. If coders are not aware of the argument, they wil llikely mean right that value, no?
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/542/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/545/ for more details.
Comment Actions
Have to admit, it's really a bit shorter and less thinking work with the randBool notation for non-0.5 probabilities, not bad.
Only the changes in belgian uplands are an example of where the changes don't improve notably.
Very few TODOs, mostly change 0.33 to 1/3 and 0.66 to 2/3 IMO, dunno if there are more occurances. going to continue review then
| binaries/data/mods/public/globalscripts/random.js | ||
|---|---|---|
| 65 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/aegean_sea.js | ||
| 255 ↗ | (On Diff #852) | ✓ (in particular < is more correct than <= and >) |
| binaries/data/mods/public/maps/random/alpine_lakes.js | ||
| 4 ↗ | (On Diff #852) | ✓, variable not used anymore |
| binaries/data/mods/public/maps/random/alpine_valley.js | ||
| 6 ↗ | (On Diff #852) | ✓ same |
| binaries/data/mods/public/maps/random/archipelago.js | ||
| 217 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/ardennes_forest.js | ||
| 400 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/belgian_uplands.js | ||
| 304 ↗ | (On Diff #852) | ✓ and below, doesn't add something notably though |
| binaries/data/mods/public/maps/random/caledonian_meadows.js | ||
| 714 ↗ | (On Diff #852) | ✓ |
| 720 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/continent.js | ||
| 229 ↗ | (On Diff #852) | ✓ good |
| binaries/data/mods/public/maps/random/corinthian_isthmus.js | ||
| 252 ↗ | (On Diff #852) | ✓ good |
| binaries/data/mods/public/maps/random/corsica.js | ||
| 109 ↗ | (On Diff #852) | ✓ even slightly better with a bool instead of 0/1 |
| 193 ↗ | (On Diff #852) | ✓, adding a space though. Also not questioning whether the numbers in that rms make sense. I'm suspecting += was meant, but not going to change it now. |
| binaries/data/mods/public/maps/random/deep_forest.js | ||
| 229 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/english_channel.js | ||
| 230 ↗ | (On Diff #852) | Indeed better than Math.sign(randFloat(-1, 1)) |
| 231 ↗ | (On Diff #852) | ✓, Looked wrong at first sight, then I noticed that this change I introduced in my branch was actually correct, as randInt(2) returns either 0 or 1 lol, so randInt(2) - 0.5 is either -0.5 or +0.5 |
| binaries/data/mods/public/maps/random/gear.js | ||
| 357 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/gulf_of_bothnia.js | ||
| 353 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/lake.js | ||
| 242 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/latium.js | ||
| 302 ↗ | (On Diff #852) | ✓ |
| 336 ↗ | (On Diff #852) | ✓ |
| 343 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/lorraine_plain.js | ||
| 221 ↗ | (On Diff #852) | ✓ but space |
| 223 ↗ | (On Diff #852) | let |
| binaries/data/mods/public/maps/random/mainland.js | ||
| 190 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/oasis.js | ||
| 246 ↗ | (On Diff #852) | ✓ |
| 358 ↗ | (On Diff #852) | ✓ |
| 365 ↗ | (On Diff #852) | ✓ |
| 369 ↗ | (On Diff #852) | ✓ in particular seeing your point that its bad to sometimes have < and other times > when comparing probability |
| 375 ↗ | (On Diff #852) | ✓ |
| 379 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/persian_highlands.js | ||
| 5 ↗ | (On Diff #852) | ✓ var not used anymore |
| 31 ↗ | (On Diff #852) | ✓ same numbersas below |
| binaries/data/mods/public/maps/random/polar_sea.js | ||
| 324 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/rivers.js | ||
| 372 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/rmgen/randombiome.js | ||
| 113 ↗ | (On Diff #852) | ✓ |
| 300 ↗ | (On Diff #852) | ✓ |
| 312 ↗ | (On Diff #852) | ✓ |
| 459 ↗ | (On Diff #852) | ✓ |
| 464 ↗ | (On Diff #852) | ✓ |
| 475 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/rmgen2/setup.js | ||
| 521 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/schwarzwald.js | ||
| 435 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/survivalofthefittest.js | ||
| 222 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/unknown.js | ||
| 280 ↗ | (On Diff #852) | ✓ |
| binaries/data/mods/public/maps/random/unknown_nomad.js | ||
| 475 ↗ | (On Diff #852) | TODO: nuke variable, comment unneeded |
| 491 ↗ | (On Diff #852) | ✓ |
| 499 ↗ | (On Diff #852) | ✓ |
| 631 ↗ | (On Diff #852) | ✓ but |
| 671 ↗ | (On Diff #852) | ✓ |
| 984 ↗ | (On Diff #852) | ✓ but |
| binaries/data/mods/public/simulation/ai/petra/diplomacyManager.js | ||
| 280 ↗ | (On Diff #852) | ✓ |
| 299 ↗ | (On Diff #852) | according to leperstandards, must have the same amount of tabs indentation as the line above, then spaces (even though the same argument didn't make it into aligning object properties yet). |
Comment Actions
Also those changed lines in the unknown*js files should rename mdd1 mdd2 to vertical, horizontal or similar descriptive names (also I was taught in school to avoid the negation in if (!foo) statement1; else statement2; patterns)
Comment Actions
Fixing comments above.
Nuking some mdd* variables in unknown*.js also inverting some cases.
randBool(0.33 || 0.66) =>randBool(1/3 || 2/3)
fix some whitespaces
Changed maps are tested in Atlas
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/574/ for more details.
Comment Actions
The patch is correct and I agree that it is better to have a unifom use of randBool than sometimes using randFloat() > foo, randFloat() < foo, randInt(x,y) > z and more variations.
While I wasn't convinced of Bernoulli function in the beginning, having the 0.5 as an optional argument should be self-evident (as opposed to not having that argument at all).
The Unknown maps are a horror and as agreed upon in IRC, we will exclude the changes in this diff, transport them to D252 and try to remove duplication in the affected hunks there (i.e. better removing instead of changing code).
Thanks for the patch and splitting this from D121. There can't be too many parts of that remaining :D
| binaries/data/mods/public/maps/random/english_channel.js | ||
|---|---|---|
| 226 ↗ | (On Diff #896) | ✓ |