Page MenuHomeWildfire Games

Fix Danubius ownershipchange subscription following rP21445
ClosedPublic

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

Details

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.

WARNING: JavaScript warning: maps/random/danubius_triggers.js line 589
reference to undefined property this.heroes
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Error calling component script function ApplyModifications
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: Script message handler OnGlobalOwnershipChanged failed
ERROR: JavaScript error: maps/random/danubius_triggers.js line 589
TypeError: this.heroes is undefined
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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/

lyv added a subscriber: lyv.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 ↗(On Diff #7034)

another way of fixing it is checking some existences here

613 ↗(On Diff #7034)

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 ↗(On Diff #7034)

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 ↗(On Diff #7034)

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
elexis edited the summary of this revision. (Show Details)Jan 26 2020, 5:26 PM
Owners added a subscriber: Restricted Owners Package.Jan 26 2020, 5:34 PM