Page MenuHomeWildfire Games
Differential D1449

Danubius attacker should ignore targets on the other side of the river
ClosedPublic
Actions

Authored by lyv on Apr 13 2018, 12:11 PM.

Details

Reviewers
elexis
Commits
rP21767: Fix gaia attackers on the left riverside of Danubius trying to attack units on…
Trac Tickets
#4622
Summary

The gaia attackers look for the closest organic unit to attack. Sometimes, that unit happens to be on the other side. And the gaia congregates on the bank not attacking anything.

Test Plan

Gather some units on the bank in one side, and move all units on the other bank to the very edge of the map. Without the patch, the attackers spawned on the side with units moved away would target the units on the opposite bank. With the patch, the attackers would patrol their side instead of gathering on the bank.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lyv created this revision.Apr 13 2018, 12:11 PM
Owners added a subscriber: Restricted Owners Package.Apr 13 2018, 12:11 PM
Vulcan added a subscriber: Vulcan.Apr 13 2018, 12:16 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 (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  14|  14| 
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|    |-	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|    |  17|+	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege", "gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|    |-	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|    |  18|+	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen", "gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|    |-	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|    |  19|+	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer", "gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|  22|  22| 	"citizen_soldiers": TriggerHelper.GetTemplateNamesByClasses("CitizenSoldier", "gaul", undefined, "Basic", true),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 290| 290| 
| 291| 291| 		let animations = ritualAnimations[
| 292| 292| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 293|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 293|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 294| 294| 
| 295| 295| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 296| 296| 		if (!cmpVisual)

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

Harbormaster completed remote builds in B5850: Diff 6382.Apr 13 2018, 12:16 PM
elexis updated the Trac tickets for this revision.Apr 13 2018, 2:08 PM
lyv updated this revision to Diff 6383.Apr 13 2018, 3:40 PM
Vulcan added a comment.Apr 13 2018, 3:42 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 (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  14|  14| 
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|    |-	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|    |  17|+	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege", "gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|    |-	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|    |  18|+	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen", "gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|    |-	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|    |  19|+	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer", "gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|  22|  22| 	"citizen_soldiers": TriggerHelper.GetTemplateNamesByClasses("CitizenSoldier", "gaul", undefined, "Basic", true),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 290| 290| 
| 291| 291| 		let animations = ritualAnimations[
| 292| 292| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 293|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 293|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 294| 294| 
| 295| 295| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 296| 296| 		if (!cmpVisual)

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

Harbormaster completed remote builds in B5851: Diff 6383.Apr 13 2018, 3:42 PM
elexis added a comment.Apr 13 2018, 3:50 PM

Bug:
The filtering would have to be done prior to the slice, otherwise you might get 0 attack targets.

Performance:
You might already be familiar with "Spawn lag" on every gaia attacker map.
This would likely worsen it due to the vector computation twice per attacker per attack target.
In comparison, JebelBarkal_SpawnAttackerGroups only computes all jebelBarkal_pathTargetClasses = "CivCentre Wonder" once per call.

So maybe we can reconsider this and possibly unify the gaia control.

#4622 also mentions that chosing 3 targets is not always good for soldiers, since these targets may have retreated already (luring) and became unreachable.
JB only picks one target then random locations and makes units patrol there.

targetCount = 3 seems reasonable for ships, not bad but not needed for siege engines and counterproductive for soldiers.
So maybe setting it to 1 is ok. That makes crossing the sea easier - or we use three different variables.
If it were 1 for all times and all difficulties, then we only need to find the closest target instead of sorting all targets (which does 2n² vector operations instead of O(n)).

Dunno, mabe the performance isn'T such a big problem in this case, but I'm dubious. We have had like 3-5 different significant spawn performance improvements on polar sea.

lyv added a comment.Apr 15 2018, 3:22 PM

I'm afraid that using this with the current method of gaia targeting would lead to a non-negligible performance cost. And considering how laggy a game can get after a while (30 mins or so) even without this patch, I think changing how the gaia targets is also needed along with this.

elexis requested changes to this revision.Apr 20 2018, 1:20 PM

The lag relevant to this patch is only the spawn-lag. Would be great to have a replay profile graph to see the lagspikes.
I'd likely still take this if the filter is done prior to the sorting and slicing.

binaries/data/mods/public/maps/random/danubius_triggers.js
390 ↗(On Diff #6383)

This comment is wrong because the function is also used for the units unloaded.

This revision now requires changes to proceed.Apr 20 2018, 1:20 PM
lyv planned changes to this revision.Apr 20 2018, 1:21 PM
lyv updated this revision to Diff 6431.Apr 20 2018, 2:30 PM

Fixed the biggest issue with the patch.

Vulcan added a comment.Apr 20 2018, 2:35 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 (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  14|  14| 
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|    |-	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|    |  17|+	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege", "gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|    |-	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|    |  18|+	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen", "gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|    |-	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|    |  19|+	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer", "gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|  22|  22| 	"citizen_soldiers": TriggerHelper.GetTemplateNamesByClasses("CitizenSoldier", "gaul", undefined, "Basic", true),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 290| 290| 
| 291| 291| 		let animations = ritualAnimations[
| 292| 292| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 293|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 293|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 294| 294| 
| 295| 295| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 296| 296| 		if (!cmpVisual)

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

Harbormaster completed remote builds in B5912: Diff 6431.Apr 20 2018, 2:35 PM
elexis requested changes to this revision.Apr 20 2018, 2:53 PM

Still performs unneeded sorting. The patch might become a performance improvement rather than a worsening this way.

This revision now requires changes to proceed.Apr 20 2018, 2:53 PM
lyv updated this revision to Diff 6437.Apr 20 2018, 4:22 PM
lyv marked an inline comment as done.
Vulcan added a comment.Apr 20 2018, 4:24 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 (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  14|  14| 
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|    |-	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|    |  17|+	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege", "gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|    |-	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|    |  18|+	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen", "gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|    |-	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|    |  19|+	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer", "gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|  22|  22| 	"citizen_soldiers": TriggerHelper.GetTemplateNamesByClasses("CitizenSoldier", "gaul", undefined, "Basic", true),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 290| 290| 
| 291| 291| 		let animations = ritualAnimations[
| 292| 292| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 293|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 293|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 294| 294| 
| 295| 295| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 296| 296| 		if (!cmpVisual)

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

Harbormaster completed remote builds in B5915: Diff 6437.Apr 20 2018, 4:24 PM
elexis added a comment.Apr 20 2018, 4:47 PM

You asked in the lobby about the purpose of 3 attacker targets.
The use case is only to optimize the effectiveness of the unit.
If there is only one unit focused, then it will take random patrol routes and possibly miss targets for quite a while (contributing to lag and removing difficulty unintentionally).

For soldiers it seems quite unlikely that the closest 3 targets (soldiers) are spread out by far.
For ships it is quite likely that the 3 closest targets (ships) are spread out far. Most often there are only 0-3 clumps of player ships on the water. But once there is one clump of 3 or more ships, the targetCount = 3 becomes useless again.
For siege engines it is not unlikely but probably not sufficiently likely with regards to performance cost).

For soldiers, targetCount = 3 also thas the issue that the target can become blocked by surrounding soldiers, then the gaia units are slaughtered without attacking anything.
This isn't the case for ships being blocked by ships, because the ships (currently) still fire at surrounding ships while trying to chase.

So it seems we have two reasons to remove it and not a sufficient reason to keep it.

So up for nuking sort and only finding the closest unit using a fancy reduce call?

binaries/data/mods/public/maps/random/danubius_triggers.js
415 ↗(On Diff #6437)

We don't want to compute this.IsLeftRiverside(attackers[0]) about 100 times but only once, right?

390 ↗(On Diff #6383)

Not units as it's also used for structures: targets

lyv updated this revision to Diff 6450.Apr 22 2018, 1:31 PM
Vulcan added a comment.Apr 22 2018, 1:35 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 (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  14|  14| 
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|    |-	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|    |  17|+	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege", "gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  15|  15| const danubiusAttackerTemplates = deepfreeze({
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|    |-	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|    |  18|+	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen", "gaul", undefined, undefined, true),
|  19|  19| 	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|  16|  16| 	"ships": TriggerHelper.GetTemplateNamesByClasses("Warship", "gaul", undefined, undefined, true),
|  17|  17| 	"siege": TriggerHelper.GetTemplateNamesByClasses("Siege","gaul", undefined, undefined, true),
|  18|  18| 	"females": TriggerHelper.GetTemplateNamesByClasses("FemaleCitizen","gaul", undefined, undefined, true),
|  19|    |-	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer","gaul", undefined, undefined, true),
|    |  19|+	"healers": TriggerHelper.GetTemplateNamesByClasses("Healer", "gaul", undefined, undefined, true),
|  20|  20| 	"champions": TriggerHelper.GetTemplateNamesByClasses("Champion", "gaul", undefined, undefined, true),
|  21|  21| 	"champion_infantry": TriggerHelper.GetTemplateNamesByClasses("Champion+Infantry", "gaul", undefined, undefined, true),
|  22|  22| 	"citizen_soldiers": TriggerHelper.GetTemplateNamesByClasses("CitizenSoldier", "gaul", undefined, "Basic", true),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 285| 285| 
| 286| 286| 		let animations = ritualAnimations[
| 287| 287| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 288|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 288|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 289| 289| 
| 290| 290| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 291| 291| 		if (!cmpVisual)
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 410| 410| 	let targets = TriggerHelper.MatchEntitiesByClass(TriggerHelper.GetAllPlayersEntities(), targetClass);
| 411| 411| 	let minDistance = Infinity;
| 412| 412| 	let closestTarget;
| 413|    |-	
|    | 413|+
| 414| 414| 	for (let target of targets)
| 415| 415| 	{
| 416| 416| 		if (this.IsLeftRiverside(target) != isLeft)
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 415| 415| 	{
| 416| 416| 		if (this.IsLeftRiverside(target) != isLeft)
| 417| 417| 			continue;
| 418|    |-			
|    | 418|+
| 419| 419| 		let targetDistance = DistanceBetweenEntities(attackers[0], target);
| 420| 420| 			
| 421| 421| 		if (targetDistance < minDistance)
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 417| 417| 			continue;
| 418| 418| 			
| 419| 419| 		let targetDistance = DistanceBetweenEntities(attackers[0], target);
| 420|    |-			
|    | 420|+
| 421| 421| 		if (targetDistance < minDistance)
| 422| 422| 		{
| 423| 423| 			closestTarget = target;
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 421| 421| 		if (targetDistance < minDistance)
| 422| 422| 		{
| 423| 423| 			closestTarget = target;
| 424|    |-			minDistance = targetDistance
|    | 424|+			minDistance = targetDistance;
| 425| 425| 		};
| 426| 426| 	};
| 427| 427| 
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 422| 422| 		{
| 423| 423| 			closestTarget = target;
| 424| 424| 			minDistance = targetDistance
| 425|    |-		};
|    | 425|+		}
| 426| 426| 	};
| 427| 427| 
| 428| 428| 	this.debugLog(debugName + " " + uneval(attackers) + " attack " + uneval(closestTarget));
|    | [NORMAL] ESLintBear (no-extra-semi):
|    | Unnecessary semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/danubius_triggers.js
| 423| 423| 			closestTarget = target;
| 424| 424| 			minDistance = targetDistance
| 425| 425| 		};
| 426|    |-	};
|    | 426|+	}
| 427| 427| 
| 428| 428| 	this.debugLog(debugName + " " + uneval(attackers) + " attack " + uneval(closestTarget));
| 429| 429| 

binaries/data/mods/public/maps/random/danubius_triggers.js
| 424| »   »   »   minDistance·=·targetDistance
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/maps/random/danubius_triggers.js
| 425| »   »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

binaries/data/mods/public/maps/random/danubius_triggers.js
| 426| »   };
|    | [NORMAL] JSHintBear:
|    | Unnecessary semicolon.

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

Harbormaster completed remote builds in B5931: Diff 6450.Apr 22 2018, 1:35 PM
elexis mentioned this in rP21766: Add some missing IsInWorld checks to Danubius for cases where all targets are….Apr 23 2018, 1:23 PM
elexis accepted this revision.Apr 23 2018, 1:25 PM

The bug is easy to reproduce:
Set shipUngarrisonInterval to 1, start with 2 players, one of them garrisons all units, then the gaia soldiers on the other side of the river cling to the shoreline.

While testing I got

ERROR: Error in timer on entity 1, IID93, function DoAction: TypeError: v1 is undefined
  Vector2D.sub@globalscripts/vector.js:199:2
  Trigger.prototype.IsLeftRiverside@maps/random/danubius_triggers.js:484:35
  Trigger.prototype.AttackAndPatrol@maps/random/danubius_triggers.js:416:7
  Trigger.prototype.CheckShipRange@maps/random/danubius_triggers.js:573:3
  Trigger.prototype.DoAction@simulation/components/Trigger.js:331:3
  Timer.prototype.OnUpdate@simulation/components/Timer.js:120:4

This is because MatchEntitiesByClass includes garrisoned units, which don't have a Position2D.
This is handled by DistanceBetweenEntities but not in the new code.

This bug should occur in other places too, GetActiveRiversides for instance.
Since that checks for structures and structures can't be garrisoned in the public mod, we didn't encounter it yet (equally broken in a22).
Good that we found that, fixed in rP21766.
Likely needs things in the other triggerscript maps too (yay maintining all of these).

Thanks for the patch, smiley!

binaries/data/mods/public/maps/random/danubius_triggers.js
387 ↗(On Diff #6450)

Looks like this function should be moved to TriggerHelper and become used by these two nub maps.

420 ↗(On Diff #6450)

no whitespace in empty lines

424 ↗(On Diff #6450)

missing semicolon, hopefully reported by the bot above

426 ↗(On Diff #6450)

no semicolon here and in the line above

This revision is now accepted and ready to land.Apr 23 2018, 1:25 PM
Closed by commit rP21767: Fix gaia attackers on the left riverside of Danubius trying to attack units on… (authored by elexis). · Explain WhyApr 23 2018, 1:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
maps/
random/
45 lines

Diff 6459

View Options

ps/trunk/binaries/data/mods/public/maps/random/danubius_triggers.js

Loading...
Wildfire Games · Phabricator