Fixed most of reported issues, removed unused code.

client/lobby/RandomMapTab.cpp

@@ -209,13 +209,12 @@ void RandomMapTab::updateMapInfoByHost()
 	for (auto& player : mapGenOptions->getPlayersSettings())
 	{
 		PlayerInfo playerInfo;
-		playerInfo.isFactionRandom = (player.second.getStartingTown() == CMapGenOptions::CPlayerSettings::RANDOM_TOWN);
+		playerInfo.isFactionRandom = (player.second.getStartingTown() == FactionID::RANDOM);
 		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;
@@ -244,31 +243,16 @@ void RandomMapTab::setMapGenOptions(std::shared_ptr<CMapGenOptions> opts)
 			compTeamsAllowed.insert(i);
 		}
 	}
+	std::set<int> humanCountAllowed;
 
 	auto * tmpl = mapGenOptions->getMapTemplate();
 	if(tmpl)
 	{
 		playerCountAllowed = tmpl->getPlayers().getNumbers();
-		compCountAllowed = tmpl->getCpuPlayers().getNumbers();
+		humanCountAllowed = tmpl->getHumanPlayers().getNumbers(); // Unused now?
-		auto compNumbers = tmpl->getCpuPlayers().getNumbers();
-		if (!compNumbers.empty())
-		{
-			compCountAllowed = compNumbers;
-			minComps = *boost::min_element(compCountAllowed);
-		}
-
-		playerCountAllowed = tmpl->getPlayers().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);
-		}	
 	}
 	
-	si8 playerLimit = opts->getPlayerLimit();
+	si8 playerLimit = opts->getMaxPlayersCount();
 	si8 humanOrCpuPlayerCount = opts->getHumanOrCpuPlayerCount();
 	si8 compOnlyPlayersCount = opts->getCompOnlyPlayerCount();
 
@@ -282,12 +266,23 @@ void RandomMapTab::setMapGenOptions(std::shared_ptr<CMapGenOptions> opts)
 		{
 			return humanOrCpuPlayerCount <= el;
 		});
-		
-		if(!playerTeamsAllowed.count(opts->getTeamCount()))
-		{
-		   opts->setTeamCount(CMapGenOptions::RANDOM_SIZE);
-		}
 	}
+	else // Random
+	{
+		vstd::erase_if(compCountAllowed, [playerLimit, humanOrCpuPlayerCount](int el)
+		{
+			return (playerLimit - 1) < el; // Must leave at least 1 human player
+		});
+		vstd::erase_if(playerTeamsAllowed, [playerLimit](int el)
+		{
+			return playerLimit <= el;
+		});
+	}
+	if(!playerTeamsAllowed.count(opts->getTeamCount()))
+	{
+		opts->setTeamCount(CMapGenOptions::RANDOM_SIZE);
+	}
+
 	if(compOnlyPlayersCount != CMapGenOptions::RANDOM_SIZE)
 	{
 		// This setting doesn't impact total number of players
@@ -328,8 +323,6 @@ void RandomMapTab::setMapGenOptions(std::shared_ptr<CMapGenOptions> opts)
 	}
 	if(auto w = widget<CToggleGroup>("groupMaxPlayers"))
 	{
-		// 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);
 	}
@@ -442,8 +435,8 @@ TeamAlignmentsWidget::TeamAlignmentsWidget(RandomMapTab & randomMapTab):
 	const JsonNode config(JsonPath::builtin("config/widgets/randomMapTeamsWidget.json"));
 	variables = config["variables"];
 	
-	int totalPlayers = randomMapTab.obtainMapGenOptions().getPlayerLimit();
+	//int totalPlayers = randomMapTab.obtainMapGenOptions().getPlayerLimit();
-	//randomMapTab.obtainMapGenOptions().getTotalPlayersCount();
+	int totalPlayers = randomMapTab.obtainMapGenOptions().getMaxPlayersCount();
 	assert(totalPlayers <= PlayerColor::PLAYER_LIMIT_I);
 	auto settings = randomMapTab.obtainMapGenOptions().getPlayersSettings();
 	variables["totalPlayers"].Integer() = totalPlayers;
@@ -482,7 +475,7 @@ TeamAlignmentsWidget::TeamAlignmentsWidget(RandomMapTab & randomMapTab):
 	
 	OBJ_CONSTRUCTION;
 	
-	// Window should have X * X columns, where X is players + compOnly players.
+	// Window should have X * X columns, where X is max players allowed for current settings
 	// For random player count, X is 8
 
 	if (totalPlayers > settings.size())

lib/rmg/CMapGenOptions.cpp

@@ -85,11 +85,48 @@ void CMapGenOptions::setHumanOrCpuPlayerCount(si8 value)
 	resetPlayersMap();
 }
 
