Page MenuHomeWildfire Games

Refactor diplomacy dialog, diplomacy colors, tribute buttons, spy request buttons, attack request buttons, minimap panel, idle worker button, ceasefire counter to use object orientation paradigm
ClosedPublic

Authored by elexis on Thu, Oct 10, 3:56 PM.

Details

Summary

In the course of #5387, this moves the diplomacy dialog components and related code from global procedures from menu.js to a class design in separate files and folders.

This does:

  • further defragmetation, group all code relating to one GUI object in one class in an according file
  • makes the different code components more agnostic of each other

The purpose of this is:

  • to increae extensibility for modders and future authors:
    • since new classes can be inserted with little hooks into existing classes (whereas previously one would be forced to overwrite various unrelated core functions),
    • since the new functions are shorter and prototype properties, the diffs to overwrite or extend them become shorter,
    • since many fixed values such as translated strings, size numbers, math constants can become modifiable properties of the prototype instead of hardcoded in the functions; without introducing more 'evil globals' (citation from removed code, see also Coding_Conventions).
  • to improve readability. While there is little complexity added to propagate messages, one can now read one file at a time, focus on that, and if needing more context checking for parent classes, becoming able to ignore everything else.

An example of this is the spy request or diplomacy colors button, which are logically very different from the other features added to the diplomacy screen that shouldn't be intertwined with diplomacy logic.

Another example of the benefit of the localization are the TributeButtons which now can hold the state of the current amount without having to rest to even more global closure hoisting hackery, thus solving D1191 without making it worse.

