Fix hero retreating handling & code cleanup

lib/CGameInfoCallback.cpp

@@ -482,7 +482,7 @@ std::vector<const CGHeroInstance *> CGameInfoCallback::getAvailableHeroes(const
 	const CGTownInstance * town = getTown(townOrTavern->id);
 
 	if(townOrTavern->ID == Obj::TAVERN || (town && town->hasBuilt(BuildingID::TAVERN)))
-		return gs->hpool->getHeroesFor(*player);
+		return gs->heroesPool->getHeroesFor(*player);
 
 	return ret;
 }

lib/GameConstants.h

@@ -358,7 +358,6 @@ class PlayerColor : public BaseForID<PlayerColor, ui8>
 	enum EPlayerColor
 	{
 		PLAYER_LIMIT_I = 8,
-		ALL_PLAYERS_MASK = 0xff
 	};
 
 	using Mask = uint8_t;

lib/NetPacks.h

@@ -342,7 +342,7 @@ struct DLL_LINKAGE SetAvailableHero : public CPackForClient
 	TavernHeroSlot slotID;
 	TavernSlotRole roleID;
 	PlayerColor player;
-	HeroTypeID hid; //-1 if no hero
+	HeroTypeID hid; //HeroTypeID::NONE if no hero
 	CSimpleArmy army;
 
 	virtual void visitTyped(ICPackVisitor & visitor) override;

lib/NetPacksLib.cpp

@@ -942,7 +942,7 @@ void FoWChange::applyGs(CGameState *gs)
 
 void SetAvailableHero::applyGs(CGameState *gs)
 {
-	gs->hpool->setHeroForPlayer(player, slotID, hid, army, roleID);
+	gs->heroesPool->setHeroForPlayer(player, slotID, hid, army, roleID);
 }
 
 void GiveBonus::applyGs(CGameState *gs)
@@ -1143,7 +1143,7 @@ void RemoveObject::applyGs(CGameState *gs)
 		}
 		//return hero to the pool, so he may reappear in tavern
 
-		gs->hpool->addHeroToPool(beatenHero);
+		gs->heroesPool->addHeroToPool(beatenHero);
 		gs->map->objects[id.getNum()] = nullptr;
 
 		//If hero on Boat is removed, the Boat disappears
@@ -1368,7 +1368,7 @@ void SetHeroesInTown::applyGs(CGameState * gs) const
 
 void HeroRecruited::applyGs(CGameState * gs) const
 {
-	CGHeroInstance *h = gs->hpool->takeHero(hid);
+	CGHeroInstance *h = gs->heroesPool->takeHeroFromPool(hid);
 	CGTownInstance *t = gs->getTown(tid);
 	PlayerState *p = gs->getPlayerState(player);
 
@@ -2017,7 +2017,7 @@ void NewTurn::applyGs(CGameState *gs)
 		hero->mana = h.mana;
 	}
 
