CGHeroInstance* can be serialized over network even when hero has been defeated. Strongly typed hero type ID introduced.

Should fix #1260.

client/CMT.cpp

@@ -573,7 +573,7 @@ void processCommand(const std::string &message)
 		if(what == "hs")
 		{
 			BOOST_FOREACH(const CGHeroInstance *h, LOCPLINT->cb->getHeroesInfo())
-				if(h->type->ID == id1)
+				if(h->type->ID.getNum() == id1)
 					if(const CArtifactInstance *a = h->getArt(ArtifactPosition(id2)))
                         std::cout << a->nodeName();
 		}

client/NetPacksClient.cpp

@@ -239,6 +239,7 @@ void DisassembledArtifact::applyCl( CClient *cl )
 
 void HeroVisit::applyCl( CClient *cl )
 {
+	assert(hero);
 	INTERFACE_CALL_IF_PRESENT(hero->tempOwner, heroVisit, hero, obj, starting);
 }
 
@@ -487,7 +488,7 @@ void SetHeroesInTown::applyCl( CClient *cl )
 
 void HeroRecruited::applyCl( CClient *cl )
 {
-	CGHeroInstance *h = GS(cl)->map->heroes.back();
+	CGHeroInstance *h = GS(cl)->map->heroesOnMap.back();
 	if(h->subID != hid)
 	{
         logNetwork->errorStream() << "Something wrong with hero recruited!";

client/mapHandler.cpp

@@ -371,11 +371,11 @@ void CMapHandler::init()
 	offsetX = (mapW - (2*frameW+1)*32)/2;
 	offsetY = (mapH - (2*frameH+1)*32)/2;
 
-	for(int i=0;i<map->heroes.size();i++)
+	for(int i=0;i<map->heroesOnMap.size();i++)
 	{
-		if( !graphics->getDef(map->heroes[i]) )
+		if( !graphics->getDef(map->heroesOnMap[i]) )
 		{
-			initHeroDef(map->heroes[i]);
+			initHeroDef(map->heroesOnMap[i]);
 		}
 	}
 

lib/CGameState.cpp

@@ -678,7 +678,7 @@ void CGameState::randomizeObject(CGObjectInstance *cur)
 		h->type = VLC->heroh->heroes[ran.second];
 		h->portrait = h->type->imageIndex;
 		h->randomizeArmy(h->type->heroClass->faction);
-		map->heroes.push_back(h);
+		map->heroesOnMap.push_back(h);
 		return; //TODO: maybe we should do something with definfo?
 	}
 	else if(ran.first==Obj::TOWN)//special code for town
@@ -1071,7 +1071,7 @@ void CGameState::init(StartInfo * si)
 				CGHeroInstance * nnn =  static_cast<CGHeroInstance*>(createObject(Obj::HERO,h,hpos,it->first));
 				nnn->id = ObjectInstanceID(map->objects.size());
 				nnn->initHero();
-				map->heroes.push_back(nnn);
+				map->heroesOnMap.push_back(nnn);
 				map->objects.push_back(nnn);
 				map->addBlockVisTiles(nnn);
 			}
@@ -1130,7 +1130,7 @@ void CGameState::init(StartInfo * si)
 					if (!found)
 					{
 						CGHeroInstance * nh = new CGHeroInstance();
-						nh->initHero(hp->subID);
+						nh->initHero(HeroTypeID(hp->subID));
 						replaceHero(gid, nh);
 					}
 				}
@@ -1236,7 +1236,7 @@ void CGameState::init(StartInfo * si)
 		heroToPlace->id = obj.second;
 		heroToPlace->tempOwner = placeholder->tempOwner;
 		heroToPlace->pos = placeholder->pos;
-		heroToPlace->type = VLC->heroh->heroes[heroToPlace->type->ID];
+		heroToPlace->type = VLC->heroh->heroes[heroToPlace->type->ID.getNum()]; //TODO is this reasonable? either old type can be still used or it can be deleted?
 
 		BOOST_FOREACH(auto &&i, heroToPlace->stacks)
 			i.second->type = VLC->creh->creatures[i.second->getCreatureID()];
