Fix checking PlayerColor's for validness

AI/Nullkiller/AIGateway.cpp

@@ -1081,7 +1081,7 @@ void AIGateway::recruitCreatures(const CGDwelling * d, const CArmedInstance * re
 void AIGateway::battleStart(const CCreatureSet * army1, const CCreatureSet * army2, int3 tile, const CGHeroInstance * hero1, const CGHeroInstance * hero2, bool side, bool replayAllowed)
 {
 	NET_EVENT_HANDLER;
-	assert(playerID > PlayerColor::PLAYER_LIMIT || status.getBattle() == UPCOMING_BATTLE);
+	assert(!playerID.isValidPlayer() || status.getBattle() == UPCOMING_BATTLE);
 	status.setBattle(ONGOING_BATTLE);
 	const CGObjectInstance * presumedEnemy = vstd::backOrNull(cb->getVisitableObjs(tile)); //may be nullptr in some very are cases -> eg. visited monolith and fighting with an enemy at the FoW covered exit
 	battlename = boost::str(boost::format("Starting battle of %s attacking %s at %s") % (hero1 ? hero1->getNameTranslated() : "a army") % (presumedEnemy ? presumedEnemy->getObjectName() : "unknown enemy") % tile.toString());

AI/Nullkiller/Engine/FuzzyHelper.cpp

@@ -111,7 +111,7 @@ ui64 FuzzyHelper::evaluateDanger(const CGObjectInstance * obj)
 {
 	auto cb = ai->cb.get();
 
-	if(obj->tempOwner < PlayerColor::PLAYER_LIMIT && cb->getPlayerRelations(obj->tempOwner, ai->playerID) != PlayerRelations::ENEMIES) //owned or allied objects don't pose any threat
+	if(obj->tempOwner.isValidPlayer() && cb->getPlayerRelations(obj->tempOwner, ai->playerID) != PlayerRelations::ENEMIES) //owned or allied objects don't pose any threat
 		return 0;
 
 	switch(obj->ID)

AI/VCAI/FuzzyHelper.cpp

@@ -282,7 +282,7 @@ ui64 FuzzyHelper::evaluateDanger(const CGObjectInstance * obj, const VCAI * ai)
 {
 	auto cb = ai->myCb;
 
-	if(obj->tempOwner < PlayerColor::PLAYER_LIMIT && cb->getPlayerRelations(obj->tempOwner, ai->playerID) != PlayerRelations::ENEMIES) //owned or allied objects don't pose any threat
+	if(obj->tempOwner.isValidPlayer() && cb->getPlayerRelations(obj->tempOwner, ai->playerID) != PlayerRelations::ENEMIES) //owned or allied objects don't pose any threat
 		return 0;
 
 	switch(obj->ID)

AI/VCAI/VCAI.cpp

@@ -1578,7 +1578,7 @@ void VCAI::completeGoal(Goals::TSubgoal goal)
 void VCAI::battleStart(const CCreatureSet * army1, const CCreatureSet * army2, int3 tile, const CGHeroInstance * hero1, const CGHeroInstance * hero2, bool side, bool replayAllowed)
 {
 	NET_EVENT_HANDLER;
-	assert(playerID > PlayerColor::PLAYER_LIMIT || status.getBattle() == UPCOMING_BATTLE);
+	assert(!playerID.isValidPlayer() || status.getBattle() == UPCOMING_BATTLE);
 	status.setBattle(ONGOING_BATTLE);
 	const CGObjectInstance * presumedEnemy = vstd::backOrNull(cb->getVisitableObjs(tile)); //may be nullptr in some very are cases -> eg. visited monolith and fighting with an enemy at the FoW covered exit
 	battlename = boost::str(boost::format("Starting battle of %s attacking %s at %s") % (hero1 ? hero1->getNameTranslated() : "a army") % (presumedEnemy ? presumedEnemy->getObjectName() : "unknown enemy") % tile.toString());

client/Client.cpp

@@ -570,7 +570,7 @@ void CClient::battleStarted(const BattleInfo * info)
 	for(auto & battleCb : battleCallbacks)
 	{
 		if(vstd::contains_if(info->sides, [&](const SideInBattle& side) {return side.color == battleCb.first; })
-			|| battleCb.first >= PlayerColor::PLAYER_LIMIT)
+			|| !battleCb.first.isValidPlayer())
 		{
 			battleCb.second->setBattle(info);
 		}

client/NetPacksClient.cpp

@@ -570,7 +570,7 @@ void ApplyClientNetPackVisitor::visitSetHeroesInTown(SetHeroesInTown & pack)
 	//inform all players that see this object
 	for(auto i = cl.playerint.cbegin(); i != cl.playerint.cend(); ++i)
 	{
-		if(i->first >= PlayerColor::PLAYER_LIMIT)
+		if(!i->first.isValidPlayer())
 			continue;
 
 		if(gs.isVisible(t, i->first) ||

client/adventureMap/CMinimap.cpp

@@ -46,7 +46,7 @@ ColorRGBA CMinimapInstance::getTileColor(const int3 & pos) const
 		if(player == PlayerColor::NEUTRAL)
 			return graphics->neutralColor;
 
-		if (player < PlayerColor::PLAYER_LIMIT)
+		if (player.isValidPlayer())
 			return graphics->playerColors[player.getNum()];
 	}
 

client/lobby/SelectionTab.cpp

@@ -886,7 +886,7 @@ Canvas SelectionTab::CMapInfoTooltipBox::createMinimapForLayer(std::unique_ptr<C
 						color = graphics->neutralColor;
 						break;
 					}
-					if (player < PlayerColor::PLAYER_LIMIT)
+					if (player.isValidPlayer())
 					{
 						color = graphics->playerColors[player.getNum()];
 						break;

client/render/Graphics.cpp

@@ -139,7 +139,7 @@ void Graphics::blueToPlayersAdv(SDL_Surface * sur, PlayerColor player)
 	if(sur->format->palette)
 	{
 		SDL_Color palette[32];
-		if(player < PlayerColor::PLAYER_LIMIT)
+		if(player.isValidPlayer())
 		{
 			for(int i=0; i<32; ++i)
 				palette[i] = CSDL_Ext::toSDL(playerColorPalette[player][i]);

client/renderSDL/SDLImage.cpp

@@ -247,7 +247,7 @@ void SDLImage::setBlitMode(EImageBlitMode mode)
 
 void SDLImage::setFlagColor(PlayerColor player)
 {
-	if(player < PlayerColor::PLAYER_LIMIT || player==PlayerColor::NEUTRAL)
+	if(player.isValidPlayer() || player==PlayerColor::NEUTRAL)
 		CSDL_Ext::setPlayerColor(surf, player);
 }
 

lib/IGameCallback.cpp

@@ -82,7 +82,7 @@ void CPrivilegedInfoCallback::getTilesInRange(std::unordered_set<int3> & tiles,
 											  int mode,
 											  int3::EDistanceFormula distanceFormula) const
 {
-	if(!!player && *player >= PlayerColor::PLAYER_LIMIT)
+	if(!!player && !player->isValidPlayer())
 	{
 		logGlobal->error("Illegal call to getTilesInRange!");
 		return;
@@ -114,7 +114,7 @@ void CPrivilegedInfoCallback::getTilesInRange(std::unordered_set<int3> & tiles,
 
 void CPrivilegedInfoCallback::getAllTiles(std::unordered_set<int3> & tiles, std::optional<PlayerColor> Player, int level, MapTerrainFilterMode tileFilterMode) const
 {
-	if(!!Player && *Player >= PlayerColor::PLAYER_LIMIT)
+	if(!!Player && !Player->isValidPlayer())
 	{
 		logGlobal->error("Illegal call to getAllTiles !");
 		return;

lib/NetPacksLib.cpp

@@ -770,7 +770,7 @@ void LobbyShowMessage::visitTyped(ICPackVisitor & visitor)
 
 void SetResources::applyGs(CGameState * gs) const
 {
-	assert(player < PlayerColor::PLAYER_LIMIT);
+	assert(player.isValidPlayer());
 	if(abs)
 		gs->getPlayerState(player)->resources = res;
 	else
@@ -2018,7 +2018,7 @@ void NewTurn::applyGs(CGameState *gs)
 
 	for(const auto & re : res)
 	{
-		assert(re.first < PlayerColor::PLAYER_LIMIT);
+		assert(re.first.isValidPlayer());
 		gs->getPlayerState(re.first)->resources = re.second;
 	}
 
@@ -2058,7 +2058,7 @@ void SetObjectProperty::applyGs(CGameState * gs) const
 				if(state->towns.empty())
 					*state->daysWithoutCastle = 0;
 			}
-			if(val < PlayerColor::PLAYER_LIMIT_I)
+			if(PlayerColor(val).isValidPlayer())
 			{
 				PlayerState * p = gs->getPlayerState(PlayerColor(val));
 				p->towns.emplace_back(t);

lib/StartInfo.cpp

@@ -203,7 +203,7 @@ PlayerInfo & LobbyInfo::getPlayerInfo(int color)
 
 TeamID LobbyInfo::getPlayerTeamId(const PlayerColor & color)
 {
-	if(color < PlayerColor::PLAYER_LIMIT)
+	if(color.isValidPlayer())
 		return getPlayerInfo(color.getNum()).team;
 	else
 		return TeamID::NO_TEAM;

lib/battle/BattleInfo.cpp

@@ -60,8 +60,7 @@ void BattleInfo::calculateCasualties(std::map<ui32,si32> * casualties) const
 CStack * BattleInfo::generateNewStack(uint32_t id, const CStackInstance & base, ui8 side, const SlotID & slot, BattleHex position)
 {
 	PlayerColor owner = sides[side].color;
-	assert((owner >= PlayerColor::PLAYER_LIMIT) ||
-		(base.armyObj && base.armyObj->tempOwner == owner));
+	assert(!owner.isValidPlayer() || (base.armyObj && base.armyObj->tempOwner == owner));
 
 	auto * ret = new CStack(&base, owner, id, side, slot);
 	ret->initialPosition = getAvaliableHex(base.getCreatureID(), side, position); //TODO: what if no free tile on battlefield was found?

lib/constants/EntityIdentifiers.cpp

@@ -47,12 +47,12 @@ const SlotID SlotID::SUMMONED_SLOT_PLACEHOLDER = SlotID(-3);
 const SlotID SlotID::WAR_MACHINES_SLOT = SlotID(-4);
 const SlotID SlotID::ARROW_TOWERS_SLOT = SlotID(-5);
 
-const PlayerColor PlayerColor::SPECTATOR = PlayerColor(252);
-const PlayerColor PlayerColor::CANNOT_DETERMINE = PlayerColor(253);
-const PlayerColor PlayerColor::UNFLAGGABLE = PlayerColor(254);
-const PlayerColor PlayerColor::NEUTRAL = PlayerColor(255);
+const PlayerColor PlayerColor::SPECTATOR = PlayerColor(-4);
+const PlayerColor PlayerColor::CANNOT_DETERMINE = PlayerColor(-3);
+const PlayerColor PlayerColor::UNFLAGGABLE = PlayerColor(-2);
+const PlayerColor PlayerColor::NEUTRAL = PlayerColor(-1);
 const PlayerColor PlayerColor::PLAYER_LIMIT = PlayerColor(PLAYER_LIMIT_I);
-const TeamID TeamID::NO_TEAM = TeamID(255);
+const TeamID TeamID::NO_TEAM = TeamID(-1);
 
 const SpellSchool SpellSchool::ANY = -1;
 const SpellSchool SpellSchool::AIR = 0;
@@ -194,12 +194,12 @@ std::string SpellIDBase::encode(const si32 index)
 
 bool PlayerColor::isValidPlayer() const
 {
-	return num < PLAYER_LIMIT_I;
+	return num >= 0 && num < PLAYER_LIMIT_I;
 }
 
 bool PlayerColor::isSpectator() const
 {
-	return num == 252;
+	return num == SPECTATOR.num;
 }
 
 std::string PlayerColor::getStr(bool L10n) const

lib/gameState/CGameState.cpp

@@ -184,9 +184,9 @@ std::pair<Obj,int> CGameState::pickObject (CGObjectInstance *obj)
 		{
 			PlayerColor align = (dynamic_cast<CGTownInstance *>(obj))->alignmentToPlayer;
 			si32 f; // can be negative (for random)
-			if(align >= PlayerColor::PLAYER_LIMIT) //same as owner / random
+			if(!align.isValidPlayer()) //same as owner / random
 			{
-				if(obj->tempOwner >= PlayerColor::PLAYER_LIMIT)
+				if(!obj->tempOwner.isValidPlayer())
 					f = -1; //random
 				else
 					f = scenarioOps->getIthPlayersSettings(obj->tempOwner).castle;
@@ -1699,7 +1699,7 @@ PlayerColor CGameState::checkForStandardWin() const
 	TeamID winnerTeam = TeamID::NO_TEAM;
 	for(const auto & elem : players)
 	{
-		if(elem.second.status == EPlayerStatus::INGAME && elem.first < PlayerColor::PLAYER_LIMIT)
+		if(elem.second.status == EPlayerStatus::INGAME && elem.first.isValidPlayer())
 		{
 			if(supposedWinner == PlayerColor::NEUTRAL)
 			{

lib/mapObjects/CArmedInstance.cpp

@@ -143,7 +143,7 @@ void CArmedInstance::armyChanged()
 
 CBonusSystemNode & CArmedInstance::whereShouldBeAttached(CGameState * gs)
 {
-	if(tempOwner < PlayerColor::PLAYER_LIMIT)
+	if(tempOwner.isValidPlayer())
 		if(auto * where = gs->getPlayerState(tempOwner))
 			return *where;
 

lib/mapObjects/CGTownInstance.cpp

@@ -489,7 +489,7 @@ void CGTownInstance::newTurn(CRandomGenerator & rand) const
 		//give resources if there's a Mystic Pond
 		if (hasBuilt(BuildingSubID::MYSTIC_POND)
 			&& cb->getDate(Date::DAY) != 1
-			&& (tempOwner < PlayerColor::PLAYER_LIMIT)
+			&& (tempOwner.isValidPlayer())
 			)
 		{
 			int resID = rand.nextInt(2, 5); //bonus to random rare resource

lib/mapObjects/MiscObjects.cpp

@@ -1568,7 +1568,7 @@ void CGLighthouse::onHeroVisit( const CGHeroInstance * h ) const
 		h->showInfoDialog(69);
 		giveBonusTo(h->tempOwner);
 
-		if(oldOwner < PlayerColor::PLAYER_LIMIT) //remove bonus from old owner
+		if(oldOwner.isValidPlayer()) //remove bonus from old owner
 		{
 			RemoveBonus rb(GiveBonus::ETarget::PLAYER);
 			rb.whoID = oldOwner.getNum();
@@ -1581,7 +1581,7 @@ void CGLighthouse::onHeroVisit( const CGHeroInstance * h ) const
 
 void CGLighthouse::initObj(CRandomGenerator & rand)
 {
-	if(tempOwner < PlayerColor::PLAYER_LIMIT)
+	if(tempOwner.isValidPlayer())
 	{
 		// FIXME: This is dirty hack
 		giveBonusTo(tempOwner, true);

lib/mapping/MapReaderH3M.cpp

@@ -147,14 +147,14 @@ TerrainId MapReaderH3M::readTerrain()
 RoadId MapReaderH3M::readRoad()
 {
 	RoadId result(readInt8());
-	assert(result.getNum() < features.roadsCount);
+	assert(result.getNum() <= features.roadsCount);
 	return result;
 }
 
 RiverId MapReaderH3M::readRiver()
 {
 	RiverId result(readInt8());
-	assert(result.getNum() < features.riversCount);
+	assert(result.getNum() <= features.riversCount);
 	return result;
 }
 
@@ -188,17 +188,24 @@ SpellID MapReaderH3M::readSpell32()
 
 PlayerColor MapReaderH3M::readPlayer()
 {
-	PlayerColor result(readUInt8());
-	assert(result < PlayerColor::PLAYER_LIMIT || result == PlayerColor::NEUTRAL);
-	return result;
+	uint8_t value = readUInt8();
+
+	if (value == 255)
+		return PlayerColor::NEUTRAL;
+
+	assert(value < PlayerColor::PLAYER_LIMIT_I);
+	return PlayerColor(value);
 }
 
 PlayerColor MapReaderH3M::readPlayer32()
 {
-	PlayerColor result(readInt32());
+	uint32_t value = readUInt32();
 
-	assert(result < PlayerColor::PLAYER_LIMIT || result == PlayerColor::NEUTRAL);
-	return result;
+	if (value == 255)
+		return PlayerColor::NEUTRAL;
+
+	assert(value < PlayerColor::PLAYER_LIMIT_I);
+	return PlayerColor(value);
 }
 
 void MapReaderH3M::readBitmaskBuildings(std::set<BuildingID> & dest, std::optional<FactionID> faction)

mapeditor/graphics.cpp

@@ -227,7 +227,7 @@ void Graphics::blueToPlayersAdv(QImage * sur, PlayerColor player)
 	if(sur->format() == QImage::Format_Indexed8)
 	{
 		auto palette = sur->colorTable();
-		if(player < PlayerColor::PLAYER_LIMIT)
+		if(player.isValidPlayer())
 		{
 			for(int i = 0; i < 32; ++i)
 				palette[224 + i] = playerColorPalette[player.getNum() * 32 + i];

mapeditor/maphandler.cpp

@@ -419,7 +419,7 @@ QRgb MapHandler::getTileColor(int x, int y, int z)
 		if(player == PlayerColor::NEUTRAL)
 			return graphics->neutralColor;
 		else
-			if (player < PlayerColor::PLAYER_LIMIT)
+			if (player.isValidPlayer())
 				return graphics->playerColors[player.getNum()];
 	}
 	

server/CGameHandler.cpp

@@ -804,7 +804,7 @@ void CGameHandler::onNewTurn()
 				setPortalDwelling(t, true, (n.specialWeek == NewTurn::PLAGUE ? true : false)); //set creatures for Portal of Summoning
 
 			if (!firstTurn)
-				if (t->hasBuilt(BuildingSubID::TREASURY) && player < PlayerColor::PLAYER_LIMIT)
+				if (t->hasBuilt(BuildingSubID::TREASURY) && player.isValidPlayer())
 						n.res[player][EGameResID::GOLD] += hadGold.at(player)/10; //give 10% of starting gold
 
 			if (!vstd::contains(n.cres, t->id))
@@ -845,7 +845,7 @@ void CGameHandler::onNewTurn()
 				}
 			}
 		}
-		if (!firstTurn  &&  player < PlayerColor::PLAYER_LIMIT)//not the first day and town not neutral
+		if (!firstTurn  &&  player.isValidPlayer())//not the first day and town not neutral
 		{
 			n.res[player] = n.res[player] + t->dailyIncome();
 		}
@@ -1312,13 +1312,13 @@ void CGameHandler::setOwner(const CGObjectInstance * obj, const PlayerColor owne
 	const CGTownInstance * town = dynamic_cast<const CGTownInstance *>(obj);
 	if (town) //town captured
 	{
-		if (owner < PlayerColor::PLAYER_LIMIT) //new owner is real player
+		if (owner.isValidPlayer()) //new owner is real player
 		{
 			if (town->hasBuilt(BuildingSubID::PORTAL_OF_SUMMONING))
 				setPortalDwelling(town, true, false);
 		}
 
-		if (oldOwner < PlayerColor::PLAYER_LIMIT) //old owner is real player
+		if (oldOwner.isValidPlayer()) //old owner is real player
 		{
 			if (getPlayerState(oldOwner)->towns.empty() && getPlayerState(oldOwner)->status != EPlayerStatus::LOSER) //previous player lost last last town
 			{

server/NetPacksServer.cpp

@@ -164,10 +164,10 @@ void ApplyGhNetPackVisitor::visitTradeOnMarketplace(TradeOnMarketplace & pack)
 
 	PlayerColor player = market->tempOwner;
 
-	if(player >= PlayerColor::PLAYER_LIMIT)
+	if(!player.isValidPlayer())
 		player = gh.getTile(market->visitablePos())->visitableObjects.back()->tempOwner;
 
-	if(player >= PlayerColor::PLAYER_LIMIT)
+	if(!player.isValidPlayer())
 		gh.throwAndComplain(&pack, "No player can use this market!");
 
 	bool allyTownSkillTrade = (pack.mode == EMarketMode::RESOURCE_SKILL && gh.getPlayerRelations(player, hero->tempOwner) == PlayerRelations::ALLIES);

server/processors/HeroPoolProcessor.cpp

@@ -270,7 +270,7 @@ std::vector<CGHeroInstance *> HeroPoolProcessor::findAvailableHeroesFor(const Pl
 
 const CHeroClass * HeroPoolProcessor::pickClassFor(bool isNative, const PlayerColor & player)
 {
-	if(player >= PlayerColor::PLAYER_LIMIT)
+	if(!player.isValidPlayer())
 	{
 		logGlobal->error("Cannot pick hero for player %d. Wrong owner!", player.getStr());
 		return nullptr;