Page MenuHomeWildfire Games

Get rid of TERRAIN_SEPARATOR
Needs ReviewPublic

Authored by smiley on Jun 26 2018, 6:41 PM.

Details

Reviewers
FeXoR
Trac Tickets
#5001
Summary

Remove the global since it could cause problems when using with actors, as "|" is also the one used for dynamic templates.
This would break pretty much every RMS. Everything needs to updated like the example map. Map fixes would be added if this goes to the commit stage.

Test Plan

Generate the african_plains map added in the diff.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6269
Build 10403: Vulcan BuildJenkins
Build 10402: arc lint + arc unit

Event Timeline

smiley created this revision.Jun 26 2018, 6:41 PM
Owners added a subscriber: Restricted Owners Package.Jun 26 2018, 6:41 PM
Vulcan added a subscriber: Vulcan.Jun 26 2018, 6:46 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (space-unary-ops):
|    | Unexpected space after unary operator '-'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|  31|  31|  * Constants needed for heightmap_manipulation.js
|  32|  32|  */
|  33|  33| const MAX_HEIGHT_RANGE = 0xFFFF / HEIGHT_UNITS_PER_METRE; // Engine limit, Roughly 700 meters
|  34|    |-const MIN_HEIGHT = - SEA_LEVEL;
|    |  34|+const MIN_HEIGHT = -SEA_LEVEL;
|  35|  35| 
|  36|  36| /**
|  37|  37|  * Length of one tile of the terrain grid in metres.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|  67|  67| 	let obstructionSize =
|  68|  68| 		obstruction.Static ?
|  69|  69| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  70|    |-		// Used for gates, should consider the position too
|    |  70|+			// Used for gates, should consider the position too
|  71|  71| 		obstruction.Obstructions ?
|  72|  72| 			new Vector2D(
|  73|  73| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|  68|  68| 		obstruction.Static ?
|  69|  69| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  70|  70| 		// Used for gates, should consider the position too
|  71|    |-		obstruction.Obstructions ?
|    |  71|+			obstruction.Obstructions ?
|  72|  72| 			new Vector2D(
|  73|  73| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|  69|  69| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  70|  70| 		// Used for gates, should consider the position too
|  71|  71| 		obstruction.Obstructions ?
|  72|    |-			new Vector2D(
|    |  72|+				new Vector2D(
|  73|  73| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  75|  75| 			new Vector2D(0, 0);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|  70|  70| 		// Used for gates, should consider the position too
|  71|  71| 		obstruction.Obstructions ?
|  72|  72| 			new Vector2D(
|  73|    |-				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|    |  73|+					Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  75|  75| 			new Vector2D(0, 0);
|  76|  76| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|  71|  71| 		obstruction.Obstructions ?
|  72|  72| 			new Vector2D(
|  73|  73| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  74|    |-				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    |  74|+					Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  75|  75| 			new Vector2D(0, 0);
|  76|  76| 
|  77|  77| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|  72|  72| 			new Vector2D(
|  73|  73| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  75|    |-			new Vector2D(0, 0);
|    |  75|+				new Vector2D(0, 0);
|  76|  76| 
|  77|  77| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|  78|  78| }
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '/*' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
| 249| 249| /**
| 250| 250|  * Create an avoid constraint for the given classes by the given distances
| 251| 251|  */
| 252|    |-function avoidClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 252|+function avoidClasses(/* class1, dist1, class2, dist2, etc*/)
| 253| 253| {
| 254| 254| 	let ar = [];
| 255| 255| 	for (let i = 0; i < arguments.length/2; ++i)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '/*' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
| 265| 265| /**
| 266| 266|  * Create a stay constraint for the given classes by the given distances
| 267| 267|  */
| 268|    |-function stayClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 268|+function stayClasses(/* class1, dist1, class2, dist2, etc*/)
| 269| 269| {
| 270| 270| 	let ar = [];
| 271| 271| 	for (let i = 0; i < arguments.length/2; ++i)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '/*' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/rmgen/library.js
| 281| 281| /**
| 282| 282|  * Create a border constraint for the given classes by the given distances
| 283| 283|  */
| 284|    |-function borderClasses(/*class1, idist1, odist1, class2, idist2, odist2, etc*/)
|    | 284|+function borderClasses(/* class1, idist1, odist1, class2, idist2, odist2, etc*/)
| 285| 285| {
| 286| 286| 	let ar = [];
| 287| 287| 	for (let i = 0; i < arguments.length/3; ++i)

binaries/data/mods/public/maps/random/rmgen/library.js
| 191| function·createTerrain(terrain)
|    | [NORMAL] ESLintBear (consistent-return):
|    | Expected to return a value at the end of function 'createTerrain'.

binaries/data/mods/public/maps/random/rmgen/library.js
|  74| »   »   »   »   Object.keys(obstruction.Obstructions).reduce((width,·key)·=>·width·+·+obstruction.Obstructions[key]["@width"],·0))·:
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required after '{'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|  36|  36| var aBush = "actor|props/flora/bush_medit_sm_dry.xml";
|  37|  37| var aRock = "actor|geology/stone_savanna_med.xml";
|  38|  38| 
|  39|    |-const pForest = [{"texture": tForestFloor, "entity": oPalm}, tForestFloor];
|    |  39|+const pForest = [{ "texture": tForestFloor, "entity": oPalm}, tForestFloor];
|  40|  40| 
|  41|  41| var heightSeaGround = -5;
|  42|  42| var heightLand = 2;
|    | [NORMAL] ESLintBear (object-curly-spacing):
|    | A space is required before '}'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|  36|  36| var aBush = "actor|props/flora/bush_medit_sm_dry.xml";
|  37|  37| var aRock = "actor|geology/stone_savanna_med.xml";
|  38|  38| 
|  39|    |-const pForest = [{"texture": tForestFloor, "entity": oPalm}, tForestFloor];
|    |  39|+const pForest = [{"texture": tForestFloor, "entity": oPalm }, tForestFloor];
|  40|  40| 
|  41|  41| var heightSeaGround = -5;
|  42|  42| var heightLand = 2;
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 124| 124| g_Map.log("Creating dirt patches");
| 125| 125| createLayeredPatches(
| 126| 126| 	[scaleByMapSize(3, 6), scaleByMapSize(5, 10), scaleByMapSize(8, 21)],
| 127|    |-	[[tDirt,tDirt3], [tDirt2,tDirt4]],
|    | 127|+	[[tDirt, tDirt3], [tDirt2,tDirt4]],
| 128| 128| 	[2],
| 129| 129| 	avoidClasses(clWater, 3, clForest, 0, clHill, 0, clDirt, 5, clPlayer, 12),
| 130| 130| 	scaleByMapSize(15, 45),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 124| 124| g_Map.log("Creating dirt patches");
| 125| 125| createLayeredPatches(
| 126| 126| 	[scaleByMapSize(3, 6), scaleByMapSize(5, 10), scaleByMapSize(8, 21)],
| 127|    |-	[[tDirt,tDirt3], [tDirt2,tDirt4]],
|    | 127|+	[[tDirt,tDirt3], [tDirt2, tDirt4]],
| 128| 128| 	[2],
| 129| 129| 	avoidClasses(clWater, 3, clForest, 0, clHill, 0, clDirt, 5, clPlayer, 12),
| 130| 130| 	scaleByMapSize(15, 45),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 159| 159| g_Map.log("Creating metal mines");
| 160| 160| createMines(
| 161| 161| 	[
| 162|    |-		[new SimpleObject(oMetalLarge, 1,1, 0,4)]
|    | 162|+		[new SimpleObject(oMetalLarge, 1, 1, 0,4)]
| 163| 163| 	],
| 164| 164| 	avoidClasses(clWater, 4, clForest, 4, clPlayer, 20, clMetal, 18, clRock, 5, clHill, 4),
| 165| 165| 	clMetal
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 159| 159| g_Map.log("Creating metal mines");
| 160| 160| createMines(
| 161| 161| 	[
| 162|    |-		[new SimpleObject(oMetalLarge, 1,1, 0,4)]
|    | 162|+		[new SimpleObject(oMetalLarge, 1,1, 0, 4)]
| 163| 163| 	],
| 164| 164| 	avoidClasses(clWater, 4, clForest, 4, clPlayer, 20, clMetal, 18, clRock, 5, clHill, 4),
| 165| 165| 	clMetal
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 168| 168| 
| 169| 169| createDecoration(
| 170| 170| 	[
| 171|    |-		[new SimpleObject(aBush, 1,3, 0,1)],
|    | 171|+		[new SimpleObject(aBush, 1, 3, 0,1)],
| 172| 172| 		[new SimpleObject(aRock, 1,2, 0,1)]
| 173| 173| 	],
| 174| 174| 	[
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 168| 168| 
| 169| 169| createDecoration(
| 170| 170| 	[
| 171|    |-		[new SimpleObject(aBush, 1,3, 0,1)],
|    | 171|+		[new SimpleObject(aBush, 1,3, 0, 1)],
| 172| 172| 		[new SimpleObject(aRock, 1,2, 0,1)]
| 173| 173| 	],
| 174| 174| 	[
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 169| 169| createDecoration(
| 170| 170| 	[
| 171| 171| 		[new SimpleObject(aBush, 1,3, 0,1)],
| 172|    |-		[new SimpleObject(aRock, 1,2, 0,1)]
|    | 172|+		[new SimpleObject(aRock, 1, 2, 0,1)]
| 173| 173| 	],
| 174| 174| 	[
| 175| 175| 		scaleByMapSize(8, 131),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 169| 169| createDecoration(
| 170| 170| 	[
| 171| 171| 		[new SimpleObject(aBush, 1,3, 0,1)],
| 172|    |-		[new SimpleObject(aRock, 1,2, 0,1)]
|    | 172|+		[new SimpleObject(aRock, 1,2, 0, 1)]
| 173| 173| 	],
| 174| 174| 	[
| 175| 175| 		scaleByMapSize(8, 131),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 180| 180| 
| 181| 181| g_Map.log("Creating giraffes");
| 182| 182| var group = new SimpleGroup(
| 183|    |-	[new SimpleObject(oGiraffe, 2,4, 0,4), new SimpleObject(oGiraffe2, 0,2, 0,4)],
|    | 183|+	[new SimpleObject(oGiraffe, 2, 4, 0,4), new SimpleObject(oGiraffe2, 0,2, 0,4)],
| 184| 184| 	true, clFood
| 185| 185| );
| 186| 186| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 180| 180| 
| 181| 181| g_Map.log("Creating giraffes");
| 182| 182| var group = new SimpleGroup(
| 183|    |-	[new SimpleObject(oGiraffe, 2,4, 0,4), new SimpleObject(oGiraffe2, 0,2, 0,4)],
|    | 183|+	[new SimpleObject(oGiraffe, 2,4, 0, 4), new SimpleObject(oGiraffe2, 0,2, 0,4)],
| 184| 184| 	true, clFood
| 185| 185| );
| 186| 186| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 180| 180| 
| 181| 181| g_Map.log("Creating giraffes");
| 182| 182| var group = new SimpleGroup(
| 183|    |-	[new SimpleObject(oGiraffe, 2,4, 0,4), new SimpleObject(oGiraffe2, 0,2, 0,4)],
|    | 183|+	[new SimpleObject(oGiraffe, 2,4, 0,4), new SimpleObject(oGiraffe2, 0, 2, 0,4)],
| 184| 184| 	true, clFood
| 185| 185| );
| 186| 186| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 180| 180| 
| 181| 181| g_Map.log("Creating giraffes");
| 182| 182| var group = new SimpleGroup(
| 183|    |-	[new SimpleObject(oGiraffe, 2,4, 0,4), new SimpleObject(oGiraffe2, 0,2, 0,4)],
|    | 183|+	[new SimpleObject(oGiraffe, 2,4, 0,4), new SimpleObject(oGiraffe2, 0,2, 0, 4)],
| 184| 184| 	true, clFood
| 185| 185| );
| 186| 186| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 185| 185| );
| 186| 186| createObjectGroupsDeprecated(group, 0,
| 187| 187| 	avoidClasses(clWater, 3, clPlayer, 20, clFood, 11, clHill, 4),
| 188|    |-	scaleByMapSize(4,12), 50
|    | 188|+	scaleByMapSize(4, 12), 50
| 189| 189| );
| 190| 190| 
| 191| 191| g_Map.log("Creating elephants");
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 190| 190| 
| 191| 191| g_Map.log("Creating elephants");
| 192| 192| group = new SimpleGroup(
| 193|    |-	[new SimpleObject(oElephant, 2,4, 0,4), new SimpleObject(oElephant2, 0,2, 0,4)],
|    | 193|+	[new SimpleObject(oElephant, 2, 4, 0,4), new SimpleObject(oElephant2, 0,2, 0,4)],
| 194| 194| 	true, clFood
| 195| 195| );
| 196| 196| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 190| 190| 
| 191| 191| g_Map.log("Creating elephants");
| 192| 192| group = new SimpleGroup(
| 193|    |-	[new SimpleObject(oElephant, 2,4, 0,4), new SimpleObject(oElephant2, 0,2, 0,4)],
|    | 193|+	[new SimpleObject(oElephant, 2,4, 0, 4), new SimpleObject(oElephant2, 0,2, 0,4)],
| 194| 194| 	true, clFood
| 195| 195| );
| 196| 196| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 190| 190| 
| 191| 191| g_Map.log("Creating elephants");
| 192| 192| group = new SimpleGroup(
| 193|    |-	[new SimpleObject(oElephant, 2,4, 0,4), new SimpleObject(oElephant2, 0,2, 0,4)],
|    | 193|+	[new SimpleObject(oElephant, 2,4, 0,4), new SimpleObject(oElephant2, 0, 2, 0,4)],
| 194| 194| 	true, clFood
| 195| 195| );
| 196| 196| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 190| 190| 
| 191| 191| g_Map.log("Creating elephants");
| 192| 192| group = new SimpleGroup(
| 193|    |-	[new SimpleObject(oElephant, 2,4, 0,4), new SimpleObject(oElephant2, 0,2, 0,4)],
|    | 193|+	[new SimpleObject(oElephant, 2,4, 0,4), new SimpleObject(oElephant2, 0,2, 0, 4)],
| 194| 194| 	true, clFood
| 195| 195| );
| 196| 196| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 195| 195| );
| 196| 196| createObjectGroupsDeprecated(group, 0,
| 197| 197| 	avoidClasses(clWater, 3, clPlayer, 20, clFood, 11, clHill, 4),
| 198|    |-	scaleByMapSize(4,12), 50
|    | 198|+	scaleByMapSize(4, 12), 50
| 199| 199| );
| 200| 200| 
| 201| 201| g_Map.log("Creating lions");
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 200| 200| 
| 201| 201| g_Map.log("Creating lions");
| 202| 202| group = new SimpleGroup(
| 203|    |-	[new SimpleObject(oLion, 0,1, 0,4), new SimpleObject(oLioness, 2,3, 0,4)],
|    | 203|+	[new SimpleObject(oLion, 0, 1, 0,4), new SimpleObject(oLioness, 2,3, 0,4)],
| 204| 204| 	true, clFood
| 205| 205| );
| 206| 206| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 200| 200| 
| 201| 201| g_Map.log("Creating lions");
| 202| 202| group = new SimpleGroup(
| 203|    |-	[new SimpleObject(oLion, 0,1, 0,4), new SimpleObject(oLioness, 2,3, 0,4)],
|    | 203|+	[new SimpleObject(oLion, 0,1, 0, 4), new SimpleObject(oLioness, 2,3, 0,4)],
| 204| 204| 	true, clFood
| 205| 205| );
| 206| 206| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 200| 200| 
| 201| 201| g_Map.log("Creating lions");
| 202| 202| group = new SimpleGroup(
| 203|    |-	[new SimpleObject(oLion, 0,1, 0,4), new SimpleObject(oLioness, 2,3, 0,4)],
|    | 203|+	[new SimpleObject(oLion, 0,1, 0,4), new SimpleObject(oLioness, 2, 3, 0,4)],
| 204| 204| 	true, clFood
| 205| 205| );
| 206| 206| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 200| 200| 
| 201| 201| g_Map.log("Creating lions");
| 202| 202| group = new SimpleGroup(
| 203|    |-	[new SimpleObject(oLion, 0,1, 0,4), new SimpleObject(oLioness, 2,3, 0,4)],
|    | 203|+	[new SimpleObject(oLion, 0,1, 0,4), new SimpleObject(oLioness, 2,3, 0, 4)],
| 204| 204| 	true, clFood
| 205| 205| );
| 206| 206| createObjectGroupsDeprecated(group, 0,
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/african_plains.js
| 205| 205| );
| 206| 206| createObjectGroupsDeprecated(group, 0,
| 207| 207| 	avoidClasses(clWater, 3, clPlayer, 20, clFood, 11, clHill, 4),
| 208|    |-	scaleByMapSize(4,12), 50
|    | 208|+	scaleByMapSize(4, 12), 50
| 209| 209| );
| 210| 210| 
| 211| 211| createFood(

Link to build: https://jenkins.wildfiregames.com/job/differential/667/display/redirect

smiley updated the Trac tickets for this revision.Jun 26 2018, 6:47 PM
FeXoR added a reviewer: FeXoR.Jul 6 2018, 9:24 AM
FeXoR added a subscriber: FeXoR.

Getting rid of a single string with a separator character is definitely a good idea independent of the same separator in the templates (though I wonder if PPL adding stuff to the templates that brake most random maps should mind a bit more about what they are doing...).
This will make long sets of "simpleTerrain" much longer (and somewhat harder to read) though so I'd wait a bit for alternative solutions that may come up.

What about reviewers? Lacking manpower or what? ;p

smiley added a subscriber: nani.Sep 14 2018, 6:57 PM

@nani since you have been working with rmgen and mapscripts, whats your opinion on this if any?

nani added a comment.EditedSep 14 2018, 11:59 PM

Although I didn't look at Terrain's methods in detail I can see this as clear improvement in code clarity. It might be more verbose but it lets you identify the intended meaning much faster.

To make this more clear: I'm for it and will review the patch when commits are allowed again.
Is that OK?

To make this more clear: I'm for it and will review the patch when commits are allowed again.
Is that OK?

Sure, no point in reviewing if it cant be commited. :p

smiley added a comment.EditedSep 18 2018, 8:05 PM

Performance was not something I considered before. Object creation has way more overhead than native types, a string in this case. Since terrain entities are used for forests usually, one can expect a couple hundred of objects to be created in the least. Overhead could add up and be noticeable. Maybe, it wont be much of a difference and it could be negligible. But it has to be considered. Especially in rmgen which already scales pretty badly.

I would do some performance tests soon. TBH, expecting a performance regression. Just hoping it wont be by much :p

smiley marked an inline comment as done.Mon, Nov 19, 2:20 PM
smiley added inline comments.
binaries/data/mods/public/maps/random/rmgen/library.js
191

Maybe this proxy ought to be removed too.

FeXoR added inline comments.Mon, Nov 19, 11:13 PM
binaries/data/mods/public/maps/random/rmgen/library.js
191

It's used 24 times, some in maps and some in libs.
If we want to stick to simple terrain (we should agree to one name and use that whenever it's used IMO) how what would you propose instead?
Otherwise maps would need a lot of "+randFloat()" or "+0.5" or whatever for x/y shifts of entities on the corresponding tile.

Or what do you mean?

FeXoR added a comment.Tue, Nov 20, 1:25 AM

At the moment we have:

  • Terrain strings, those containing the terrain separator
  • simpleTerrain objects
  • randomterrain objects
  • Texture strings in the Map object
  • Terrain entities in the map object

did I miss something related?

This is a bit messy and misleading (which type of argument does what function except?) and IMO can be made clearer.

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

The function does not always return something.
If not clear what to do with the given argument throw an error.

elexis added a subscriber: elexis.Tue, Nov 20, 6:41 PM
elexis added inline comments.
binaries/data/mods/public/maps/random/african_plains.js
39

space after { and before }

Object looks reasonable, the alternative would be an array with 2 items.

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

Yes, it would be better to weed that out like the rest of the library.js functions and constants (#4804, #4964) and use the SimpleTerrain or RandomTerrain constructors to remove indirection (possibly extending RandomTerrain type to support string input, or not).
Possibly better in a separate commit.

There is no 0.5 in this function, we would only refactor the existing code, so still same mapgen with cleaner code.

196

Erroring in the third case sounds reasonable.
Also no else after return (if it doesnt change the code flow)

elexis added inline comments.Tue, Nov 20, 6:50 PM
binaries/data/mods/public/maps/random/rmgen/library.js
191

or actually setTexture to avoid some useless object construction (In particular as the g_Map constant shouldn't be hardcoded in the library).
Also one could investigate whether all the const tFoo = should become Terrain instances rather than strings, dunno (sounds like work)

FeXoR added inline comments.Tue, Nov 20, 9:56 PM
binaries/data/mods/public/maps/random/rmgen/library.js
191

I'm for replacing the strings with object (or arrays but objects would be easier to check for when the object's values can be arrays).

Yea, +0.5/randFloat() (shifting terrain entities to the center of the tile) is done somewhere else - so unrelated.

Perhaps instead of an object with 2 properties or an arry with 2 items, it would even be cleaner to use a function / constructor with 2 arguments (texture, entity = undefined), like using SimpleTerrain or RandomTerrain which receives an array of SimpleTerrain?

This comment was removed by FeXoR.