Page MenuHomeWildfire Games

Ignore entities out of the passable map area in GetLandSpawnPoints
AbandonedPublic

Authored by smiley on Dec 4 2018, 8:57 PM.

Details

Reviewers
None
Summary
Test Plan

Can generate the replay available in the thread and see that 3 relics are spawned in the map.

Diff Detail

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

Event Timeline

smiley created this revision.Dec 4 2018, 8:57 PM
Owners added a subscriber: Restricted Owners Package.Dec 4 2018, 8:57 PM
Vulcan added a subscriber: Vulcan.Dec 4 2018, 8:59 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 (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/

smiley added inline comments.Dec 4 2018, 9:03 PM
binaries/data/mods/public/maps/scripts/TriggerHelper.js
340

Hardcoded in CCmpRangeManger. https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/simulation2/components/CCmpRangeManager.cpp$2023

One could argue that function be used here after exposing it. But personally, this code is simple enough that I see no reason of exposing the thing through C++

Stan added a subscriber: Stan.Dec 4 2018, 9:38 PM
Stan added inline comments.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
340

Speed maybe ? Could you do some profiling ?

348

Can't simplify that ? Vector math maybe ?

smiley marked an inline comment as done.Dec 4 2018, 9:47 PM
smiley added inline comments.
binaries/data/mods/public/maps/scripts/TriggerHelper.js
340

Pretty sure it would be irrelevant. Probably a few ms.

348

Without the operator support in JS vectors like its's C++ counterpart, no I don't think it could be cleaner than this.

Stan added inline comments.Dec 4 2018, 10:17 PM
binaries/data/mods/public/maps/scripts/TriggerHelper.js
340

No similar code anywhere else ?

348

We don't have operators ? I thought we add everything ?

smiley added inline comments.Dec 4 2018, 10:23 PM
binaries/data/mods/public/maps/scripts/TriggerHelper.js
340

No. Most use cases covered by CCmpRangeManager AFAIK.

348

Comparison operators.

smiley added inline comments.Dec 5 2018, 8:19 AM
binaries/data/mods/public/maps/scripts/TriggerHelper.js
344

halfSize - edgeSize

elexis added a subscriber: elexis.Dec 7 2018, 1:39 PM
  • 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).
binaries/data/mods/public/maps/scripts/TriggerHelper.js
340

Other than the hardcoding of the border width, the duplication issue must be real, as this logic is needed in many places (this only covers one place, but the problem is systematic.) For example RandomMap.prototype.validTile has the same logic already.
One possibility could be a globalscripts change (which works for rmgen, simulation, AI, GUI), another possibility would be to add a ICmpPosition function (IsValidPosition/IsReachablePosition/IsInShroudOfDarkness or something).

344

11 occurrences of Vector3D, 455 of Vector2D

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.

binaries/data/mods/public/maps/scripts/TriggerHelper.js
340

The problem of adding it to ICmpPosition is that it could just be used by simulation. So I am on board moving it to globalscripts.

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

smiley abandoned this revision.Jan 18 2019, 11:30 AM

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

binaries/data/mods/public/maps/scripts/TriggerHelper.js
306

valid -> passable