* refactoring

AI/BattleAI/BattleAI.cpp

@@ -83,7 +83,7 @@ void CBattleAI::init( CBattleCallback * CB )
 {
 	print("init called, saving ptr to IBattleCallback");
 	cbc = cb = CB;
-	playerID = CB->getPlayerID();; //TODO should be sth in callback
+	playerID = *CB->getPlayerID();; //TODO should be sth in callback
 
 	wasWaitingForRealize = cb->waitTillRealize;
 	wasUnlockingGs = CB->unlockGsWhenWaiting;

AI/VCAI/VCAI.cpp

@@ -868,7 +868,7 @@ void VCAI::init(CCallback * CB)
 	cbc = CB;
 	NET_EVENT_HANDLER;
 	LOG_ENTRY;
-	playerID = myCb->getMyColor();
+	playerID = *myCb->getMyColor();
 	myCb->waitTillRealize = true;
 	myCb->unlockGsWhenWaiting = true;
 

CCallback.cpp

@@ -58,6 +58,7 @@ bool CCallback::moveHero(const CGHeroInstance *h, int3 dst)
 
 int CCallback::selectionMade(int selection, int queryID)
 {
+	ASSERT_IF_CALLED_WITH_PLAYER
 	if(queryID == -1)
 	{
 		tlog1 << "Cannot answer the query -1!\n";
@@ -65,7 +66,7 @@ int CCallback::selectionMade(int selection, int queryID)
 	}
 
 	QueryReply pack(queryID,selection);
-	pack.player = player;
+	pack.player = *player;
 	return sendRequest(&pack);
 }
 
@@ -97,7 +98,7 @@ bool CCallback::upgradeCreature(const CArmedInstance *obj, int stackPos, Creatur
 
 void CCallback::endTurn()
 {
-	tlog5 << "Player " << (unsigned)player << " ended his turn." << std::endl;
+	tlog5 << "Player " << (unsigned)*player << " ended his turn." << std::endl;
 	EndTurn pack;
 	sendRequest(&pack); //report that we ended turn
 }
@@ -186,7 +187,7 @@ int CBattleCallback::battleMakeAction(BattleAction* action)
 
 int CBattleCallback::sendRequest(const CPack *request)
 {
-	int requestID = cl->sendRequest(request, player);
+	int requestID = cl->sendRequest(request, *player);
 	if(waitTillRealize)
 	{
 		tlog5 << boost::format("We'll wait till request %d is answered.\n") % requestID;
@@ -199,7 +200,7 @@ int CBattleCallback::sendRequest(const CPack *request)
 
 void CCallback::swapGarrisonHero( const CGTownInstance *town )
 {
-	if(town->tempOwner != player) return;
+	if(town->tempOwner != *player) return;
 
 	GarrisonHeroSwap pack(town->id);
 	sendRequest(&pack);
@@ -235,7 +236,7 @@ void CCallback::setFormation(const CGHeroInstance * hero, bool tight)
 void CCallback::setSelection(const CArmedInstance * obj)
 {
 	SetSelection ss;
-	ss.player = player;
+	ss.player = *player;
 	ss.id = obj->id;
 	sendRequest(&(CPackForClient&)ss);
 
@@ -245,7 +246,7 @@ void CCallback::setSelection(const CArmedInstance * obj)
 			cl->calculatePaths(static_cast<const CGHeroInstance *>(obj));
 
 		//nasty workaround. TODO: nice workaround
-		cl->gs->getPlayer(player)->currentSelection = obj->id;
+		cl->gs->getPlayer(*player)->currentSelection = obj->id;
 	}
 }
 
@@ -254,12 +255,12 @@ void CCallback::recruitHero(const CGObjectInstance *townOrTavern, const CGHeroIn
 	assert(townOrTavern);
 	assert(hero);
 	ui8 i=0;
-	for(; i<gs->players[player].availableHeroes.size(); i++)
+	for(; i<gs->players[*player].availableHeroes.size(); i++)
 	{
-		if(gs->players[player].availableHeroes[i] == hero)
+		if(gs->players[*player].availableHeroes[i] == hero)
 		{
 			HireHero pack(i,townOrTavern->id);
-			pack.player = player;
+			pack.player = *player;
 			sendRequest(&pack);
 			return;
 		}
@@ -274,7 +275,8 @@ void CCallback::save( const std::string &fname )
 
 void CCallback::sendMessage(const std::string &mess)
 {
-	PlayerMessage pm(player, mess);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	PlayerMessage pm(*player, mess);
 	sendRequest(&(CPackForClient&)pm);
 }
 
@@ -285,7 +287,7 @@ void CCallback::buildBoat( const IShipyard *obj )
 	sendRequest(&bb);
 }
 
-CCallback::CCallback( CGameState * GS, int Player, CClient *C ) 
+CCallback::CCallback( CGameState * GS, boost::optional<TPlayerColor> Player, CClient *C ) 
 	:CBattleCallback(GS, Player, C)
 {
 	waitTillRealize = false;
@@ -314,7 +316,7 @@ bool CCallback::getPath2( int3 dest, CGPath &ret )
 
 void CCallback::recalculatePaths()
 {
-	cl->calculatePaths(cl->IGameCallback::getSelectedHero(player));
+	cl->calculatePaths(cl->IGameCallback::getSelectedHero(*player));
 }
 
 void CCallback::calculatePaths( const CGHeroInstance *hero, CPathsInfo &out, int3 src /*= int3(-1,-1,-1)*/, int movement /*= -1*/ )
@@ -340,15 +342,16 @@ void CCallback::castSpell(const CGHeroInstance *hero, SpellID spellID, const int
 
 void CCallback::unregisterMyInterface()
 {
-	assert(player >= 0); //works only for player callback
+	ASSERT_IF_CALLED_WITH_PLAYER
-	cl->playerint.erase(player);
+	cl->playerint.erase(*player);
-	cl->battleints.erase(player);
+	cl->battleints.erase(*player);
 	//TODO? should callback be disabled as well?
 }
 
 void CCallback::validatePaths()
 {
-	const CGHeroInstance *h = cl->IGameCallback::getSelectedHero(player);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	const CGHeroInstance *h = cl->IGameCallback::getSelectedHero(*player);
 	if(h  && ( cl->pathInfo->hero != h							//wrong hero
 		       || cl->pathInfo->hpos != h->getPosition(false)  //wrong hero positoin
 		       || !cl->pathInfo->isValid)) //paths invalidated by game event
@@ -365,7 +368,7 @@ int CCallback::mergeOrSwapStacks(const CArmedInstance *s1, const CArmedInstance
 		return swapCreatures(s1, s2, p1, p2);
 }
 
-CBattleCallback::CBattleCallback(CGameState *GS, int Player, CClient *C )
+CBattleCallback::CBattleCallback(CGameState *GS, boost::optional<TPlayerColor> Player, CClient *C )
 {
 	gs = GS;
 	player = Player;

CCallback.h

@@ -86,7 +86,7 @@ protected:
 	//virtual bool hasAccess(int playerId) const;
 
 public:
-	CBattleCallback(CGameState *GS, int Player, CClient *C);
+	CBattleCallback(CGameState *GS, boost::optional<TPlayerColor> Player, CClient *C);
 	int battleMakeAction(BattleAction* action) OVERRIDE;//for casting spells by hero - DO NOT use it for moving active stack
 	bool battleMakeTacticAction(BattleAction * action) OVERRIDE; // performs tactic phase actions
 	
@@ -101,7 +101,7 @@ private:
 	void validatePaths(); //recalcualte paths if necessary
 
 public:
-	CCallback(CGameState * GS, int Player, CClient *C);
+	CCallback(CGameState * GS, boost::optional<TPlayerColor> Player, CClient *C);
 
 	//client-specific functionalities (pathfinding)
 	virtual const CGPathNode *getPathInfo(int3 tile); //uses main, client pathfinder info

Global.h

@@ -519,6 +519,8 @@ template<typename T, size_t N> char (&_ArrayCountObj(const T (&)[N]))[N];
 
 #define THROW_FORMAT(message, formatting_elems)  throw std::runtime_error(boost::str(boost::format(message) % formatting_elems))
 
+#define ASSERT_IF_CALLED_WITH_PLAYER if(!player) {tlog1 << __FUNCTION__ << "\n"; assert(0);}
+
 //XXX pls dont - 'debug macros' are usually more trouble than it's worth
 #define HANDLE_EXCEPTION  \
 	catch (const std::exception& e) {	\

VCMI_global_debug.props

@@ -4,7 +4,10 @@
   <PropertyGroup Label="UserMacros">
     <VCMI_Out>$(SolutionDir)</VCMI_Out>
   </PropertyGroup>
-  <PropertyGroup />
+  <PropertyGroup>
+    <IncludePath>C:\Program Files (x86)\Microsoft SDKs\Windows\v7.1A\Include;$(IncludePath)</IncludePath>
+    <LibraryPath>C:\Program Files (x86)\Microsoft SDKs\Windows\v7.1A\Lib;G:\boost\SDL_mixer-1.2.11\lib;G:\boost\zlib125-dll\lib;G:\boost\SDL-1.2.14\lib;G:\boost\SDL_image-1.2.10\lib;G:\boost\SDL_ttf-2.0.10\lib;G:\boost\boost_1_51_0\stage\lib;$(VCInstallDir)lib;$(VCInstallDir)atlmfc\lib;$(WindowsSdkDir)lib;$(FrameworkSDKDir)\lib</LibraryPath>
+  </PropertyGroup>
   <ItemDefinitionGroup>
     <ClCompile>
       <RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>

client/CMessage.cpp

@@ -114,7 +114,7 @@ void CMessage::dispose()
 	delete cancel;
 }
 
-SDL_Surface * CMessage::drawDialogBox(int w, int h, int playerColor)
+SDL_Surface * CMessage::drawDialogBox(int w, int h, TPlayerColor playerColor)
 {
 	//prepare surface
 	SDL_Surface * ret = SDL_CreateRGBSurface(SDL_SWSURFACE, w, h, screen->format->BitsPerPixel, screen->format->Rmask, screen->format->Gmask, screen->format->Bmask, screen->format->Amask);
@@ -219,7 +219,7 @@ std::vector<std::string> CMessage::breakText( std::string text, size_t maxLineSi
 	return ret;
 }
 
-void CMessage::drawIWindow(CInfoWindow * ret, std::string text, int player)
+void CMessage::drawIWindow(CInfoWindow * ret, std::string text, TPlayerColor player)
 {
 	bool blitOr = false;
 	if(dynamic_cast<CSelWindow*>(ret)) //it's selection window, so we'll blit "or" between components
@@ -304,7 +304,7 @@ void CMessage::drawIWindow(CInfoWindow * ret, std::string text, int player)
 		ret->components[i]->moveBy(Point(ret->pos.x, ret->pos.y));
 }
 
-void CMessage::drawBorder(int playerColor, SDL_Surface * ret, int w, int h, int x, int y)
+void CMessage::drawBorder(TPlayerColor playerColor, SDL_Surface * ret, int w, int h, int x, int y)
 {	
 	std::vector<SDL_Surface *> &box = piecesOfBox[playerColor];
 

client/CMessage.h

@@ -31,12 +31,12 @@ public:
 	static SDL_Surface * blitTextOnSur(std::vector<std::vector<SDL_Surface*> > * txtg, int fontHeight, int & curh, SDL_Surface * ret, int xCenterPos=-1); //xPos==-1 works as if ret->w/2
 
 	/// Draw border on exiting surface
-	static void drawBorder(int playerColor, SDL_Surface * ret, int w, int h, int x=0, int y=0);
+	static void drawBorder(TPlayerColor playerColor, SDL_Surface * ret, int w, int h, int x=0, int y=0);
 
 	/// Draw simple dialog box (borders and background only)
-	static SDL_Surface * drawDialogBox(int w, int h, int playerColor=1);
+	static SDL_Surface * drawDialogBox(int w, int h, TPlayerColor playerColor=1);
 
-	static void drawIWindow(CInfoWindow * ret, std::string text, int player);
+	static void drawIWindow(CInfoWindow * ret, std::string text, TPlayerColor player);
 
 	/// split text in lines
 	static std::vector<std::string> breakText(std::string text, size_t maxLineWidth, EFonts font);

client/GUIClasses.cpp

@@ -644,7 +644,7 @@ bool CGarrisonInt::getSplittingMode()
 
 void CGarrisonInt::setArmy(const CArmedInstance *army, bool bottomGarrison)
 {
-	owned[bottomGarrison] =  army ? (army->tempOwner == LOCPLINT->playerID || army->tempOwner == 254) : false; //254 - neutral objects (pandora, banks)
+	owned[bottomGarrison] =  army ? (army->tempOwner == LOCPLINT->playerID || army->tempOwner == GameConstants::UNFLAGGABLE_PLAYER) : false;
 	armedObjs[bottomGarrison] = army;
 }
 
@@ -4063,13 +4063,13 @@ void CArtPlace::createImage()
 	if (ourArt)
 		graphic = ourArt->artType->iconIndex;
 	if (locked)
-		graphic = GameConstants::ID_LOCK;
+		graphic = ArtifactID::ART_LOCK;
 
 	image = new CAnimImage("artifact", graphic);
 	if (!ourArt)
 		image->disable();
 
-	selection = new CAnimImage("artifact", GameConstants::ID_SELECTION);
+	selection = new CAnimImage("artifact", ArtifactID::ART_SELECTION);
 	selection->disable();
 }
 
@@ -4081,7 +4081,7 @@ void CArtPlace::lockSlot(bool on)
 	locked = on;
 
 	if (on)
-		image->setFrame(GameConstants::ID_LOCK);
+		image->setFrame(ArtifactID::ART_LOCK);
 	else
 		image->setFrame(ourArt->artType->iconIndex);
 }
@@ -4388,7 +4388,7 @@ void CArtPlace::setArtifact(const CArtifactInstance *art)
 	else
 	{
 		image->enable();
-		image->setFrame(locked ? GameConstants::ID_LOCK : art->artType->iconIndex);
+		image->setFrame(locked ? ArtifactID::ART_LOCK : art->artType->iconIndex);
 
 		std::string artDesc = ourArt->artType->Description();
 		if (vstd::contains (artDesc, '{'))

lib/CBattleCallback.cpp

@@ -41,9 +41,9 @@ namespace SiegeStuffThatShouldBeMovedToHandlers //  <=== TODO
 		}
 	}
 
-	static int lineToWallHex(int line) //returns hex with wall in given line (y coordinate)
+	static BattleHex lineToWallHex(int line) //returns hex with wall in given line (y coordinate)
 	{
-		static const int lineToHex[] = {12, 29, 45, 62, 78, 95, 112, 130, 147, 165, 182};
+		static const BattleHex lineToHex[] = {12, 29, 45, 62, 78, 95, 112, 130, 147, 165, 182};
 
 		return lineToHex[line];
 	}
@@ -107,7 +107,7 @@ void CCallbackBase::setBattle(const BattleInfo *B)
 	battle = B;
 }
 
-int CCallbackBase::getPlayerID() const
+boost::optional<TPlayerColor> CCallbackBase::getPlayerID() const
 {
 	return player;
 }
@@ -136,7 +136,7 @@ std::vector<shared_ptr<const CObstacleInstance> > CBattleInfoEssentials::battleG
 	}
 	else
 	{
-		if(player >= 0 && *perspective != battleGetMySide())
+		if(!!player && *perspective != battleGetMySide())
 		{
 			tlog1 << "Unauthorized access attempt!\n";
 			assert(0); //I want to notice if that happens
@@ -219,14 +219,14 @@ const CGTownInstance * CBattleInfoEssentials::battleGetDefendedTown() const
 BattlePerspective::BattlePerspective CBattleInfoEssentials::battleGetMySide() const
 {
 	RETURN_IF_NOT_BATTLE(BattlePerspective::INVALID);
-	if(player < 0)
+	if(!player)
 		return BattlePerspective::ALL_KNOWING;
-	if(player == getBattle()->sides[0])
+	if(*player == getBattle()->sides[0])
 		return BattlePerspective::LEFT_SIDE;
-	if(player == getBattle()->sides[1])
+	if(*player == getBattle()->sides[1])
 		return BattlePerspective::RIGHT_SIDE;
 
-	tlog1 << "Cannot find player " << player << " in battle!\n";
+	tlog1 << "Cannot find player " << *player << " in battle!\n";
 	return BattlePerspective::INVALID;
 }
 
@@ -423,7 +423,7 @@ si8 CBattleInfoCallback::battleHasWallPenalty(const IBonusBearer *bonusBearer, B
 		if (shooterPosition > destHex && ((destHex % GameConstants::BFIELD_WIDTH - shooterPosition % GameConstants::BFIELD_WIDTH) < 2)) //shooting up high
 			row -= 2;
 		const int wallPos = lineToWallHex(row);
-		if (battleHexToWallPart(wallPos) != -1) //wall still exists or is indestructible
+		if (battleHexToWallPart(wallPos) < 0) //wall still exists or is indestructible
 			return true;
 	}
 
@@ -2218,31 +2218,36 @@ ReachabilityInfo::Parameters::Parameters(const CStack *Stack)
 ESpellCastProblem::ESpellCastProblem CPlayerBattleCallback::battleCanCastThisSpell(const CSpell * spell) const
 {
 	RETURN_IF_NOT_BATTLE(ESpellCastProblem::INVALID);
-	return CBattleInfoCallback::battleCanCastThisSpell(player, spell, ECastingMode::HERO_CASTING);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	return CBattleInfoCallback::battleCanCastThisSpell(*player, spell, ECastingMode::HERO_CASTING);
 }
 
 ESpellCastProblem::ESpellCastProblem CPlayerBattleCallback::battleCanCastThisSpell(const CSpell * spell, BattleHex destination) const
 {
 	RETURN_IF_NOT_BATTLE(ESpellCastProblem::INVALID);
-	return battleCanCastThisSpellHere(player, spell, ECastingMode::HERO_CASTING, destination);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	return battleCanCastThisSpellHere(*player, spell, ECastingMode::HERO_CASTING, destination);
 }
 
 ESpellCastProblem::ESpellCastProblem CPlayerBattleCallback::battleCanCreatureCastThisSpell(const CSpell * spell, BattleHex destination) const
 {
 	RETURN_IF_NOT_BATTLE(ESpellCastProblem::INVALID);
-	return battleCanCastThisSpellHere(player, spell, ECastingMode::CREATURE_ACTIVE_CASTING, destination);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	return battleCanCastThisSpellHere(*player, spell, ECastingMode::CREATURE_ACTIVE_CASTING, destination);
 }
 
 bool CPlayerBattleCallback::battleCanFlee() const
 {
 	RETURN_IF_NOT_BATTLE(false);
-	return CBattleInfoEssentials::battleCanFlee(player);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	return CBattleInfoEssentials::battleCanFlee(*player);
 }
 
 TStacks CPlayerBattleCallback::battleGetStacks(EStackOwnership whose /*= MINE_AND_ENEMY*/, bool onlyAlive /*= true*/) const
 {
 	TStacks ret;
 	RETURN_IF_NOT_BATTLE(ret);
+	ASSERT_IF_CALLED_WITH_PLAYER
 	vstd::copy_if(battleGetAllStacks(), std::back_inserter(ret), [=](const CStack *s) -> bool
 	{
 		const bool ownerMatches = (whose == MINE_AND_ENEMY)
@@ -2258,13 +2263,15 @@ TStacks CPlayerBattleCallback::battleGetStacks(EStackOwnership whose /*= MINE_AN
 int CPlayerBattleCallback::battleGetSurrenderCost() const
 {
 	RETURN_IF_NOT_BATTLE(-3)
-	return CBattleInfoCallback::battleGetSurrenderCost(player);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	return CBattleInfoCallback::battleGetSurrenderCost(*player);
 }
 
 bool CPlayerBattleCallback::battleCanCastSpell(ESpellCastProblem::ESpellCastProblem *outProblem /*= nullptr*/) const
 {
 	RETURN_IF_NOT_BATTLE(false);
-	auto problem = CBattleInfoCallback::battleCanCastSpell(player, ECastingMode::HERO_CASTING);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	auto problem = CBattleInfoCallback::battleCanCastSpell(*player, ECastingMode::HERO_CASTING);
 	if(outProblem)
 		*outProblem = problem;
 

lib/CBattleCallback.h

@@ -45,13 +45,13 @@ class DLL_LINKAGE CCallbackBase
 
 protected:
 	CGameState *gs;
-	int player; // -1 gives access to all information, otherwise callback provides only information "visible" for player
+	boost::optional<TPlayerColor> player; // not set gives access to all information, otherwise callback provides only information "visible" for player
 
-	CCallbackBase(CGameState *GS, int Player)
+	CCallbackBase(CGameState *GS, boost::optional<TPlayerColor> Player)
 		: battle(nullptr), gs(GS), player(Player)
 	{}
 	CCallbackBase()
-		: battle(nullptr), gs(nullptr), player(-1)
+		: battle(nullptr), gs(nullptr)
 	{}
 	
 	void setBattle(const BattleInfo *B);
@@ -59,7 +59,7 @@ protected:
 
 public:
 	boost::shared_mutex &getGsMutex(); //just return a reference to mutex, does not lock nor anything
-	int getPlayerID() const;
+	boost::optional<TPlayerColor> getPlayerID() const;
 
 	friend class CBattleInfoEssentials;
 };

lib/CGameState.cpp

@@ -2109,12 +2109,12 @@ bool CGameState::isVisible(int3 pos, TPlayerColor player)
 	return getPlayerTeam(player)->fogOfWarMap[pos.x][pos.y][pos.z];
 }
 
-bool CGameState::isVisible( const CGObjectInstance *obj, int player )
+bool CGameState::isVisible( const CGObjectInstance *obj, boost::optional<TPlayerColor> player )
 {
-	if(player == -1)
+	if(!player)
 		return true;
 
-	if(player == GameConstants::NEUTRAL_PLAYER) //-> TODO ??? needed?
+	if(*player == GameConstants::NEUTRAL_PLAYER) //-> TODO ??? needed?
 		return false;
 	//object is visible when at least one blocked tile is visible
 	for(int fx=0; fx<8; ++fx)
@@ -2124,7 +2124,7 @@ bool CGameState::isVisible( const CGObjectInstance *obj, int player )
 			int3 pos = obj->pos + int3(fx-7,fy-5,0);
 			if(map->isInTheMap(pos)
 				&& !((obj->defInfo->blockMap[fy] >> (7 - fx)) & 1)
-				&& isVisible(pos, player)  )
+				&& isVisible(pos, *player)  )
 				return true;
 		}
 	}

lib/CGameState.h

@@ -419,7 +419,7 @@ public:
 	void deserializationFix();
 
 	bool isVisible(int3 pos, TPlayerColor player);
-	bool isVisible(const CGObjectInstance *obj, int player);
+	bool isVisible(const CGObjectInstance *obj, boost::optional<TPlayerColor> player);
 
 	CGameState(); //c-tor
 	virtual ~CGameState(); //d-tor

lib/GameConstants.h

@@ -75,6 +75,7 @@ namespace GameConstants
 	const int HEROES_QUANTITY=156;
 	const int SPELLS_QUANTITY=70;
 	const int PRIMARY_SKILLS=4;
+	const int UNFLAGGABLE_PLAYER = 254; //254 - neutral objects (pandora, banks)
 	const int NEUTRAL_PLAYER=255;
 	const int NAMES_PER_TOWN=16;
 	const int CREATURES_PER_TOWN = 7; //without upgrades
@@ -86,7 +87,6 @@ namespace GameConstants
 	const int BATTLE_PENALTY_DISTANCE = 10; //if the distance is > than this, then shooting stack has distance penalty
 
 	const ui16 BACKPACK_START = 19;
-	const int ID_CATAPULT = 3, ID_SELECTION=144, ID_LOCK = 145;
 
 	const int TERRAIN_TYPES=10;
 	const int RESOURCE_QUANTITY=8;
@@ -596,6 +596,7 @@ public:
 		CENTAUR_AXE = 7,
 		BLACKSHARD_OF_THE_DEAD_KNIGHT = 8,
 		CORNUCOPIA = 140,
+		ART_SELECTION = 144,
 		ART_LOCK = 145,
 		AXE_OF_SMASHING = 146,
 		MITHRIL_MAIL = 147,

lib/IGameCallback.cpp

@@ -427,18 +427,18 @@ std::vector < std::string > CGameInfoCallback::getObjDescriptions(int3 pos) cons
 	return ret;
 }
 
-bool CGameInfoCallback::isVisible(int3 pos, int Player) const
+bool CGameInfoCallback::isVisible(int3 pos, boost::optional<TPlayerColor> Player) const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
-	return gs->map->isInTheMap(pos) && (Player == -1 || gs->isVisible(pos, Player));
+	return gs->map->isInTheMap(pos) && (!Player || gs->isVisible(pos, *Player));
 }
 
 bool CGameInfoCallback::isVisible(int3 pos) const
 {
-	return isVisible(pos,player);
+	return isVisible(pos, player);
 }
 
-bool CGameInfoCallback::isVisible( const CGObjectInstance *obj, int Player ) const
+bool CGameInfoCallback::isVisible( const CGObjectInstance *obj, boost::optional<TPlayerColor> Player ) const
 {
 	return gs->isVisible(obj, Player);
 }
@@ -508,11 +508,12 @@ int3 CGameInfoCallback::getMapSize() const
 
 std::vector<const CGHeroInstance *> CGameInfoCallback::getAvailableHeroes(const CGObjectInstance * townOrTavern) const
 {
+	ASSERT_IF_CALLED_WITH_PLAYER
 	std::vector<const CGHeroInstance *> ret;
 	//ERROR_RET_VAL_IF(!isOwnedOrVisited(townOrTavern), "Town or tavern must be owned or visited!", ret);
 	//TODO: town needs to be owned, advmap tavern needs to be visited; to be reimplemented when visit tracking is done
-	ret.resize(gs->players[player].availableHeroes.size());
+	ret.resize(gs->players[*player].availableHeroes.size());
-	std::copy(gs->players[player].availableHeroes.begin(),gs->players[player].availableHeroes.end(),ret.begin());
+	std::copy(gs->players[*player].availableHeroes.begin(),gs->players[*player].availableHeroes.end(),ret.begin());
 	return ret;
 }	
 
@@ -625,9 +626,9 @@ const CMapHeader * CGameInfoCallback::getMapHeader() const
 	return gs->map;
 }
 
-bool CGameInfoCallback::hasAccess(int playerId) const
+bool CGameInfoCallback::hasAccess(boost::optional<TPlayerColor> playerId) const
 {
-	return player < 0 || gs->getPlayerRelations( playerId, player ) != PlayerRelations::ENEMIES;
+	return !player || gs->getPlayerRelations( *playerId, *player ) != PlayerRelations::ENEMIES;
 }
 
 EPlayerStatus::EStatus CGameInfoCallback::getPlayerStatus(TPlayerColor player) const
@@ -653,7 +654,7 @@ bool CGameInfoCallback::canGetFullInfo(const CGObjectInstance *obj) const
 	return !obj || hasAccess(obj->tempOwner);
 }
 
-int CGameInfoCallback::getHeroCount( int player, bool includeGarrisoned ) const
+int CGameInfoCallback::getHeroCount( TPlayerColor player, bool includeGarrisoned ) const
 {
 	int ret = 0;
 	const PlayerState *p = gs->getPlayer(player);
@@ -687,7 +688,7 @@ CGameInfoCallback::CGameInfoCallback()
 {
 }
 
-CGameInfoCallback::CGameInfoCallback(CGameState *GS, int Player)
+CGameInfoCallback::CGameInfoCallback(CGameState *GS, boost::optional<TPlayerColor> Player)
 {
 	gs = GS;
 	player = Player;
@@ -696,28 +697,27 @@ CGameInfoCallback::CGameInfoCallback(CGameState *GS, int Player)
 const std::vector< std::vector< std::vector<ui8> > > & CPlayerSpecificInfoCallback::getVisibilityMap() const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
-	return gs->getPlayerTeam(player)->fogOfWarMap;
+	return gs->getPlayerTeam(*player)->fogOfWarMap;
 }
 
 int CPlayerSpecificInfoCallback::howManyTowns() const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
-	ERROR_RET_VAL_IF(player == -1, "Applicable only for player callbacks", -1);
+	ERROR_RET_VAL_IF(!player, "Applicable only for player callbacks", -1);
-	return CGameInfoCallback::howManyTowns(player);
+	return CGameInfoCallback::howManyTowns(*player);
 }
 
 std::vector < const CGTownInstance *> CPlayerSpecificInfoCallback::getTownsInfo(bool onlyOur) const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
 	std::vector < const CGTownInstance *> ret = std::vector < const CGTownInstance *>();
-	for ( auto i=gs->players.begin() ; i!=gs->players.end();i++)
+	BOOST_FOREACH(auto i, gs->players)
 	{
-		for (size_t j = 0; j < (*i).second.towns.size(); ++j)
+		BOOST_FOREACH(auto town, i.second.towns)
 		{
-			if ((*i).first==player  
+			if (i.first==player || (isVisible(town, player) && !onlyOur))
-				|| (isVisible((*i).second.towns[j],player) && !onlyOur))
 			{
-				ret.push_back((*i).second.towns[j]);
+				ret.push_back(town);
 			}
 		}
 	} //	for ( std::map<int, PlayerState>::iterator i=gs->players.begin() ; i!=gs->players.end();i++)
@@ -727,18 +727,18 @@ std::vector < const CGHeroInstance *> CPlayerSpecificInfoCallback::getHeroesInfo
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
 	std::vector < const CGHeroInstance *> ret;
-	for(size_t i=0;i<gs->map->heroes.size();i++)
+	BOOST_FOREACH(auto hero, gs->map->heroes)
 	{
-		if(	 (gs->map->heroes[i]->tempOwner==player) ||
+		if( !player || (hero->tempOwner == *player) ||
-			(isVisible(gs->map->heroes[i]->getPosition(false),player) && !onlyOur)	)
+			(isVisible(hero->getPosition(false), player) && !onlyOur)	)
 		{
-			ret.push_back(gs->map->heroes[i]);
+			ret.push_back(hero);
 		}
 	}
 	return ret;
 }
 
-int CPlayerSpecificInfoCallback::getMyColor() const
+boost::optional<TPlayerColor> CPlayerSpecificInfoCallback::getMyColor() const
 {
 	return player;
 }
@@ -749,9 +749,9 @@ int CPlayerSpecificInfoCallback::getHeroSerial(const CGHeroInstance * hero, bool
 		return -1;
 
 	size_t index = 0;
-	auto & heroes = gs->players[player].heroes;
+	auto & heroes = gs->players[*player].heroes;
 
-	for (auto curHero= heroes.begin(); curHero!=heroes.end(); curHero++)
+	for (auto curHero = heroes.begin(); curHero!=heroes.end(); curHero++)
 	{
 		if (includeGarrisoned || !(*curHero)->inTownGarrison)
 			index++;
@@ -764,13 +764,13 @@ int CPlayerSpecificInfoCallback::getHeroSerial(const CGHeroInstance * hero, bool
 
 int3 CPlayerSpecificInfoCallback::getGrailPos( double &outKnownRatio )
 {
-	if (CGObelisk::obeliskCount == 0)
+	if (!player || CGObelisk::obeliskCount == 0)
 	{
 		outKnownRatio = 0.0;
 	}
 	else
 	{
-		outKnownRatio = static_cast<double>(CGObelisk::visited[gs->getPlayerTeam(player)->id]) / CGObelisk::obeliskCount;
+		outKnownRatio = static_cast<double>(CGObelisk::visited[gs->getPlayerTeam(*player)->id]) / CGObelisk::obeliskCount;
 	}
 	return gs->map->grailPos;
 }
@@ -788,8 +788,9 @@ std::vector < const CGObjectInstance * > CPlayerSpecificInfoCallback::getMyObjec
 
 std::vector < const CGDwelling * > CPlayerSpecificInfoCallback::getMyDwellings() const
 {
+	ASSERT_IF_CALLED_WITH_PLAYER
 	std::vector < const CGDwelling * > ret;
-	BOOST_FOREACH(CGDwelling * dw, gs->getPlayer(player)->dwellings)
+	BOOST_FOREACH(CGDwelling * dw, gs->getPlayer(*player)->dwellings)
 	{
 		ret.push_back(dw);
 	}
@@ -799,7 +800,7 @@ std::vector < const CGDwelling * > CPlayerSpecificInfoCallback::getMyDwellings()
 std::vector <QuestInfo> CPlayerSpecificInfoCallback::getMyQuests() const
 {
 	std::vector <QuestInfo> ret;
-	BOOST_FOREACH (auto quest, gs->getPlayer(player)->quests)
+	BOOST_FOREACH (auto quest, gs->getPlayer(*player)->quests)
 	{
 		ret.push_back (quest);
 	}
@@ -809,13 +810,14 @@ std::vector <QuestInfo> CPlayerSpecificInfoCallback::getMyQuests() const
 int CPlayerSpecificInfoCallback::howManyHeroes(bool includeGarrisoned) const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
-	ERROR_RET_VAL_IF(player == -1, "Applicable only for player callbacks", -1);
+	ERROR_RET_VAL_IF(!player, "Applicable only for player callbacks", -1);
-	return getHeroCount(player,includeGarrisoned);
+	return getHeroCount(*player,includeGarrisoned);
 }
 
 const CGHeroInstance* CPlayerSpecificInfoCallback::getHeroBySerial(int serialId, bool includeGarrisoned) const
 {
-	const PlayerState *p = getPlayer(player);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	const PlayerState *p = getPlayer(*player);
 	ERROR_RET_VAL_IF(!p, "No player info", NULL);
 
 	if (!includeGarrisoned)
@@ -830,7 +832,8 @@ const CGHeroInstance* CPlayerSpecificInfoCallback::getHeroBySerial(int serialId,
 
 const CGTownInstance* CPlayerSpecificInfoCallback::getTownBySerial(int serialId) const
 {
-	const PlayerState *p = getPlayer(player);
+	ASSERT_IF_CALLED_WITH_PLAYER
+	const PlayerState *p = getPlayer(*player);
 	ERROR_RET_VAL_IF(!p, "No player info", NULL);
 	ERROR_RET_VAL_IF(serialId < 0 || serialId >= p->towns.size(), "No player info", NULL);
 	return p->towns[serialId];
@@ -839,15 +842,15 @@ const CGTownInstance* CPlayerSpecificInfoCallback::getTownBySerial(int serialId)
 int CPlayerSpecificInfoCallback::getResourceAmount(Res::ERes type) const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
-	ERROR_RET_VAL_IF(player == -1, "Applicable only for player callbacks", -1);
+	ERROR_RET_VAL_IF(!player, "Applicable only for player callbacks", -1);
-	return getResource(player, type);
+	return getResource(*player, type);
 }
 
 TResources CPlayerSpecificInfoCallback::getResourceAmount() const
 {
 	//boost::shared_lock<boost::shared_mutex> lock(*gs->mx);
-	ERROR_RET_VAL_IF(player == -1, "Applicable only for player callbacks", TResources());
+	ERROR_RET_VAL_IF(!player, "Applicable only for player callbacks", TResources());
-	return gs->players[player].resources;
+	return gs->players[*player].resources;
 }
 
 CGHeroInstance *CNonConstInfoCallback::getHero(int objid)
@@ -880,7 +883,7 @@ const TeamState * CGameInfoCallback::getTeam( ui8 teamID ) const
 {
 	ERROR_RET_VAL_IF(!vstd::contains(gs->teams, teamID), "Cannot find info for team " << int(teamID), NULL);
 	const TeamState *ret = &gs->teams[teamID];
-	ERROR_RET_VAL_IF(player != -1 && !vstd::contains(ret->players, player), "Illegal attempt to access team data!", NULL);
+	ERROR_RET_VAL_IF(!!player && !vstd::contains(ret->players, *player), "Illegal attempt to access team data!", NULL);
 	return ret;
 }
 

lib/IGameCallback.h

@@ -64,10 +64,10 @@ class DLL_LINKAGE CGameInfoCallback : public virtual CCallbackBase
 {
 protected:
 	CGameInfoCallback();
-	CGameInfoCallback(CGameState *GS, int Player);
+	CGameInfoCallback(CGameState *GS, boost::optional<TPlayerColor> Player);
-	bool hasAccess(int playerId) const;
+	bool hasAccess(boost::optional<TPlayerColor> playerId) const;
-	bool isVisible(int3 pos, int Player) const;
+	bool isVisible(int3 pos, boost::optional<TPlayerColor> Player) const;
-	bool isVisible(const CGObjectInstance *obj, int Player) const;
+	bool isVisible(const CGObjectInstance *obj, boost::optional<TPlayerColor> Player) const;
 	bool isVisible(const CGObjectInstance *obj) const;
 
 	bool canGetFullInfo(const CGObjectInstance *obj) const; //true we player owns obj or ally owns obj or privileged mode
@@ -97,7 +97,7 @@ public:
 	//hero
 	const CGHeroInstance* getHero(int objid) const;
 	const CGHeroInstance* getHeroWithSubid(int subid) const;
-	int getHeroCount(int player, bool includeGarrisoned) const;
+	int getHeroCount(TPlayerColor player, bool includeGarrisoned) const;
 	bool getHeroInfo(const CGObjectInstance *hero, InfoAboutHero &dest) const;
 	int getSpellCost(const CSpell * sp, const CGHeroInstance * caster) const; //when called during battle, takes into account creatures' spell cost reduction
 	int estimateSpellDamage(const CSpell * sp, const CGHeroInstance * hero) const; //estimates damage of given spell; returns 0 if spell causes no dmg
@@ -146,7 +146,7 @@ public:
 	int howManyTowns() const;
 	int howManyHeroes(bool includeGarrisoned = true) const;
 	int3 getGrailPos(double &outKnownRatio);
-	int getMyColor() const;
+	boost::optional<TPlayerColor> getMyColor() const;
 
 	std::vector <const CGTownInstance *> getTownsInfo(bool onlyOur = true) const; //true -> only owned; false -> all visible
 	int getHeroSerial(const CGHeroInstance * hero, bool includeGarrisoned=true) const;

lib/Mapping/MapFormatH3M.cpp

@@ -599,7 +599,7 @@ void CMapLoaderH3M::loadArtifactsOfHero(CGHeroInstance * hero)
 			if(!loadArtifactToSlot(hero, ArtifactPosition::MACH4))
 			{
 				// catapult by default
-				hero->putArtifact(ArtifactPosition::MACH4, createArtifact(GameConstants::ID_CATAPULT));
+				hero->putArtifact(ArtifactPosition::MACH4, createArtifact(ArtifactID::CATAPULT));
 			}
 		}
 

server/CGameHandler.cpp

@@ -2361,8 +2361,8 @@ bool CGameHandler::arrangeStacks( si32 id1, si32 id2, ui8 what, ui8 p1, ui8 p2,
 
 	if(what==1) //swap
 	{
-		if ( ((s1->tempOwner != player && s1->tempOwner != 254) && s1->getStackCount(p1)) //why 254??
+		if ( ((s1->tempOwner != player && s1->tempOwner != GameConstants::UNFLAGGABLE_PLAYER) && s1->getStackCount(p1)) //why 254??
-		  || ((s2->tempOwner != player && s2->tempOwner != 254) && s2->getStackCount(p2)))
+		  || ((s2->tempOwner != player && s2->tempOwner != GameConstants::UNFLAGGABLE_PLAYER) && s2->getStackCount(p2)))
 		{
 			complain("Can't take troops from another player!");
 			return false;
@@ -2373,7 +2373,7 @@ bool CGameHandler::arrangeStacks( si32 id1, si32 id2, ui8 what, ui8 p1, ui8 p2,
 	else if(what==2)//merge
 	{
 		if (( s1->getCreature(p1) != s2->getCreature(p2) && complain("Cannot merge different creatures stacks!"))
-		|| (((s1->tempOwner != player && s1->tempOwner != 254) && s2->getStackCount(p2)) && complain("Can't take troops from another player!")))
+		|| (((s1->tempOwner != player && s1->tempOwner != GameConstants::UNFLAGGABLE_PLAYER) && s2->getStackCount(p2)) && complain("Can't take troops from another player!")))
 			return false;
 
 		moveStack(sl1, sl2);
@@ -2425,7 +2425,7 @@ bool CGameHandler::arrangeStacks( si32 id1, si32 id2, ui8 what, ui8 p1, ui8 p2,
 	return true;
 }
 
-int CGameHandler::getPlayerAt( CConnection *c ) const
+TPlayerColor CGameHandler::getPlayerAt( CConnection *c ) const
 {
 	std::set<int> all;
 	for(std::map<int,CConnection*>::const_iterator i=connections.begin(); i!=connections.end(); i++)
@@ -2852,7 +2852,7 @@ bool CGameHandler::moveArtifact(const ArtifactLocation &al1, const ArtifactLocat
 		&& srcArtifact && !srcArtifact->canBePutAt(dst, true))
 		COMPLAIN_RET("Cannot move artifact!");
 
-	if ((srcArtifact && srcArtifact->artType->id == GameConstants::ID_LOCK) || (destArtifact && destArtifact->artType->id == GameConstants::ID_LOCK))
+	if ((srcArtifact && srcArtifact->artType->id == ArtifactID::ART_LOCK) || (destArtifact && destArtifact->artType->id == ArtifactID::ART_LOCK))
 		COMPLAIN_RET("Cannot move artifact locks.");
 
 	if (dst.slot >= GameConstants::BACKPACK_START && srcArtifact->artType->isBig())
@@ -3548,7 +3548,7 @@ bool CGameHandler::makeBattleAction( BattleAction &ba )
 			const CGHeroInstance * attackingHero = gs->curB->heroes[ba.side];
 			CHeroHandler::SBallisticsLevelInfo sbi = VLC->heroh->ballistics[attackingHero->getSecSkillLevel(SecondarySkill::BALLISTICS)];
 
-			int attackedPart = gs->curB->battleHexToWallPart(ba.destinationTile);
+			EWallParts::EWallParts attackedPart = gs->curB->battleHexToWallPart(ba.destinationTile);
 			if(attackedPart < 0)
 			{
 				complain("catapult tried to attack non-catapultable hex!");
@@ -3571,20 +3571,20 @@ bool CGameHandler::makeBattleAction( BattleAction &ba )
 				int dmgChance[] = { sbi.noDmg, sbi.oneDmg, sbi.twoDmg }; //dmgChance[i] - chance for doing i dmg when hit is successful
 				switch(attackedPart)
 				{
-				case 0: //keep
+				case EWallParts::KEEP:
 					chanceForHit = sbi.keep;
 					break;
-				case 1: //bottom tower
+				case EWallParts::BOTTOM_TOWER:
-				case 6: //upper tower
+				case EWallParts::UPPER_TOWER:
 					chanceForHit = sbi.tower;
 					break;
-				case 2: //bottom wall
+				case EWallParts::BOTTOM_WALL:
-				case 3: //below gate
+				case EWallParts::BELOW_GATE:
-				case 4: //over gate
+				case EWallParts::OVER_GATE:
-				case 5: //upper wall
+				case EWallParts::UPPER_WAL:
 					chanceForHit = sbi.wall;
 					break;
-				case 7: //gate
+				case EWallParts::GATE:
 					chanceForHit = sbi.gate;
 					break;
 				}
@@ -6188,7 +6188,7 @@ CasualtiesAfterBattle::CasualtiesAfterBattle(const CArmedInstance *army, BattleI
 	heroWithDeadCommander = -1;
 
 	int color = army->tempOwner;
-	if(color == 254)
+	if(color == GameConstants::UNFLAGGABLE_PLAYER)
 		color = GameConstants::NEUTRAL_PLAYER;
 
 	BOOST_FOREACH(CStack *st, bat->stacks)

server/CGameHandler.h

@@ -193,7 +193,7 @@ public:
 
 	void init(StartInfo *si);
 	void handleConnection(std::set<int> players, CConnection &c);
-	int getPlayerAt(CConnection *c) const;
+	TPlayerColor getPlayerAt(CConnection *c) const;
 
 	void playerMessage( TPlayerColor player, const std::string &message);
 	bool makeBattleAction(BattleAction &ba);