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
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 781
Build 1237: Vulcan BuildJenkins
Build 1236: arc lint + arc unit

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

good

binaries/data/mods/public/maps/random/caledonian_meadows.js
284

good

318

good

358–359

good

359

good

363

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

370

391

421

444

721

all of that ✓

750

binaries/data/mods/public/maps/random/heightmap/heightmap.js
81

binaries/data/mods/public/maps/random/island_stronghold.js
311

binaries/data/mods/public/maps/random/islands.js
253

binaries/data/mods/public/maps/random/rmgen/library.js
146–147

binaries/data/mods/public/maps/random/rmgen/misc.js
636–637

binaries/data/mods/public/maps/random/rmgen/placer.js
158–159

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

Verify that chain placer function by starting anatolian plateau

476

567

binaries/data/mods/public/maps/random/rmgen/randombiome.js
533

binaries/data/mods/public/maps/random/rmgen/terrain.js
71

binaries/data/mods/public/maps/random/rmgen2/setup.js
313

✓ all of that

binaries/data/mods/public/maps/random/syria.js
166

binaries/data/mods/public/maps/random/unknown_nomad.js
1303

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

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
636–637

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

binaries/data/mods/public/maps/random/rmgen/library.js
147

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

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

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.