Merge pull request #5806 from IvanSavenko/bugfixing

Fixes for reported bugs & crashes

AI/VCAI/VCAI.cpp

@@ -453,19 +453,6 @@ void VCAI::objectRemoved(const CGObjectInstance * obj, const PlayerColor & initi
 	//clear resource manager goal cache
 	ah->removeOutdatedObjectives(checkRemovalValidity);
 
-	//TODO: Find better way to handle hero boat removal
-	if(auto hero = dynamic_cast<const CGHeroInstance *>(obj))
-	{
-		if(hero->inBoat())
-		{
-			vstd::erase_if_present(visitableObjs, hero->getBoat());
-			vstd::erase_if_present(alreadyVisited, hero->getBoat());
-
-			for(auto h : cb->getHeroesInfo())
-				unreserveObject(h, hero->getBoat());
-		}
-	}
-
 	//TODO
 	//there are other places where CGObjectinstance ptrs are stored...
 	//

client/NetPacksClient.cpp

@@ -40,6 +40,7 @@
 #include "../lib/CSoundBase.h"
 #include "../lib/StartInfo.h"
 #include "../lib/CConfigHandler.h"
+#include "../lib/mapObjects/MiscObjects.h"
 #include "../lib/mapObjects/CGMarket.h"
 #include "../lib/mapObjects/CGTownInstance.h"
 #include "../lib/gameState/CGameState.h"
@@ -478,8 +479,11 @@ void ApplyClientNetPackVisitor::visitRemoveBonus(RemoveBonus & pack)
 void ApplyFirstClientNetPackVisitor::visitRemoveObject(RemoveObject & pack)
 {
 	const CGObjectInstance *o = cl.gameInfo().getObj(pack.objectID);
+	const auto * h = dynamic_cast<const CGHeroInstance*>(o);
 
 	GAME->map().onObjectFadeOut(o, pack.initiator);
+	if (h && h->inBoat())
+		GAME->map().onObjectFadeOut(h->getBoat(), pack.initiator);
 
 	//notify interfaces about removal
 	for(auto i=cl.playerint.begin(); i!=cl.playerint.end(); i++)
@@ -487,7 +491,11 @@ void ApplyFirstClientNetPackVisitor::visitRemoveObject(RemoveObject & pack)
 		//below line contains little cheat for AI so it will be aware of deletion of enemy heroes that moved or got re-covered by FoW
 		//TODO: loose requirements as next AI related crashes appear, for example another pack.player collects object that got re-covered by FoW, unsure if AI code workarounds this
 		if(gs.isVisibleFor(o, i->first) || (!cl.gameInfo().getPlayerState(i->first)->human && o->ID == Obj::HERO && o->tempOwner != i->first))
+		{
 			i->second->objectRemoved(o, pack.initiator);
+			if (h && h->inBoat())
+				i->second->objectRemoved(h->getBoat(), pack.initiator);
+		}
 	}
 
 	GAME->map().waitForOngoingAnimations();

client/windows/CMapOverview.cpp

@@ -34,7 +34,7 @@
 #include "../../lib/mapping/MapFormat.h"
 #include "../../lib/TerrainHandler.h"
 #include "../../lib/filesystem/Filesystem.h"
-
+#include "../../lib/callback/EditorCallback.h"
 #include "../../lib/StartInfo.h"
 #include "../../lib/mapObjects/CGHeroInstance.h"
 #include "../../lib/rmg/CMapGenOptions.h"
@@ -105,7 +105,8 @@ std::vector<std::shared_ptr<CanvasImage>> CMapOverviewWidget::createMinimaps(con
 	std::unique_ptr<CMap> map;
 	try
 	{
-		map = mapService.loadMap(resource, nullptr);
+		auto cb = std::make_unique<EditorCallback>(map.get());
+		map = mapService.loadMap(resource, cb.get());
 	}
 	catch (const std::exception & e)
 	{

config/schemas/mod.json

@@ -90,6 +90,7 @@
 		},
 		"version" : {
 			"type" : "string",
+			"format" : "version",
 			"description" : "Current mod version, up to 3 numbers, dot-separated. Format: A.B.C"
 		},
 		"changelog" : {

docs/modders/Bonus/Bonus_Limiters.md

@@ -28,8 +28,8 @@ Bonus is only active if affected entity has another bonus that meets conditions
 Parameters:
 
 - Bonus type
-- bonus subtype
-- bonus sourceType and sourceId in struct
+- Bonus subtype (only used if bonus type is set)
+- Bonus source type and bonus source ID
 
 All parameters are optional. Values that don't need checking can be replaces with `null`
 
@@ -42,11 +42,11 @@ Examples:
 		{
 			"type" : "HAS_ANOTHER_BONUS_LIMITER",
 			"parameters" : [
-				null,
-				null,
+				null, // bonus type is ignored
+				null, // bonus subtype is also ignored
 				{
-					"type" : "SPELL_EFFECT",
-					"id" : "spell.bless"
+					"type" : "SPELL_EFFECT", // look for bonus of type SPELL_EFFECT
+					"id" : "spell.bless"     // ... from spell "Bless"
 				}
 				]
 		}

lib/battle/BattleInfo.cpp

@@ -358,13 +358,13 @@ std::unique_ptr<BattleInfo> BattleInfo::setupBattle(IGameInfoCallback *cb, const
 
 	if (currentBattle->townID.hasValue())
 	{
-		if (currentBattle->getTown()->fortificationsLevel().citadelHealth != 0)
+		if (currentBattle->getDefendedTown()->fortificationsLevel().citadelHealth != 0)
 			currentBattle->generateNewStack(currentBattle->nextUnitId(), CStackBasicDescriptor(CreatureID::ARROW_TOWERS, 1), BattleSide::DEFENDER, SlotID::ARROW_TOWERS_SLOT, BattleHex::CASTLE_CENTRAL_TOWER);
 
-		if (currentBattle->getTown()->fortificationsLevel().upperTowerHealth != 0)
+		if (currentBattle->getDefendedTown()->fortificationsLevel().upperTowerHealth != 0)
 			currentBattle->generateNewStack(currentBattle->nextUnitId(), CStackBasicDescriptor(CreatureID::ARROW_TOWERS, 1), BattleSide::DEFENDER, SlotID::ARROW_TOWERS_SLOT, BattleHex::CASTLE_UPPER_TOWER);
 
-		if (currentBattle->getTown()->fortificationsLevel().lowerTowerHealth != 0)
+		if (currentBattle->getDefendedTown()->fortificationsLevel().lowerTowerHealth != 0)
 			currentBattle->generateNewStack(currentBattle->nextUnitId(), CStackBasicDescriptor(CreatureID::ARROW_TOWERS, 1), BattleSide::DEFENDER, SlotID::ARROW_TOWERS_SLOT, BattleHex::CASTLE_BOTTOM_TOWER);
 
 		//Moat generating is done on server
@@ -568,13 +568,6 @@ const CGHeroInstance * BattleInfo::getSideHero(BattleSide side) const
 	return getSide(side).getHero();
 }
 
-const CGTownInstance * BattleInfo::getTown() const
-{
-	if (townID.hasValue())
-		return cb->getTown(townID);
-	return nullptr;
-}
-
 uint8_t BattleInfo::getTacticDist() const
 {
 	return tacticDistance;

lib/battle/BattleInfo.h

@@ -101,8 +101,6 @@ public:
 	const CArmedInstance * getSideArmy(BattleSide side) const override;
 	const CGHeroInstance * getSideHero(BattleSide side) const override;
 
-	const CGTownInstance * getTown() const;
-
 	ui8 getTacticDist() const override;
 	BattleSide getTacticsSide() const override;
 

lib/callback/IGameEventCallback.h

@@ -102,7 +102,6 @@ public:
 	virtual void startBattle(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, const CGHeroInstance *hero1, const CGHeroInstance *hero2, const BattleLayout & layout, const CGTownInstance *town)=0; //use hero=nullptr for no hero
 	virtual void startBattle(const CArmedInstance *army1, const CArmedInstance *army2)=0; //if any of armies is hero, hero will be used, visitable tile of second obj is place of battle
 	virtual bool moveHero(ObjectInstanceID hid, int3 dst, EMovementMode moveMove, bool transit = false, PlayerColor asker = PlayerColor::NEUTRAL)=0;
-	virtual bool swapGarrisonOnSiege(ObjectInstanceID tid)=0;
 	virtual void giveHeroBonus(GiveBonus * bonus)=0;
 	virtual void setMovePoints(SetMovePoints * smp)=0;
 	virtual void setMovePoints(ObjectInstanceID hid, int val, ChangeValueMode mode)=0;

lib/gameState/CGameState.cpp

@@ -554,6 +554,7 @@ void CGameState::placeStartingHero(const PlayerColor & playerColor, const HeroTy
 		hero = std::dynamic_pointer_cast<CGHeroInstance>(object);
 		hero->ID = Obj::HERO;
 		hero->setHeroType(heroTypeId);
+		assert(hero->appearance != nullptr);
 	}
 
 	hero->tempOwner = playerColor;

lib/gameState/GameStatePackVisitor.cpp

@@ -385,7 +385,6 @@ void GameStatePackVisitor::visitRemoveObject(RemoveObject & pack)
 		auto beatenHero = dynamic_cast<CGHeroInstance*>(obj);
 		assert(beatenHero);
 
-		auto * siegeNode = beatenHero->whereShouldBeAttachedOnSiege(gs);
 		vstd::erase_if(beatenHero->artifactsInBackpack, [](const ArtSlotInfo& asi)
 		{
 			return asi.getArt()->getTypeId() == ArtifactID::GRAIL;
@@ -400,14 +399,6 @@ void GameStatePackVisitor::visitRemoveObject(RemoveObject & pack)
 
 			beatenHero->setVisitedTown(nullptr, false);
 		}
-		beatenHero->detachFromBonusSystem(gs);
-		beatenHero->tempOwner = PlayerColor::NEUTRAL; //no one owns beaten hero
-
-		// FIXME: workaround:
-		// hero should be attached to siegeNode after battle
-		// however this code might also be called on dismissing hero while in town
-		if (siegeNode && vstd::contains(beatenHero->getParentNodes(), siegeNode))
-			beatenHero->detachFrom(*siegeNode);
 
 		//If hero on Boat is removed, the Boat disappears
 		if(beatenHero->inBoat())
@@ -417,12 +408,13 @@ void GameStatePackVisitor::visitRemoveObject(RemoveObject & pack)
 			gs.getMap().eraseObject(boat->id);
 		}
 
+		beatenHero->detachFromBonusSystem(gs);
+		beatenHero->tempOwner = PlayerColor::NEUTRAL; //no one owns beaten hero
 		auto beatenObject = gs.getMap().eraseObject(obj->id);
 
 		//return hero to the pool, so he may reappear in tavern
 		gs.heroesPool->addHeroToPool(beatenHero->getHeroTypeID());
 		gs.getMap().addToHeroPool(std::dynamic_pointer_cast<CGHeroInstance>(beatenObject));
-
 		return;
 	}
 
@@ -1173,6 +1165,18 @@ void GameStatePackVisitor::visitBattleStart(BattleStart & pack)
 	pack.info->battleID = gs.nextBattleID;
 	pack.info->localInit();
 
+	if (pack.info->getDefendedTown() && pack.info->getSideHero(BattleSide::DEFENDER))
+	{
+		CGTownInstance * town = gs.getTown(pack.info->townID);
+		CGHeroInstance * hero = gs.getHero(pack.info->getSideHero(BattleSide::DEFENDER)->id);
+
+		if (town->getVisitingHero() == hero)
+		{
+			hero->detachFrom(town->townAndVis);
+			hero->attachTo(*town);
+		}
+	}
+
 	gs.currentBattles.push_back(std::move(pack.info));
 	gs.nextBattleID = BattleID(gs.nextBattleID.getNum() + 1);
 }
@@ -1232,17 +1236,6 @@ void GameStatePackVisitor::visitBattleUpdateGateState(BattleUpdateGateState & pa
 		gs.getBattle(pack.battleID)->si.gateState = pack.state;
 }
 
-void GameStatePackVisitor::visitBattleCancelled(BattleCancelled & pack)
-{
-	auto currentBattle = boost::range::find_if(gs.currentBattles, [&](const auto & battle)
-	{
-		return battle->battleID == pack.battleID;
-	});
-
-	assert(currentBattle != gs.currentBattles.end());
-	gs.currentBattles.erase(currentBattle);
-}
-
 void GameStatePackVisitor::visitBattleResultAccepted(BattleResultAccepted & pack)
 {
 	// Remove any "until next battle" bonuses
@@ -1348,8 +1341,44 @@ void GameStatePackVisitor::visitBattleUnitsChanged(BattleUnitsChanged & pack)
 	pack.visitTyped(battleVisitor);
 }
 
+void GameStatePackVisitor::restorePreBattleState(BattleID battleID)
+{
+	auto battleIter = boost::range::find_if(gs.currentBattles, [&](const auto & battle)
+	{
+		return battle->battleID == battleID;
+	});
+
+	const auto & currentBattle = **battleIter;
+
+	if (currentBattle.getDefendedTown() && currentBattle.getSideHero(BattleSide::DEFENDER))
+	{
+		CGTownInstance * town = gs.getTown(currentBattle.townID);
+		CGHeroInstance * hero = gs.getHero(currentBattle.getSideHero(BattleSide::DEFENDER)->id);
+
+		if (town->getVisitingHero() == hero)
+		{
+			hero->detachFrom(*town);
+			hero->attachTo(town->townAndVis);
+		}
+	}
+}
+
+void GameStatePackVisitor::visitBattleCancelled(BattleCancelled & pack)
+{
+	restorePreBattleState(pack.battleID);
+
+	auto battleIter = boost::range::find_if(gs.currentBattles, [&](const auto & battle)
+	{
+		return battle->battleID == pack.battleID;
+	});
+
+	assert(battleIter != gs.currentBattles.end());
+	gs.currentBattles.erase(battleIter);
+}
+
 void GameStatePackVisitor::visitBattleResultsApplied(BattleResultsApplied & pack)
 {
+	restorePreBattleState(pack.battleID);
 	pack.learnedSpells.visit(*this);
 
 	for(auto & discharging : pack.dischargingArtifacts)

lib/gameState/GameStatePackVisitor.h

@@ -17,6 +17,7 @@ class CGameState;
 
 class GameStatePackVisitor final : public ICPackVisitor
 {
+	void restorePreBattleState(BattleID battleID);
 private:
 	CGameState & gs;
 

lib/json/JsonBonus.cpp

@@ -502,7 +502,7 @@ std::shared_ptr<const ILimiter> JsonUtils::parseLimiter(const JsonNode & limiter
 	case JsonNode::JsonType::DATA_STRUCT: //customizable limiters
 		{
 			std::string limiterType = limiter["type"].String();
-			const JsonVector & parameters = limiter["parameters"].Vector();
+			const JsonNode & parameters = limiter["parameters"];
 			if(limiterType == "CREATURE_TYPE_LIMITER")
 			{
 				auto creatureLimiter = std::make_shared<CCreatureTypeLimiter>();
@@ -512,7 +512,7 @@ std::shared_ptr<const ILimiter> JsonUtils::parseLimiter(const JsonNode & limiter
 				});
 				auto includeUpgrades = false;
 
-				if(parameters.size() > 1)
+				if(parameters.Vector().size() > 1)
 				{
 					bool success = true;
 					includeUpgrades = parameters[1].TryBoolFromString(success);
@@ -527,43 +527,37 @@ std::shared_ptr<const ILimiter> JsonUtils::parseLimiter(const JsonNode & limiter
 			{
 				auto bonusLimiter = std::make_shared<HasAnotherBonusLimiter>();
 
-				if (!parameters[0].isNull())
+				static const JsonNode nullNode;
+				const JsonNode & jsonType = parameters[0];
+				const JsonNode & jsonSubtype = parameters.Vector().size() > 2 ? parameters[1] : nullNode;
+				const JsonNode & jsonSource = parameters.Vector().size() > 2 ? parameters[2] : parameters[1];
+
+				if (!jsonType.isNull())
 				{
-					LIBRARY->identifiers()->requestIdentifier("bonus", parameters[0], [bonusLimiter](si32 bonusID)
+					LIBRARY->identifiers()->requestIdentifier("bonus", jsonType, [bonusLimiter, jsonSubtype](si32 bonusID)
 					{
 						bonusLimiter->type = static_cast<BonusType>(bonusID);
+						if (jsonSubtype.isNull())
+							loadBonusSubtype(bonusLimiter->subtype, bonusLimiter->type, jsonSubtype);
 					});
 				}
 
-				auto findSource = [&](const JsonNode & parameter)
+				if(!jsonSource.isNull())
 				{
-					if(parameter.getType() == JsonNode::JsonType::DATA_STRUCT)
+					auto sourceIt = bonusSourceMap.find(jsonSource["type"].String());
+					if(sourceIt != bonusSourceMap.end())
 					{
-						auto sourceIt = bonusSourceMap.find(parameter["type"].String());
-						if(sourceIt != bonusSourceMap.end())
-						{
-							bonusLimiter->source = sourceIt->second;
-							bonusLimiter->isSourceRelevant = true;
-							if(!parameter["id"].isNull()) {
-								loadBonusSourceInstance(bonusLimiter->sid, bonusLimiter->source, parameter["id"]);
-								bonusLimiter->isSourceIDRelevant = true;
-							}
+						bonusLimiter->source = sourceIt->second;
+						bonusLimiter->isSourceRelevant = true;
+						if(!jsonSource["id"].isNull()) {
+							loadBonusSourceInstance(bonusLimiter->sid, bonusLimiter->source, jsonSource["id"]);
+							bonusLimiter->isSourceIDRelevant = true;
 						}
 					}
-					return false;
-				};
-				if(parameters.size() > 1)
-				{
-					if(findSource(parameters[1]) && parameters.size() == 2)
-						return bonusLimiter;
 					else
-					{
-						loadBonusSubtype(bonusLimiter->subtype, bonusLimiter->type, parameters[1]);
-						bonusLimiter->isSubtypeRelevant = true;
-						if(parameters.size() > 2)
-							findSource(parameters[2]);
-					}
+						logMod->warn("HAS_ANOTHER_BONUS_LIMITER: unknown bonus source type '%s'!", jsonSource["type"].String());
 				}
+
 				return bonusLimiter;
 			}
 			else if(limiterType == "CREATURE_ALIGNMENT_LIMITER")
@@ -586,7 +580,7 @@ std::shared_ptr<const ILimiter> JsonUtils::parseLimiter(const JsonNode & limiter
 			else if(limiterType == "CREATURE_LEVEL_LIMITER")
 			{
 				auto levelLimiter = std::make_shared<CreatureLevelLimiter>();
-				if(!parameters.empty()) //If parameters is empty, level limiter works as CREATURES_ONLY limiter
+				if(!parameters.Vector().empty()) //If parameters is empty, level limiter works as CREATURES_ONLY limiter
 				{
 					levelLimiter->minLevel = parameters[0].Integer();
 					if(parameters[1].isNumber())
@@ -597,7 +591,7 @@ std::shared_ptr<const ILimiter> JsonUtils::parseLimiter(const JsonNode & limiter
 			else if(limiterType == "CREATURE_TERRAIN_LIMITER")
 			{
 				auto terrainLimiter = std::make_shared<CreatureTerrainLimiter>();
-				if(!parameters.empty())
+				if(!parameters.Vector().empty())
 				{
 					LIBRARY->identifiers()->requestIdentifier("terrain", parameters[0], [terrainLimiter](si32 terrain)
 					{
@@ -608,9 +602,9 @@ std::shared_ptr<const ILimiter> JsonUtils::parseLimiter(const JsonNode & limiter
 			}
 			else if(limiterType == "UNIT_ON_HEXES") {
 				auto hexLimiter = std::make_shared<UnitOnHexLimiter>();
-				if(!parameters.empty())
+				if(!parameters.Vector().empty())
 				{
-					for (const auto & parameter: parameters){
+					for (const auto & parameter : parameters.Vector()){
 						if(parameter.isNumber())
 							hexLimiter->applicableHexes.insert(BattleHex(parameter.Integer()));
 					}

lib/json/JsonValidator.cpp

@@ -19,6 +19,7 @@
 #include "../modding/CModHandler.h"
 #include "../texts/TextOperations.h"
 #include "../ScopeGuard.h"
+#include "modding/CModVersion.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -538,6 +539,14 @@ static std::string videoFile(const JsonNode & node)
 }
 #undef TEST_FILE
 
+static std::string version(const JsonNode & node)
+{
+	auto version = CModVersion::fromString(node.String());
+	if (version == CModVersion())
+		return "Failed to parse mod version: " + node.toCompactString() + ". Expected format X.Y.Z, where X, Y, Z are non-negative numbers";
+	return "";
+}
+
 JsonValidator::TValidatorMap createCommonFields()
 {
 	JsonValidator::TValidatorMap ret;
@@ -629,6 +638,7 @@ JsonValidator::TFormatMap createFormatMap()
 	ret["animationFile"] = animationFile;
 	ret["imageFile"]     = imageFile;
 	ret["videoFile"]     = videoFile;
+	ret["version"]     = version;
 
 	//TODO:
 	// uri-reference

lib/mapObjects/CGHeroInstance.cpp

@@ -1346,25 +1346,6 @@ void CGHeroInstance::detachFromBonusSystem(CGameState & gs)
 	}
 }
 
-CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(const bool isBattleOutsideTown) const
-{
-	if(!getVisitedTown())
-		return nullptr;
-
-	if (isBattleOutsideTown)
-		return const_cast<CTownAndVisitingHero *>(&getVisitedTown()->townAndVis);
-
-	return const_cast<CGTownInstance*>(getVisitedTown());
-}
-
-CBonusSystemNode * CGHeroInstance::whereShouldBeAttachedOnSiege(CGameState & gs)
-{
-	if(getVisitedTown())
-		return whereShouldBeAttachedOnSiege(getVisitedTown()->isBattleOutsideTown(this));
-
-	return &CArmedInstance::whereShouldBeAttached(gs);
-}
-
 CBonusSystemNode & CGHeroInstance::whereShouldBeAttached(CGameState & gs)
 {
 	if(visitedTown.hasValue())

lib/mapObjects/CGHeroInstance.h

@@ -276,9 +276,6 @@ public:
 	///IConstBonusProvider
 	const IBonusBearer* getBonusBearer() const override;
 
-	CBonusSystemNode * whereShouldBeAttachedOnSiege(const bool isBattleOutsideTown) const;
-	CBonusSystemNode * whereShouldBeAttachedOnSiege(CGameState & gs);
-
 	///spells::Caster
 	int32_t getCasterUnitId() const override;
 	int32_t getSpellSchoolLevel(const spells::Spell * spell, SpellSchool * outSelectedSchool = nullptr) const override;

lib/mapObjects/CGTownInstance.cpp

@@ -307,19 +307,12 @@ void CGTownInstance::onHeroVisit(IGameEventCallback & gameEvents, const CGHeroIn
 		if(armedGarrison() || getVisitingHero())
 		{
 			const CGHeroInstance * defendingHero = getVisitingHero() ? getVisitingHero() : getGarrisonHero();
-			const CArmedInstance * defendingArmy = defendingHero ? (CArmedInstance *)defendingHero : this;
+			const CArmedInstance * defendingArmy = defendingHero ? static_cast<const CArmedInstance*>(defendingHero) : this;
 			const bool isBattleOutside = isBattleOutsideTown(defendingHero);
 
-			if(!isBattleOutside && getVisitingHero() && defendingHero == getVisitingHero())
-			{
-				//we have two approaches to merge armies: mergeGarrisonOnSiege() and used in the CGameHandler::garrisonSwap(ObjectInstanceID tid)
-				auto * nodeSiege = defendingHero->whereShouldBeAttachedOnSiege(isBattleOutside);
-
-				if(nodeSiege == (CBonusSystemNode *)this)
-					gameEvents.swapGarrisonOnSiege(this->id);
+			if(!isBattleOutside && defendingHero == getVisitingHero())
+				mergeGarrisonOnSiege(gameEvents);
 
-				const_cast<CGHeroInstance *>(defendingHero)->setVisitedTown(this, false); //hack to return visitor from garrison after battle
-			}
 			gameEvents.startBattle(h, defendingArmy, getSightCenter(), h, defendingHero, BattleLayout::createDefaultLayout(*cb, h, defendingArmy), (isBattleOutside ? nullptr : this));
 		}
 		else

lib/mapping/MapFormatH3M.cpp

@@ -42,6 +42,7 @@
 #include "../networkPacks/ArtifactLocation.h"
 #include "../spells/CSpellHandler.h"
 #include "../texts/TextOperations.h"
+#include "entities/hero/CHeroClass.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -843,8 +844,9 @@ void CMapLoaderH3M::readPredefinedHeroes()
 		if(!custom)
 			continue;
 
-		auto hero = std::make_shared<CGHeroInstance>(map->cb);
-		hero->ID = Obj::HERO;
+		auto handler = LIBRARY->objtypeh->getHandlerFor(Obj::HERO, HeroTypeID(heroID).toHeroType()->heroClass->getIndex());
+		auto object = handler->create(map->cb, handler->getTemplates().front());
+		auto hero = std::dynamic_pointer_cast<CGHeroInstance>(object);
 		hero->subID = heroID;
 
 		bool hasExp = reader->readBool();

lib/modding/CModVersion.cpp

@@ -18,29 +18,41 @@ CModVersion CModVersion::GameVersion()
 	return CModVersion(VCMI_VERSION_MAJOR, VCMI_VERSION_MINOR, VCMI_VERSION_PATCH);
 }
 
-CModVersion CModVersion::fromString(std::string from)
+CModVersion CModVersion::fromString(const std::string & from)
 {
-	int major = Any;
-	int minor = Any;
-	int patch = Any;
+	std::vector<std::string> segments;
+	boost::split(segments, from, boost::is_any_of("."));
+
+	if (from.empty())
+		return CModVersion();
+
+	if (segments.size() > 3)
+		return CModVersion();
+
+	static const std::string whitelist = "1234567890.";
+
+	for (const auto & ch : from)
+		if (whitelist.find(ch) == std::string::npos)
+			return CModVersion();
+
 	try
 	{
-		auto pointPos = from.find('.');
-		major = std::stoi(from.substr(0, pointPos));
-		if(pointPos != std::string::npos)
-		{
-			from = from.substr(pointPos + 1);
-			pointPos = from.find('.');
-			minor = std::stoi(from.substr(0, pointPos));
-			if(pointPos != std::string::npos)
-				patch = std::stoi(from.substr(pointPos + 1));
-		}
+		int major = Any;
+		int minor = Any;
+		int patch = Any;
+
+		major = std::stoi(segments[0]);
+		if (segments.size() > 1)
+			minor = std::stoi(segments[1]);
+		if (segments.size() > 2)
+			patch = std::stoi(segments[2]);
+
+		return CModVersion(major, minor, patch);
 	}
-	catch(const std::invalid_argument &)
+	catch(const std::logic_error &)
 	{
 		return CModVersion();
 	}
-	return CModVersion(major, minor, patch);
 }
 
 std::string CModVersion::toString() const

lib/modding/CModVersion.h

@@ -32,7 +32,7 @@ struct DLL_LINKAGE CModVersion
 	CModVersion(int mj, int mi, int p): major(mj), minor(mi), patch(p) {}
 
 	static CModVersion GameVersion();
-	static CModVersion fromString(std::string from);
+	static CModVersion fromString(const std::string & from);
 	std::string toString() const;
 
 	bool operator !=(const CModVersion & other) const;

server/CGameHandler.cpp

@@ -2548,33 +2548,6 @@ void CGameHandler::moveArmy(const CArmedInstance *src, const CArmedInstance *dst
 	}
 }
 
-bool CGameHandler::swapGarrisonOnSiege(ObjectInstanceID tid)
-{
-	const CGTownInstance * town = gameInfo().getTown(tid);
-
-	if(!town->getGarrisonHero() == !town->getVisitingHero())
-		return false;
-
-	SetHeroesInTown intown;
-	intown.tid = tid;
-
-	if(town->getGarrisonHero()) //garrison -> vising
-	{
-		intown.garrison = ObjectInstanceID();
-		intown.visiting = town->getGarrisonHero()->id;
-	}
-	else //visiting -> garrison
-	{
-		if(town->armedGarrison())
-			town->mergeGarrisonOnSiege(*this);
-
-		intown.visiting = ObjectInstanceID();
-		intown.garrison = town->getVisitingHero()->id;
-	}
-	sendAndApply(intown);
-	return true;
-}
-
 bool CGameHandler::garrisonSwap(ObjectInstanceID tid)
 {
 	const CGTownInstance * town = gameInfo().getTown(tid);

server/CGameHandler.h

@@ -224,7 +224,6 @@ public:
 	//void lootArtifacts (TArtHolder source, TArtHolder dest, std::vector<ui32> &arts); //after battle - move al arts to winer
 	bool buySecSkill( const IMarket *m, const CGHeroInstance *h, SecondarySkill skill);
 	bool garrisonSwap(ObjectInstanceID tid);
-	bool swapGarrisonOnSiege(ObjectInstanceID tid) override;
 	bool upgradeCreature( ObjectInstanceID objid, SlotID pos, CreatureID upgID );
 	bool recruitCreatures(ObjectInstanceID objid, ObjectInstanceID dst, CreatureID crid, int32_t cram, int32_t level, PlayerColor player);
 	bool buildStructure(ObjectInstanceID tid, BuildingID bid, bool force=false);//force - for events: no cost, no checkings

server/battles/BattleProcessor.cpp

@@ -78,6 +78,7 @@ void BattleProcessor::restartBattle(const BattleID & battleID, const CArmedInsta
 				SetMana restoreInitialMana;
 				restoreInitialMana.val = lastBattleQuery->initialHeroMana[i];
 				restoreInitialMana.hid = heroes[i]->id;
+				restoreInitialMana.mode = ChangeValueMode::ABSOLUTE;
 				gameHandler->sendAndApply(restoreInitialMana);
 			}
 		}

server/battles/BattleResultProcessor.cpp

@@ -322,14 +322,6 @@ void BattleResultProcessor::endBattleConfirm(const CBattleInfoCallback & battle)
 	const auto winnerHero = battle.battleGetFightingHero(finishingBattle->winnerSide);
 	const auto loserHero = battle.battleGetFightingHero(CBattleInfoEssentials::otherSide(finishingBattle->winnerSide));
 
-	if(battleResult->winner == BattleSide::DEFENDER
-	   && winnerHero
-	   && winnerHero->getVisitedTown()
-	   && !winnerHero->isGarrisoned()
-	   && winnerHero->getVisitedTown()->getGarrisonHero() == winnerHero)
-	{
-		gameHandler->swapGarrisonOnSiege(winnerHero->getVisitedTown()->id); //return defending visitor from garrison to its rightful place
-	}
 	//give exp
 	if(!finishingBattle->isDraw() && battleResult->exp[finishingBattle->winnerSide])
 	{

test/mock/mock_IGameEventCallback.h

@@ -72,7 +72,6 @@ public:
 	void startBattle(const CArmedInstance *army1, const CArmedInstance *army2, int3 tile, const CGHeroInstance *hero1, const CGHeroInstance *hero2, const BattleLayout & layout, const CGTownInstance *town) override {} //use hero=nullptr for no hero
 	void startBattle(const CArmedInstance *army1, const CArmedInstance *army2) override {}
 	bool moveHero(ObjectInstanceID hid, int3 dst, EMovementMode movementMode, bool transit, PlayerColor asker) override {return false;}
-	bool swapGarrisonOnSiege(ObjectInstanceID tid) override {return false;}
 	void giveHeroBonus(GiveBonus * bonus) override {}
 	void setMovePoints(SetMovePoints * smp) override {}
 	void setMovePoints(ObjectInstanceID hid, int val, ChangeValueMode mode) override {};