Details

Reviewers

elexis

Commits

rP21107: Diplomacy Colors

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.

minimap-diplo-highlight.png (128×128 px, 5 KB)

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.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4801
Build 8323: Vulcan Build (Windows)	Jenkins
Build 8322: Vulcan Build	Jenkins
Build 8321: arc lint + arc unit

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
141	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)
299	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).
300	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.
555	(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
786	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.
812	We should replace that -1 with a constant some day.
817	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.

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

Harbormaster completed remote builds in B4693: Diff 5522.Jan 27 2018, 9:03 PM

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
795	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.
812	`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
812	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

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...

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

Harbormaster completed remote builds in B4782: Diff 5615.Feb 1 2018, 4:17 AM

elexis added inline comments.Feb 1 2018, 4:41 AM

binaries/data/mods/public/simulation/components/GuiInterface.js
806	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 }; }}}
812	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
806	(If it becomes a function, the IIDs can remain the inner loop too)

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.

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({

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)

Harbormaster completed remote builds in B4786: Diff 5623.Feb 1 2018, 6:39 PM

add a comment

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 (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

Harbormaster completed remote builds in B4789: Diff 5627.Feb 1 2018, 8:19 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
801	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.
291	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.
635	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.
683	(could use range-based loop)
703	same
814	(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 ? )
825	maybe continue
831	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.)

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

Placeholder looks good.

binaries/data/mods/public/gui/session/session.js
385	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
385	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
385	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.

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 (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

Harbormaster completed remote builds in B4799: Diff 5638.Feb 2 2018, 9:31 PM

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

binaries/data/mods/public/gui/session/session.js
385	Oh, right.
source/simulation2/components/CCmpTerritoryManager.cpp
291	Rename sounds good. (Added in D910.)
814	Yeah, this one deals with more than one color so I went for the plural.

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

Harbormaster completed remote builds in B4801: Diff 5639.Feb 3 2018, 12:14 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
385	(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
291	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) )

Commit rP21107.

elexis added a commit: rP21107: Diplomacy Colors.Feb 4 2018, 7:56 PM

lol u took now the aggressive colors? :(

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

hurting really :(

Diplomacy Colors
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 5639

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-off-highlight.png

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-off.png

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-on-highlight.png

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-on.png

binaries/data/mods/public/gui/gamesetup/gamesetup.js

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

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

binaries/data/mods/public/gui/session/minimap_panel.xml

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

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

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

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

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

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

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

source/graphics/TerritoryTexture.cpp

source/ps/Game.cpp

source/simulation2/components/CCmpAIManager.cpp

source/simulation2/components/CCmpMinimap.cpp

source/simulation2/components/CCmpRallyPointRenderer.cpp

source/simulation2/components/CCmpRangeOverlayRenderer.cpp

source/simulation2/components/CCmpSelectable.cpp

source/simulation2/components/CCmpTerritoryManager.cpp

source/simulation2/components/ICmpPlayer.h

source/simulation2/components/ICmpPlayer.cpp

source/simulation2/components/ICmpRallyPointRenderer.h

source/simulation2/components/ICmpRallyPointRenderer.cpp

source/simulation2/components/ICmpRangeOverlayRenderer.h

source/simulation2/components/ICmpRangeOverlayRenderer.cpp

source/simulation2/components/ICmpSelectable.h

source/simulation2/components/ICmpSelectable.cpp

source/simulation2/components/ICmpTerritoryManager.h

source/simulation2/components/ICmpTerritoryManager.cpp

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp

Diplomacy ColorsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 5639

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-off-highlight.png

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-off.png

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-on-highlight.png

binaries/data/mods/public/art/textures/ui/session/minimap-diplomacy-on.png

binaries/data/mods/public/gui/gamesetup/gamesetup.js

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

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

binaries/data/mods/public/gui/session/minimap_panel.xml

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

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

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

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

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

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

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

source/graphics/TerritoryTexture.cpp

source/ps/Game.cpp

source/simulation2/components/CCmpAIManager.cpp

source/simulation2/components/CCmpMinimap.cpp

source/simulation2/components/CCmpRallyPointRenderer.cpp

source/simulation2/components/CCmpRangeOverlayRenderer.cpp

source/simulation2/components/CCmpSelectable.cpp

source/simulation2/components/CCmpTerritoryManager.cpp

source/simulation2/components/ICmpPlayer.h

source/simulation2/components/ICmpPlayer.cpp

source/simulation2/components/ICmpRallyPointRenderer.h

source/simulation2/components/ICmpRallyPointRenderer.cpp

source/simulation2/components/ICmpRangeOverlayRenderer.h

source/simulation2/components/ICmpRangeOverlayRenderer.cpp

source/simulation2/components/ICmpSelectable.h

source/simulation2/components/ICmpSelectable.cpp

source/simulation2/components/ICmpTerritoryManager.h

source/simulation2/components/ICmpTerritoryManager.cpp

source/tools/atlas/GameInterface/Handlers/ObjectHandlers.cpp

Diplomacy Colors
ClosedPublic
Actions

Revision Contents
Changeset List