Page MenuHomeWildfire Games

Ignore entities out of the passable map area in GetLandSpawnPoints
Needs ReviewPublic

Authored by smiley on Tue, Dec 4, 8:57 PM.
This revision needs review, but there are no reviewers specified.

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.Tue, Dec 4, 8:57 PM
Owners added a subscriber: Restricted Owners Package.Tue, Dec 4, 8:57 PM
Vulcan added a subscriber: Vulcan.Tue, Dec 4, 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.Tue, Dec 4, 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.Tue, Dec 4, 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.Tue, Dec 4, 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.Tue, Dec 4, 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.Tue, Dec 4, 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.Wed, Dec 5, 8:19 AM
binaries/data/mods/public/maps/scripts/TriggerHelper.js
344

halfSize - edgeSize

elexis added a subscriber: elexis.Fri, Dec 7, 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