Page MenuHomeWildfire Games

Scroll through loading screen tips
Needs ReviewPublic

Authored by Stan on Jan 4 2019, 12:12 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#2087
Summary
Test Plan
  • Test that everything works properly and there are no issues in multiplayer.

Event Timeline

Stan created this revision.Jan 4 2019, 12:12 PM
Owners added a subscriber: Restricted Owners Package.Jan 4 2019, 12:12 PM
Vulcan added a subscriber: Vulcan.Jan 4 2019, 12:14 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/901/

smiley added a subscriber: smiley.Jan 4 2019, 1:08 PM

This should be exculsive to single-player.

elexis added a subscriber: elexis.Jan 4 2019, 1:17 PM

Feature idea okay in principle, implementation mostly okay, art needs update.
Didn't we see this patch some years ago on trac before?

Judging from the screenshot in your link, (other than the size difference and the alignment), the button sprites don't fit so well to the scroll image IMO.
Perhaps one could use an arrow left and arrow right icon that uses a similar palette to the image of the scroll (or otherwise fit better to the scroll)?

ps/trunk/binaries/data/config/default.cfg
241 ↗(On Diff #7235)

Hotkeys okay;
intro.txt

367 ↗(On Diff #7235)

if kept, add description

ps/trunk/binaries/data/mods/public/gui/loading/loading.js
1 ↗(On Diff #7235)

(Insert generic procedural mess vs OOP by design and merge conflict rant)

39 ↗(On Diff #7235)

If Vulcan didn't say anything about it, we need to update the linter.

74 ↗(On Diff #7235)

delete if anything

93 ↗(On Diff #7235)

(really?)

129 ↗(On Diff #7235)

(OT refactoring: If there are no contents to display, it could just not display the scrolls thing and the rest. Some games that use the pyrogenesis-gui mod might not want to show errors if they don't want to use this feature, so handling it gracefully has advantages.)

ps/trunk/binaries/data/mods/public/gui/loading/loading.xml
65 ↗(On Diff #7235)

Perhaps "Start", dunno

ps/trunk/binaries/data/mods/public/gui/options/options.json
90 ↗(On Diff #7235)

If it's implemented in an unobtrusive way, there is probably no need to have it optional?
Or is it only to have that "Play" button optional for SP, as some people want to read more before playing and others want to play more before reading in SP?

Stan updated this revision to Diff 7238.EditedJan 4 2019, 1:38 PM
  • Play → Start
  • Fix the cursor when the tips button are not enabled
  • Remove the config option
  • Do not unpause the game if it wasn't paused before
  • Fix the multiplayer behavior.
  • Fix the patch relative paths.
  • Remove commented out code.
  • Add the line in the intro.txt (Should not forget to update @wiki:Hotkeys)
Vulcan added a comment.Jan 4 2019, 2:15 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 4 spaces.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
| 109| 109| 
| 110| 110| function nextTip()
| 111| 111| {
| 112|    |-    updateTip((g_TipIndex + 1) % g_Tips.length);
|    | 112|+	updateTip((g_TipIndex + 1) % g_Tips.length);
| 113| 113| }
| 114| 114| 
| 115| 115| function prevTip()
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 4 spaces.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
| 114| 114| 
| 115| 115| function prevTip()
| 116| 116| {
| 117|    |-    updateTip((g_Tips.length + g_TipIndex - 1) % g_Tips.length);
|    | 117|+	updateTip((g_Tips.length + g_TipIndex - 1) % g_Tips.length);
| 118| 118| }
| 119| 119| 
| 120| 120| 
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
| 118| 118| }
| 119| 119| 
| 120| 120| 
| 121|    |-function updateTip(nextTipIndex) 
|    | 121|+function updateTip(nextTipIndex)
| 122| 122| {
| 123| 123| 	g_TipIndex = nextTipIndex;
| 124| 124| 	let tipFile = g_Tips[g_TipIndex];

binaries/data/mods/public/gui/loading/loading.js
|  42| »   »   switch·(data.attribs.mapType)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/902/

Stan updated this revision to Diff 7241.Jan 4 2019, 4:26 PM
  • Fix the Lint warnings.
Vulcan added a comment.Jan 4 2019, 4:35 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/905/

Stan updated this revision to Diff 7242.Jan 4 2019, 4:48 PM
Vulcan added a comment.Jan 4 2019, 4:48 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/906/

Stan updated this revision to Diff 7243.Jan 4 2019, 5:00 PM

crossing fingers.

Vulcan added a comment.Jan 4 2019, 5:00 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/907/

smiley added inline comments.Jan 4 2019, 5:19 PM
binaries/data/mods/public/gui/loading/loading.js
13–14

If this were to be used, what would be displayed here?
Map script logs perhaps. But that won't work for skirmish/scenarios.
Anyway, no need to keep it around for the time being.

Stan marked an inline comment as done.Jan 4 2019, 5:22 PM
Stan added inline comments.
binaries/data/mods/public/gui/loading/loading.js
13–14

I believe RMS messages, like loading trees or whatever.

smiley added a comment.Jan 4 2019, 5:23 PM
witch (data.attribs.mapType)
 		{
+		case "random":
+			loadingMapName.caption = sprintf(translate("Generating ΓÇ£%(map)sΓÇ¥"), { "map": mapName });
+			break;
+		
 		case "skirmish":
 		case "scenario":
+		default:
 			loadingMapName.caption = sprintf(translate("Loading ΓÇ£%(map)sΓÇ¥"), { "map": mapName });
 			break;
-
-		case "random":
-			loadingMapName.caption = sprintf(translate("Generating ΓÇ£%(map)sΓÇ¥"), { "map": mapName });
-			break;
 		}

From the raw diff. That doesn't look good.

Stan updated this revision to Diff 7245.Jan 4 2019, 5:24 PM

No context patch. Fix a typo.

Vulcan added a comment.Jan 4 2019, 5:24 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/908/

Stan updated this revision to Diff 7246.Jan 4 2019, 5:28 PM
Vulcan added a comment.Jan 4 2019, 6:03 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/loading/loading.js
|  44|  44| 		case "random":
|  45|  45| 			loadingMapName.caption = sprintf(translate("Generating “%(map)s”"), { "map": mapName });
|  46|  46| 			break;
|  47|    |-		
|    |  47|+
|  48|  48| 		case "skirmish":
|  49|  49| 		case "scenario":
|  50|  50| 		default:

binaries/data/mods/public/gui/loading/loading.js
|  49| »   »   case·"scenario":
|    | [NORMAL] JSHintBear:
|    | Expected a 'break' statement before 'default'.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/909/

smiley added inline comments.Jan 8 2019, 1:13 PM
binaries/data/mods/public/gui/loading/loading.js
14
if (data.attribs.mapType == "random")
    loadingMapName.caption = sprintf(translate("Generating “%(map)s”"), { "map": mapName });
else
    loadingMapName.caption = sprintf(translate("Loading “%(map)s”"), { "map": mapName });

Is cleaner than a switch case statement IMO.

elexis added inline comments.Jan 8 2019, 1:54 PM
binaries/data/mods/public/gui/loading/loading.js
14

could remove duplication, and then remove duplication

Stan marked an inline comment as done.Jan 8 2019, 2:12 PM
Stan added inline comments.
binaries/data/mods/public/gui/loading/loading.js
14

Ternary would work there too.

loadingMapName.caption = sprintf(translate(data.attribs.maptype == random ? "Generating" : "Loading" "“%(map)s”"), { "map": mapName });

elexis added a comment.Jan 8 2019, 3:06 PM

It would probably be easy to use a generic arrow and transparency?

binaries/data/mods/public/gui/loading/loading.js
14

string extracting python script expects literal strings inside translate(), otherwise yes, possibly different whitespace

elexis retitled this revision from Improving the Loading Screen to Scroll through loading screen tips.Jan 8 2019, 3:08 PM
Stan added a comment.Jan 8 2019, 3:24 PM

For arrows sure. I used buttons because that was what was there in the beginning

Polakrity added inline comments.
binaries/data/config/default.cfg
245

Comments.

Most shortcuts have comments.

binaries/data/mods/public/gui/loading/loading.js
21–22

Engine.ResetCursor(); ?

23

Same objects names.

binaries/data/mods/public/gui/manual/intro.txt
30

Uppercases: Arrow - Switch

Point at the end of the sentence.

wraitii requested changes to this revision.May 28 2019, 5:37 PM
wraitii added a subscriber: wraitii.

@Stan I'm relatively sure this actually starts the game in the background and you don't notice because you haven't switched to the session screen? Could be weird if a player spends 5 minutes reading tips only to realised that the AI has a 5 minute head start.

Also the fact that "ReallyStartGame" doesn't really start the game now is super ugly. The fact that it calls "StartGame" is even worst (I guess that's "ReallyReallyStartGame" :p ).
TBH I don't see an easy way around that, I think you'd need to set a global variable that the c++ has access to and checks before calling reallyStartGame or something to give the player the hand.

binaries/data/mods/public/gui/loading/loading.js
13–14

Why is this being reordered?

21

Why is this being moved inside the if?

23

This is super ugly also, use a global variable for g_TipsEnabled instead.

This revision now requires changes to proceed.May 28 2019, 5:37 PM
Stan added a comment.May 28 2019, 5:40 PM

@Stan I'm relatively sure this actually starts the game in the background and you don't notice because you haven't switched to the session screen? Could be weird if a player spends 5 minutes reading tips only to realised that the AI has a 5 minute head start.

It does, that's why it pauses the game.

Also the fact that "ReallyStartGame" doesn't really start the game now is super ugly. The fact that it calls "StartGame" is even worst (I guess that's "ReallyReallyStartGame" :p ).
TBH I don't see an easy way around that, I think you'd need to set a global variable that the c++ has access to and checks before calling reallyStartGame or something to give the player the hand.

Yeah, that aside, the UI side of things is as ugly as the code.

Stan updated this revision to Diff 11827.May 10 2020, 12:55 PM
Stan marked 7 inline comments as done.

Preliminary rebase. Still to solve, camera bug, start button position.

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

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

Some thoughts:

  • You might want to reset the cursor when loading has finished? It looks strange to have the hourglass when it's not loading anymore.
  • Perhaps split the patch into the start button after loading and scrolling the tips? You don't really *need* the delay in starting the game when scrolling the tips (and it should be allowed in MP as well). And people might want an option to disable waiting.

@Stan what happened to this (any update)?

Grapjas added a subscriber: Grapjas.Jul 2 2021, 6:03 PM

I believe this is still useful

Stan marked an inline comment as done.Thu, Jan 13, 10:49 PM
Stan added a subscriber: Langbart.

Maybe @Grapjas or @Langbart want to take it on?