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

[Teppo Mäenpää]

Adds version data to map files.

not done yet:
- Bundled maps would not have version information, yet.
- Associated code with Widelands web page not even started yet.
- Version data is not displayed to the player anywhere.

See the bug discussion thread for more info.

[SirVer (sirver)]

I only did a quick review for now - I think this can go into b18, but it needs some cleanups.

I suggest to refactor the map versioning stuff into a class of its own instead of having it all in Map. Map than only contains one member of this new class type. Map already has too many responsabilities. Also, the map could contain N of these to keep all revisions information around if this is desired - the original_author feels a little weird otherwise imho.

- m_map_source_url.clear(); and the line below are not necessary. strings start out empty.

- + * Copyright (C) 2002-2004, 2006-2008, 2010 by the Widelands Development Team
this is outdated. it's 2013.

- +#include <SDL_image.h>
in the new file this is not needed - some other includes might not be needed either. Please check this.

- there is already packet_version, why do you need packet_compatibility?

- widelands_map_version_data_packet.h
copyright and comments are wrong.

review: Needs Fixing

[Teppo Mäenpää (kxq)]

The _compatibility is needed for forward compatibility. If one would, for example, add a compelete history to map revision (suggestion of yours), in a way where the latest is store as before, then the older versions of the program could still read the map.

The alternative would lead to maps that cannot be loaded because of revision history. People downloading maps for their latest release would be less happy. I of course cannot know, but I would guess that people making maps also are more likely to download development snapshots than those just playing downloaded maps.

[Teppo Mäenpää (kxq)]

I tested yesterday's changes. No obvious errors popped up..

review: Needs Resubmitting

[SirVer (sirver)]

I think this looks good codewise now. Do you have plans to add the packet to the currently shipped maps or will this be delayed till after b18? Note that we might have this for some maps then (which are changed in the next few days) but not for others - not a big problem as long as all formats still load of course. So what are your plans for this now?

I have a few more nits. Feel free to merge this to trunk when you have addressed those.

map_version -> typenames are camlcased in Widelands. Either Map_Version, but much rather MapVersion (the first style is still all over Widelands, but is rather silly and very uncommon in the rest of the world).
m_map_source_url.clear() -> the clears in the constructor are unnecessary.
class map_version -> should be a struct MapVersion as you have no member functions and it really is a plain struct (minus the constructor, which is acceptable imho). If you have plans to make this more complex in the future though it might make sense to add getters/setters right now. Your call.

Thanks for taking ownership of this problem - I gladly see it go away.

[SirVer (sirver)]

review: Approve

[Nasenbaer (nasenbaer)]

Your code looks good so far, however I disagree concerning "The _compatibility" - there is no need for a second variable - the compatibility is already defined through the packet version - to _compatibility is really not needed.

Further it would be great to have a real use case - a message box that tells the user if a map is incompatible. The case, where this will happen the most simply is, if a map was created with a newer widelands version. Therefore this would be great to have in build18 - maybe we can make it work until tomorrow evening :).

My idea would be to add up all packet versions of every packet of the map and use that number as compatibility check -> as soon as said number is higher in the map to be loaded than the same number of the used widelands build, the map is definitely incompatible, as at least one of the map packets has a newer version number. Widelands should now show a pop up telling the user that this map is incompatible and the map should not be selectable.

just my 5¢ ... however I have to come back to my first sentence: You did a good job here! I would just like to further improve it. :)

review: Needs Fixing

[SirVer (sirver)]

the _compatibility is needed for forward compatibility - i.e. b18 will be able to read maps that are made with 'later' versions too if we are careful. This is a good thing to have for maps I think.

And having this in maps now is useful in the future - so while a use case would be nice, I do not see it happening till tomorrow evening. But having b18 maps contain a version string might be useful for this feature in the future.

So I am still +1 for merging as is right now and go from there.

[Teppo Mäenpää (kxq)]