The patch is conservative with regards to features and translated strings (very few strings have changed).
Minor changes while at it:

  • input.js massbarter state was removed, it looks like it was only added to create the on-released event (which is more recent (rP19028) than the button), compare with the masstribute implementation
  • Fixes D1191.
  • SpyButton tooltip simplification
  • Makes the diplomacy dialog resizing from rP22970 more friendly (https://en.wikipedia.org/wiki/Separation_of_concerns)

While reviewing (viewing again?) my patch, I noticed a cmd.source variable problem in the SpyRequestButton and AttackRequestButton from rP16533, rP19247.
Since that changes petra and simulation code, it should be fixed separately.

Test Plan

The performance question I suppose.

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.Thu, Oct 10, 3:56 PM

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

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

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

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

nani awarded a token.Thu, Oct 10, 7:16 PM

On performance, it should not matter too much for this patch, since none of the classes do reoccurring updates on their own, and the only way they are called reoccurringly is through updateGUIObjects() which only calls g_DiplomacyDialog.update();, g_MiniMapPanel.update();.
The first call is early-returning if the dialog is closed, so doesnt affect unless the dialog is opened. If the dialog is opened, then the other dialogs are closed. It's few more function calls if it is opened which sounds negligible.
On the other side there is also less code performed in the update functions, since some of the attributes are only set once on init that previously were done each update call.

I measured updateGUIObjects of session.js and updatePanels of DiplomacyDialog.js on 8 player FFA mainland normal sized map.
Selecting the starting units makes great impact on the performance, more than one order of magnitude in performance loss.
If nothing is selected, updateGUIObjects takes about 2ms on my computer for that setup, if the dialog is opened, it consumes one more 1ms.
Given that a turn length of 100ms is envisioned, that sounds meh.

WARNING: updateGUIObjects 4801
WARNING: updateGUIObjects 2726
WARNING: updateGUIObjects 2617
WARNING: updateGUIObjects 2516
WARNING: updateGUIObjects 2220
WARNING: updateGUIObjects 2834
WARNING: updateGUIObjects 2347
WARNING: updateGUIObjects 2320
WARNING: updateGUIObjects 2230
WARNING: updateGUIObjects 2476
WARNING: updateGUIObjects 3092
WARNING: updateGUIObjects 2128
WARNING: updateGUIObjects 1955
WARNING: updateGUIObjects 1882
WARNING: updateGUIObjects 1956
WARNING: updateGUIObjects 2195
WARNING: updateGUIObjects 1964
WARNING: updateGUIObjects 1915
WARNING: updateGUIObjects 2186
WARNING: updateGUIObjects 1954
WARNING: updateGUIObjects 2091
WARNING: updateGUIObjects 1828
WARNING: updateGUIObjects 1888
WARNING: updateGUIObjects 1893
WARNING: updateGUIObjects 127926
WARNING: updateGUIObjects 12764
WARNING: updateGUIObjects 12444
WARNING: updateGUIObjects 10270
WARNING: updateGUIObjects 10271
WARNING: updateGUIObjects 12229
WARNING: updateGUIObjects 10904
WARNING: updateGUIObjects 1835
WARNING: updateGUIObjects 2095
WARNING: updateGUIObjects 2299
WARNING: updateGUIObjects 2116
WARNING: updateGUIObjects 1865
WARNING: updateGUIObjects 1923
WARNING: updateGUIObjects 1808
WARNING: updateGUIObjects 1832
WARNING: updateGUIObjects 1786
WARNING: updateGUIObjects 1984
WARNING: updateGUIObjects 13112
WARNING: updateGUIObjects 10122
WARNING: updateGUIObjects 10334
WARNING: updateGUIObjects 10607
WARNING: updateGUIObjects 10089
WARNING: updateGUIObjects 9906
WARNING: updateGUIObjects 11208
WARNING: updateGUIObjects 10999
WARNING: updateGUIObjects 9809
WARNING: updateGUIObjects 9313
WARNING: updateGUIObjects 9732
WARNING: updateGUIObjects 8721
WARNING: updateGUIObjects 9422
WARNING: updateGUIObjects 8975
WARNING: updateGUIObjects 11082
WARNING: updatePanels 3282
WARNING: updatePanels 1741
WARNING: updateGUIObjects 11602
WARNING: updatePanels 1127
WARNING: updateGUIObjects 10417
WARNING: updatePanels 1079
WARNING: updateGUIObjects 10268
WARNING: updatePanels 1121
WARNING: updateGUIObjects 10516
WARNING: updatePanels 1267
WARNING: updateGUIObjects 10617
WARNING: updatePanels 1105
WARNING: updateGUIObjects 10553
WARNING: updatePanels 1079
WARNING: updateGUIObjects 10841
WARNING: updatePanels 1262
WARNING: updateGUIObjects 11616
WARNING: updatePanels 1121
WARNING: updateGUIObjects 12091
WARNING: updatePanels 1232
WARNING: updateGUIObjects 23998
WARNING: updatePanels 1191
WARNING: updateGUIObjects 10440
WARNING: updatePanels 1092
WARNING: updateGUIObjects 10637
WARNING: updatePanels 1077
WARNING: updateGUIObjects 10646
WARNING: updatePanels 1409
WARNING: updateGUIObjects 10558
WARNING: updatePanels 1155
WARNING: updateGUIObjects 10430
WARNING: updatePanels 1020
WARNING: updateGUIObjects 10409
WARNING: updatePanels 1010
WARNING: updateGUIObjects 9954
WARNING: updatePanels 975
WARNING: updateGUIObjects 10060
WARNING: updatePanels 1876
WARNING: updateGUIObjects 10946
WARNING: updatePanels 2607
WARNING: updateGUIObjects 12151
WARNING: updatePanels 1455
WARNING: updateGUIObjects 10861
WARNING: updatePanels 1051
WARNING: updateGUIObjects 11559
WARNING: updatePanels 975
WARNING: updateGUIObjects 10320
WARNING: updatePanels 964
WARNING: updateGUIObjects 10033
WARNING: updatePanels 1421
WARNING: updateGUIObjects 10384
WARNING: updatePanels 1113
WARNING: updateGUIObjects 15826
WARNING: updatePanels 1038
WARNING: updateGUIObjects 12833
WARNING: updatePanels 962
WARNING: updateGUIObjects 10169
WARNING: updatePanels 970
WARNING: updateGUIObjects 3577
WARNING: updatePanels 989
WARNING: updateGUIObjects 3226
WARNING: updatePanels 1047
WARNING: updateGUIObjects 3141
WARNING: updatePanels 1026
WARNING: updateGUIObjects 2892
WARNING: updatePanels 987
WARNING: updateGUIObjects 2850
WARNING: updatePanels 965
WARNING: updateGUIObjects 3366
WARNING: updatePanels 1002
WARNING: updateGUIObjects 2976
WARNING: updatePanels 974
WARNING: updateGUIObjects 2819
WARNING: updatePanels 976
WARNING: updateGUIObjects 2872
WARNING: updatePanels 949
WARNING: updateGUIObjects 2815
WARNING: updatePanels 955
WARNING: updateGUIObjects 2853
WARNING: updateGUIObjects 1793
WARNING: updateGUIObjects 1700
WARNING: updateGUIObjects 1781
WARNING: updateGUIObjects 1787
WARNING: updateGUIObjects 2361
WARNING: updateGUIObjects 1830
WARNING: updateGUIObjects 1764
WARNING: updateGUIObjects 1862
WARNING: updateGUIObjects 2006
WARNING: updateGUIObjects 2010
WARNING: updateGUIObjects 1813
WARNING: updateGUIObjects 10477
WARNING: updateGUIObjects 9564
WARNING: updateGUIObjects 9500
WARNING: updateGUIObjects 11958
WARNING: updateGUIObjects 8889
WARNING: updateGUIObjects 12129
WARNING: updateGUIObjects 8413
WARNING: updateGUIObjects 1723
WARNING: updateGUIObjects 1857
WARNING: updateGUIObjects 1739
WARNING: updateGUIObjects 1896
WARNING: updateGUIObjects 10544
WARNING: updateGUIObjects 8629
WARNING: updateGUIObjects 9369
WARNING: updateGUIObjects 8663
WARNING: updateGUIObjects 8283
WARNING: updateGUIObjects 9597
WARNING: updateGUIObjects 8723
WARNING: updateGUIObjects 16725
WARNING: updateGUIObjects 9343
WARNING: updateGUIObjects 7739
WARNING: updatePanels 1177
WARNING: updatePanels 1144
WARNING: updateGUIObjects 9789
WARNING: updatePanels 1045
WARNING: updateGUIObjects 10692
WARNING: updatePanels 1285
WARNING: updateGUIObjects 9474
WARNING: updatePanels 919
WARNING: updateGUIObjects 8912
WARNING: updatePanels 899
WARNING: updateGUIObjects 8692
WARNING: updatePanels 893
WARNING: updateGUIObjects 9261
WARNING: updatePanels 897
WARNING: updateGUIObjects 8728
WARNING: updatePanels 907
WARNING: updateGUIObjects 8705
WARNING: updatePanels 964
WARNING: updateGUIObjects 9076
WARNING: updatePanels 1095
WARNING: updateGUIObjects 9548
WARNING: updatePanels 1370
WARNING: updateGUIObjects 10092
WARNING: updatePanels 920
WARNING: updateGUIObjects 9165
WARNING: updatePanels 1144
WARNING: updateGUIObjects 9472
WARNING: updatePanels 880
WARNING: updateGUIObjects 10082
WARNING: updatePanels 890
WARNING: updateGUIObjects 9298
WARNING: updatePanels 866
WARNING: updateGUIObjects 8783

A second look:

WARNING: diplomacyDialogCeasefireCounter 5
WARNING: diplomacyDialogColorsButton 50
WARNING: diplomacyDialogPlayerControlManager 970
WARNING: updateGUIObjects 2923

A third:

WARNING: updateGUIObjects 3594
WARNING: AttackRequestButton 1 11
WARNING: AttackRequestButton 2 11
WARNING: AttackRequestButton 3 8
WARNING: AttackRequestButton 4 7
WARNING: AttackRequestButton 5 11
WARNING: AttackRequestButton 6 9
WARNING: AttackRequestButton 7 8
WARNING: AttackRequestButton 8 7
WARNING: DiplomacyPlayerText 1 39
WARNING: DiplomacyPlayerText 2 27
WARNING: DiplomacyPlayerText 3 24
WARNING: DiplomacyPlayerText 4 23
WARNING: DiplomacyPlayerText 5 24
WARNING: DiplomacyPlayerText 6 23
WARNING: DiplomacyPlayerText 7 23
WARNING: DiplomacyPlayerText 8 24
WARNING: SpyRequestButton 1 8
WARNING: SpyRequestButton 2 13
WARNING: SpyRequestButton 3 7
WARNING: SpyRequestButton 4 8
WARNING: SpyRequestButton 5 75
WARNING: SpyRequestButton 6 41
WARNING: SpyRequestButton 7 38
WARNING: SpyRequestButton 8 36
WARNING: StanceButtonManager 1 14
WARNING: StanceButtonManager 2 33
WARNING: StanceButtonManager 3 26
WARNING: StanceButtonManager 4 26
WARNING: StanceButtonManager 5 25
WARNING: StanceButtonManager 6 26
WARNING: StanceButtonManager 7 25
WARNING: StanceButtonManager 8 25
WARNING: TributeButtonManager 1 25
WARNING: TributeButtonManager 2 29
WARNING: TributeButtonManager 3 28
WARNING: TributeButtonManager 4 28
WARNING: TributeButtonManager 5 28
WARNING: TributeButtonManager 6 27
WARNING: TributeButtonManager 7 27
WARNING: TributeButtonManager 8 27
WARNING: updateGUIObjects 3443
WARNING: AttackRequestButton 1 12
WARNING: AttackRequestButton 2 12
WARNING: AttackRequestButton 3 8
WARNING: AttackRequestButton 4 7
WARNING: AttackRequestButton 5 12
WARNING: AttackRequestButton 6 8
WARNING: AttackRequestButton 7 8
WARNING: AttackRequestButton 8 9
WARNING: DiplomacyPlayerText 1 40
WARNING: DiplomacyPlayerText 2 27
WARNING: DiplomacyPlayerText 3 24
WARNING: DiplomacyPlayerText 4 23
WARNING: DiplomacyPlayerText 5 24
WARNING: DiplomacyPlayerText 6 22
WARNING: DiplomacyPlayerText 7 24
WARNING: DiplomacyPlayerText 8 22
WARNING: SpyRequestButton 1 7
WARNING: SpyRequestButton 2 13
WARNING: SpyRequestButton 3 8
WARNING: SpyRequestButton 4 6
WARNING: SpyRequestButton 5 75
WARNING: SpyRequestButton 6 41
WARNING: SpyRequestButton 7 39
WARNING: SpyRequestButton 8 36
WARNING: StanceButtonManager 1 14
WARNING: StanceButtonManager 2 33
WARNING: StanceButtonManager 3 26
WARNING: StanceButtonManager 4 25
WARNING: StanceButtonManager 5 24
WARNING: StanceButtonManager 6 25
WARNING: StanceButtonManager 7 25
WARNING: StanceButtonManager 8 25
WARNING: TributeButtonManager 1 25
WARNING: TributeButtonManager 2 28
WARNING: TributeButtonManager 3 28
WARNING: TributeButtonManager 4 27
WARNING: TributeButtonManager 5 27
WARNING: TributeButtonManager 6 27
WARNING: TributeButtonManager 7 27
WARNING: TributeButtonManager 8 26

While working on that code, I had noticed that in many cases the value of a GUI Object setting doesn't change, yet it is reassigned.

For example .hidden = state. So I wonder if that shouldn't be changed in the C++ code to an early return. Otherwise it broadcasts a SettingsChanged message to every GUI object recursively.

Another performance improvement would be to update less setting values, only when they are needed.

The current code is written for correct functionality. Every time the sim state changes, the elements will be refreshed from any state that is simulation state dependent.

For example the spybutton is updated each simulation update, but IsTechnologyResearched events could also be listened instead of that GUIInterface call each simstate update.

We see in the last measurement that building the SpyButton tooltip text takes sometimes 10x more than early returning in case the button is hidden.

So it seems those two described performance bottlenecks (reassign same value, doing work more often than needed) seem most damning.

Notice that the translate calls are also not anymore fixed in anticipation of the user being able to change settings (including language) during the game. Then we don't want the GUI to show 50% of the previously selected language and 50% of the new language.

Same goes for hotkeys that can be changed during the game and thus should be updated everytime the config values are updated (but not every updateGUIObjects, while this is currently still implemented due to lack of that event being present. If the event will be added, then it can be refined).

Another thing I see is controlsPlayer calls that could be cached / passed as arg, in particular controlsPlayer(g_ViewedPlayer).

So I don't see the point of saving fractions of microseconds if we already know that the code that could be optimized (translate, hotkey only dependent code) would have to be changed yet again once the hotkey / language change feature is added when we have bottlenecks that are multiple orders of magnitude more relevant.

A last measurement is taken from a23 (git clone, not release bundle) for function updateDiplomacy:

WARNING: 17290
WARNING: 4740
WARNING: 3972
WARNING: 3799
WARNING: 3709
WARNING: 4065
WARNING: 4118
WARNING: 4813
WARNING: 5253
WARNING: 3675
WARNING: 4185
WARNING: 3860
WARNING: 3600
WARNING: 3642
WARNING: 3670
WARNING: 3639
WARNING: 3410
WARNING: 4263
WARNING: 3502
WARNING: 3558
WARNING: 3487
WARNING: 3347
WARNING: 3418
WARNING: 3360
WARNING: 3175
WARNING: 4300
WARNING: 3104

So it seems twice as fast as in a23, which might be due to the source/gui/ rewrites or not.

Added classes:

DiplomacyButton
DiplomacyDialog
DiplomacyDialogCeasefireCounter
DiplomacyDialogColorsButton
DiplomacyDialogPlayerControl
DiplomacyDialogPlayerControlManager
Minimap
MiniMapDiplomacyColorsButton
MiniMapIdleWorkerButton
MiniMapPanel
AttackRequestButton
DiplomacyPlayerText
SpyRequestButton
StanceButtonManager
StanceButton
TributeButtonManager
TributeButton

Added / removed global variables:

+var g_DiplomacyButton;
+var g_DiplomacyColors;
+var g_DiplomacyDialog;
+var g_MiniMapPanel;

-var g_BribeButtonsWaiting = {};
-var g_DiplomacyColorsToggle = false;
-var g_DisplayedPlayerColors;
-var g_FlushTributing = function() {};
-var g_IsDiplomacyOpen = false;
-const INPUT_MASSTRIBUTING = 10;

(g_DiplomacyButton might become a member of a future g_TopPanel)

Removed global functions:

-function handleMinimapEvent(target)
-function resizeDiplomacyDialog()
-function openDiplomacy()
-function closeDiplomacy()
-function toggleDiplomacy()
-function updateDiplomacy(opening = false)
-function diplomacySetupTexts(i, rowsize)
-function diplomacyFormatStanceButtons(i, hidden)
-function diplomacyFormatTributeButtons(i, hidden)
-function diplomacyFormatAttackRequestButton(i, hidden)
-function diplomacyFormatSpyRequestButton(i, hidden)
-function formatTributeTooltip(playerID, resourceCode, amount)
-function getPlayerHighlightColor(player)
-function updateDiplomacyColorsButton()
-function updateDisplayedPlayerColors()
-function updateIdleWorkerButton()
elexis retitled this revision from Rewrite diplomacy dialog, tribute buttons, diplomacy colors, tribute buttons, spy request buttons, attack request buttons, minimap panel, idle worker button to use object orientation paradigm to Refactor diplomacy dialog, diplomacy colors, tribute buttons, spy request buttons, attack request buttons, minimap panel, idle worker button, ceasefire counter to use object orientation paradigm.Fri, Oct 11, 2:10 PM
elexis edited the summary of this revision. (Show Details)Fri, Oct 11, 2:14 PM
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Oct 11, 2:31 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.Fri, Oct 11, 2:31 PM

Now a measurement of the diplomacy panel update function before and after the patch (which I couldnt really do due to new folders and moved files):

Before the commit:

before the commit:
WARNING: 2789
WARNING: 3825
WARNING: 3318
WARNING: 3846
WARNING: 3239
WARNING: 2999
WARNING: 2907
WARNING: 2936
WARNING: 2813
WARNING: 2834
WARNING: 3262
WARNING: 2886
WARNING: 2816
WARNING: 2894
WARNING: 2868
WARNING: 3038
WARNING: 2840
WARNING: 2765
WARNING: 90020
WARNING: 2874
WARNING: 2942
WARNING: 3046
WARNING: 2834
WARNING: 2757
WARNING: 2840
WARNING: 2741
WARNING: 2795
WARNING: 2748
WARNING: 2765
WARNING: 3053
WARNING: 2776
WARNING: 2791
WARNING: 2723
WARNING: 2736
WARNING: 2751
WARNING: 2798
WARNING: 2737
WARNING: 2821
WARNING: 2879
WARNING: 2705
WARNING: 2820
WARNING: 2706
WARNING: 2835
WARNING: 2760
WARNING: 2760
WARNING: 2745
WARNING: 2721
WARNING: 2754
WARNING: 2670
WARNING: 2670
WARNING: 2773
WARNING: 2783
WARNING: 2795
WARNING: 2508
WARNING: 10437
WARNING: 2885
WARNING: 2505
WARNING: 2497
WARNING: 2437
WARNING: 2666
WARNING: 2381
WARNING: 2464
WARNING: 2802
WARNING: 2559
WARNING: 2467
WARNING: 2446
WARNING: 2446
WARNING: 2571
WARNING: 2415
WARNING: 2399
WARNING: 2517
WARNING: 2395
WARNING: 2383
WARNING: 2519
WARNING: 2417
WARNING: 2415
WARNING: 2391
WARNING: 2410
WARNING: 2450
WARNING: 2448
WARNING: 2570
WARNING: 2406
WARNING: 2397
WARNING: 2389
WARNING: 2410
WARNING: 2402
WARNING: 2412
WARNING: 2400
WARNING: 2420
WARNING: 2371
WARNING: 2409
WARNING: 2439
WARNING: 2615
WARNING: 2580
WARNING: 2398
WARNING: 7652
WARNING: 7068
WARNING: 4806
WARNING: 4016
WARNING: 3782
WARNING: 3725
WARNING: 4196
WARNING: 4398
WARNING: 4552
WARNING: 4143
WARNING: 3781
WARNING: 3558
WARNING: 3547
WARNING: 3886
WARNING: 3558
WARNING: 4358
WARNING: 3728
WARNING: 3375
WARNING: 3134
WARNING: 3315
WARNING: 3184
WARNING: 3321
WARNING: 3225
WARNING: 3350
WARNING: 3133
WARNING: 3801
WARNING: 3240
WARNING: 3237
WARNING: 3133
WARNING: 3007
WARNING: 3128
WARNING: 3018
WARNING: 2986
WARNING: 3199
WARNING: 3139
WARNING: 4936
WARNING: 4080
WARNING: 3198
WARNING: 3189
WARNING: 3017
WARNING: 2919
WARNING: 2868
WARNING: 2980
WARNING: 2835
WARNING: 2883
WARNING: 2903
WARNING: 2947
WARNING: 2902
WARNING: 3030
WARNING: 2902
WARNING: 2936
WARNING: 3050
WARNING: 2972
WARNING: 2891

After the commit:

after the commit:
WARNING: 3233 < first open
WARNING: 1985
WARNING: 1502
WARNING: 1248
WARNING: 1280
WARNING: 1249
WARNING: 1206
WARNING: 1230
WARNING: 1247
WARNING: 1226
WARNING: 1377
WARNING: 1206
WARNING: 1250
WARNING: 1206
WARNING: 1286
WARNING: 1353
WARNING: 1400
WARNING: 1278
WARNING: 2839 < selectionchange
WARNING: 1316
WARNING: 1434
WARNING: 1104
WARNING: 1280
WARNING: 1191
WARNING: 1088
WARNING: 1609
WARNING: 1657
WARNING: 1080
WARNING: 1142

which sounds nice, but perhaps still terrible because it could be faster.