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 (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Player.js
|  97|  97| 
|  98|  98| 		// Note: this is not yet implemented but I leave it commented to highlight it's easy
|  99|  99| 		// If anyone ever adds handicap.
| 100|    |-		//if (getSetting(playerData, playerDefaults, i, "GatherRateMultiplier") !== undefined)
|    | 100|+		// if (getSetting(playerData, playerDefaults, i, "GatherRateMultiplier") !== undefined)
| 101| 101| 		//	cmpPlayer.SetGatherRateMultiplier(getSetting(playerData, playerDefaults, i, "GatherRateMultiplier"));
| 102| 102| 
| 103| 103| 		if (getSetting(playerData, playerDefaults, i, "PopulationLimit") !== undefined)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Player.js
| 136| 136| 
| 137| 137| 			// Set all but self as enemies as SetTeam takes care of allies
| 138| 138| 			for (var j = 0; j < numPlayers; ++j)
| 139|    |-			{
|    | 139|+			
| 140| 140| 				if (i == j)
| 141| 141| 					cmpPlayer.SetAlly(j);
| 142| 142| 				else
| 143| 143| 					cmpPlayer.SetEnemy(j);
| 144|    |-			}
|    | 144|+			
| 145| 145| 			cmpPlayer.SetTeam(myTeam === undefined ? -1 : myTeam);
| 146| 146| 		}
| 147| 147| 
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/helpers/Player.js
| 303| 303| 	return IsOwnedByHelper(player, target, "IsMutualAlly");
| 304| 304| }
| 305| 305| 
| 306|    |-function IsOwnedByNeutralOfPlayer(player,target)
|    | 306|+function IsOwnedByNeutralOfPlayer(player, target)
| 307| 307| {
| 308| 308| 	return IsOwnedByHelper(player, target, "IsNeutral");
| 309| 309| }

binaries/data/mods/public/simulation/helpers/Player.js
|  78| »   »   let·cmpPlayer·=·QueryPlayerIDInterface(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'cmpPlayer' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Player.js
| 160| »   »   for·(let·i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/simulation/helpers/Player.js
|  67| »   »   var·entID·=·cmpPlayerManager.GetPlayerByID(i);
|    | [NORMAL] JSHintBear:
|    | 'entID' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
|  76| »   for·(var·i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/helpers/Player.js
| 138| »   »   »   for·(var·j·=·0;·j·<·numPlayers;·++j)
|    | [NORMAL] JSHintBear:
|    | 'j' is already defined.

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

(rP16473)

What's causing more frequent playerdrops in alpha 23 (D1513):

18:43 < temple> guess is that after rP21785 auras get removed whereas previously because of the bug they stayed applied
18:43 < temple> so removing the auras is causing the extra slowdown
18:44 < temple> but even in a22 with seven players on a team, just setting up the auras takes seconds
18:44 < temple> takes lots more seconds in a23
18:45 < temple> it's still the ai interface
18:46 < temple> if that's not necessary then there's no seconds
18:46 < temple> presumably it's getting called more often because auras are being removed from the templates now

A23.1 + A23.2 compatibility:

21:59 < elexis> temple: does D1559 trigger an OOS without rejoin if one applies it and hte other doesnt?
22:33 < temple> checked both ways, host/rejoiner with/without patch and no oos

The patch is correct if and only if the AIInterface disabling does not change the code flow for the statements between the new and old call.

Tested, no OOS, rejoin or not. We can probably get away with relying on the hash match indicator, but we could also get away with verification.

There are MT_DiplomacyChanged messages sent for the SetTeam call, resulting in many template modification messages. Who of us knows what the heck the AI interface is doing with that and which components or global simulation states could be influenced?
Notice that the AInterface serializes data too, for instance this.changedEntities. SkirmishReplace is called from`InitGame` prior to this function, so that looks fine, dunno.

Perhaps we can disable it even much sooner (InitGame.js?). Perhaps after a23 we can avoid creating the component in C++ altogether if it's disabled.
There already are quite a number of cmpAIManager calls looping over the player array.

It's used to pass data to the AI, so no AIs -> useless. Order shouldn't matter here unless there's also an OOS bug in the AI Interface.

One extra thing that should be tested on that patch is whether a saved game with AIs can be re-loaded without issues.

Replays too ?

/**
 * Disable all registering functions for this component
 * Gets called in case no AI players are present to save resources
 */
AIInterface.prototype.Disable = function()
{
	this.enabled = false;
	let nop = function(){};
	this.ChangedEntity = nop;
	this.PushEvent = nop;
	this.OnGlobalPlayerDefeated = nop;
	this.OnGlobalEntityRenamed = nop;
	this.OnGlobalTributeExchanged = nop;
	this.OnTemplateModification = nop;
	this.OnGlobalValueModification = nop;
};

This was from rP16473, however there's been new functions since then that haven't been added to the list, for example OnDiplomacyChanged and OnTerritoriesChanged. The associated data (events.DiplomacyChanged and events.TerritoriesChanged) gets changed and is serialized. So at the moment we can't add the new functions to the list (otherwise OOS), but that should be done after the re-release. Although this seems pretty hacky, so instead we should find a more general way to disable components.

The OnTemplateModification function alters this.changedTemplateInfo which is serialized, however GetFullRepresentation "Intended to be called first, during the map initialization: no caching" then resets that data. OnGlobalValueModification is called too, but this.changedEntityTemplateInfo is also reset. I've tested old replays and the hashes match, so I think everything is okay. Saved games seem fine.

More general way to disable sounds like an early return if IID_AIInterface in CComponentManager::ConstructComponent / AddComponent.

Saved games seem fine.

I guess saving with the old version and loading with the new one is in question.

So we don't consider the move to InitGame.js, because preventing component construction in the ComponentManager were even better, had a similar effort and we already thoroughly tested this iteration?

But. If removing the diplomacy AIInterface interaction improves the performance by 90% then we could miss out on a 90% performance improvement on auras on diplomacy change in running games too when changing diplomacy.

grep "\.On" binaries/data/mods/public/simulation/components/AIInterface.js

=> These missing

OnCeasefireEnded
OnDiplomacyChanged
OnTerritoriesChanged

Territories change often, but the data collection seems cheap.

(Likely ideal if the AI had read and read-only access to the simulation state. If there was some locking process, it could be thread-safe and nothing would have to be copied.)
(This way mods don't have to extend the AIInterface but can extend the AIs directly)

In D1559#63082, @elexis wrote:

So we don't consider the move to InitGame.js, because preventing component construction in the ComponentManager were even better, had a similar effort and we already thoroughly tested this iteration?

This one's been tested, is my excuse.

(This problem of course still exists if there is a multiplayergame with one or more AIs, #3700 is what makes the network latency independent from loadingscreen CPU/algorithm latency.)