Page MenuHomeWildfire Games

Diplomacy Colors
ClosedPublic

Authored by temple on Jul 25 2017, 3:23 AM.

Details

Reviewers
elexis
Commits
rP21107: Diplomacy Colors
Trac Tickets
#4747
Summary

This patch adds a button to toggle between normal and "diplomacy" colors.

  • When playing, players are colored according to your stance towards them: green are allies, yellow are neutral, red are enemies, and blue is you.
  • When observing, players are colored according to their team.

There are lots of things that need color, hopefully I've found them all.

There would be an OOS error if diplomacyColor in Player.js was serialized, so I removed that.

I made the button from minimap-idle.png, icons/minimap/zoom.png, and icons/diplomacy.png. It's fine if someone wants to make a better one.


Test Plan

Test out the button (in the bottom left corner of the minimap).
See that everything has been properly recolored and I haven't overlooked anything.
(Units on the minimap only update every so often, so you'll notice those colors taking a fraction of a second to update.)
Play multiplayer or otherwise test that there aren't out-of-sync errors.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Jan 27 2018, 1:46 AM
binaries/data/mods/public/gui/session/selection_details.js
154

unused (hence misleading) space I believe

binaries/data/mods/public/gui/session/session.js
19

IMO good practice to have one very short JS doc comment for each global, if one can add information (i.e. something non-obvious)

292

Future feature:
Not sure if they shoudn't even be in default.cfg instead. (Would just have to sanitize it so it wont crash terribly but use.... maybe diplomacy_colors.json as fallback colors).

293

I believe this call is unneeded. All components start with the player color after init / deserialization, otherwise it should be considered a component bug IMO.

535

(This is what I mean with complexity that had made the gamesetup terrible and could be simplified to one updateGUIObjects that updates the entire GUI state from the given state variables. (input-processing-output model).)

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

Reading this function, one might still think it's cleaner to pass the diplomacy color as an argument here.

In case the diplomacyColor display is disabled, the GUIInterface would have to either pass the playerColor to the simulation components or the simulation components conditionally get their color from two different components.
Both seem worse choices than what is proposed here (making the conditionally playercolor-using components pull their color from the player component).

Not sure if avoiding the hardcoded simulation component names by sending an MT_NonSyncPlayerColor simulation message would be worth the invitation to add a non-synced code. Also no clue about the performance overhead of the messages.

820

We should replace that -1 with a constant some day.

825

The entire block could be minimized by mandating the updated functions to have an UpdateDisplayedPlayerColor function and calling that in a loop over the component interface IDs.

binaries/data/mods/public/simulation/components/Player.js
35

I suppose after this patch, panelEntities can be removed too from serialization and the panelEntityClasses hardcoding could be deleted, passed from the GUI.

36

I'm not convinced that one gets away with not introducing a Deserialize function.

This one proves that the diplomacy color key are undefined after the deserialization, rather than having the value from init (for example false for displayDiplomacyColor):

Player.prototype.OnGlobalDeserialized = function(msg) { warn(uneval(Object.keys(this))) };

The CStdDeserializer::ReadScriptVal defines the default deserialize, which just seems to copy over all properties, but not sure if that is extending or replacing the previous object without looking further.

Looking at the TechnologyManager (which doesn't have a deserialize function either), it sets the Init default for the one 'non-serialized'
property. Should do the same here.

source/graphics/TerritoryTexture.cpp
181

No more ->GetColor( in the entire C++ code base except FCollada, so patch seems complete.
Only 2 JS calls remaining.
The one new in the playermanager in rP20951 is only relevant for atlas and can use the displayedplayercolor, as that color should persist after having changed the civ. I couldn't get atlas to bug, but I think we might have to copy over the diplo properties too.

source/simulation2/components/CCmpRallyPointRenderer.cpp
360–361

If we want to time things, might want to compare the performance of the hack to the performance of the rebuild.

source/simulation2/components/ICmpSelectable.h
34

The comment might be pointless, but it was grammatically correct, no? But the s doesn't hurt either.

temple updated this revision to Diff 5522.Jan 27 2018, 8:52 PM
temple marked 3 inline comments as done.

combine gui interface functions; player serialization

source/simulation2/components/CCmpRallyPointRenderer.cpp
360–361

ConstructAllOverlayLines seems to be much slower, for example time = 0.000110266 compared to this code at time = 1.641e-06 or so (for a cc with ten rally points).

source/simulation2/components/ICmpSelectable.h
34

Doesn't really matter, was just making the tense consistent since I was here.

temple added inline comments.Jan 27 2018, 8:58 PM
binaries/data/mods/public/gui/session/messages.js
335

Probably only needed if diplo colors are toggled (here and below).

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 0 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/StatusBars.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/StatusBars.js
|  26|  26| 	"CaptureBar",
|  27|  27| 	"HealthBar",
|  28|  28| 	"AuraIcons",
|  29|    |-	];
|    |  29|+];
|  30|  30| 
|  31|  31| StatusBars.prototype.Init = function()
|  32|  32| {
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
| 414| 414| 	let otherEnts = [];
| 415| 415| 
| 416| 416| 	for (let ent of garrisonHolders)
| 417|    |-	{
|    | 417|+	
| 418| 418| 		if (controlsPlayer(GetEntityState(ent).player))
| 419| 419| 			ownEnts.push(ent);
| 420| 420| 		else
| 421| 421| 			otherEnts.push(ent);
| 422|    |-	}
|    | 422|+	
| 423| 423| 
| 424| 424| 	if (ownEnts.length)
| 425| 425| 		Engine.PostNetworkCommand({
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'goods'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|  89|  89| 		this.resourceCount[res] = 300;
|  90|  90| 		this.resourceNames[res] = Resources.GetResource(res).name;
|  91|  91| 		this.tradingGoods.push({
|  92|    |-			"goods":  res,
|    |  92|+			"goods": res,
|  93|  93| 			"proba": 5 * (quotient + (+i < remainder ? 1 : 0))
|  94|  94| 		});
|  95|  95| 	}

binaries/data/mods/public/simulation/components/Player.js
| 363| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 390| 390| 				// Players see colors depending on diplomacy
| 391| 391| 				g_DisplayedPlayerColors[i] =
| 392| 392| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 393|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
|    | 393|+						g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 394| 394| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 395| 395| 					g_DiplomacyColorPalette.Enemy;
| 396| 396| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 391| 391| 				g_DisplayedPlayerColors[i] =
| 392| 392| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 393| 393| 					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 394|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
|    | 394|+							g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 395| 395| 					g_DiplomacyColorPalette.Enemy;
| 396| 396| 
| 397| 397| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 392| 392| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 393| 393| 					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 394| 394| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 395|    |-					g_DiplomacyColorPalette.Enemy;
|    | 395|+								g_DiplomacyColorPalette.Enemy;
| 396| 396| 
| 397| 397| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 398| 398| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1687|1687| 	for (let rct of resourcesCounterTypes)
|1688|1688| 		for (let rt of resourcesTypes)
|1689|1689| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1690|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1690|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1691|1691| 
|1692|1692| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1693|1693| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1050| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1125| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1126| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1127| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/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 (key-spacing):
|    | Extra space after key 'upgrades'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 286| 286| 	var cmpUpgrade = Engine.QueryInterface(ent, IID_Upgrade);
| 287| 287| 	if (cmpUpgrade)
| 288| 288| 		ret.upgrade = {
| 289|    |-			"upgrades" : cmpUpgrade.GetUpgrades(),
|    | 289|+			"upgrades": cmpUpgrade.GetUpgrades(),
| 290| 290| 			"progress": cmpUpgrade.GetProgress(),
| 291| 291| 			"template": cmpUpgrade.GetUpgradingTo()
| 292| 292| 		};
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'isIdle'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 360| 360| 			"isGuarding": cmpUnitAI.IsGuardOf(),
| 361| 361| 			"canPatrol": cmpUnitAI.CanPatrol(),
| 362| 362| 			"possibleStances": cmpUnitAI.GetPossibleStances(),
| 363|    |-			"isIdle":cmpUnitAI.IsIdle(),
|    | 363|+			"isIdle": cmpUnitAI.IsIdle(),
| 364| 364| 		};
| 365| 365| 
| 366| 366| 	let cmpGuard = Engine.QueryInterface(ent, IID_Guard);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 424| 424| 			ret.attack[type].elevationBonus = range.elevationBonus;
| 425| 425| 
| 426| 426| 			if (cmpUnitAI && cmpPosition && cmpPosition.IsInWorld())
| 427|    |-			{
|    | 427|+			
| 428| 428| 				// For units, take the range in front of it, no spread. So angle = 0
| 429| 429| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 0);
| 430|    |-			}
|    | 430|+			
| 431| 431| 			else if(cmpPosition && cmpPosition.IsInWorld())
| 432| 432| 			{
| 433| 433| 				// For buildings, take the average elevation around it. So angle = 2*pi
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 429| 429| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 0);
| 430| 430| 			}
| 431| 431| 			else if(cmpPosition && cmpPosition.IsInWorld())
| 432|    |-			{
|    | 432|+			
| 433| 433| 				// For buildings, take the average elevation around it. So angle = 2*pi
| 434| 434| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 2*Math.PI);
| 435|    |-			}
|    | 435|+			
| 436| 436| 			else
| 437| 437| 			{
| 438| 438| 				// not in world, set a default?
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 434| 434| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 2*Math.PI);
| 435| 435| 			}
| 436| 436| 			else
| 437|    |-			{
|    | 437|+			
| 438| 438| 				// not in world, set a default?
| 439| 439| 				ret.attack[type].elevationAdaptedRange = ret.attack.maxRange;
| 440|    |-			}
|    | 440|+			
| 441| 441| 		}
| 442| 442| 	}
| 443| 443| 
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'r'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 842| 842| 		let color = playerColors[owner];
| 843| 843| 		if (!color)
| 844| 844| 		{
| 845|    |-			color = { "r":1, "g":1, "b":1 };
|    | 845|+			color = { "r": 1, "g":1, "b":1 };
| 846| 846| 			let cmpPlayer = QueryPlayerIDInterface(owner);
| 847| 847| 			if (cmpPlayer)
| 848| 848| 				color = cmpPlayer.GetDisplayedColor();
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'g'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 842| 842| 		let color = playerColors[owner];
| 843| 843| 		if (!color)
| 844| 844| 		{
| 845|    |-			color = { "r":1, "g":1, "b":1 };
|    | 845|+			color = { "r":1, "g": 1, "b":1 };
| 846| 846| 			let cmpPlayer = QueryPlayerIDInterface(owner);
| 847| 847| 			if (cmpPlayer)
| 848| 848| 				color = cmpPlayer.GetDisplayedColor();
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'b'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 842| 842| 		let color = playerColors[owner];
| 843| 843| 		if (!color)
| 844| 844| 		{
| 845|    |-			color = { "r":1, "g":1, "b":1 };
|    | 845|+			color = { "r":1, "g":1, "b": 1 };
| 846| 846| 			let cmpPlayer = QueryPlayerIDInterface(owner);
| 847| 847| 			if (cmpPlayer)
| 848| 848| 				color = cmpPlayer.GetDisplayedColor();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 4 spaces.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 921| 921| 
| 922| 922| GuiInterface.prototype.GetNonGaiaEntities = function()
| 923| 923| {
| 924|    |-    return Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager).GetNonGaiaEntities();
|    | 924|+	return Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager).GetNonGaiaEntities();
| 925| 925| };
| 926| 926| 
| 927| 927| /**
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|1336|1336| 		}
|1337|1337| 	}
|1338|1338| 	else
|1339|    |-	{
|    |1339|+	
|1340|1340| 		// Didn't snap to an existing entity, add the starting tower manually. To prevent odd-looking rotation jumps
|1341|1341| 		// when shift-clicking to build a wall, reuse the placement angle that was last seen on a validly positioned
|1342|1342| 		// wall piece.
|1357|1357| 			"pos": start.pos,
|1358|1358| 			"angle": previewEntities.length > 0 ? previewEntities[0].angle : this.placementWallLastAngle
|1359|1359| 		});
|1360|    |-	}
|    |1360|+	
|1361|1361| 
|1362|1362| 	if (end.pos)
|1363|1363| 	{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/publ
elexis added inline comments.Jan 30 2018, 4:40 PM
binaries/data/mods/public/gui/session/session.js
17

(Distribute the three sentences to the three variables?)

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

INVALID_PLAYER is the observer playerID here? I suspect we want an own constant for that, (I've left it at -1 in some observer places iirc since not sure)

The condition is independent of the entityID, so it can be computed once outside of the loop. And if we do that already, we might just as well construct the IID array which is also independent of the entityID outside of the loop too. And then we can add an UpdateColor funciton to cmpStatusBars that calls RegenerateSprites (and possibly more in the future) to unify that part under the IID loop too.

820

RegenerateSprites is run twice for the same selected entity owned by a player if showAllStatusBars is true.
Wondering if a owner == gaia condition would be worth the performance.
On the other side, currently the GUI decided to prohibit gaia + other entity selection in observermode, but this can and might probably likely should change in some way eventually.

binaries/data/mods/public/simulation/components/Player.js
37

right

source/simulation2/components/CCmpRallyPointRenderer.cpp
360–361

That is 10^-4 seconds compared to 10^-6 seconds?
I guess what matters is the 'worst average' case. That would be a player with 50 entities that have 50 rallypoints?
So that'd be 0.25seconds?
Seems better to not have the bug then.
Should have better numbers I suppose if we want to find the better choice.

source/simulation2/components/CCmpTerritoryManager.cpp
161

Here we see the evidence that no proposed change causes an OOS in the TerritoryManager component.

temple added inline comments.Feb 1 2018, 3:03 AM
binaries/data/mods/public/simulation/components/GuiInterface.js
820

We can select at most 200 units, so there's a cap on how bad this part can be (~20ms, maybe).
If observers have showAllStatusBars then that can affect a lot of units, but it doesn't seem too bad? Doing 2000 units in the editor, it's a total 100ms with showAllStatusBars and 20ms without (on my old computer).

source/simulation2/components/CCmpRallyPointRenderer.cpp
360–361

50 houses with 50 rally points each is ~20ms, which I guess is acceptable, so okay.

temple updated this revision to Diff 5615.Feb 1 2018, 3:03 AM
Vulcan added a comment.Feb 1 2018, 4:15 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Feb 1 2018, 4:17 AM
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...

binaries/data/mods/public/gui/session/menu.js
| 469| »   »   button.onPress·=·(function(player,·stance)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'stance' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'resCode' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 544| »   button.onPress·=·(function(i)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 610| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 610| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'goods'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|  89|  89| 		this.resourceCount[res] = 300;
|  90|  90| 		this.resourceNames[res] = Resources.GetResource(res).name;
|  91|  91| 		this.tradingGoods.push({
|  92|    |-			"goods":  res,
|    |  92|+			"goods": res,
|  93|  93| 			"proba": 5 * (quotient + (+i < remainder ? 1 : 0))
|  94|  94| 		});
|  95|  95| 	}

binaries/data/mods/public/simulation/components/Player.js
| 363| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 397| 397| 				// Players see colors depending on diplomacy
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 400|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
|    | 400|+						g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 402| 402| 					g_DiplomacyColorPalette.Enemy;
| 403| 403| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/dat
elexis added inline comments.Feb 1 2018, 4:41 AM
binaries/data/mods/public/simulation/components/GuiInterface.js
814

Looks good! packed, no redundancy. :-)
The second IID loop could be the outer loop, then the array could be inlined.
Perhaps we could create a helper function in this function:
{{{
let updateEntityColors = (iids, entityIDs) => {

for iiids...
    for entityIDs...
         cmpUpdate

};
}}}

820

That 200 might become 300 eventually, not set in stone, but realistically capped yes.
Yeah not bad, just noticed it. leper said "doing pointless work is just that" in such a situation :-)
But adding the code to prevent the redundancy might make things more complex than what is neat and useful.

source/simulation2/components/CCmpRallyPointRenderer.cpp
360–361

great!

elexis added inline comments.Feb 1 2018, 4:53 AM
binaries/data/mods/public/simulation/components/GuiInterface.js
814

(If it becomes a function, the IIDs can remain the inner loop too)

temple updated this revision to Diff 5623.Feb 1 2018, 5:34 PM

Add a helper function.
For status bars, we only need to update the ones that use player color, i.e. that have the capture bar. So let's keep track of that and avoid unnecessary RegenerateSprites calls.

Vulcan added a comment.Feb 1 2018, 6:38 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Feb 1 2018, 6:39 PM
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
| 414| 414| 	let otherEnts = [];
| 415| 415| 
| 416| 416| 	for (let ent of garrisonHolders)
| 417|    |-	{
|    | 417|+	
| 418| 418| 		if (controlsPlayer(GetEntityState(ent).player))
| 419| 419| 			ownEnts.push(ent);
| 420| 420| 		else
| 421| 421| 			otherEnts.push(ent);
| 422|    |-	}
|    | 422|+	
| 423| 423| 
| 424| 424| 	if (ownEnts.length)
| 425| 425| 		Engine.PostNetworkCommand({

binaries/data/mods/public/gui/session/menu.js
| 469| »   »   button.onPress·=·(function(player,·stance)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'stance' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'resCode' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 544| »   button.onPress·=·(function(i)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 610| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 610| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 469| 469| 		g_Selection.reset();
| 470| 470| 		g_Selection.addList(selection, false, cmd.type == "gather");
| 471| 471| 	},
| 472|    |-	"play-tracks": function (notification, player)
|    | 472|+	"play-tracks": function(notification, player)
| 473| 473| 	{
| 474| 474| 		if (notification.lock)
| 475| 475| 		{
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 576| 576| 	let notificationText =
| 577| 577| 		notification.instructions.reduce((instructions, item) =>
| 578| 578| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 579|    |-			"");
|    | 579|+		"");
| 580| 580| 
| 581| 581| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 582| 582| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|1084|1084| 
|1085|1085| 	let message = "";
|1086|1086| 	if (notifyPhase == "all")
|1087|    |-	{
|    |1087|+	
|1088|1088| 		if (msg.phaseState == "started")
|1089|1089| 			message = translate("%(player)s is advancing to the %(phaseName)s.");
|1090|1090| 		else if (msg.phaseState == "aborted")
|1091|1091| 			message = translate("The %(phaseName)s of %(player)s has been aborted.");
|1092|    |-	}
|    |1092|+	
|1093|1093| 	if (msg.phaseState == "completed")
|1094|1094| 		message = translate("%(player)s has reached the %(phaseName)s.");
|1095|1095| 

binaries/data/mods/public/gui/session/messages.js
| 635| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 397| 397| 				// Players see colors depending on diplomacy
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 400|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
|    | 400|+						g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 402| 402| 					g_DiplomacyColorPalette.Enemy;
| 403| 403| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 398| 398| 				g_DisplayedPlayerColors[i] =
| 399| 399| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 401|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
|    | 401|+							g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 402| 402| 					g_DiplomacyColorPalette.Enemy;
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 399| 399| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 400| 400| 					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 401| 401| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 402|    |-					g_DiplomacyColorPalette.Enemy;
|    | 402|+								g_DiplomacyColorPalette.Enemy;
| 403| 403| 
| 404| 404| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 405| 405| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1229|1229| 
|1230|1230| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1231|1231| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1232|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1232|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1233|1233| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1234|1234| 	});
|1235|1235| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1230|1230| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1231|1231| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1232|1232| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1233|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1233|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1234|1234| 	});
|1235|1235| 
|1236|1236| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1231|1231| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1232|1232| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1233|1233| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1234|    |-	});
|    |1234|+		});
|1235|1235| 
|1236|1236| 	let resCodes = g_ResourceData.GetCodes();
|1237|1237| 	for (let r = 0; r < resCodes.length; ++r)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1717|1717| 	for (let rct of resourcesCounterTypes)
|1718|1718| 		for (let rt of resourcesTypes)
|1719|1719| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1720|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1720|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1721|1721| 
|1722|1722| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1723|1723| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1057| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1132| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1133| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1134| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i)
temple updated this revision to Diff 5627.Feb 1 2018, 7:24 PM

add a comment

Vulcan added a comment.Feb 1 2018, 8:17 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Feb 1 2018, 8:19 PM
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
| 414| 414| 	let otherEnts = [];
| 415| 415| 
| 416| 416| 	for (let ent of garrisonHolders)
| 417|    |-	{
|    | 417|+	
| 418| 418| 		if (controlsPlayer(GetEntityState(ent).player))
| 419| 419| 			ownEnts.push(ent);
| 420| 420| 		else
| 421| 421| 			otherEnts.push(ent);
| 422|    |-	}
|    | 422|+	
| 423| 423| 
| 424| 424| 	if (ownEnts.length)
| 425| 425| 		Engine.PostNetworkCommand({
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 469| 469| 		g_Selection.reset();
| 470| 470| 		g_Selection.addList(selection, false, cmd.type == "gather");
| 471| 471| 	},
| 472|    |-	"play-tracks": function (notification, player)
|    | 472|+	"play-tracks": function(notification, player)
| 473| 473| 	{
| 474| 474| 		if (notification.lock)
| 475| 475| 		{
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 576| 576| 	let notificationText =
| 577| 577| 		notification.instructions.reduce((instructions, item) =>
| 578| 578| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 579|    |-			"");
|    | 579|+		"");
| 580| 580| 
| 581| 581| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 582| 582| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|1084|1084| 
|1085|1085| 	let message = "";
|1086|1086| 	if (notifyPhase == "all")
|1087|    |-	{
|    |1087|+	
|1088|1088| 		if (msg.phaseState == "started")
|1089|1089| 			message = translate("%(player)s is advancing to the %(phaseName)s.");
|1090|1090| 		else if (msg.phaseState == "aborted")
|1091|1091| 			message = translate("The %(phaseName)s of %(player)s has been aborted.");
|1092|    |-	}
|    |1092|+	
|1093|1093| 	if (msg.phaseState == "completed")
|1094|1094| 		message = translate("%(player)s has reached the %(phaseName)s.");
|1095|1095| 

binaries/data/mods/public/gui/session/messages.js
| 635| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' cond
elexis accepted this revision.Feb 1 2018, 9:54 PM

After noticing the obligatory OOS oversight, you found the cause (Player.Serialize typo this vs. state), I was unable to trigger any OOS on rejoin with both blackbox and whitebox testing.
Finally audited the CCmpTerritoryManager code, that's correct too.
Thus for the first time I managed to read the patch from top to bottom, know what every line is about, without seeing any improvement or reduction while knowing that every line is correct.
Even if there were another oversight, this patch is a piece of jewelry and it was a both exciting and educating journey for all of us :-)
I hereby claim ability and secondary responsibility to fix oversights.
Thanks for your performance temple!

binaries/data/mods/public/gui/session/session.js
16

(since it's convention to have operators at the end of the previous line rather than at the start of the next line, I was doing the same with code comments and their conjunction and. After all we put the comma there for the same reason)

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

Maybe that becomes more readable (some irrelevant nanoseconds more performant) ìf we don't pass an IID array but call updateEntityColors for one IID. The entities wouldn't be updated one by one anymore. Anyway, very good code as is.

binaries/data/mods/public/simulation/components/Player.js
35

s/this/state

binaries/data/mods/public/simulation/data/settings/diplomacy_colors.json
7

I'm still a bit dubious about these colors. IMO there should be no color that appears in both player colors and diplo color sets. Otherwise you have more than one guy in the MP chat speaking of the red player that forgot that he had enabled diplo colors and that other players see a different red one.

It should become visible to the player if the mode is toggled or not. For instance by indicating that in the GUI button. Can be done separately, but a should-have.

Even then, using different colors seems preferable to me.

source/simulation2/components/CCmpTerritoryManager.cpp
263

Examining the correctness of this line, the only deduction from assuming this to be wrong line of code I could find is the assumption that it's unneded. Reading the surroudning code we notice MakeDirty which might already satisfy the components need to update the territory texture color. No need to estimate if it's a critical performance improvement, because MakeDirty sends a simulation message and that's something we have to avoid. So all good.

290

Does this function need to check for the player color too?
Their purpose is not commented in the header file, a small defect in the curent code.
A rename to distinguish them better (NeedUpdateTexture NeedUpdateSomethingInThesimulation) might satisfy too.

634

Offtopic, but a bit awkward to push an empty struct and then change the property of the back of the vector, rather than initializing a struct fully and pushing it when it's done.
I assume we want an STL initialization for this, the code comes from back in the day when rP9929 was committed.

682

(could use range-based loop)

702

same

813

(UpdateColor would be consistent with the other components, but the other components are entity components while this one is a system component, so maybe its good, whatever ? )

824

maybe continue

830

Function correct, RenderSubmit pulls the overlays each frame, so we don't need to do any further calls to apply the new color as far as I see.

This revision is now accepted and ready to land.Feb 1 2018, 9:54 PM
temple added inline comments.Feb 2 2018, 7:46 PM
binaries/data/mods/public/simulation/data/settings/diplomacy_colors.json
7

There's only so many words for colors and we already use eight of them, plus the diplomacy colors won't match unless they're viewing the same player, so it's not a big concern to me. Do you have suggestions for four colors that aren't similar to any player color (and that also don't conflict with resources)?

It sounds good to indicate the toggle status in the button. Hopefully an artist will want to improve the placeholder I've made.

source/graphics/TerritoryTexture.cpp
181

(I think the PlayerManager one is correct as is.)

temple updated this revision to Diff 5638.Feb 2 2018, 7:47 PM

Use a different button when diplomacy colors are on or off.

elexis accepted this revision.Feb 2 2018, 8:03 PM

Placeholder looks good.

binaries/data/mods/public/gui/session/session.js
378

What did we say about splitting filenames when we did the minimap panel button commit?

binaries/data/mods/public/simulation/data/settings/diplomacy_colors.json
7

Color spectre is limited and nearly fully used yet already, so not much we can do with mere colors.
So only using a different concept could counteract. ffm (ticket somewhere) proposed that lines between allies become dashed. But that would solve the problem for territory rendering only.

temple added inline comments.Feb 2 2018, 8:45 PM
binaries/data/mods/public/gui/session/session.js
378

I forgot since we ended up doing something different.

var g_IdleWorkerIcons = {
	"default": "stretched:session/minimap-idle.png",
	"highlight": "stretched:session/minimap-idle-highlight.png",
	"disabled": "stretched:session/minimap-idle-disabled.png"
}

Okay, I'll make a global variable to store them.

elexis added inline comments.Feb 2 2018, 8:56 PM
binaries/data/mods/public/gui/session/session.js
378

The important part is that we can do string searches for filenames, so those shouldn't be split.
If the stretched effect is supposed to be the same in all cases, it can be left out.

Vulcan added a comment.Feb 2 2018, 9:29 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Feb 2 2018, 9:31 PM
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
| 414| 414| 	let otherEnts = [];
| 415| 415| 
| 416| 416| 	for (let ent of garrisonHolders)
| 417|    |-	{
|    | 417|+	
| 418| 418| 		if (controlsPlayer(GetEntityState(ent).player))
| 419| 419| 			ownEnts.push(ent);
| 420| 420| 		else
| 421| 421| 			otherEnts.push(ent);
| 422|    |-	}
|    | 422|+	
| 423| 423| 
| 424| 424| 	if (ownEnts.length)
| 425| 425| 		Engine.PostNetworkCommand({
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'goods'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|  89|  89| 		this.resourceCount[res] = 300;
|  90|  90| 		this.resourceNames[res] = Resources.GetResource(res).name;
|  91|  91| 		this.tradingGoods.push({
|  92|    |-			"goods":  res,
|    |  92|+			"goods": res,
|  93|  93| 			"proba": 5 * (quotient + (+i < remainder ? 1 : 0))
|  94|  94| 		});
|  95|  95| 	}

binaries/data/mods/public/simulation/components/Player.js
| 363| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 469| 469| 		g_Selection.reset();
| 470| 470| 		g_Selection.addList(selection, false, cmd.type == "gather");
| 471| 471| 	},
| 472|    |-	"play-tracks": function (notification, player)
|    | 472|+	"play-tracks": function(notification, player)
| 473| 473| 	{
| 474| 474| 		if (notification.lock)
| 475| 475| 		{
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
| 576| 576| 	let notificationText =
| 577| 577| 		notification.instructions.reduce((instructions, item) =>
| 578| 578| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 579|    |-			"");
|    | 579|+		"");
| 580| 580| 
| 581| 581| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 582| 582| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/messages.js
|1084|1084| 
|1085|1085| 	let message = "";
|1086|1086| 	if (notifyPhase == "all")
|1087|    |-	{
|    |1087|+	
|1088|1088| 		if (msg.phaseState == "started")
|1089|1089| 			message = translate("%(player)s is advancing to the %(phaseName)s.");
|1090|1090| 		else if (msg.phaseState == "aborted")
|1091|1091| 			message = translate("The %(phaseName)s of %(player)s has been aborted.");
|1092|    |-	}
|    |1092|+	
|1093|1093| 	if (msg.phaseState == "completed")
|1094|1094| 		message = translate("%(player)s has reached the %(phaseName)s.");
|1095|1095| 

binaries/data/mods/public/gui/session/messages.js
| 635| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/session/menu.js
| 469| »   »   button.onPress·=·(function(player,·stance)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'stance' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'resCode' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 501| »   »   button.onPress·=·(function(i,·resCode,·button)·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 544| »   button.onPress·=·(function(i)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 610| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/menu.js
| 610| »   button.onPress·=·(function(i,·button)·{·return·function()·{
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'button' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 407| 407| 				// Players see colors depending on diplomacy
| 408| 408| 				g_DisplayedPlayerColors[i] =
| 409| 409| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 410|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
|    | 410|+						g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 411| 411| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 412| 412| 					g_DiplomacyColorPalette.Enemy;
| 413| 413| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 408| 408| 				g_DisplayedPlayerColors[i] =
| 409| 409| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 410| 410| 					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 411|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
|    | 411|+							g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 412| 412| 					g_DiplomacyColorPalette.Enemy;
| 413| 413| 
| 414| 414| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 409| 409| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 410| 410| 					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 411| 411| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 412|    |-					g_DiplomacyColorPalette.Enemy;
|    | 412|+								g_DiplomacyColorPalette.Enemy;
| 413| 413| 
| 414| 414| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
| 415| 415| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1239|1239| 
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|    |-		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|    |1242|+			"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1240|1240| 	let orderHotkeyTooltip = Object.keys(viewablePlayerStates).length <= 1 ? "" :
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|    |-		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|    |1243|+			"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|1244| 	});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1241|1241| 		"\n" + sprintf(translate("%(order)s: %(hotkey)s to change order."), {
|1242|1242| 		"hotkey": setStringTags("\\[Click]", g_HotkeyTags),
|1243|1243| 		"order": tooltipSort == 0 ? translate("Unordered") : tooltipSort == 1 ? translate("Descending") : translate("Ascending")
|1244|    |-	});
|    |1244|+		});
|1245|1245| 
|1246|1246| 	let resCodes = g_ResourceData.GetCodes();
|1247|1247| 	for (let r = 0; r < resCodes.length; ++r)
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|1727|1727| 	for (let rct of resourcesCounterTypes)
|1728|1728| 		for (let rt of resourcesTypes)
|1729|1729| 			reportObject[rt + rct.substr(9)] = playerStatistics[rct][rt];
|1730|    |-			// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|    |1730|+	// eg. rt = food rct.substr = Gathered rct = resourcesGathered
|1731|1731| 
|1732|1732| 	reportObject.vegetarianFoodGathered = playerStatistics.resourcesGathered.vegetarianFood;
|1733|1733| 	for (let type of unitsClasses)

binaries/data/mods/public/gui/session/session.js
|1067| »   let·getPanelEntNameTooltip·=·panelEntState·=>·"[font=\"sans-bold-16\"]"·+·template.name.specific·+·"[/font]";
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'panelEntState' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1142| »   »   button.onpress·=·(function(i)·{·return·function()·{·performGroup((Engine.HotkeyIsPressed("selection.add")·?·"add"·:·"select"),·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1143| »   »   button.ondoublepress·=·(function(i)·{·return·function()·{·performGroup("snap",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
|1144| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/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 (key-spacing):
|    | Extra space after key 'upgrades'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 286| 286| 	var cmpUpgrade = Engine.QueryInterface(ent, IID_Upgrade);
| 287| 287| 	if (cmpUpgrade)
| 288| 288| 		ret.upgrade = {
| 289|    |-			"upgrades" : cmpUpgrade.GetUpgrades(),
|    | 289|+			"upgrades": cmpUpgrade.GetUpgrades(),
| 290| 290| 			"progress": cmpUpgrade.GetProgress(),
| 291| 291| 			"template": cmpUpgrade.GetUpgradingTo()
| 292| 292| 		};
|    | [NORMAL] ESLintBear (key-spacing):
|    | Missing space before value for key 'isIdle'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 352| 352| 			"isGuarding": cmpUnitAI.IsGuardOf(),
| 353| 353| 			"canPatrol": cmpUnitAI.CanPatrol(),
| 354| 354| 			"possibleStances": cmpUnitAI.GetPossibleStances(),
| 355|    |-			"isIdle":cmpUnitAI.IsIdle(),
|    | 355|+			"isIdle": cmpUnitAI.IsIdle(),
| 356| 356| 		};
| 357| 357| 
| 358| 358| 	let cmpGuard = Engine.QueryInterface(ent, IID_Guard);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/GuiInterface.js
| 416| 416| 			ret.attack[type].elevationBonus = range.elevationBonus;
| 417| 417| 
| 418| 418| 			if (cmpUnitAI && cmpPosition && cmpPosition.IsInWorld())
| 419|    |-			{
|    | 419|+			
| 420| 420| 				// For units, take the range in front of it, no spread. So angle = 0
| 421| 421| 				ret.attack[type].elevationAdaptedRange = cmpRangeManager.GetElevationAdaptedRange(cmp
temple marked 18 inline comments as done.Feb 2 2018, 11:18 PM

Thanks for all the guidance, elexis. It's been a fun journey.

binaries/data/mods/public/gui/session/session.js
378

Oh, right.

source/simulation2/components/CCmpTerritoryManager.cpp
290

Rename sounds good. (Added in D910.)

813

Yeah, this one deals with more than one color so I went for the plural.

temple updated this revision to Diff 5639.Feb 2 2018, 11:19 PM
temple marked an inline comment as done.

Last few changes.
Fix icon names.
Differentiate NeedUpdate functions.

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (309 tests).....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: ''.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: 'MESSAGES_SKIP_STRUCTS'.

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp
|  33| #include·"graphics/Terrain.h"
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character ({) when these macros are defined: '_MSC_VER'.
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/selection_panels_helpers.js
| 414| 414| 	let otherEnts = [];
| 415| 415| 
| 416| 416| 	for (let ent of garrisonHolders)
| 417|    |-	{
|    | 417|+	
| 418| 418| 		if (controlsPlayer(GetEntityState(ent).player))
| 419| 419| 			ownEnts.push(ent);
| 420| 420| 		else
| 421| 421| 			otherEnts.push(ent);
| 422|    |-	}
|    | 422|+	
| 423| 423| 
| 424| 424| 	if (ownEnts.length)
| 425| 425| 		Engine.PostNetworkCommand({
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'goods'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Player.js
|  89|  89| 		this.resourceCount[res] = 300;
|  90|  90| 		this.resourceNames[res] = Resources.GetResource(res).name;
|  91|  91| 		this.tradingGoods.push({
|  92|    |-			"goods":  res,
|    |  92|+			"goods": res,
|  93|  93| 			"proba": 5 * (quotient + (+i < remainder ? 1 : 0))
|  94|  94| 		});
|  95|  95| 	}

binaries/data/mods/public/simulation/components/Player.js
| 363| »   for·(var·type·in·amounts)
|    | [NORMAL] JSHintBear:
|    | 'type' is already defined.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 410| 410| 				// Players see colors depending on diplomacy
| 411| 411| 				g_DisplayedPlayerColors[i] =
| 412| 412| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 413|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
|    | 413|+						g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 414| 414| 					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 415| 415| 					g_DiplomacyColorPalette.Enemy;
| 416| 416| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/gui/session/session.js
| 411| 411| 				g_DisplayedPlayerColors[i] =
| 412| 412| 					g_ViewedPlayer == i ? g_DiplomacyColorPalette.Self :
| 413| 413| 					g_Players[g_ViewedPlayer].isAlly[i] ? g_DiplomacyColorPalette.Ally :
| 414|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
|    | 414|+							g_Players[g_ViewedPlayer].isNeutral[i] ? g_DiplomacyColorPalette.Neutral :
| 415| 415| 					g_DiplomacyColorPalette.Enemy;
| 416| 416| 
| 417| 417| 		g_DisplayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /mnt/data/jenkins-p
elexis accepted this revision.Feb 3 2018, 11:12 AM

(Never speak of the last change, revision is something that doesn't go away. Always when I saw people post "last update" on their diff, they uploaded another :P #4237 for instance )

binaries/data/mods/public/gui/session/session.js
378

(Imagine the guy who wants to find out where an icon is used, if it can be renamed, replaced or deleted. Also works the other way around, reading the filename first makes it easier to open the image file than piecing together the filename)

(About the globals, In general splitting the code from the data is good practice (https://en.wikipedia.org/wiki/Separation_of_concerns pattern) and having them as globals means mods can change the global in a newly added file. But then again they can just replace the file and we observe the added complexity, especially when they were grouped logically, so having the png globals is not relevant here.)

source/simulation2/components/CCmpTerritoryManager.cpp
290

Reading the calls to this a bit more, it seems that the purpose of both functions is that other components and graphics files can be kept in sync with this. So the previous function names are actually ok in the previous code.
But that you add the playercolor condition to exactly one of them indeed makes it reasonable to rename it - otherwise it became a trap and some sim component might check for UpdateState and expect something simulation safe and trigger an OOS.

The "could also have other consequences in the simulation" part of D910 makes me think it could be relevant for the simulation in general too. Could go for NeedUpdateState or leave as is, since the other function name is distinguished with the Texture word.

(Adding the non-serialized playercolor condition to the one function is correct as it's only used from graphics/). (Perhaps some knucklehead will try to use the texture function in the sim and expect it to be sim-safe. That might be avoided by just adding getters for the three variables and deleting the two above functions, but who cares now. Commit your patch please X) )

lol u took now the aggressive colors? :(

can we go for a little more decent blue and red (and yellow)?

hurting really :(