Page MenuHomeWildfire Games

Scroll through loading screen tips
Needs ReviewPublic

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

Details

Reviewers
None
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.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 6702
Build 11025: Vulcan BuildJenkins

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
76

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
76

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
44
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
44

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
44

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
44

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