Merge lp:~widelands-dev/widelands/autosave_on_objectives into lp:widelands

[cghislai]

I just applied the patch provided in the bug report and removed the option checkbox.

Concerning the new lua function, i added it to the const static struct luaL_reg wlgame [] and was able to use it in a newly created win condition, so i guess this was ok?

[SirVer (sirver)]

I pushed my review of this branch in r6613. You can find all my comments by grep-ing for #cghislai. I changed some nits around (see the diff) and added a short comment when I found it appropriate - feel free to just delete them again, they are for your eyes only :). I did not touch anything that would affect the logic because you are more suited to do this as you know your code better.

I think the code looks very reasonable already. The biggest issues is that we now have wl.Game():save and wl.game.save_game - the first one is only used in the persistence.wmf test (see tests/lua/persistence.wmf/scripting/init.lua). I think you should move your logic to the method that is already there and make sure that its new features are exercised in the persistence tests - comments are in the source code as well.

review: Needs Fixing

[cghislai (charlyghislain)]

I'm a bit lost in the lua tests. It appears I cannot make it run even with trunk code :
"could not load "test/lua/persistence.wmf"

Could you check it? I will try to make a save-in-corountine test afterwards.

I refactored m_allow_autosaving to m_allow_saving, because i guess we have to prevent manual saves as well then, right?

I removed some comments i grasped, and left some that may still be useful

[SirVer (sirver)]

I looked over the code again and removed more of my comments that you adressed or that I made up my mind about :). One more thing: If you change a Lua method make sure that you update where it is used in the code (i.e. in all *.lua files) and add in a backwards compatible version (I did this for this change). The reason for this is that the Lua interface is a kind of public API that people should rely on and that should not change to quickly so that they have time to adapt - there is no proper deprecation warning for this yet, for now I just simply added a comment.

These methods must be around for at least three builds - the reason is that the lua code is saved into savegames, so if the API changes and a player tries to reload a scenario with older code, the API calls must still be around.

About the test running, the comments in the file where slightly outdated. I updated them to this:
$PATH_TO_WIDELANDS_BINARY --datadir="." --nozip --scenario=test/lua/persistence.wmf && $PATH_TO_WIDELANDS_BINARY --datadir="." --loadgame=~/.widelands/save/lua_persistence.wgf

of course a rm -rf ~/.widelands/save/lua_persistence.wgf should be a good idea as well.

Could you try again if this works for you? You have to be in the root directory of the repository for this to work.

[cghislai (charlyghislain)]

Running the coroutine test I get a PANIC: unprotected error in call to Lua API (Attempt to persist a C function) which I'm unable to track down...
I'm not very familiar with lua or scripting at that low level (yet).

In the meantime, the former persistence test passed, I think i was trying with the system executable.

[SirVer (sirver)]

This is the error you get when Lua tries to persist a running coroutine. So this is precisely the error you want to fix :).

In trunk, the save routine must only be called when no coroutine is running, i.e. outside of any coroutine, i.e. when the script is run the first time. This is not very flexible and means that you can essentially not save from inside Lua. I did it because I needed a save routine for the persistence test and this did the job.

Your code now should be able to handle this though, because as soon as you hit the main loop and poll if the game should be saved, there cannot be a Lua coroutine running anymore, so saving should work.

[cghislai (charlyghislain)]

The autosave seems to work as expected in campaign. Also calling wl.Game():save("test") in a win condition script worked. The coroutine_persistence.wmf test passed, but please check that it tests correctly.

[SirVer (sirver)]

I did slight modifications to the test and added a bunch of comments - I found the reason why the test did work for you and then didn't and did again: lunit did define some handles to cfunctions, so it needs to be pulled in after loading :).

I will merge this as soon as i get good internet again - seems impossible in this train right now.

review: Approve

=== modified file 'campaigns/tutorial01.wmf/scripting/init.lua'
--- campaigns/tutorial01.wmf/scripting/init.lua	2010-11-12 12:42:11 +0000
+++ campaigns/tutorial01.wmf/scripting/init.lua	2013-07-19 21:32:25 +0000
@@ -32,7 +32,7 @@
 use("map", "texts")
 
 -- =================
--- Helper functions 
+-- Helper functions
 -- =================
 
 -- A small helper class to disable/enable autosaving and user interaction