I had only this version file and the worries of SirVer, expressed in the bug discusssion, in mind while making the _compatibility flag.

I agree that a global forward compatibility handling would be useful, even for savegames, but that would require one additional processing layer to saving and loading. Since slowing down saving harms playing experience, I won't even start anything like that despite I in a way would like to.

Maybe the discussion should continue in bug #1210892 page?

=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2013-09-03 18:46:02 +0000
+++ src/logic/map.cc	2013-09-15 04:43:13 +0000
@@ -22,6 +22,7 @@
 #include <algorithm>
 #include <cstdio>
 
+#include "build_info.h"
 #include "economy/flag.h"
 #include "economy/road.h"
 #include "editor/tools/editor_increase_resources_tool.h"
@@ -44,7 +45,6 @@
 #include "wui/overlay_manager.h"
 
 
-
 namespace Widelands {
 
 

=== modified file 'src/logic/map.h'
--- src/logic/map.h	2013-07-26 20:19:36 +0000
+++ src/logic/map.h	2013-09-15 04:43:13 +0000
@@ -30,6 +30,7 @@
 #include "logic/field.h"
 #include "interval.h"
 #include "manager.h"
+#include "logic/map_revision.h"
 #include "logic/notification.h"
 #include "logic/objective.h"
 #include "random.h"
@@ -124,6 +125,7 @@
 	friend struct WL_Map_Loader;
 	friend struct Map_Elemental_Data_Packet;
 	friend struct Map_Extradata_Data_Packet;
+	friend struct Map_Version_Data_Packet;
 	friend class Editor;
 	friend struct Main_Menu_New_Map;
 	friend struct MapGenerator;
@@ -433,6 +435,8 @@
 		void find_reachable(Area<FCoords>, const CheckStep &, functorT &);
 
 	template<typename functorT> void find(const Area<FCoords>, functorT &) const;
+
+	MapVersion m_map_version;
 };
 
 

=== added file 'src/logic/map_revision.cc'
--- src/logic/map_revision.cc	1970-01-01 00:00:00 +0000
+++ src/logic/map_revision.cc	2013-09-15 04:43:13 +0000
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) 2013 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 "logic/map_revision.h"
+
+#include "build_info.h"
+
+namespace Widelands {
+
+
+MapVersion::MapVersion() :
+m_map_version_major(0),
+m_map_version_minor(0)
+{
+	m_map_creator_version = build_id();
+	m_map_version_timestamp = static_cast<uint32_t>(time(NULL));
+}
+
+}

