Page MenuHomeWildfire Games
Differential D1476

Fix hero ring not changing color
Needs ReviewPublic
Actions

Authored by temple on Apr 28 2018, 11:33 PM.

Details

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

Heroes show a selection ring (star) when not selected, and currently the color doesn't update when toggling diplomacy colors.

The patch fixes that.

Test Plan

Is there a way to select only the heroes (entities with always visible selectable overlay) rather than updating every unit?
In my test took 2-3ms to update the color now rather than 1-2ms.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5992
Build 9989: Vulcan BuildJenkins
Build 9986: arc lint + arc unit

Event Timeline

temple created this revision.Apr 28 2018, 11:33 PM
Vulcan added a subscriber: Vulcan.Apr 28 2018, 11:37 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 (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_Selectable, IID_StatusBars] :
| 793| 793| 			[IID_Minimap, IID_RangeOverlayRenderer, IID_RallyPointRenderer, IID_Selectable],
| 794|    |-			cmpRangeManager.GetEntitiesByPlayer(i));
|    | 794|+		cmpRangeManager.GetEntitiesByPlayer(i));
| 795| 795| 	}
| 796| 796| 	updateEntityColor([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/447/display/redirect

Harbormaster completed remote builds in B5992: Diff 6488.Apr 28 2018, 11:37 PM
elexis added a subscriber: elexis.Apr 29 2018, 12:19 AM

Would need an isAlwaysVisible function in the component interface, or actually that can be derived from the template without asking the component (still asking template manager component however I guess, so might just as well ask Selectable probably).
Implementing that might add a bit of code which may not be worth it from the performance point of view, but it would also have a benefit of informing the reader why unselected selectables are updated, so I wouldn't decline.

elexis added a comment.Apr 29 2018, 12:50 AM

If the UpdateColor method in selectable is only an early return of some sort, then the IsAlwaysVisible check would be truly useless though

elexis added a comment.Apr 29 2018, 10:13 AM

In my test took 2-3ms to update the color now rather than 1-2ms.

How many entities did that test encompass? There can be 10k selectables if we consider larger treespammed maps with gaia attackers and players setting 300 pop.

Especially when we only need to recompute 2-3 heroes rather than 10k selectables it'd seem a bit unreasonable to update all of them (even though thereis bigger fish to fry).
Same is true for the RallyPointRenderer and RangeOverlayRenderer I suppose.

I suspect it's possible to add the early return to UpdateColor of the components that make them at least not regenerate the actual texture data if that isn't shown.
But needs inspection, some certainty.

wraitii added a reviewer: Restricted Owners Package.May 14 2018, 11:54 AM

Revision Contents
Changeset List

PathSize
binaries/
data/
mods/
public/
simulation/
components/
6 lines

Diff 6488

View Options

binaries/data/mods/public/simulation/components/GuiInterface.js

Loading...
Wildfire Games · Phabricator