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 (indent):
|    | Expected indentation of 0 tabs but found 1 space.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/TriggerHelper.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/TriggerHelper.js
|  61|  61| 	return Engine.QueryInterface(SYSTEM_ENTITY, IID_Timer).GetTime();
|  62|  62| };
|  63|  63| 
|  64|    |- /**
|    |  64|+/**
|  65|  65|   * Returns the elpased ingame time in minutes since the gamestart. Useful for balancing.
|  66|  66|   */
|  67|  67| TriggerHelper.GetMinutes = function()
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/TriggerHelper.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 345| 345| 				continue;
| 346| 346| 		}
| 347| 347| 		else
| 348|    |-			if (pos.x < edgeSize || pos.z < edgeSize || pos.x > size - edgeSize || pos.z > size - edgeSize)
|    | 348|+		if (pos.x < edgeSize || pos.z < edgeSize || pos.x > size - edgeSize || pos.z > size - edgeSize)
| 349| 349| 				continue;
| 350| 350| 
| 351| 351| 		if (cmpTerritoryManager.GetOwner(pos.x, pos.z) == 0)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 4.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/TriggerHelper.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/scripts/TriggerHelper.js
| 346| 346| 		}
| 347| 347| 		else
| 348| 348| 			if (pos.x < edgeSize || pos.z < edgeSize || pos.x > size - edgeSize || pos.z > size - edgeSize)
| 349|    |-				continue;
|    | 349|+			continue;
| 350| 350| 
| 351| 351| 		if (cmpTerritoryManager.GetOwner(pos.x, pos.z) == 0)
| 352| 352| 			neutralSpawnPoints.push(ent);

Link to build: https://jenkins.wildfiregames.com/job/differential/816/

• There is a ticket for removing the instances of the 3 border
• The map should be fixed, the commit should be dug out.
• I recall Petra complains too about stuff outside of the map area.
• I wonder if there shouldn't be a warning or error message about entities placed outside of the map area (in the border or beyond) when parsing the finished mapgeneration. There is a ticket for warnings about gatherable entities that are on a too slopy position to be gatherable). CCmpPosition has the OutsideOfWorld boolean to represent units that are outside of the map area. There might be edge cases where one could argue that entities outside of the map area would be justified (for example there is a skirmish map that has a wonder where 50% of the area is inside the map and 50% in the shroud of darkness). But perhaps these edge cases can all be satisfied with the given restrictions (the center position may not be on a slope and must be inside the map). Otherwise if these edge cases are really relevant, one could add a boolean flag to the map that allows or complains (and possibly removes) entities at invalid positions. Thus adding an automated way of informing map authors when they broke their map. If the engine only loads valid maps, or if the PositionComponent prevents setting such an out-of-world position without setting out-of-world to true, it wouldn't have to add these border checks to every place in the simulation (including here).

I wonder if there shouldn't be a warning or error message about entities placed outside of the map area (in the border or beyond) when parsing the finished mapgeneration.

Relevant for Atlas maps too.

The map should be fixed, the commit should be dug out.

I see that you have raised a concern in the relevant commit. I assume the author would take it from there.

If the engine only loads valid maps, or if the PositionComponent prevents setting such an out-of-world position without setting out-of-world to true

The position component need not worry about this kind of out-of-world IMO. The engine should not add entities out of bounds (MapReader.cpp) and any Jumping or Moving from the simulation side should respect the bounds.

I guess MapReader should do the same check before placing an entity.

In D1688#70268, @smiley wrote:

I guess MapReader should do the same check before placing an entity.

This patch is bad because it tries to deal with the failure of an earlier process without actually fixing the failure. Abandoning in favor of the actually needed fix. (Does not mean I have it. Just that this one is bad.)

Well it's questionable whether or not it's a failure. There is this one scenario map with a wonder that is half inside the map and half outside of the map. Perhaps some super special map needs this as an awkward feature?
It's comparable to the unintentional placement of trees on unreachable cliffs. It could be detected in the pathfinder component (or whatever checks the reachability depending on terrain slope). Ungatherable trees is something that is unwanted for 100% of all maps of all maptypes in 0ad, but might be wanted for some kind of other entity on some kind of map.

About GetLandSpawnPoints, it was proposed to do more of an area check than an entity check, as the relic placement probability correlates with the entity density (so relic always in the middle in Oasis).
The idea was to still pick first such an entity whose accessiblility is assumed (<- apparently wrong assumption?) and then check with thie hierarchical pathfinder which area is reachable and connected to that entity.

The other weirdness is that relics, regicide and nomad function differently on the different maptypes.
For nomad mode it was proposed to place special trigger entities and then replace them with the units in the SkirmishReplacer component.
If this was done for relics (and regicide heroes?) too, it would be consistent.

Scenario maps are, as far as I can observe from the implementation (fixed gamesettings, civs, teams, fixed map) are supposed to be fully predetermined. So it seems like the relic positions and the hero starting position should also be done for scenario maps by having special trigger entities placed in atlas.
rmgen could place these as well too.
So there's the rattail that hide behinds every bug.

Taking all of that into account and assuming that a special map might wants to do the placement of unreachable entities,
your patch may still be correct for the entire lifetime of that function, because the callers to this function still want a passable position (otherwise the objective of capture the relic is not possible to achieve).

It would be good if not another 3 hardcoding would be added. But that could also be noted in the according ticket in case the patch was to be committed.

So I guess both this function and the map should be fixed (because the map author did not intend to place entities in unreachable area), one can do the special trigger entity revision elsewhere, and at last (also elsewhere), pass a bool to the engine (from rmgen JS and scenario/skirmish XML) that if set, warns or deletes such impassable entities (in order for such bugs being cut at the root cause without making it impossible to place unreachable entities)?

(Also, i that function is supposed to return only passable areas, it should also ignore entities that are unpassable on too slopy terrain. I guess that might be checkable with the hierarchical pathfinder.)