Page MenuHomeWildfire Games
Differential D1453

Update auras of entites of non-defeated players that affect defeated players
ClosedPublic
Actions

Authored by elexis on Apr 16 2018, 3:36 PM.

Details

Reviewers
temple
Commits
rP21779: Update auras of entities of other players that affected the defeated player…
Summary

As reported by fatherbushido in rP21713, the check in rP21711 was too strict and rP21713 should have extended it to encompass cleaning of non-player entities having playerauras to exclude defeated players.

Test Plan

Apply this diff:

Index: binaries/data/mods/public/simulation/templates/template_structure_civic_civil_centre.xml
===================================================================
--- binaries/data/mods/public/simulation/templates/template_structure_civic_civil_centre.xml	(revision 21730)
+++ binaries/data/mods/public/simulation/templates/template_structure_civic_civil_centre.xml	(working copy)
@@ -1,7 +1,10 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Entity parent="template_structure_civic">
+  <Auras datatype="tokens">
+    teambonuses/ptol_player_teambonus
+  </Auras>
   <AlertRaiser>
     <List datatype="tokens">FemaleCitizen</List>
     <RaiseAlertRange>140</RaiseAlertRange>
     <EndOfAlertRange>190</EndOfAlertRange>
     <SearchRange>100</SearchRange>

Start a 2v1 with cheats enabled / singleplayermode.
Type "exodia 2" to defeat your ally.
Press Alt+D to change the perspective to that ally.
Notice the defeated player still gets a food trickle, but defeated players should not receive anything anymore, unless this patch is applied.

Diff Detail

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

Event Timeline

