Works more or less

client/lobby/RandomMapTab.cpp

@@ -65,7 +65,7 @@ RandomMapTab::RandomMapTab():
 	
 	addCallback("setPlayersCount", [&](int btnId)
 	{
-		mapGenOptions->setPlayerCount(btnId);
+		mapGenOptions->setHumanOrCpuPlayerCount(btnId);
 		setMapGenOptions(mapGenOptions);
 		updateMapInfoByHost();
 	});
@@ -184,12 +184,9 @@ void RandomMapTab::updateMapInfoByHost()
 
 	// Generate player information
 	int playersToGen = PlayerColor::PLAYER_LIMIT_I;
-	if(mapGenOptions->getPlayerCount() != CMapGenOptions::RANDOM_SIZE)
+	if(mapGenOptions->getHumanOrCpuPlayerCount() != CMapGenOptions::RANDOM_SIZE)
 	{
-		if(mapGenOptions->getCompOnlyPlayerCount() != CMapGenOptions::RANDOM_SIZE)
+		playersToGen = mapGenOptions->getHumanOrCpuPlayerCount();
-			playersToGen = mapGenOptions->getPlayerCount() + mapGenOptions->getCompOnlyPlayerCount();
-		else
-			playersToGen = mapGenOptions->getPlayerCount();
 	}
 
 	mapInfo->mapHeader->howManyTeams = playersToGen;
@@ -198,33 +195,39 @@ void RandomMapTab::updateMapInfoByHost()
 	//TODO: Where are human / CPU players toggled in player configuration?
 	//TODO: Get human player count
 
-	std::set<TeamID> occupiedTeams;
+	//std::set<TeamID> occupiedTeams;
 	for(int i = 0; i < PlayerColor::PLAYER_LIMIT_I; ++i)
 	{
 		mapInfo->mapHeader->players[i].canComputerPlay = false;
 		mapInfo->mapHeader->players[i].canHumanPlay = false;
 	}
 
-	for(int i = 0; i < playersToGen; ++i)
+	std::vector<PlayerColor> availableColors;
+	for (ui8 color = 0; color < PlayerColor::PLAYER_LIMIT_I; color++)
 	{
-		PlayerInfo player;
+		availableColors.push_back(PlayerColor(color));
-		player.isFactionRandom = true;
-		player.canComputerPlay = true;
-		if(mapGenOptions->getCompOnlyPlayerCount() != CMapGenOptions::RANDOM_SIZE && i >= mapGenOptions->getPlayerCount())
-		{
-			player.canHumanPlay = false;
-		}
-		else
-		{
-			player.canHumanPlay = true;
-		}
-		auto team = mapGenOptions->getPlayersSettings().at(PlayerColor(i)).getTeam();
-		player.team = team;
-		occupiedTeams.insert(team);
-		player.hasMainTown = true;
-		player.generateHeroAtMainTown = true;
-		mapInfo->mapHeader->players[i] = player;
 	}
+
+	//First restore known players
+	for (auto& player : mapGenOptions->getPlayersSettings())
+	{
+		PlayerInfo playerInfo;
+		playerInfo.isFactionRandom = (player.second.getStartingTown() == CMapGenOptions::CPlayerSettings::RANDOM_TOWN);
+		playerInfo.canComputerPlay = (player.second.getPlayerType() != EPlayerType::HUMAN);
+		playerInfo.canHumanPlay = (player.second.getPlayerType() != EPlayerType::COMP_ONLY);
+
+		auto team = player.second.getTeam();
+		playerInfo.team = team;
+		//occupiedTeams.insert(team);
+		playerInfo.hasMainTown = true;
+		playerInfo.generateHeroAtMainTown = true;
+		mapInfo->mapHeader->players[player.first] = playerInfo;
+		vstd::erase(availableColors, player.first);
+	}
+
+	/*
+	//Reset teams to default (?)
+	// TODO: Do not reset teams here, this is handled by CMapGenOptions
 	for(auto & player : mapInfo->mapHeader->players)
 	{
 		for(int i = 0; player.team == TeamID::NO_TEAM; ++i)
@@ -238,15 +241,17 @@ void RandomMapTab::updateMapInfoByHost()
 			}
 		}
 	}
+	*/
 
 	mapInfoChanged(mapInfo, mapGenOptions);
 }
 
+// TODO: This method only sets GUI options, doesn't alter any actual configurations done
 void RandomMapTab::setMapGenOptions(std::shared_ptr<CMapGenOptions> opts)
 {
 	mapGenOptions = opts;
 	
-	//prepare allowed options
+	//Prepare allowed options - add all, then erase the ones above the limit
 	for(int i = 0; i <= PlayerColor::PLAYER_LIMIT_I; ++i)
 	{
 		playerCountAllowed.insert(i);
@@ -260,39 +265,63 @@ void RandomMapTab::setMapGenOptions(std::shared_ptr<CMapGenOptions> opts)
 			compTeamsAllowed.insert(i);
 		}
 	}
+	int minComps = 0;
+
 	auto * tmpl = mapGenOptions->getMapTemplate();
 	if(tmpl)
 	{
+		// TODO: Debug / print actual numbers
+		// Most templates just skip this setting
+		auto compNumbers = tmpl->getCpuPlayers().getNumbers();
+		if (!compNumbers.empty())
+		{
+			compCountAllowed = compNumbers;
+			minComps = *boost::min_element(compCountAllowed);
+		}
+
 		playerCountAllowed = tmpl->getPlayers().getNumbers();
-		compCountAllowed = tmpl->getCpuPlayers().getNumbers();
+
+		auto minPlayerCount = *boost::min_element(playerCountAllowed);
+		auto maxCompCount = *boost::max_element(compCountAllowed);
+		for (int i = 1; i >= (minPlayerCount - maxCompCount) && i >= 1; i--)
+		{
+			//We can always add extra CPUs to meet the minimum total player count
+			playerCountAllowed.insert(i);
+		}	
 	}