@@ -1253,57 +1253,65 @@ void CGameState::init(StartInfo * si)
 		BOOST_FOREACH(auto &&i, heroToPlace->artifactsInBackpack)
 			fixArtifact(i.artifact);
 
-		map->heroes.push_back(heroToPlace);
+		map->heroesOnMap.push_back(heroToPlace);
 		map->objects[heroToPlace->id.getNum()] = heroToPlace;
 		map->addBlockVisTiles(heroToPlace);
 
 		//const auto & travelOptions = scenarioOps->campState->getCurrentScenario().travelOptions;
 	}
 
-
-	std::set<int> hids; //hero ids to create pool
+	
+	std::set<HeroTypeID> heroesToCreate; //ids of heroes to be created and put into the pool
 
 	for(ui32 i=0; i<map->allowedHeroes.size(); i++) //add to hids all allowed heroes
 		if(map->allowedHeroes[i])
-			hids.insert(i);
+			heroesToCreate.insert(HeroTypeID(i));
 
-	for (ui32 i=0; i<map->heroes.size();i++) //heroes instances initialization
+	BOOST_FOREACH(auto hero, map->heroesOnMap)  //heroes instances initialization
 	{
-		if (map->heroes[i]->getOwner() == PlayerColor::UNFLAGGABLE)
+		if (hero->getOwner() == PlayerColor::UNFLAGGABLE)
 		{
             logGlobal->warnStream() << "Warning - hero with uninitialized owner!";
 			continue;
 		}
-		CGHeroInstance * vhi = map->heroes[i];
-		vhi->initHero();
-		players.find(vhi->getOwner())->second.heroes.push_back(vhi);
-		hids.erase(vhi->subID);
-	}
-
 
+		hero->initHero();
+		getPlayer(hero->getOwner())->heroes.push_back(hero);
+		heroesToCreate.erase(hero->type->ID);
+		map->allHeroes[hero->type->ID.getNum()] = hero;
+	}
 
 	BOOST_FOREACH(auto obj, map->objects) //prisons
 	{
 		if(obj && obj->ID == Obj::PRISON)
-			hids.erase(obj->subID);
+		{
+			heroesToCreate.erase(HeroTypeID(obj->subID));
+
+			map->allHeroes[obj->subID] = dynamic_cast<CGHeroInstance*>(obj.get());
+		}
 	}
 
 	BOOST_FOREACH(auto ph, map->predefinedHeroes)
 	{
-		if(!vstd::contains(hids, ph->subID))
+		if(!vstd::contains(heroesToCreate, HeroTypeID(ph->subID)))
 			continue;
 		ph->initHero();
 		hpool.heroesPool[ph->subID] = ph;
 		hpool.pavailable[ph->subID] = 0xff;
-		hids.erase(ph->subID);
+		heroesToCreate.erase(ph->type->ID);
+
+		map->allHeroes[ph->subID] = ph;
 	}
 
-	BOOST_FOREACH(int hid, hids) //all not used allowed heroes go with default state into the pool
+	BOOST_FOREACH(HeroTypeID htype, heroesToCreate) //all not used allowed heroes go with default state into the pool
 	{
 		CGHeroInstance * vhi = new CGHeroInstance();
-		vhi->initHero(hid);
-		hpool.heroesPool[hid] = vhi;
-		hpool.pavailable[hid] = 0xff;
+		vhi->initHero(htype);
+
+		int typeID = htype.getNum();
+		map->allHeroes[typeID] = vhi;
+		hpool.heroesPool[typeID] = vhi;
+		hpool.pavailable[typeID] = 0xff;
 	}
 
 	for(ui32 i=0; i<map->disposedHeroes.size(); i++)
@@ -1684,7 +1692,7 @@ void CGameState::initDuel()
 			BOOST_FOREACH(TSecSKill secSkill, ss.heroSecSkills)
 				h->setSecSkillLevel(SecondarySkill(secSkill.first), secSkill.second, 1);
 
-			h->initHero(h->subID);
+			h->initHero(HeroTypeID(h->subID));
 			obj->initObj();
 		}
 		else

lib/CGameState.h

@@ -367,7 +367,6 @@ public:
 	bmap<PlayerColor, PlayerState> players;
 	bmap<TeamID, TeamState> teams;
 	CBonusSystemNode globalEffects;
