"Play Tutorial" button

[Roboron]

A while ago, I asked how could we give more exposure to the scenario tutorial, and we settled on adding a button to the initial banner (https://forum.simutrans.com/index.php/topic,21878.0/viewresults.html)

This patch is the implementation.

It adds a "Play Tutorial" button next to the "New Game" button. This means every other button is displaced one position, and the exit button now is in the third row.



The button does the same that "Load Scenario" -> select scenario tutorial would do, it's simply a shortcut. Of course, for this to work the scenario tutorial directory needs to be defined by pakset creators. The new parameter "pakset_tutorial_dir" in simuconf.tab takes care of this.

If no tutorial is defined, or the tutorial defined is not found, the button will be disabled. Additionally, a tooltip will appear redirecting players to Pak128 ("Scenario Tutorial is not available for this pakset. Please try loading Pak128.")



tutorial_button.diff

[Matthew]

It's sad to spoil the symmetry but spotlighting the tutorial is definitely worth it! I especially like the redirection to pak128.

[Yona-TYT]

And if it is better to use a dialog window that asks: Do you want to start the tutorial?,  It will only appear the first time we start simutrans. 
This is how many games do it.


By the way, this is great, since pak64, pak128 and pak192.comic have their tutorial scenario.

[prissi]

I think a popup if you start with the default map might be a good idea too. And then a play tutorial button in the load game etc. dialoge.

[Roboron]

I think a popup if you start with the default map might be a good idea too.

Currently, the initial banner stop the player from interacting with pop-ups. And we may overwhelm players a little with so many windows. But we can change the initial flow, I guess. I suggest the following:

1. Show Tutorial pop-up (if scenario tutorial is present).
2a. Click "Play Tutorial" and load tutorial.
2b. Click "Dismiss" and show the banner.

We can also add a "don't show again" checkbox so it does not bother the player again.

[Yona-TYT]

I think the pop-up window should appear when you press the [New Game] button, it will appear along with the "Create new game" window the first time you start the game.

A checkbox for "Do not show again" is also a good idea.

[ceeac]

I'm not really a fan of the pop-up window approach - users in general are already trained to dismiss these without thinking twice or might mis-click and select the wrong choice. Additionally, I think we also have to take into account "over-confident" first-time players who realize they need a tutorial after starting their first game. So, I believe the best way is to make the "Play Tutorial" button always available and in the same spot every time like in the first post.

It's sad to spoil the symmetry but spotlighting the tutorial is definitely worth it!

Maybe it's a good idea to move the buttons to be vertically aligned on the left-hand side with a divider separating them from the rest of the window?

[Flemmbrav]

I'm also team "no popup". It should either be on the new map menu, or the starting menu. There's really no need for yet another dialogue, as these two already feel very redundant.

[prissi]

With the feedback, I feel like the new map window might be indeed a better place for the button.

Also we have the "load game" button at least twice, in the banner and the new game window. Same for "quit". Maybe just three buttons at the bottom in the new world dialogue:
"Start Game" "Start Scenaria" (instead "load scenario") "Start Tutorial".

[ceeac]

How about we use the banner window as the main menu? Something like this (The new buttons are not yet functional):


This way, we can remove the Load Game, Load scenario etc. buttons from the New Game window and also from the Options window. Both windows could then have a button to go back to the Main Menu; same for the Load Game and Save Game windows. The button to start the tutorial could then either be in the Main Menu or in the New Game window (but personally I'd prefer the former).

[Roboron]

I like ceeac's suggestion. I would also suggest to move the "Install" button from Options to Main Menu, I feel it makes more sense there.

The Options window would be simplified to have only the relevant functionality for the current game.

[prissi]

The banner is a model dialog and will disconnect you from online games. Also on first startup, continue game is not a sensible choice.

Also when playing, I use options quite often, especially the day and night modus and some display options.

In order to not confuse old users too much, I think that:

• the banner should come up after clicking "New Game" in the settings windows as now
• remove "Load Game", "Scenario", and maybe also "Install" from the option window (but then in must appear on the banner or no new pakset can be installed on Android, since the pak selection dialog will not show up if only a single pakset has been installed)
  Quit should be stay there, at least I quit Simutrans on Android there ...
  
• remove "Load Game", "Load Scenario" from the new world window, either remove "Quit" or change it to "continue game" (essentially the same as closing the window)
• add "Install" and "Tutorial" and maybe a "continue game" button to the banner and show the banner each time on startup (Alternatively to continue game, just close the banner when clicking outside or pressing a key as it is handled in almost any other game?)

[Roboron]

Also on first startup, continue game is not a sensible choice.

Right. Should be disabled in such a case.

In order to not confuse old users too much, I think that the banner should come up after clicking "New Game" in the settings windows as now

On the other hand, I think that having two buttons with the same name that do different things is... certainly confusing.

Quit should be stay there, at least I quit Simutrans on Android there ...

If we add the "Quit" button to the banner/main menu then the flow would be to press "Return to main menu" -> "Quit". But I don't have a strong opinion on this, I don't care if we keep both buttons. At least they will do the same!

Alternatively to continue game, just close the banner when clicking outside or pressing a key as it is handled in almost any other game?

[Roboron]

I followed ceaac's banner redesign, and tried to improve it.

Large theme


Small theme


- Optimized the space used to show the first information.
- Gave it a bit more space to breathe.
- Changed the color of the highlighted message. I could barely read the orange with the default theme.
- Moved version information to the bottom-right.
- The banner now can be dismissed by clicking outside of it or by pressing any key (is any key ok or should we listen only to Escape?). This was achieved by a new parameter "dismissible" when creating a modal dialog.
- Also added the banner texts to base.tab for translation.


Redesigned the options window as well, removing the extra buttons.



I grouped the options logically:
- The left column has general options.
- The right column has current game related options.


And finally removed the extra buttons from the new game dialog, and added the button to return to menu.



I also made so the load save and load scenario windows return to the banner menu when clicking "cancel" (achieved by adding a new parameter back_to_menu to saveframe_t). However you can close them through the window title bar and the banner menu will not be shown. Not sure if this is the desired behavior.


Apart from that, the other thing I have many doubts with is the "Continue" button. prissi mentioned that it is a good idea that the banner is shown each time on startup. However the reload_and_save_on_quit option assumes this will not happen and that is problematic (it will try to open the saved windows only for them to be destroyed by the banner). And if save on quit is not enabled, what to continue anyway?

Patch attached, where every button is functional, but the "Continue" button right now only closes the banner as if it were dismissed. Feel free to build upon it.

[prissi]

Thank you but I have some remarks:
Easiest would be probably to move the buttons to the right since all Simutrans dialogues have their button on the right,
Second, return to main menu from the option is not a good name. Sound rather like closing the option window and not "This will kick me out of any netowrk game and will cause panic as I suddenly have to create a new world" Thats witht he continue button. Maye rename this to "Play on". I would, at least for the option window call this "New World". I mean the option window is most frequently used for display options. Return to menu sound very inviting if you would otherwise have to press the smaller close button on a touchscreen.
To "return" to the banner after cancelling a save game, I do not know if this is good. For loading and scenario laoding, yes, that makes sense.

I see the reasoning with open dialoge windows and indeed, that is problematic. One would need that dialogue as a normal window for that to happen. Hmm

[ceeac]

My intention was for the Continue button to be a "Load most recent save/Connect to most recent server" button, but obviously the text does not fit on the button. Also, in this case there is no button to close the window; maybe the Main Menu should have a titlebar with an x button to close the window (in addition to the click outside/press key to close)?

I see the reasoning with open dialoge windows and indeed, that is problematic. One would need that dialogue as a normal window for that to happen. Hmm

Can there be more than 1 modal dialogue? If no, I think it makes sense to register the gui_frame with the event manager. This would also not lead to being kicked one out of server games if there is only one main loop.

The rest of the suggestions seem sensible or I don't have a strong opinion about them.

[Andarix]

Since they are currently working on the windows, there is still a mistake. The loading window triggers the button for the loading script tool window.

pak64.german 123.0.0.7




The loading window also closes all open windows. This should only happen when actually loading.

It is also irritating that an error message appears when you click Ok without making a selection.

[Roboron]

Easiest would be probably to move the buttons to the right since all Simutrans dialogues have their button on the right,

When buttons are on the right on dialogues they are usually Confirm/Cancel buttons. I don't really care about the position of the banner menu buttons, but if you look at other games it is normal to find the main menu buttons either at the left or at the center. Right is... unusual.

Second, return to main menu from the option is not a good name. Sound rather like closing the option window and not "This will kick me out of any netowrk game and will cause panic as I suddenly have to create a new world"

You are right, it does not state well enough what it does. "Return" is not an appropriate word. Maybe "Quit/Exit to menu" is a better wording.

Alternatively, we could show a confirmation dialog when clicking this button so the player knows it will get disconnected from the online game, and optionally prompting it to save the current game (also could be done for "Quit" button, at least if reload_and_save_on_quit is not enabled).

To "return" to the banner after cancelling a save game, I do not know if this is good. For loading and scenario laoding, yes, that makes sense.

Worry not, I only added it to loading and scenario loading.

My intention was for the Continue button to be a "Load most recent save/Connect to most recent server" button

Thanks, that makes sense. Then we would need to store the last saved game (excluding autosaves?) and last connected server (and check if it is online at start?).

The loading window triggers the button for the loading script tool window.

The loading window also closes all open windows. This should only happen when actually loading.

Fixed both of those in 10987.

It is also irritating that an error message appears when you click Ok without making a selection.

I have noticed this also. The design of the load windows is... improvable. The "Ok" button makes you believe you have to select a save, then press the button. But that's not right, when you press one of the option it automatically loads the file.

I think the root of this issue is the text input. Of course, it is needed for the save dialog, but I don't think much people use it to load... I can see the benefit when you have tons of saves and writing the name would be quicker than searching in the list.

If not removing it, I would start by moving it next to the text input, so it is clear that the Ok button is only associated to the text input. I would also disable it until text is written, to avoid pressing it by mistake.



But this would need a refactor because many classes use this dialog under the hood.

I also have another problem with this screen, it seems it only shows the "X" (delete) buttons on some paksets. I really need this to be shown always. Steam Cloud Save only allows for deletion of saves inside the game. If the user deletes saves externally, Steam restores them from the cloud :-/

[Andarix]

@Roboron

thank you

[Andarix]

...
I think the root of this issue is the text input. Of course, it is needed for the save dialog, but I don't think much people use it to load... I can see the benefit when you have tons of saves and writing the name would be quicker than searching in the list.

...

Paths are also possible in the text field, at least that's how it should be.

The list only lists the files from the save folder

[prissi]

The x in the load window can be switched on and off, depending on the settings. But you can define something that overides that setting to be always true. Also for android it may be better to be able to delete games without being developer.

One could make loading a two step process, transfer the name into the box and then click load. But I think this is bad UI. The manual input is only useful for servergames, when I have to force a connection or if the server is not on the serverlist. But apart from this, indeed, the textbox is needed only for saving.

Since this evolves in a massive overhaul, we could add the server loading function to the network window (like a checkbox: connect to mismatch) and then remove the box from all loading dialoges together with ok and cancel.

[Roboron]

The x in the load window can be switched on and off, depending on the settings. But you can define something that overides that setting to be always true. Also for android it may be better to be able to delete games without being developer.

I don't see why would be a good idea to make this optional, so I'll rather get rid of such config completely (specially because I don't want paksets messing with this). I did so in r10991.

But apart from this, indeed, the textbox is needed only for saving.

I am afraid I asked for opinions on this and there is evidence the input text is used to load savegames as well.



So, we better keep it (but change the design to my proposed one).

However, it was also suggested (and widely supported!) to instead use this input text as a search, so it can be used to filter the list of files instead (at least for loading).

It was also suggested that we add a confirmation dialog when overwriting (and I will add: deleting) savegames.

like a checkbox: connect to mismatch

👍

[ceeac]

I don't see why would be a good idea to make this optional, so I'll rather get rid of such config completely (specially because I don't want paksets messing with this). I did so in r10991.

The old value still needs to be read from old saves; this is fixed in r10992.

[ceeac]

Updated the patch to the latest revision, no functional changes.

Game crashes with an assertion failure when the tutorial button is clicked in the main menu:

Code Select Expand

simutrans: src/simutrans/world/simworld.cc:2222: void karte_t::local_set_tool(tool_t*, player_t*): Assertion `tool_in->is_init_keeps_game_state() || !init_result' failed.
I'm using the latest pak128 nightly version. The crash does not happen when the tutorial is started via the scenario window.

[Roboron]

I swear I can not reproduce the crash. Did you do something else before?

[ceeac]

No, nothing special, just compiled with CMAKE_BUILD_TYPE=Debug, started the game, pressed the tutorial button and got this backtrace:

Code Select Expand

(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=140737326714880) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140737326714880) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140737326714880, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff7455476 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff743b7f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff743b71b in __assert_fail_base (fmt=0x7ffff75f0150 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x555555b322a8 "tool_in->is_init_keeps_game_state() || !init_result", file=0x555555b31e68 "/home/ceeac/Projects/code/simutrans/src/simutrans/world/simworld.cc", line=2222, function=<optimised out>) at ./assert/assert.c:92
#6  0x00007ffff744ce96 in __GI___assert_fail (assertion=0x555555b322a8 "tool_in->is_init_keeps_game_state() || !init_result", file=0x555555b31e68 "/home/ceeac/Projects/code/simutrans/src/simutrans/world/simworld.cc", line=2222, function=0x555555b32270 "void karte_t::local_set_tool(tool_t*, player_t*)") at ./assert/assert.c:101
#7  0x0000555555a74e51 in karte_t::local_set_tool(tool_t*, player_t*) (this=0x555570663c40, tool_in=0x55555780d910, player=0x0) at /home/ceeac/Projects/code/simutrans/src/simutrans/world/simworld.cc:2222
#8  0x0000555555a74b86 in karte_t::set_tool_api(tool_t*, player_t*, bool&) (this=0x555570663c40, tool_in=0x55555780d910, player=0x0, suspended=@0x7fffffffa567: false) at /home/ceeac/Projects/code/simutrans/src/simutrans/world/simworld.cc:2197
#9  0x00005555556f4ee5 in karte_t::set_tool(tool_t*, player_t*) (this=0x555570663c40, tool_in=0x55555780d910, player=0x0) at /home/ceeac/Projects/code/simutrans/src/simutrans/player/../tool/../world/simworld.h:924
#10 0x00005555557bfcee in scenario_frame_t::load_scenario(char const*, bool) (this=0x55557070d110, fullpath=0x555558ba0230 "/media/ceeac/Projects/code/simutrans-pak128/simutrans/pak128/scenario/tutorial_pak128", is_easy_server=false) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/scenario_frame.cc:56
#11 0x00005555556f48d9 in banner_t::action_triggered(gui_action_creator_t*, value_t) (this=0x55557070cbc0, comp=0x55557070cda8) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/banner.cc:205
#12 0x000055555570ad4f in gui_action_creator_t::call_listeners(value_t) (this=0x55557070cda8, v=...) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/components/gui_action_creator.h:32
#13 0x000055555570c195 in button_t::infowin_event(event_t const*) (this=0x55557070cd88, ev=0x7fffffffa7a0) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/components/gui_button.cc:312
#14 0x0000555555712269 in gui_container_t::infowin_event(event_t const*) (this=0x5555596ef490, ev=0x7fffffffa8d0) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/components/gui_container.cc:204
#15 0x0000555555712269 in gui_container_t::infowin_event(event_t const*) (this=0x55557070d0c0, ev=0x7fffffffa950) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/components/gui_container.cc:204
#16 0x000055555577116c in gui_frame_t::infowin_event(event_t const*) (this=0x55557070cbc0, ev=0x7fffffffaa30) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/gui_frame.cc:134
#17 0x00005555556f47c1 in banner_t::infowin_event(event_t const*) (this=0x55557070cbc0, ev=0x7fffffffaa30) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/banner.cc:184
#18 0x00005555557df913 in check_pos_win(event_t*, bool) (ev=0x7fffffffab20, modal=true) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/simwin.cc:1711
#19 0x00005555557e1aa7 in modal_dialogue(gui_frame_t*, long, karte_t*, bool (*)(), bool) (gui=0x55557070cbc0, magic=-1, welt=0x555570663c40, quit=0x5555559e520c <never_quit()>, dismissible=true) at /home/ceeac/Projects/code/simutrans/src/simutrans/gui/simwin.cc:2204
#20 0x00005555559e9bb2 in simu_main(int, char**) (argc=4, argv=0x555555dd82f0) at /home/ceeac/Projects/code/simutrans/src/simutrans/simmain.cc:1564
#21 0x00005555559f085f in sysmain(int, char**) (argc=4, argv=0x7fffffffdbc8) at /home/ceeac/Projects/code/simutrans/src/simutrans/sys/simsys.cc:1364
#22 0x0000555555aedbbe in main(int, char**) (argc=4, argv=0x7fffffffdbc8) at /home/ceeac/Projects/code/simutrans/src/simutrans/sys/simsys_s2.cc:1186


[Roboron]

Interesting. The debug build does indeed crash. The release build does not

[Andarix]

...
I am afraid I asked for opinions on this and there is evidence the input text is used to load savegames as well.



So, we better keep it (but change the design to my proposed one).
...

I think an option to reduce the list to only compatible saves could provide a better overview.

[Andarix]

Interesting. The debug build does indeed crash. The release build does not

Information about the operating system would probably be helpful.

[Roboron]

Information about the operating system would probably be helpful.

Not related to OS.

The issue is this check in local_set_tool

Code Select Expand

// set a new tool on our client, calls init
void karte_t::local_set_tool( tool_t *tool_in, player_t * player )
{
    tool_in->flags |= tool_t::WFL_LOCAL;
    // now call init
    bool init_result = tool_in->init(player);
    // for unsafe tools init() must return false
    assert(tool_in->is_init_keeps_game_state()  ||  !init_result);
Here the condition is failing because !init_result is true (tool_load_scenario_t::init returns true on success and tool_load_scenario_t::is_init_keeps_game_state always return false).

I guess the issue is that local_set_tool is only called if calling tool_load_scenario_t directly from the banner. Why? Because of this condition in karte_t::set_tool_api

Code Select Expand

if (tool_in->is_init_keeps_game_state()  ||  (get_random_mode() & INTERACTIVE_RANDOM) == 0) {
// network safe or not during sync_step: call directly
local_set_tool(tool_in, player);
}
else {
// queue tool for execution
We are not in sync_step (I think) during the banner display, so local_set_tool is called and the condition fails.

The assertion says tool_t::init() should return false for unsafe tools, while tool_load_scenario_t::init() returns true. I don't know what an unsafe tool is, so I don't know if there's a way around this, or if the solution is to try to call it inside sync_step someway.

[ceeac]

Crash should be gone in r10999.

The assertion says tool_t::init() should return false for unsafe tools, while tool_load_scenario_t::init() returns true. I don't know what an unsafe tool is, so I don't know if there's a way around this, or if the solution is to try to call it inside sync_step someway

unsafe == alters game state. For example, the query tool is network safe because it does not alter game state, so we can execute it immediately without sending the command to the server. In the case of the load scenario tool, work() is a no-op so we can just return false at the end of init().

[Yona-TYT]

Not related to OS.

The issue is this check in local_set_tool

Code Select Expand

// set a new tool on our client, calls init
void karte_t::local_set_tool( tool_t *tool_in, player_t * player )
{
    tool_in->flags |= tool_t::WFL_LOCAL;
    // now call init
    bool init_result = tool_in->init(player);
    // for unsafe tools init() must return false
    assert(tool_in->is_init_keeps_game_state()  ||  !init_result);
Here the condition is failing because !init_result is true (tool_load_scenario_t::init returns true on success and tool_load_scenario_t::is_init_keeps_game_state always return false).

I guess the issue is that local_set_tool is only called if calling tool_load_scenario_t directly from the banner. Why? Because of this condition in karte_t::set_tool_api

Code Select Expand

if (tool_in->is_init_keeps_game_state()  ||  (get_random_mode() & INTERACTIVE_RANDOM) == 0) {
// network safe or not during sync_step: call directly
local_set_tool(tool_in, player);
}
else {
// queue tool for execution
We are not in sync_step (I think) during the banner display, so local_set_tool is called and the condition fails.

The assertion says tool_t::init() should return false for unsafe tools, while tool_load_scenario_t::init() returns true. I don't know what an unsafe tool is, so I don't know if there's a way around this, or if the solution is to try to call it inside sync_step someway.

Wow, that brings back memories, I spent days trying to implement a script function that would get functions from the building tools, but I can't was able to deal with those kinds of crashes and never achieved my goal. : (

[Andarix]

What I also miss a bit is the ability to leave a server game.

Currently this is only possible via new game. In other words, you have to somehow provoke a disconnect or restart Simutrans.

Simply loading another game is not possible if players are protected with a password. Because then the password query comes instead of the loading dialog.

[ceeac]

The "Continue Game" button is now implemented. It loads the most recent manual save for the current pakset; support for autosaves is still missing, however.

[prissi]

Isn't this more like "Restore last saved game"? (Ok, too long for a button, so rather "Restore Game"?