Page MenuHomeWildfire Games
Differential D1430

Fix some auras not being removed
ClosedPublic
Actions

Authored by temple on Apr 3 2018, 6:00 PM.

Details

Reviewers
None
Commits
rP21785: Stop some auras from being applied extra times, don't apply player auras to…
Summary

As mentioned in D1426, the Iberian team bonus wasn't removed after resigning if there was more than one player on the team. This was because it was accidentally applied extra times in rP19092. This patch fixes that.

Furthermore, since player auras only affect player entities, I don't think there's any need to apply them to templates as well. So this patch treats player auras separately from global auras.

Test Plan

See that bonuses are properly removed now.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

temple created this revision.Apr 3 2018, 6:00 PM
fatherbushido added a subscriber: fatherbushido.EditedApr 3 2018, 6:21 PM

Doing that first could have been an option:

Index: binaries/data/mods/public/simulation/components/tests/test_Auras.js
===================================================================
--- binaries/data/mods/public/simulation/components/tests/test_Auras.js	(révision 21641)
+++ binaries/data/mods/public/simulation/components/tests/test_Auras.js	(copie de travail)
@@ -96,8 +96,19 @@
 testAuras("global", (name, cmpAuras) => {
 	TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Component/Value", 5, targetEnt), 15);
 	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[1], template), 15);
+	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[2], template), 15);
 });
 
+testAuras("global", (name, cmpAuras) => {
+	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[1], template), 15);
+	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[2], template), 15);
+	AddMock(sourceEnt, IID_Ownership, {
+		"GetOwner": () => -1
+	});
+	cmpAuras.OnOwnershipChanged({ "from": sourceEnt, "to": -1 })
+	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[2], template), 5);
+});
+
 targetEnt = playerEnt[playerID[2]];
 testAuras("player", (name, cmpAuras) => {
 	TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Component/Value", 5, targetEnt), 15);

as it shows the issue (not specifically related to team bonus or to defeat, but related to the fact that 2-1=1) and would have prevent the guy I was to not catch that obvious mistake.

edit : it's not a comment about that diff, it's After-Sales-Management about my own mistake.

edit 2 : but basically fixing a bug and doing another stuff needs some good emulsifier

Vulcan added a subscriber: Vulcan.Apr 4 2018, 3:00 PM

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'Identity'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  29|  29| let sourceEnt = 20;
|  30|  30| let targetEnt = 30;
|  31|  31| let auraRange = 40;
|  32|    |-let template = { "Identity" : { "Classes" : { "_string" : "CorrectClass OtherClass" } } };
|    |  32|+let template = { "Identity": { "Classes" : { "_string" : "CorrectClass OtherClass" } } };
|  33|  33| 
|  34|  34| function testAuras(name, test_function)
|  35|  35| {
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'Classes'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  29|  29| let sourceEnt = 20;
|  30|  30| let targetEnt = 30;
|  31|  31| let auraRange = 40;
|  32|    |-let template = { "Identity" : { "Classes" : { "_string" : "CorrectClass OtherClass" } } };
|    |  32|+let template = { "Identity" : { "Classes": { "_string" : "CorrectClass OtherClass" } } };
|  33|  33| 
|  34|  34| function testAuras(name, test_function)
|  35|  35| {
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key '_string'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  29|  29| let sourceEnt = 20;
|  30|  30| let targetEnt = 30;
|  31|  31| let auraRange = 40;
|  32|    |-let template = { "Identity" : { "Classes" : { "_string" : "CorrectClass OtherClass" } } };
|    |  32|+let template = { "Identity" : { "Classes" : { "_string": "CorrectClass OtherClass" } } };
|  33|  33| 
|  34|  34| function testAuras(name, test_function)
|  35|  35| {
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'playerID'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  52|  52| 
|  53|  53| 	AddMock(playerEnt[1], IID_Player, {
|  54|  54| 		"IsAlly": id => id == playerID[1] || id == playerID[2],
|  55|    |-		"IsEnemy": id => id !=  playerID[1] || id != playerID[2],
|    |  55|+		"IsEnemy": id => id != playerID[1] || id != playerID[2],
|  56|  56| 		"GetPlayerID": () => playerID[1],
|  57|  57| 		"GetState": () => playerState
|  58|  58| 	});
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'added'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
| 121| 121| });
| 122| 122| 
| 123| 123| testAuras("garrisonedUnits", (name, cmpAuras) => {
| 124|    |-	cmpAuras.OnGarrisonedUnitsChanged({ "added" : [targetEnt], "removed": [] });
|    | 124|+	cmpAuras.OnGarrisonedUnitsChanged({ "added": [targetEnt], "removed": [] });
| 125| 125| 	TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Component/Value", 5, targetEnt), 15);
| 126| 126| 	cmpAuras.OnGarrisonedUnitsChanged({ "added" : [], "removed": [targetEnt] });
| 127| 127| 	TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Component/Value", 5, targetEnt), 5);
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space after key 'added'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/tests/test_Auras.js
| 123| 123| testAuras("garrisonedUnits", (name, cmpAuras) => {
| 124| 124| 	cmpAuras.OnGarrisonedUnitsChanged({ "added" : [targetEnt], "removed": [] });
| 125| 125| 	TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Component/Value", 5, targetEnt), 15);
| 126|    |-	cmpAuras.OnGarrisonedUnitsChanged({ "added" : [], "removed": [targetEnt] });
|    | 126|+	cmpAuras.OnGarrisonedUnitsChanged({ "added": [], "removed": [targetEnt] });
| 127| 127| 	TS_ASSERT_EQUALS(ApplyValueModificationsToEntity("Component/Value", 5, targetEnt), 5);
| 128| 128| });
| 129| 129| 

binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  31| let·auraRange·=·40;
|    | [NORMAL] JSHintBear:
|    | 'auraRange' was used before it was defined.
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'texture'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
| 100| 100| 				// Specify default in order not to specify it in about 40 auras
| 101| 101| 				{
| 102| 102| 					"radius": this.GetRange(name),
| 103|    |-					"texture":  "outline_border.png",
|    | 103|+					"texture": "outline_border.png",
| 104| 104| 					"textureMask":  "outline_border_mask.png",
| 105| 105| 					"thickness":  0.2
| 106| 106| 				});
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'textureMask'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
| 101| 101| 				{
| 102| 102| 					"radius": this.GetRange(name),
| 103| 103| 					"texture":  "outline_border.png",
| 104|    |-					"textureMask":  "outline_border_mask.png",
|    | 104|+					"textureMask": "outline_border_mask.png",
| 105| 105| 					"thickness":  0.2
| 106| 106| 				});
| 107| 107| 	}
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'thickness'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
| 102| 102| 					"radius": this.GetRange(name),
| 103| 103| 					"texture":  "outline_border.png",
| 104| 104| 					"textureMask":  "outline_border_mask.png",
| 105|    |-					"thickness":  0.2
|    | 105|+					"thickness": 0.2
| 106| 106| 				});
| 107| 107| 	}
| 108| 108| 
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
| 122| 122| 
| 123| 123| 	var numPlayers = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers();
| 124| 124| 	for (var i = 0; i < numPlayers; ++i)
| 125|    |-	{
|    | 125|+	
| 126| 126| 		for (let p of affectedPlayers)
| 127| 127| 		{
| 128| 128| 			if (p == "Player" ? cmpPlayer.GetPlayerID() == i : cmpPlayer["Is" + p](i))
| 131| 131| 				break;
| 132| 132| 			}
| 133| 133| 		}
| 134|    |-	}
|    | 134|+	
| 135| 135| };
| 136| 136| 
| 137| 137| Auras.prototype.CanApply = function(name)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
| 124| 124| 	for (var i = 0; i < numPlayers; ++i)
| 125| 125| 	{
| 126| 126| 		for (let p of affectedPlayers)
| 127|    |-		{
|    | 127|+		
| 128| 128| 			if (p == "Player" ? cmpPlayer.GetPlayerID() == i : cmpPlayer["Is" + p](i))
| 129| 129| 			{
| 130| 130| 				this.affectedPlayers[name].push(i);
| 131| 131| 				break;
| 132| 132| 			}
| 133|    |-		}
|    | 133|+		
| 134| 134| 	}
| 135| 135| };
| 136| 136|