-	bmap<const CGHeroInstance*, const CGObjectInstance*> ongoingVisits;
 
 	struct DLL_LINKAGE HeroesPool
 	{

lib/CHeroHandler.cpp

@@ -469,24 +469,24 @@ std::vector<JsonNode> CHeroHandler::loadLegacyData(size_t dataSize)
 void CHeroHandler::loadObject(std::string scope, std::string name, const JsonNode & data)
 {
 	auto object = loadFromJson(data);
-	object->ID = heroes.size();
+	object->ID = HeroTypeID(heroes.size());
 	object->imageIndex = heroes.size() + 8; // 2 special frames + some extra portraits
 
 	heroes.push_back(object);
 
-	VLC->modh->identifiers.registerObject(scope, "hero", name, object->ID);
+	VLC->modh->identifiers.registerObject(scope, "hero", name, object->ID.getNum());
 }
 
 void CHeroHandler::loadObject(std::string scope, std::string name, const JsonNode & data, size_t index)
 {
 	auto object = loadFromJson(data);
-	object->ID = index;
+	object->ID = HeroTypeID(index);
 	object->imageIndex = index;
 
 	assert(heroes[index] == nullptr); // ensure that this id was not loaded before
 	heroes[index] = object;
 
-	VLC->modh->identifiers.registerObject(scope, "hero", name, object->ID);
+	VLC->modh->identifiers.registerObject(scope, "hero", name, object->ID.getNum());
 }
 
 ui32 CHeroHandler::level (ui64 experience) const

lib/CHeroHandler.h

@@ -59,7 +59,7 @@ public:
 		}
 	};
 
-	si32 ID;
+	HeroTypeID ID;
 	si32 imageIndex;
 
 	std::vector<InitialArmyStack> initialArmy;

lib/CObjectHandler.cpp

@@ -757,9 +757,9 @@ CGHeroInstance::CGHeroInstance()
 	secSkills.push_back(std::make_pair(SecondarySkill::DEFAULT, -1));
 }
 
-void CGHeroInstance::initHero(int SUBID)
+void CGHeroInstance::initHero(HeroTypeID SUBID)
 {
-	subID = SUBID;
+	subID = SUBID.getNum();
 	initHero();
 }
 
@@ -4355,7 +4355,7 @@ bool CQuest::checkQuest (const CGHeroInstance * h) const
 			}
 			return true;
 		case MISSION_HERO:
-			if (m13489val == h->type->ID)
+			if (m13489val == h->type->ID.getNum())
 				return true;
 			return false;
 		case MISSION_PLAYER:

lib/CObjectHandler.h

@@ -405,7 +405,7 @@ public:
 	//////////////////////////////////////////////////////////////////////////
 
 	void initHero();
-	void initHero(int SUBID);
+	void initHero(HeroTypeID SUBID);
 
 	void putArtifact(ArtifactPosition pos, CArtifactInstance *art);
 	void putInBackpack(CArtifactInstance *art);

lib/Connection.cpp

@@ -468,12 +468,21 @@ CSerializer::CSerializer()
 
 void CSerializer::addStdVecItems(CGameState *gs, LibClasses *lib)
 {
-	registerVectoredType(&gs->map->objects, &CGObjectInstance::id);
-	registerVectoredType(&lib->heroh->heroes, &CHero::ID);
-	registerVectoredType(&lib->creh->creatures, &CCreature::idNumber);
-	registerVectoredType(&lib->arth->artifacts, &CArtifact::id);
-	registerVectoredType(&gs->map->artInstances, &CArtifactInstance::id);
-	registerVectoredType(&gs->map->quests, &CQuest::qid);
+	registerVectoredType<CGObjectInstance, ObjectInstanceID>(&gs->map->objects, 
+		[](const CGObjectInstance &obj){ return obj.id; });
+	registerVectoredType<CHero, HeroTypeID>(&lib->heroh->heroes, 
+		[](const CHero &h){ return h.ID; });
+	registerVectoredType<CGHeroInstance, HeroTypeID>(&gs->map->allHeroes,
+		[](const CGHeroInstance &h){ return h.type->ID; });
+	registerVectoredType<CCreature, CreatureID>(&lib->creh->creatures, 
+		[](const CCreature &cre){ return cre.idNumber; });
+	registerVectoredType<CArtifact, ArtifactID>(&lib->arth->artifacts,
+		[](const CArtifact &art){ return art.id; });
+	registerVectoredType<CArtifactInstance, ArtifactInstanceID>(&gs->map->artInstances, 
+		[](const CArtifactInstance &artInst){ return artInst.id; });
+	registerVectoredType<CQuest, si32>(&gs->map->quests, 
+		[](const CQuest &q){ return q.qid; });
+
 	smartVectorMembersSerialization = true;
 }
 

