CMap towns and heroes on map fields are now private

AI/VCAI/Goals/GatherTroops.cpp

@@ -44,7 +44,7 @@ TSubgoal GatherTroops::whatToDoToAchieve()
 {
 	logAi->trace("Entering GatherTroops::whatToDoToAchieve");
 
-	auto heroes = cb->getHeroesInfo(true);
+	auto heroes = cb->getHeroesInfo();
 
 	for(auto hero : heroes)
 	{

client/NetPacksClient.cpp

@@ -625,18 +625,18 @@ void ApplyClientNetPackVisitor::visitSetHeroesInTown(SetHeroesInTown & pack)
 
 void ApplyClientNetPackVisitor::visitHeroRecruited(HeroRecruited & pack)
 {
-	auto & h = gs.getMap().heroesOnMap.back();
+	auto * h = gs.getMap().getHero(pack.hid);
 	if(h->getHeroTypeID() != pack.hid)
 	{
 		logNetwork->error("Something wrong with hero recruited!");
 	}
 
-	if(callInterfaceIfPresent(cl, h->tempOwner, &IGameEventsReceiver::heroCreated, h.get()))
+	if(callInterfaceIfPresent(cl, h->tempOwner, &IGameEventsReceiver::heroCreated, h))
 	{
 		if(const CGTownInstance *t = gs.getTown(pack.tid))
 			callInterfaceIfPresent(cl, h->getOwner(), &IGameEventsReceiver::heroInGarrisonChange, t);
 	}
-	GAME->map().onObjectInstantAdd(h.get(), h->getOwner());
+	GAME->map().onObjectInstantAdd(h, h->getOwner());
 }
 
 void ApplyClientNetPackVisitor::visitGiveHero(GiveHero & pack)

lib/CGameInfoCallback.cpp

@@ -774,20 +774,10 @@ std::vector < const CGTownInstance *> CPlayerSpecificInfoCallback::getTownsInfo(
 	} //	for ( std::map<int, PlayerState>::iterator i=gs->players.begin() ; i!=gs->players.end();i++)
 	return ret;
 }
-std::vector < const CGHeroInstance *> CPlayerSpecificInfoCallback::getHeroesInfo(bool onlyOur) const
+std::vector < const CGHeroInstance *> CPlayerSpecificInfoCallback::getHeroesInfo() const
 {
-	//std::shared_lock<std::shared_mutex> lock(*gs->mx);
-	std::vector < const CGHeroInstance *> ret;
-	for(auto hero : gs->getMap().heroesOnMap)
-	{
-		// !player || // - why would we even get access to hero not owned by any player?
-		if((hero->tempOwner == *getPlayerID()) ||
-			(isVisible(hero->visitablePos(), getPlayerID()) && !onlyOur)	)
-		{
-			ret.push_back(hero.get());
-		}
-	}
-	return ret;
+	const auto * playerState = gs->getPlayerState(*getPlayerID());
+	return playerState->getHeroes();
 }
 
 int CPlayerSpecificInfoCallback::getHeroSerial(const CGHeroInstance * hero, bool includeGarrisoned) const

lib/CGameInfoCallback.h

@@ -250,7 +250,7 @@ public:
 	virtual int getHeroSerial(const CGHeroInstance * hero, bool includeGarrisoned=true) const;
 	virtual const CGTownInstance* getTownBySerial(int serialId) const; // serial id is [0, number of towns)
 	virtual const CGHeroInstance* getHeroBySerial(int serialId, bool includeGarrisoned=true) const; // serial id is [0, number of heroes)
-	virtual std::vector <const CGHeroInstance *> getHeroesInfo(bool onlyOur = true) const; //true -> only owned; false -> all visible
+	virtual std::vector <const CGHeroInstance *> getHeroesInfo() const; //true -> only owned; false -> all visible
 	virtual std::vector <const CGObjectInstance * > getMyObjects() const; //returns all objects flagged by belonging player
 	virtual std::vector <QuestInfo> getMyQuests() const;
 

lib/gameState/CGameState.cpp

@@ -518,8 +518,9 @@ void CGameState::initPlayerStates()
 
 void CGameState::placeStartingHero(const PlayerColor & playerColor, const HeroTypeID & heroTypeId, int3 townPos)
 {
-	for(auto town : map->towns)
+	for (const auto & townID : map->getAllTowns())
 	{
+		auto town = getTown(townID);
 		if(town->anchorPos() == townPos)
 		{
 			townPos = town->visitablePos();
@@ -580,8 +581,10 @@ void CGameState::removeHeroPlaceholders()
 
 void CGameState::initHeroes()
 {
-	for(auto hero : map->heroesOnMap)  //heroes instances initialization
+	//heroes instances initialization
+	for (auto heroID : map->getHeroesOnMap())
 	{
+		auto hero = getHero(heroID);
 		if (hero->getOwner() == PlayerColor::UNFLAGGABLE)
 		{
 			logGlobal->warn("Hero with uninitialized owner!");
@@ -591,8 +594,9 @@ void CGameState::initHeroes()
 	}
 
 	// generate boats for all heroes on water
-	for(auto hero : map->heroesOnMap)
+	for (auto heroID : map->getHeroesOnMap())
 	{
+		auto hero = getHero(heroID);
 		assert(map->isInTheMap(hero->visitablePos()));
 		const auto & tile = map->getTile(hero->visitablePos());
 		if (tile.isWater())
@@ -626,7 +630,7 @@ void CGameState::initHeroes()
 		if(!vstd::contains(heroesToCreate, ph->getHeroTypeID()))
 			continue;
 		ph->initHero(getRandomGenerator());
-		heroesPool->addHeroToPool(ph.get());
+		heroesPool->addHeroToPool(ph);
 		heroesToCreate.erase(ph->getHeroTypeID());
 	}
 
@@ -637,7 +641,7 @@ void CGameState::initHeroes()
 
 		int typeID = htype.getNum();
 		map->allHeroes[typeID] = vhi;
-		heroesPool->addHeroToPool(vhi.get());
+		heroesPool->addHeroToPool(vhi);
 	}
 
 	for(auto & elem : map->disposedHeroes)
@@ -739,9 +743,9 @@ void CGameState::initTownNames()
 		}
 	}
 
-	for(auto & vti : map->towns)
+	for (const auto & townID : map->getAllTowns())
 	{
-		assert(vti->getTown());
+		auto vti = getTown(townID);
 
 		if(!vti->getNameTextID().empty())
 			continue;
@@ -781,9 +785,10 @@ void CGameState::initTowns()
 	map->townUniversitySkills.push_back(SecondarySkill(SecondarySkill::WATER_MAGIC));
 	map->townUniversitySkills.push_back(SecondarySkill(SecondarySkill::EARTH_MAGIC));
 
-	for (auto & vti : map->towns)
+	for (const auto & townID : map->getAllTowns())
 	{
-		assert(vti->getTown());
+		auto vti = getTown(townID);
+
 		assert(vti->getTown()->creatures.size() <= GameConstants::CREATURES_PER_TOWN);
 
 		constexpr std::array basicDwellings = { BuildingID::DWELL_LVL_1, BuildingID::DWELL_LVL_2, BuildingID::DWELL_LVL_3, BuildingID::DWELL_LVL_4, BuildingID::DWELL_LVL_5, BuildingID::DWELL_LVL_6, BuildingID::DWELL_LVL_7, BuildingID::DWELL_LVL_8 };
@@ -992,10 +997,12 @@ void CGameState::initVisitingAndGarrisonedHeroes()
 			}
 		}
 	}
-	for (auto hero : map->heroesOnMap)
+
+	for (auto heroID : map->getHeroesOnMap())
 	{
+		auto hero = getHero(heroID);
 		if (hero->getVisitedTown())
-			assert(hero->getVisitedTown()->getVisitingHero() == hero.get());
+			assert(hero->getVisitedTown()->getVisitingHero() == hero);
 	}
 }
 
@@ -1569,8 +1576,9 @@ void CGameState::buildBonusSystemTree()
 	buildGlobalTeamPlayerTree();
 	attachArmedObjects();
 
-	for(auto & t : map->towns)
+	for (const auto & townID : getMap().getAllTowns())
 	{
+		auto t = getTown(townID);
 		t->deserializationFix();
 	}
 }
