Page MenuHomeWildfire Games

Fix stats of peak percentage of map controlled
ClosedPublic

Authored by mimo on Feb 11 2018, 10:58 AM.

Details

Reviewers
bb
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP21717: Fix stats of peak percentage of map controlled
Summary

Sometimes, the peak percentage of map controlled happens to be smaller than the final percentage. This i guess (i've not checked that part of the code) is because the PlayerDefeated message is treated before the TerritoryChanged one, and thus the peak percentage is not updated for the final gain.

With the patch, the peakControlledMap statistics are independent of the order of such messages.

Test Plan

A way to test is to use autostart-nonvisual as it prints the final stats after a game.

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

mimo created this revision.Feb 11 2018, 10:58 AM
Vulcan added a subscriber: Vulcan.Feb 11 2018, 11:45 AM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'for-in'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/StatisticsTracker.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/StatisticsTracker.js
| 577| 577| {
| 578| 578| 	if (typeof fromData == "object")
| 579| 579| 		for (let prop in fromData)
| 580|    |-		{
|    | 580|+		
| 581| 581| 			if (typeof toData[prop] != "object")
| 582| 582| 				toData[prop] = [fromData[prop]];
| 583| 583| 			else
| 584| 584| 				this.PushValue(fromData[prop], toData[prop]);
| 585|    |-		}
|    | 585|+		
| 586| 586| 	else
| 587| 587| 		toData.push(fromData);
| 588| 588| };

Link to build: https://jenkins.wildfiregames.com/job/phabricator_lint/1022/display/redirect

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

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

elexis added a subscriber: elexis.Feb 11 2018, 12:57 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/StatisticsTracker.js
529 ↗(On Diff #5756)

A Getter should not set things. It seems like a way to fix a bug after it happened, should rather fix it in the right place.
In the worst case this pattern could trigger an OOS as the getter (not in the current code but not unlikely) might be called from the GUIInterface for only one of the clients.
Your suspicion that the order of messages is relevant sounds like a hot trail.
A UpdatePercentageMapControlled function called from PlayerDefeated, (PlayerWon?) and TerritoriesChanged would seem cleaner to me.

558 ↗(On Diff #5756)

(Math.max removes one step of interpretation when reading)

mimo updated this revision to Diff 5775.Feb 12 2018, 8:57 PM

update following elexis suggestions

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

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

CMessageTerritoriesChanged is only sent upon MT_Update, i.e. once per turn, whereas PlayerDefeated can be sent in the middle of a turn.
If I understand correctly, the problem is that the autostart subscribes to the playerdefeat event, prints the outdated statistics, but after that the statistics are corrected?
Then we could add a comment stating that it's relevant for that one turn.

binaries/data/mods/public/simulation/components/StatisticsTracker.js
573 ↗(On Diff #5775)

Could be one line (with tolerable 121 characters) or

	this.teamPeakPercentMapControlled = Math.max(
		this.teamPeakPercentMapControlled,
		this.GetTeamPercentMapControlled());

(or remain this way idc)

bb accepted this revision.Apr 13 2018, 2:34 PM
bb added a subscriber: bb.

New code looks much simpler and less error prone

the elexis comment seems simple enough => accept

This revision is now accepted and ready to land.Apr 13 2018, 2:34 PM

New code looks much simpler and less error prone

But is it correct and needed? Was the problem analyzed correctly?

the elexis comment seems simple enough => accept

The whitespace is not so important to me than the open question, otherwise I had accepted this before.

The patch is attractive and I would really like to accept it, but I'm a bit eager to commit a solution without understanding the problem well.

This i guess (i've not checked that part of the code) is because the PlayerDefeated message is treated before the TerritoryChanged one, and thus the peak percentage is not updated for the final gain.

So it is only relevant if the player or the autostart thing ends the game in the middle of a turn, after PlayerDefeated but before TerritoryChanged?
If so, we should add a code comment informing the reader.

binaries/data/mods/public/simulation/components/StatisticsTracker.js
562 ↗(On Diff #5775)

OnGlobalPlayerWon: If it isn't sufficient to only call it for affected playerentities, then it is easier to recognize and more consistent to use a global subscription. D1426 removes the broadcasting of the message like D733.

mimo added a comment.Apr 14 2018, 11:26 AM
In D1294#59071, @bb wrote:

New code looks much simpler and less error prone
the elexis comment seems simple enough => accept

thanks

In D1294#59078, @elexis wrote:

New code looks much simpler and less error prone

But is it correct and needed? Was the problem analyzed correctly?

the elexis comment seems simple enough => accept

The whitespace is not so important to me than the open question, otherwise I had accepted this before.
The patch is attractive and I would really like to accept it, but I'm a bit eager to commit a solution without understanding the problem well.

This i guess (i've not checked that part of the code) is because the PlayerDefeated message is treated before the TerritoryChanged one, and thus the peak percentage is not updated for the final gain.

So it is only relevant if the player or the autostart thing ends the game in the middle of a turn, after PlayerDefeated but before TerritoryChanged?
If so, we should add a code comment informing the reader.

Do you now have analysed it correctly? then you'd realize the comment is not needed. The goal of the patch is to make the result independent of the message orders, independently of all the details you are discussing. Of course (and you can do it if that problem really bothers you), you could change the way autostart games listen to messages so that it is always done at the end of a turn, but that would add complexity and make it less robust imo.

This revision was automatically updated to reflect the committed changes.

As mentioned I suspect the player can end the game or open the summary screen in the middle of a turn too, so it's not only for autostarted games.
Still think that the first question that a reader of the code has is why it's needed, but I guess he can use archeology too or do some code digging and guessing.
(Otherwise thanks for having implemented my proposals)