-si8 CMapGenOptions::getTotalPlayersCount() const
+si8 CMapGenOptions::getMinPlayersCount(bool withTemplateLimit) const
 {
 	auto totalPlayers = 0;
 	si8 humans = getHumanOrCpuPlayerCount();
 	si8 cpus = getCompOnlyPlayerCount();
+
+	if (humans == RANDOM_SIZE && cpus == RANDOM_SIZE)
+	{
+		totalPlayers = 2;
+	}
+	else if (humans == RANDOM_SIZE)
+	{
+		totalPlayers = cpus + 1; // Must add at least 1 player
+	}
+	else if (cpus == RANDOM_SIZE)
+	{
+		totalPlayers = humans;
+	}
+	else
+	{
+		totalPlayers = humans + cpus;
+	}
+
+	if (withTemplateLimit && mapTemplate)
+	{
+		auto playersRange = mapTemplate->getPlayers();
+
+		//New template can also impose higher limit than current settings
+		vstd::amax(totalPlayers, playersRange.minValue());
+	}
+
+	// Can't play without at least 2 players
+	vstd::amax(totalPlayers, 2);
+	return totalPlayers;
+}
+
+si8 CMapGenOptions::getMaxPlayersCount(bool withTemplateLimit) const
+{
+	// Max number of players possible with current settings
+	auto totalPlayers = 0;
+	si8 humans = getHumanOrCpuPlayerCount();
+	si8 cpus = getCompOnlyPlayerCount();
 	if (humans == RANDOM_SIZE || cpus == RANDOM_SIZE)
 	{
 		totalPlayers = PlayerColor::PLAYER_LIMIT_I;
@@ -98,6 +135,15 @@ si8 CMapGenOptions::getTotalPlayersCount() const
 	{
 		totalPlayers = humans + cpus;
 	}
+
+	if (withTemplateLimit && mapTemplate)
+	{
+		auto playersRange = mapTemplate->getPlayers();
+
+		//New template can also impose higher limit than current settings
+		vstd::amin(totalPlayers, playersRange.maxValue());
+	}
+
 	assert (totalPlayers <= PlayerColor::PLAYER_LIMIT_I);
 	assert (totalPlayers >= 2);
 	return totalPlayers;
@@ -121,10 +167,11 @@ si8 CMapGenOptions::getCompOnlyPlayerCount() const
 
 si8 CMapGenOptions::getPlayerLimit() const
 {
+	//How many players could we set with current template, ignoring other settings
 	si8 playerLimit = PlayerColor::PLAYER_LIMIT_I;
 	if (auto temp = getMapTemplate())
 	{
-		playerLimit = *boost::max_element(temp->getPlayers().getNumbers());
+		playerLimit = static_cast<si8>(temp->getPlayers().maxValue());
 	}
 	return playerLimit;
 }
@@ -207,48 +254,28 @@ void CMapGenOptions::resetPlayersMap()
 
 	savePlayersMap();
 
-	/*
+	int realPlayersCnt = getMaxPlayersCount();
-	//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;
-	});
-	*/
 
-	// FIXME: This should be total players count
+	//Trim the number of AI players, then CPU-only players, finally human players
-	int realPlayersCnt = getHumanOrCpuPlayerCount();
+	auto eraseLastPlayer = [this](EPlayerType playerType) -> bool
-	if (realPlayersCnt != RANDOM_SIZE)
 	{
-		//Trim the number of AI players, then CPU-only players, finally human players
+		for (auto it = players.rbegin(); it != players.rend(); ++it)
-		auto eraseLastPlayer = [this](EPlayerType playerType) -> bool
 		{
-			//FIXME: Infinite loop for 0 players
+			if (it->second.getPlayerType() == playerType)
-			for (auto it = players.rbegin(); it != players.rend(); ++it)
 			{
-				if (it->second.getPlayerType() == playerType)
+				players.erase(it->first);
-				{
+				return true;
-					players.erase(it->first);
-					return true;
-				}
 			}
-			return false; //Can't earse any player of this type
-		};
-
-		while (players.size() > realPlayersCnt)
-		{
-			while (eraseLastPlayer(EPlayerType::AI));
-			while (eraseLastPlayer(EPlayerType::COMP_ONLY));
-			while (eraseLastPlayer(EPlayerType::HUMAN));
 		}
-	}
+		return false; //Can't earse any player of this type
-	else
+	};
-	{
-		//If count is random, generate info for all the players
-		realPlayersCnt = PlayerColor::PLAYER_LIMIT_I;
-	}
 
-	int realCompOnlyPlayersCnt = getCompOnlyPlayerCount();
+	while (players.size() > realPlayersCnt)
-	//int totalPlayersLimit = getPlayerLimit();
+	{
+		while (eraseLastPlayer(EPlayerType::AI));
+		while (eraseLastPlayer(EPlayerType::COMP_ONLY));
+		while (eraseLastPlayer(EPlayerType::HUMAN));
+	}
 
 	//First colors from the list are assigned to human players, then to CPU players
 	std::vector<PlayerColor> availableColors;
@@ -263,7 +290,7 @@ void CMapGenOptions::resetPlayersMap()
 		{
 			if (player.second.getPlayerType() == playerType)
 			{
-				vstd::erase(availableColors, player.second.getColor()); //FIXME: Where is this color initialized at lobby launch?
+				vstd::erase(availableColors, player.second.getColor());
 			}
 		}
 	};