Link to build: https://jenkins.wildfiregames.com/job/differential/337/display/redirect

Harbormaster failed remote builds in B5744: Diff 6309!Apr 4 2018, 3:00 PM
fatherbushido added inline comments.Apr 4 2018, 5:28 PM
binaries/data/mods/public/simulation/components/tests/test_Auras.js
149 ↗(On Diff #6309)

uhm old me, I guess the thing you wanted to do was

testAuras("global", (name, cmpAuras) => {
	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[2], template), 15);
	playerState = "defeated";
	cmpAuras.OnPlayerDefeated({ "playerId": playerID[1] });
	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[2], template), 5);
	TS_ASSERT_EQUALS(ApplyValueModificationsToTemplate("Component/Value", 5, playerID[1], template), 5);
});

(source owned by player 1, bonus for templates of its ally player 2, player defeated, no bonus for templates of its ally player 2)

temple updated this revision to Diff 6360.Apr 9 2018, 4:33 AM

update tests, fix linting

Vulcan added a comment.Apr 9 2018, 4:35 AM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  31| let·auraRange·=·40;
|    | [NORMAL] JSHintBear:
|    | 'auraRange' was used before it was defined.
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
| 122| 122| 
| 123| 123| 	var numPlayers = Engine.QueryInterface(SYSTEM_ENTITY, IID_PlayerManager).GetNumPlayers();
| 124| 124| 	for (var i = 0; i < numPlayers; ++i)
| 125|    |-	{
|    | 125|+	
| 126| 126| 		for (let p of affectedPlayers)
| 127| 127| 		{
| 128| 128| 			if (p == "Player" ? cmpPlayer.GetPlayerID() == i : cmpPlayer["Is" + p](i))
| 131| 131| 				break;
| 132| 132| 			}
| 133| 133| 		}
| 134|    |-	}
|    | 134|+	
| 135| 135| };
| 136| 136| 
| 137| 137| Auras.prototype.CanApply = function(name)
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-of'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Auras.js
| 124| 124| 	for (var i = 0; i < numPlayers; ++i)
| 125| 125| 	{
| 126| 126| 		for (let p of affectedPlayers)
| 127|    |-		{
|    | 127|+		
| 128| 128| 			if (p == "Player" ? cmpPlayer.GetPlayerID() == i : cmpPlayer["Is" + p](i))
| 129| 129| 			{
| 130| 130| 				this.affectedPlayers[name].push(i);
| 131| 131| 				break;
| 132| 132| 			}
| 133|    |-		}
|    | 133|+		
| 134| 134| 	}
| 135| 135| };
| 136| 136|

