Barbarian cities not appearing

[joseasoler]

http://forums.civfanatics.com/showpost.php?p=13983452&postcount=1091

EDIT: Fixed in BarbsPlus. Merge the change.

[lfgr]

There is definitely something odd with some of the revolution code:

int iOccupiedAreaMultiplier = 50;
int iOwnedPlots = 0;

if(bRagingBarbs)
{
    for( iI = 0; iI < GC.getMAX_PLAYERS(); iI++ )
    {
        iOwnedPlots += GET_PLAYER((PlayerTypes)iI).getTotalLand();
    }

    // When map mostly open, emphasize areas with other civs
    iOccupiedAreaMultiplier += 100 - (iOwnedPlots)/GC.getMapINLINE().getLandPlots();

    // If raging barbs is on, emphasize areas with other civs
    if( bRagingBarbs )
    {
        iOccupiedAreaMultiplier *= 3;
        iOccupiedAreaMultiplier /= 2;
    }
}

Integer division guarantees (iOwnedPlots)/GC.getMapINLINE().getLandPlots() to be 0 or - if every plot is owned - 1. Or am I missing something?

It doesn't seem to do much without Raging barbarians, though.

[joseasoler]

Nice catch; you are right. That specific line is certainly wrong and it will always provide nearly the same multiplier. I have been checking how the code works and I believe that the idea behind even the original code is wrong for Fall from Heaven. The code for placing barbarian cities gives places where civilization is present a HUGE weight, but in Erebus barbarian cities should be able to appear anywhere. For example, check the following original code commented out by revolutions (the code that runs when raging barbarians is off is quite similar):

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

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

The [i]iValue *= pLoopPlot->area()->getNumOwnedTiles()[/i] line is giving a huge value to areas with some owned tiles and eliminating the possibility of having barbarian cities in empty areas when iTargetCitiesMultiplier is greater than 100. As the following code shows, this value will be greater than 100 as long as there are less barbarian cities than the target number.

    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 (bRagingBarbs)
        {
            iTargetCitiesMultiplier *= 3;
            iTargetCitiesMultiplier /= 2;
        }
    }

My conclusion is that the reporter is right and that barbarian cities are extremely rare outside of civilized areas. Tomorrow I will finish running some test games in order to check what is happening (I saw Acheron pop up in the first one, by the way).

[joseasoler]

I forgot to post the results but what I got indicated that barbarian cities placed far away from civilization are quite rare. I'm also going to test with the code I consider faulty commented out so I can see if they start appearing everywhere or not.

[lfgr]

I'm working on this. There are quite a few odd section more, that sometimes even work against each other. It seems like they were added after another and only poorly tested (which is understandable, given their complexity). There are also some checks for raging barbs I don't understand. I'd say we should go for a solution as simple as possible.

I'm currently running tests with the current code to see whether there are more barbarian cities founded on inhabited continents than on uninhabited (fist test without raging barbs after 300 turns: 8 on inhabited, 0 on uninhabited...). Then I'll test variations.

[lfgr]

A quick discussion on the weird parts of the code:

if (bRagingBarbs || isOption(GAMEOPTION_NO_SETTLERS))
{
    if( getNumCivCities() <= (countCivPlayersAlive() * 2))
    {
        return;
    }
}
else
{
    if (getNumCivCities() < (countCivPlayersAlive() * 2))
    {
        return;
    }
}

No idea what that is supposed to do. The only difference is the "<" vs. the "<=" sign, which only makes a difference of one city.

int iRand = getSorenRandNum(100, "Barb City Creation");
if (bRagingBarbs)
{
    if( countCivPlayersAlive() + GET_PLAYER(BARBARIAN_PLAYER).getNumCities()
            < GC.getWorldInfo(GC.getMapINLINE().getWorldSize()).getDefaultPlayers() )
    {
        iRand *= 2;
        iRand /= 3;
    }
}

if ( iRand >= GC.getHandicapInfo(getHandicapType()).getBarbarianCityCreationProb())
{
    return;
}

The first if is missing in vanilla. I have no idea why you would want LESS cities if there are not enough players + barb cities to fill all default slots.

iTargetCitiesMultiplier is, as you said, >100 if there are fewer cities than desired (desired are from 0.2 to 0.4 times the number of non-barb cities, depending on difficulty). It is also always >100 if raging barbarians is activated.

The iOccupiedAreaMultiplier seems to do what it is supposed to if you replace (iOwnedPlots) with (100*iOwnedPlots). It is then ( 50 + (percent of land owned) ) [* 1.5 if raging barbs].

iTargetCities is, as expected, the number of cities desired on the continent based on UnownedTilesPerBarbarianCity (160 to 45, depending on difficulty); however, there are the following exceptions:

• if the continent is empty or there are only barb cities on it, iTargetCities is tripled.
• if the continent's size is lower than UnownedTilesPerBarbarianCity / 3 (so even if empty iTargetCities would be 0), it is multiplied with iTargetCitiesMultiplier / 100. This is the only time that multiplier is used (despite >100 checks) and leads to the odd situation that it is quite possible that a continent of size (UnownedTilesPerBarbarianCity / 3 - 1 ) has a much higher chance to create cities than one of size (UnownedTilesPerBarbarianCity / 3).

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

This is the vanilla section that emphasizes inhabited continents greatly. It is always on if raging barbarians is on, meaning with that option uninhabited areas are only last resort, and without they are mostly, in a pretty erratic way (the "total desired cities" doesn't seem to have to do much with the other values).

The Revolution code works the same without raging barbarians. With raging barbarians, the following code is used instead:

if( pLoopPlot->area()->getNumCities() == pLoopPlot->area()->getCitiesPerPlayer(BARBARIAN_PLAYER) )
{
    // Counteracts the AI_foundValue emphasis on empty areas
    iValue *= 2;
    iValue /= 3;
}

if( iTargetCitiesMultiplier > 100 )     // Either raging barbs is set or fewer barb cities than desired
{
    // Emphasis on placing barb cities in populated areas
    iValue += (iOccupiedAreaMultiplier*(pLoopPlot->area()->getNumCities() - pLoopPlot->area()->getCitiesPerPlayer(BARBARIAN_PLAYER)))/getNumCivCities();
}

The first if may be sensible, no idea why it is just used with raging barbs. The second if... is always true (raging barbs is set here). It applies the probably much more sensible and subtle way of emphasizing inhabited continents.

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

Also vanilla only. That + seems... very suspicious. It should most probably be a *. Else the random +50 are victim to integer rounding, and the +100 doesn't do anything as it is added to every possible city.

if( pLoopPlot->isBestAdjacentFound(BARBARIAN_PLAYER) ) 
{
    iValue *= 120 + getSorenRandNum(30, "Barb City Found");
    iValue /= 100;
}

The Revolution way. This emphasizes plots that are surrounded by worse plots. Other plots don't get a random addition. This sounds good at first look, as it emphasizes local maxima (and, as such, good founding tiles in each ragion). Still, may be too deterministic if there are few such maxima. Maybe other tiles should get a small random value, too.

I'm currently testing a much simplified version of the whole code, commenting most Revolution code and the weird sections of the vanilla code out. There are still a few problems that may arise:

• AI may overvalue uninhabited areas (as suggested in the Revolution comment) and this may lead to too few cities on inhabited continents
• Even without overvaluing, inhabited continents have much less tiles invisible to any player that can spawn barbarian cities
• Many cities on uninhabited continents produce too many units that just run around without doing anything to the game (this is actually a more general problem)

All in all, maybe the more subtle ephasis in the revolution code could be used.

[joseasoler]

I agree with the analysis of the code you write in your first comment. It is quite confusing and contradictory, and whenever I tried to solve this I always ended up running in circles.

Testing multiple variations also led me nowhere as it was difficult for me to compare games as some other factors could also be modifying the result. I hope you will have more luck :)

Your second comment is very helpful; now that I understand better the chaos, I agree with your conclusion. Taking away the messier parts and coming up with a better solution that suits the particular conditions of Fall from Heaven and BarbsPlus sounds like the best idea.

Perhaps it would make sense to include the Wilderness value into the calculation? It could work against the "AI_foundValue emphasis on empty areas" along with the "all cities in the area are barbarian" check. It would make sense to apply a similar penalty in areas with wilderness above some medium-high value. Barbarians may care less about wilderness than civilized people, but everyone prefers to live with less problems.

[lfgr]

Using wilderness seems like a good idea, but I wanted to try it without first to make inclusion in MNAI possible, and it seems to work quite well. I commited the final version here.

My test results are attached here (can't seem to attach files in comments).

Without any bias to inhabited continents, barbarian cities only rarely spawn there.

With the fixed Revolution bias, from turn 0 to 300, there spawn some more cities on inhabited continents than on uninhabited, but that's okay IMO, since these are more interesting anyway. At least in every game a few cities spawned on uninhabited continents (and, while I have no data on this, I had the impression that acheron also spawned in most games).

[joseasoler]

Awesome!

I have been checking your logs. The first game of the last solution (occupied bonus 2) seems to cluster barbarian cities a lot, but since they are clustering in areas with big numbers of player cities (which are also probably big areas) that seems okay. The other logs seem to be the best in comparison with the others so I agree that this looks like the best solution overall. I guess we can let the matter of Acheron (which already seemed okay in my tests without these improvements) as it is now until we get more feedback after a release.

[joseasoler]

I forgot to push this one to 0.6, as there are also a lot of nice new features in BarbsPlus that should wait for a beta to be merged. I got Acheron today in two different games while testing, by the way :)

[joseasoler]

This issue should be fixed after the BarbsPlus merge.