lib/Connection.h

@@ -297,14 +297,15 @@ struct SerializationLevel
 	static const int value = SerializationLevel::type::value;
 };
 
-template <typename T, typename U>
+template <typename ObjType, typename IdType>
 struct VectorisedObjectInfo
 {
-	const std::vector<ConstTransitivePtr<T> > *vector;	//pointer to the appropriate vector
-	const U T::*idPtr;			//pointer to the field representing the position in the vector
+	const std::vector<ConstTransitivePtr<ObjType> > *vector;	//pointer to the appropriate vector
+	std::function<IdType(const ObjType &)> idRetriever;
+	//const IdType ObjType::*idPtr;			//pointer to the field representing the position in the vector
 
-	VectorisedObjectInfo(const std::vector< ConstTransitivePtr<T> > *Vector, const U T::*IdPtr)
-		:vector(Vector), idPtr(IdPtr)
+	VectorisedObjectInfo(const std::vector< ConstTransitivePtr<ObjType> > *Vector, std::function<IdType(const ObjType &)> IdGetter)
+		:vector(Vector), idRetriever(IdGetter)
 	{
 	}
 };
@@ -337,14 +338,14 @@ public:
     virtual void reportState(CLogger * out){};
 
 	template <typename T, typename U>
-	void registerVectoredType(const std::vector<T*> *Vector, const U T::*IdPtr)
+	void registerVectoredType(const std::vector<T*> *Vector, const std::function<U(const T&)> &idRetriever)
 	{
-		vectors[&typeid(T)] = VectorisedObjectInfo<T, U>(Vector, IdPtr);
+		vectors[&typeid(T)] = VectorisedObjectInfo<T, U>(Vector, idRetriever);
 	}
 	template <typename T, typename U>
-	void registerVectoredType(const std::vector<ConstTransitivePtr<T> > *Vector, const U T::*IdPtr)
+	void registerVectoredType(const std::vector<ConstTransitivePtr<T> > *Vector, const std::function<U(const T&)> &idRetriever)
 	{
-		vectors[&typeid(T)] = VectorisedObjectInfo<T, U>(Vector, IdPtr);
+		vectors[&typeid(T)] = VectorisedObjectInfo<T, U>(Vector, idRetriever);
 	}
 
 	template <typename T, typename U>
@@ -387,7 +388,7 @@ public:
 		if(!obj)
 			return U(-1);
 
-		return obj->*oInfo.idPtr;
+		return oInfo.idRetriever(*obj);
 	}
 
 	void addStdVecItems(CGameState *gs, LibClasses *lib = VLC);
@@ -422,11 +423,14 @@ struct VectorisedTypeFor
 {
 	typedef typename
 		//if
+		mpl::eval_if<boost::is_same<CGHeroInstance,T>,
+		mpl::identity<CGHeroInstance>,
+		//else if
 		mpl::eval_if<boost::is_base_of<CGObjectInstance,T>,
 		mpl::identity<CGObjectInstance>,
 		//else
 		mpl::identity<T>
-		>::type type;
+		> >::type type;
 };
 template <typename U>
 struct VectorizedIDType
@@ -439,14 +443,20 @@ struct VectorizedIDType
 		mpl::eval_if<boost::is_same<CCreature,U>,
 		mpl::identity<CreatureID>,
 		//else if
+		mpl::eval_if<boost::is_same<CHero,U>,
+		mpl::identity<HeroTypeID>,
+		//else if
 		mpl::eval_if<boost::is_same<CArtifactInstance,U>,
 		mpl::identity<ArtifactInstanceID>,
 		//else if