@@ -49,14 +49,14 @@
 function UserInputDisabler:establish_blocks()
    self._ui_state = wl.ui.get_user_input_allowed()
    local game = wl.Game()
-   self._as_state = game.allow_autosaving
+   self._as_state = game.allow_saving
 
    wl.ui.set_user_input_allowed(false)
-   wl.Game().allow_autosaving = false
+   wl.Game().allow_saving = false
 end
 function UserInputDisabler:lift_blocks()
    wl.ui.set_user_input_allowed(self._ui_state)
-   wl.Game().allow_autosaving= self._as_state
+   wl.Game().allow_saving = self._as_state
 end
 
 function _try_add_objective(i)
@@ -108,7 +108,7 @@
    sleep(130)
    return o
 end
-   
+
 function click_on_field(f, g_T, g_sleeptime)
    local sleeptime = g_sleeptime or 500
 
@@ -125,7 +125,7 @@
 
 function click_on_panel(panel, g_T, g_sleeptime)
    local sleeptime = g_sleeptime or 500
-   
+
    local blocker = UserInputDisabler:new()
 
    sleep(sleeptime)
@@ -143,7 +143,7 @@
 function warp_houses(descriptions)
    local blocker = UserInputDisabler:new()
 
-   for idx, d in ipairs(descriptions) do 
+   for idx, d in ipairs(descriptions) do
       local name, x, y = d[1], d[2], d[3]
       mouse_smoothly_to(wl.Game().map:get_field(x, y))
       sleep(300)
@@ -167,7 +167,7 @@
 
 function build_road(field, ...)
    -- Build a road by clicking the UI. A little faster than before
-   
+
    -- Make sure that there is room for the road: Rip all immovables
    local cf = field
    for idx, d in ipairs{...} do
@@ -206,11 +206,11 @@
       end
    end
 end
-   
+
 function build_eastern_trainings_area(citadel_field)
    -- Build some infrastructure as another example
    local blocker = UserInputDisabler:new()
-   
+
    plr:reveal_fields(citadel_field:region(8))
    scroll_smoothly_to(wl.Game().map:get_field(21,9))
    scroll_smoothly_to(citadel_field)
@@ -262,16 +262,16 @@
    -- add buildwares to the warehouse
    local ts = map:get_field(31,56).immovable
    ts:set_wares(ts.valid_wares)
-   
+
    scroll_smoothly_to(citadel_field)
 
    blocker:lift_blocks()
 end
-   
+
 -- Remove all stones in a given environment. This is done
 -- in a loop for a nice optical effect
 function remove_all_stones(fields, g_sleeptime)
