widelands-dev team mailing list archive

[cghislai <charlyghislain@xxxxxxxxx>]

cghislai has proposed merging lp:~widelands-dev/widelands/formerbuildings_index into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1205149 in widelands: "Crash on Ubuntu 12.04 when clicking to open a building window"
  https://bugs.launchpad.net/widelands/+bug/1205149

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/formerbuildings_index/+merge/179570

Previously former building information was stored as Building_Descr pointers.
Here it is stored as building_Index values.
It prevents the crash to happen on Ubuntu 12.04.
-- 
https://code.launchpad.net/~widelands-dev/widelands/formerbuildings_index/+merge/179570
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/formerbuildings_index into lp:widelands.

=== modified file 'src/logic/building.cc'
--- src/logic/building.cc	2013-08-08 19:55:12 +0000
+++ src/logic/building.cc	2013-08-10 12:30:44 +0000
@@ -193,8 +193,8 @@
 	Building & b = construct ? create_constructionsite() : create_object();
 	b.m_position = pos;
 	b.set_owner(&owner);
-	BOOST_FOREACH(const Building_Descr * descr, former_buildings) {
-		b.m_old_buildings.push_back(descr);
+	BOOST_FOREACH(Building_Index idx, former_buildings) {
+		b.m_old_buildings.push_back(idx);
 	}
 	if (loading) {
 		b.Building::init(egbase);

=== modified file 'src/logic/building.h'
--- src/logic/building.h	2013-08-08 19:55:12 +0000
+++ src/logic/building.h	2013-08-10 12:30:44 +0000
@@ -60,7 +60,7 @@
  */
 struct Building_Descr : public Map_Object_Descr {
 	typedef std::set<Building_Index> Enhancements;
-	typedef std::vector<const Building_Descr*> FormerBuildings;
+	typedef std::vector<Building_Index> FormerBuildings;
 
 	Building_Descr
 		(char const * _name, char const * _descname,
@@ -176,7 +176,7 @@
 		PCap_Enhancable = 1 << 2, // can be enhanced to something
 	};
 
-	typedef std::vector<const Building_Descr*> FormerBuildings;
+	typedef std::vector<Building_Index> FormerBuildings;
 
 public:
 	Building(const Building_Descr &);
@@ -240,9 +240,9 @@
 	/**
 	 * The former buildings vector keeps track of all former buildings
 	 * that have been enhanced up to the current one. The current building
-	 * descr will be in the last position. For construction sites, it is
-	 * empty except if a former building is being enhanced. For a dismantle
-	 * site, the last item will be the one being dismantled.
+	 * index will be in the last position. For construction sites, it is
+	 * empty exceptenhancements. For a dismantle site, the last item will
+	 * be the one being dismantled.
 	 */
 	const FormerBuildings get_former_buildings() {
 		return m_old_buildings;
@@ -311,7 +311,7 @@
 	// Signals connected for the option window
 	std::vector<boost::signals2::connection> options_window_connections;
 
-	// The former buildings descrs, with the current one in last position.
+	// The former buildings names, with the current one in last position.
 	FormerBuildings m_old_buildings;
 };
 

=== modified file 'src/logic/constructionsite.cc'
--- src/logic/constructionsite.cc	2013-07-26 20:19:36 +0000
+++ src/logic/constructionsite.cc	2013-08-10 12:30:44 +0000
@@ -132,7 +132,9 @@
 	const std::map<Ware_Index, uint8_t> * buildcost;
 	if (!m_old_buildings.empty()) {
 		// Enhancement
-		m_info.was = m_old_buildings.back();
+		Building_Index was_index = m_old_buildings.back();
+		const Building_Descr* was_descr = tribe().get_building_descr(was_index);
+		m_info.was = was_descr;
 		buildcost = &m_building->enhancement_cost();
 	} else {
 		buildcost = &m_building->buildcost();
@@ -169,7 +171,8 @@
 
 	if (m_work_steps <= m_work_completed) {
 		// Put the real building in place
-		m_old_buildings.push_back(m_building);
+		Building_Index becomes_idx = tribe().building_index(m_building->name());
+		m_old_buildings.push_back(becomes_idx);
 		Building & b =
 			m_building->create(egbase, owner(), m_position, false, false, m_old_buildings);
 		if (Worker * const builder = m_builder.get(egbase)) {
@@ -387,7 +390,8 @@
 		//  draw the prev pic from top to where next image will be drawing
 		dst.drawanimrect(pos, anim, tanim - FRAME_LENGTH, get_owner(), Rect(Point(0, 0), w, h - lines));
 	else if (!m_old_buildings.empty()) {
-		const Building_Descr* prev_building = m_old_buildings.back();
+		Building_Index prev_idx = m_old_buildings.back();
+		const Building_Descr* prev_building = tribe().get_building_descr(prev_idx);
 		//  Is the first picture but there was another building here before,
 		//  get its most fitting picture and draw it instead.
 		uint32_t a;

=== modified file 'src/logic/dismantlesite.cc'
--- src/logic/dismantlesite.cc	2013-07-28 09:02:37 +0000
+++ src/logic/dismantlesite.cc	2013-08-10 12:30:44 +0000
@@ -71,10 +71,11 @@
 Partially_Finished_Building(gdescr)
 {
 	assert(!former_buildings.empty());
-	BOOST_FOREACH(const Building_Descr* old_descr, former_buildings) {
-		m_old_buildings.push_back(old_descr);
+	BOOST_FOREACH(Building_Index former_idx, former_buildings) {
+		m_old_buildings.push_back(former_idx);
 	}
-	set_building(*m_old_buildings.back());
+	const Building_Descr* cur_descr = owner().tribe().get_building_descr(m_old_buildings.back());
+	set_building(*cur_descr);
 
 	m_position = c;
 	set_owner(&plr);
@@ -133,9 +134,10 @@
 	(Building* building,
 	 std::map<Ware_Index, uint8_t>   & res)
 {
-	BOOST_FOREACH(const Building_Descr* former_descr, building->get_former_buildings()) {
+	BOOST_FOREACH(Building_Index former_idx, building->get_former_buildings()) {
 		const std::map<Ware_Index, uint8_t> * return_wares;
-		if (former_descr != building->get_former_buildings().front()) {
+		const Building_Descr* former_descr = building->tribe().get_building_descr(former_idx);
+		if (former_idx != building->get_former_buildings().front()) {
 			return_wares = & former_descr->returned_wares_enhanced();
 		} else {
 			return_wares = & former_descr->returned_wares();

=== modified file 'src/logic/militarysite.cc'
--- src/logic/militarysite.cc	2013-08-08 19:55:12 +0000
+++ src/logic/militarysite.cc	2013-08-10 12:30:44 +0000
@@ -867,8 +867,9 @@
 
 		// Add suffix to all descr in former buildings in cases
 		// the new owner comes from another tribe
-		Building_Descr::FormerBuildings former_buildings;
-		BOOST_FOREACH(const Building_Descr * old_descr, m_old_buildings) {
+		Building::FormerBuildings former_buildings;
+		BOOST_FOREACH(Building_Index former_idx, m_old_buildings) {
+			const Building_Descr * old_descr = tribe().get_building_descr(former_idx);
 			std::string bldname = old_descr->name();
 			// Has this building already a suffix? == conquered building?
 			std::string::size_type const dot = bldname.rfind('.');
@@ -879,8 +880,7 @@
 			} else if (enemytribe.name() == bldname.substr(dot + 1, bldname.size()))
 				bldname = bldname.substr(0, dot);
 			Building_Index bldi = enemytribe.safe_building_index(bldname.c_str());
-			const Building_Descr * former_descr = enemytribe.get_building_descr(bldi);
-			former_buildings.push_back(former_descr);
+			former_buildings.push_back(bldi);
 		}
 
 		// Now we destroy the old building before we place the new one.

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2013-08-08 20:02:13 +0000
+++ src/logic/player.cc	2013-08-10 12:30:44 +0000
@@ -110,19 +110,18 @@
  */
 void find_former_buildings
 	(const Widelands::Tribe_Descr & tribe_descr, const Widelands::Building_Index bi,
-	 Widelands::Building_Descr::FormerBuildings* former_buildings)
+	 Widelands::Building::FormerBuildings* former_buildings)
 {
 	assert(former_buildings && former_buildings->empty());
-	const Widelands::Building_Descr * first_descr = tribe_descr.get_building_descr(bi);
-	former_buildings->push_back(first_descr);
+	former_buildings->push_back(bi);
 	bool done = false;
 	while (not done) {
-		const Widelands::Building_Descr * oldest = former_buildings->front();
+		Widelands::Building_Index oldest_idx = former_buildings->front();
+		const Widelands::Building_Descr * oldest = tribe_descr.get_building_descr(oldest_idx);
 		if (!oldest->is_enhanced()) {
 			done = true;
 			break;
 		}
-		const Widelands::Building_Index & oldest_idx = tribe_descr.building_index(oldest->name());
 		for
 			(Widelands::Building_Index i = Widelands::Building_Index::First();
 			 i < tribe_descr.get_nrbuildings();
@@ -130,7 +129,7 @@
 		{
 			const Widelands::Building_Descr* ob = tribe_descr.get_building_descr(i);
 			if (ob->enhancements().count(oldest_idx)) {
-				former_buildings->insert(former_buildings->begin(), ob);
+				former_buildings->insert(former_buildings->begin(), i);
 				break;
 			}
 		}
@@ -506,7 +505,8 @@
 	 const Building_Descr::FormerBuildings & former_buildings)
 {
 	Map & map = egbase().map();
-	const Building_Descr* descr = former_buildings.back();
+	Building_Index idx = former_buildings.back();
+	const Building_Descr* descr = tribe().get_building_descr(idx);
 	terraform_for_building(egbase(), player_number(), location, descr);
 	FCoords flag_loc;
 	map.get_brn(map.get_fcoords(location), &flag_loc);
@@ -522,7 +522,8 @@
 	 const Building_Descr::FormerBuildings & former_buildings)
 {
 	Map & map = egbase().map();
-	const Building_Descr * descr = tribe().get_building_descr(b_idx);
+	Building_Index idx = former_buildings.back();
+	const Building_Descr* descr = tribe().get_building_descr(idx);
 	terraform_for_building(egbase(), player_number(), location, descr);
 	FCoords flag_loc;
 	map.get_brn(map.get_fcoords(location), &flag_loc);

=== modified file 'src/logic/player.h'
--- src/logic/player.h	2013-08-01 10:51:41 +0000
+++ src/logic/player.h	2013-08-10 12:30:44 +0000
@@ -31,6 +31,7 @@
 #include "logic/warehouse.h"
 #include "logic/widelands.h"
 
+class Node;
 namespace Widelands {
 
 class Economy;
@@ -398,15 +399,14 @@
 		throw ();
 
 	/// Call see_node for each node in the area.
-	void see_area(const Area<FCoords>& area)
-		throw ()
-	{
+	void see_area(const Area<FCoords>& area) throw () {
 		const Time gametime = egbase().get_gametime();
 		const Map & map = egbase().map();
 		const Widelands::Field & first_map_field = map[0];
 		MapRegion<Area<FCoords> > mr(map, area);
-		do see_node(map, first_map_field, mr.location(), gametime);
-		while (mr.advance(map));
+		do {
+			see_node(map, first_map_field, mr.location(), gametime);
+		} while (mr.advance(map));
 		m_view_changed = true;
 	}
 
@@ -458,12 +458,12 @@
 	Road * build_road(const Path &); /// Build a road if it is allowed.
 	Building & force_building
 		(const Coords,
-		 const Building_Descr::FormerBuildings &);
+		 const Building::FormerBuildings &);
 	Building & force_csite
 		(const Coords,
 		 Building_Index,
-		 const Building_Descr::FormerBuildings & = Building_Descr::FormerBuildings());
-	Building * build(Coords, Building_Index, bool, Building_Descr::FormerBuildings &);
+		 const Building::FormerBuildings & = Building::FormerBuildings());
+	Building * build(Coords, Building_Index, bool, Building::FormerBuildings &);
 	void bulldoze(PlayerImmovable &, bool recurse = false);
 	void flagaction(Flag &);
 	void start_stop_building(PlayerImmovable &);
@@ -649,7 +649,7 @@
 
 void find_former_buildings
 	(const Tribe_Descr & tribe_descr, const Building_Index bi,
-	 Building_Descr::FormerBuildings* former_buildings);
+	 Building::FormerBuildings* former_buildings);
 
 }
 

=== modified file 'src/logic/playercommand.cc'
--- src/logic/playercommand.cc	2013-08-07 12:32:36 +0000
+++ src/logic/playercommand.cc	2013-08-10 12:30:44 +0000
@@ -226,7 +226,7 @@
 void Cmd_Build::execute (Game & game)
 {
 	// Empty former vector since its a new csite.
-	Building_Descr::FormerBuildings former_buildings;
+	Building::FormerBuildings former_buildings;
 	game.player(sender()).build(coords, bi, true, former_buildings);
 }
 

=== modified file 'src/map_io/widelands_map_building_data_packet.cc'
--- src/map_io/widelands_map_building_data_packet.cc	2013-07-31 17:00:28 +0000
+++ src/map_io/widelands_map_building_data_packet.cc	2013-08-10 12:30:44 +0000
@@ -87,8 +87,7 @@
 							if (special_type == 1) { // Constructionsite
 								building = &egbase.warp_constructionsite(c, p, index, true);
 							} else if (special_type == 2) {// DismantleSite
-								const Building_Descr* former_desc = tribe.get_building_descr(index);
-								Building::FormerBuildings formers = {former_desc};
+								Building::FormerBuildings formers = {index};
 								building = &egbase.warp_dismantlesite(c, p, true, formers);
 							} else {
 								building = &egbase.warp_building(c, p, index);

=== modified file 'src/map_io/widelands_map_buildingdata_data_packet.cc'
--- src/map_io/widelands_map_buildingdata_data_packet.cc	2013-08-01 03:23:11 +0000
+++ src/map_io/widelands_map_buildingdata_data_packet.cc	2013-08-10 12:30:44 +0000
@@ -153,13 +153,15 @@
 						// will be built after other data are loaded, see below.
 						// read_formerbuildings_v2()
 						while (fr.Unsigned8()) {
-							const Building_Descr* former_descr =
-								building.descr().tribe().get_building_descr
-								(building.descr().tribe().safe_building_index(fr.CString()));
-							building.m_old_buildings.push_back(former_descr);
+							Building_Index oldidx = building.descr().tribe().safe_building_index(fr.CString());
+							building.m_old_buildings.push_back(oldidx);
 						}
 						// Only construction sites may have an empty list
-						assert(!building.m_old_buildings.empty() || is_a(ConstructionSite, &building));
+						if (building.m_old_buildings.empty() && !is_a(ConstructionSite, &building)) {
+							throw game_data_error
+								("Failed to read %s %u: No former buildings informations.\n"
+								"Your savegame is corrupted", building.descr().descname().c_str(), building.serial());
+						}
 					}
 					if (fr.Unsigned8()) {
 						if (upcast(ProductionSite, productionsite, &building))
@@ -251,12 +253,14 @@
 void Map_Buildingdata_Data_Packet::read_formerbuildings_v2
 	(Building& b, FileRead&, Game&, Map_Map_Object_Loader&)
 {
+	const Tribe_Descr & t = b.descr().tribe();
+	Building_Index b_idx = t.building_index(b.descr().name());
 	if (is_a(ProductionSite, &b)) {
 		assert(b.m_old_buildings.empty());
-		b.m_old_buildings.push_back(&b.descr());
+		b.m_old_buildings.push_back(b_idx);
 	} else if (is_a(Warehouse, &b)) {
 		assert(b.m_old_buildings.empty());
-		b.m_old_buildings.push_back(&b.descr());
+		b.m_old_buildings.push_back(b_idx);
 	} else if (upcast(DismantleSite, dsite, &b)) {
 		// Former buildings filled with the current one
 		// upon building init.
@@ -269,17 +273,16 @@
 	}
 
 	// iterate through all buildings to find first predecessor
-	const Tribe_Descr & t = b.descr().tribe();
 	for (;;) {
-		const Building_Descr * oldest = b.m_old_buildings.front();
+		Building_Index former_idx = b.m_old_buildings.front();
+		const Building_Descr * oldest = t.get_building_descr(former_idx);
 		if (!oldest->is_enhanced()) {
 			break;
 		}
-		const Building_Index & oldest_idx = t.building_index(oldest->name());
 		for (Building_Index i = Building_Index::First(); i < t.get_nrbuildings(); ++i) {
 			Building_Descr const * ob = t.get_building_descr(i);
-			if (ob->enhancements().count(oldest_idx)) {
-				b.m_old_buildings.insert(b.m_old_buildings.begin(), ob);
+			if (ob->enhancements().count(former_idx)) {
+				b.m_old_buildings.insert(b.m_old_buildings.begin(), i);
 				break;
 			}
 		}
@@ -371,10 +374,8 @@
 
 			if (packet_version <= 2) {
 				if (fr.Unsigned8()) {
-					const Building_Descr* former_descr =
-						tribe.get_building_descr
-						(tribe.safe_building_index(fr.CString()));
-					constructionsite.m_old_buildings.push_back(former_descr);
+					Building_Index idx = tribe.safe_building_index(fr.CString());
+					constructionsite.m_old_buildings.push_back(idx);
 				}
 			}
 
@@ -397,10 +398,8 @@
 	constructionsite.m_building =
 		tribe.get_building_descr(tribe.safe_building_index(fr.CString()));
 	if (fr.Unsigned8()) {
-		const Building_Descr * former_descr =
-			tribe.get_building_descr
-			(tribe.safe_building_index(fr.CString()));
-		constructionsite.m_old_buildings.push_back(former_descr);
+		Building_Index bidx = tribe.safe_building_index(fr.CString());
+		constructionsite.m_old_buildings.push_back(bidx);
 	}
 
 	delete constructionsite.m_builder_request;
@@ -1318,9 +1317,11 @@
 				fw.Unsigned32(0);
 			}
 			{
-				BOOST_FOREACH(const Building_Descr* former_descr, building->m_old_buildings) {
+				const Tribe_Descr& td = building->descr().tribe();
+				BOOST_FOREACH(Building_Index b_idx, building->m_old_buildings) {
+					const Building_Descr* b_descr = td.get_building_descr(b_idx);
 					fw.Unsigned8(1);
-					fw.String(former_descr->name());
+					fw.String(b_descr->name());
 				}
 				fw.Unsigned8(0);
 			}