@@ -409,13 +436,17 @@ void CMapGenOptions::setMapTemplate(const CRmgTemplate * value)
 			setHeight(sizes.first.y);
 			setHasTwoLevels(sizes.first.z - 1);
 		}
-		
+
-		// FIXME: GUI settings are not the same as template parameters
+		si8 maxPlayerCount = getMaxPlayersCount(false);
-		// TODO: Recalculate GUI ranges in separate method
+		si8 minPlayerCount = getMinPlayersCount(false);
-		if(!mapTemplate->getPlayers().isInRange(getHumanOrCpuPlayerCount()))
+
+		// Neither setting can fit within the template range
+		if(!mapTemplate->getPlayers().isInRange(minPlayerCount) &&
+			!mapTemplate->getPlayers().isInRange(maxPlayerCount))
+		{
 			setHumanOrCpuPlayerCount(RANDOM_SIZE);
-		if(!mapTemplate->getCpuPlayers().isInRange(getCompOnlyPlayerCount()))
 			setCompOnlyPlayerCount(RANDOM_SIZE);
+		}
 		if(!mapTemplate->getWaterContentAllowed().count(getWaterContent()))
 			setWaterContent(EWaterContent::RANDOM);
 	}
@@ -473,11 +504,17 @@ void CMapGenOptions::finalize(CRandomGenerator & rand)
 	
 	logGlobal->info("RMG template name: %s", mapTemplate->getName());
 
+	auto maxPlayers = getMaxPlayersCount();
 	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()));
+
+		vstd::erase_if(possiblePlayers, [maxPlayers](int i)
+		{
+			return i > maxPlayers;
+		});
 		assert(!possiblePlayers.empty());
 		setHumanOrCpuPlayerCount (*RandomGeneratorUtil::nextItem(possiblePlayers, rand));
 		updatePlayers();
@@ -490,7 +527,13 @@ void CMapGenOptions::finalize(CRandomGenerator & rand)
 	}
 	if(compOnlyPlayerCount == RANDOM_SIZE)
 	{
-		auto possiblePlayers = mapTemplate->getCpuPlayers().getNumbers();
+		// Use remaining range
+		auto presentPlayers = getHumanOrCpuPlayerCount();
+		auto possiblePlayers = mapTemplate->getPlayers().getNumbers();
+		vstd::erase_if(possiblePlayers, [maxPlayers, presentPlayers](int i)
+		{
+			return i > (maxPlayers - presentPlayers);
+		});
 		compOnlyPlayerCount = *RandomGeneratorUtil::nextItem(possiblePlayers, rand);
 		updateCompOnlyPlayers();
 	}
@@ -541,13 +584,8 @@ 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);
 	}
-	// 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()));
 }
 
@@ -669,21 +707,38 @@ std::vector<const CRmgTemplate *> CMapGenOptions::getPossibleTemplates() const
 		if(!tmpl->isWaterContentAllowed(getWaterContent()))
 			return true;
 
-		if(getHumanOrCpuPlayerCount() != CMapGenOptions::RANDOM_SIZE)
+		auto humanOrCpuPlayerCount = getHumanOrCpuPlayerCount();
+		auto compOnlyPlayerCount =  getCompOnlyPlayerCount();
+		// Check if total number of players fall inside given range
+
+		if(humanOrCpuPlayerCount != CMapGenOptions::RANDOM_SIZE && compOnlyPlayerCount != CMapGenOptions::RANDOM_SIZE)
 		{
-			if (!tmpl->getPlayers().isInRange(getHumanOrCpuPlayerCount()))
+			if (!tmpl->getPlayers().isInRange(humanOrCpuPlayerCount + compOnlyPlayerCount))
+				return true;
+
+		}
+		else if(humanOrCpuPlayerCount != CMapGenOptions::RANDOM_SIZE)
+		{
+			// We can always add any number CPU players, but not subtract
+			if (!(humanOrCpuPlayerCount <= tmpl->getPlayers().maxValue()))
+				return true;
+		}
+		else if(compOnlyPlayerCount != CMapGenOptions::RANDOM_SIZE)
+		{
+			//We must fit at least one more human player, but can add any number
+			if (!(compOnlyPlayerCount < tmpl->getPlayers().maxValue()))
 				return true;
 		}
 		else
 		{
 			// Human players shouldn't be banned when playing with random player count
-			if(humanPlayers > *boost::min_element(tmpl->getPlayers().getNumbers()))
+			if(humanPlayers > tmpl->getPlayers().minValue())
 				return true;
 		}
 
 		if(compOnlyPlayerCount != CMapGenOptions::RANDOM_SIZE)
 		{
-			if (!tmpl->getCpuPlayers().isInRange(compOnlyPlayerCount))
+			if (!tmpl->getHumanPlayers().isInRange(compOnlyPlayerCount))
 				return true;
 		}
 

