Page MenuHomeWildfire Games

Get rid of TERRAIN_SEPARATOR
AbandonedPublic

Authored by lyv 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

lyv 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

lyv 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

lyv 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?

lyv added a comment.Sep 18 2018, 12:17 PM

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

lyv 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.

lyv added a comment.Sep 18 2018, 8:10 PM

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

lyv marked an inline comment as done.Nov 19 2018, 2:20 PM
lyv 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.Nov 19 2018, 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.Nov 20 2018, 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.Nov 20 2018, 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.Nov 20 2018, 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.Nov 20 2018, 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.
lyv added a comment.Dec 31 2018, 8:20 PM
In D1589#66440, @elexis wrote:

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?

Maybe, I am not really sure. @FeXoR what do you think?

FeXoR added a comment.EditedJan 5 2019, 6:23 PM

Well, I think there are 2 types of random terrain that would be useful:

  1. An array of multiple objects/associative arrays (key/value pairs) of exactly one texture and (optional) entity/actor each
  2. An array of multiple textures and entities/actors to result in a random combination of those

So in the end we would have

  • A "simple" terrain (one texture, no or one entity/actor)
  • A random terrain (multiple textures and multiple entities/actors combined randomly)
  • An array of multiple of those two (resulting in more control over terrain/texture combinations while still being random)

Weather or not the first 2 are objects or just key/value pairs I don't mind. But all 3 should be excepted as arguments for whichever function places them in the end. Thinks will definitely be easier to manage if the first 2 would be clearly distinguishable by type from the 3rd one (which definitely should just be an array IMO).

P.S.: And we should find clear names for them! (E.g. simple_terrain is less simple than a texture obviously. But calling those texture entity combinations "terrain" definitely makes sense to me. So "terrain", "random_terrain" and "terrain_list" do come to mind. Suggestions welcome.)

FeXoR added a comment.EditedJan 5 2019, 8:38 PM

I'm sorry for the confusion that I may have caused because this should have been a plain:
A) I agree with both of you to replace the actor/entity combination strings with objects. Just that { "texture": tForestFloor, "entity": oPalm } would do. Using simpleTerrain right away would also do in cases where there is no array.
B) In the long run (likely a later patch) the simpleTerrain and randomTerrain should be used (what was what I was talking about but is not really related to this patch, so sorry.).

One step after the other I guess ^^

lyv added a comment.Jun 1 2022, 1:18 AM

A terrain refactoring needs to happen at some point and might as well put the requirements here.

SimpleTerrain is constructed unnecessarily too many times.

Should be globally cached if terrains are to be referred by strings. I am not thrilled by the prospect of changing IIRC over 100 maps.

The terrain entity logic should be separated to not make SimpleTerrain so dynamic and make it possible to add other logic to it. RandomTerrain can also be absorbed into it.

class Terrain {
    static terrainCache;

    apply(pos);
}

class TerrainWithEntity extends Terrain {};

class RandomTerrain extends Terrain {};

Terrain blending logic can be encapsulated in Terrain which is currently unsupported in rmgen.

Potentially treat decals and actors as terrain. By treating them as actors they are agnostic of terrain which is well and good, but there are cases where we need them to not be.

Actually bite the bullet and don't reference terrains by their name. Thank the heavens for sed and awk.

lyv abandoned this revision.Jun 23 2022, 12:44 PM

In favor of D4680