-   local sleeptime = g_sleeptime or 150 
+   local sleeptime = g_sleeptime or 150
    local map = wl.Game().map
    while #fields > 0 do
       local idx = math.random(#fields)
@@ -283,7 +283,7 @@
          if n then
             n = tonumber(n)
             f.immovable:remove()
-            if n > 1 then 
+            if n > 1 then
                remove_field = false
                map:place_immovable("stones" .. n-1, f)
             end
@@ -298,7 +298,7 @@
 end
 
 -- ==============
--- Sentry Thread 
+-- Sentry Thread
 -- ==============
 -- This thread makes sure that the player does not build stuff where he
 -- is not supposed to. He gets a message box when he tries and what he build
@@ -345,7 +345,7 @@
       sleep(1000)
    end
 end
-   
+
 -- Allows constructionsites for the given buildings, all others are invalid
 -- as is any other immovable build by the player
 function allow_constructionsite(i, buildings)
@@ -366,7 +366,7 @@
 end
 
 -- ================
--- Message threads 
+-- Message threads
 -- ================
 function starting_infos()
    sleep(100)
@@ -418,7 +418,7 @@
    click_on_field(wl.Game().map.player_slots[1].starting_field.brn)
 
    msg_box(lumberjack_message_04)
-   
+
    register_immovable_as_allowed(first_lumberjack_field.immovable) -- hut + flag
 
    local f = wl.Game().map:get_field(14,11)
@@ -429,25 +429,25 @@
    blocker:lift_blocks()
 
    sleep(15000)
-   
+
    local o = msg_box(lumberjack_message_05)
 
    local blocker = UserInputDisabler:new()
-   
+
    local f = wl.Game().map:get_field(14,11)
    scroll_smoothly_to(f)
    mouse_smoothly_to(f)
-   
+
    blocker:lift_blocks()
-   
+
    -- Wait for flag
    while not (f.immovable and f.immovable.type == "flag") do sleep(300) end
    o.done = true
 
    sleep(300)
-   
+
    msg_box(lumberjack_message_06)
-   
+
    while #plr:get_buildings("lumberjacks_hut") < 1 do sleep(300) end
 
    msg_box(lumberjack_message_07)
@@ -458,7 +458,7 @@
 function learn_to_move()
    -- Teaching the user how to scroll on the map
    local o = msg_box(inform_about_stones)
-   
+
    function _wait_for_move()
       local cx = wl.ui.MapView().viewpoint_x
       local cy = wl.ui.MapView().viewpoint_y
@@ -477,12 +477,12 @@
    _wait_for_move()
    o.done = true
    sleep(3000) -- Give the player a chance to try this some more
-   
+
    msg_box(congratulate_and_on_to_quarry)
 
    build_a_quarry()
 end
-   
+
 function build_a_quarry()
    sleep(200)
 
@@ -505,12 +505,12 @@
 
    local function _rip_road()
       for idx,f in ipairs(cs.fields[1].brn:region(2)) do
-         if f.immovable and f.immovable.type == "road" then 
+         if f.immovable and f.immovable.type == "road" then
             click_on_field(f)
             click_on_panel(wl.ui.MapView().windows.
                field_action.buttons.destroy_road, 300)
             sleep(200)
-            return 
+            return
          end
       end
    end
@@ -527,17 +527,17 @@
    click_on_field(map:get_field(11,12))
    click_on_field(map:get_field(12,12))
    click_on_field(map:get_field(12,11))
-   
+
    sleep(3000)
 
    _rip_road()
-   
+
    msg_box(talk_about_roadbuilding_01)
    -- Showoff direct roadbuilding
    click_on_field(cs.fields[1].brn)
    click_on_panel(wl.ui.MapView().windows.field_action.buttons.build_road, 300)
    click_on_field(map.player_slots[1].starting_field.brn)
-   
+
    sleep(3000)
 
    _rip_road()
@@ -545,12 +545,12 @@
    blocker:lift_blocks()
 
    local o = msg_box(talk_about_roadbuilding_02)
-   
+
    -- From now on, the player can build whatever he wants
    terminate_bad_boy_sentinel = true
-   
+
    -- Wait a while
-   sleep( 100*1000 )   
+   sleep( 100*1000 )
 
    -- Interludium: talk about census and statistics
    census_and_statistics(cs.fields[1])
@@ -560,17 +560,17 @@
 
    messages()
 end
-   
+
 function census_and_statistics(field)
    sleep(15000)
-   
+
    local blocker = UserInputDisabler:new()
 
    wl.ui.MapView().census = false
    wl.ui.MapView().statistics = false
-               
+
    wl.ui.MapView():abort_road_building()
-  
+
    msg_box(census_and_statistics_00)
    -- Pick any empty field
    local function _pick_empty_field()
@@ -591,7 +591,7 @@
    click_on_panel(wl.ui.MapView().windows.field_action.buttons.statistics)
 
    msg_box(census_and_statistics_01)
-   
+
    blocker:lift_blocks()
 end
 
@@ -611,7 +611,7 @@
    -- Wait for messages window to close
    while wl.ui.MapView().windows.messages do sleep(300) end
    o.done = true
-   
+
    msg_box(closing_msg_window_01)
 
    -- Remove all stones
@@ -627,10 +627,10 @@
    expansion()
 end
 
-function expansion() 
+function expansion()
    -- Teach about expanding your territory.
    sleep(10)
-   
+
    -- This is not really needed since the stones are already removed, but if
    -- we're debugging and we start with this function it is most useful to have
    -- the stones away already
@@ -655,7 +655,7 @@
       while #fields > 0 do
          local idx = math.random(#fields)
          local f = fields[idx]
-         
+
          if f.terr:match("berg%d+") and f.terd:match("berg%d+") then
             if pcall(function() plr:place_flag(f) end) then
                f.immovable:remove()
@@ -678,7 +678,7 @@
    end
 
    scroll_smoothly_to(conquer_field)
-   
+
    local dest = _find_good_flag_position()
    local start = _find_nearby_flag()
 
@@ -699,7 +699,7 @@
    local function _wait_for_some_resi(wanted)
       while 1 do
          local cnt = 0
-         for idx, f in ipairs(dest:region(6)) do 
+         for idx, f in ipairs(dest:region(6)) do
             if f.immovable and f.immovable.name:sub(1,4) == "resi" then
                cnt = cnt + 1
                if cnt >= wanted then return end
@@ -720,7 +720,7 @@
 function training()
    -- Teach about trainingsites and soldiers
    sleep(300)
-   
+
    msg_box(warfare_and_training_00)
 
    local citadel_field = wl.Game().map:get_field(31, 63)
@@ -734,7 +734,7 @@
    scroll_smoothly_to(citadel_field)
 
    local o = msg_box(enhance_fortress)
-   while not (citadel_field.immovable and 
+   while not (citadel_field.immovable and
       citadel_field.immovable.name == "citadel") do sleep(800) end
    o.done = true
 
 
=== modified file 'src/logic/game.h'
--- src/logic/game.h	2013-07-14 17:05:25 +0000
+++ src/logic/game.h	2013-07-19 21:32:25 +0000
@@ -90,10 +90,6 @@
 	friend struct ::Game_Main_Menu_Load_Game;
 	friend struct ::WLApplication;
 
-	// This friend is for legacy reasons and should probably be removed
-	// at least after summer 2008, maybe even earlier.
-	friend struct Game_Interactive_Player_Data_Packet;
-
 	Game();
 	~Game();
 
 
=== modified file 'src/save_handler.cc'
--- src/save_handler.cc	2013-07-16 13:26:14 +0000
+++ src/save_handler.cc	2013-07-19 21:32:25 +0000
@@ -39,19 +39,35 @@
  */
 void SaveHandler::think(Widelands::Game & game, int32_t realtime) {
 	initialize(realtime);
-
-	if (not m_allow_autosaving) // Is autosaving allowed atm?
-		return;
-
-	int32_t const autosaveInterval =
-		g_options.pull_section("global").get_int
-			("autosave", DEFAULT_AUTOSAVE_INTERVAL * 60);
-	if (autosaveInterval <= 0)
-		return; // no autosave requested
-
-	int32_t const elapsed = (realtime - m_last_saved_time) / 1000;
-	if (elapsed < autosaveInterval)
-		return;
+	std::string filename = "wl_autosave";
+
+	if (!m_allow_saving) {
+		return;
+	}
+
+	if (m_save_requested) {
+		if (!m_save_filename.empty()) {
+			filename = m_save_filename;
+		}
+
+		log("Autosave: save requested : %s\n", filename.c_str());
+		m_save_requested = false;
+		m_save_filename = "";
+	} else {
+		const int32_t autosave_interval_in_seconds =
+			g_options.pull_section("global").get_int
+				("autosave", DEFAULT_AUTOSAVE_INTERVAL * 60);
+		if (autosave_interval_in_seconds <= 0) {
+			return; // no autosave requested
+		}
+
+		const int32_t elapsed = (realtime - m_last_saved_time) / 1000;
+		if (elapsed < autosave_interval_in_seconds) {
+			return;
+		}
+
+		log("Autosave: interval elapsed (%d s), saving\n", elapsed);
+	}
 
 	// TODO: defer saving to next tick so that this message is shown
 	// before the actual save, or put the saving logic in another thread
@@ -59,20 +75,20 @@
 		(_("Saving game..."));
 
 	// save the game
-	std::string complete_filename =
-		create_file_name (get_base_dir(), "wl_autosave");
+	const std::string complete_filename = create_file_name(get_base_dir(), filename);
 	std::string backup_filename;
 
 	// always overwrite a file
 	if (g_fs->FileExists(complete_filename)) {
-		backup_filename = create_file_name (get_base_dir(), "wl_autosave2");
+		filename += "2";
+		backup_filename = create_file_name (get_base_dir(), filename);
 		if (g_fs->FileExists(backup_filename)) {
 			g_fs->Unlink(backup_filename);
 		}
 		g_fs->Rename(complete_filename, backup_filename);
 	}
 
-	static std::string error;
+	std::string error;
 	if (!save_game(game, complete_filename, &error)) {
 		log("Autosave: ERROR! - %s\n", error.c_str());
 		game.get_ipl()->get_chat_provider()->send_local
 
=== modified file 'src/save_handler.h'
--- src/save_handler.h	2013-07-14 11:48:13 +0000
+++ src/save_handler.h	2013-07-19 21:32:25 +0000
@@ -32,7 +32,8 @@
 
 class SaveHandler {
 public:
-	SaveHandler() : m_last_saved_time(0), m_initialized(false), m_allow_autosaving(true) {}
+	SaveHandler() : m_last_saved_time(0), m_initialized(false), m_allow_saving(true),
+		m_save_requested(false), m_save_filename("") {}
 	void think(Widelands::Game &, int32_t currenttime);
 	std::string create_file_name(std::string dir, std::string filename);
 	bool save_game
@@ -43,13 +44,20 @@
 	static std::string get_base_dir() {return "save";}
 	const std::string get_cur_filename() {return m_current_filename;}
 	void set_current_filename(std::string filename) {m_current_filename = filename;}
-	void set_allow_autosaving(bool t) {m_allow_autosaving = t;}
-	bool get_allow_autosaving() {return m_allow_autosaving;}
+	void set_allow_saving(bool t) {m_allow_saving = t;}
+	bool get_allow_saving() {return m_allow_saving;}
+	void request_save(std::string filename = "")
+	{
+		m_save_requested = true;
+		m_save_filename = filename;
+	}
 
 private:
 	int32_t m_last_saved_time;
 	bool m_initialized;
-	bool m_allow_autosaving;
+	bool m_allow_saving;
+	bool m_save_requested;
+	std::string m_save_filename;
 	std::string m_current_filename;
 
 	void initialize(int32_t currenttime);
 
=== modified file 'src/scripting/lua_game.cc'
--- src/scripting/lua_game.cc	2013-07-15 05:18:12 +0000
+++ src/scripting/lua_game.cc	2013-07-19 21:32:25 +0000
@@ -470,7 +470,7 @@
 	uint32_t cspeed = game.gameController()->desiredSpeed();
 	game.gameController()->setDesiredSpeed(0);
 
-	game.save_handler().set_allow_autosaving(false);
+	game.save_handler().set_allow_saving(false);
 
 	Story_Message_Box * mb =
 		new Story_Message_Box
@@ -486,7 +486,7 @@
 
 	game.gameController()->setDesiredSpeed(cspeed);
 
-	game.save_handler().set_allow_autosaving(true);
+	game.save_handler().set_allow_saving(true);
 
 	return 1;
 }
@@ -1062,8 +1062,10 @@
 	.. attribute:: done
 
 		(RW) defines if this objective is already fulfilled. If done is
-		:const`true`, the objective will not be shown to the user, no matter what
-		:attr:`visible` is set to.
+		:const`true`, the objective will not be shown to the user, no matter what.
+		:attr:`visible` is set to. A savegame will be created when this attribute
+		is changed to :const`true`.
+
 */
 int L_Objective::get_done(lua_State * L) {
 	Objective & o = get(L, get_game(L));
@@ -1073,6 +1075,19 @@
 int L_Objective::set_done(lua_State * L) {
 	Objective & o = get(L, get_game(L));
 	o.set_done(luaL_checkboolean(L, -1));
+
+	const int32_t autosave = g_options.pull_section("global").get_int("autosave", 0);
+	if (autosave <= 0) {
+		return 0;
+	}
+
+	if (o.done()) {
+		std::string filename = get_egbase(L).get_map()->get_name();
+		char buffer[128];
+		snprintf(buffer, sizeof(buffer), _(" (achieved %s)"), o.descname().c_str());
+		filename.append(buffer);
+		get_game(L).save_handler().request_save(filename);
+	}
 	return 0;
 }
 
 
=== modified file 'src/scripting/lua_root.cc'
--- src/scripting/lua_root.cc	2012-06-08 22:33:16 +0000
+++ src/scripting/lua_root.cc	2013-07-19 21:32:25 +0000
@@ -82,6 +82,7 @@
 	PROP_RO(L_Game, time),
 	PROP_RW(L_Game, desired_speed),
 	PROP_RW(L_Game, allow_autosaving),
+	PROP_RW(L_Game, allow_saving),
 	{0, 0, 0},
 };
 
@@ -131,22 +132,33 @@
 }
 
 /* RST
-	.. attribute:: allow_autosaving
+	.. attribute:: allow_saving
 
-		(RW) Disable or enable auto-saving. When you show off UI features in a
-		tutorial or scenario, you have to disallow auto-saving because UI
+		(RW) Disable or enable saving. When you show off UI features in a
+		tutorial or scenario, you have to disallow saving because UI
 		elements can not be saved and therefore reloading a game saved in the
 		meantime would crash the game.
 */
 // UNTESTED
+int L_Game::set_allow_saving(lua_State * L) {
+	get_game(L).save_handler().set_allow_saving
+		(luaL_checkboolean(L, -1));
+	return 0;
+}
+// UNTESTED
+int L_Game::get_allow_saving(lua_State * L) {
+	lua_pushboolean(L, get_game(L).save_handler().get_allow_saving());
+	return 1;
+}
 int L_Game::set_allow_autosaving(lua_State * L) {
-	get_game(L).save_handler().set_allow_autosaving
+	// WAS_DEPRECATED_BEFORE(build18), use allow_saving
+	get_game(L).save_handler().set_allow_saving
 		(luaL_checkboolean(L, -1));
 	return 0;
 }
-// UNTESTED
 int L_Game::get_allow_autosaving(lua_State * L) {
-	lua_pushboolean(L, get_game(L).save_handler().get_allow_autosaving());
+	// WAS_DEPRECATED_BEFORE(build18), use allow_saving
+	lua_pushboolean(L, get_game(L).save_handler().get_allow_saving());
 	return 1;
 }
 
@@ -192,35 +204,23 @@
 /* RST
 	.. method:: save(name)
 
-		Saves the game exactly as if the player had entered the save dialog and
-		entered name as an argument. If some error occurred while saving, this
-		will throw an Lua error. Note that this currently doesn't work when
-		called from inside a Coroutine.
+		Requests a savegame. Note that the actual save will be performed
+		later, and that you have no control over any error that may happen
+		by then currently.
 
-		:arg name: name of save game. If this game already exists, it will be
-			silently overwritten
+		:arg name: name of save game, as if entered in the save dialog.
+			If this game already exists, it will be silently overwritten.
+			If empty, the autosave name will be used.
 		:type name: :class:`string`
 		:returns: :const:`nil`
 */
-int L_Game::save(lua_State * const L) {
-	Widelands::Game & game = get_game(L);
-
-	std::string const complete_filename =
-		game.save_handler().create_file_name
-			(SaveHandler::get_base_dir(), luaL_checkstring(L, -1));
-
-	lua_pop(L, 2); // Make stack empty before persistence starts.
-
-	if (g_fs->FileExists(complete_filename))
-		g_fs->Unlink(complete_filename);
-	std::string error;
-	if (!game.save_handler().save_game(game, complete_filename, &error))
-		return report_error(L, "save error: %s", error.c_str());
+int L_Game::save(lua_State * L) {
+	std::string filename = luaL_checkstring(L, -1);
+	get_game(L).save_handler().request_save(filename);
 
 	return 0;
 }
 
-
 /*
  ==========================================================
  C METHODS
 
=== modified file 'src/scripting/lua_root.h'
--- src/scripting/lua_root.h	2012-02-15 21:25:34 +0000
+++ src/scripting/lua_root.h	2013-07-19 21:32:25 +0000
@@ -53,6 +53,8 @@
 	int set_desired_speed(lua_State *);
 	int get_allow_autosaving(lua_State *);
 	int set_allow_autosaving(lua_State *);
+	int get_allow_saving(lua_State *);
+	int set_allow_saving(lua_State *);
 
 	/*
 	 * Lua methods
 
=== modified file 'test/lua/persistence.wmf/scripting/init.lua'
--- test/lua/persistence.wmf/scripting/init.lua	2011-02-21 20:36:10 +0000
+++ test/lua/persistence.wmf/scripting/init.lua	2013-07-19 21:32:25 +0000
@@ -1,128 +1,138 @@
 -- =======================================================================
---                           LOADING/SAVING TESTS                           
+--                           LOADING/SAVING TESTS
 -- =======================================================================
--- This tests saving and loading of various Lua objects in the global 
+-- This tests saving and loading of various Lua objects in the global
 -- environment.
 --
--- To run this test use:
--- ./widelands --nozip --scenario=src/scripting/test/persistence.wmf && 
---   ./widelands --loadgame=~/.widelands/save/lua_persistence.wgf
-
+-- To run this test, go to the top level directory, set the path to your
+-- widelands binary and run:
+--
+-- $PATH_TO_WIDELANDS_BINARY --datadir="." --nozip --scenario=test/lua/persistence.wmf && \
+--   $PATH_TO_WIDELANDS_BINARY --datadir="." --loadgame=~/.widelands/save/lua_persistence.wgf
 
 -- ====================
--- Test Data to persist 
+-- Test Data to persist
 -- ====================
 use("aux", "set")
-
-my_name = "SirVer"
-pi = 3.1415
-eight = 8
-is_true = true
-is_false = false
-
 game = wl.Game()
-p = game.players[1]
-map = game.map
-a = { "Hallo", "Welt" }
-c = { func = function(a) return "I say " .. a .. "!" end }
-field = map:get_field(32,34)
-tree = map:place_immovable("tree3", field)
-removed_tree = map:place_immovable("tree4", map:get_field(34,34))
-removed_tree:remove()
-corout = coroutine.create(function()
-   local a = 100
-   coroutine.yield("What cool is that?")
-   coroutine.yield(a)
-end)
-objective = p:add_objective("lumber", "House", "Ship!")
-objective.done = true
-
-p:send_message("dummy msg1", "dummy msg 1")
-msg = p:send_message("hello nice", "World", {sender="blah", field = field })
-player_slot = map.player_slots[1]
-
-myset = Set:new{
-   map:get_field(10,10), map:get_field(10,10), map:get_field(10,11)
-}
-
--- ========================
--- Test after unpersisting 
--- ========================
-function check_persistence()
-coroutine.yield(wl.Game().time + 2000)
-
-use("map", "lunit")
-lunit.import "assertions"
-
-print("###################### CHECKING FOR CORRECT PERSISTENCE")
-assert_equal("SirVer", my_name)
-assert_equal(3.1415, pi)
-assert_equal(8, eight)
-assert_equal(true, is_true)
-assert_equal(false, is_false)
-
-assert_equal(1, p.number)
-
-assert_table(a)
-assert_equal(a[1], "Hallo")
-assert_equal(a[2], "Welt")
-
-assert_table(c)
-assert_function(c.func)
-assert_equal("I say zonk!", c.func("zonk"))
-
-assert_equal("tree3", tree.name)
-
-assert_equal(32, field.x)
-assert_equal(34, field.y)
-assert_equal(tree, field.immovable)
-
-assert_thread(corout)
-_,rv = coroutine.resume(corout)
-assert_equal("What cool is that?", rv)
-_,rv = coroutine.resume(corout)
-assert_equal(100, rv)
-
-assert_table(objective)
-assert_equal("lumber", objective.name)
-assert_equal("House", objective.title)
-assert_equal("Ship!", objective.body)
-assert_equal(true, objective.done)
-
-assert_table(msg)
-assert_equal("hello nice", msg.title)
-assert_equal("World", msg.body)
-assert_equal("blah", msg.sender)
-assert_equal(field, msg.field)
-
-assert_table(map)
-assert_equal(64, map.width)
-assert_equal(64, map.height)
-
-assert_table(player_slot)
-assert_equal("barbarians", player_slot.tribe_name)
-assert_equal("Player 1", player_slot.name)
-assert_equal(player_slot.name, map.player_slots[1].name)
-assert_equal(player_slot.tribe_name, map.player_slots[1].tribe_name)
-
-assert_equal(2, myset.size)
-assert_true(myset:contains(map:get_field(10,10)))
-assert_true(myset:contains(map:get_field(10,11)))
-
-print("################### ALL TEST PASS!")
-
-wl.ui.MapView():close()
+
+function save_coroutine()
+   game = wl.Game()
+
+   my_name = "SirVer"
+   pi = 3.1415
+   eight = 8
+   is_true = true
+   is_false = false
+
+   game = wl.Game()
+   p = game.players[1]
+   map = game.map
+   a = { "Hallo", "Welt" }
+   c = { func = function(a) return "I say " .. a .. "!" end }
+   field = map:get_field(32,34)
+   tree = map:place_immovable("tree3", field)
+   removed_tree = map:place_immovable("tree4", map:get_field(34,34))
+   removed_tree:remove()
+
+   corout = coroutine.create(function()
+      local a = 100
+      coroutine.yield("What cool is that?")
+      coroutine.yield(a)
+   end)
+   objective = p:add_objective("lumber", "House", "Ship!")
+   objective.done = true
+
+   p:send_message("dummy msg1", "dummy msg 1")
+   msg = p:send_message("hello nice", "World", {sender="blah", field = field })
+   player_slot = map.player_slots[1]
+
+   myset = Set:new{
+      map:get_field(10,10), map:get_field(10,10), map:get_field(10,11)
+   }
+
+   game:save("lua_persistence")
+   print("Save requested\n");
+
+   -- Stick around, so that we are needed to get loaded too.
+   coroutine.yield(wl.Game().time + 8000)
+end
+
+function check_coroutine()
+   -- Sleep till the save routine has done its job.
+   coroutine.yield(wl.Game().time + 2000)
+
+   -- Attention, lunit contains code that can not be persisted (c functions),
+   -- so it must be imported after reload.
+   use("map", "lunit")
+   lunit.import "assertions"
+
+   print("###################### CHECKING FOR CORRECT PERSISTENCE")
+   assert_equal("SirVer", my_name)
+   assert_equal(3.1415, pi)
+   assert_equal(8, eight)
+   assert_equal(true, is_true)
+   assert_equal(false, is_false)
+
+   assert_equal(1, p.number)
+
+   assert_table(a)
+   assert_equal(a[1], "Hallo")
+   assert_equal(a[2], "Welt")
+
+   assert_table(c)
+   assert_function(c.func)
+   assert_equal("I say zonk!", c.func("zonk"))
+
+   assert_equal("tree3", tree.name)
+
+   assert_equal(32, field.x)
+   assert_equal(34, field.y)
+   assert_equal(tree, field.immovable)
+
+   assert_thread(corout)
+   _,rv = coroutine.resume(corout)
+   assert_equal("What cool is that?", rv)
+   _,rv = coroutine.resume(corout)
+   assert_equal(100, rv)
+
+   assert_table(objective)
+   assert_equal("lumber", objective.name)
+   assert_equal("House", objective.title)
+   assert_equal("Ship!", objective.body)
+   assert_equal(true, objective.done)
+
+   assert_table(msg)
+   assert_equal("hello nice", msg.title)
+   assert_equal("World", msg.body)
+   assert_equal("blah", msg.sender)
+   assert_equal(field, msg.field)
+
+   assert_table(map)
+   assert_equal(64, map.width)
+   assert_equal(64, map.height)
+
+   assert_table(player_slot)
+   assert_equal("barbarians", player_slot.tribe_name)
+   assert_equal("Player 1", player_slot.name)
+   assert_equal(player_slot.name, map.player_slots[1].name)
+   assert_equal(player_slot.tribe_name, map.player_slots[1].tribe_name)
+
+   assert_equal(2, myset.size)
+   assert_true(myset:contains(map:get_field(10,10)))
+   assert_true(myset:contains(map:get_field(10,11)))
+
+   print("################### ALL TEST PASS!")
+
+   wl.ui.MapView():close()
 end
 
 
 -- ==========
--- Main Code 
+-- Main Code
 -- ==========
 -- This starts the test routine, saves the game and exits.
--- Loading the saved game will check that all objects are 
+-- Loading the saved game will check that all objects are
 -- correctly unpersisted
-game = wl.Game()
-game:launch_coroutine(coroutine.create(check_persistence))
-game:save("lua_persistence")
-wl.ui.MapView():close()
-
+game:launch_coroutine(coroutine.create(check_coroutine))
+game:launch_coroutine(coroutine.create(save_coroutine))