lib/rmg/CMapGenOptions.h

@@ -53,9 +53,6 @@ public:
 		TeamID getTeam() const;
 		void setTeam(const TeamID & value);
 
-		/// Constant for a random town selection.
-		static const si32 RANDOM_TOWN = -1;
-
 	private:
 		PlayerColor color;
 		si32 startingTown;
@@ -91,7 +88,8 @@ public:
 	si8 getHumanOrCpuPlayerCount() const;
 	void setHumanOrCpuPlayerCount(si8 value);
 
-	si8 getTotalPlayersCount() const;
+	si8 getMinPlayersCount(bool withTemplateLimit = true) const;
+	si8 getMaxPlayersCount(bool withTemplateLimit = true) const;
 	si8 getPlayerLimit() const;
 
 	/// The count of the teams ranging from 0 to <players count - 1> or RANDOM_SIZE for random.

lib/rmg/CRmgTemplate.cpp

@@ -557,9 +557,9 @@ const CRmgTemplate::CPlayerCountRange & CRmgTemplate::getPlayers() const
 	return players;
 }
 
-const CRmgTemplate::CPlayerCountRange & CRmgTemplate::getCpuPlayers() const
+const CRmgTemplate::CPlayerCountRange & CRmgTemplate::getHumanPlayers() const
 {
-	return cpuPlayers;
+	return humanPlayers;
 }
 
 const CRmgTemplate::Zones & CRmgTemplate::getZones() const
@@ -675,13 +675,23 @@ void CRmgTemplate::CPlayerCountRange::fromString(const std::string & value)
 	}
 }
 
+int CRmgTemplate::CPlayerCountRange::maxValue() const
+{
+	return *boost::max_element(getNumbers());
+}
+
+int CRmgTemplate::CPlayerCountRange::minValue() const
+{
+	return *boost::min_element(getNumbers());
+}
+
 void CRmgTemplate::serializeJson(JsonSerializeFormat & handler)
 {
 	handler.serializeString("name", name);
 	serializeSize(handler, minSize, "minSize");
 	serializeSize(handler, maxSize, "maxSize");
 	serializePlayers(handler, players, "players");
-	serializePlayers(handler, cpuPlayers, "cpu");
+	serializePlayers(handler, humanPlayers, "cpu"); // TODO: Rename this parameter
 
 	{
 		auto connectionsData = handler.enterArray("connections");

lib/rmg/CRmgTemplate.h

@@ -231,6 +231,9 @@ public:
 		std::string toString() const;
 		void fromString(const std::string & value);
 
+		int maxValue() const;
+		int minValue() const;
+
 	private:
 		std::vector<std::pair<int, int> > range;
 	};
@@ -247,7 +250,7 @@ public:
 	const std::string & getName() const;
 
 	const CPlayerCountRange & getPlayers() const;
-	const CPlayerCountRange & getCpuPlayers() const;
+	const CPlayerCountRange & getHumanPlayers() const;
 	std::pair<int3, int3> getMapSizes() const;
 	const Zones & getZones() const;
 	const std::vector<rmg::ZoneConnection> & getConnectedZoneIds() const;
@@ -261,7 +264,7 @@ private:
 	std::string id;
 	std::string name;
 	int3 minSize, maxSize;
-	CPlayerCountRange players, cpuPlayers;
+	CPlayerCountRange players, humanPlayers;
 	Zones zones;
 	std::vector<rmg::ZoneConnection> connectedZoneIds;
 	std::set<EWaterContent::EWaterContent> allowedWaterContent;

test/map/CMapFormatTest.cpp

@@ -46,7 +46,7 @@ TEST(MapFormat, Random)
 	CRmgTemplate tmpl;
 	std::shared_ptr<ZoneOptionsFake> zoneOptions = std::make_shared<ZoneOptionsFake>();
 
-	const_cast<CRmgTemplate::CPlayerCountRange &>(tmpl.getCpuPlayers()).addRange(1, 4);
+	const_cast<CRmgTemplate::CPlayerCountRange &>(tmpl.getHumanPlayers()).addRange(1, 4);
 	const_cast<CRmgTemplate::Zones &>(tmpl.getZones())[0] = zoneOptions;
 
 	zoneOptions->setOwner(1);