-	if(mapGenOptions->getPlayerCount() != CMapGenOptions::RANDOM_SIZE)
+	
+	si8 playerLimit = opts->getPlayerLimit();
+	si8 humanOrCpuPlayerCount = opts->getHumanOrCpuPlayerCount();
+	si8 compOnlyPlayersCount =  opts->getCompOnlyPlayerCount();
+
+	if(mapGenOptions->getHumanOrCpuPlayerCount() != CMapGenOptions::RANDOM_SIZE)
 	{
-		vstd::erase_if(compCountAllowed,
+		vstd::erase_if(compCountAllowed, [playerLimit, humanOrCpuPlayerCount](int el)
-		[opts](int el){
+		{
-			return PlayerColor::PLAYER_LIMIT_I - opts->getPlayerCount() < el;
+			return (playerLimit - humanOrCpuPlayerCount) < el;
 		});
-		vstd::erase_if(playerTeamsAllowed,
+		vstd::erase_if(playerTeamsAllowed, [humanOrCpuPlayerCount](int el)
-		[opts](int el){
+		{
-			return opts->getPlayerCount() <= el;
+			return humanOrCpuPlayerCount <= el;
 		});
 		
 		if(!playerTeamsAllowed.count(opts->getTeamCount()))
+		{
 		   opts->setTeamCount(CMapGenOptions::RANDOM_SIZE);
+		}
 	}
 	if(mapGenOptions->getCompOnlyPlayerCount() != CMapGenOptions::RANDOM_SIZE)
 	{
-		vstd::erase_if(playerCountAllowed,
+		// This setting doesn't impact total number of players
-		[opts](int el){
+		vstd::erase_if(compTeamsAllowed, [compOnlyPlayersCount](int el)
-			return PlayerColor::PLAYER_LIMIT_I - opts->getCompOnlyPlayerCount() < el;
+		{
-		});
+			return compOnlyPlayersCount<= el;
-		vstd::erase_if(compTeamsAllowed,
-		[opts](int el){
-			return opts->getCompOnlyPlayerCount() <= el;
 		});
 		
 		if(!compTeamsAllowed.count(opts->getCompOnlyTeamCount()))
+		{
 			opts->setCompOnlyTeamCount(CMapGenOptions::RANDOM_SIZE);
+		}
 	}
 	
 	if(auto w = widget<CToggleGroup>("groupMapSize"))
@@ -321,7 +350,9 @@ void RandomMapTab::setMapGenOptions(std::shared_ptr<CMapGenOptions> opts)
 	}
 	if(auto w = widget<CToggleGroup>("groupMaxPlayers"))
 	{
-		w->setSelected(opts->getPlayerCount());
+		// FIXME: OH3 allows any setting here, even if currently selected template doesn't fit it
+		// TODO: Set max players to current template limit wherever template is explicitely selected
+		w->setSelected(opts->getHumanOrCpuPlayerCount());
 		deactivateButtonsFrom(*w, playerCountAllowed);
 	}
 	if(auto w = widget<CToggleGroup>("groupMaxTeams"))
@@ -433,10 +464,7 @@ TeamAlignmentsWidget::TeamAlignmentsWidget(RandomMapTab & randomMapTab):
 	const JsonNode config(JsonPath::builtin("config/widgets/randomMapTeamsWidget.json"));
 	variables = config["variables"];
 	
-	int humanPlayers = randomMapTab.obtainMapGenOptions().getPlayerCount();
+	int totalPlayers = randomMapTab.obtainMapGenOptions().getTotalPlayersCount();
-	int cpuPlayers = randomMapTab.obtainMapGenOptions().getCompOnlyPlayerCount();
-	int totalPlayers = humanPlayers == CMapGenOptions::RANDOM_SIZE || cpuPlayers == CMapGenOptions::RANDOM_SIZE
-	? PlayerColor::PLAYER_LIMIT_I : humanPlayers + cpuPlayers;
 	assert(totalPlayers <= PlayerColor::PLAYER_LIMIT_I);
 	auto settings = randomMapTab.obtainMapGenOptions().getPlayersSettings();
 	variables["totalPlayers"].Integer() = totalPlayers;
@@ -475,6 +503,29 @@ TeamAlignmentsWidget::TeamAlignmentsWidget(RandomMapTab & randomMapTab):
 	
 	OBJ_CONSTRUCTION;
 	
+
+	// Window should have X * X columns, where X is players + compOnly players.
+	// For random player count, X is 8
+
+	if (totalPlayers > settings.size())
+	{
+		auto savedPlayers = randomMapTab.obtainMapGenOptions().getSavedPlayersMap();
+		for (const auto & player : savedPlayers)
+		{
+			if (!vstd::contains(settings, player.first))
+			{
+				settings[player.first] = player.second;
+			}
+		}
+	}
+
+	std::vector<CMapGenOptions::CPlayerSettings> settingsVec;
+	for (const auto & player : settings)
+	{
+		settingsVec.push_back(player.second);
+	}
+
+	// FIXME: Flag is missing on windows show
 	for(int plId = 0; plId < totalPlayers; ++plId)
 	{
 		players.push_back(std::make_shared<CToggleGroup>([&, totalPlayers, plId](int sel)
@@ -501,6 +552,7 @@ TeamAlignmentsWidget::TeamAlignmentsWidget(RandomMapTab & randomMapTab):
 		}));
 		
 		OBJ_CONSTRUCTION_TARGETED(players.back().get());
+
 		for(int teamId = 0; teamId < totalPlayers; ++teamId)
 		{
 			variables["point"]["x"].Integer() = variables["cellOffset"]["x"].Integer() + plId * variables["cellMargin"]["x"].Integer();
@@ -509,9 +561,13 @@ TeamAlignmentsWidget::TeamAlignmentsWidget(RandomMapTab & randomMapTab):
 			players.back()->addToggle(teamId, std::dynamic_pointer_cast<CToggleBase>(button));
 		}
 		
-		auto team = settings.at(PlayerColor(plId)).getTeam();
+		// plId is not neccessarily player color, just an index
+		auto team = settingsVec.at(plId).getTeam();
 		if(team == TeamID::NO_TEAM)
+		{
+			logGlobal->warn("Player %d (id %d) has uninitialized team", settingsVec.at(plId).getColor(), plId);
 			players.back()->setSelected(plId);
+		}
 		else
 			players.back()->setSelected(team.getNum());
 	}

lib/rmg/CMapGenOptions.cpp

@@ -22,8 +22,9 @@ VCMI_LIB_NAMESPACE_BEGIN
 
 CMapGenOptions::CMapGenOptions()
 	: width(CMapHeader::MAP_SIZE_MIDDLE), height(CMapHeader::MAP_SIZE_MIDDLE), hasTwoLevels(true),
-	playerCount(RANDOM_SIZE), teamCount(RANDOM_SIZE), compOnlyPlayerCount(RANDOM_SIZE), compOnlyTeamCount(RANDOM_SIZE),
+	humanOrCpuPlayerCount(RANDOM_SIZE), teamCount(RANDOM_SIZE), compOnlyPlayerCount(RANDOM_SIZE), compOnlyTeamCount(RANDOM_SIZE),
-	waterContent(EWaterContent::RANDOM), monsterStrength(EMonsterStrength::RANDOM), mapTemplate(nullptr)
+	waterContent(EWaterContent::RANDOM), monsterStrength(EMonsterStrength::RANDOM), mapTemplate(nullptr),
+	customizedPlayers(false)
 {
 	initPlayersMap();
 	setRoadEnabled(RoadId(Road::DIRT_ROAD), true);
@@ -63,23 +64,47 @@ void CMapGenOptions::setHasTwoLevels(bool value)
 	hasTwoLevels = value;
 }
 
-si8 CMapGenOptions::getPlayerCount() const
+si8 CMapGenOptions::getHumanOrCpuPlayerCount() const
 {
-	return playerCount;
+	return humanOrCpuPlayerCount;
 }
 
-void CMapGenOptions::setPlayerCount(si8 value)
+void CMapGenOptions::setHumanOrCpuPlayerCount(si8 value)
 {
-	assert((value >= 1 && value <= PlayerColor::PLAYER_LIMIT_I) || value == RANDOM_SIZE);
+	// Set total player count (human + AI)?
-	playerCount = value;
 
-	auto possibleCompPlayersCount = PlayerColor::PLAYER_LIMIT_I - value;
+	assert((value >= 1 && value <= PlayerColor::PLAYER_LIMIT_I) || value == RANDOM_SIZE);
+	humanOrCpuPlayerCount = value;
+
+	// Use template player limit, if any
+	auto playerLimit = getPlayerLimit();
+	auto possibleCompPlayersCount = playerLimit - std::max<si8>(0, humanOrCpuPlayerCount);
 	if (compOnlyPlayerCount > possibleCompPlayersCount)
+	{
 		setCompOnlyPlayerCount(possibleCompPlayersCount);
+	}
 
 	resetPlayersMap();
 }
 
+si8 CMapGenOptions::getTotalPlayersCount() const
+{
+	auto totalPlayers = 0;
+	si8 humans = getHumanOrCpuPlayerCount();
+	si8 cpus = getCompOnlyPlayerCount();
+	if (humans == RANDOM_SIZE || cpus == RANDOM_SIZE)
+	{
+		totalPlayers = PlayerColor::PLAYER_LIMIT_I;
+	}
+	else
+	{
+		totalPlayers = humans + cpus;
+	}
+	assert (totalPlayers <= PlayerColor::PLAYER_LIMIT_I);
+	assert (totalPlayers >= 2);
+	return totalPlayers;
+}
+
 si8 CMapGenOptions::getTeamCount() const
 {
 	return teamCount;
@@ -87,7 +112,7 @@ si8 CMapGenOptions::getTeamCount() const
 
 void CMapGenOptions::setTeamCount(si8 value)
 {
-	assert(getPlayerCount() == RANDOM_SIZE || (value >= 0 && value < getPlayerCount()) || value == RANDOM_SIZE);
+	assert(getHumanOrCpuPlayerCount() == RANDOM_SIZE || (value >= 0 && value < getHumanOrCpuPlayerCount()) || value == RANDOM_SIZE);
 	teamCount = value;
 }
 
@@ -96,9 +121,21 @@ si8 CMapGenOptions::getCompOnlyPlayerCount() const
 	return compOnlyPlayerCount;
 }
 
+si8 CMapGenOptions::getPlayerLimit() const
+{
+	si8 playerLimit = PlayerColor::PLAYER_LIMIT_I;
+	if (auto temp = getMapTemplate())
+	{
+		playerLimit = *boost::max_element(temp->getPlayers().getNumbers());
+	}
+	return playerLimit;
+}
+
 void CMapGenOptions::setCompOnlyPlayerCount(si8 value)
 {
-	assert(value == RANDOM_SIZE || (getPlayerCount() == RANDOM_SIZE || (value >= 0 && value <= PlayerColor::PLAYER_LIMIT_I - getPlayerCount())));
+	auto playerLimit = getPlayerLimit();
+
+	assert(value == RANDOM_SIZE || (getHumanOrCpuPlayerCount() == RANDOM_SIZE || (value >= 0 && value <= playerLimit - getHumanOrCpuPlayerCount())));
 	compOnlyPlayerCount = value;
 
 	resetPlayersMap();
@@ -137,75 +174,69 @@ void CMapGenOptions::setMonsterStrength(EMonsterStrength::EMonsterStrength value
 
 void CMapGenOptions::initPlayersMap()
 {
-	std::map<PlayerColor, FactionID> rememberTownTypes;
-	std::map<PlayerColor, TeamID> rememberTeam;
-
-	for(const auto & p : players)
-	{
-		auto town = p.second.getStartingTown();
-		if (town != CPlayerSettings::RANDOM_TOWN)
-			rememberTownTypes[p.first] = FactionID(town);
-		rememberTeam[p.first] = p.second.getTeam();
-	}
-
-
 	players.clear();
-	int realPlayersCnt = playerCount;
+	int realPlayersCnt = getHumanOrCpuPlayerCount();
-	int realCompOnlyPlayersCnt = (compOnlyPlayerCount == RANDOM_SIZE) ? (PlayerColor::PLAYER_LIMIT_I - realPlayersCnt) : compOnlyPlayerCount;
-	int totalPlayersLimit = realPlayersCnt + realCompOnlyPlayersCnt;
-	if (getPlayerCount() == RANDOM_SIZE || compOnlyPlayerCount == RANDOM_SIZE)
-		totalPlayersLimit = static_cast<int>(PlayerColor::PLAYER_LIMIT_I);
 
-	for(int color = 0; color < totalPlayersLimit; ++color)
+	// TODO: Initialize settings for all color even if not present?
+	for(int color = 0; color < getPlayerLimit(); ++color)
 	{
 		CPlayerSettings player;
 		auto pc = PlayerColor(color);
 		player.setColor(pc);
+
+		/*
+		if (vstd::contains(savedPlayerSettings, pc))
+		{
+			player.setTeam(savedPlayerSettings[pc].getTeam());
+			player.setStartingTown(savedPlayerSettings[pc].getStartingTown());
+			//TODO: Restore starting hero and bonus?
+		}
+		// Assign new owner of this player
+		*/
+
 		auto playerType = EPlayerType::AI;
-		if (getPlayerCount() != RANDOM_SIZE && color < realPlayersCnt)
+		// Color doesn't have to be continuous. Player colors can later be changed manually
+		if (getHumanOrCpuPlayerCount() != RANDOM_SIZE && color < realPlayersCnt)
 		{
 			playerType = EPlayerType::HUMAN;
 		}
-		else if((getPlayerCount() != RANDOM_SIZE && color >= realPlayersCnt)
+		else if((getHumanOrCpuPlayerCount() != RANDOM_SIZE && color >= realPlayersCnt)
-		   || (compOnlyPlayerCount != RANDOM_SIZE && color >= (PlayerColor::PLAYER_LIMIT_I-compOnlyPlayerCount)))
+		   || (compOnlyPlayerCount != RANDOM_SIZE && color >= (PlayerColor::PLAYER_LIMIT_I - compOnlyPlayerCount)))
 		{
 			playerType = EPlayerType::COMP_ONLY;
 		}
 		player.setPlayerType(playerType);
-		player.setTeam(rememberTeam[pc]);
-		players[pc] = player;
 
-		if (vstd::contains(rememberTownTypes, pc))
+		players[pc] = player;
-			players[pc].setStartingTown(rememberTownTypes[pc]);
 	}
+	savePlayersMap();
 }
 
 void CMapGenOptions::resetPlayersMap()
 {
+	// Called when number of player changes
+	// TODO: Should it?
+
 	//But do not update info about already made selections
-	std::map<PlayerColor, FactionID> rememberTownTypes;
-	std::map<PlayerColor, TeamID> rememberTeam;
 
-	for(const auto & p : players)
+	savePlayersMap();
-	{
-		auto town = p.second.getStartingTown();
-		if (town != CPlayerSettings::RANDOM_TOWN)
-			rememberTownTypes[p.first] = FactionID(town);
-		rememberTeam[p.first] = p.second.getTeam();
-	}
 
+	/*
 	//Remove players who have undefined properties
 	vstd::erase_if(players, [](const std::pair<PlayerColor, CPlayerSettings> & p)
 	{
 		return p.second.getPlayerType() != EPlayerType::AI && p.second.getStartingTown() == CPlayerSettings::RANDOM_TOWN;
 	});
+	*/
 
-	int realPlayersCnt = getPlayerCount();
+	// FIXME: This should be total players count
+	int realPlayersCnt = getHumanOrCpuPlayerCount();
 	if (realPlayersCnt != RANDOM_SIZE)
 	{
 		//Trim the number of AI players, then CPU-only players, finally human players
 		auto eraseLastPlayer = [this](EPlayerType playerType) -> bool
 		{
+			//FIXME: Infinite loop for 0 players
 			for (auto it = players.rbegin(); it != players.rend(); ++it)
 			{
 				if (it->second.getPlayerType() == playerType)
@@ -214,33 +245,28 @@ void CMapGenOptions::resetPlayersMap()
 					return true;
 				}
 			}
-			return false;
+			return false; //Can't earse any player of this type
 		};
 
-		while (players.size() < realPlayersCnt)
+		while (players.size() > realPlayersCnt)
 		{
-			if (eraseLastPlayer(EPlayerType::AI))
+			while (eraseLastPlayer(EPlayerType::AI));
-				continue;
+			while (eraseLastPlayer(EPlayerType::COMP_ONLY));
-			if (eraseLastPlayer(EPlayerType::COMP_ONLY))
+			while (eraseLastPlayer(EPlayerType::HUMAN));
-				continue;
-			if (eraseLastPlayer(EPlayerType::HUMAN))
-				continue;
 		}
 	}
+	else
+	{
+		//If count is random, generate info for all the players
+		realPlayersCnt = PlayerColor::PLAYER_LIMIT_I;
+	}
 
-	int realCompOnlyPlayersCnt = (compOnlyPlayerCount == RANDOM_SIZE) ? (PlayerColor::PLAYER_LIMIT_I - realPlayersCnt) : compOnlyPlayerCount;
+	int realCompOnlyPlayersCnt = getCompOnlyPlayerCount();
-	int totalPlayersLimit = realPlayersCnt + realCompOnlyPlayersCnt;
+	//int totalPlayersLimit = getPlayerLimit();
-	if (getPlayerCount() == RANDOM_SIZE || compOnlyPlayerCount == RANDOM_SIZE)
-		totalPlayersLimit = static_cast<int>(PlayerColor::PLAYER_LIMIT_I);
 
 	//First colors from the list are assigned to human players, then to CPU players
-	//FIXME: Assign human players colors first
-
-	//TODO: Where is player type is set in void CVCMIServer::updateAndPropagateLobbyState()
-	//in ApplyOnServerAfterAnnounceNetPackVisitor::visitForLobby
-	//CPackForLobby
 	std::vector<PlayerColor> availableColors;
-	for (ui8 color = 0; color < PlayerColor::PLAYER_LIMIT_I; ++color)
+	for (ui8 color = 0; color < PlayerColor::PLAYER_LIMIT_I; color++)
 	{
 		availableColors.push_back(PlayerColor(color));
 	}
@@ -251,7 +277,7 @@ void CMapGenOptions::resetPlayersMap()
 		{
 			if (player.second.getPlayerType() == playerType)
 			{
-				vstd::erase(availableColors, player.second.getColor());
+				vstd::erase(availableColors, player.second.getColor()); //FIXME: Where is this color initialized at lobby launch?
 			}
 		}
 	};
@@ -259,8 +285,59 @@ void CMapGenOptions::resetPlayersMap()
 	removeUsedColors(EPlayerType::COMP_ONLY);
 	//removeUsedColors(EPlayerType::AI);
 
-	//TODO: Assign the remaining colors to random players (AI players)
+	//Assign unused colors to remaining AI players
+	while (players.size() < realPlayersCnt && !availableColors.empty())
+	{
+		auto color = availableColors.front();
+		setPlayerTypeForStandardPlayer(color, EPlayerType::AI);
+		players[color].setColor(color);
+		availableColors.erase(availableColors.begin());
 
+		if (vstd::contains(savedPlayerSettings, color))
+		{
+			setPlayerTeam(color, savedPlayerSettings.at(color).getTeam());
+			// TODO: setter
+			players[color].setStartingTown(savedPlayerSettings.at(color).getStartingTown());
+		}
+		else
+		{
+			logGlobal->warn("Adding settings for player %s", color.encode(color));
+			// Usually, all players should be initialized in initPlayersMap()
+			CPlayerSettings settings;
+			players[color] = settings;
+		}
+	}
+	// TODO: Assign players to teams at the beginning (if all players belong to the same team)
+
+	std::set<TeamID> occupiedTeams;
+	for(auto & player : players)
+	{
+		auto team = player.second.getTeam();
+		if (team != TeamID::NO_TEAM)
+		{
+			occupiedTeams.insert(team);
+		}
+	}
+	// TODO: Handle situation when we remove a player and remaining players belong to only one team
+
+	for(auto & player : players)
+	{
+		if (player.second.getTeam() == TeamID::NO_TEAM)
+		{
+			//Find first unused team
+			for(int i = 0; i < PlayerColor::PLAYER_LIMIT_I; ++i)
+			{
+				TeamID team(i);
+				if(!occupiedTeams.count(team))
+				{
+					player.second.setTeam(team);
+					occupiedTeams.insert(team);
+					break;
+				}
+			}
+		}
+	}
+	/*
 	for(int color = 0; color < totalPlayersLimit; ++color)
 	{
 		CPlayerSettings player;
@@ -284,6 +361,21 @@ void CMapGenOptions::resetPlayersMap()
 		if (vstd::contains(rememberTownTypes, pc))
 			players[pc].setStartingTown(rememberTownTypes[pc]);
 	}
+	*/
+}
+
+void CMapGenOptions::savePlayersMap()
+{
+	//Only save already configured players
+	for (const auto& player : players)
+	{
+		savedPlayerSettings[player.first] = player.second;
+	}
+}
+
+const std::map<PlayerColor, CMapGenOptions::CPlayerSettings> & CMapGenOptions::getSavedPlayersMap() const
+{
+	return savedPlayerSettings;
 }
 
 const std::map<PlayerColor, CMapGenOptions::CPlayerSettings> & CMapGenOptions::getPlayersSettings() const
@@ -294,16 +386,18 @@ const std::map<PlayerColor, CMapGenOptions::CPlayerSettings> & CMapGenOptions::g
 void CMapGenOptions::setStartingTownForPlayer(const PlayerColor & color, si32 town)
 {
 	auto it = players.find(color);
-	if(it == players.end()) assert(0);
+	assert(it != players.end());
 	it->second.setStartingTown(town);
 }
 
 void CMapGenOptions::setPlayerTypeForStandardPlayer(const PlayerColor & color, EPlayerType playerType)
 {
+	// FIXME: Why actually not set it to COMP_ONLY? Ie. when swapping human to another color?
 	assert(playerType != EPlayerType::COMP_ONLY);
 	auto it = players.find(color);
-	if(it == players.end()) assert(0);
+	assert(it != players.end());
 	it->second.setPlayerType(playerType);
+	customizedPlayers = true;
 }
 
 const CRmgTemplate * CMapGenOptions::getMapTemplate() const
@@ -325,8 +419,8 @@ void CMapGenOptions::setMapTemplate(const CRmgTemplate * value)
 			setHasTwoLevels(sizes.first.z - 1);
 		}
 		
-		if(!mapTemplate->getPlayers().isInRange(getPlayerCount()))
+		if(!mapTemplate->getPlayers().isInRange(getHumanOrCpuPlayerCount()))
-			setPlayerCount(RANDOM_SIZE);
+			setHumanOrCpuPlayerCount(RANDOM_SIZE);
 		if(!mapTemplate->getCpuPlayers().isInRange(getCompOnlyPlayerCount()))
 			setCompOnlyPlayerCount(RANDOM_SIZE);
 		if(!mapTemplate->getWaterContentAllowed().count(getWaterContent()))
@@ -365,15 +459,17 @@ bool CMapGenOptions::isRoadEnabled() const
 void CMapGenOptions::setPlayerTeam(const PlayerColor & color, const TeamID & team)
 {
 	auto it = players.find(color);
-	if(it == players.end()) assert(0);
+	assert(it != players.end());
+	// TODO: Make pivate friend method to avoid unintended changes?
 	it->second.setTeam(team);
+	customizedPlayers = true;
 }
 
 void CMapGenOptions::finalize(CRandomGenerator & rand)
 {
 	logGlobal->info("RMG map: %dx%d, %s underground", getWidth(), getHeight(), getHasTwoLevels() ? "WITH" : "NO");
 	logGlobal->info("RMG settings: players %d, teams %d, computer players %d, computer teams %d, water %d, monsters %d",
-		static_cast<int>(getPlayerCount()), static_cast<int>(getTeamCount()), static_cast<int>(getCompOnlyPlayerCount()),
+		static_cast<int>(getHumanOrCpuPlayerCount()), static_cast<int>(getTeamCount()), static_cast<int>(getCompOnlyPlayerCount()),
 		static_cast<int>(getCompOnlyTeamCount()), static_cast<int>(getWaterContent()), static_cast<int>(getMonsterStrength()));
 
 	if(!mapTemplate)
@@ -384,18 +480,18 @@ void CMapGenOptions::finalize(CRandomGenerator & rand)
 	
 	logGlobal->info("RMG template name: %s", mapTemplate->getName());
 
-	if (getPlayerCount() == RANDOM_SIZE)
+	if (getHumanOrCpuPlayerCount() == RANDOM_SIZE)
 	{
 		auto possiblePlayers = mapTemplate->getPlayers().getNumbers();
 		//ignore all non-randomized players, make sure these players will not be missing after roll
 		possiblePlayers.erase(possiblePlayers.begin(), possiblePlayers.lower_bound(countHumanPlayers() + countCompOnlyPlayers()));
 		assert(!possiblePlayers.empty());
-		setPlayerCount (*RandomGeneratorUtil::nextItem(possiblePlayers, rand));
+		setHumanOrCpuPlayerCount (*RandomGeneratorUtil::nextItem(possiblePlayers, rand));
 		updatePlayers();
 	}
 	if(teamCount == RANDOM_SIZE)
 	{
-		teamCount = rand.nextInt(getPlayerCount() - 1);
+		teamCount = rand.nextInt(getHumanOrCpuPlayerCount() - 1);
 		if (teamCount == 1)
 			teamCount = 0;
 	}
@@ -432,11 +528,6 @@ void CMapGenOptions::finalize(CRandomGenerator & rand)
 	assert (vstd::iswithin(waterContent, EWaterContent::NONE, EWaterContent::ISLANDS));
 	assert (vstd::iswithin(monsterStrength, EMonsterStrength::GLOBAL_WEAK, EMonsterStrength::GLOBAL_STRONG));
 
-
-	//rectangular maps are the future of gaming
-	//setHeight(20);
-	//setWidth(50);
-
 	logGlobal->trace("Player config:");
 	int cpuOnlyPlayers = 0;
 	for(const auto & player : players)
@@ -457,21 +548,25 @@ void CMapGenOptions::finalize(CRandomGenerator & rand)
 			default:
 				assert(false);
 		}
+		// FIXME: Every player is player 0 with type of AI
+		// FIXME: player.first != player.second.getColor()
+		// TODO: Set player color everywhere players is set, or only once here
 		logGlobal->trace("Player %d: %s", player.second.getColor(), playerType);
 	}
-	setCompOnlyPlayerCount(cpuOnlyPlayers); //human players are set automaticlaly (?)
+	// FXIME: Do not set this again after options were set
+	setCompOnlyPlayerCount(cpuOnlyPlayers); //human players are set automatically (?)
 	logGlobal->info("Final player config: %d total, %d cpu-only", players.size(), static_cast<int>(getCompOnlyPlayerCount()));
 }
 
 void CMapGenOptions::updatePlayers()
 {
-	// Remove AI players only from the end of the players map if necessary
+	// Remove non-human players only from the end of the players map if necessary
 	for(auto itrev = players.end(); itrev != players.begin();)
 	{
 		auto it = itrev;
 		--it;
-		if (players.size() == getPlayerCount()) break;
+		if (players.size() == getHumanOrCpuPlayerCount()) break;
-		if(it->second.getPlayerType() == EPlayerType::AI)
+		if(it->second.getPlayerType() != EPlayerType::HUMAN)
 		{
 			players.erase(it);
 		}
@@ -489,7 +584,7 @@ void CMapGenOptions::updateCompOnlyPlayers()
 	{
 		auto it = itrev;
 		--it;
-		if (players.size() <= getPlayerCount()) break;
+		if (players.size() <= getHumanOrCpuPlayerCount()) break;
 		if(it->second.getPlayerType() == EPlayerType::COMP_ONLY)
 		{
 			players.erase(it);
@@ -501,11 +596,11 @@ void CMapGenOptions::updateCompOnlyPlayers()
 	}
 
 	// Add some comp only players if necessary
-	int compOnlyPlayersToAdd = static_cast<int>(getPlayerCount() - players.size());
+	int compOnlyPlayersToAdd = static_cast<int>(getHumanOrCpuPlayerCount() - players.size());
 
 	if (compOnlyPlayersToAdd < 0)
 	{
-		logGlobal->error("Incorrect number of players to add. Requested players %d, current players %d", playerCount, players.size());
+		logGlobal->error("Incorrect number of players to add. Requested players %d, current players %d", humanOrCpuPlayerCount, players.size());
 		assert (compOnlyPlayersToAdd < 0);
 	}
 	for(int i = 0; i < compOnlyPlayersToAdd; ++i)
@@ -561,6 +656,11 @@ bool CMapGenOptions::checkOptions() const
 	}
 }
 
+bool CMapGenOptions::arePlayersCustomized() const
+{
+	return customizedPlayers;
+}
+
 std::vector<const CRmgTemplate *> CMapGenOptions::getPossibleTemplates() const
 {
 	int3 tplSize(width, height, (hasTwoLevels ? 2 : 1));
@@ -576,9 +676,9 @@ std::vector<const CRmgTemplate *> CMapGenOptions::getPossibleTemplates() const
 		if(!tmpl->isWaterContentAllowed(getWaterContent()))
 			return true;
 
-		if(getPlayerCount() != -1)
+		if(getHumanOrCpuPlayerCount() != CMapGenOptions::RANDOM_SIZE)
 		{
-			if (!tmpl->getPlayers().isInRange(getPlayerCount()))
+			if (!tmpl->getPlayers().isInRange(getHumanOrCpuPlayerCount()))
 				return true;
 		}
 		else
@@ -588,7 +688,7 @@ std::vector<const CRmgTemplate *> CMapGenOptions::getPossibleTemplates() const
 				return true;
 		}
 
-		if(compOnlyPlayerCount != -1)
+		if(compOnlyPlayerCount != CMapGenOptions::RANDOM_SIZE)
 		{
 			if (!tmpl->getCpuPlayers().isInRange(compOnlyPlayerCount))
 				return true;

lib/rmg/CMapGenOptions.h

@@ -88,8 +88,11 @@ public:
 
 	/// The count of all (human or computer) players ranging from 1 to PlayerColor::PLAYER_LIMIT or RANDOM_SIZE for random. If you call
 	/// this method, all player settings are reset to default settings.
-	si8 getPlayerCount() const;
+	si8 getHumanOrCpuPlayerCount() const;
-	void setPlayerCount(si8 value);
+	void setHumanOrCpuPlayerCount(si8 value);
+
+	si8 getTotalPlayersCount() const;
+	si8 getPlayerLimit() const;
 
 	/// The count of the teams ranging from 0 to <players count - 1> or RANDOM_SIZE for random.
 	si8 getTeamCount() const;
@@ -117,6 +120,7 @@ public:
 	/// The first player colors belong to standard players and the last player colors belong to comp only players.
 	/// All standard players are by default of type EPlayerType::AI.
 	const std::map<PlayerColor, CPlayerSettings> & getPlayersSettings() const;
+	const std::map<PlayerColor, CPlayerSettings> & getSavedPlayersMap() const;
 	void setStartingTownForPlayer(const PlayerColor & color, si32 town);
 	/// Sets a player type for a standard player. A standard player is the opposite of a computer only player. The
 	/// values which can be chosen for the player type are EPlayerType::AI or EPlayerType::HUMAN.
@@ -139,12 +143,15 @@ public:
 
 	/// Returns false if there is no template available which fits to the currently selected options.
 	bool checkOptions() const;
+	/// Returns true if player colors or teams were set in game GUI
+	bool arePlayersCustomized() const;
 
 	static const si8 RANDOM_SIZE = -1;
 
 private:
 	void initPlayersMap();
 	void resetPlayersMap();
+	void savePlayersMap();
 	int countHumanPlayers() const;
 	int countCompOnlyPlayers() const;
 	PlayerColor getNextPlayerColor() const;
@@ -154,11 +161,13 @@ private:
 
 	si32 width, height;
 	bool hasTwoLevels;
-	si8 playerCount, teamCount, compOnlyPlayerCount, compOnlyTeamCount;
+	si8 humanOrCpuPlayerCount, teamCount, compOnlyPlayerCount, compOnlyTeamCount;
 	EWaterContent::EWaterContent waterContent;
 	EMonsterStrength::EMonsterStrength monsterStrength;
 	std::map<PlayerColor, CPlayerSettings> players;
+	std::map<PlayerColor, CPlayerSettings> savedPlayerSettings;
 	std::set<RoadId> enabledRoads;
+	bool customizedPlayers;
 	
 	const CRmgTemplate * mapTemplate;
 
@@ -169,7 +178,7 @@ public:
 		h & width;
 		h & height;
 		h & hasTwoLevels;
-		h & playerCount;
+		h & humanOrCpuPlayerCount;
 		h & teamCount;
 		h & compOnlyPlayerCount;
 		h & compOnlyTeamCount;

lib/rmg/CMapGenerator.cpp

@@ -104,7 +104,7 @@ void CMapGenerator::initPrisonsRemaining()
 		if (isAllowed)
 			allowedPrisons++;
 	}
-	allowedPrisons = std::max<int> (0, allowedPrisons - 16 * mapGenOptions.getPlayerCount()); //so at least 16 heroes will be available for every player
+	allowedPrisons = std::max<int> (0, allowedPrisons - 16 * mapGenOptions.getHumanOrCpuPlayerCount()); //so at least 16 heroes will be available for every player
 }
 
 void CMapGenerator::initQuestArtsRemaining()
@@ -162,7 +162,7 @@ std::string CMapGenerator::getMapDescription() const
     std::stringstream ss;
     ss << boost::str(boost::format(std::string("Map created by the Random Map Generator.\nTemplate was %s, size %dx%d") +
         ", levels %d, players %d, computers %d, water %s, monster %s, VCMI map") % mapTemplate->getName() %
-		map->width() % map->height() % static_cast<int>(map->levels()) % static_cast<int>(mapGenOptions.getPlayerCount()) %
+		map->width() % map->height() % static_cast<int>(map->levels()) % static_cast<int>(mapGenOptions.getHumanOrCpuPlayerCount()) %
 		static_cast<int>(mapGenOptions.getCompOnlyPlayerCount()) % waterContentStr[mapGenOptions.getWaterContent()] %
 		monsterStrengthStr[monsterStrengthIndex]);
 
@@ -185,84 +185,120 @@ std::string CMapGenerator::getMapDescription() const
 
 void CMapGenerator::addPlayerInfo()
 {
-	// Calculate which team numbers exist
+	// Teams are already configured in CMapGenOptions. However, it's not the case when it comes to map editor
 
-	enum ETeams {CPHUMAN = 0, CPUONLY = 1, AFTER_LAST = 2}; // Used as a kind of a local named array index, so left as enum, not enum class
-	std::array<std::list<int>, 2> teamNumbers;
 	std::set<int> teamsTotal;
 
-	int teamOffset = 0;
+	if (mapGenOptions.arePlayersCustomized())
-	int playerCount = 0;
-	int teamCount = 0;
-
-	for (int i = CPHUMAN; i < AFTER_LAST; ++i)
 	{
-		if (i == CPHUMAN)
+		// Simply copy existing settings set in GUI
-		{
-			playerCount = mapGenOptions.getPlayerCount();
-			teamCount = mapGenOptions.getTeamCount();
-		}
-		else
-		{
-			playerCount = mapGenOptions.getCompOnlyPlayerCount();
-			teamCount = mapGenOptions.getCompOnlyTeamCount();
-		}
 
-		if(playerCount == 0)
+		for (const auto & player : mapGenOptions.getPlayersSettings())
 		{
-			continue;
+			PlayerInfo playerInfo;
+			playerInfo.team = player.second.getTeam();
+			if (player.second.getPlayerType() == EPlayerType::COMP_ONLY)
+			{
+				playerInfo.canHumanPlay = false;
+			}
+			else
+			{
+				playerInfo.canHumanPlay = true;
+			}
+			map->getMap(this).players[player.first.getNum()] = playerInfo;
+			teamsTotal.insert(player.second.getTeam());
 		}
-		int playersPerTeam = playerCount / (teamCount == 0 ? playerCount : teamCount);
+	}
-		int teamCountNorm = teamCount;
+	else
-		if(teamCountNorm == 0)
+	{
+		// Assign standard teams (in map editor)
+
+		// Calculate which team numbers exist
+
+		enum ETeams {CPHUMAN = 0, CPUONLY = 1, AFTER_LAST = 2}; // Used as a kind of a local named array index, so left as enum, not enum class
+		std::array<std::list<int>, 2> teamNumbers;
+		
+		int teamOffset = 0;
+		int playerCount = 0;
+		int teamCount = 0;
+
+		// FIXME: Player can be any color, not just 0
+		for (int i = CPHUMAN; i < AFTER_LAST; ++i)
 		{
-			teamCountNorm = playerCount;
+			if (i == CPHUMAN)
-		}
+			{
-		for(int j = 0; j < teamCountNorm; ++j)
+				playerCount = mapGenOptions.getHumanOrCpuPlayerCount();
-		{
+				teamCount = mapGenOptions.getTeamCount();
-			for(int k = 0; k < playersPerTeam; ++k)
+			}
+			else
+			{
+				playerCount = mapGenOptions.getCompOnlyPlayerCount();
+				teamCount = mapGenOptions.getCompOnlyTeamCount();
+			}
+
+			if(playerCount == 0)
+			{
+				continue;
+			}
+			int playersPerTeam = playerCount / (teamCount == 0 ? playerCount : teamCount);
+			int teamCountNorm = teamCount;
+			if(teamCountNorm == 0)
+			{
+				teamCountNorm = playerCount;
+			}
+			for(int j = 0; j < teamCountNorm; ++j)
+			{
+				for(int k = 0; k < playersPerTeam; ++k)
+				{
+					teamNumbers[i].push_back(j + teamOffset);
+				}
+			}
+			for(int j = 0; j < playerCount - teamCountNorm * playersPerTeam; ++j)
 			{
 				teamNumbers[i].push_back(j + teamOffset);
 			}
+			teamOffset += teamCountNorm;
 		}
-		for(int j = 0; j < playerCount - teamCountNorm * playersPerTeam; ++j)
+		logGlobal->info("Current player settings size: %d",  mapGenOptions.getPlayersSettings().size());
-		{
-			teamNumbers[i].push_back(j + teamOffset);
-		}
-		teamOffset += teamCountNorm;
-	}
 
-	// Team numbers are assigned randomly to every player
+		// Team numbers are assigned randomly to every player
-	//TODO: allow customize teams in rmg template
+		//TODO: allow to customize teams in rmg template
-	for(const auto & pair : mapGenOptions.getPlayersSettings())
+		for(const auto & pair : mapGenOptions.getPlayersSettings())
-	{
-		const auto & pSettings = pair.second;
-		PlayerInfo player;
-		player.canComputerPlay = true;
-		int j = (pSettings.getPlayerType() == EPlayerType::COMP_ONLY) ? CPUONLY : CPHUMAN;
-		if (j == CPHUMAN)
 		{
-			player.canHumanPlay = true;
+			const auto & pSettings = pair.second;
-		}
+			PlayerInfo player;
-
+			player.canComputerPlay = true;
-		if(pSettings.getTeam() != TeamID::NO_TEAM)
+			int j = (pSettings.getPlayerType() == EPlayerType::COMP_ONLY) ? CPUONLY : CPHUMAN;
-		{
+			if (j == CPHUMAN)
-			player.team = pSettings.getTeam();
-		}
-		else
-		{
-			if (teamNumbers[j].empty())
 			{
-				logGlobal->error("Not enough places in team for %s player", ((j == CPUONLY) ? "CPU" : "CPU or human"));
+				player.canHumanPlay = true;
-				assert (teamNumbers[j].size());
 			}
-			auto itTeam = RandomGeneratorUtil::nextItem(teamNumbers[j], rand);
+
-			player.team = TeamID(*itTeam);
+			if(pSettings.getTeam() != TeamID::NO_TEAM)
-			teamNumbers[j].erase(itTeam);
+			{
+				player.team = pSettings.getTeam();
+			}
+			else
+			{
+				if (teamNumbers[j].empty())
+				{
+					logGlobal->error("Not enough places in team for %s player", ((j == CPUONLY) ? "CPU" : "CPU or human"));
+					assert (teamNumbers[j].size());
+				}
+				auto itTeam = RandomGeneratorUtil::nextItem(teamNumbers[j], rand);
+				player.team = TeamID(*itTeam);
+				teamNumbers[j].erase(itTeam);
+			}
+			teamsTotal.insert(player.team.getNum());
+			map->getMap(this).players[pSettings.getColor().getNum()] = player;
 		}
-		teamsTotal.insert(player.team.getNum());
+
-		map->getMap(this).players[pSettings.getColor().getNum()] = player;
+		logGlobal->info("Current team count: %d", teamsTotal.size());
+
 	}
+	// FIXME: 0
+	// Can't find info for player 0 (starting zone)
+	// Can't find info for player 1 (starting zone)
 
 	map->getMap(this).howManyTeams = teamsTotal.size();
 }

lib/rmg/CZonePlacer.cpp

@@ -444,9 +444,13 @@ void CZonePlacer::prepareZones(TZoneMap &zones, TZoneVector &zonesVector, const
 				auto playerSettings = map.getMapGenOptions().getPlayersSettings();
 				si32 faction = FactionID::RANDOM;
 				if (vstd::contains(playerSettings, player))
+				{
 					faction = playerSettings[player].getStartingTown();
+				}
 				else
-					logGlobal->error("Can't find info for player %d (starting zone)", player.getNum());
+				{
+					logGlobal->trace("Player %d (starting zone %d) does not participate in game", player.getNum(), zone.first);
+				}
 
 				if (faction == FactionID::RANDOM) //TODO: check this after a town has already been randomized
 					zonesToPlace.push_back(zone);

mapeditor/windownewmap.cpp

@@ -332,7 +332,7 @@ void WindowNewMap::on_humanCombo_activated(int index)
 		ui->humanCombo->setCurrentIndex(humans);
 	}
 
-	mapGenOptions.setPlayerCount(humans);
+	mapGenOptions.setHumanOrCpuPlayerCount(humans);
 
 	int teams = mapGenOptions.getTeamCount();
 	if(teams > humans - 1)
@@ -361,8 +361,10 @@ void WindowNewMap::on_humanCombo_activated(int index)
 
 void WindowNewMap::on_cpuCombo_activated(int index)
 {
-	int humans = mapGenOptions.getPlayerCount();
+	int humans = mapGenOptions.getHumanOrCpuPlayerCount();
 	int cpu = ui->cpuCombo->currentData().toInt();
+
+	// FIXME: Use mapGenOption method only to calculate actual number of players for current template
 	if(cpu > PlayerColor::PLAYER_LIMIT_I - humans)
 	{
 		cpu = PlayerColor::PLAYER_LIMIT_I - humans;
@@ -455,7 +457,7 @@ void WindowNewMap::on_checkSeed_toggled(bool checked)
 
 void WindowNewMap::on_humanTeamsCombo_activated(int index)
 {
-	int humans = mapGenOptions.getPlayerCount();
+	int humans = mapGenOptions.getHumanOrCpuPlayerCount();
 	int teams = ui->humanTeamsCombo->currentData().toInt();
 	if(teams >= humans)
 	{

test/map/CMapFormatTest.cpp

@@ -55,7 +55,7 @@ TEST(MapFormat, Random)
 	opt.setHeight(CMapHeader::MAP_SIZE_MIDDLE);
 	opt.setWidth(CMapHeader::MAP_SIZE_MIDDLE);
 	opt.setHasTwoLevels(true);
-	opt.setPlayerCount(4);
+	opt.setHumanOrCpuPlayerCount(4);
 
 	opt.setPlayerTypeForStandardPlayer(PlayerColor(0), EPlayerType::HUMAN);
 	opt.setPlayerTypeForStandardPlayer(PlayerColor(1), EPlayerType::AI);