- catapult attacks should be identical to H3

- catapult may miss and attack another part of wall instead (this is how it works in H3)
- minor fixes

Mods/WoG/mod.json

@@ -38,7 +38,7 @@
 		"MAPS/":
 		[
 			{"type" : "dir", "path" : "/Maps"}
-		],
+		]
 	},
 
 	"name" : "In The Wake of Gods",

client/CMusicHandler.cpp

@@ -476,10 +476,9 @@ MusicEntry::~MusicEntry()
 {
 	logGlobal->traceStream()<<"Del-ing music file "<<currentName;
 	if (music)
-	{
 		Mix_FreeMusic(music);
-		SDL_FreeRW(musicFile);
+	/*if (musicFile) // Mix_FreeMusic will also free file data? Needs checking
-	}
+		SDL_FreeRW(musicFile);*/
 }
 
 void MusicEntry::load(std::string musicURI)
@@ -488,8 +487,13 @@ void MusicEntry::load(std::string musicURI)
 	{
 		logGlobal->traceStream()<<"Del-ing music file "<<currentName;
 		Mix_FreeMusic(music);
-		SDL_FreeRW(musicFile);
+		music = nullptr;
 	}
+	/*if (musicFile) // Mix_FreeMusic will also free file data? Needs checking
+	{
+		SDL_FreeRW(musicFile);
+		musicFile = nullptr;
+	}*/
 
 	currentName = musicURI;
 
@@ -502,6 +506,7 @@ void MusicEntry::load(std::string musicURI)
 	if(!music)
 	{
 		SDL_FreeRW(musicFile);
+		musicFile = nullptr;
 		logGlobal->warnStream() << "Warning: Cannot open " << currentName << ": " << Mix_GetError();
 		return;
 	}

client/battle/CBattleInterface.cpp

@@ -1189,16 +1189,16 @@ void CBattleInterface::stackIsCatapulting(const CatapultAttack & ca)
 	for(auto it = ca.attackedParts.begin(); it != ca.attackedParts.end(); ++it)
 	{
 		const CStack * stack = curInt->cb->battleGetStackByID(ca.attacker);
-		addNewAnim(new CShootingAnimation(this, stack, it->first.second, nullptr, true, it->second));
+		addNewAnim(new CShootingAnimation(this, stack, it->destinationTile, nullptr, true, it->damageDealt));
 	}
 
 	waitForAnims();
 
 	for(auto it = ca.attackedParts.begin(); it != ca.attackedParts.end(); ++it)
 	{
-		SDL_FreeSurface(siegeH->walls[it->first.first + 2]);
+		SDL_FreeSurface(siegeH->walls[it->attackedPart + 2]);
-		siegeH->walls[it->first.first + 2] = BitmapHandler::loadBitmap(
+		siegeH->walls[it->attackedPart + 2] = BitmapHandler::loadBitmap(
-			siegeH->getSiegeName(it->first.first + 2, curInt->cb->battleGetWallState(it->first.first)) );
+			siegeH->getSiegeName(it->attackedPart + 2, curInt->cb->battleGetWallState(it->attackedPart)) );
 	}
 }
 
@@ -2873,15 +2873,30 @@ CBattleInterface::SiegeHelper::~SiegeHelper()
 	}
 }
 