Link to build: https://jenkins.wildfiregames.com/job/differential/372/display/redirect

Harbormaster completed remote builds in B5811: Diff 6360.Apr 9 2018, 4:35 AM
elexis mentioned this in rP19093: Remove team bonus when the source player is defeated. Reviewed by wraitii..Apr 11 2018, 5:05 PM
fatherbushido added inline comments.Apr 13 2018, 6:59 PM
binaries/data/mods/public/simulation/components/tests/test_Auras.js
66 ↗(On Diff #6360)

How azy was the author of those lines to avoid a diplomacy matrix

temple updated this revision to Diff 6384.Apr 13 2018, 8:16 PM

rebase

Vulcan added a comment.Apr 13 2018, 9:12 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  31| var·auraRange·=·40;
|    | [NORMAL] JSHintBear:
|    | 'auraRange' was used before it was defined.

Link to build: https://jenkins.wildfiregames.com/job/differential/383/display/redirect

Harbormaster completed remote builds in B5856: Diff 6384.Apr 13 2018, 9:12 PM
fatherbushido added a comment.Apr 14 2018, 7:57 AM

capsule time:
when the typo/copypaste/bug fix and another thing can be split, perhaps could they be split.

elexis mentioned this in rP21713: Don't apply team bonuses to defeated players, counterpart to rP19093 / D12..Apr 14 2018, 12:09 PM
elexis added a subscriber: elexis.Apr 20 2018, 2:05 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/Auras.js
190 ↗(On Diff #6384)

I'm not too familiar with the template modification functions, but it looks indeed cleaner to me to have a 1 to 1 / bijective mapping between type property and function return value.

245 ↗(On Diff #6384)

So the primary bugfix is this ApplyTemplateBonus call not performed anymore for auras affecting player entities?

246 ↗(On Diff #6384)

(I'm not a fan of forEach unless we need a function to address closure issues or if both arguments of forEach are used, as we use for-loops everywhere else consistently for the same use case.)

252 ↗(On Diff #6384)

(Maybe shorten by inlining cmpPlayerManager)

255 ↗(On Diff #6384)

This if-continue-if-continue pattern looks cleaner than the nesting before too.
(It also makes me think of using different aura components all implementing an aura interface. Should consider performance cost or benefit then. Might be faster since less type checks have to be performed.)

fatherbushido added a comment.Apr 20 2018, 4:47 PM

The bug fix is
https://code.wildfiregames.com/rP19092#29903
It was an obvious 'copy paste' error I did (mixing two ways of writing things). Straightforward.

About oil and water: https://code.wildfiregames.com/D1430#59103

temple added inline comments.Apr 20 2018, 7:35 PM
binaries/data/mods/public/simulation/components/Auras.js
247–253 ↗(On Diff #6384)

Bugfix was this whole thing was applied affectedPlayers.length times instead of just once.

temple updated this revision to Diff 6444.Apr 20 2018, 11:10 PM
temple marked an inline comment as done.
Vulcan added a comment.Apr 20 2018, 11:51 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_Auras.js
|  31| var·auraRange·=·40;
|    | [NORMAL] JSHintBear:
|    | 'auraRange' was used before it was defined.

Link to build: https://jenkins.wildfiregames.com/job/differential/421/display/redirect

Harbormaster completed remote builds in B5924: Diff 6444.Apr 20 2018, 11:52 PM
fatherbushido added a comment.Apr 23 2018, 11:09 PM

I guess moving one line is something bad and it s better to keep that obvious broken loop.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 26 2018, 2:35 AM
Closed by commit rP21785: Stop some auras from being applied extra times, don't apply player auras to… (authored by temple). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
ps/
trunk/
binaries/
data/
mods/
public/
simulation/
components/
28 lines
tests/
50 lines

Diff 6476

View Options

ps/trunk/binaries/data/mods/public/simulation/components/Auras.js

Loading...
View Options

ps/trunk/binaries/data/mods/public/simulation/components/tests/test_Auras.js

Loading...
Wildfire Games · Phabricator