Page MenuHomeWildfire Games

randInt changes for random maps
ClosedPublic

Authored by bb on Mar 30 2017, 5:53 PM.

Details

Summary

randInt changes from randomPatch excluding unknown* those require more changes all over those files, so needs to be done separately.

refs D121

Test Plan

Load maps in atlas.

See that the new and old code is equivalent.

grep on "randInt(" and see that all remaining ones are in unkown* (and function declaration).

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
Owners added a subscriber: Restricted Owners Package.Mar 30 2017, 5:53 PM
bb updated this revision to Diff 1014.Mar 30 2017, 6:13 PM

Belgian uplands was forgotten in last patch :/

Vulcan added a subscriber: Vulcan.Mar 30 2017, 6:39 PM

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/643/ for more details.

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/644/ for more details.

elexis added inline comments.Mar 31 2017, 5:04 AM
binaries/data/mods/public/maps/random/canyon.js
227 ↗(On Diff #1013)

whitespace

246 ↗(On Diff #1013)

One might wonder whether the - 1 was an accident, but looks very much like it was intentional, considering that we also have +1 at the start

binaries/data/mods/public/maps/random/cycladic_archipelago.js
342 ↗(On Diff #1013)

good

binaries/data/mods/public/maps/random/deep_forest.js
90 ↗(On Diff #1013)

While this is nicely readable, I'm a duplication avoider fetishist

bb added inline comments.Mar 31 2017, 11:48 AM
binaries/data/mods/public/maps/random/deep_forest.js
90 ↗(On Diff #1013)

What do you suggest, storing the Math.cos/sin under a variable or using randFloat and Math.round?

elexis accepted this revision.Apr 21 2017, 11:41 PM

Also tested a bunch of maps in atlas, some of those that were not obvious, deep forest, latium, kerala. flood.

So it seems the only thing that is remaining for the random function file to be closed is the duplication removal of the unknown* maps.

-> Thanks

binaries/data/mods/public/maps/random/african_plains.js
317 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/alpine_lakes.js
378 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/alpine_valley.js
254 ↗(On Diff #1014)

570 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/anatolian_plateau.js
316 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/archipelago.js
194 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/ardennes_forest.js
569 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/atlas_mountains.js
276 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/belgian_uplands.js
233 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/caledonian_meadows.js
312 ↗(On Diff #1014)

385 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/cantabrian_highlands.js
324 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/canyon.js
231 ↗(On Diff #1014)

✓, ix, iz, placer still defined globally, ix2, iz2 not refered to anywhere else

248 ↗(On Diff #1014)

randIntInclusive 1, ... -1 seems better to read here as it highlights that we remove 1 tile from both sides

506 ↗(On Diff #1014)

507 ↗(On Diff #1014)

✓ (whitespace fix only)

509 ↗(On Diff #1014)

510 ↗(On Diff #1014)

✓ (whitespace fix only)

binaries/data/mods/public/maps/random/cycladic_archipelago.js
342 ↗(On Diff #1013)

binaries/data/mods/public/maps/random/deep_forest.js
90 ↗(On Diff #1013)

The latter IMO

binaries/data/mods/public/maps/random/empire.js
10 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/english_channel.js
374 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/flood.js
227 ↗(On Diff #1014)

✓, randMountains not refered to anywhere else

230 ↗(On Diff #1014)

randX, randY not refered to anywhere else

238 ↗(On Diff #1014)

239 ↗(On Diff #1014)

252 ↗(On Diff #1014)

262 ↗(On Diff #1014)

264 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/fortress.js
344 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/frontier.js
13 ↗(On Diff #1014)

✓, guess could remain 30 without making a big difference, but keeping the same function here is good too

15 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/guadalquivir_river.js
383 ↗(On Diff #1014)

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

456 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/heightmap/heightmap.js
142 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/hyrcanian_shores.js
456 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/islands.js
495 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/kerala.js
207 ↗(On Diff #1014)

One function call less, one factor more, well, fine

436 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/latium.js
510 ↗(On Diff #1014)

With this whitespace, it isn't entirely obvious what the first and second argument of that function is

518 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/lorraine_plain.js
213 ↗(On Diff #1014)

430 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/migration.js
423 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/northern_lights.js
189 ↗(On Diff #1014)

fine

binaries/data/mods/public/maps/random/persian_highlands.js
247 ↗(On Diff #1014)

398 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/phoenician_levant.js
446 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/pyrenean_sierra.js
748 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/rhine_marshlands.js
384 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/rmgen/library.js
138 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/rmgen/misc.js
639 ↗(On Diff #1014)

788 ↗(On Diff #1014)

After 10+ minutes of computing things back and forth, reverting this and adding whitespace and Math only

binaries/data/mods/public/maps/random/rmgen/placer.js
161 ↗(On Diff #1014)

366 ↗(On Diff #1014)

same as below

437 ↗(On Diff #1014)

✓ count not used anywhere else

454 ↗(On Diff #1014)

This should be done in an isolated patch, it looks very much like it fixes a bug

binaries/data/mods/public/maps/random/rmgen/randombiome.js
65 ↗(On Diff #1014)

151 ↗(On Diff #1014)

168 ↗(On Diff #1014)

448 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/rmgen/utilityfunctions.js
73 ↗(On Diff #1014)

binaries/data/mods/public/maps/random/rmgen/wall_builder.js
864 ↗(On Diff #1014)

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

393 ↗(On Diff #1014)

this appears wrong.

Example:
min = 1, max =3

before it was 1 + randIntExcl(3-1) = 1 + randIntExcl(2) i.e. in (1, 2), now it is in (1, 3)

595 ↗(On Diff #1014)

1266 ↗(On Diff #1014)

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

312 ↗(On Diff #1014)

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

This revision is now accepted and ready to land.Apr 21 2017, 11:41 PM
This revision was automatically updated to reflect the committed changes.