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

[cghislai]

This is first attempt to persist the mapview. I thought that census and statistics test failed, but after merging trunk and testing again the test seems to pass now - I have no idea why.

I also tried to reproduce the bug as explained in the report, and could not trigger the crash, even on empire 2.

[SirVer (sirver)]

I think this is the wrong approach - the LuaClass does not have any data members, so it should not persist anything at all. The MapView (c++ class) should save its data to save games one day, so that you have the same view and so on when loading.

For the Lua fix here, all clases that can get a handle again on the c++ class on a reaload should overwrite the __persists methods which basically does nothing.

The logic here is: if you have a handle to a Window (e.g. a building window of any building) in a lua variable and the game is saved what should the window persist? It cannot reopen the window again after reload (since there is no open_window(name) method), the reloaded game will not have opened the window too, so the variable would contain crap after reloading. For the MapView this is not true: you can always get a hold on the map view again, because there is always a singular one available.

mmh, I fear this was not very well explained. Do you still get what I mean?

review: Needs Fixing

[cghislai (charlyghislain)]

I think so. but, then, the fix is already there, as I think mapview saves the state (census/statistics/viewpoint) and they are loaded.

So, in this case, the L_Mapview class shoud override __persits/__unpersist, and make sure the pointer is correct upon loading (or it is not even needed?).

Now for the other window, that would need another data paquet class, which would look for UniqueWindow registries, or buildingwindows serials, or field windows coords, and save positions and so. I am not sure for the scripted windows though.

[SirVer (sirver)]

>
> So, in this case, the L_Mapview class shoud override __persits/__unpersist,
> and make sure the pointer is correct upon loading (or it is not even needed?).
I think nothing is needed - just two empty functions.

> Now for the other window, that would need another data paquet class, which
> would look for UniqueWindow registries, or buildingwindows serials, or field
> windows coords, and save positions and so.
the unique windows would need special treatment. Building windows would need special treatment. And mission texts would be very difficult to get right. Starting with the unique windows would be a possiblity, but I think all of this is pretty low priority right now.

> I am not sure for the scripted windows though.

[SirVer (sirver)]

btw, is this ready for review too?

[cghislai (charlyghislain)]

Yes it is. I just needed to restore the m_panel pointer in the __unpersist method.

[SirVer (sirver)]

I think this is the very right approach to doing this. I am still in the USA right now, and pretty loaded, so I am still slow to review code. However one thing immediately: game_mapview_player_data_paquet should be game_mapview_player_data_packet. Same for the classes. Why did you change this to Paquet?

[SirVer (sirver)]

review: Needs Fixing

[cghislai (charlyghislain)]

I revert the renaming interactiveplayer->mapview. I did it because I thought it would make more sense, and I didn't even notice i misspelled 'packet'. And now that I think of it, interactive_player makes more sense. Unfortunately, the diff doesn't show the content differences. The only thing that changed is that it tries to persist interactive_base fields, and only use the interactive_player pointer to persist the player number.

Speaking of which, does game.get_ibase() sometimes return null? Here the check is made, but I remember some place in the code where it assumed the pointed object exists.

[SirVer (sirver)]

I applied a diff of this to trunk and committed this as a new revision. That means your editing history is gone, but I didn't feel comfortable having this removed and added of the same file in the same commit - I do not trust bzr this far :).

This is therefore merged, i.e. it's content is in trunk.

review: Approve

