Page MenuHomeWildfire Games

Improve performance of GetEntityState by caching ApplyValueModificationsToEntity.
Needs ReviewPublic

Authored by echotangoecho on Aug 19 2018, 4:05 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Improve performance of GetEntityState by caching ApplyValueModificationsToEntity in the Armour, Attack, Loot and ResourceSupply components, as originally suggested by @mimo on D1331.
On Combat Demo (Huge) this gives (on my hardware) a ~30% decrease in time consumption by GetEntityState.

Test Plan

Verify that functionally nothing changed, and test that the game still works well (especially in cases like tech upgrades, while units are selected).

Event Timeline

echotangoecho created this revision.Aug 19 2018, 4:05 PM
Vulcan added a subscriber: Vulcan.Aug 19 2018, 4:10 PM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|  11|  11| Loot.prototype.Init = function()
|  12|  12| {
|  13|  13| 	this.UpdateProperties();
|  14|    |-}
|    |  14|+};
|  15|  15| 
|  16|  16| Loot.prototype.Serialize = null; // we have no dynamic state to save
|  17|  17| 
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Loot.js
|  18|  18| // Cache some properties that are expensive to compute.
|  19|  19| Loot.prototype.UpdateProperties = function()
|  20|  20| {
|  21|    |-	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity))
|    |  21|+	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity));
|  22|  22| 
|  23|  23| 	this.resources = {};
|  24|  24| 	for (let res of Resources.GetCodes())