elexis created this revision.Apr 16 2018, 3:36 PM
Vulcan added a subscriber: Vulcan.Apr 16 2018, 3:50 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (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| 

binaries/data/mods/public/simulation/components/Auras.js
| 251| »   »   »   »   »   let·playerEnts·=·affectedPlayers.map(player·=>·cmpPlayerManager.GetPlayerByID(player));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'player' is already declared in the upper scope.

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

Harbormaster completed remote builds in B5886: Diff 6410.Apr 16 2018, 3:50 PM
elexis updated this revision to Diff 6424.Apr 18 2018, 5:02 PM

Notice that this cleans affected auras twice (once on ownership and once on defeat), but I don't see a clean way to prevent that if we don't want to assume design decisions such as gaia affecting the player.
(Tending to not be convinced to add a comment explaining that these entities are updated twice.)
Can I commit this bugfix for the release?

Vulcan added a comment.Apr 18 2018, 5:59 PM

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

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (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| 

binaries/data/mods/public/simulation/components/Auras.js
| 251| »   »   »   »   »   let·playerEnts·=·affectedPlayers.map(player·=>·cmpPlayerManager.GetPlayerByID(player));
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'player' is already declared in the upper scope.

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

Harbormaster completed remote builds in B5902: Diff 6424.Apr 18 2018, 5:59 PM
temple accepted this revision.Apr 18 2018, 10:54 PM
temple added a subscriber: temple.

I guess it only needs to update an aura for the player entity for the player that resigned, rather than all players, but whatever.
Other auras are still affecting the defeated player's units, except he doesn't have units anymore so it doesn't matter.

This revision is now accepted and ready to land.Apr 18 2018, 10:54 PM
elexis added a subscriber: fatherbushido.Apr 19 2018, 11:48 AM
In D1453#59646, @temple wrote:

I guess it only needs to update an aura for the player entity for the player that resigned, rather than all players

The auras of allies that affect the defeated player entity (ptolemian resource trickle) have to be updated as well, right?
It only doesn't add the code to test which ones are affected to avoid the logic duplicate with CalculateAffectedPlayers.
Perhaps I should add the comments for both this fact and the fact that structure/unit entities affecting player entities are updated redundantly too, so people have less questions after analyzing the code and spend less time analyzing the code by reading the motivation in the comment.

but whatever.

If the updates were unneeded and can be removed by shortening code, it should be done however.

Other auras are still affecting the defeated player's units, except he doesn't have units anymore so it doesn't matter.

Ranged, formation and garrison auras should be updated by the ownership change already.
I can add a comment as well that auras affecting owned entities of the players don't need to be updated because the player doesn't own any entities anymore.

@fatherbushido any objections so far?

fatherbushido added a comment.Apr 19 2018, 11:52 AM

(((One source of redundant clean is that they are done for all auras of a given entity. Clean(name) was a possibility, but it didn't seem worth the value)))
In the context of that diff, I wonder which situation trigger a (true) duplicate Ownership / Defeated call to clean. If so, there exists a (clean?) way to avoid it.

elexis added a comment.Apr 19 2018, 11:57 AM

which situation trigger a (true) duplicate Ownership / Defeated call to clean

Wonders are cleaned twice if a player resigns

If so, there exists a (clean?) way to avoid it.

Maybe CalculateAffectedPlayers could return the players rather than setting the private variable, but not sure if it's worth to add the code.

fatherbushido added a comment.Apr 19 2018, 12:23 PM

Wonders are cleaned twice if a player resigns doesn't look like an ownership / defeated one.

CalculateAffectedPlayers calculates the affected players
GetAffectedPlayers returns them
So I guess one of the solution (of that potential issue) I had in mind is already exibithed.

elexis added a comment.Apr 19 2018, 12:40 PM

Wonders are cleaned twice if a player resigns doesn't look like an ownership / defeated one.

Wonders are cleaned upon resign once from MT_OwnershipChange and one from MT_PlayerDefeated (not sure what you mean).

Cleaning only some of the auras is a possible optimization too, but looking at the existing complexity of Clean, it sounds like easily introducing bugs.
Calling CalculateAffectedPlayers outside of clean also sounds like trouble, as it would invalidate the comment // this makes sure the template bonuses are removed from the correct players.

fatherbushido added a comment.EditedApr 19 2018, 12:49 PM
  • so it looks that the relevant case (about that useless clean call) is when the aura source owner is defeated. If so, there is several solutions.
  • If my memory is good, the result of the discussion about Clean(name) was something like that.
  • Calling CalculateAffectedPlayers outside Clean is a bad idea, calling GetAffectedPlayers is a good one.
elexis added a comment.Apr 19 2018, 2:13 PM

Ah, indeed we need the previously affected players, not the new one, then it seems easy.
While testing I also noticed that auras are cleaned twice when creating the entity: Once on init, once on ownership change. Not sure if something can be done about that due to the autogarrisoning voodoo.

elexis updated this revision to Diff 6428.Apr 19 2018, 2:14 PM

Use GetAffectedPlayers to remove a number of redundant Clean call cases.

Vulcan added a comment.Apr 19 2018, 2:36 PM

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

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

binaries/data/mods/public/simulation/components/Auras.js
| 487| »   let·cmpPlayer·==·Engine.QueryInterface(this.entity,·IID_Player);
|    | [MAJOR] ESLintBear:
|    | Parsing error: Unexpected token ==

binaries/data/mods/public/simulation/components/Auras.js
| 487| »   let·cmpPlayer·==·Engine.QueryInterface(this.entity,·IID_Player);
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Auras.js
| 487| »   let·cmpPlayer·==·Engine.QueryInterface(this.entity,·IID_Player);
|    | [MAJOR] JSHintBear:
|    | Expected an identifier and instead saw '=='.

binaries/data/mods/public/simulation/components/Auras.js
| 487| »   let·cmpPlayer·==·Engine.QueryInterface(this.entity,·IID_Player);
|    | [NORMAL] JSHintBear:
|    | Expected an assignment or function call and instead saw an expression.

binaries/data/mods/public/simulation/components/Auras.js
| 487| »   let·cmpPlayer·==·Engine.QueryInterface(this.entity,·IID_Player);
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/Auras.js
| 490| »   ····this.Clean();
|    | [MAJOR] JSHintBear:
|    | Expected ')' to match '(' from line 488 and instead saw 'this'.

binaries/data/mods/public/simulation/components/Auras.js
| 490| »   ····this.Clean();
|    | [MAJOR] JSHintBear:
|    | Expected an identifier and instead saw '.'.

binaries/data/mods/public/simulation/components/Auras.js
| 490| »   ····this.Clean();
|    | [NORMAL] JSHintBear:
|    | Expected an assignment or function call and instead saw an expression.

binaries/data/mods/public/simulation/components/Auras.js
| 490| »   ····this.Clean();
|    | [MAJOR] JSHintBear:
|    | Missing semicolon.

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

Harbormaster failed remote builds in B5907: Diff 6428!Apr 19 2018, 2:37 PM
fatherbushido added a comment.Apr 19 2018, 2:38 PM
This comment was removed by fatherbushido.
temple added inline comments.Apr 19 2018, 6:31 PM
binaries/data/mods/public/simulation/components/Auras.js
493

And player aura, since the defeated player doesn't have any other entities anymore?

494

tab

elexis added inline comments.Apr 19 2018, 9:32 PM
binaries/data/mods/public/simulation/components/Auras.js
493

Entities owned by that player are cleaned on OwnershipChange, then both conditions here are false and we avoid any redundant unneeded cleans.
All player entities and entities owned by other players are cleaned here if and only if they affected the player.
Should cover all cases, am I wrong?

temple accepted this revision.Apr 20 2018, 2:03 AM
temple added inline comments.
binaries/data/mods/public/simulation/components/Auras.js
491

=

493

I was just saying that cleaning auras that aren't a player aura won't change any entities since the defeated player doesn't have any other entities. But I guess more correct to do it anyway.

Also missing a )

elexis added a comment.Apr 20 2018, 12:16 PM

@fatherbushido good with the patch too? (besides the syntax issues, I was in a hurry to receive some too hot sunlight and too cold water)

binaries/data/mods/public/simulation/components/Auras.js
493

The IsPlayerAura check is unneeded now, so I have removed it and in case there were any non-player-aura entities (when some assumption changed in the future), it would be more solid to clean these too, so a win-win situation afaics.

elexis added a comment.Apr 24 2018, 12:27 PM

I thought that this still cleans gaia entities twice that were formerly owned by the now defeated player. But since defeated players cant be affected anymore, the ownershipchange clean prohibits the defeated clean.
Sure there isn't any odd case missed? Otherwise I'll commit it later.

Index: binaries/data/mods/public/simulation/components/Auras.js
===================================================================
--- binaries/data/mods/public/simulation/components/Auras.js	(revision 21772)
+++ binaries/data/mods/public/simulation/components/Auras.js	(working copy)
@@ -200,6 +200,7 @@
  */
 Auras.prototype.Clean = function()
 {
+	warn(this.entity)
 	var cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager);
 	var auraNames = this.GetAuraNames();
 	let targetUnitsClone = {};
@@ -480,14 +481,17 @@
 };
 
 /**
- * Only update playerauras, since units and structures are updated OnOwnershipChanged.
+ * Update auras of the player entity and entities affecting player entities that didn't change ownership.
  */
 Auras.prototype.OnGlobalPlayerDefeated = function(msg)
 {
-	if (!Engine.QueryInterface(this.entity, IID_Player))
-		return;
-
-	this.Clean();
+	let cmpPlayer = Engine.QueryInterface(this.entity, IID_Player);
+	if (cmpPlayer && cmpPlayer.GetPlayerID() == msg.playerId ||
+	    this.GetAuraNames().some(name => {
+	    	warn(this.entity + " " + name + " " + uneval(this.GetAffectedPlayers(name)))
+	    	return this.GetAffectedPlayers(name).indexOf(msg.playerId) != -1;
+	    }))
+		this.Clean();
 };
 
 Engine.RegisterComponentType(IID_Auras, "Auras", Auras);
Index: binaries/data/mods/public/simulation/data/auras/teambonuses/ptol_player_teambonus.json
===================================================================
--- binaries/data/mods/public/simulation/data/auras/teambonuses/ptol_player_teambonus.json	(revision 21772)
+++ binaries/data/mods/public/simulation/data/auras/teambonuses/ptol_player_teambonus.json	(working copy)
@@ -1,7 +1,7 @@
 {
 	"type": "player",
 	"affects": ["Player"],
-	"affectedPlayers": ["ExclusiveMutualAlly"],
+	"affectedPlayers": ["Player", "Ally", "Enemy"],
 	"modifications": [
 		{ "value": "ResourceTrickle/Rates/food", "add": 1.0 }
 	],
Index: binaries/data/mods/public/simulation/templates/template_structure_civic_civil_centre.xml
===================================================================
--- binaries/data/mods/public/simulation/templates/template_structure_civic_civil_centre.xml	(revision 21772)
+++ binaries/data/mods/public/simulation/templates/template_structure_civic_civil_centre.xml	(working copy)
@@ -1,5 +1,8 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Entity parent="template_structure_civic">
+  <Auras datatype="tokens">
+    teambonuses/ptol_player_teambonus
+  </Auras>
   <AlertRaiser>
     <List datatype="tokens">FemaleCitizen</List>
     <RaiseAlertRange>140</RaiseAlertRange>
fatherbushido added a comment.Apr 24 2018, 3:19 PM

GetAffectedPlayers did a strike

Closed by commit rP21779: Update auras of entities of other players that affected the defeated player… (authored by elexis). · Explain WhyApr 24 2018, 7:28 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

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

Diff 6424

View Options

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

Loading...
Wildfire Games · Phabricator