Page MenuHomeWildfire Games

Small mainmenu cleaup: Add MainMenuPage, BackgroundLayer class, remove MainMenuItemHandler hardcoding
ClosedPublic

Authored by elexis on Oct 26 2019, 1:01 PM.

Details

Summary

While working with the main menu again (FPS counter, refs D2389/rP23091) I noticed some small improvements that could be applied to the code from rP22854/D2240, in particular before the code is extended.

  1. MainMenuPage class: As learnt in D2310/rP22985 after having moved everything into classes, there will be one umbrella class remaining owning everything else (hierarchy).

Hence this diff implements that for the main menu page. This has the advantage that if there were global procedures introduced, they'd not be mixed in with class definitions, discouraging their implementation in preference over class architecture.

  1. The BackgroundHandler class does two acts per background layer instance. That logically qualifies the introduction of a class that does these two things (BackgroundLayer). In particular this has the advantage that the Engine.GetGUIObject call is (always) in the constructor, which seems to be a benefitial pattern and makes the code also slightly faster. Notice that the previous code also defined functions inside the functions that are now just members. This also seems to be a pattern (if there are local functions, its an indication one may consider adding a new class to avoid redefintion of the local). The tiny classes are now owned too, merely for consistency (and hypothetical extensibility/moddability).
  1. Marginal performance improvement of the BackgroundHandler. Amongst not obtaining the GUIObject reference every frame anymore, the getComputedSize() call only needs to be done when resizing the window. (I tried avoiding new GUISize call too but failed to, since we may not modify the parent size cache)
  1. constructor(menuItems, menuSpeed = 1.2, margin = 4, buttonHeight = 28) still is a hardcoding and out of place. As the last 7 weeks past that commit showed, the prototype chain is a good place to store such values. Notice the disadvantage of the prototype as a storage facility is that its slower than a member of the instance (two steps to traverse instead of one), but that doesn't justify less modifiable code.
  1. bind: As Krinkle had recommended in the revision proposal bind is a nice feature, and it was used often in later commits, so this makes it consistent.
  1. "100%-2 " + this.submenu.size.bottom + " 100% 100%"; is bad hardcoding. As learnt in the past months, size values are best stated in XML and only in XML. This diff only modifies the relevant property of the size and reuses XML stated values otherwise, thus removing almost all hardcoded literals.
  1. Some global references were replaced by passing the references through the constructor.
  1. onTick assignment moved from XML to JS. I thought that didn't work, but it does. Leaves XML free of JS.
Test Plan

Verify that the resluts presented to the player are the same. In particular there may not be any glitches (teleported sprites) while resizing the window.
Notice that the ButtonHeight and Margin are freely chosable.
I performed a measurement of the BackgroundHandler onTick function and it is marginally faster (seems like a nothingbomb but at least theres the evidence it didnt get worse).

average time in microseconds spent per BackgroundHandler onTick call without applying the patch:
45.2

average time in microseconds spent per BackgroundHandler onTick call after applying the patch:
39.7

That's 13% or something, which is probably entirely useless in comparison to the work the renderer has to perform. Also I dont recall measuring the perf diff in rP22854, whatever.

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, 1:01 PM

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

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

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

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

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|   6| »   »   »   new·BackgroundLayer(layer,·i));
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'BackgroundLayer' was used before it was defined.

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|  30| class·BackgroundLayer
|    | [NORMAL] JSHintBear:
|    | 'BackgroundLayer' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  11| »   »   this.musicHandler·=·new·MusicHandler();
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  12| »   »   this.projectInformationHandler·=·new·ProjectInformationHandler(projectInformation);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  13| »   »   this.communityButtonHandler·=·new·CommunityButtonHandler(communityButtons);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'CommunityButtonHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  24| class·MusicHandler
|    | [NORMAL] JSHintBear:
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  33| class·ProjectInformationHandler
|    | [NORMAL] JSHintBear:
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  43| class·CommunityButtonHandler
|    | [NORMAL] JSHintBear:
|    | 'CommunityButtonHandler' was used before it was defined.
Executing section cli...

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

elexis edited the test plan for this revision. (Show Details)Oct 26 2019, 1:28 PM
In D2390#99676, @Vulcan wrote:

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

Linter detected issues:
Executing section Source...
Executing section JS...
binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|   6| »   »   »   new·BackgroundLayer(layer,·i));
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'BackgroundLayer' was used before it was defined.
binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|  30| class·BackgroundLayer
|    | [NORMAL] JSHintBear:
|    | 'BackgroundLayer' was used before it was defined.
binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  11| »   »   this.musicHandler·=·new·MusicHandler();
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'MusicHandler' was used before it was defined.
binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  12| »   »   this.projectInformationHandler·=·new·ProjectInformationHandler(projectInformation);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'ProjectInformationHandler' was used before it was defined.
binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  13| »   »   this.communityButtonHandler·=·new·CommunityButtonHandler(communityButtons);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'CommunityButtonHandler' was used before it was defined.
binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  24| class·MusicHandler
|    | [NORMAL] JSHintBear:
|    | 'MusicHandler' was used before it was defined.
binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  33| class·ProjectInformationHandler
|    | [NORMAL] JSHintBear:
|    | 'ProjectInformationHandler' was used before it was defined.
binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  43| class·CommunityButtonHandler
|    | [NORMAL] JSHintBear:
|    | 'CommunityButtonHandler' was used before it was defined.
Executing section cli...

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