+		mpl::eval_if<boost::is_same<CGHeroInstance,U>,
+		mpl::identity<HeroTypeID>,
+		//else if
 		mpl::eval_if<boost::is_base_of<CGObjectInstance,U>,
 		mpl::identity<ObjectInstanceID>,
 		//else
 		mpl::identity<si32>
-		> > > >::type type;
+		> > > > > >::type type;
 };
 
 template <typename Handler>

lib/GameConstants.h

@@ -190,6 +190,12 @@ class ObjectInstanceID : public BaseForID<ObjectInstanceID, si32>
 	friend class CNonConstInfoCallback;
 };
 
+
+class HeroTypeID : public BaseForID<HeroTypeID, si32>
+{
+	INSTID_LIKE_CLASS_COMMON(HeroTypeID, si32)
+};
+
 class SlotID : public BaseForID<SlotID, si32>
 {
 	INSTID_LIKE_CLASS_COMMON(SlotID, si32)

lib/IGameCallback.cpp

@@ -767,7 +767,7 @@ std::vector < const CGHeroInstance *> CPlayerSpecificInfoCallback::getHeroesInfo
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
 	std::vector < const CGHeroInstance *> ret;
-	BOOST_FOREACH(auto hero, gs->map->heroes)
+	BOOST_FOREACH(auto hero, gs->map->heroesOnMap)
 	{
 		if( !player || (hero->tempOwner == *player) ||
 			(isVisible(hero->getPosition(false), player) && !onlyOur)	)
@@ -946,7 +946,7 @@ const TeamState * CGameInfoCallback::getPlayerTeam( PlayerColor color ) const
 
 const CGHeroInstance* CGameInfoCallback::getHeroWithSubid( int subid ) const
 {
-	BOOST_FOREACH(const CGHeroInstance *h, gs->map->heroes)
+	BOOST_FOREACH(const CGHeroInstance *h, gs->map->heroesOnMap)
 		if(h->subID == subid)
 			return h;
 

lib/NetPacksLib.cpp

@@ -308,7 +308,7 @@ DLL_LINKAGE void RemoveObject::applyGs( CGameState *gs )
 	{
 		CGHeroInstance *h = static_cast<CGHeroInstance*>(obj);
 		PlayerState *p = gs->getPlayer(h->tempOwner);
-		gs->map->heroes -= h;
+		gs->map->heroesOnMap -= h;
 		p->heroes -= h;
 		h->detachFrom(h->whereShouldBeAttached(gs));
 		h->tempOwner = PlayerColor::NEUTRAL; //no one owns beaten hero
@@ -510,7 +510,7 @@ DLL_LINKAGE void HeroRecruited::applyGs( CGameState *gs )
 		gs->map->objects[h->id.getNum()] = h;
 
 	h->initHeroDefInfo();
-	gs->map->heroes.push_back(h);
+	gs->map->heroesOnMap.push_back(h);
 	p->heroes.push_back(h);
 	h->attachTo(p);
 	h->initObj();
@@ -534,7 +534,7 @@ DLL_LINKAGE void GiveHero::applyGs( CGameState *gs )
 	h->setOwner(player);
 	h->movement =  h->maxMovePoints(true);
 	h->initHeroDefInfo();
-	gs->map->heroes.push_back(h);
+	gs->map->heroesOnMap.push_back(h);
 	gs->getPlayer(h->getOwner())->heroes.push_back(h);
 	gs->map->addBlockVisTiles(h);
 	h->inTownGarrison = false;
@@ -894,10 +894,6 @@ DLL_LINKAGE void DisassembledArtifact::applyGs( CGameState *gs )
 
 DLL_LINKAGE void HeroVisit::applyGs( CGameState *gs )
 {
-	if(starting)
-		gs->ongoingVisits[hero] = obj;
-	else
-		gs->ongoingVisits.erase(hero);
 }
 
 DLL_LINKAGE void SetAvailableArtifacts::applyGs( CGameState *gs )

lib/mapping/CMap.cpp

@@ -230,9 +230,9 @@ void CMap::addBlockVisTiles(CGObjectInstance * obj)
 
 CGHeroInstance * CMap::getHero(int heroID)
 {
-	for(ui32 i=0; i<heroes.size();i++)
-		if(heroes[i]->subID == heroID)
-			return heroes[i];
+	for(ui32 i=0; i<heroesOnMap.size();i++)
+		if(heroesOnMap[i]->subID == heroID)
+			return heroesOnMap[i];
 	return nullptr;
 }
 

lib/mapping/CMap.h

@@ -366,11 +366,17 @@ public:
 	int3 grailPos;
 	int grailRadious;
 
+
+	//Central lists of items in game. Position of item in the vectors below is their (instance) id.
 	std::vector< ConstTransitivePtr<CGObjectInstance> > objects;
-	std::vector< ConstTransitivePtr<CGHeroInstance> > heroes;
 	std::vector< ConstTransitivePtr<CGTownInstance> > towns;
 	std::vector< ConstTransitivePtr<CArtifactInstance> > artInstances;
 	std::vector< ConstTransitivePtr<CQuest> > quests;
+	std::vector< ConstTransitivePtr<CGHeroInstance> > allHeroes; //indexed by [hero_type_id]; on map, disposed, prisons, etc.
+
+	//Helper lists
+	std::vector< ConstTransitivePtr<CGHeroInstance> > heroesOnMap;
+
 	/// associative list to identify which hero/creature id belongs to which object id(index for objects)
 	bmap<si32, ObjectInstanceID> questIdentifierToId;
 
@@ -428,7 +434,7 @@ public:
 		}
 
 		h & customDefs & objects;
-		h & heroes & towns & artInstances;
+		h & heroesOnMap & towns & artInstances;
 
 		// static members
 		h & CGTeleport::objs;

lib/mapping/CMapEditManager.cpp

@@ -924,7 +924,7 @@ void CInsertObjectOperation::execute()
 	}
 	if(obj->ID == Obj::HERO)
 	{
-		map->heroes.push_back(static_cast<CGHeroInstance*>(obj));
+		map->heroesOnMap.push_back(static_cast<CGHeroInstance*>(obj));
 	}
 	map->addBlockVisTiles(obj);
 }

lib/mapping/MapFormatH3M.cpp

@@ -92,6 +92,8 @@ void CMapLoaderH3M::init()
 	readHeader();
 	times.push_back(MapLoadingTime("header", sw.getDiff()));
 
+	map->allHeroes.resize(map->allowedHeroes.size());
+
 	readDisposedHeroes();
 	times.push_back(MapLoadingTime("disposed heroes", sw.getDiff()));
 
@@ -1469,11 +1471,11 @@ void CMapLoaderH3M::readObjects()
 		}
 		if(nobj->ID == Obj::HERO)
 		{
-			map->heroes.push_back(static_cast<CGHeroInstance*>(nobj));
+			map->heroesOnMap.push_back(static_cast<CGHeroInstance*>(nobj));
 		}
 	}
 
