Page MenuHomeWildfire Games

OverlayCounter classes
ClosedPublic

Authored by elexis on Oct 26 2019, 8:21 PM.

Details

Summary

The purpose of this diff is to use object semantics using class syntax for the
FPS/realtime/gametime/ceasefiretime counters shown in the top right corner of the screen in the course of #5387 and
to make the gui/common/ code agnostic of the session code.

The gui/common/functions_global_object.js file hardcodes the name of a session function and calls that if it exists.
Prior to rP19268 there were even more references including g_SimState in gui/common/.

rP16624 had hadded the ceasefire hotkey refernce without inserting them in default.cfg, so basically noone knew about the hotkey, fixed here.

Another small odditiy removed here are the objects in global.xml that on first read appear like they would be the counter objects.
But they are not. Despite having the type "text", they never show any text, the style is unused too. The objects only
exist to have something to attach the hotkey to, thus can be superseded by Engine.SetGlobalHotkey from D2260/rP22851.

The advantages of the class hierarchy are that one can perform some tiny performance tunings and to split the different counters into different files,
allowing to read, comprehend, insert, delete or modify one without having to modify or read the others.

The (mostly negligible but relevant in principle) performance leverage is that one can

  • avoid constructing a sprintf JS object every call and can reuse a once allocated one,
  • avoid doing Engine GetConfig calls every call

To achieve these benefits:

  • ceasefire message subscription following style of rP23076/D2378
  • move of configchange message from rP23091/D2389 from session/ to global/, fired from within options page and mainmenu when closing the options page

In order to avoid quadduplication of the overlaycounters logic,
this is the first patch that uses the class inheritance using the extends keyword.

Why the patch isn't ideal:

  • The ConfigChange message should still be sent from C++, not JS
  • g_OverlayCounterManager needs to be assigned in onLoad in XML, since the XML file wasnt loaded yet when the JS file is loaded and since it can't insert itself into JS and since we dont want JS code in global scope that isnt a class, function or variable definition.
  • This still requires one function call per counter and requires traversing the prototype chain. So in this regard slower than the mashing all counters in two functions.
  • More complexity. Not updating everything at all all the time means there is more possibility to get the wrong state in some circumstance somewhere.
  • The order in how the counters appear cannot be determined in a way that is compatible with the objective of the patch to allow insertion and removal of one ceasefire without changing the manager or requiring knowledge of the other overlay counters. One would have to use an array like ["counterName2", "counterName1", ...] to specify an order, or use arbitrary intransparent priority numbers, or remove the ability to use a specific order. But the latter would be wrong, people expect that the ceasefire counter appears last. Since this problem is not solveable with the current ideas, I used the path of least ugliness and chose the classname, and thus had to use RemainingCeasefire instead of Ceasefire in order to change the, order.

Aside from that, there is also a much more effective and simple performance boost in this diff

  • Update the counter 4 times per second, not once per frame
  • Don't resize if the number of lines didn't change (happens very rarely)
Test Plan

Test the hotkeys, triggering the different options in the main menu and in a running game, ceasefire, add warn() to show how often the functions are really are called, and to see that there are really only as many counters iterated as are enabled.

Notice that with the 250ms delay the performance consideration should have become irrelevant.

I did the following measurement for the time spent in onTick before and after.
If there are only two counters, the new code is slightly slower. If there are more, the new code is slightly faster. Per call that is, i.e. still irrelevant.


In the graphics and table we see the average timeconsumption is about 450 microseconds on my machine with the previous code and about 420microseconds with the new code.
That it is very consistent for the new code, but the previosu code had many bumps at 100 microseconds. If those are ignored, I get the 30ms faster performance, I didn't further check whether that is a valid comparison, because the performance doesn't matter with this 250ms delay anyhow.

'Test that it works.'
Notice that the ceasefire event subscription is necessary for diplomacy colors, because the diplomacy-changed GUI message is only sent from Commands.js, when a player sent a command, not when the components decided for themselves to change the diplomacies.

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

elexis created this revision.Oct 26 2019, 8:21 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/488/display/redirect