=== added file 'src/game_io/game_interactive_player_data_packet.cc'
--- src/game_io/game_interactive_player_data_packet.cc	1970-01-01 00:00:00 +0000
+++ src/game_io/game_interactive_player_data_packet.cc	2013-08-01 08:24:32 +0000
@@ -0,0 +1,116 @@
+/*
+ * Copyright (C) 2002-2004, 2006-2009 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#include "game_io/game_interactive_player_data_packet.h"
+
+#include "logic/game.h"
+#include "logic/game_data_error.h"
+#include "logic/player.h"
+#include "logic/tribe.h"
+#include "logic/widelands_fileread.h"
+#include "logic/widelands_filewrite.h"
+#include "wui/interactive_player.h"
+#include "wui/mapview.h"
+#include "wui/overlay_manager.h"
+
+namespace Widelands {
+
+#define CURRENT_PACKET_VERSION 2
+
+
+void Game_Interactive_Player_Data_Paquet::Read
+	(FileSystem & fs, Game & game, Map_Map_Object_Loader *)
+{
+	try {
+		FileRead fr;
+		fr.Open(fs, "binary/interactive_player");
+		uint16_t const packet_version = fr.Unsigned16();
+		if (packet_version == CURRENT_PACKET_VERSION) {
+			Player_Number player_number =
+				fr.Player_Number8(game.map().get_nrplayers());
+			if (not game.get_player(player_number)) {
+				// This happens if the player, that saved the game, was a spectator
+				// and the slot for player 1 was not used in the game.
+				// So now we try to create an InteractivePlayer object for another
+				// player instead.
+				const Player_Number max = game.map().get_nrplayers();
+				for (player_number = 1; player_number <= max; ++player_number)
+					if (game.get_player(player_number))
+						break;
+				if (player_number > max)
+					throw game_data_error(_("The game has no players!"));
+			}
+			int32_t       const x             = fr.Unsigned16();
+			int32_t       const y             = fr.Unsigned16();
+			uint32_t      const display_flags = fr.Unsigned32();
+
+			if (Interactive_Base * const ibase = game.get_ibase()) {
+				ibase->set_viewpoint(Point(x, y), true);
+
+				uint32_t const loaded_df =
+					Interactive_Base::dfShowCensus |
+					Interactive_Base::dfShowStatistics;
+				uint32_t const olddf = ibase->get_display_flags();
+				uint32_t const realdf =
+					(olddf & ~loaded_df) | (display_flags & loaded_df);
+				ibase->set_display_flags(realdf);
+			}
+			if (Interactive_Player * const ipl = game.get_ipl()) {
+				ipl->set_player_number(player_number);
+			}
+		} else
+			throw game_data_error
+				(_("unknown/unhandled version %u"), packet_version);
+	} catch (const _wexception & e) {
+		throw game_data_error(_("interactive player: %s"), e.what());
+	}
+}
+
+/*
+ * Write Function
+ */
+void Game_Interactive_Player_Data_Paquet::Write
+	(FileSystem & fs, Game & game, Map_Map_Object_Saver * const)
+{
+	FileWrite fw;
+
+	// Now packet version
+	fw.Unsigned16(CURRENT_PACKET_VERSION);
+
+	Interactive_Base * const ibase = game.get_ibase();
+	Interactive_Player * const iplayer = game.get_ipl();
+
+	// Player number
+	fw.Unsigned8(iplayer ? iplayer->player_number() : 1);
+
+	// Map Position
+	if (ibase) {
+		assert(0 <= ibase->get_viewpoint().x);
+		assert(0 <= ibase->get_viewpoint().y);
+	}
+	fw.Unsigned16(ibase ? ibase->get_viewpoint().x : 0);
+	fw.Unsigned16(ibase ? ibase->get_viewpoint().y : 0);
+
+	// Display flags
+	fw.Unsigned32(ibase ? ibase->get_display_flags() : 0);
+
+	fw.Write(fs, "binary/interactive_player");
+}
+
+}

