Page MenuHomeWildfire Games

Fix Danubius ownershipchange subscription following rP21445
AcceptedPublic

Authored by elexis on Dec 8 2018, 3:43 PM.

Details

Reviewers
bb
Summary

In rP21445, the Danubius triggerscript variables are initialized OnInitGame rather than each time the file is loaded.
This seems like the intended way to startup scripts and it's good to do it consistently,
but this bugged, because the OnOwnershipChanged subscription is called prior to OnInitGame if for intance the DisableTreasure setting is enabled.
Bug found by mord and nani.

Test Plan

It seems other than getting a red error message, there is no harm to the player / running match.

Diff Detail

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

Event Timeline

elexis created this revision.Dec 8 2018, 3:43 PM
Vulcan added a subscriber: Vulcan.Dec 8 2018, 3:44 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
| 288| 288| 
| 289| 289| 		let animations = ritualAnimations[
| 290| 290| 			cmpIdentity.HasClass("Healer") ? "healer" :
| 291|    |-			cmpIdentity.HasClass("Female") ? "female" : "male"];
|    | 291|+				cmpIdentity.HasClass("Female") ? "female" : "male"];
| 292| 292| 
| 293| 293| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
| 294| 294| 		if (!cmpVisual)

binaries/data/mods/public/maps/random/danubius_triggers.js
| 638| {
|    | [NORMAL] ESLintBear (no-lone-blocks):
|    | Block is redundant.

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

smiley added a subscriber: smiley.EditedDec 19 2018, 9:50 AM

I guess it is worth mentioning that the same goes for skirmish maps. The message MT_SkirmishReplace and the subsequent messages would be sent before MT_InitGame.

bb added a subscriber: bb.Dec 26 2018, 5:58 PM
bb added inline comments.
binaries/data/mods/public/maps/random/danubius_triggers.js
589

another way of fixing it is checking some existences here

613

I did vote for a comment explaining why this is here (explain this bug exists otherwise)

elexis marked 2 inline comments as done.Dec 27 2018, 11:24 AM
elexis added inline comments.
binaries/data/mods/public/maps/random/danubius_triggers.js
589

In general it's more expected to use the logical order: 1. initialize variable 2. read from variable, rather than reading if initialized, then calling initialize. The advantage is that the calls to the functions then reveal when the init takes placeand when the function can be read from (especially compared to getters that initialize if not initialized for instance).
So the alternative to this diff seems rather to init the Sets when the file is loaded, and then subscribe the Ownership change function after that (still when the file is loaded as well).
Seems cleaner to initialize the simulation functions when the simulation is initialized, not when the file is loaded, yes?

613

That information can be inferred from the lines exactly above the call and the lines inside the function, which are exactly the lines that the reader should know about.

bb accepted this revision.Jan 4 2019, 4:00 PM

Accepting because it is correct and complete for Danubius, but while looking at D1694, I guess the same bug can happen in (most of) the victory conditions, and potentially the other trigger scripts too (SotF, JB etc.)

This revision is now accepted and ready to land.Jan 4 2019, 4:00 PM