Page MenuHomeWildfire Games

Abort instead of throwing an error when dealing with empty areas.
AbandonedPublic

Authored by FeXoR on May 15 2018, 10:46 AM.

Details

Reviewers
elexis
lyv
Trac Tickets
#5112
Summary

Sometimes mapgen fails to generate areas that would be used later when placing objectGroups or other areas in that specific area. In such a case, an empty array is passed and pickRandom throws an error and aborts the mapGen. This diff adds a check and abort that placement if the given areas are non-existent.

Test Plan

Pass an empty array into any function that takes an area.

Diff Detail

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

Event Timeline

lyv created this revision.May 15 2018, 10:46 AM
Owners added a subscriber: Restricted Owners Package.May 15 2018, 10:46 AM
Vulcan added a subscriber: Vulcan.May 15 2018, 10:49 AM

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
|  32|  32|  * Constants needed for heightmap_manipulation.js
|  33|  33|  */
|  34|  34| const MAX_HEIGHT_RANGE = 0xFFFF / HEIGHT_UNITS_PER_METRE; // Engine limit, Roughly 700 meters
|  35|    |-const MIN_HEIGHT = - SEA_LEVEL;
|    |  35|+const MIN_HEIGHT = -SEA_LEVEL;
|  36|  36| 
|  37|  37| /**
|  38|  38|  * 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
|  68|  68| 	let obstructionSize =
|  69|  69| 		obstruction.Static ?
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|    |-		// Used for gates, should consider the position too
|    |  71|+			// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				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
|  69|  69| 		obstruction.Static ?
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|  71| 		// Used for gates, should consider the position too
|  72|    |-		obstruction.Obstructions ?
|    |  72|+			obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				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
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|  71| 		// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|    |-			new Vector2D(
|    |  73|+				new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			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
|  71|  71| 		// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|    |-				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|    |  74|+					Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			new Vector2D(0, 0);
|  77|  77| 
|    | [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
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|    |-				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    |  75|+					Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			new Vector2D(0, 0);
|  77|  77| 
|  78|  78| 	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
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|    |-			new Vector2D(0, 0);
|    |  76|+				new Vector2D(0, 0);
|  77|  77| 
|  78|  78| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|  79|  79| }
|    | [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
| 255| 255| /**
| 256| 256|  * Create an avoid constraint for the given classes by the given distances
| 257| 257|  */
| 258|    |-function avoidClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 258|+function avoidClasses(/* class1, dist1, class2, dist2, etc*/)
| 259| 259| {
| 260| 260| 	let ar = [];
| 261| 261| 	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
| 271| 271| /**
| 272| 272|  * Create a stay constraint for the given classes by the given distances
| 273| 273|  */
| 274|    |-function stayClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 274|+function stayClasses(/* class1, dist1, class2, dist2, etc*/)
| 275| 275| {
| 276| 276| 	let ar = [];
| 277| 277| 	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
| 287| 287| /**
| 288| 288|  * Create a border constraint for the given classes by the given distances
| 289| 289|  */
| 290|    |-function borderClasses(/*class1, idist1, odist1, class2, idist2, odist2, etc*/)
|    | 290|+function borderClasses(/* class1, idist1, odist1, class2, idist2, odist2, etc*/)
| 291| 291| {
| 292| 292| 	let ar = [];
| 293| 293| 	for (let i = 0; i < arguments.length/3; ++i)

binaries/data/mods/public/maps/random/rmgen/library.js
|  75| »   »   »   »   Object.keys(obstruction.Obstructions).reduce((width,·key)·=>·width·+·+obstruction.Obstructions[key]["@width"],·0))·:
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

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

Stan added a subscriber: Stan.EditedMay 15 2018, 11:01 AM

Why not:

return;

Instead of:

return undefined;
lyv added a comment.EditedMay 15 2018, 11:20 AM
In D1492#61144, @Stan wrote:

Why not:

return;

Instead of:

return undefined;

Most of the rmgen use the latter. So its just a matter of consistency. No strong opinion as it doesnt matter anyway.

https://eslint.org/docs/rules/consistent-return

(Dunno if undefined is the value we want)

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

!length

lyv updated this revision to Diff 6547.May 15 2018, 11:57 AM

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
|  32|  32|  * Constants needed for heightmap_manipulation.js
|  33|  33|  */
|  34|  34| const MAX_HEIGHT_RANGE = 0xFFFF / HEIGHT_UNITS_PER_METRE; // Engine limit, Roughly 700 meters
|  35|    |-const MIN_HEIGHT = - SEA_LEVEL;
|    |  35|+const MIN_HEIGHT = -SEA_LEVEL;
|  36|  36| 
|  37|  37| /**
|  38|  38|  * 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
|  68|  68| 	let obstructionSize =
|  69|  69| 		obstruction.Static ?
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|    |-		// Used for gates, should consider the position too
|    |  71|+			// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				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
|  69|  69| 		obstruction.Static ?
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|  71| 		// Used for gates, should consider the position too
|  72|    |-		obstruction.Obstructions ?
|    |  72|+			obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				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
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|  71| 		// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|    |-			new Vector2D(
|    |  73|+				new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			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
|  71|  71| 		// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|    |-				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|    |  74|+					Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			new Vector2D(0, 0);
|  77|  77| 
|    | [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
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|    |-				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    |  75|+					Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			new Vector2D(0, 0);
|  77|  77| 
|  78|  78| 	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
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|    |-			new Vector2D(0, 0);
|    |  76|+				new Vector2D(0, 0);
|  77|  77| 
|  78|  78| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|  79|  79| }
|    | [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
| 255| 255| /**
| 256| 256|  * Create an avoid constraint for the given classes by the given distances
| 257| 257|  */
| 258|    |-function avoidClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 258|+function avoidClasses(/* class1, dist1, class2, dist2, etc*/)
| 259| 259| {
| 260| 260| 	let ar = [];
| 261| 261| 	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
| 271| 271| /**
| 272| 272|  * Create a stay constraint for the given classes by the given distances
| 273| 273|  */
| 274|    |-function stayClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 274|+function stayClasses(/* class1, dist1, class2, dist2, etc*/)
| 275| 275| {
| 276| 276| 	let ar = [];
| 277| 277| 	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
| 287| 287| /**
| 288| 288|  * Create a border constraint for the given classes by the given distances
| 289| 289|  */
| 290|    |-function borderClasses(/*class1, idist1, odist1, class2, idist2, odist2, etc*/)
|    | 290|+function borderClasses(/* class1, idist1, odist1, class2, idist2, odist2, etc*/)
| 291| 291| {
| 292| 292| 	let ar = [];
| 293| 293| 	for (let i = 0; i < arguments.length/3; ++i)

binaries/data/mods/public/maps/random/rmgen/library.js
|  75| »   »   »   »   Object.keys(obstruction.Obstructions).reduce((width,·key)·=>·width·+·+obstruction.Obstructions[key]["@width"],·0))·:
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

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

FeXoR added a subscriber: FeXoR.Jul 6 2018, 9:34 AM

IMO the way to solve this is:

  • Identify the places that trow an arror when dealing with an empty array
  • Fix them.
vladislavbelov added inline comments.
binaries/data/mods/public/maps/random/rmgen/library.js
157

Why is it undefined and not null? We know here, what we return.

elexis added a comment.Jul 7 2018, 2:13 AM
In D1492#63928, @FeXoR wrote:

IMO the way to solve this is:

  • Identify the places that trow an arror when dealing with an empty array
  • Fix them.

It's not a map script error, the conditions for creating the error are, well, conditionally satisfied.

So it means having to find many places and add an if (areas.length) check to it and then not calling createAreas in this case.
So there is no gain in doing so.

Secondly, it is impossible to find all places in advance because one can't predict if conditions are always satisfied.

For these two reasons I had created the ticket.

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

What we need to distinguish is valid from invalid values, not the different ways to fail, I never had a use case for null. If we want to catch errors specifically, it should be a more specific check for that error, either throw a JS error or only printed error, possibly a return code and insert more words here

vladislavbelov added inline comments.Jul 7 2018, 2:51 AM
binaries/data/mods/public/maps/random/rmgen/library.js
157

null - because we have an empty result, it's not undefined.

00:47 < Vladislav> FeXoR: what's the original problem?

You can click on the ticket.
And there is no single original problem as I mentioned in the last comment, this can occur with every function call where the areas array can be empty and I experienced it with a number of maps already and keep getting it.
Either we have to add this if to most function calls or we add it only once.

(This almost never occured before because this function was almost never called (because of laziness, but this function is more performant than createAreas)

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

undefined is more simple because we don't want to distinguish the return result, don't want to confuse anyone with the fact that there are two possibilities for return values in case the function doesn't return something valid and because for sure we don't want any condition to fail that tests for undefined. If we do want to distinguish it, it should be more explicit than null vs undefined

elexis requested changes to this revision.Jul 7 2018, 11:50 AM

Adding an early return is correct, because the purpose of the function (do something at a random point in a random given area) cannot be performed if the number of areas given is zero. Returning something falsy seems reasonable if nothing could be done.
Returning something falsy is wrong however, since rP21406 it should be an empty array, before it should be 0.
createAreasInAreas should return an empty array.

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

Forgotton return value desription in rP21406, this should be fixed when changing the return value.

This revision now requires changes to proceed.Jul 7 2018, 11:50 AM
FeXoR added a comment.EditedJul 7 2018, 12:56 PM

The comment areas.js is states:

An Area is a set of Vector2D points and a cache to lookup membership quickly.

Shouldn't it be:

  • An Area is an array of Vector2D with integer components representing tile positions as position vectors and a cache to lookup membership quickly.

for Areas.getClosestPointTo uses this.points[0] and e.g. in rmgen-common createPassage() uses arrays?

And createAreasInAreas states:

Returns actually placed areas.

Shouldn't it be:

  • Returns actually placed areas.

And shouldn't it return new Area([]) in case of no area places?

lyv added a comment.Jul 7 2018, 2:46 PM

And shouldn't it return new Area([]) in case of no area places?

I have wondered about that as well. I think it should be what FeXoR mentioned.

elexis added a comment.Jul 7 2018, 2:50 PM

This returns areas (= array), not one area.

lyv updated this revision to Diff 6806.Jul 18 2018, 5:37 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
|  32|  32|  * Constants needed for heightmap_manipulation.js
|  33|  33|  */
|  34|  34| const MAX_HEIGHT_RANGE = 0xFFFF / HEIGHT_UNITS_PER_METRE; // Engine limit, Roughly 700 meters
|  35|    |-const MIN_HEIGHT = - SEA_LEVEL;
|    |  35|+const MIN_HEIGHT = -SEA_LEVEL;
|  36|  36| 
|  37|  37| /**
|  38|  38|  * 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
|  68|  68| 	let obstructionSize =
|  69|  69| 		obstruction.Static ?
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|    |-		// Used for gates, should consider the position too
|    |  71|+			// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				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
|  69|  69| 		obstruction.Static ?
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|  71| 		// Used for gates, should consider the position too
|  72|    |-		obstruction.Obstructions ?
|    |  72|+			obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				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
|  70|  70| 			new Vector2D(obstruction.Static["@depth"], obstruction.Static["@width"]) :
|  71|  71| 		// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|    |-			new Vector2D(
|    |  73|+				new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			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
|  71|  71| 		// Used for gates, should consider the position too
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|    |-				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|    |  74|+					Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			new Vector2D(0, 0);
|  77|  77| 
|    | [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
|  72|  72| 		obstruction.Obstructions ?
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|    |-				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|    |  75|+					Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|  76| 			new Vector2D(0, 0);
|  77|  77| 
|  78|  78| 	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
|  73|  73| 			new Vector2D(
|  74|  74| 				Object.keys(obstruction.Obstructions).reduce((depth, key) => Math.max(depth, +obstruction.Obstructions[key]["@depth"]), 0),
|  75|  75| 				Object.keys(obstruction.Obstructions).reduce((width, key) => width + +obstruction.Obstructions[key]["@width"], 0)) :
|  76|    |-			new Vector2D(0, 0);
|    |  76|+				new Vector2D(0, 0);
|  77|  77| 
|  78|  78| 	return obstructionSize.div(TERRAIN_TILE_SIZE).add(new Vector2D(2, 2).mult(margin));
|  79|  79| }
|    | [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
| 255| 255| /**
| 256| 256|  * Create an avoid constraint for the given classes by the given distances
| 257| 257|  */
| 258|    |-function avoidClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 258|+function avoidClasses(/* class1, dist1, class2, dist2, etc*/)
| 259| 259| {
| 260| 260| 	let ar = [];
| 261| 261| 	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
| 271| 271| /**
| 272| 272|  * Create a stay constraint for the given classes by the given distances
| 273| 273|  */
| 274|    |-function stayClasses(/*class1, dist1, class2, dist2, etc*/)
|    | 274|+function stayClasses(/* class1, dist1, class2, dist2, etc*/)
| 275| 275| {
| 276| 276| 	let ar = [];
| 277| 277| 	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
| 287| 287| /**
| 288| 288|  * Create a border constraint for the given classes by the given distances
| 289| 289|  */
| 290|    |-function borderClasses(/*class1, idist1, odist1, class2, idist2, odist2, etc*/)
|    | 290|+function borderClasses(/* class1, idist1, odist1, class2, idist2, odist2, etc*/)
| 291| 291| {
| 292| 292| 	let ar = [];
| 293| 293| 	for (let i = 0; i < arguments.length/3; ++i)

binaries/data/mods/public/maps/random/rmgen/library.js
|  75| »   »   »   »   Object.keys(obstruction.Obstructions).reduce((width,·key)·=>·width·+·+obstruction.Obstructions[key]["@width"],·0))·:
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

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

FeXoR added a comment.Aug 3 2018, 5:00 PM

@elexis True, so as you said: [] it is ;)

elexis added a reviewer: FeXoR.Jan 7 2019, 2:54 PM

Inclined to accept, given that I had the same patch in some branch buried by time and dust somewhere.
Reading these 2 involved lines, perhaps areas could also contain one area that does not contain any points (a placer with an according failFraction)?

lyv added a comment.Jan 8 2019, 11:52 AM

perhaps areas could also contain one area that does not contain any points (a placer with an according failFraction)?

Techincally (not logically), an area with 0 points is still an area, no?

setCenterPosition(undefined) still unhealthy, ain't?

lyv abandoned this revision.Jun 25 2019, 7:55 PM

Nobody cares about tiny maps anyway.

In D1492#83822, @smiley wrote:

Nobody cares about tiny maps anyway.

Why?

lyv added a comment.EditedJun 25 2019, 9:16 PM
  1. Look at the date on which this was uploaded
  2. Look at what was changed
  3. Try to come up with a reason
vladislavbelov added a comment.EditedJun 25 2019, 9:38 PM
In D1492#83837, @smiley wrote:
  1. Look at the date on which this was uploaded
  2. Look at what was changed
  3. Try to come up with a reason

I thought (as probably some others) that there wasn't your answer to elexis's question.

I have some patches attached to tickets more than few years. It happens because all people here are volunteers and I don't have enough time to finish these patches.

In D1492#83837, @smiley wrote:
  1. Look at the date on which this was uploaded
  2. Look at what was changed
  3. Try to come up with a reason

You are very well aware of the reasons why I have not committed anything since the last release, lack of care for the code is not it.

lyv added a comment.Jun 25 2019, 10:20 PM

Why would I know that? When I first mentioned that none of this stuff are going anywhere, (most of those posts were removed)

I guess someone is confusing Wildfire Games for their personal patchslave.

So, best I can do is guess based on evidence. The way I see it, I am doing wfg a favor.

It's fine to not have time. We all got an afk life. Just don't expect everyone else to have never ending patience I guess.

(Also, this discussion never results in anything good)

In D1492#83849, @smiley wrote:

(most of those posts were removed)

I'm not aware of it, could you post some links to the topics where it happened, if you don't mind.

Silier added a subscriber: Silier.Feb 21 2020, 10:24 PM
In D1492#69823, @elexis wrote:

setCenterPosition(undefined) still unhealthy, ain't?

yes it is. Do you suggest to fix that one as well together with these fixes or it can be left as violation of precondition?

FeXoR commandeered this revision.Jan 9 2021, 6:39 PM
FeXoR edited reviewers, added: lyv; removed: FeXoR.