Page MenuHomeWildfire Games

New tutorial based on triggers.
ClosedPublic

Authored by mimo on Mar 5 2017, 12:11 PM.

Details

Summary

We currently have no working tutorials, and that is something which is really needed by new players. So I've made a simple trigger based tutorial model, which can in principle be easily extended to cover more aspects of the game, even by someone without much coding experience. As i think that would be nice to have it in next alpha, it includes a simple interface to gamesetup (which could be changed when the campaign manager is done).
This is still wip, but as i didn't get much feedback on the preversion on the team forum, let's start discussing it here.
It seems that phabricator has troubles to move binaries files, so the two map files starting_economy_walkthrough.pmp and .xml are still in the maps/scenarios folder, but they will be moved to maps/tutorials when committing.

Test Plan

the tutorial is accessible from the main menu, in the learn to play submenu.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 689
Build 1093: Vulcan BuildJenkins
Build 1092: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Mar 6 2017, 6:37 PM
binaries/data/mods/public/maps/scenarios/starting_economy_walkthrough.js
3 ↗(On Diff #725)

teach you how to

9 ↗(On Diff #725)

"Alt+Enter" -> Sounds like we should add an interface for the colorizeHotkey GUI function, so that it's not hardcoded and highlighted too. Can be done in a separate, maybe simple ticket

12 ↗(On Diff #725)

"Females have the higher food gather rates"
-> "Females have higher food gathering rates on fields and berries than men"

"Selected units have their selection ring highlighted"
-> "Selected units are highlighted by a ring in the player color" maybe

16 ↗(On Diff #725)

This should check that the player followed the actually given instruction and didn't order some other unit to gather the specific resource

39 ↗(On Diff #725)

First batch of 5 should be more females for food.
Then if there are huntable animals around, those should be gathered first with some newly trained cavalry.
The second batch of 5 should be the hoplites. Otherwise we don't have enough food income to reproduce quickly enough.
Might also want to mention the batch train count option.

79 ↗(On Diff #725)

near -> close

96 ↗(On Diff #725)

switch back > automatically switch to? (they dont go back to work but start gathering resources)

129 ↗(On Diff #725)

should be shortened, it's a bit tldr and players shouldn't have to scroll to read the entire instruction ideally.
The queueing hint can be a separate instruction.

empty space? rightclick?

138 ↗(On Diff #725)

Then -> Now
Infinite -> unlimited
"But to minimize the path done by our farmers to bring back the food to a dropsite" -> "To maximize the efficiency of our farmers, we need to minimize the distance from the farm to the nearest dropsite"

The TLDRness can be helped by colorizing some keywords like the buildings to construct.
Or even better splitting this into one hint that can be continued by clicking on a ready button and then doing the actual instruction in a separate step

additional gatherers is -> are

second hotkey that might use the colorizeHotkey thing

The grabbing sentence should be rephrased.

Hints that are not an instruction could be shown in a separate color, or the actual instruction could become bold.

Having a warning in each instruction would be neat.

control-clicking -> pressing the control key

149 ↗(On Diff #725)

This seems to assume that the house is not finished yet before the farmstead is, but this might not be the case at all and the pupil might be stuck here not knowing what to do. Also having more idlers somewhere else means it won't continue.

155 ↗(On Diff #725)

near -> close

179 ↗(On Diff #725)

on the -> on an unfinished

207 ↗(On Diff #725)

"what their costs and effects are"
Doesnt actually state that you have to research something

215 ↗(On Diff #725)

"We now have enough resources", "one structure is missing" assumes that the player didn't do something other than the instructions.
Maybe we can state that assumption explicitly to circumvent potential contradictions

mimo added a comment.Mar 6 2017, 9:03 PM

thanks for the detailed comments, i will implement some but not those modifying the content of the tutorial because:

  • the main goal of the patch was to provide a framework, which can be easily extended to cover different aspects of the game. I fully encourage people to either improve it or add new tutorials when this framework is commited.
  • this tutorial was designed to be used by complete beginners, with a goal of learning the basics of the commands, learning how to gather resources, build a field, setup a rallypoint, ... and not at all that they would need several farms to sustain a pop of 300 or other such "more advanced" tips. That's for a next tutorial that i'd be happy to see contributers.

My main goal now is to fix the serialization, and then to have it committed with the current functionnalities (to allow other people to contribute :-).
In addition, to overcome the limitations of phabricator, i propose to already create the maps/tutorials folder in svn, and to move there the pmp and xml files which are currently in maps/scenarios. That would allow to have the real setup for review, if there are no opposition on this folder structure.
Possibly if we could decide what we want to do when several tutos are available (would it be a sub-sub-menu in the "learn-to-play"-"tutorial"-"list-of-tutorials" or should we add a panel in the gamesetup instead of switching immediately to the only existing tutorial as is done now), i could add it to this patch, but better deal with it in a next one.

elexis added a comment.Mar 6 2017, 9:31 PM

My main goal now is to fix the serialization, and then to have it committed with the current functionnalities

At least move the selectMap and launchGame call to a separate function launchTutorial, call it onInit instead of onTick and disable perist match settings if isTutorial, as it crashes when having a random map script selected before.
Trying to keep a limit of 4 sentences per instruction would help significantly.

maps/tutorials/

implying that we can only have atlas map tutorials? I guess that assumption holds. And the maptype dropdown is a filter too, so it doesn't inherently conflict with the gamesetup.

Some English suggestions. :)

binaries/data/mods/public/maps/scenarios/starting_economy_walkthrough.js
3 ↗(On Diff #725)

civil -> civic
make "them" develop is a tad ambiguous. Perhaps "make your empire develop".
Its selection ring should be -> Its selection ring will be (that is, unless we expect it to be buggy :D)

6 ↗(On Diff #725)

"civil" -> "civic"
"possibilities" -> "options" (possibilities feels less in control)
pass the mouse over, more natural is "hover the mouse over" x2
"The top line display the units which" -> "The top line displays the units that" (and also which -> that in second half feel more natural as well)
"icon of the bottom line" -> Perhaps indicate this by icon? "icon II: the tooltip will indicated that [we have to build...]"

9 ↗(On Diff #725)

The second sentences sounds a little convoluted. Perhaps "But first, you can toggle between full screen and windowed mode using [HOTKEY]."
Also, "confortable" -> "comfortable", and it doesn't explicitly say which one is more comfortable (and we probably shouldn't), so perhaps we could change it to "Pick the setting that you feel most comfortable with."

12 ↗(On Diff #725)

"rates, thus select" -> "rates; thus, select"
"grab them" has weird connotations xP. perhaps "select them all at once"
"asking" -> "tasking" or "ordering"
"and click" -> "and you can click"

We should probably mention that the left click is mostly for selection while right click is mostly for orders.

39 ↗(On Diff #725)

"set a rally-point[...]" -> "set a rally-point to automatically designate a task upon completion of training"
"gathering wood" -> "to gather wood"
"bunch" -> "group"
"in the south" -> "to the south"
"civil" -> "civic"

binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js
7

"Hover your mouse over" seems more natural than "pass the mouse over".

scythetwirler added inline comments.Mar 6 2017, 11:21 PM
binaries/data/mods/public/maps/scenarios/starting_economy_walkthrough.js
21 ↗(On Diff #725)

"Let's now" -> "Now, let's"
";" -> "."
"tree," -> "tree;"
"tree cursor" -> "wood cursor"

30 ↗(On Diff #725)

Second sentence can be omitted.
"chiken" -> "chicken"

46 ↗(On Diff #725)

"civil" -> "civic"
"pass" -> "hover"
"Right" -> "right"

54 ↗(On Diff #725)

"setup" -> "set up"
"soldiers" -> "citizen soldiers" to be more precise.
"wood gathering" -> "gathering wood"
"civil" -> "civic"
"allows to produce" -> "produces"

72 ↗(On Diff #725)

"top panel" -> more precise "panel at the top of your screen"
"you see there[...]" -> "There, you can see your current resource counts. As each worker brings resources back to the civic center (or another dropsite), you will see the amount of the corresponding resource increase."
"That's an" -> "This is a very"
"and your" -> "and you"
"minimize the distance" -> "minimize the distance between resource and nearest dropsite"

79 ↗(On Diff #725)

Should detail which resources the storehouse acts as a dropsite for.
To be more clear, elexis's comment refers to the near in "move the mouse as near", not the first occurrence near, which is fine. :)
"soldiers and look" -> "soldiers, look" and "then click" -> "click"
"are indicated in red" -> "will show the building preview overlay in red"

96 ↗(On Diff #725)

"And when" -> "When"
agree with elexis's comment.
"civil" -> "civic"
"females workers" -> "female workers"
"female icon" -> "female citizen icon" :P

mimo updated this revision to Diff 740.Mar 8 2017, 7:41 PM

updated version with fixes from elexis and scythetwirler comments, and support of serialization

Vulcan added a comment.Mar 8 2017, 8:48 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/486/ for more details.

Other sections still need some English proofing (which I will do later if I have time)

binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js
31

chiken -> chicken.
when you see a meat cursor -> "Your cursor will turn into a meat icon when you hover over a chicken.

mimo updated this revision to Diff 791.Mar 13 2017, 10:15 PM

With corrections from scythetwirler

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/516/ for more details.

elexis requested changes to this revision.Apr 25 2017, 2:25 PM

Quite certain that the Serialize and Deserialize function of the Trigger component breaks serialization of other trigger scripts which extend the same Trigger component. Just not adding those 2 functions to implicitly serialize/deserialize all properties of the prototype should be sufficient, unless we have one property that may not be serialized under any circumstance

binaries/data/mods/public/gui/gamesetup/gamesetup.js
273

!!foo.bar

binaries/data/mods/public/gui/pregame/mainmenu.xml
7

(What) is this inclusion needed (for)?

binaries/data/mods/public/gui/session/messages.js
539

The colors should become globals g_TutorialColorWarning and g_TutorialColorText so that if we want to change the color, we only have to change one place without investigating whether we fixed all occurances. Nice side effect that mods can change it from other files

544

If this is tutorial interface is going to be used by developers and players, someone will eventually attempt to set a color and break it.
Why not prevent the breakage (instead of adding code and documentation warnings) and support adding colors?

When receiving a new tutorial message, the caption could just show
g_TutorialMessages.join("\n") + "\n" + '[color="' + g_TutorialTextColor + '"]' + notification.message + "[/color] and then append it using
g_TutorialMessages.push(notification.message).

(Could also use a string and append it to it, but might be handy in the future to do something with individual notifications at prior times)

548

If used more than once:
let tutorialWarning = Engine.GetGUIObjectByName("tutorialReady");
let tutorialWarning = Engine.GetGUIObjectByName("tutorialWarning");

Replacing an existing button caption and onPress often results in issues, for example the launchGame button in the gamesetup which is also used as a ready button.
Might be better to have a second button that already has the correct properties and then just hiding / showing them here.

binaries/data/mods/public/gui/session/tutorial_panel.xml
13

Might be nice to have the caption and onPress all in the same place in JS instead of 50% in the XML and 50% in JS

binaries/data/mods/public/maps/scripts/Tutorial.js
2

This will break once another map, victory condition or other trigger script adds a events property. Not the fault of this patch but perhaps tutorialEvents, tutorialIndex, tutorialCount works too for the time being.
We will have to do something about this sometime ( https://wildfiregames.com/forum/index.php?/topic/22179-trigger-prototype-naming-conflicts/ )

Why isn't tutorialEvents serialized?

4

/** \n
Capital letter, call->called?, potentially period

5

This actually sounds like it would break serialization, since the other Trigger scripts also extend the Trigger prototype, so all of those properties will be lost.

Why implement custom serialize/deserialize functions? It is sufficient to omit these functions and let the default serialization serialize all properties as far as I know (could be tested by starting an MP game and rejoining that and observing whether we get an OOS error, or pyrogenesis -replay="/path/to/replay/commands.txt" -mod=public -rejointest=someNumber doing the same)

30

(potentially unneeded braces)

This revision now requires changes to proceed.Apr 25 2017, 2:25 PM
mimo added a comment.Apr 27 2017, 7:56 PM
In D194#15547, @elexis wrote:

Quite certain that the Serialize and Deserialize function of the Trigger component breaks serialization of other trigger scripts which extend the same Trigger component. Just not adding those 2 functions to implicitly serialize/deserialize all properties of the prototype should be sufficient, unless we have one property that may not be serialized under any circumstance

Why would you need to load any of the other trigger scripts in such a tutorial? the problem is that we need to call the startTutorial function.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
273

and you find it more readable than what is written? i don't want to loose time arguing when you include such a thing in your own patches, but i will never write it myself!

binaries/data/mods/public/gui/pregame/mainmenu.xml
7

Don't remember. Maybe a remnant of a previous change. I'll check it if still needed.

binaries/data/mods/public/gui/session/messages.js
539

I think having a working tutorial is the most important feature missing in 0ad, and that it should be included, tested and then improved (not the other way). That would allow everybody being able to modify such things which for the moment i really don't care compared to the goal of having such a tutorial easily accessible from the learn to play menu (for me that would be the most important feature for A22 to help new players), and which certainly will change when people try and use it.

548

i'm not convinced that's better.

binaries/data/mods/public/gui/session/tutorial_panel.xml
13

I think it is fine to have invariable properties in xml, and variable ones in js.

binaries/data/mods/public/maps/scripts/Tutorial.js
2

I don't see why you would need any victory conditions in such a tutorial which is just to learn the basic commands, but ok. I will change to tutorialXXX

4

ok

5

Better test it before saying so. It was tested and does not break anything because you don't (and should never) need to load other trigger scripts.
This Deserialize call is needed because it has to call the StartTutorial function. But if there is a better (and tested as what you propose would not work) approach, i'd be fine with it.

30

I prefer having such brace as otherwise, the code may break in the future when people add one line.
Imo omitting such braces should be restricted to cases where the following block is only a one line block.

leper added inline comments.Apr 28 2017, 2:01 AM
binaries/data/mods/public/maps/scripts/Tutorial.js
5

Isn't the call to StartTutorial only needed because events isn't serialized, which is caused by it being in the prototype as opposed to a normal member?

binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js
273

Same here, why is this part of the prototype instead of just being a normal member?

mimo updated this revision to Diff 1515.EditedApr 28 2017, 9:14 PM
mimo edited edge metadata.

Updated version with changes on serialization and revised tutorial text from scythetwirler

To remove the need for the StartTutorial call when deserializing, i've now exposed the Deserialized messages to JS so that the trigger script can now react to them.

The goals can not be serialized as they contain functions, but with the new approach, it's not a problem anymore.

Finally, scythetwirler has reviewed (and improved) all the tutorial strings (thanks).

Owners added a subscriber: Restricted Owners Package.Apr 28 2017, 9:14 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/910/ for more details.

To remove the need for the StartTutorial call when deserializing, i've now exposed the Deserialized messages to JS so that the trigger script can now react to them.

The goals can not be serialized as they contain functions, but with the new approach, it's not a problem anymore.

I guess one could rewrite those in a way that the functions don't need to be serialized, but the rest of the state is.
However the current way does seem like less work and clean enough.

elexis requested changes to this revision.May 11 2017, 1:45 PM

Keeping trigger script support means tutorials for victory conditions or tutorials that comes with an own trigger script scheduling some events remains possible.
For the non-serialization remarks I can upload a patch after the commit.

Indeed by executing the Init function after the default deserialization has finished is a feasible way to keep trigger script support. The C++ change appears correct.

functions don't need to be serialized, but the rest of the state is.

We don't support serializing functions (for a good reason, otherwise the NetServer could send players arbitrary JS code to execute, or people could distribute savegames containing code).
This also means when trying to save the tutorial, we get this crash:

ERROR: Cannot serialise JS objects of type 'function': (unnamed)
terminate called after throwing an instance of 'PSERROR_Serialize_InvalidScriptValue'
  what():  Serialize_InvalidScriptValue

Since the tutorial functions are already stored in a global and since the functions never change, we can store the current index of the tutorial array and replace the trigger property with that according function.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
273

!! returns a boolean, this && returns undefined or true

Returns expr1 if it can be converted to false; otherwise, returns expr2.

https://wildfiregames.com/forum/index.php?/topic/22251-coding-conventions-updates/

1110

Here the rebased version

1403

(Removed thoughts about merging this with D11)

binaries/data/mods/public/gui/session/messages.js
539

I think having a working tutorial is the most important feature missing in 0ad

Agree that we should have it in Alpha 22 to give people a good place to start before they are lost when starting the first games.

it should be included, tested and then improved (not the other way)

I don't mind if it's reviewed or not as long as defects are worked out

binaries/data/mods/public/gui/session/tutorial_panel.xml
13

(These are not invariable since they are changed in JS)

binaries/data/mods/public/maps/scripts/Tutorial.js
57

Should this be this.OnPlayerCommandTrigger?

binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.js
277

(Note to myself: D195)

binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.xml
562 ↗(On Diff #1515)

(Did that chicken misbehave?)

This revision now requires changes to proceed.May 11 2017, 1:45 PM
mimo added a comment.May 15 2017, 8:01 PM
In D194#19163, @elexis wrote:

Keeping trigger script support means tutorials for victory conditions or tutorials that comes with an own trigger script scheduling some events remains possible.
For the non-serialization remarks I can upload a patch after the commit.

But i think such tutorial which would need additional scripts like victory conditions or whatever will have to provide their own script to do complicated things. This one was only a basic script with LINEAR execution to allow everybody (even non programmer) to do easily their own tutorial, not something that has to be used by all tutorials as it was not meant to do fancy things. But i've already said that several time here, and i'd be happy to leave this ticket to anybody wanting to add anything to this tutorial as i don't want to continue such discussions which have make me loose motivations for this ticket, but would find it a pity if we still have another release without tutorial for new comers.

btw is there a way to abandon ownership of a phabricator ticket without abondonning the ticket? i could not find it.

binaries/data/mods/public/maps/scripts/Tutorial.js
57

no as we can't serialize it

binaries/data/mods/public/maps/tutorials/starting_economy_walkthrough.xml
562 ↗(On Diff #1515)

yep, was inaccessible

elexis accepted this revision.May 15 2017, 10:38 PM
This revision was automatically updated to reflect the committed changes.