-std::string CBattleInterface::SiegeHelper::getSiegeName(ui16 what, ui16 additInfo) const
+std::string CBattleInterface::SiegeHelper::getSiegeName(ui16 what) const
 {
-	if(what == 2 || what == 3 || what == 8)
+	return getSiegeName(what, EWallState::INTACT);
-		vstd::amin(additInfo, 2);
+}
+
+std::string CBattleInterface::SiegeHelper::getSiegeName(ui16 what, int state) const
+{
+	auto getImageIndex = [&]() -> int
+	{
+		switch (state)
+		{
+		case EWallState::INTACT : return 1;
+		case EWallState::DAMAGED : return 2;
+		case EWallState::DESTROYED :
+			if(what == 2 || what == 3 || what == 8) // towers don't have separate image here
+				return 2;
 			else
-		vstd::amin(additInfo, 3);
+				return 3;
+		}
+		return 1;
+	};
 
 	std::string & prefix = town->town->clientInfo.siegePrefix;
-	std::string addit = boost::lexical_cast<std::string>(additInfo);
+	std::string addit = boost::lexical_cast<std::string>(getImageIndex());
 
 	switch(what)
 	{

client/battle/CBattleInterface.h

@@ -199,7 +199,13 @@ private:
 		~SiegeHelper(); //d-tor
 
 		//filename getters
-		std::string getSiegeName(ui16 what, ui16 additInfo = 1) const; //what: 0 - background, 1 - background wall, 2 - keep, 3 - bottom tower, 4 - bottom wall, 5 - below gate, 6 - over gate, 7 - upper wall, 8 - uppert tower, 9 - gate, 10 - gate arch, 11 - bottom static wall, 12 - upper static wall, 13 - moat, 14 - mlip, 15 - keep creature cover, 16 - bottom turret creature cover, 17 - upper turret creature cover; additInfo: 1 - intact, 2 - damaged, 3 - destroyed
+		//what: 0 - background,  1 - background wall,  2 - keep,          3 - bottom tower,  4 - bottom wall,
+		//      5 - below gate,  6 - over gate,        7 - upper wall,    8 - upper tower,   9 - gate,
+		//      10 - gate arch, 11 - bottom static    12 - upper static, 13 - moat,         14 - moat background,
+		//      15 - keep battlement, 16 - bottom battlement, 17 - upper battlement;
+		//      state uses EWallState enum
+		std::string getSiegeName(ui16 what) const;
+		std::string getSiegeName(ui16 what, int state) const;
 
 		void printPartOfWall(SDL_Surface * to, int what);//what: 1 - background wall, 2 - keep, 3 - bottom tower, 4 - bottom wall, 5 - below gate, 6 - over gate, 7 - upper wall, 8 - uppert tower, 9 - gate, 10 - gate arch, 11 - bottom static wall, 12 - upper static wall, 15 - keep creature cover, 16 - bottom turret creature cover, 17 - upper turret creature cover
 

config/schemas/townSiege.json

@@ -5,7 +5,7 @@
 	"title" : "VCMI siege screen format",
 	"description" : "Format used to define town siege screen in VCMI",
 	"required" : [
-		"gate", "imagePrefix", "moat", "shooterHeight", "shooter",
+		"gate", "imagePrefix", "moat", "shooter",
 		"static", "towers", "walls"
 	 ],
 	

lib/BattleState.cpp

@@ -379,9 +379,20 @@ BattleInfo * BattleInfo::setupBattle( int3 tile, ETerrainType terrain, BFieldTyp
 	//setting up siege obstacles
 	if (town && town->hasFort())
 	{
-		for (int b = 0; b < ARRAY_COUNT (curB->si.wallState); ++b)
+		for (int b = 0; b < curB->si.wallState.size(); ++b)
 		{
-			curB->si.wallState[b] = 1;
+			curB->si.wallState[b] = EWallState::INTACT;
+		}
+
+		if (!town->hasBuilt(BuildingID::CITADEL))
+		{
+			curB->si.wallState[EWallParts::KEEP] = EWallState::NONE;
+		}
+
+		if (!town->hasBuilt(BuildingID::CASTLE))
+		{
+			curB->si.wallState[EWallParts::UPPER_TOWER] = EWallState::NONE;
+			curB->si.wallState[EWallParts::BOTTOM_TOWER] = EWallState::NONE;
 		}
 	}
 

lib/BattleState.h

@@ -33,7 +33,22 @@ struct BattleStackAttacked;
 //only for use in BattleInfo
 struct DLL_LINKAGE SiegeInfo
 {
-	ui8 wallState[EWallParts::PARTS_COUNT];
+	std::array<si8, EWallParts::PARTS_COUNT> wallState;
+
+	// return EWallState decreased by value of damage points
+	static EWallState::EWallState applyDamage(EWallState::EWallState state, unsigned int value)
+	{
+		if (value == 0)
+			return state;
+
+		switch (applyDamage(state, value - 1))
+		{
+		case EWallState::INTACT    : return EWallState::DAMAGED;
+		case EWallState::DAMAGED   : return EWallState::DESTROYED;
+		case EWallState::DESTROYED : return EWallState::DESTROYED;
+		default: return EWallState::NONE;
+		}
+	}
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{

lib/CBattleCallback.cpp

@@ -54,10 +54,7 @@ namespace SiegeStuffThatShouldBeMovedToHandlers //  <=== TODO
 		return stackLeft != destLeft;
 	}
 
-	static EWallParts::EWallParts hexToWallPart(BattleHex hex)
-	{
 	//potentially attackable parts of wall
-		// -2 - indestructible walls
 	static const std::pair<int, EWallParts::EWallParts> attackable[] =
 	{
 		std::make_pair(50,  EWallParts::KEEP),
@@ -65,7 +62,7 @@ namespace SiegeStuffThatShouldBeMovedToHandlers //  <=== TODO
 		std::make_pair(182, EWallParts::BOTTOM_WALL),
 		std::make_pair(130, EWallParts::BELOW_GATE),
 		std::make_pair(62,  EWallParts::OVER_GATE),
-			std::make_pair(29,  EWallParts::UPPER_WAL),
+		std::make_pair(29,  EWallParts::UPPER_WALL),
 		std::make_pair(12,  EWallParts::UPPER_TOWER),
 		std::make_pair(95,  EWallParts::GATE),
 		std::make_pair(96,  EWallParts::GATE),
@@ -75,6 +72,8 @@ namespace SiegeStuffThatShouldBeMovedToHandlers //  <=== TODO
 		std::make_pair(147, EWallParts::INDESTRUCTIBLE_PART)
 	};
 
+	static EWallParts::EWallParts hexToWallPart(BattleHex hex)
+	{
 		for(auto & elem : attackable)
 		{
 			if(elem.first == hex)
@@ -83,6 +82,17 @@ namespace SiegeStuffThatShouldBeMovedToHandlers //  <=== TODO
 
 		return EWallParts::INVALID; //not found!
 	}
+
+	static BattleHex WallPartToHex(EWallParts::EWallParts part)
+	{
+		for(auto & elem : attackable)
+		{
+			if(elem.second == part)
+				return elem.first;
+		}
+
+		return BattleHex::INVALID; //not found!
+	}
 }
 
 using namespace SiegeStuffThatShouldBeMovedToHandlers;
@@ -410,7 +420,7 @@ ui8 CBattleInfoEssentials::battleGetWallState(int partOfWall) const
 {
 	RETURN_IF_NOT_BATTLE(0);
 	if(getBattle()->siege == CGTownInstance::NONE)
-		return 0;
+		return EWallState::NONE;
 
 	assert(partOfWall >= 0 && partOfWall < EWallParts::PARTS_COUNT);
 	return getBattle()->si.wallState[partOfWall];
@@ -1079,7 +1089,7 @@ AccessibilityInfo CBattleInfoCallback::getAccesibility() const
 	}
 
 	//gate -> should be before stacks
-	if(battleGetSiegeLevel() > 0 && battleGetWallState(EWallParts::GATE) < 3) //gate is not destroyed
+	if(battleGetSiegeLevel() > 0 && battleGetWallState(EWallParts::GATE) != EWallState::DESTROYED)
 	{
 		ret[95] = ret[96] = EAccessibility::GATE; //block gate's hexes
 	}
@@ -1113,7 +1123,7 @@ AccessibilityInfo CBattleInfoCallback::getAccesibility() const
 
 		for(auto & elem : lockedIfNotDestroyed)
 		{
-			if(battleGetWallState(elem.first) < 3)
+			if(battleGetWallState(elem.first) != EWallState::DESTROYED)
 				ret[elem.second] = EAccessibility::DESTRUCTIBLE_WALL;
 		}
 	}
@@ -1491,6 +1501,12 @@ si8 CBattleInfoCallback::battleHasDistancePenalty(const IBonusBearer *bonusBeare
 	return true;
 }
 
+BattleHex CBattleInfoCallback::wallPartToBattleHex(EWallParts::EWallParts part) const
+{
+	RETURN_IF_NOT_BATTLE(BattleHex::INVALID);
+	return WallPartToHex(part);
+}
+
 EWallParts::EWallParts CBattleInfoCallback::battleHexToWallPart(BattleHex hex) const
 {
 	RETURN_IF_NOT_BATTLE(EWallParts::INVALID);

lib/CBattleCallback.h

@@ -248,6 +248,8 @@ public:
 	si8 battleHasDistancePenalty(const IBonusBearer *bonusBearer, BattleHex shooterPosition, BattleHex destHex ) const;
 	si8 battleHasWallPenalty(const CStack * stack, BattleHex destHex) const; //checks if given stack has wall penalty
 	si8 battleHasWallPenalty(const IBonusBearer *bonusBearer, BattleHex shooterPosition, BattleHex destHex) const; //checks if given stack has wall penalty
+
+	BattleHex wallPartToBattleHex(EWallParts::EWallParts part) const;
 	EWallParts::EWallParts battleHexToWallPart(BattleHex hex) const; //returns part of destructible wall / gate / keep under given hex or -1 if not found
 
 	//*** MAGIC 

lib/CHeroHandler.cpp

@@ -418,6 +418,8 @@ void CHeroHandler::loadBallistics()
 		bli.twoDmg = ballParser.readNumber();
 		bli.sum    = ballParser.readNumber();
 		ballistics.push_back(bli);
+
+		assert(bli.noDmg + bli.oneDmg + bli.twoDmg == 100 && bli.sum == 100);
 	}
 	while (ballParser.endLine());
 }

lib/CObjectHandler.cpp

@@ -1947,7 +1947,7 @@ void CGDwelling::blockingDialogAnswered(const CGHeroInstance *hero, ui32 answer)
 		if(answer)
 			cb->startBattleI(hero, this);
 	}
-	else if(relations == PlayerRelations::SAME_PLAYER && answer)
+	else if(answer)
 	{
 		heroAcceptsCreatures(hero);
 	}

lib/GameConstants.h

@@ -444,7 +444,7 @@ namespace EWallParts
 	enum EWallParts
 	{
 		INDESTRUCTIBLE_PART = -2, INVALID = -1,
-		KEEP = 0, BOTTOM_TOWER, BOTTOM_WALL, BELOW_GATE, OVER_GATE, UPPER_WAL, UPPER_TOWER, GATE,
+		KEEP = 0, BOTTOM_TOWER, BOTTOM_WALL, BELOW_GATE, OVER_GATE, UPPER_WALL, UPPER_TOWER, GATE,
 		PARTS_COUNT
 	};
 }
@@ -453,10 +453,12 @@ namespace EWallState
 {
 	enum EWallState
 	{
-		NONE, //no wall
+		NONE = -1, //no wall
-		INTACT,
+		DESTROYED,
 		DAMAGED,
-		DESTROYED
+		INTACT
+
+
 	};
 }
 

lib/NetPacks.h

@@ -1665,14 +1665,24 @@ struct ObstaclesRemoved : public CPackForClient //3014
 
 struct CatapultAttack : public CPackForClient //3015
 {
+	struct AttackInfo
+	{
+		si16 destinationTile;
+		ui8 attackedPart;
+		ui8 damageDealt;
+
+		template <typename Handler> void serialize(Handler &h, const int version)
+		{
+			h & destinationTile & attackedPart & damageDealt;
+		}
+	};
+
 	CatapultAttack(){type = 3015;}
 
 	DLL_LINKAGE void applyGs(CGameState *gs);
 	void applyCl(CClient *cl);
 
-	std::set< std::pair< std::pair< ui8, si16 >, ui8> > attackedParts; // < <attackedPartOfWall, attacked hex >, damageDealt>
+	std::vector< AttackInfo > attackedParts;
-	//attackedPartOfWall; //[0] - keep, [1] - bottom tower, [2] - bottom wall, [3] - below gate, [4] - over gate, [5] - upper wall, [6] - uppert tower, [7] - gate;
-	//damageDealt;
 
 	int attacker; //if -1, then a spell caused this
 

lib/NetPacksLib.cpp

@@ -246,7 +246,10 @@ DLL_LINKAGE void GiveBonus::applyGs( CGameState *gs )
 		&& gs->map->objects[bonus.sid]->ID == Obj::EVENT) //it's morale/luck bonus from an event without description
 	{
 		descr = VLC->generaltexth->arraytxt[bonus.val > 0 ? 110 : 109]; //+/-%d Temporary until next battle"
+
+		// Some of(?) versions of H3 use %s here instead of %d. Try to replace both of them
 		boost::replace_first(descr,"%d",boost::lexical_cast<std::string>(std::abs(bonus.val)));
+		boost::replace_first(descr,"%s",boost::lexical_cast<std::string>(std::abs(bonus.val)));
 	}
 	else
 	{
@@ -1471,8 +1474,8 @@ DLL_LINKAGE void CatapultAttack::applyGs( CGameState *gs )
 	{
 		for(const auto &it :attackedParts)
 		{
-			gs->curB->si.wallState[it.first.first] =
+			gs->curB->si.wallState[it.attackedPart] =
-				std::min( gs->curB->si.wallState[it.first.first] + it.second, 3 );
+			        SiegeInfo::applyDamage(EWallState::EWallState(gs->curB->si.wallState[it.attackedPart]), it.damageDealt);
 		}
 	}
 }

server/CGameHandler.cpp

@@ -3513,71 +3513,109 @@ bool CGameHandler::makeBattleAction( BattleAction &ba )
 		}
 	case Battle::CATAPULT:
 		{
+			auto getCatapultHitChance = [&](EWallParts::EWallParts part, const CHeroHandler::SBallisticsLevelInfo & sbi) -> int
+			{
+				switch(part)
+				{
+				case EWallParts::GATE:
+					return sbi.gate;
+				case EWallParts::KEEP:
+					return sbi.keep;
+				case EWallParts::BOTTOM_TOWER:
+				case EWallParts::UPPER_TOWER:
+					return sbi.tower;
+				case EWallParts::BOTTOM_WALL:
+				case EWallParts::BELOW_GATE:
+				case EWallParts::OVER_GATE:
+				case EWallParts::UPPER_WALL:
+					return sbi.wall;
+				default:
+					return 0;
+				}
+			};
+
 			StartAction start_action(ba);
 			sendAndApply(&start_action);
 			const CGHeroInstance * attackingHero = gs->curB->battleGetFightingHero(ba.side);
 			CHeroHandler::SBallisticsLevelInfo sbi = VLC->heroh->ballistics[attackingHero->getSecSkillLevel(SecondarySkill::BALLISTICS)];
 
-			EWallParts::EWallParts attackedPart = gs->curB->battleHexToWallPart(ba.destinationTile);
+			EWallParts::EWallParts desiredTarget = gs->curB->battleHexToWallPart(ba.destinationTile);
-			if(attackedPart < 0)
+			if(desiredTarget < 0)
 			{
 				complain("catapult tried to attack non-catapultable hex!");
 				break;
 			}
-			int wallInitHP = gs->curB->si.wallState[attackedPart];
+
-			int dmgAlreadyDealt = 0; //in successive iterations damage is dealt but not yet subtracted from wall's HPs
+			//in successive iterations damage is dealt but not yet subtracted from wall's HPs
+			auto currentHP = gs->curB->si.wallState;
+
+			if (currentHP[desiredTarget] != EWallState::DESTROYED &&
+			    currentHP[desiredTarget] != EWallState::NONE)
+				desiredTarget = EWallParts::INVALID;
+
 			for(int g=0; g<sbi.shots; ++g)
 			{
-				if(wallInitHP + dmgAlreadyDealt == 3) //it's not destroyed
+				bool hitSuccessfull = false;
-					continue;
+				EWallParts::EWallParts attackedPart = desiredTarget;
+
+				do // catapult has chance to attack desired target. Othervice - attacks randomly
+				{
+					if(currentHP[attackedPart] != EWallState::DESTROYED && // this part can be hit
+					   currentHP[attackedPart] != EWallState::NONE &&
+					   rand()%100 < getCatapultHitChance(attackedPart, sbi))//hit is successful
+					{
+						hitSuccessfull = true;
+					}
+					else // select new target
+					{
+						std::vector<EWallParts::EWallParts> allowedTargets;
+						for (size_t i=0; i< currentHP.size(); i++)
+						{
+							if (currentHP[i] != EWallState::DESTROYED &&
+							    currentHP[i] != EWallState::NONE)
+								allowedTargets.push_back(EWallParts::EWallParts(i));
+						}
+						if (allowedTargets.empty())
+							break;
+						attackedPart = allowedTargets[rand()%allowedTargets.size()];
+					}
+				}
+				while (!hitSuccessfull);
+
+				if (!hitSuccessfull) // break triggered - no target to shoot at
+					break;
 
 				CatapultAttack ca; //package for clients
-				std::pair< std::pair< ui8, si16 >, ui8> attack; //<< attackedPart , destination tile >, damageDealt >
+				CatapultAttack::AttackInfo attack;
-				attack.first.first = attackedPart;
+				attack.attackedPart = attackedPart;
-				attack.first.second = ba.destinationTile;
+				attack.destinationTile = ba.destinationTile;
-				attack.second = 0;
+				attack.damageDealt = 0;
 
-				int chanceForHit = 0;
 				int dmgChance[] = { sbi.noDmg, sbi.oneDmg, sbi.twoDmg }; //dmgChance[i] - chance for doing i dmg when hit is successful
-				switch(attackedPart)
-				{
-				case EWallParts::KEEP:
-					chanceForHit = sbi.keep;
-					break;
-				case EWallParts::BOTTOM_TOWER:
-				case EWallParts::UPPER_TOWER:
-					chanceForHit = sbi.tower;
-					break;
-				case EWallParts::BOTTOM_WALL:
-				case EWallParts::BELOW_GATE:
-				case EWallParts::OVER_GATE:
-				case EWallParts::UPPER_WAL:
-					chanceForHit = sbi.wall;
-					break;
-				case EWallParts::GATE:
-					chanceForHit = sbi.gate;
-					break;
-				}
 
-				if(rand()%100 <= chanceForHit) //hit is successful
-				{
 				int dmgRand = rand()%100;
 				//accumulating dmgChance
 				dmgChance[1] += dmgChance[0];
 				dmgChance[2] += dmgChance[1];
 				//calculating dealt damage
-					for(int v = 0; v < ARRAY_COUNT(dmgChance); ++v)
+				for(int damage = 0; damage < ARRAY_COUNT(dmgChance); ++damage)
 				{
-						if(dmgRand <= dmgChance[v])
+					if(dmgRand <= dmgChance[damage])
 					{
-							attack.second = std::min(3 - dmgAlreadyDealt - wallInitHP, v);
+						currentHP[attackedPart] = SiegeInfo::applyDamage(EWallState::EWallState(currentHP[attackedPart]), damage);
-							dmgAlreadyDealt += attack.second;
+
+						attack.damageDealt = damage;
 						break;
 					}
 				}
+				// attacked tile may have changed - update destination
+				attack.destinationTile = gs->curB->wallPartToBattleHex(EWallParts::EWallParts(attack.attackedPart));
+
+				logGlobal->traceStream() << "Catapult attacks " << (int)attack.attackedPart
+				                         << " dealing " << (int)attack.damageDealt << " damage";
 
 				//removing creatures in turrets / keep if one is destroyed
-					if(attack.second > 0 && (attackedPart == EWallParts::KEEP ||
+				if(attack.damageDealt > 0 && (attackedPart == EWallParts::KEEP ||
 					attackedPart == EWallParts::BOTTOM_TOWER || attackedPart == EWallParts::UPPER_TOWER))
 				{
 					int posRemove = -1;
@@ -3606,9 +3644,8 @@ bool CGameHandler::makeBattleAction( BattleAction &ba )
 
 					sendAndApply(&bsr);
 				}
-				}
 				ca.attacker = ba.stackNumber;
-				ca.attackedParts.insert(attack);
+				ca.attackedParts.push_back(attack);
 
 				sendAndApply(&ca);
 			}