-	std::sort(map->heroes.begin(), map->heroes.end(), [](const ConstTransitivePtr<CGHeroInstance> & a, const ConstTransitivePtr<CGHeroInstance> & b)
+	std::sort(map->heroesOnMap.begin(), map->heroesOnMap.end(), [](const ConstTransitivePtr<CGHeroInstance> & a, const ConstTransitivePtr<CGHeroInstance> & b)
 	{
 		return a->subID < b->subID;
 	});

server/CGameHandler.cpp

@@ -4840,6 +4840,7 @@ void CGameHandler::objectVisitEnded(const CObjectVisitQuery &query)
 	HeroVisit hv;
 	hv.obj = nullptr; //not necessary, moreover may have been deleted in the meantime
 	hv.hero = query.visitingHero;
+	assert(hv.hero);
 	hv.starting = false;
 	sendAndApply(&hv);
 }
@@ -4977,7 +4978,7 @@ void CGameHandler::checkLossVictory( PlayerColor player )
 		if(gs->scenarioOps->campState)
 		{
 			std::vector<CGHeroInstance *> hes;
-			BOOST_FOREACH(CGHeroInstance * ghi, gs->map->heroes)
+			BOOST_FOREACH(CGHeroInstance * ghi, gs->map->heroesOnMap)
 			{
 				if (ghi->tempOwner == player)
 				{