=== removed file 'src/game_io/game_interactive_player_data_packet.cc'
--- src/game_io/game_interactive_player_data_packet.cc	2013-07-26 20:19:36 +0000
+++ src/game_io/game_interactive_player_data_packet.cc	1970-01-01 00:00:00 +0000
@@ -1,114 +0,0 @@
-/*
- * Copyright (C) 2002-2004, 2006-2009 by the Widelands Development Team
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
- *
- */
-
-#include "game_io/game_interactive_player_data_packet.h"
-
-#include "logic/game.h"
-#include "logic/game_data_error.h"
-#include "logic/player.h"
-#include "logic/tribe.h"
-#include "logic/widelands_fileread.h"
-#include "logic/widelands_filewrite.h"
-#include "wui/interactive_player.h"
-#include "wui/mapview.h"
-#include "wui/overlay_manager.h"
-
-namespace Widelands {
-
-#define CURRENT_PACKET_VERSION 2
-
-
-void Game_Interactive_Player_Data_Packet::Read
-	(FileSystem & fs, Game & game, Map_Map_Object_Loader *)
-{
-	try {
-		FileRead fr;
-		fr.Open(fs, "binary/interactive_player");
-		uint16_t const packet_version = fr.Unsigned16();
-		if (packet_version == CURRENT_PACKET_VERSION) {
-			Player_Number player_number =
-				fr.Player_Number8(game.map().get_nrplayers());
-			if (not game.get_player(player_number)) {
-				// This happens if the player, that saved the game, was a spectator
-				// and the slot for player 1 was not used in the game.
-				// So now we try to create an InteractivePlayer object for another
-				// player instead.
-				const Player_Number max = game.map().get_nrplayers();
-				for (player_number = 1; player_number <= max; ++player_number)
-					if (game.get_player(player_number))
-						break;
-				if (player_number > max)
-					throw game_data_error(_("The game has no players!"));
-			}
-			int32_t       const x             = fr.Unsigned16();
-			int32_t       const y             = fr.Unsigned16();
-			uint32_t      const display_flags = fr.Unsigned32();
-
-			if (Interactive_Player * const plr = game.get_ipl()) {
-				plr->set_player_number(player_number);
-
-				plr->set_viewpoint(Point(x, y), true);
-
-				uint32_t const loaded_df =
-					Interactive_Base::dfShowCensus |
-					Interactive_Base::dfShowStatistics;
-				uint32_t const olddf = plr->get_display_flags();
-				uint32_t const realdf =
-					(olddf & ~loaded_df) | (display_flags & loaded_df);
-				plr->set_display_flags(realdf);
-			}
-		} else
-			throw game_data_error
-				(_("unknown/unhandled version %u"), packet_version);
-	} catch (const _wexception & e) {
-		throw game_data_error(_("interactive player: %s"), e.what());
-	}
-}
-
-/*
- * Write Function
- */
-void Game_Interactive_Player_Data_Packet::Write
-	(FileSystem & fs, Game & game, Map_Map_Object_Saver * const)
-{
-	FileWrite fw;
-
-	// Now packet version
-	fw.Unsigned16(CURRENT_PACKET_VERSION);
-
-	Interactive_Player * const plr = game.get_ipl();
-
-	// Player number
-	fw.Unsigned8(plr ? plr->player_number() : 1);
-
-	// Map Position
-	if (plr) {
-		assert(0 <= plr->get_viewpoint().x);
-		assert(0 <= plr->get_viewpoint().y);
-	}
-	fw.Unsigned16(plr ? plr->get_viewpoint().x : 0);
-	fw.Unsigned16(plr ? plr->get_viewpoint().y : 0);
-
-	// Display flags
-	fw.Unsigned32(plr ? plr->get_display_flags() : 0);
-
-	fw.Write(fs, "binary/interactive_player");
-}
-
-}

=== added file 'src/game_io/game_interactive_player_data_packet.h'
--- src/game_io/game_interactive_player_data_packet.h	1970-01-01 00:00:00 +0000
+++ src/game_io/game_interactive_player_data_packet.h	2013-08-01 08:24:32 +0000
@@ -0,0 +1,38 @@
+/*
+ * Copyright (C) 2002-2004, 2006-2009 by the Widelands Development Team
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ *
+ */
+
+#ifndef GAME_INTERACTIVE_PLAYER_DATA_PACKET_H
+#define GAME_INTERACTIVE_PLAYER_DATA_PACKET_H
+
+#include "game_io/game_data_packet.h"
+
+namespace Widelands {
+
+/*
+ * Information about the interactive player. Mostly scrollpos,
+ * player number and so on
+ */
+struct Game_Interactive_Player_Data_Paquet : public Game_Data_Packet {
+	void Read (FileSystem &, Game &, Map_Map_Object_Loader * = 0);
+	void Write(FileSystem &, Game &, Map_Map_Object_Saver  * = 0);
+};
+
+}
+
+#endif

=== removed file 'src/game_io/game_interactive_player_data_packet.h'
--- src/game_io/game_interactive_player_data_packet.h	2013-07-26 20:19:36 +0000
+++ src/game_io/game_interactive_player_data_packet.h	1970-01-01 00:00:00 +0000
@@ -1,38 +0,0 @@
-/*
- * Copyright (C) 2002-2004, 2006-2009 by the Widelands Development Team
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License
- * as published by the Free Software Foundation; either version 2
- * of the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
- *
- */
-
-#ifndef GAME_INTERACTIVE_PLAYER_DATA_PACKET_H
-#define GAME_INTERACTIVE_PLAYER_DATA_PACKET_H
-
-#include "game_io/game_data_packet.h"
-
-namespace Widelands {
-
-/*
- * Information about the interactive player. Mostly scrollpos,
- * player number and so on
- */
-struct Game_Interactive_Player_Data_Packet : public Game_Data_Packet {
-	void Read (FileSystem &, Game &, Map_Map_Object_Loader * = 0);
-	void Write(FileSystem &, Game &, Map_Map_Object_Saver  * = 0);
-};
-
-}
-
-#endif

