Details
- Reviewers
- None
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
Verify that functionally nothing changed, and test that the game still works well (especially in cases like tech upgrades, while units are selected).
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 9328 Build 15440: Vulcan Build Jenkins Build 15439: Vulcan Build (Windows) Jenkins Build 15438: arc lint + arc unit
Event Timeline
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
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
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:
- Optimising existing calls so that this is no longer a problem
- Changing algorithms so that this is no longer a problem
- ???
- Caching
I thought about that as well, thats why I first asked on IRC (should have tagged you).
I think I have part of a patch for that lying around on Phab. somewhere.
[EDIT}
[EDIT2] D2301