binaries/data/mods/public/simulation/components/Loot.js
|  14| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Loot.js
|  21| »   this.xp·=·Math.floor(ApplyValueModificationsToEntity("Loot/xp",·+(this.template.xp·||·0),·this.entity))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Armour.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Armour.js
|  80|  80| 	this.armourStrengths = {};
|  81|  81| 	for (let damageType of DamageTypes.GetTypes())
|  82|  82| 		this.armourStrengths[damageType] = applyMods(damageType, foundation);
|  83|    |-}
|    |  83|+};
|  84|  84| 
|  85|  85| Armour.prototype.GetArmourStrengths = function()
|  86|  86| {

binaries/data/mods/public/simulation/components/Armour.js
|  83| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 484| 484| 	}
| 485| 485| 
| 486| 486| 	this.properties = attack;
| 487|    |-}
|    | 487|+};
| 488| 488| 
| 489| 489| Attack.prototype.GetTimers = function(type)
| 490| 490| {
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 518| 518| Attack.prototype.GetProperties = function()
| 519| 519| {
| 520| 520| 	return this.properties;
| 521|    |-}
|    | 521|+};
| 522| 522| 
| 523| 523| Attack.prototype.GetBonusTemplate = function(type)
| 524| 524| {
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 550| 550| 
| 551| 551| 		let horizSpeed = +this.template[type].ProjectileSpeed;
| 552| 552| 		let gravity = +this.template[type].Gravity;
| 553|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 553|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 554| 554| 
| 555| 555| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 556| 556| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 670| 670| 			});
| 671| 671| 	}
| 672| 672| 	else
| 673|    |-	{
|    | 673|+	
| 674| 674| 		// Melee attack - hurt the target immediately
| 675| 675| 		cmpDamage.CauseDamage({
| 676| 676| 			"strengths": this.GetAttackStrengths(type),
| 680| 680| 			"type": type,
| 681| 681| 			"attackerOwner": attackerOwner
| 682| 682| 		});
| 683|    |-	}
|    | 683|+	
| 684| 684| };
| 685| 685| 
| 686| 686| /**

binaries/data/mods/public/simulation/components/Attack.js
| 540| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 487| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Attack.js
| 521| }
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Attack.js
| 645| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|  66|  66| 		let phase = "";
|  67|  67| 		let cmpTechnologyManager = QueryPlayerIDInterface(i, IID_TechnologyManager);
|  68|  68| 		if (cmpTechnologyManager)
|  69|    |-		{
|    |  69|+		
|  70|  70| 			if (cmpTechnologyManager.IsTechnologyResearched("phase_city"))
|  71|  71| 				phase = "city";
|  72|  72| 			else if (cmpTechnologyManager.IsTechnologyResearched("phase_town"))
|  73|  73| 				phase = "town";
|  74|  74| 			else if (cmpTechnologyManager.IsTechnologyResearched("phase_village"))
|  75|  75| 				phase = "village";
|  76|    |-		}
|    |  76|+		
|  77|  77| 
|  78|  78| 		// store player ally/neutral/enemy data as arrays
|  79|  79| 		let allies = [];
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 394| 394| 			let max = ret.attack[type].maxRange;
| 395| 395| 
| 396| 396| 			if (cmpUnitAI && cmpPosition && cmpPosition.IsInWorld())
| 397|    |-			{
|    | 397|+			
| 398| 398| 				// For units, take the range in front of it, no spread. So angle = 0
| 399| 399| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 0);
| 400|    |-			}
|    | 400|+			
| 401| 401| 			else if(cmpPosition && cmpPosition.IsInWorld())
| 402| 402| 			{
| 403| 403| 				// For buildings, take the average elevation around it. So angle = 2*pi
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 399| 399| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 0);
| 400| 400| 			}
| 401| 401| 			else if(cmpPosition && cmpPosition.IsInWorld())
| 402|    |-			{
|    | 402|+			
| 403| 403| 				// For buildings, take the average elevation around it. So angle = 2*pi
| 404| 404| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 2*Math.PI);
| 405|    |-			}
|    | 405|+			
| 406| 406| 			else
| 407| 407| 			{
| 408| 408| 				// not in world, set a default?
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 404| 404| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), max, ret.attack[type].elevationBonus, 2*Math.PI);
| 405| 405| 			}
| 406| 406| 			else
| 407|    |-			{
|    | 407|+			
| 408| 408| 				// not in world, set a default?
| 409| 409| 				ret.attack[type].elevationAdaptedRange = ret.attack[type].maxRange;
| 410|    |-			}
|    | 410|+			
| 411| 411| 		}
| 412| 412| 	}
| 413| 413| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
| 775| 775| 		updateEntityColor(data.showAllStatusBars && (i == player || player == -1) ?
| 776| 776| 			[IID_Minimap, IID_RangeOverlayRenderer, IID_RallyPointRenderer, IID_StatusBars] :
| 777| 777| 			[IID_Minimap, IID_RangeOverlayRenderer, IID_RallyPointRenderer],
| 778|    |-			cmpRangeManager.GetEntitiesByPlayer(i));
|    | 778|+		cmpRangeManager.GetEntitiesByPlayer(i));
| 779| 779| 	}
| 780| 780| 	updateEntityColor([IID_Selectable, IID_StatusBars], data.selected);
| 781| 781| 	Engine.QueryInterface(SYSTEM_ENTITY, IID_TerritoryManager).UpdateColors();
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1294|1294| 		}
|1295|1295| 	}
|1296|1296| 	else
|1297|    |-	{
|    |1297|+	
|1298|1298| 		// Didn't snap to an existing entity, add the starting tower manually. To prevent odd-looking rotation jumps
|1299|1299| 		// when shift-clicking to build a wall, reuse the placement angle that was last seen on a validly positioned
|1300|1300| 		// wall piece.
|1315|1315| 			"pos": start.pos,
|1316|1316| 			"angle": previewEntities.length > 0 ? previewEntities[0].angle : this.placementWallLastAngle
|1317|1317| 		});
|1318|    |-	}
|    |1318|+	
|1319|1319| 
|1320|1320| 	if (end.pos)
|1321|1321| 	{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1318|1318| 	}
|1319|1319| 
|1320|1320| 	if (end.pos)
|1321|    |-	{
|    |1321|+	
|1322|1322| 		// Analogous to the starting side case above
|1323|1323| 		if (end.snappedEnt && end.snappedEnt != INVALID_ENTITY)
|1324|1324| 		{
|1356|1356| 				"pos": end.pos,
|1357|1357| 				"angle": previewEntities.length > 0 ? previewEntities[previewEntities.length-1].angle : this.placementWallLastAngle
|1358|1358| 			});
|1359|    |-	}
|    |1359|+	
|1360|1360| 
|1361|1361| 	let cmpTerrain = Engine.QueryInterface(SYSTEM_ENTITY, IID_Terrain);
|1362|1362| 	if (!cmpTerrain)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1534|1534| 
|1535|1535| 		let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
|1536|1536| 		if (cmpVisual)
|1537|    |-		{
|    |1537|+		
|1538|1538| 			if (!allPiecesValid || !canAfford)
|1539|1539| 				cmpVisual.SetShadingColor(1.4, 0.4, 0.4, 1);
|1540|1540| 			else
|1541|1541| 				cmpVisual.SetShadingColor(1, 1, 1, 1);
|1542|    |-		}
|    |1542|+		
|1543|1543| 
|1544|1544| 		++entPool.numUsed;
|1545|1545| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1608|1608| 			{
|1609|1609| 				minDist2 = dist2;
|1610|1610| 				minDistEntitySnapData = {
|1611|    |-						"x": pos.x,
|    |1611|+					"x": pos.x,
|1612|1612| 						"z": pos.z,
|1613|1613| 						"angle": cmpPosition.GetRotation().y,
|1614|1614| 						"ent": ent
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1609|1609| 				minDist2 = dist2;
|1610|1610| 				minDistEntitySnapData = {
|1611|1611| 						"x": pos.x,
|1612|    |-						"z": pos.z,
|    |1612|+					"z": pos.z,
|1613|1613| 						"angle": cmpPosition.GetRotation().y,
|1614|1614| 						"ent": ent
|1615|1615| 				};
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1610|1610| 				minDistEntitySnapData = {
|1611|1611| 						"x": pos.x,
|1612|1612| 						"z": pos.z,
|1613|    |-						"angle": cmpPosition.GetRotation().y,
|    |1613|+					"angle": cmpPosition.GetRotation().y,
|1614|1614| 						"ent": ent
|1615|1615| 				};
|1616|1616| 			}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 6.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1611|1611| 						"x": pos.x,
|1612|1612| 						"z": pos.z,
|1613|1613| 						"angle": cmpPosition.GetRotation().y,
|1614|    |-						"ent": ent
|    |1614|+					"ent": ent
|1615|1615| 				};
|1616|1616| 			}
|1617|1617| 		}
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1756|1756| 			result.gain = cmpEntityTrader.GetGoods().amount;
|1757|1757| 	}
|1758|1758| 	else if (data.target === secondMarket)
|1759|    |-	{
|    |1759|+	
|1760|1760| 		result = {
|1761|1761| 			"type": "is second",
|1762|1762| 			"gain": cmpEntityTrader.GetGoods().amount,
|1763|1763| 		};
|1764|    |-	}
|    |1764|+	
|1765|1765| 	else if (!firstMarket)
|1766|1766| 	{
|1767|1767| 		result = { "type": "set first" };
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1763|1763| 		};
|1764|1764| 	}
|1765|1765| 	else if (!firstMarket)
|1766|    |-	{
|    |1766|+	
|1767|1767| 		result = { "type": "set first" };
|1768|    |-	}
|    |1768|+	
|1769|1769| 	else if (!secondMarket)
|1770|1770| 	{
|1771|1771| 		result = {
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1767|1767| 		result = { "type": "set first" };
|1768|1768| 	}
|1769|1769| 	else if (!secondMarket)
|1770|    |-	{
|    |1770|+	
|1771|1771| 		result = {
|1772|1772| 			"type": "set second",
|1773|1773| 			"gain": cmpEntityTrader.CalculateGain(firstMarket, data.target),
|1774|1774| 		};
|1775|    |-	}
|    |1775|+	
|1776|1776| 	else
|1777|1777| 	{
|1778|1778| 		// Else both markets are not null and target is different from them
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/GuiInterface.js
|1774|1774| 		};
|1775|1775| 	}
|1776|1776| 	else
|1777|    |-	{
|    |1777|+	
|1778|1778| 		// Else both markets are not null and target is different from them
|1779|1779| 		result = { "type": "set first" };
|1780|    |-	}
|    |1780|+	
|1781|1781| 	return result;
|1782|1782| };
|1783|1783|

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

echotangoecho planned changes to this revision.Aug 19 2018, 4:11 PM

Hmm, so apparently parts don't really work as it is now. Will investigate.

Freagarach updated this revision to Diff 9721.Thu, Sep 12, 9:46 AM
Freagarach retitled this revision from Improve performance of GetEntityState by caching ApplyValueModificationsToEntity to Improve performance of GetEntityState by caching ApplyValueModificationsToEntity..
Freagarach edited the summary of this revision. (Show Details)
Freagarach added a reviewer: Restricted Owners Package.
Freagarach edited subscribers, added: Freagarach; removed: Vulcan, mimo.

Rebased, extended.

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/139/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/Loot.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Loot.js
|  16|  16| // Cache some properties that are expensive to compute.
|  17|  17| Loot.prototype.UpdateProperties = function()
|  18|  18| {
|  19|    |-	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity))
|    |  19|+	this.xp = Math.floor(ApplyValueModificationsToEntity("Loot/xp", +(this.template.xp || 0), this.entity));
|  20|  20| 
|  21|  21| 	this.resources = {};
|  22|  22| 	for (let res of Resources.GetCodes())

binaries/data/mods/public/simulation/components/Loot.js
|  19| »   this.xp·=·Math.floor(ApplyValueModificationsToEntity("Loot/xp",·+(this.template.xp·||·0),·this.entity))
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/648/display/redirect

Stan added a subscriber: Stan.Thu, Sep 12, 10:08 AM

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

Why not both ?

In D1618#94946, @Stan wrote:

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

Why not both ?

Caching values is annoying, because you have to make sure to invalidate the cache at the correct time, and cache it also at the correct time. See the complexity of Attack.prototype.UpdateProperties for example.
Further, with caching you pay the cost of caching whenever you cache, which can actually be more often than you would call the function if you didn't cache at all, so you could be making some bad case better but the general game slower, or improve some cases but worsen others.

In particular, I'm wary of entity creation. It already takes a fair few µs per entity, and components that cache on unit creation are a big part of that.

Anyways, what I'm saying is that caching should be a last resort of sort, and I think other avenues should be explored before, in order:

  1. Optimising existing calls so that this is no longer a problem
  2. Changing algorithms so that this is no longer a problem
  3. ???
  4. Caching
Freagarach added a comment.EditedThu, Sep 12, 4:53 PM

In particular, I'm wary of entity creation. It already takes a fair few µs per entity, and components that cache on unit creation are a big part of that.

I thought about that as well, thats why I first asked on IRC (should have tagged you).

TBH, given our current GUI, I'm wondering if it wouldn't be far more efficient to call this once per template type when we have multiple entities selected, and not care when we have only one entity selected.

I think I have part of a patch for that lying around on Phab. somewhere.

[EDIT}

Created a variable for unique templates in a selection, thus reducing the call time of g_SelectionPanels.PrefAttack.GetItems. This function can probably used for more functions, but that's out of scope.

[EDIT2] D2301