widelands-dev team mailing list archive

[cghislai <charlyghislain@xxxxxxxxx>]

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

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #1205010 in widelands: "segfault on dismantle enemy building"
  https://bugs.launchpad.net/widelands/+bug/1205010

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

This fixes the linked bug. When a building is conquered, it goes through all Building_Descr in the former_buildings vector (the previous enhancements), add the suffix and reload from the conquering tribe. This allows correct computation of returned wares on dismantle and prevent the crash.

The force_building function signature changed and I split the construction site case in a force_csite function. I updated the code wherever i could grep its use.

I tested conquering building from the same tribe or another one. In the former case, in case of enhanced building, the correct amount of ware was computed for dismantling.
-- 
https://code.launchpad.net/~widelands-dev/widelands/bug1205010/+merge/177137
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug1205010 into lp:widelands.

=== modified file 'src/logic/militarysite.cc'
--- src/logic/militarysite.cc	2013-07-24 09:19:24 +0000
+++ src/logic/militarysite.cc	2013-07-26 11:30:40 +0000
@@ -864,23 +864,30 @@
 		// the old location.
 		Player            * enemyplayer = enemy.get_owner();
 		const Tribe_Descr & enemytribe  = enemyplayer->tribe();
-		std::string bldname = name();
 
-		// Has this building already a suffix? == conquered building?
-		std::string::size_type const dot = bldname.rfind('.');
-		if (dot >= bldname.size()) {
-			// Add suffix, if the new owner uses another tribe than we.
-			if (enemytribe.name() != owner().tribe().name())
-				bldname += "." + owner().tribe().name();
-		} 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());
+		// 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) {
+			std::string bldname = old_descr->name();
+			// Has this building already a suffix? == conquered building?
+			std::string::size_type const dot = bldname.rfind('.');
+			if (dot >= bldname.size()) {
+				// Add suffix, if the new owner uses another tribe than we.
+				if (enemytribe.name() != owner().tribe().name())
+					bldname += "." + owner().tribe().name();
+			} 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);
+		}
 
 		// Now we destroy the old building before we place the new one.
 		set_defeating_player(enemy.owner().player_number());
 		schedule_destroy(game);
 
-		enemyplayer->force_building(coords, bldi);
+		enemyplayer->force_building(coords, former_buildings);
 		BaseImmovable * const newimm = game.map()[coords].get_immovable();
 		upcast(MilitarySite, newsite, newimm);
 		newsite->reinit_after_conqueration(game);

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2013-07-22 07:26:07 +0000
+++ src/logic/player.cc	2013-07-26 11:30:40 +0000
@@ -404,11 +404,8 @@
 	return Road::create(egbase(), start, end, path);
 }
 