@@ -1633,8 +1641,8 @@ std::set<HeroTypeID> CGameState::getUnusedAllowedHeroes(bool alsoIncludeNotAllow
 			ret -= HeroTypeID(playerSettingPair.second.hero);
 	}
 
-	for(auto hero : map->heroesOnMap)  //heroes instances initialization
-		ret -= hero->getHeroTypeID();
+	for (auto heroID : map->getHeroesOnMap())
+		ret -= getHero(heroID)->getHeroTypeID();
 
 	for(auto obj : map->objects) //prisons
 	{

lib/gameState/CGameStateCampaign.cpp

@@ -634,9 +634,9 @@ void CGameStateCampaign::initTowns()
 	if (chosenBonus->type != CampaignBonusType::BUILDING)
 		return;
 
-	for (int g=0; g<gameState->map->towns.size(); ++g)
+	for (const auto & townID : gameState->map->getAllTowns())
 	{
-		auto town = gameState->map->towns[g];
+		auto town = gameState->getTown(townID);
 
 		PlayerState * owner = gameState->getPlayerState(town->getOwner());
 		if (!owner)

lib/gameState/TavernHeroesPool.cpp

@@ -14,15 +14,13 @@
 
 VCMI_LIB_NAMESPACE_BEGIN
 
-TavernHeroesPool::~TavernHeroesPool()
-{
-	for(const auto & ptr : heroesPool) // clean hero pool
-		delete ptr.second;
-}
-
 std::map<HeroTypeID, CGHeroInstance*> TavernHeroesPool::unusedHeroesFromPool() const
 {
-	std::map<HeroTypeID, CGHeroInstance*> pool = heroesPool;
+	std::map<HeroTypeID, CGHeroInstance*> pool;
+
+	for (const auto & hero : heroesPool)
+		pool[hero.first] = hero.second.get();
+
 	for(const auto & slot : currentTavern)
 		pool.erase(slot.hero->getHeroTypeID());
 
@@ -48,7 +46,7 @@ void TavernHeroesPool::setHeroForPlayer(PlayerColor player, TavernHeroSlot slot,
 	if (hero == HeroTypeID::NONE)
 		return;
 
-	CGHeroInstance * h = heroesPool[hero];
+	auto h = heroesPool[hero];
 
 	if (h && army)
 		h->setToArmy(army);
@@ -60,7 +58,7 @@ void TavernHeroesPool::setHeroForPlayer(PlayerColor player, TavernHeroSlot slot,
 	}
 
 	TavernSlot newSlot;
-	newSlot.hero = h;
+	newSlot.hero = h.get();
 	newSlot.player = player;
 	newSlot.role = role;
 	newSlot.slot = slot;
@@ -97,11 +95,11 @@ std::vector<const CGHeroInstance *> TavernHeroesPool::getHeroesFor(PlayerColor c
 	return result;
 }
 
-CGHeroInstance * TavernHeroesPool::takeHeroFromPool(HeroTypeID hero)
+std::shared_ptr<CGHeroInstance> TavernHeroesPool::takeHeroFromPool(HeroTypeID hero)
 {
 	assert(heroesPool.count(hero));
 
-	CGHeroInstance * result = heroesPool[hero];
+	auto result = heroesPool[hero];
 	heroesPool.erase(hero);
 
 	vstd::erase_if(currentTavern, [&](const TavernSlot & entry){
@@ -135,7 +133,7 @@ void TavernHeroesPool::onNewDay()
 	}
 }
 
-void TavernHeroesPool::addHeroToPool(CGHeroInstance * hero)
+void TavernHeroesPool::addHeroToPool(std::shared_ptr<CGHeroInstance> hero)
 {
 	heroesPool[hero->getHeroTypeID()] = hero;
 }

lib/gameState/TavernHeroesPool.h

@@ -41,7 +41,7 @@ class DLL_LINKAGE TavernHeroesPool : public Serializeable
 	};
 
 	/// list of all heroes in pool, including those currently present in taverns
-	std::map<HeroTypeID, CGHeroInstance* > heroesPool;
+	std::map<HeroTypeID, std::shared_ptr<CGHeroInstance> > heroesPool;
 
 	/// list of which players are able to purchase specific hero
 	/// if hero is not present in list, he is available for everyone
@@ -51,8 +51,6 @@ class DLL_LINKAGE TavernHeroesPool : public Serializeable
 	std::vector<TavernSlot> currentTavern;
 
 public:
-	~TavernHeroesPool();
-
 	/// Returns heroes currently available in tavern of a specific player
 	std::vector<const CGHeroInstance *> getHeroesFor(PlayerColor color) const;
 
@@ -64,12 +62,12 @@ public:
 
 	TavernSlotRole getSlotRole(HeroTypeID hero) const;
 
-	CGHeroInstance * takeHeroFromPool(HeroTypeID hero);
+	std::shared_ptr<CGHeroInstance> takeHeroFromPool(HeroTypeID hero);
 
 	/// reset mana and movement points for all heroes in pool
 	void onNewDay();
 
-	void addHeroToPool(CGHeroInstance * hero);
+	void addHeroToPool(std::shared_ptr<CGHeroInstance> hero);
 
 	/// Marks hero as available to only specific set of players
 	void setAvailability(HeroTypeID hero, std::set<PlayerColor> mask);

lib/mapping/CMap.cpp

@@ -255,9 +255,8 @@ void CMap::calculateGuardingGreaturePositions()
 
 CGHeroInstance * CMap::getHero(HeroTypeID heroID)
 {
-	for(auto & elem : heroesOnMap)
-		if(elem->getHeroTypeID() == heroID)
-			return elem.get();
+	if (vstd::contains(heroesOnMap, heroID))
+		return allHeroes.at(heroID.getNum()).get();
 	return nullptr;
 }
 
@@ -421,32 +420,32 @@ void CMap::checkForObjectives()
 						cond.objectID = getObjectiveObjectFrom(cond.position, Obj::TOWN)->id;
 					break;
 
-//				case EventCondition::CONTROL:
-//					if (isInTheMap(cond.position))
-//						cond.objectID = getObjectiveObjectFrom(cond.position, cond.objectType.as<MapObjectID>())->id;
-//
-//					if (cond.objectID != ObjectInstanceID::NONE)
-//					{
-//						const auto * town = dynamic_cast<const CGTownInstance *>(objects[cond.objectID].get());
-//						if (town)
-//							event.onFulfill.replaceRawString(town->getNameTranslated());
-//						const auto * hero = dynamic_cast<const CGHeroInstance *>(objects[cond.objectID].get());
-//						if (hero)
-//							event.onFulfill.replaceRawString(hero->getNameTranslated());
-//					}
-//					break;
-//
-//				case EventCondition::DESTROY:
-//					if (isInTheMap(cond.position))
-//						cond.objectID = getObjectiveObjectFrom(cond.position, cond.objectType.as<MapObjectID>())->id;
-//
-//					if (cond.objectID != ObjectInstanceID::NONE)
-//					{
-//						const auto * hero = dynamic_cast<const CGHeroInstance *>(objects[cond.objectID].get());
-//						if (hero)
-//							event.onFulfill.replaceRawString(hero->getNameTranslated());
-//					}
-//					break;
+				case EventCondition::CONTROL:
+					if (isInTheMap(cond.position))
+						cond.objectID = getObjectiveObjectFrom(cond.position, cond.objectType.as<MapObjectID>())->id;
+
+					if (cond.objectID != ObjectInstanceID::NONE)
+					{
+						const auto * town = dynamic_cast<const CGTownInstance *>(objects[cond.objectID].get());
+						if (town)
+							event.onFulfill.replaceRawString(town->getNameTranslated());
+						const auto * hero = dynamic_cast<const CGHeroInstance *>(objects[cond.objectID].get());
+						if (hero)
+							event.onFulfill.replaceRawString(hero->getNameTranslated());
+					}
+					break;
+
+				case EventCondition::DESTROY:
+					if (isInTheMap(cond.position))
+						cond.objectID = getObjectiveObjectFrom(cond.position, cond.objectType.as<MapObjectID>())->id;
+
+					if (cond.objectID != ObjectInstanceID::NONE)
+					{
+						const auto * hero = dynamic_cast<const CGHeroInstance *>(objects[cond.objectID].get());
+						if (hero)
+							event.onFulfill.replaceRawString(hero->getNameTranslated());
+					}
+					break;
 				case EventCondition::TRANSPORT:
 					cond.objectID = getObjectiveObjectFrom(cond.position, Obj::TOWN)->id;
 					break;
@@ -504,109 +503,128 @@ void CMap::addNewQuestInstance(std::shared_ptr<CQuest> quest)
 	quests.emplace_back(quest);
 }
 
-void CMap::removeQuestInstance(std::shared_ptr<CQuest> quest)
+void CMap::clearQuestInstance(const CQuest * quest)
 {
-	//TODO: should be called only by map editor.
+	assert(quests.at(quest->qid).get() == quest);
 
-	//Shift indexes
-	auto iter = std::next(quests.begin(), quest->qid);
-	iter = quests.erase(iter);
-	for (int i = quest->qid; iter != quests.end(); ++i, ++iter)
+	quests.at(quest->qid) = nullptr;
+}
+
+void CMap::generateUniqueInstanceName(CGObjectInstance * target)
+{
+	//this gives object unique name even if objects are removed later
+	auto uid = uidCounter++;
+
+	boost::format fmt("%s_%d");
+	fmt % target->getTypeName() % uid;
+	target->instanceName = fmt.str();
+}
+
+void CMap::addNewObject(std::shared_ptr<CGObjectInstance> obj)
+{
+	if(obj->id != ObjectInstanceID(static_cast<si32>(objects.size())))
+		throw std::runtime_error("Invalid object instance id");
+
+	if(obj->instanceName.empty())
+		throw std::runtime_error("Object instance name missing");
+
+	if (vstd::contains(instanceNames, obj->instanceName))
+		throw std::runtime_error("Object instance name duplicated: "+obj->instanceName);
+
+	objects.emplace_back(obj);
+	instanceNames[obj->instanceName] = obj;
+	addBlockVisTiles(obj.get());
+
+	//TODO: how about defeated heroes recruited again?
+
+	obj->afterAddToMap(this);
+}
+
+void CMap::moveObject(ObjectInstanceID target, const int3 & dst)
+{
+	auto obj = objects.at(target).get();
+	removeBlockVisTiles(obj);
+	obj->setAnchorPos(dst);
+	addBlockVisTiles(obj);
+}
+
+std::shared_ptr<CGObjectInstance> CMap::removeObject(ObjectInstanceID oldObject)
+{
+	auto obj = objects.at(oldObject);
+
+	removeBlockVisTiles(obj.get());
+	instanceNames.erase(obj->instanceName);
+
+	//update indices
+
+	auto iter = std::next(objects.begin(), obj->id.getNum());
+	iter = objects.erase(iter);
+	for(int i = obj->id.getNum(); iter != objects.end(); ++i, ++iter)
 	{
-		(*iter)->qid = i;
+		(*iter)->id = ObjectInstanceID(i);
 	}
+
+	obj->afterRemoveFromMap(this);
+
+	//TODO: Clean artifact instances (mostly worn by hero?) and quests related to this object
+	//This causes crash with undo/redo in editor
+
+	return obj;
 }
 
-void CMap::clearQuestInstance(const CQuest * quest)
+std::shared_ptr<CGObjectInstance> CMap::replaceObject(ObjectInstanceID oldObjectID, const std::shared_ptr<CGObjectInstance> & newObject)
 {
-	assert(quests.at(quest->qid).get() == quest);
+	auto oldObject = objects.at(oldObjectID.getNum());
 
-	quests.at(quest->qid) = nullptr;
+	newObject->id = oldObjectID;
+
+	removeBlockVisTiles(oldObject.get(), true);
+	instanceNames.erase(oldObject->instanceName);
+
+	objects.at(oldObjectID.getNum()) = newObject;
+	addBlockVisTiles(newObject.get());
+	instanceNames[newObject->instanceName] = newObject;
+
+	oldObject->afterRemoveFromMap(this);
+	newObject->afterAddToMap(this);
+
+	return oldObject;
 }
 
-//void CMap::setUniqueInstanceName(CGObjectInstance * obj)
-//{
-//	//this gives object unique name even if objects are removed later
-//
-//	auto uid = uidCounter++;
-//
-//	boost::format fmt("%s_%d");
-//	fmt % obj->getTypeName() % uid;
-//	obj->instanceName = fmt.str();
-//}
-
-//void CMap::addNewObject(CGObjectInstance * obj)
-//{
-//	if(obj->id != ObjectInstanceID(static_cast<si32>(objects.size())))
-//		throw std::runtime_error("Invalid object instance id");
-//
-//	if(obj->instanceName.empty())
-//		throw std::runtime_error("Object instance name missing");
-//
-//	if (vstd::contains(instanceNames, obj->instanceName))
-//		throw std::runtime_error("Object instance name duplicated: "+obj->instanceName);
-//
-//	objects.emplace_back(obj);
-//	instanceNames[obj->instanceName] = obj;
-//	addBlockVisTiles(obj);
-//
-//	//TODO: how about defeated heroes recruited again?
-//
-//	obj->afterAddToMap(this);
-//}
-//
-//void CMap::moveObject(CGObjectInstance * obj, const int3 & pos)
-//{
-//	removeBlockVisTiles(obj);
-//	obj->setAnchorPos(pos);
-//	addBlockVisTiles(obj);
-//}
-//
-//void CMap::removeObject(CGObjectInstance * obj)
-//{
-//	removeBlockVisTiles(obj);
-//	instanceNames.erase(obj->instanceName);
-//
-//	//update indices
-//
-//	auto iter = std::next(objects.begin(), obj->id.getNum());
-//	iter = objects.erase(iter);
-//	for(int i = obj->id.getNum(); iter != objects.end(); ++i, ++iter)
-//	{
-//		(*iter)->id = ObjectInstanceID(i);
-//	}
-//
-//	obj->afterRemoveFromMap(this);
-//
-//	//TODO: Clean artifact instances (mostly worn by hero?) and quests related to this object
-//	//This causes crash with undo/redo in editor
-//}
-
-//void CMap::replaceObject(ObjectInstanceID oldObjectID, const std::shared_ptr<CGObjectInstance> & newObject)
-//{
-//	auto oldObject = objects.at(oldObjectID.getNum());
-//
-//	newObject->id = oldObjectID;
-//
-//	removeBlockVisTiles(oldObject.get(), true);
-//	instanceNames.erase(oldObject->instanceName);
-//
-//	objects.at(oldObjectID.getNum()) = newObject;
-//	addBlockVisTiles(newObject.get());
-//	instanceNames[newObject->instanceName] = newObject;
-//
-//	oldObject->afterRemoveFromMap(this);
-//	newObject->afterAddToMap(this);
-//}
-//
-//void CMap::eraseObject(ObjectInstanceID oldObjectID)
-//{
-//	auto oldObject = objects.at(oldObjectID.getNum());
-//
-//	objects.at(oldObjectID) = nullptr;
-//	removeBlockVisTiles(oldObject.get(), true);
-//	oldObject->afterRemoveFromMap(this);
-//}
+std::shared_ptr<CGObjectInstance> CMap::eraseObject(ObjectInstanceID oldObjectID)
+{
+	auto oldObject = objects.at(oldObjectID.getNum());
+
+	objects.at(oldObjectID) = nullptr;
+	removeBlockVisTiles(oldObject.get(), true);
+	oldObject->afterRemoveFromMap(this);
+
+	return oldObject;
+}
+
+void CMap::heroAddedToMap(const CGHeroInstance * hero)
+{
+	assert(!vstd::contains(heroesOnMap, hero->id));
+	heroesOnMap.push_back(hero->id);
+}
+
+void CMap::heroRemovedFromMap(const CGHeroInstance * hero)
+{
+	assert(vstd::contains(heroesOnMap, hero->id));
+	vstd::erase(heroesOnMap, hero->id);
+}
+
+void CMap::townAddedToMap(const CGTownInstance * town)
+{
+	assert(!vstd::contains(towns, town->id));
+	towns.push_back(town->id);
+}
+
+void CMap::townRemovedFromMap(const CGTownInstance * town)
+{
+	assert(vstd::contains(towns, town->id));
+	vstd::erase(towns, town->id);
+}
 
 bool CMap::isWaterMap() const
 {
@@ -825,4 +843,27 @@ const CArtifactInstance * CMap::getArtifactInstance(const ArtifactInstanceID & a
 	return artInstances.at(artifactID.getNum()).get();
 }
 
+const std::vector<ObjectInstanceID> & CMap::getAllTowns()
+{
+	return towns;
+}
+
+const std::vector<ObjectInstanceID> & CMap::getHeroesOnMap()
+{
+	return heroesOnMap;
+}
+
+void CMap::postInitialize()
+{
+	//TODO: check whether this is actually needed
+	boost::range::sort(heroesOnMap, [this](const ObjectInstanceID & a, const ObjectInstanceID & b)
+	{
+		const auto aHero = std::dynamic_pointer_cast<const CGHeroInstance>(objects.at(a.getNum()));
+		const auto bHero = std::dynamic_pointer_cast<const CGHeroInstance>(objects.at(b.getNum()));
+
+		return aHero->getHeroTypeID() < bHero->getHeroTypeID();
+	});
+}
+
+
 VCMI_LIB_NAMESPACE_END

lib/mapping/CMap.h

@@ -62,8 +62,12 @@ class DLL_LINKAGE CMap : public CMapHeader, public GameCallbackHolder
 
 	std::unique_ptr<GameSettings> gameSettings;
 
-	std::vector< std::shared_ptr<CQuest> > quests;
-	std::vector< std::shared_ptr<CArtifactInstance> > artInstances;
+	std::vector<std::shared_ptr<CQuest>> quests;
+	std::vector<std::shared_ptr<CArtifactInstance>> artInstances;
+
+	std::vector<ObjectInstanceID> towns;
+
+	std::vector<ObjectInstanceID> heroesOnMap;
 
 public:
 	explicit CMap(IGameCallback *cb);
@@ -97,31 +101,31 @@ public:
 	void removeArtifactInstance(CArtifactSet & set, const ArtifactPosition & slot);
 
 	void addNewQuestInstance(std::shared_ptr<CQuest> quest);
-	void removeQuestInstance(std::shared_ptr<CQuest> quest);
 	void clearQuestInstance(const CQuest * quest);
 
-	void setUniqueInstanceName(ObjectInstanceID target){};
+	void generateUniqueInstanceName(CGObjectInstance * target);
+
 	///Use only this method when creating new map object instances
-	void addNewObject(std::shared_ptr<CGObjectInstance> obj){}
-	void moveObject(ObjectInstanceID target, const int3 & dst){}
+	void addNewObject(std::shared_ptr<CGObjectInstance> obj);
+	void moveObject(ObjectInstanceID target, const int3 & dst);
 
 	/// Remove objects and shifts object indicies.
 	/// Only for use in map editor / RMG
-	std::shared_ptr<CGObjectInstance> removeObject(ObjectInstanceID oldObject){return nullptr;}
+	std::shared_ptr<CGObjectInstance> removeObject(ObjectInstanceID oldObject);
 
 	/// Replaced map object with specified ID with new object
 	/// Old object must exist and will be removed from map
 	/// Returns pointer to old object, which can be manipulated or dropped
-	std::shared_ptr<CGObjectInstance> replaceObject(ObjectInstanceID oldObject, const std::shared_ptr<CGObjectInstance> & newObject){return nullptr;}
+	std::shared_ptr<CGObjectInstance> replaceObject(ObjectInstanceID oldObject, const std::shared_ptr<CGObjectInstance> & newObject);
 
 	/// Erases object from map without shifting indices
 	/// Returns pointer to old object, which can be manipulated or dropped
-	std::shared_ptr<CGObjectInstance> eraseObject(ObjectInstanceID oldObject){return nullptr;}
+	std::shared_ptr<CGObjectInstance> eraseObject(ObjectInstanceID oldObject);
 
-	void heroAddedToMap(const CGHeroInstance * hero) {}
-	void heroRemovedFromMap(const CGHeroInstance * hero) {}
-	void townAddedToMap(const CGTownInstance * town) {}
-	void townRemovedFromMap(const CGTownInstance * town) {}
+	void heroAddedToMap(const CGHeroInstance * hero);
+	void heroRemovedFromMap(const CGHeroInstance * hero);
+	void townAddedToMap(const CGTownInstance * town);
+	void townRemovedFromMap(const CGTownInstance * town);
 
 	bool isWaterMap() const;
 	bool calculateWaterContent();
@@ -137,6 +141,14 @@ public:
 	const CGObjectInstance * getObjectiveObjectFrom(const int3 & pos, Obj type);
 	CGHeroInstance * getHero(HeroTypeID heroId);
 
+	/// Returns ID's of all heroes that are currently present on map
+	/// All garrisoned heroes are included from this list
+	/// All prisons are excluded from this list
+	const std::vector<ObjectInstanceID> & getHeroesOnMap();
+
+	/// Returns ID's of all towns present on map
+	const std::vector<ObjectInstanceID> & getAllTowns();
+
 	/// Sets the victory/loss condition objectives ??
 	void checkForObjectives();
 
@@ -155,11 +167,9 @@ public:
 
 	//Central lists of items in game. Position of item in the vectors below is their (instance) id.
 	std::vector< std::shared_ptr<CGObjectInstance> > objects;
-	std::vector< std::shared_ptr<CGTownInstance> > towns;
 	std::vector< std::shared_ptr<CGHeroInstance> > allHeroes; //indexed by [hero_type_id]; on map, disposed, prisons, etc.
 
 	//Helper lists
-	std::vector< std::shared_ptr<CGHeroInstance> > heroesOnMap;
 	std::map<TeleportChannelID, std::shared_ptr<TeleportChannel> > teleportChannels;
 	std::vector<std::shared_ptr<CGHeroInstance> > predefinedHeroes;
 
@@ -183,6 +193,8 @@ public:
 	void overrideGameSetting(EGameSettings option, const JsonNode & input);
 	const IGameSettings & getSettings() const;
 
+	void postInitialize();
+
 private:
 	/// a 3-dimensional array of terrain tiles, access is as follows: x, y, level. where level=1 is underground
 	boost::multi_array<TerrainTile, 3> terrain;

lib/mapping/CMapOperation.cpp

@@ -589,11 +589,7 @@ void CInsertObjectOperation::execute()
 {
 	obj->id = ObjectInstanceID(map->objects.size());
 
-	do
-	{
-		map->setUniqueInstanceName(obj->id);
-	} while(vstd::contains(map->instanceNames, obj->instanceName));
-
+	map->generateUniqueInstanceName(obj.get());
 	map->addNewObject(obj);
 }
 

lib/mapping/MapFormatH3M.cpp

@@ -1964,14 +1964,7 @@ void CMapLoaderH3M::readObjects()
 		map->addNewObject(newObject);
 	}
 
-	std::sort(
-		map->heroesOnMap.begin(),
-		map->heroesOnMap.end(),
-		[](const std::shared_ptr<CGHeroInstance> & a, const std::shared_ptr<CGHeroInstance> & b)
-		{
-			return a->subID < b->subID;
-		}
-	);
+	map->postInitialize();
 }
 
 void CMapLoaderH3M::readCreatureSet(CCreatureSet * out, int number)

lib/mapping/MapFormatJson.cpp

@@ -1134,11 +1134,7 @@ void CMapLoaderJson::readObjects()
 	for(auto & ptr : loaders)
 		ptr->configure();
 
-	std::sort(map->heroesOnMap.begin(), map->heroesOnMap.end(), [](const std::shared_ptr<CGHeroInstance> & a, const std::shared_ptr<CGHeroInstance> & b)
-	{
-		return a->getObjTypeIndex() < b->getObjTypeIndex();
-	});
-
+	map->postInitialize();
 
 	std::set<HeroTypeID> debugHeroesOnMap;
 	for (auto const & object : map->objects)

lib/networkPacks/NetPacksLib.cpp

@@ -1118,9 +1118,9 @@ void PlayerEndsGame::applyGs(CGameState *gs)
 		if(p->human && gs->getStartInfo()->campState)
 		{
 			std::vector<CGHeroInstance *> crossoverHeroes;
-			for (auto hero : gs->getMap().heroesOnMap)
+			for (auto hero : p->getHeroes())
 				if (hero->tempOwner == player)
-					crossoverHeroes.push_back(hero.get());
+					crossoverHeroes.push_back(hero);
 
 			gs->getStartInfo()->campState->setCurrentMapAsConquered(crossoverHeroes);
 		}
@@ -1200,9 +1200,9 @@ void RemoveObject::applyGs(CGameState *gs)
 
 	if(obj->ID == Obj::HERO) //remove beaten hero
 	{
-		auto * beatenHero = dynamic_cast<CGHeroInstance *>(obj);
+		auto beatenObject = gs->getMap().eraseObject(obj->id);
+		auto beatenHero = std::dynamic_pointer_cast<CGHeroInstance>(beatenObject);
 		assert(beatenHero);
-		gs->getMap().eraseObject(beatenHero->id);
 
 		auto * siegeNode = beatenHero->whereShouldBeAttachedOnSiege(gs);
 
@@ -1220,7 +1220,7 @@ void RemoveObject::applyGs(CGameState *gs)
 
 		if(beatenHero->getVisitedTown())
 		{
-			if(beatenHero->getVisitedTown()->getGarrisonHero() == beatenHero)
+			if(beatenHero->getVisitedTown()->getGarrisonHero() == beatenHero.get())
 				beatenHero->getVisitedTown()->setGarrisonedHero(nullptr);
 			else
 				beatenHero->getVisitedTown()->setVisitingHero(nullptr);
@@ -1424,7 +1424,7 @@ void SetHeroesInTown::applyGs(CGameState *gs)
 
 void HeroRecruited::applyGs(CGameState *gs)
 {
-	CGHeroInstance *h = gs->heroesPool->takeHeroFromPool(hid);
+	auto h = gs->heroesPool->takeHeroFromPool(hid);
 	CGTownInstance *t = gs->getTown(tid);
 	PlayerState *p = gs->getPlayerState(player);
 
@@ -1449,16 +1449,15 @@ void HeroRecruited::applyGs(CGameState *gs)
 		h->id = ObjectInstanceID(static_cast<si32>(gs->getMap().objects.size()));
 		gs->getMap().objects.emplace_back(h);
 	}
-//	else
-//		gs->getMap().replaceObject(h->id, h);
+	else
+		gs->getMap().replaceObject(h->id, h);
 
-	gs->getMap().heroesOnMap.emplace_back(h);
-	p->addOwnedObject(h);
+	gs->getMap().addNewObject(h);
+	p->addOwnedObject(h.get());
 	h->attachTo(*p);
-	gs->getMap().addBlockVisTiles(h);
 
 	if(t)
-		t->setVisitingHero(h);
+		t->setVisitingHero(h.get());
 }
 
 void GiveHero::applyGs(CGameState *gs)
@@ -1487,7 +1486,7 @@ void GiveHero::applyGs(CGameState *gs)
 	h->setOwner(player);
 	h->setMovementPoints(h->movementPointsLimit(true));
 	h->setAnchorPos(h->convertFromVisitablePos(oldVisitablePos));
-	gs->getMap().heroesOnMap.emplace_back(h);
+	gs->getMap().heroAddedToMap(h);
 	gs->getPlayerState(h->getOwner())->addOwnedObject(h);
 
 	gs->getMap().addBlockVisTiles(h);
@@ -1930,8 +1929,9 @@ void NewTurn::applyGs(CGameState *gs)
 	for(auto & creatureSet : availableCreatures) //set available creatures in towns
 		creatureSet.applyGs(gs);
 
-	for(auto & t : gs->getMap().towns)
+	for (const auto & townID : gs->getMap().getAllTowns())
 	{
+		auto t = gs->getTown(townID);
 		t->built = 0;
 		t->spellResearchCounterDay = 0;
 	}

server/CGameHandler.cpp

@@ -657,8 +657,9 @@ void CGameHandler::onNewTurn()
 		addStatistics(gameState()->statistic); // write at end of turn
 	}
 
-	for (const auto & t : gs->getMap().towns)
+	for (const auto & townID : gameState()->getMap().getAllTowns())
 	{
+		auto t = gameState()->getTown(townID);
 		PlayerColor player = t->tempOwner;
 
 		if(t->hasBuilt(BuildingID::GRAIL)
@@ -671,8 +672,9 @@ void CGameHandler::onNewTurn()
 		}
 	}
 
-	for (const auto & t : gs->getMap().towns)
+	for (const auto & townID : gameState()->getMap().getAllTowns())
 	{
+		auto t = gameState()->getTown(townID);
 		if (t->hasBonusOfType (BonusType::DARKNESS))
 		{
 			for (auto & player : gs->players)

server/processors/NewTurnProcessor.cpp

@@ -507,8 +507,9 @@ RumorState NewTurnProcessor::pickNewRumor()
 
 std::tuple<EWeekType, CreatureID> NewTurnProcessor::pickWeekType(bool newMonth)
 {
-	for (const auto & t : gameHandler->gameState()->getMap().towns)
+	for (const auto & townID : gameHandler->gameState()->getMap().getAllTowns())
 	{
+		auto t = gameHandler->gameState()->getTown(townID);
 		if (t->hasBuilt(BuildingID::GRAIL, ETownType::INFERNO))
 			return { EWeekType::DEITYOFFIRE, CreatureID::IMP };
 	}
@@ -666,8 +667,11 @@ NewTurn NewTurnProcessor::generateNewTurnPack()
 
 	if (newWeek)
 	{
-		for (const auto & t : gameHandler->gameState()->getMap().towns)
-			n.availableCreatures.push_back(generateTownGrowth(t.get(), n.specialWeek, n.creatureid, firstTurn));
+		for (const auto & townID : gameHandler->gameState()->getMap().getAllTowns())
+		{
+			auto t = gameHandler->gameState()->getTown(townID);
+			n.availableCreatures.push_back(generateTownGrowth(t, n.specialWeek, n.creatureid, firstTurn));
+		}
 	}
 
 	if (newWeek)
@@ -695,17 +699,21 @@ void NewTurnProcessor::onNewTurn()
 
 	if (newWeek)
 	{
-		for (const auto & t : gameHandler->gameState()->getMap().towns)
+		for (const auto & townID : gameHandler->gameState()->getMap().getAllTowns())
+		{
+			auto t = gameHandler->gameState()->getTown(townID);
 			if (t->hasBuilt(BuildingSubID::PORTAL_OF_SUMMONING))
-				gameHandler->setPortalDwelling(t.get(), true, (n.specialWeek == EWeekType::PLAGUE ? true : false)); //set creatures for Portal of Summoning
+				gameHandler->setPortalDwelling(t, true, (n.specialWeek == EWeekType::PLAGUE ? true : false)); //set creatures for Portal of Summoning
+		}
 	}
 
 	if (newWeek && !firstTurn)
 	{
-		for (const auto & t : gameHandler->gameState()->getMap().towns)
+		for (const auto & townID : gameHandler->gameState()->getMap().getAllTowns())
 		{
+			auto t = gameHandler->gameState()->getTown(townID);
 			if (!t->getOwner().isValidPlayer())
-				updateNeutralTownGarrison(t.get(), 1 + gameHandler->getDate(Date::DAY) / 7);
+				updateNeutralTownGarrison(t, 1 + gameHandler->getDate(Date::DAY) / 7);
 		}
 	}
 

test/game/CGameStateTest.cpp

@@ -178,15 +178,13 @@ public:
 			}
 		}
 
-
 		Load::ProgressAccumulator progressTracker;
 		gameState->init(&mapService, &si, progressTracker, false);
 
 		ASSERT_NE(map, nullptr);
-		ASSERT_EQ(map->heroesOnMap.size(), 2);
+		ASSERT_EQ(map->getHeroesOnMap().size(), 2);
 	}
 
-
 	void startTestBattle(const CGHeroInstance * attacker, const CGHeroInstance * defender)
 	{
 		BattleSideArray<const CGHeroInstance *> heroes = {attacker, defender};
@@ -226,8 +224,11 @@ TEST_F(CGameStateTest, DISABLED_issue2765)
 {
 	startTestGame();
 
-	auto attacker = map->heroesOnMap[0];
-	auto defender = map->heroesOnMap[1];
+	auto attackerID = map->getHeroesOnMap()[0];
+	auto defenderID = map->getHeroesOnMap()[1];
+
+	auto attacker = std::dynamic_pointer_cast<CGHeroInstance>(map->objects.at(attackerID.getNum()));
+	auto defender = std::dynamic_pointer_cast<CGHeroInstance>(map->objects.at(defenderID.getNum()));
 
 	ASSERT_NE(attacker->tempOwner, defender->tempOwner);
 
@@ -308,8 +309,11 @@ TEST_F(CGameStateTest, DISABLED_battleResurrection)
 {
 	startTestGame();
 
-	auto attacker = map->heroesOnMap[0];
-	auto defender = map->heroesOnMap[1];
+	auto attackerID = map->getHeroesOnMap()[0];
+	auto defenderID = map->getHeroesOnMap()[1];
+
+	auto attacker = std::dynamic_pointer_cast<CGHeroInstance>(map->objects.at(attackerID.getNum()));
+	auto defender = std::dynamic_pointer_cast<CGHeroInstance>(map->objects.at(defenderID.getNum()));
 
 	ASSERT_NE(attacker->tempOwner, defender->tempOwner);