-	gs->hpool->onNewDay();
+	gs->heroesPool->onNewDay();
 
 	for(const auto & re : res)
 	{

lib/gameState/CGameState.cpp

@@ -385,7 +385,7 @@ int CGameState::getDate(Date::EDateType mode) const
 CGameState::CGameState()
 {
 	gs = this;
-	hpool = std::make_unique<TavernHeroesPool>();
+	heroesPool = std::make_unique<TavernHeroesPool>();
 	applier = std::make_shared<CApplier<CBaseForGSApply>>();
 	registerTypesClientPacks1(*applier);
 	registerTypesClientPacks2(*applier);
@@ -875,7 +875,7 @@ void CGameState::initHeroes()
 		if(!vstd::contains(heroesToCreate, HeroTypeID(ph->subID)))
 			continue;
 		ph->initHero(getRandomGenerator());
-		hpool->addHeroToPool(ph);
+		heroesPool->addHeroToPool(ph);
 		heroesToCreate.erase(ph->type->getId());
 
 		map->allHeroes[ph->subID] = ph;
@@ -888,11 +888,11 @@ void CGameState::initHeroes()
 
 		int typeID = htype.getNum();
 		map->allHeroes[typeID] = vhi;
-		hpool->addHeroToPool(vhi);
+		heroesPool->addHeroToPool(vhi);
 	}
 
 	for(auto & elem : map->disposedHeroes)
-		hpool->setAvailability(elem.heroId, elem.players);
+		heroesPool->setAvailability(elem.heroId, elem.players);
 
 	if (campaign)
 		campaign->initHeroes();

lib/gameState/CGameState.h

@@ -82,7 +82,7 @@ class DLL_LINKAGE CGameState : public CNonConstInfoCallback
 
 public:
 	//we have here all heroes available on this map that are not hired
-	std::unique_ptr<TavernHeroesPool> hpool;
+	std::unique_ptr<TavernHeroesPool> heroesPool;
 
 	CGameState();
 	virtual ~CGameState();
@@ -154,7 +154,7 @@ public:
 		h & map;
 		h & players;
 		h & teams;
-		h & hpool;
+		h & heroesPool;
 		h & globalEffects;
 		h & rand;
 		h & rumor;

lib/gameState/TavernHeroesPool.cpp

@@ -11,6 +11,7 @@
 #include "TavernHeroesPool.h"
 
 #include "../mapObjects/CGHeroInstance.h"
+#include "../CHeroHandler.h"
 
 VCMI_LIB_NAMESPACE_BEGIN
 
@@ -56,7 +57,7 @@ void TavernHeroesPool::setHeroForPlayer(PlayerColor player, TavernHeroSlot slot,
 	TavernSlot newSlot;
 	newSlot.hero = h;
 	newSlot.player = player;
-	newSlot.role = TavernSlotRole::SINGLE_UNIT; // TODO
+	newSlot.role = role;
 	newSlot.slot = slot;
 
 	currentTavern.push_back(newSlot);
@@ -72,8 +73,8 @@ void TavernHeroesPool::setHeroForPlayer(PlayerColor player, TavernHeroSlot slot,
 
 bool TavernHeroesPool::isHeroAvailableFor(HeroTypeID hero, PlayerColor color) const
 {
-	if (pavailable.count(hero))
-		return pavailable.at(hero) & (1 << color.getNum());
+	if (perPlayerAvailability.count(hero))
+		return perPlayerAvailability.at(hero) & (1 << color.getNum());
 
 	return true;
 }
@@ -91,13 +92,17 @@ std::vector<const CGHeroInstance *> TavernHeroesPool::getHeroesFor(PlayerColor c
 	return result;
 }
 
-CGHeroInstance * TavernHeroesPool::takeHero(HeroTypeID hero)
+CGHeroInstance * TavernHeroesPool::takeHeroFromPool(HeroTypeID hero)
 {
 	assert(heroesPool.count(hero));
 
 	CGHeroInstance * result = heroesPool[hero];
 	heroesPool.erase(hero);
 
+	vstd::erase_if(currentTavern, [&](const TavernSlot & entry){
+		return entry.hero->type->getId() == hero;
+	});
+
 	assert(result);
 	return result;
 }
@@ -131,7 +136,7 @@ void TavernHeroesPool::addHeroToPool(CGHeroInstance * hero)
 
 void TavernHeroesPool::setAvailability(HeroTypeID hero, PlayerColor::Mask mask)
 {
-	pavailable[hero] = mask;
+	perPlayerAvailability[hero] = mask;
 }
 
 VCMI_LIB_NAMESPACE_END

lib/gameState/TavernHeroesPool.h

@@ -44,7 +44,7 @@ class DLL_LINKAGE TavernHeroesPool
 
 	/// list of which players are able to purchase specific hero
 	/// if hero is not present in list, he is available for everyone
-	std::map<HeroTypeID, PlayerColor::Mask> pavailable;
+	std::map<HeroTypeID, PlayerColor::Mask> perPlayerAvailability;
 
 	/// list of heroes currently available in taverns
 	std::vector<TavernSlot> currentTavern;
@@ -63,19 +63,23 @@ public:
 
 	TavernSlotRole getSlotRole(HeroTypeID hero) const;
 
-	CGHeroInstance * takeHero(HeroTypeID hero);
+	CGHeroInstance * takeHeroFromPool(HeroTypeID hero);
 
 	/// reset mana and movement points for all heroes in pool
 	void onNewDay();
 
 	void addHeroToPool(CGHeroInstance * hero);
+
+	/// Marks hero as available to only specific set of players
 	void setAvailability(HeroTypeID hero, PlayerColor::Mask mask);
+
+	/// Makes hero available in tavern of specified player
 	void setHeroForPlayer(PlayerColor player, TavernHeroSlot slot, HeroTypeID hero, CSimpleArmy & army, TavernSlotRole role);
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{
 		h & heroesPool;
-		h & pavailable;
+		h & perPlayerAvailability;
 		h & currentTavern;
 	}
 };

lib/mapping/CMap.h

@@ -57,7 +57,7 @@ struct DLL_LINKAGE DisposedHero
 	DisposedHero();
 
 	HeroTypeID heroId;
-	ui32 portrait; /// The portrait id of the hero, -1 is default.
+	HeroTypeID portrait; /// The portrait id of the hero, -1 is default.
 	std::string name;
 	PlayerColor::Mask players; /// Who can hire this hero (bitfield).
 

server/HeroPoolProcessor.cpp

@@ -57,9 +57,9 @@ bool HeroPoolProcessor::playerEndedTurn(const PlayerColor & player)
 
 TavernHeroSlot HeroPoolProcessor::selectSlotForRole(const PlayerColor & player, TavernSlotRole roleID)
 {
-	const auto & hpool = gameHandler->gameState()->hpool;
+	const auto & heroesPool = gameHandler->gameState()->heroesPool;
 
-	const auto & heroes = hpool->getHeroesFor(player);
+	const auto & heroes = heroesPool->getHeroesFor(player);
 
 	// if tavern has empty slot - use it
 	if (heroes.size() == 0)
@@ -71,8 +71,8 @@ TavernHeroSlot HeroPoolProcessor::selectSlotForRole(const PlayerColor & player,
 	// try to find "better" slot to overwrite
 	// we want to avoid overwriting retreated heroes when tavern still has slot with random hero
 	// as well as avoid overwriting surrendered heroes if we can overwrite retreated hero
-	auto roleLeft = hpool->getSlotRole(HeroTypeID(heroes[0]->subID));
-	auto roleRight = hpool->getSlotRole(HeroTypeID(heroes[1]->subID));
+	auto roleLeft = heroesPool->getSlotRole(HeroTypeID(heroes[0]->subID));
+	auto roleRight = heroesPool->getSlotRole(HeroTypeID(heroes[1]->subID));
 
 	if (roleLeft > roleRight)
 		return TavernHeroSlot::RANDOM;
@@ -98,8 +98,6 @@ void HeroPoolProcessor::onHeroSurrendered(const PlayerColor & color, const CGHer
 	sah.slotID = selectSlotForRole(color, sah.roleID);
 	sah.player = color;
 	sah.hid = hero->subID;
-	sah.army.clear();
-	sah.army.setCreature(SlotID(0), hero->type->initialArmy.at(0).creature, 1);
 	gameHandler->sendAndApply(&sah);
 }
 
@@ -114,6 +112,8 @@ void HeroPoolProcessor::onHeroEscaped(const PlayerColor & color, const CGHeroIns
 	sah.slotID = selectSlotForRole(color, sah.roleID);
 	sah.player = color;
 	sah.hid = hero->subID;
+	sah.army.clear();
+	sah.army.setCreature(SlotID(0), hero->type->initialArmy.at(0).creature, 1);
 
 	gameHandler->sendAndApply(&sah);
 }
@@ -134,23 +134,22 @@ void HeroPoolProcessor::selectNewHeroForSlot(const PlayerColor & color, TavernHe
 	sah.player = color;
 	sah.slotID = slot;
 
-	//first hero - native if possible, second hero -> any other class
-	CGHeroInstance *h = pickHeroFor(needNativeHero, color);
+	CGHeroInstance *newHero = pickHeroFor(needNativeHero, color);
 
-	if (h)
+	if (newHero)
 	{
-		sah.hid = h->subID;
+		sah.hid = newHero->subID;
 
 		if (giveArmy)
 		{
 			sah.roleID = TavernSlotRole::FULL_ARMY;
-			h->initArmy(getRandomGenerator(color), &sah.army);
+			newHero->initArmy(getRandomGenerator(color), &sah.army);
 		}
 		else
 		{
 			sah.roleID = TavernSlotRole::SINGLE_UNIT;
 			sah.army.clear();
-			sah.army.setCreature(SlotID(0), h->type->initialArmy[0].creature, 1);
+			sah.army.setCreature(SlotID(0), newHero->type->initialArmy[0].creature, 1);
 		}
 	}
 	else
@@ -162,11 +161,11 @@ void HeroPoolProcessor::selectNewHeroForSlot(const PlayerColor & color, TavernHe
 
 void HeroPoolProcessor::onNewWeek(const PlayerColor & color)
 {
-	const auto & hpool = gameHandler->gameState()->hpool;
-	const auto & heroes = hpool->getHeroesFor(color);
+	const auto & heroesPool = gameHandler->gameState()->heroesPool;
+	const auto & heroes = heroesPool->getHeroesFor(color);
 
-	const auto nativeSlotRole = heroes.size() < 1 ? TavernSlotRole::NONE : hpool->getSlotRole(heroes[0]->type->getId());
-	const auto randomSlotRole = heroes.size() < 2 ? TavernSlotRole::NONE : hpool->getSlotRole(heroes[1]->type->getId());
+	const auto nativeSlotRole = heroes.size() < 1 ? TavernSlotRole::NONE : heroesPool->getSlotRole(heroes[0]->type->getId());
+	const auto randomSlotRole = heroes.size() < 2 ? TavernSlotRole::NONE : heroesPool->getSlotRole(heroes[1]->type->getId());
 
 	bool resetNativeSlot = nativeSlotRole != TavernSlotRole::RETREATED_TODAY && nativeSlotRole != TavernSlotRole::SURRENDERED_TODAY;
 	bool resetRandomSlot = randomSlotRole != TavernSlotRole::RETREATED_TODAY && randomSlotRole != TavernSlotRole::SURRENDERED_TODAY;
@@ -184,10 +183,17 @@ void HeroPoolProcessor::onNewWeek(const PlayerColor & color)
 		selectNewHeroForSlot(color, TavernHeroSlot::RANDOM, false, true);
 }
 
-bool HeroPoolProcessor::hireHero(const CGObjectInstance *obj, const HeroTypeID & heroToRecruit, const PlayerColor & player)
+bool HeroPoolProcessor::hireHero(const ObjectInstanceID & objectID, const HeroTypeID & heroToRecruit, const PlayerColor & player)
 {
 	const PlayerState * playerState = gameHandler->getPlayerState(player);
-	const CGTownInstance * town = gameHandler->getTown(obj->id);
+	const CGObjectInstance * mapObject = gameHandler->getObj(objectID);
+	const CGTownInstance * town = gameHandler->getTown(objectID);
+
+	if (!mapObject && gameHandler->complain("Invalid map object!"))
+		return false;
+
+	if (!playerState && gameHandler->complain("Invalid player!"))
+		return false;
 
 	if (playerState->resources[EGameResID::GOLD] < GameConstants::HERO_GOLD_COST && gameHandler->complain("Not enough gold for buying hero!"))
 		return false;
@@ -200,6 +206,9 @@ bool HeroPoolProcessor::hireHero(const CGObjectInstance *obj, const HeroTypeID &
 
 	if(town) //tavern in town
 	{
+		if(gameHandler->getPlayerRelations(mapObject->tempOwner, player) == PlayerRelations::ENEMIES && gameHandler->complain("Can't buy hero in enemy town!"))
+			return false;
+
 		if(!town->hasBuilt(BuildingID::TAVERN) && gameHandler->complain("No tavern!"))
 			return false;
 
@@ -207,13 +216,13 @@ bool HeroPoolProcessor::hireHero(const CGObjectInstance *obj, const HeroTypeID &
 			return false;
 	}
 
-	if(obj->ID == Obj::TAVERN)
+	if(mapObject->ID == Obj::TAVERN)
 	{
-		if(gameHandler->getTile(obj->visitablePos())->visitableObjects.back() != obj && gameHandler->complain("Tavern entry must be unoccupied!"))
+		if(gameHandler->getTile(mapObject->visitablePos())->visitableObjects.back() != mapObject && gameHandler->complain("Tavern entry must be unoccupied!"))
 			return false;
 	}
 
-	auto recruitableHeroes = gameHandler->gameState()->hpool->getHeroesFor(player);
+	auto recruitableHeroes = gameHandler->gameState()->heroesPool->getHeroesFor(player);
 
 	const CGHeroInstance * recruitedHero = nullptr;
 
@@ -230,19 +239,19 @@ bool HeroPoolProcessor::hireHero(const CGObjectInstance *obj, const HeroTypeID &
 	}
 
 	HeroRecruited hr;
-	hr.tid = obj->id;
+	hr.tid = mapObject->id;
 	hr.hid = recruitedHero->subID;
 	hr.player = player;
-	hr.tile = recruitedHero->convertFromVisitablePos(obj->visitablePos());
+	hr.tile = recruitedHero->convertFromVisitablePos(mapObject->visitablePos());
 	if(gameHandler->getTile(hr.tile)->isWater())
 	{
 		//Create a new boat for hero
-		gameHandler->createObject(obj->visitablePos(), Obj::BOAT, recruitedHero->getBoatType().getNum());
+		gameHandler->createObject(mapObject->visitablePos(), Obj::BOAT, recruitedHero->getBoatType().getNum());
 
 		hr.boatId = gameHandler->getTopObj(hr.tile)->id;
 	}
 
-	// apply netpack -> this will remove hired hero from tavern slot
+	// apply netpack -> this will remove hired hero from pool
 	gameHandler->sendAndApply(&hr);
 
 	if(recruitableHeroes[0] == recruitedHero)
@@ -264,15 +273,15 @@ std::vector<const CHeroClass *> HeroPoolProcessor::findAvailableClassesFor(const
 {
 	std::vector<const CHeroClass *> result;
 
-	const auto & hpool = gameHandler->gameState()->hpool;
+	const auto & heroesPool = gameHandler->gameState()->heroesPool;
 	FactionID factionID = gameHandler->getPlayerSettings(player)->castle;
 
-	for(auto & elem : hpool->unusedHeroesFromPool())
+	for(auto & elem : heroesPool->unusedHeroesFromPool())
 	{
 		if (vstd::contains(result, elem.second->type->heroClass))
 			continue;
 
-		bool heroAvailable = hpool->isHeroAvailableFor(elem.first, player);
+		bool heroAvailable = heroesPool->isHeroAvailableFor(elem.first, player);
 		bool heroClassBanned = elem.second->type->heroClass->selectionProbability[factionID] == 0;
 
 		if(heroAvailable && !heroClassBanned)
@@ -286,13 +295,13 @@ std::vector<CGHeroInstance *> HeroPoolProcessor::findAvailableHeroesFor(const Pl
 {
 	std::vector<CGHeroInstance *> result;
 
-	const auto & hpool = gameHandler->gameState()->hpool;
+	const auto & heroesPool = gameHandler->gameState()->heroesPool;
 
-	for(auto & elem : hpool->unusedHeroesFromPool())
+	for(auto & elem : heroesPool->unusedHeroesFromPool())
 	{
 		assert(!vstd::contains(result, elem.second));
 
-		bool heroAvailable = hpool->isHeroAvailableFor(elem.first, player);
+		bool heroAvailable = heroesPool->isHeroAvailableFor(elem.first, player);
 		bool heroClassMatches = elem.second->type->heroClass == heroClass;
 
 		if(heroAvailable && heroClassMatches)
@@ -311,8 +320,8 @@ const CHeroClass * HeroPoolProcessor::pickClassFor(bool isNative, const PlayerCo
 	}
 
 	FactionID factionID = gameHandler->getPlayerSettings(player)->castle;
-	const auto & hpool = gameHandler->gameState()->hpool;
-	const auto & currentTavern = hpool->getHeroesFor(player);
+	const auto & heroesPool = gameHandler->gameState()->heroesPool;
+	const auto & currentTavern = heroesPool->getHeroesFor(player);
 
 	std::vector<const CHeroClass *> potentialClasses = findAvailableClassesFor(player);
 	std::vector<const CHeroClass *> possibleClasses;

server/HeroPoolProcessor.h

@@ -16,7 +16,7 @@ enum class TavernSlotRole : int8_t;
 class PlayerColor;
 class CGHeroInstance;
 class HeroTypeID;
-class CGObjectInstance;
+class ObjectInstanceID;
 class CRandomGenerator;
 class CHeroClass;
 
@@ -55,10 +55,12 @@ public:
 
 	void onNewWeek(const PlayerColor & color);
 
-	bool hireHero(const CGObjectInstance *obj, const HeroTypeID & hid, const PlayerColor & player);
+	/// Incoming net pack handling
+	bool hireHero(const ObjectInstanceID & objectID, const HeroTypeID & hid, const PlayerColor & player);
 
 	template <typename Handler> void serialize(Handler &h, const int version)
 	{
+		// h & gameHandler; // FIXME: make this work instead of using deserializationFix in gameHandler
 		h & playerSeed;
 	}
 };

server/NetPacksServer.cpp

@@ -248,12 +248,10 @@ void ApplyGhNetPackVisitor::visitSetFormation(SetFormation & pack)
 
 void ApplyGhNetPackVisitor::visitHireHero(HireHero & pack)
 {
-	const CGObjectInstance * obj = gh.getObj(pack.tid);
-	const CGTownInstance * town = dynamic_ptr_cast<CGTownInstance>(obj);
-	if(town && PlayerRelations::ENEMIES == gh.getPlayerRelations(obj->tempOwner, gh.getPlayerAt(pack.c)))
-		gh.throwAndComplain(&pack, "Can't buy hero in enemy town!");
+	if (!gh.hasPlayerAt(pack.player, pack.c))
+		gh.throwAndComplain(&pack, "No such pack.player!");
 
-	result = gh.heroPool->hireHero(obj, pack.hid, pack.player);
+	result = gh.heroPool->hireHero(pack.tid, pack.hid, pack.player);
 }
 
 void ApplyGhNetPackVisitor::visitBuildBoat(BuildBoat & pack)