createBarbarianCities - bugs

[f1rpo]

I came across two issues with the placement of barbarian cities that I'd like to point out to fellow modders. Also: Does any mod fix these already (perhaps in a smarter way)? K-Mod essentially doesn't change CvGame::createBarbarianCities.

1) When evaluating city plots, the function starts, understandably, with the found value:

iValue = kBarbarianPlayer.AI_foundValue(
    pLoopPlot->getX(), pLoopPlot->getY(),
    GC.getDefineINT("MIN_BARBARIAN_CITY_STARTING_DISTANCE"));

This is multiplied by the number of owned tiles in the area, presumably in order to fill inhabited areas before any terra incognita. (iTargetCitiesMultiplier is normally above 100, actually always at least 150 when playing with Raging Barbarians.)

if(iTargetCitiesMultiplier > 100)
  iValue *= kArea.getNumOwnedTiles();

When placing the first barbarian city in a previously empty area, NumOwnedTiles is 0, which makes the found value irrelevant. Consequently, the first barb city per area is placed entirely at random, which is also the impression I got from playing (see attached screenshot). Proposed fix: Add 1 to NumOwnedTiles.

2) Finally, iValue is deliberately randomized a bit, but really just a tiny bit because iValue can be several hundred thousand after multiplication with NumOwnedTiles.

iValue += 100 + getSorenRandNum(50, "Barb City Found");
iValue /= 100;

So, if barb cities aren't placed entirely at random, they're placed almost in the best spot that the AI can find. It looks like a multiplication with a random number between 1 and 1.5 was intended. The division at the end is pointless in any case. Proposed fix: iValue *= 100 + getSorenRandNum(50, "...");

For reference, the whole loop (BtS 3.19):

Spoiler :

iBestValue = 0;
pBestPlot = NULL;

int iTargetCitiesMultiplier = 100;
{
	int iTargetBarbCities = (getNumCivCities() * 5 * GC.getHandicapInfo(getHandicapType()).getBarbarianCityCreationProb()) / 100;
	int iBarbCities = GET_PLAYER(BARBARIAN_PLAYER).getNumCities();
	if (iBarbCities < iTargetBarbCities)
	{
		iTargetCitiesMultiplier += (300 * (iTargetBarbCities - iBarbCities)) / iTargetBarbCities;
	}

	if (isOption(GAMEOPTION_RAGING_BARBARIANS))
	{
		iTargetCitiesMultiplier *= 3;
		iTargetCitiesMultiplier /= 2;
	}
}

for (iI = 0; iI < GC.getMapINLINE().numPlotsINLINE(); iI++)
{
	pLoopPlot = GC.getMapINLINE().plotByIndexINLINE(iI);

	if (!(pLoopPlot->isWater()))
	{
		if (!(pLoopPlot->isVisibleToCivTeam()))
		{
			iTargetCities = pLoopPlot->area()->getNumUnownedTiles();

			if (pLoopPlot->area()->getNumCities() == pLoopPlot->area()->getCitiesPerPlayer(BARBARIAN_PLAYER))
			{
				iTargetCities *= 3;
			}
								
			int iUnownedTilesThreshold = GC.getHandicapInfo(getHandicapType()).getUnownedTilesPerBarbarianCity();
				
			if (pLoopPlot->area()->getNumTiles() < (iUnownedTilesThreshold / 3))
			{
				iTargetCities *= iTargetCitiesMultiplier;
				iTargetCities /= 100;
			}				

			iTargetCities /= std::max(1, iUnownedTilesThreshold);

			if (pLoopPlot->area()->getCitiesPerPlayer(BARBARIAN_PLAYER) < iTargetCities)
			{
				iValue = GET_PLAYER(BARBARIAN_PLAYER).AI_foundValue(pLoopPlot->getX_INLINE(), pLoopPlot->getY_INLINE(), GC.getDefineINT("MIN_BARBARIAN_CITY_STARTING_DISTANCE"));

				if (iTargetCitiesMultiplier > 100)
				{
					iValue *= pLoopPlot->area()->getNumOwnedTiles();
				}

				iValue += (100 + getSorenRandNum(50, "Barb City Found"));
				iValue /= 100;

				if (iValue > iBestValue)
				{
					iBestValue = iValue;
					pBestPlot = pLoopPlot;
				}
			}
		}
	}
}

if (pBestPlot != NULL)
{
	GET_PLAYER(BARBARIAN_PLAYER).found(pBestPlot->getX_INLINE(), pBestPlot->getY_INLINE());
}