bug in assignStartingPlots ?

[stingo]

In handicap xml there is a parameter called iStartingLocationPercent. The value is low for Settler difficulty and high for Diety. According to logic in CvGame::assignStartingPlots it makes it so that human player can get preference to choose starting location. The lower the parameter the high priority player gets. This is expected, however later in code it essentially removes this advantage and gives random spot to the human player... In the
line 959 in CvGame.cpp it passes True to the function, which makes choosing plot random instead of the best one. It seems to be a bug or possibly disabled intentionally. I think correctly it should pass False instead of True for human player in this line (959) if we want to utilize the effect of iStartingLocationPercent parameter instead of complete randomness. GET_PLAYER((PlayerTypes)iI).setStartingPlot(GET_PLAYER((PlayerTypes)iI).findStartingPlot(), true);

agree or I am missing something?

Edit: There is more to this. The last loop in assignStartingPlots does not make sense. All plots have been assigned earlier but it does it again and also reset to be random for human player again. The entire implementation of assigning plots is terrible. Also this kind of code makes me wonder ...
bValid = true;
if (bValid)
{
iBestValue = iValue;
pBestPlot = pLoopPlot;
}

 

[karadoc]

I haven't looked much at this stuff, but now than I am looking at it I can say that it looks like a complete mess. I sometimes wonder if Civ4 programmers were paid based on the number of lines of code they write. Some of this stuff is just unnecessarily complex.

For example, the code in the first section of assignStartingPlots scans through every plot on the map, for every player, and then at each potential starting plot it finds it scans through every player again to see if the plot is taken. The code therefore scales like plots * players * players. -- What it _should_ do is just go through the map once and make a list of all the starting plots, then assign them one by one. That would scale like plots + players which is around 250 times faster, and would be easier to read. ...

But anyway, I don't think there is a bug. findStartingPlot is the function with the randomizing argument, bRandomize. And the `true` at that part of the code is actually the argument for setStartingPlot, not findStartingPlot.

 

[stingo]

I haven't looked much at this stuff, but now than I am looking at it I can say that it looks like a complete mess. I sometimes wonder if Civ4 programmers were paid based on the number of lines of code they write. Some of this stuff is just unnecessarily complex.

For example, the code in the first section of assignStartingPlots scans through every plot on the map, for every player, and then at each potential starting plot it finds it scans through every player again to see if the plot is taken. The code therefore scales like plots * players * players. -- What it _should_ do is just go through the map once and make a list of all the starting plots, then assign them one by one. That would scale like plots + players which is around 250 times faster, and would be easier to read. ...

But anyway, I don't think there is a bug. findStartingPlot is the function with the randomizing argument, bRandomize. And the `true` at that part of the code is actually the argument for setStartingPlot, not findStartingPlot.

Ahh, you are right about the argument! Some time wasted or may be not. I also noticed the first loop makes all subsequent loops in this function possibly redundant. I commented out first and last loop and map is still generated ok. I am trying to get the best possible location for human player. Thanks for your help.

 

[karadoc]

That first bit does have the power to essentially block the rest of it, but I'm pretty sure that no plot is actually pre-marked as a starting plot when using normally generated maps. ie. The section we're talking about will have no effect unless the map script explicitly specifies where the starting plots should be.