-
-Building & Player::force_building
-	(Coords                const location,
-	 Building_Index        const idx,
-	 bool                  constructionsite)
+void Player::prepare_terrain_for_building
+	(Coords const location, const Building_Descr * descr)
 {
 	Map & map = egbase().map();
 	FCoords c[4]; //  Big buildings occupy 4 locations.
@@ -417,10 +414,9 @@
 	force_flag(c[1]);
 	if (BaseImmovable * const immovable = c[0].field->get_immovable())
 		immovable->remove(egbase());
-	const Building_Descr & descr = *tribe().get_building_descr(idx);
 	{
 		size_t nr_locations = 1;
-		if ((descr.get_size() & BUILDCAPS_SIZEMASK) == BUILDCAPS_BIG) {
+		if ((descr->get_size() & BUILDCAPS_SIZEMASK) == BUILDCAPS_BIG) {
 			nr_locations = 4;
 			map.get_trn(c[0], &c[1]);
 			map.get_tln(c[0], &c[2]);
@@ -437,12 +433,32 @@
 				immovable->remove(egbase());
 		}
 	}
-
-	if (constructionsite)
-		return egbase().warp_constructionsite(c[0], m_plnum, idx);
-	else
-		return descr.create (egbase(), *this, c[0], false);
-}
+}
+
+Building & Player::force_building
+	(Coords                const location,
+	 Building_Descr::FormerBuildings former_buildings)
+{
+	Map & map = egbase().map();
+	const Building_Descr* descr = former_buildings.back();
+	prepare_terrain_for_building(location, descr);
+	return
+		descr->create
+			(egbase(), *this, map.get_fcoords(location), false, false, former_buildings);
+}
+
+Building& Player::force_csite
+	(Coords const location, Building_Index b_idx,
+	 Building_Descr::FormerBuildings former_buildings)
+{
+	Map & map = egbase().map();
+	const Building_Descr * descr = tribe().get_building_descr(b_idx);
+	prepare_terrain_for_building(location, descr);
+	return
+		egbase().warp_constructionsite
+			(map.get_fcoords(location), m_plnum, b_idx, false, former_buildings);
+}
+
 
 
 /*

=== modified file 'src/logic/player.h'
--- src/logic/player.h	2013-07-08 03:35:09 +0000
+++ src/logic/player.h	2013-07-26 11:30:40 +0000
@@ -455,8 +455,11 @@
 	Road * build_road(const Path &); /// Build a road if it is allowed.
 	Building & force_building
 		(Coords,
+		 Building_Descr::FormerBuildings);
+	Building & force_csite
+		(Coords,
 		 Building_Index,
-		 bool = false);
+		 Building_Descr::FormerBuildings = Building_Descr::FormerBuildings());
 	Building * build(Coords, Building_Index, bool = true);
 	void bulldoze(PlayerImmovable &, bool recurse = false);
 	void flagaction(Flag &);
@@ -572,6 +575,8 @@
 	void play_message_sound(const std::string & sender);
 	void _enhance_or_dismantle
 		(Building *, Building_Index const index_of_new_building = Building_Index::Null());
+	// Used in force_building / force_csite
+	void prepare_terrain_for_building(Coords location, const Building_Descr * descr);
 
 private:
 	MessageQueue           m_messages;

=== modified file 'src/logic/ship.cc'
--- src/logic/ship.cc	2013-07-21 08:27:10 +0000
+++ src/logic/ship.cc	2013-07-26 11:30:40 +0000
@@ -658,7 +658,8 @@
 /// @note only called via player command
 void Ship::exp_construct_port (Game &, const Coords& c) {
 	assert(m_expedition);
-	m_economy->owner().force_building(c, m_economy->owner().tribe().safe_building_index("port"), true);
+	Building_Index port_idx = m_economy->owner().tribe().safe_building_index("port");
+	m_economy->owner().force_csite(c, port_idx);
 	m_ship_state = EXP_COLONIZING;
 }
 

=== modified file 'src/scripting/lua_bases.cc'
--- src/scripting/lua_bases.cc	2012-06-08 22:33:16 +0000
+++ src/scripting/lua_bases.cc	2013-07-26 11:30:40 +0000
@@ -394,13 +394,21 @@
 		return report_error(L, "Unknown Building: '%s'", name);
 
 	Building * b = 0;
-	if (force)
-		b = &get(L, get_egbase(L)).force_building
-			(c->coords(), i, constructionsite);
-	else
+	if (force) {
+		if (constructionsite) {
+			b = &get(L, get_egbase(L)).force_csite
+				(c->coords(), i);
+		} else {
+			Building_Descr::FormerBuildings former_buildings;
+			const Building_Descr * descr = get(L, get_egbase(L)).tribe().get_building_descr(i);
+			former_buildings.push_back(descr);
+			b = &get(L, get_egbase(L)).force_building
+				(c->coords(), former_buildings);
+		}
+	} else {
 		b = get(L, get_egbase(L)).build
 			(c->coords(), i, constructionsite);
-
+	}
 	if (not b)
 		return report_error(L, "Couldn't place building!");