[PATCH] Schwarzwald tiny map - tile is undefined

[elexis]

As reported by bb today in irc and comment:12:ticket:2787, this error still occurs on schwarzwald with tiny mapsizes:

ERROR: JavaScript error: maps/random/heightmap/heightmap.js line 128
TypeError: tile is undefined
  distributeEntitiesByHeight@maps/random/heightmap/heightmap.js:128:1
  @maps/random/schwarzwald.js:266:78

Replay:

start {"settings":{"PlayerData":[{"Name":"elexis","Civ":"brit","Color":{"r":46,"g":46,"b":200},"AI":"","AIDiff":3,"Team":0},{"Name":"Player 2","Civ":"mace","Color":{"r":150,"g":20,"b":20},"AI":"","AIDiff":3,"Team":0},{"Name":"Player 3","Civ":"athen","Color":{"r":50,"g":165,"b":5},"AI":"","AIDiff":3,"Team":1}],"Seed":28622,"CheatsEnabled":true,"AISeed":64793,"Size":128,"GameType":"conquest","VictoryScripts":["scripts/TriggerHelper.js","scripts/ConquestCommon.js","scripts/Conquest.js"],"WonderDuration":1,"PopulationCap":300,"Ceasefire":0,"StartingResources":50000,"Name":"Schwarzwald","Description":"A forest heavy map with a lake in the middle and plenty of resources.","RatingEnabled":false,"RevealMap":true,"Script":"schwarzwald.js","BaseTerrain":["temp_grass_plants","temp_plants_bog"],"BaseHeight":0,"Preview":"schwarzwald.png","LockTeams":false,"TriggerScripts":["scripts/TriggerHelper.js","scripts/ConquestCommon.js","scripts/Conquest.js"],"CircularMap":true,"mapType":"random"},"map":"maps/random/schwarzwald","mapType":"random","mapPath":"maps/random/","mapFilter":"default","matchID":"134F4A8798F833C3","gameSpeed":1,"script":"schwarzwald.js","timestamp":"1464660017","engine_version":"0.0.21","mods":["mod","public"]}

[FeXoR]

Graciously handles arguments that doesn't contain any valid tile in distributeEntitiesByHeight

[FeXoR]

Something's still very wrong

[FeXoR]

The heightmap just before ExportMap() is 129x129 and should be correct. It seems the map border shadow is not exactly round. I'll still have to check if the arguments the function is called with in Schwarzwald are actually sane...

[FeXoR]

For comparrison Aegean Sea, tiny, 2 Players, Seed 0

[FeXoR]

And an empty map resized to tiny in Atlas

[FeXoR]

That map border shadow issue is likely a problem of my hardware

[FeXoR]

Fixes everything I could find in distributeEntitiesByHeight

[FeXoR]

The function distributeEntitiesByHeight is a lot faster now for later tries (splices points that became invalid due to placement) if the number of tries are high and the number of valid points is low. It is safe to say that at least one other function (getStartLocationsByHeightmap) is not entirely accurate (mixes tile coordinates with points so off by 0.5 tile width/2 horizontal engine space units). I'm not sure if that may produce errors (the error in the ticked didn't come from it but I fixed it anyways) but I'd like to fix those missmatches in this commit too. Any opinions?

[FeXoR]

I'd say it's ready for review since the patch fixes all issues within that function.

[elexis]

Just few style things:

• where != were
• // Maybe better to -> would be nice to figure that out before and either remove the comment or replace it with a // TODO:
• Comments should be on a separate line (recall the 80 character per line recommendation of @wiki:Coding_Conventions)
• Those 2 if statements inside the loop could become inverted ("early continue")

[FeXoR]

@elexis: Which 2 if statements (There are 2 for loops with 2+ if's in them ;p)

[FeXoR]

Fixes as suggersted by elexis but the loop

[Stan]

Why don't you use let checkPointIndex ?

[FeXoR]

@stanislas69: Huh? It's used twice AFAICS?

[FeXoR]

Early continue suggested by elexis and Vladislav

[mimo]

Not a review as I've not checked the functionnalities, but as regards the code:

line 126 to 136 could be replaced by

if (avoidPoints.every(ap => getDistance(x + 0.5, y + 0.5, ap.x, ap.y) > minDistance))
validPoints.push({ "x": x + 0.5, "y": y + 0.5 });

as every returns as soon as one of the elements does not fulfil the condition.

I don't see the full code for lines 153 to 166, but maybe we could also use every or some there too.

Personnally, i would move 140 to 147, and replace 145 by "return []". And the code is clear enough, so the comment line 141 could be removed.

[FeXoR]

Adjusted to mimo's suggestion and added optional argument playerID

[elexis]

Moved to ​https://code.wildfiregames.com/D114

[FeXoR]

In 19288:

Fixes a bug that caused an error when distributeEntitiesByHeight() in hightmap.js was called with arguments that didn't result in any return value.

Differential Revision: ​https://code.wildfiregames.com/D114
Revieved by: elexis
Fixed #4152.

[elexis]

Thanks for the patch, that map is actually interesting to play on tiny mapsizes!