=== modified file 'src/game_io/game_loader.cc'
--- src/game_io/game_loader.cc	2013-07-27 15:09:18 +0000
+++ src/game_io/game_loader.cc	2013-08-01 08:24:32 +0000
@@ -24,8 +24,8 @@
 
 #include "game_io/game_cmd_queue_data_packet.h"
 #include "game_io/game_game_class_data_packet.h"
+#include "game_io/game_map_data_packet.h"
 #include "game_io/game_interactive_player_data_packet.h"
-#include "game_io/game_map_data_packet.h"
 #include "game_io/game_player_economies_data_packet.h"
 #include "game_io/game_player_info_data_packet.h"
 #include "game_io/game_preload_data_packet.h"
@@ -126,7 +126,7 @@
 	// player.
 	if (!multiplayer) {
 		log("Game: Reading Interactive Player Data ... ");
-		{Game_Interactive_Player_Data_Packet       p; p.Read(m_fs, m_game, mol);}
+		{Game_Interactive_Player_Data_Paquet       p; p.Read(m_fs, m_game, mol);}
 		log(" done\n");
 	}
 

=== modified file 'src/game_io/game_saver.cc'
--- src/game_io/game_saver.cc	2013-07-26 20:19:36 +0000
+++ src/game_io/game_saver.cc	2013-08-01 08:24:32 +0000
@@ -21,8 +21,8 @@
 
 #include "game_io/game_cmd_queue_data_packet.h"
 #include "game_io/game_game_class_data_packet.h"
+#include "game_io/game_map_data_packet.h"
 #include "game_io/game_interactive_player_data_packet.h"
-#include "game_io/game_map_data_packet.h"
 #include "game_io/game_player_economies_data_packet.h"
 #include "game_io/game_player_info_data_packet.h"
 #include "game_io/game_preload_data_packet.h"
@@ -70,7 +70,7 @@
 	log(" done\n");
 
 	log("Game: Writing Interactive Player Data ... ");
-	{Game_Interactive_Player_Data_Packet         p; p.Write(m_fs, m_game, mos);}
+	{Game_Interactive_Player_Data_Paquet         p; p.Write(m_fs, m_game, mos);}
 	log(" done\n");
 }
 

=== modified file 'src/scripting/lua_ui.cc'
--- src/scripting/lua_ui.cc	2013-07-26 20:19:36 +0000
+++ src/scripting/lua_ui.cc	2013-08-01 08:24:32 +0000
@@ -25,6 +25,7 @@
 #include "logic/player.h"
 #include "scripting/c_utils.h"
 #include "scripting/lua_map.h"
+#include "scripting/luna.h"
 #include "upcast.h"
 #include "wui/interactive_player.h"
 
@@ -525,6 +526,12 @@
 	L_Panel(get_egbase(L).get_ibase()) {
 }
 
+void L_MapView::__unpersist(lua_State* L)
+{
+	Widelands::Game & game = get_game(L);
+	m_panel = game.get_ibase();
+}
+
 
 /*
  * Properties

=== modified file 'src/scripting/lua_ui.h'
--- src/scripting/lua_ui.h	2013-07-26 20:19:36 +0000
+++ src/scripting/lua_ui.h	2013-08-01 08:24:32 +0000
@@ -179,6 +179,9 @@
 	L_MapView(lua_State * L);
 	virtual ~L_MapView() {}
 
+	virtual void __persist(lua_State * L) {}
+	virtual void __unpersist(lua_State * L);
+
 	/*
 	 * Properties
 	 */

=== modified file 'test/lua/persistence.wmf/scripting/init.lua'
--- test/lua/persistence.wmf/scripting/init.lua	2013-07-19 17:07:41 +0000
+++ test/lua/persistence.wmf/scripting/init.lua	2013-08-01 08:24:32 +0000
@@ -51,6 +51,12 @@
       map:get_field(10,10), map:get_field(10,10), map:get_field(10,11)
    }
 
+   mapview = wl.ui.MapView()
+   mapview.viewpoint_x = 10
+   mapview.viewpoint_y = 40
+   mapview.statistics = false
+   mapview.census = true
+
    game:save("lua_persistence")
    print("Save requested\n");
 
@@ -121,6 +127,11 @@
    assert_equal(2, myset.size)
    assert_true(myset:contains(map:get_field(10,10)))
    assert_true(myset:contains(map:get_field(10,11)))
+   mapview = wl.ui.MapView()
+   assert_equal(mapview.viewpoint_x, 10)
+   assert_equal(mapview.viewpoint_y, 40)
+   assert_equal(false, mapview.statistics)
+   assert_equal(true, mapview.census)
 
    print("################### ALL TEST PASS!")