=== added file 'src/logic/map_revision.h'
--- src/logic/map_revision.h	1970-01-01 00:00:00 +0000
+++ src/logic/map_revision.h	2013-09-15 04:43:13 +0000
@@ -0,0 +1,51 @@
+/*
+ * Copyright (C) 2013 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 MAP_REVISION_H
+#define MAP_REVISION_H
+
+#include <cstring>
+#include <memory>
+#include <set>
+#include <string>
+#include <vector>
+
+namespace Widelands {
+
+
+
+
+struct MapVersion {
+
+	std::string m_map_source_url;
+	std::string m_map_source_release;
+	std::string m_map_creator_version;
+	int32_t     m_map_version_major;
+	int32_t     m_map_version_minor;
+	uint32_t    m_map_version_timestamp;
+
+	MapVersion();
+
+};
+
+}
+
+
+
+#endif

=== modified file 'src/map_io/widelands_map_loader.cc'
--- src/map_io/widelands_map_loader.cc	2013-07-26 20:19:36 +0000
+++ src/map_io/widelands_map_loader.cc	2013-09-15 04:43:13 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2002-2004, 2006-2008, 2010-2011 by the Widelands Development Team
+ * Copyright (C) 2002-2004, 2006-2008, 2010-2011, 2013 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
@@ -52,6 +52,7 @@
 #include "map_io/widelands_map_roaddata_data_packet.h"
 #include "map_io/widelands_map_scripting_data_packet.h"
 #include "map_io/widelands_map_terrain_data_packet.h"
+#include "map_io/widelands_map_version_data_packet.h"
 #include "map_io/widelands_map_ware_data_packet.h"
 #include "map_io/widelands_map_waredata_data_packet.h"
 #include "warning.h"
@@ -198,6 +199,11 @@
 	{Map_Extradata_Data_Packet      p; p.Read(m_fs, egbase, !scenario, *m_mol);}
 	log("done!\n ");
 
+	log("Reading Map Version Data ... ");
+	{Map_Version_Data_Packet      p; p.Read(m_fs, egbase, !scenario, *m_mol);}
+	log("done!\n ");
+
+
 	log("Reading Allowed Worker Types Data ... ");
 	{
 		Map_Allowed_Worker_Types_Data_Packet p;

=== modified file 'src/map_io/widelands_map_saver.cc'
--- src/map_io/widelands_map_saver.cc	2013-07-26 20:19:36 +0000
+++ src/map_io/widelands_map_saver.cc	2013-09-15 04:43:13 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2002-2004, 2006-2011 by the Widelands Development Team
+ * Copyright (C) 2002-2004, 2006-2011, 2013 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
@@ -49,6 +49,7 @@
 #include "map_io/widelands_map_roaddata_data_packet.h"
 #include "map_io/widelands_map_scripting_data_packet.h"
 #include "map_io/widelands_map_terrain_data_packet.h"
+#include "map_io/widelands_map_version_data_packet.h"
 #include "wexception.h"
 
 namespace Widelands {
@@ -114,6 +115,12 @@
 	{Map_Extradata_Data_Packet               p; p.Write(m_fs, m_egbase, *m_mos);}
 	log("done!\n ");
 
+	log("Writing Map Version ... ");
+	{Map_Version_Data_Packet               p; p.Write(m_fs, m_egbase, *m_mos);}
+	log("done!\n ");
+
+
+
 	const Map & map = m_egbase.map();
 
 	Player_Number const nr_players = map.get_nrplayers();

=== added file 'src/map_io/widelands_map_version_data_packet.cc'
--- src/map_io/widelands_map_version_data_packet.cc	1970-01-01 00:00:00 +0000
+++ src/map_io/widelands_map_version_data_packet.cc	2013-09-15 04:43:13 +0000
@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2013 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 "map_io/widelands_map_version_data_packet.h"
+
+#include "build_info.h"
+#include "logic/editor_game_base.h"
+#include "logic/game_data_error.h"
+#include "logic/map.h"
+#include "logic/widelands_fileread.h"
+#include "logic/widelands_filewrite.h"
+#include "profile/profile.h"
+
+namespace Widelands {
+
+#define CURRENT_PACKET_VERSION 1
+
+
+void Map_Version_Data_Packet::Read
+	(FileSystem            &       fs,
+	 Editor_Game_Base      &       egbase,
+	 bool                    const skip,
+	 Map_Map_Object_Loader &)
+throw (_wexception)
+{
+	if (skip)
+		return;
+
+	Profile prof;
+	try {prof.read("version", 0, fs);} catch (...)
+		{
+			Map & map = egbase.map();
+			map.m_map_version.m_map_version_timestamp = 0;
+			map.m_map_version.m_map_creator_version = "unknown";
+			return;
+		}
+
+	try {
+		Section & globv = prof.get_safe_section("global");
+		int32_t const packet_version =
+			globv.get_safe_int("packet_version");
+		int32_t const forward_compatibility =
+			globv.get_safe_int("packet_compatibility");
+		if
+		((packet_version == CURRENT_PACKET_VERSION)
+			|| (packet_version > CURRENT_PACKET_VERSION && forward_compatibility <= CURRENT_PACKET_VERSION))
+		{
+			Map & map = egbase.map();
+			map.m_map_version.m_map_source_url = globv.get_safe_string("map_source_url");
+			map.m_map_version.m_map_source_release = globv.get_safe_string("map_release");
+			map.m_map_version.m_map_creator_version = globv.get_safe_string("map_creator_version");
+			map.m_map_version.m_map_version_major = globv.get_safe_int("map_version_major");
+			map.m_map_version.m_map_version_minor = globv.get_safe_int("map_version_minor");
+			uint32_t ts = static_cast<uint32_t>(globv.get_safe_int("map_version_timestamp"));
+			map.m_map_version.m_map_version_timestamp = ts;
+		} else
+			throw game_data_error
+				(_("unknown/unhandled version %u"), packet_version);
+	} catch (const _wexception & e) {
+		throw game_data_error(_("version: %s"), e.what());
+	}
+}
+
+
+void Map_Version_Data_Packet::Write
+	(FileSystem & fs, Editor_Game_Base & egbase, Map_Map_Object_Saver &)
+throw (_wexception)
+{
+	Profile prof;
+	Section & globs = prof.create_section("global");
+
+	// This writes the map revision information to savegame.
+	// revision information is put into a separate file, assuming that
+	// revision information for bundled maps would be written to the maps
+	// on-the-fly by the build script. Therefore, we need a file which
+	// will not go to Software Configuration Management.
+
+	// Maps come from three different sources:
+	// - User makes those
+	// - Maps are downloaded from widelands.org webpage
+	// - Maps are bundled with releases.
+	//
+	// For maps that are downloaded from website,
+	// map_source_url should be non-zero. I assume that
+	// it is always widelands.org, but chose to put an url there for completeness.
+	//
+	// For maps bundled with a release, map_source_release should be non-empty,
+	// preferably listing the release that the map came with.
+	// For a map made by user, map_creator_version should list the version
+	// that the map was done with.
+	//
+	// If there are many maps with a same name, the major version should be stepped.
+	// This is mostly intended for maps downloaded from widelands.org
+	// I also include minor number which is stepped at each save, and a timestamp.
+	//
+	// Note -- None of the version numbers are displayed anywhere. I intend to do something
+	// about that in the future, and only make these pieces of data now so that most
+	// running copies of widelands would be compatible and relay these data forward!
+	//
+	// For now, these are meaningless. Let's hope it will not stay that way!
+
+	// FIXME -- we could store the unix time in uint32, as a partial fix to 2038 problem.
+	// There seems to be a get_safe_natural method, but not corresponding setter.
+
+	Map & map = egbase.map();
+	globs.set_string("map_source_url", map.m_map_version.m_map_source_url);
+	globs.set_string("map_release", map.m_map_version.m_map_source_release);
+	globs.set_string("map_creator_version", map.m_map_version.m_map_creator_version);
+	globs.set_int("map_version_major", map.m_map_version.m_map_version_major);
+	globs.set_int("map_version_minor", 1 + map.m_map_version.m_map_version_minor);
+	globs.set_int("map_version_timestamp", static_cast<uint32_t>(time(NULL)));
+	globs.set_int("packet_version", CURRENT_PACKET_VERSION);
+	globs.set_int("packet_compatibility", CURRENT_PACKET_VERSION);
+
+	prof.write("version", false, fs);
+}
+
+}

=== added file 'src/map_io/widelands_map_version_data_packet.h'
--- src/map_io/widelands_map_version_data_packet.h	1970-01-01 00:00:00 +0000
+++ src/map_io/widelands_map_version_data_packet.h	2013-09-15 04:43:13 +0000
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2013 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 WIDELANDS_MAP_VERSION_DATA_PACKET_H
+#define WIDELANDS_MAP_VERSION_DATA_PACKET_H
+
+#include "map_io/widelands_map_data_packet.h"
+
+/*
+ * This packet contains the version information of the map.
+ */
+MAP_DATA_PACKET(Map_Version_Data_Packet);
+
+#endif