Page MenuHomeWildfire Games

Pickrandom changes from randomPatch
ClosedPublic

Authored by bb on Mar 15 2017, 8:42 PM.

Details

Summary

Splitted from D121, changes including pickRandom in rms.

Test Plan

Test changed random maps and scripts.

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

bb created this revision.Mar 15 2017, 8:42 PM
Owners added a subscriber: Restricted Owners Package.Mar 15 2017, 8:42 PM
Vulcan added a subscriber: Vulcan.Mar 15 2017, 8:43 PM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/522/
See console output for more information: http://jw:8080/job/phabricator/522/console

elexis added a subscriber: elexis.Mar 16 2017, 5:50 PM

30 pickRandom uses in this proposal.
29 uses in my branch that was based on your patch in D121, not sure where that one is coming from, dont care enough to look it up now.

In the last D121 patch you had uploaded, after removing all hunks outside of the maps directory,
subtract the setSkySet changes introduced recently, we find 37 uses cat D121.id488.diff | grep pickRandom | grep -v setSkySet | wc -l.
Looking at that patch, we can find few more occurances: alpine_lakes.js f.e.

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

good

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

good

318 ↗(On Diff #802)

good

358 ↗(On Diff #802)

good

359 ↗(On Diff #802)

good

363 ↗(On Diff #802)

As discussed in irc, we pull that dAngle out of that term

370 ↗(On Diff #802)

391 ↗(On Diff #802)

421 ↗(On Diff #802)

444 ↗(On Diff #802)

721 ↗(On Diff #802)

all of that ✓

750 ↗(On Diff #802)

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

binaries/data/mods/public/maps/random/island_stronghold.js
311 ↗(On Diff #802)

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

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

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

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

As discussed in irc, var [cx, cz] = pickRandom(edges); to nuke point

Verify that chain placer function by starting anatolian plateau

478 ↗(On Diff #802)

569 ↗(On Diff #802)

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

binaries/data/mods/public/maps/random/rmgen/terrain.js
71 ↗(On Diff #802)

binaries/data/mods/public/maps/random/rmgen2/setup.js
313 ↗(On Diff #802)

✓ all of that

binaries/data/mods/public/maps/random/syria.js
166 ↗(On Diff #802)

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

As discussed in irc, [playerX[i], playerZ[i]] = pickRandom(placableArea);

Verify by starting unknown nomad

binaries/data/mods/public/maps/skirmishes/Gallic Fields (3).js
3 ↗(On Diff #802)

check

bb updated this revision to Diff 814.Mar 16 2017, 6:34 PM

Fix comments above

Build has FAILED

Link to build: http://jw:8080/job/phabricator/532/
See console output for more information: http://jw:8080/job/phabricator/532/console

Occurances that are absent from the patch of the version Diff 1 because this patch was based on my github branch that hadn't transfered all changes from D121:

  • alpine_lakes.js one occurance
  • island_stronghold.js one occurance
  • heightmap.js had pickRandom change introduced in rP19288
  • heightmap.js in D121 had an incorrect change removing checkPointIndex while that was still used later down
  • islands.js one occurance
binaries/data/mods/public/maps/random/rmgen/misc.js
637 ↗(On Diff #802)

can nuke point var too as you pointed out in irc

elexis added inline comments.Mar 16 2017, 6:46 PM
binaries/data/mods/public/maps/random/caledonian_meadows.js
363 ↗(On Diff #814)

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

NOPE, destructor Destructuring assignment https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment only works with arrays, not objects with other properties

would have to be [placer.x, placer.z] = ...

bb added a comment.Mar 16 2017, 6:50 PM
In D222#8411, @elexis wrote:

Occurances that are absent from the patch of the version Diff 1 because this patch was based on my github branch that hadn't transfered all changes from D121:

  • alpine_lakes.js one occurance
  • island_stronghold.js one occurance
  • islands.js one occurance

Nope they need the surrounding code to be rewritten, can be done, but seems out of scope.

bb updated this revision to Diff 822.Mar 16 2017, 7:38 PM

fix

bb added inline comments.Mar 16 2017, 8:04 PM
binaries/data/mods/public/maps/random/rmgen/library.js
147 ↗(On Diff #822)

This doesn't work, test cycladic and see the crash, placer = pickRandom(pickRandom(areas).points) does work. (pickRandom returns an object right...)

bb updated this revision to Diff 825.Mar 16 2017, 8:07 PM

Revert library change.

Build has FAILED

Link to build: http://jw:8080/job/phabricator/534/
See console output for more information: http://jw:8080/job/phabricator/534/console

Build has FAILED

Link to build: http://jw:8080/job/phabricator/535/
See console output for more information: http://jw:8080/job/phabricator/535/console

Searching for randInt(0, yields no more occurances that can be replaced with pickRandom, and the non-rms part of D121 had been committed already, so the patch is complete too.

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

right, fail while failing.
I'm not convinced though that this is a good change. The returned placer might not be used as of now, but it might be in the future:

/**
 * Helper function for randomly placing areas and groups in the given areas.
 */
function randomizePlacerCoordinatesFromAreas(placer, areas)
{
	warn("BEFORE");
	for (let prop in placer)
		warn(prop);

	placer = pickRandom(pickRandom(areas).points);

	warn("AFTER");
	for (let prop in placer)
		warn(prop);
}

yields:

WARNING: BEFORE
WARNING: elements
WARNING: tileClass
WARNING: avoidSelf
WARNING: x
WARNING: z
WARNING: place

WARNING: AFTER
WARNING: x
WARNING: z
elexis accepted this revision.EditedMar 16 2017, 10:25 PM

Reverted the library.js change to a previous one

Also: (22:01:17) bb_: tested changed maps in atlas

This revision is now accepted and ready to land.Mar 16 2017, 10:25 PM
This revision was automatically updated to reflect the committed changes.