https://eslint.org/docs/rules/no-use-before-define will be impossible to fulfil in any case. Notice we get these warnings only in this diff since its about the only time where there is more than one class in a file. The files are loaded in alphabetic order, so there won't be any guarantee in which they can be loaded unless we use subdirectories or the alphabetic sorting. Sorting these classes so that they are defined before some function refers to them would be counterintuitive, because the reader expects some sort of top down hierarchy.
For the same reason we don't do this for functions either, first we have the init function in GUI pages and then the lower tier helper functions. In simulation components we have the Init, Deserialize, Serialize ones and then the helper functions. So I don't feel compelled to change the order of class definitions so that helper functions appear prior to the root class. Finding myself arguing with a bot -_-

Stan added a subscriber: Stan.Oct 26 2019, 1:59 PM
Stan added inline comments.
binaries/data/mods/public/gui/pregame/BackgroundHandler.js
72 ↗(On Diff #10202)

This is still not a good idea to hardcode that (though it's not related to that patch directly)

  1. Because this should be computed and/or cached and updated on window resize.
  2. Because the minimum resolution we claim to support is 1024/768 and that's a 4:3 ratio
binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
58 ↗(On Diff #10202)

For of could work here ?

elexis updated this revision to Diff 10203.Oct 26 2019, 2:03 PM
elexis edited the test plan for this revision. (Show Details)

Fix missing mainmenu.xml onTick removal and missing set MainMenuPanelRightBorderBottom rbottom to 100% in closed and to 0 in openSubmenu.

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

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

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

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

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  11| »   »   this.musicHandler·=·new·MusicHandler();
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  12| »   »   this.projectInformationHandler·=·new·ProjectInformationHandler(projectInformation);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  13| »   »   this.communityButtonHandler·=·new·CommunityButtonHandler(communityButtons);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'CommunityButtonHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  24| class·MusicHandler
|    | [NORMAL] JSHintBear:
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  33| class·ProjectInformationHandler
|    | [NORMAL] JSHintBear:
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  43| class·CommunityButtonHandler
|    | [NORMAL] JSHintBear:
|    | 'CommunityButtonHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|   6| »   »   »   new·BackgroundLayer(layer,·i));
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'BackgroundLayer' was used before it was defined.

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|  30| class·BackgroundLayer
|    | [NORMAL] JSHintBear:
|    | 'BackgroundLayer' was used before it was defined.
Executing section cli...

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

elexis added inline comments.Oct 26 2019, 2:17 PM
binaries/data/mods/public/gui/pregame/BackgroundHandler.js
72 ↗(On Diff #10202)

This diff is not supposed to change the art. I will reply to these statements in D1680.

elexis added inline comments.Oct 26 2019, 2:20 PM
binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
58 ↗(On Diff #10202)

Indeed, this must be remanent of a previous iteration where I used an object instead of an array, so it's misleading to use name when elsewhere it's i.

elexis updated this revision to Diff 10204.EditedOct 26 2019, 2:33 PM

for name in -> for item of

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

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

elexis updated the Trac tickets for this revision.Oct 26 2019, 2:37 PM

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

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

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  11| »   »   this.musicHandler·=·new·MusicHandler();
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  12| »   »   this.projectInformationHandler·=·new·ProjectInformationHandler(projectInformation);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  13| »   »   this.communityButtonHandler·=·new·CommunityButtonHandler(communityButtons);
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'CommunityButtonHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  24| class·MusicHandler
|    | [NORMAL] JSHintBear:
|    | 'MusicHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  33| class·ProjectInformationHandler
|    | [NORMAL] JSHintBear:
|    | 'ProjectInformationHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/MainMenuPage.js
|  43| class·CommunityButtonHandler
|    | [NORMAL] JSHintBear:
|    | 'CommunityButtonHandler' was used before it was defined.

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|   6| »   »   »   new·BackgroundLayer(layer,·i));
|    | [MAJOR] ESLintBear (no-use-before-define):
|    | 'BackgroundLayer' was used before it was defined.

binaries/data/mods/public/gui/pregame/BackgroundHandler.js
|  30| class·BackgroundLayer
|    | [NORMAL] JSHintBear:
|    | 'BackgroundLayer' was used before it was defined.
Executing section cli...

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

elexis added inline comments.Oct 26 2019, 2:54 PM
binaries/data/mods/public/gui/pregame/MainMenuItemHandler.js
58 ↗(On Diff #10202)

Actually using for of is wrong, try it and you will see, the reference will be to the last item iterated

binaries/data/mods/public/gui/pregame/menupanel.xml
15 ↗(On Diff #10204)

(obsolete/unnecessary change as of 2nd upload)

onTick hack for the splashscreenhandler missing, this code is a freakin mess, it should stop calling itself onTick too if I go to such lengths to save some cycles, but thats not implemented, see other TODO // TODO: support actually deleting the handler in session.js and this onTick being present since years and that also only being the case due to some weird hotloading crash which is present since probably introduction of the splashscreenhandler at least. But its impossible for me to take care of this simultaneously. So it has to be deferred and this will get its ugly hack permuted.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 26 2019, 3:32 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Oct 26 2019, 3:32 PM