Structure costs should be whole numbers. Most of them are multiples of ten so when affected by e.g. the Persian catafalque's aura which is 90% of the cost, they're still whole numbers. However, walls aren't multiples of ten so instead a long palisade at 13 wood becomes 11.7 wood, and this is the actual cost that's subtracted as well as the cost shown in the panel tooltip. (The wall preview tooltip shows a floored/rounded cost, I didn't trace to see exactly where that was done.) The patch fixes both of those.
Details
Details
- Reviewers
wraitii - Group Reviewers
Restricted Owners Package (Owns No Changed Paths)
See that I didn't miss anything.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 5776 Build 9688: Vulcan Build Jenkins Build 9687: arc lint + arc unit
Event Timeline
Comment Actions
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 (indent):
| | Expected indentation of 2 tabs but found 3.
|----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 393| 393| function getRepairTimeTooltip(entState)
| 394| 394| {
| 395| 395| return sprintf(translate("%(label)s %(details)s"), {
| 396| |- "label": headerFont(translate("Number of repairers:")),
| | 396|+ "label": headerFont(translate("Number of repairers:")),
| 397| 397| "details": entState.repairable.numBuilders
| 398| 398| }) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| sprintf(translatePlural(
| | [NORMAL] ESLintBear (indent):
| | Expected indentation of 2 tabs but found 3.
|----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 394| 394| {
| 395| 395| return sprintf(translate("%(label)s %(details)s"), {
| 396| 396| "label": headerFont(translate("Number of repairers:")),
| 397| |- "details": entState.repairable.numBuilders
| | 397|+ "details": entState.repairable.numBuilders
| 398| 398| }) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| sprintf(translatePlural(
| 400| 400| "Add another worker to speed up the repairs by %(second)s second.",
| | [NORMAL] ESLintBear (indent):
| | Expected indentation of 1 tab but found 2.
|----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 395| 395| return sprintf(translate("%(label)s %(details)s"), {
| 396| 396| "label": headerFont(translate("Number of repairers:")),
| 397| 397| "details": entState.repairable.numBuilders
| 398| |- }) + "\n" + (entState.repairable.numBuilders ?
| | 398|+ }) + "\n" + (entState.repairable.numBuilders ?
| 399| 399| sprintf(translatePlural(
| 400| 400| "Add another worker to speed up the repairs by %(second)s second.",
| 401| 401| "Add another worker to speed up the repairs by %(second)s seconds.",
| | [NORMAL] ESLintBear (indent):
| | Expected indentation of 2 tabs but found 3.
|----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 415| 415| function getBuildTimeTooltip(entState)
| 416| 416| {
| 417| 417| return sprintf(translate("%(label)s %(details)s"), {
| 418| |- "label": headerFont(translate("Number of builders:")),
| | 418|+ "label": headerFont(translate("Number of builders:")),
| 419| 419| "details": entState.foundation.numBuilders
| 420| 420| }) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| sprintf(translatePlural(
| | [NORMAL] ESLintBear (indent):
| | Expected indentation of 2 tabs but found 3.
|----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 416| 416| {
| 417| 417| return sprintf(translate("%(label)s %(details)s"), {
| 418| 418| "label": headerFont(translate("Number of builders:")),
| 419| |- "details": entState.foundation.numBuilders
| | 419|+ "details": entState.foundation.numBuilders
| 420| 420| }) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| sprintf(translatePlural(
| 422| 422| "Add another worker to speed up the construction by %(second)s second.",
| | [NORMAL] ESLintBear (indent):
| | Expected indentation of 1 tab but found 2.
|----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/common/tooltips.js
| 417| 417| return sprintf(translate("%(label)s %(details)s"), {
| 418| 418| "label": headerFont(translate("Number of builders:")),
| 419| 419| "details": entState.foundation.numBuilders
| 420| |- }) + "\n" + (entState.foundation.numBuilders ?
| | 420|+ }) + "\n" + (entState.foundation.numBuilders ?
| 421| 421| sprintf(translatePlural(
| 422| 422| "Add another worker to speed up the construction by %(second)s second.",
| 423| 423| "Add another worker to speed up the construction by %(second)s seconds.",
| | [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
| 410| 410| ret.attack[type].elevationBonus = range.elevationBonus;
| 411| 411|
| 412| 412| if (cmpUnitAI && cmpPosition && cmpPosition.IsInWorld())
| 413| |- {
| | 413|+
| 414| 414| // For units, take the range in front of it, no spread. So angle = 0
| 415| 415| ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 0);
| 416| |- }
| | 416|+
| 417| 417| else if(cmpPosition && cmpPosition.IsInWorld())
| 418| 418| {
| 419| 419| // 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
| 415| 415| ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 0);
| 416| 416| }
| 417| 417| else if(cmpPosition && cmpPosition.IsInWorld())
| 418| |- {
| | 418|+
| 419| 419| // For buildings, take the average elevation around it. So angle = 2*pi
| 420| 420| ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 2*Math.PI);
| 421| |- }
| | 421|+
| 422| 422| else
| 423| 423| {
| 424| 424| // 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
| 420| 420| ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 2*Math.PI);
| 421| 421| }
| 422| 422| else
| 423| |- {
| | 423|+
| 424| 424| // not in world, set a default?
| 425| 425| ret.attack[type].elevationAdaptedRange = ret.attack.maxRange;
| 426| |- }
| | 426|+
| 427| 427| }
| 428| 428| }
| 429| 429|
| | [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
| 791| 791| updateEntityColor(data.showAllStatusBars && (i == player || player == -1) ?
| 792| 792| [IID_Minimap, IID_RangeOverlayRenderer, IID_RallyPointRenderer, IID_StatusBars] :
| 793| 793| [IID_Minimap, IID_RangeOverlayRenderer, IID_RallyPointRenderer],
| 794| |- cmpRangeManager.GetEntitiesByPlayer(i));
| | 794|+ cmpRangeManager.GetEntitiesByPlayer(i));
| 795| 795| }
| 796| 796| updateEntityColor([IID_Selectable, IID_StatusBars], data.selected);
| 797| 797| 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
|1310|1310| }
|1311|1311| }
|1312|1312| else
|1313| |- {
| |1313|+
|1314|1314| // Didn't snap to an existing entity, add the starting tower manually. To prevent odd-looking rotation jumps
|1315|1315| // when shift-clicking to build a wall, reuse the placement angle that was last seen on a validly positioned
|1316|1316| // wall piece.
|1331|1331| "pos": start.pos,
|1332|1332| "angle": previewEntities.length > 0 ? previewEntities[0].angle : this.placementWallLastAngle
|1333|1333| });
|1334| |- }
| |1334|+
|1335|1335|
|1336|1336| if (end.pos)
|1337|1337| {
| | [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
|1334|1334| }
|1335|1335|
|1336|1336| if (end.pos)
|1337| |- {
| |1337|+
|1338|1338| // Analogous to the starting side case above
|1339|1339| if (end.snappedEnt && end.snappedEnt != INVALID_ENTITY)
|1340|1340| {
|1372|1372| "pos": end.pos,
|1373|1373| "angle": previewEntities.length > 0 ? previewEntities[previewEntities.length-1].angle : this.placementWallLastAngle
|1374|1374| });
|1375| |- }
| |1375|+
|1376|1376|
|1377|1377| let cmpTerrain = Engine.QueryInterface(SYSTEM_ENTITY, IID_Terrain);
|1378|1378| 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
|1550|1550|
|1551|1551| let cmpVisual = Engine.QueryInterface(ent, IID_Visual);
|1552|1552| if (cmpVisual)
|1553| |- {
| |1553|+
|1554|1554| if (!allPiecesValid || !canAfford)
|1555|1555| cmpVisual.SetShadingColor(1.4, 0.4, 0.4, 1);
|1556|1556| else
|1557|1557| cmpVisual.SetShadingColor(1, 1, 1, 1);
|1558| |- }
| |1558|+
|1559|1559|
|1560|1560| ++entPool.numUsed;
|1561|1561| }
| | [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
|1624|1624| {
|1625|1625| minDist2 = dist2;
|1626|1626| minDistEntitySnapData = {
|1627| |- "x": pos.x,
| |1627|+ "x": pos.x,
|1628|1628| "z": pos.z,
|1629|1629| "angle": cmpPosition.GetRotation().y,
|1630|1630| "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
|1625|1625| minDist2 = dist2;
|1626|1626| minDistEntitySnapData = {
|1627|1627| "x": pos.x,
|1628| |- "z": pos.z,
| |1628|+ "z": pos.z,
|1629|1629| "angle": cmpPosition.GetRotation().y,
|1630|1630| "ent": ent
|1631|1631| };
| | [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
|1626|1626| minDistEntitySnapData = {
|1627|1627| "x": pos.x,
|1628|1628| "z": pos.z,
|1629| |- "angle": cmpPosition.GetRotation().y,
| |1629|+ "angle": cmpPosition.GetRotation().y,
|1630|1630| "ent": ent
|1631|1631| };
|1632|1632| }
| | [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
|1627|1627| "x": pos.x,
|1628|1628| "z": pos.z,
|1629|1629| "angle": cmpPosition.GetRotation().y,
|1630| |- "ent": ent
| |1630|+ "ent": ent
|1631|1631| };
|1632|1632| }
|1633|1633| }
| | [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
|1772|1772| result.gain = cmpEntityTrader.GetGoods().amount;
|1773|1773| }
|1774|1774| else if (data.target === secondMarket)
|1775| |- {
| |1775|+
|1776|1776| result = {
|1777|1777| "type": "is second",
|1778|1778| "gain": cmpEntityTrader.GetGoods().amount,
|1779|1779| };
|1780| |- }
| |1780|+
|1781|1781| else if (!firstMarket)
|1782|1782| {
|1783|1783| 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
|1779|1779| };
|1780|1780| }
|1781|1781| else if (!firstMarket)
|1782| |- {
| |1782|+
|1783|1783| result = { "type": "set first" };
|1784| |- }
| |1784|+
|1785|1785| else if (!secondMarket)
|1786|1786| {
|1787|1787| 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
|1783|1783| result = { "type": "set first" };
|1784|1784| }
|1785|1785| else if (!secondMarket)
|1786| |- {
| |1786|+
|1787|1787| result = {
|1788|1788| "type": "set second",
|1789|1789| "gain": cmpEntityTrader.CalculateGain(firstMarket, data.target),
|1790|1790| };
|1791| |- }
| |1791|+
|1792|1792| else
|1793|1793| {
|1794|1794| // 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
|1790|1790| };
|1791|1791| }
|1792|1792| else
|1793| |- {
| |1793|+
|1794|1794| // Else both markets are not null and target is different from them
|1795|1795| result = { "type": "set first" };
|1796| |- }
| |1796|+
|1797|1797| return result;
|1798|1798| };
|1799|1799|Link to build: https://jenkins.wildfiregames.com/job/differential/351/display/redirect
| binaries/data/mods/public/simulation/components/Cost.js | ||
|---|---|---|
| 69 | While i agree for the gui rounding, what is the reason to round in the simu? The current way is well supported while rounding usually lead to troubles in the past, so I don't quite agree with the first line of this ticket "Structure costs should be whole numbers". Futhermore, that change does not only affect structure costs, but all costs. Also a simple 'grep -ir applyvaluemod * | grep -i "cost/resources"' in simulation shows that productionQueue and Commands would have to be changed also. And grepping also subtractResources give lot of things possibly needing to be changed. Also the AI should do the same (see AIInterface for the other roundings) | |
| binaries/data/mods/public/simulation/components/Cost.js | ||
|---|---|---|
| 69 | Yes, but all should be done consistenly. ProductionQueue for example round count*cost and this can lead to hard-to-debug bugs if other places (for example when we test if we can afford it) would round only individual cost and multiply after. All theses should be checked. | |
| binaries/data/mods/public/simulation/components/Cost.js | ||
|---|---|---|
| 69 | Not rounding causes troubles too. For example, killing 15 women should give you 150 experience, so cavalry should be promoted. A jav cav will promote but a spear cav won't because he's at 149.999..97 xp instead. | |
Comment Actions
Ever since I upladed my first patch, the montra has been: "No rounding in sim, only in GUI."
Both rounding and not have there problems as proven above, then the question is: "What is worse?"
I would go with rounding in gui as then the bug at least would be straightly visible (149 xp printed instead of 150), otherwise the bug would be masked in the code and "hard-to-debug". Furthermore not rounding in the sim has the advantage to use fractional resources (like we do for attacks).