elexis edited the test plan for this revision. (Show Details)Oct 26 2019, 8:27 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/OverlayCounterElapsedTime.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/OverlayCounterElapsedTime.js
|   9|   9| 
|  10|  10| 		// Performance optimization
|  11|  11| 		this.caption = translate(this.Caption);
|  12|    |-		this.sprintfData = {}
|    |  12|+		this.sprintfData = {};
|  13|  13| 	}
|  14|  14| 
|  15|  15| 	get()

binaries/data/mods/public/gui/session/OverlayCounterElapsedTime.js
|  12| »   »   this.sprintfData·=·{}
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 193| 193| 		"tooltip": translate("Exits the game."),
| 194| 194| 		"onPress": () => {
| 195| 195| 			messageBox(
| 196|    |-					400, 200,
|    | 196|+				400, 200,
| 197| 197| 					translate("Are you sure you want to quit 0 A.D.?"),
| 198| 198| 					translate("Confirmation"),
| 199| 199| 					[translate("No"), translate("Yes")],
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 194| 194| 		"onPress": () => {
| 195| 195| 			messageBox(
| 196| 196| 					400, 200,
| 197|    |-					translate("Are you sure you want to quit 0 A.D.?"),
|    | 197|+				translate("Are you sure you want to quit 0 A.D.?"),
| 198| 198| 					translate("Confirmation"),
| 199| 199| 					[translate("No"), translate("Yes")],
| 200| 200| 					[null, Engine.Exit]);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 195| 195| 			messageBox(
| 196| 196| 					400, 200,
| 197| 197| 					translate("Are you sure you want to quit 0 A.D.?"),
| 198|    |-					translate("Confirmation"),
|    | 198|+				translate("Confirmation"),
| 199| 199| 					[translate("No"), translate("Yes")],
| 200| 200| 					[null, Engine.Exit]);
| 201| 201| 		}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 196| 196| 					400, 200,
| 197| 197| 					translate("Are you sure you want to quit 0 A.D.?"),
| 198| 198| 					translate("Confirmation"),
| 199|    |-					[translate("No"), translate("Yes")],
|    | 199|+				[translate("No"), translate("Yes")],
| 200| 200| 					[null, Engine.Exit]);
| 201| 201| 		}
| 202| 202| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/pregame/MainMenuItems.js
| 197| 197| 					translate("Are you sure you want to quit 0 A.D.?"),
| 198| 198| 					translate("Confirmation"),
| 199| 199| 					[translate("No"), translate("Yes")],
| 200|    |-					[null, Engine.Exit]);
|    | 200|+				[null, Engine.Exit]);
| 201| 201| 		}
| 202| 202| 	}
| 203| 203| ];
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
| 119| 119| 			sprintf(
| 120| 120| 				option.min !== undefined && option.max !== undefined ?
| 121| 121| 					translateWithContext("option number", "Min: %(min)s, Max: %(max)s") :
| 122|    |-				option.min !== undefined && option.max === undefined ?
|    | 122|+					option.min !== undefined && option.max === undefined ?
| 123| 123| 					translateWithContext("option number", "Min: %(min)s") :
| 124| 124| 				option.min === undefined && option.max !== undefined ?
| 125| 125| 					translateWithContext("option number", "Max: %(max)s") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
| 120| 120| 				option.min !== undefined && option.max !== undefined ?
| 121| 121| 					translateWithContext("option number", "Min: %(min)s, Max: %(max)s") :
| 122| 122| 				option.min !== undefined && option.max === undefined ?
| 123|    |-					translateWithContext("option number", "Min: %(min)s") :
|    | 123|+						translateWithContext("option number", "Min: %(min)s") :
| 124| 124| 				option.min === undefined && option.max !== undefined ?
| 125| 125| 					translateWithContext("option number", "Max: %(max)s") :
| 126| 126| 					"",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 4.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
| 121| 121| 					translateWithContext("option number", "Min: %(min)s, Max: %(max)s") :
| 122| 122| 				option.min !== undefined && option.max === undefined ?
| 123| 123| 					translateWithContext("option number", "Min: %(min)s") :
| 124|    |-				option.min === undefined && option.max !== undefined ?
|    | 124|+						option.min === undefined && option.max !== undefined ?
| 125| 125| 					translateWithContext("option number", "Max: %(max)s") :
| 126| 126| 					"",
| 127| 127| 				{
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
| 122| 122| 				option.min !== undefined && option.max === undefined ?
| 123| 123| 					translateWithContext("option number", "Min: %(min)s") :
| 124| 124| 				option.min === undefined && option.max !== undefined ?
| 125|    |-					translateWithContext("option number", "Max: %(max)s") :
|    | 125|+							translateWithContext("option number", "Max: %(max)s") :
| 126| 126| 					"",
| 127| 127| 				{
| 128| 128| 					"min": option.min,
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/options/options.js
| 123| 123| 					translateWithContext("option number", "Min: %(min)s") :
| 124| 124| 				option.min === undefined && option.max !== undefined ?
| 125| 125| 					translateWithContext("option number", "Max: %(max)s") :
| 126|    |-					"",
|    | 126|+							"",
| 127| 127| 				{
| 128| 128| 					"min": option.min,
| 129| 129| 					"max": option.max

binaries/data/mods/public/gui/options/options.js
| 235| »   »   »   let·value·=·optionType.guiToValue(control);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'value' is already declared in the upper scope.

binaries/data/mods/public/gui/session/session.js
| 733| »   »   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
| 734| »   »   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
| 735| »   »   button.onpressright·=·(function(i)·{·return·function()·{·performGroup("breakUp",·i);·};·})(i);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/messages.js
| 403| 403| 	let notificationText =
| 404| 404| 		notification.instructions.reduce((instructions, item) =>
| 405| 405| 			instructions + (typeof item == "string" ? translate(item) : colorizeHotkey(translate(item.text), item.hotkey)),
| 406|    |-			"");
|    | 406|+		"");
| 407| 407| 
| 408| 408| 	Engine.GetGUIObjectByName("tutorialText").caption = g_TutorialMessages.concat(setStringTags(notificationText, g_TutorialNewMessageTags)).join("\n");
| 409| 409| 	g_TutorialMessages.push(notificationText);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/DiplomacyColors.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/DiplomacyColors.js
| 108| 108| 				// Players see colors depending on diplomacy
| 109| 109| 				this.displayedPlayerColors[i] =
| 110| 110| 					g_ViewedPlayer == i ? this.getDiplomacyColor("self") :
| 111|    |-					g_Players[g_ViewedPlayer].isAlly[i] ? this.getDiplomacyColor("ally") :
|    | 111|+						g_Players[g_ViewedPlayer].isAlly[i] ? this.getDiplomacyColor("ally") :
| 112| 112| 					g_Players[g_ViewedPlayer].isNeutral[i] ? this.getDiplomacyColor("neutral") :
| 113| 113| 					this.getDiplomacyColor("enemy");
| 114| 114| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/DiplomacyColors.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/DiplomacyColors.js
| 109| 109| 				this.displayedPlayerColors[i] =
| 110| 110| 					g_ViewedPlayer == i ? this.getDiplomacyColor("self") :
| 111| 111| 					g_Players[g_ViewedPlayer].isAlly[i] ? this.getDiplomacyColor("ally") :
| 112|    |-					g_Players[g_ViewedPlayer].isNeutral[i] ? this.getDiplomacyColor("neutral") :
|    | 112|+							g_Players[g_ViewedPlayer].isNeutral[i] ? this.getDiplomacyColor("neutral") :
| 113| 113| 					this.getDiplomacyColor("enemy");
| 114| 114| 
| 115| 115| 		this.displayedPlayerColors[0] = g_Players[0].color;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 5.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/DiplomacyColors.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/DiplomacyColors.js
| 110| 110| 					g_ViewedPlayer == i ? this.getDiplomacyColor("self") :
| 111| 111| 					g_Players[g_ViewedPlayer].isAlly[i] ? this.getDiplomacyColor("ally") :
| 112| 112| 					g_Players[g_ViewedPlayer].isNeutral[i] ? this.getDiplomacyColor("neutral") :
| 113|    |-					this.getDiplomacyColor("enemy");
|    | 113|+								this.getDiplomacyColor("enemy");
| 114| 114| 
| 115| 115| 		this.displayedPlayerColors[0] = g_Players[0].color;
| 116| 116| 	}
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1003/display/redirect

elexis edited the test plan for this revision. (Show Details)Oct 26 2019, 8:30 PM
elexis edited the summary of this revision. (Show Details)Oct 26 2019, 8:35 PM
Stan added a subscriber: Stan.Oct 26 2019, 9:36 PM
Stan added inline comments.
binaries/data/mods/public/gui/common/OverlayCounterFPS.js
22 ↗(On Diff #10206)

What's the point of that variable ? Can't we pass the value directly each time ?

binaries/data/mods/public/gui/common/OverlayCounterManager.js
81 ↗(On Diff #10206)

.join('\n') too slow ?

binaries/data/mods/public/gui/common/OverlayCounterRealtime.js
13 ↗(On Diff #10206)

Guess we are forced to create a new object each time ?

binaries/data/mods/public/gui/session/OverlayCounterElapsedTime.js
33 ↗(On Diff #10206)

I think there is a special ASCII multiplication x used in D1707

Freagarach added inline comments.
binaries/data/mods/public/gui/session/OverlayCounterElapsedTime.js
33 ↗(On Diff #10206)

× Not ASCII I guess ;)

elexis added inline comments.Oct 27 2019, 1:30 PM
binaries/data/mods/public/gui/common/OverlayCounterFPS.js
22 ↗(On Diff #10206)

You either have read more or less than me, in the first case read the comment of the line, in the second case I would argue that I don't want to change strings that I touch, but that this optimization may be used globally sometime (never passing objects to sprintf but use %s everywhere) https://github.com/alexei/sprintf.js

binaries/data/mods/public/gui/common/OverlayCounterManager.js
81 ↗(On Diff #10206)

unnecessary array creation every call, createArray.map.filter.join works elegantly well where performance doesnt matter, but it was used in many places in the session (tooltips) where performance is at least not negligible, so its an antipattern to be avoided. .reduce can do something similar but usually looks ugly in comparison without leverage

binaries/data/mods/public/gui/common/OverlayCounterRealtime.js
13 ↗(On Diff #10206)

Strictly youre correct that one can squeeze out some CPU cycles here too (still useless here following the 250ms delay but in principle whatever)

let foo = Engine.GetMicroseconds();

for (let i = 0; i < 5000; ++i)
{
	let boo = (new Date()).toLocaleTimeString();
}
warn(Engine.GetMicroseconds() - foo);

warn("Type 2");

foo = Engine.GetMicroseconds();
var date = (new Date());
for (let i = 0; i < 5000; ++i)
{
	date.setTime(Date.now())
	let boo = date.toLocaleTimeString();
}
warn(Engine.GetMicroseconds() - foo);

Result:

WARNING: 18345
WARNING: Type 2
WARNING: 15886

The test is not complete because theres also the this member lookup, whatever + thanks

binaries/data/mods/public/gui/session/OverlayCounterElapsedTime.js
33 ↗(On Diff #10206)

(If there is a reason to not conserve the strings, then that reason should be expressed)

This revision was not accepted when it landed; it landed in state Needs Review.Oct 27 2019, 1:39 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Oct 27 2019, 1:39 PM
Stan added inline comments.Oct 27 2019, 3:36 PM
binaries/data/mods/public/gui/common/OverlayCounterFPS.js
22 ↗(On Diff #10206)

Fair enough. I got the point of not recreating objects, which is what that comment says, so here it's just a variable assignation, I was merely wondering if member access would have been slower that directly passing the function call. The second part of your comment adresses that. Thanks for the time and patience with me.

binaries/data/mods/public/gui/common/OverlayCounterManager.js
81 ↗(On Diff #10206)

Thanks for the explanation. In this case maybe a standard for would have been even faster.

binaries/data/mods/public/gui/common/OverlayCounterRealtime.js
13 ↗(On Diff #10206)

No pb.

binaries/data/mods/public/gui/session/OverlayCounterElapsedTime.js
33 ↗(On Diff #10206)