The modders' lair: (CIP) Code Improvement Project

[Nightinggale]

Second, as we see there are three almost identical for-loops iterating over MAX_PLAYERS, which in the case of RaR equals 48, making the total of all iterations not less than "just" 110592... Edit: I've done the math wrong. It is 48 plus 48², which totals up to 2352 (I hope this time I am right )

You are wrong again

First loop executes 48 times, but all it does is counting players in the game with water start. No real overkill here.

Second loop runs 48 times too, but it does nothing for players not alive or not having a water start meaning the amount of iterations of the inner loop is way lower than 48². In fact the body of the inner loop iterates X² where X is the number of active European civilizations. This mean the real number could be something like 25 rather than the fairly large number you came up with.

Besides it is kind of hard to make overkill here. It executes when starting a new game. I'm ok with having poorly optimized startup code, which uses say 0.3 seconds more than it has to. It isn't in a location where it really matters.

The real question is if the approach is a good idea. I don't really have any comments on that other than it seems to work just fine.

 

[Commander Bello]

You are wrong again

First loop executes 48 times, but all it does is counting players in the game with water start. No real overkill here.

Oh yes.
For what reason do I need a loop iterating 48 times to tell me that there are max 8 players with a water start? And one can even very well argue about the need to know this.

Second loop runs 48 times too, but it does nothing for players not alive or not having a water start meaning the amount of iterations of the inner loop is way lower than 48². In fact the body of the inner loop iterates X² where X is the number of active European civilizations. This mean the real number could be something like 25 rather than the fairly large number you came up with.

To this I have to agree.

Besides it is kind of hard to make overkill here. It executes when starting a new game. I'm ok with having poorly optimized startup code, which uses say 0.3 seconds more than it has to. It isn't in a location where it really matters.

The real question is if the approach is a good idea. I don't really have any comments on that other than it seems to work just fine.

Let's say "it works" - from a technical point of view.
From a conceptual point of view I am still missing the added value. All I could think of is that you might get the same order of appearances of European nations if you always would have set the same players manually.
And if you want to avoid this you could even do something like this (dirty pseudo-code):

for (iJ = 0; iJ < MAX_PLAYERS; iJ++)
{
if (GET_PLAYER((PlayerTypes)iJ).isEurope() && GET_PLAYER((PlayerTypes)iJ).hasParent() && !GET_PLAYER((PlayerTypes)iJ).isHuman())
{
iRandStart = getSorenRandNum(iJ, "");
if (iRandStart < iJ)
{
pTempPlot = GET_PLAYER((PlayerTypes)iJ).getStartingPlot();
GET_PLAYER((PlayerTypes)iJ).setStartingPlot(GET_PLAYER((PlayerTypes)0).getStartingPlot(), true);
GET_PLAYER((PlayerTypes)0).setStartingPlot(pTempPlot, true);
iJ = MAX_PLAYERS;
}}}


I just always get a strange feeling when seeing a lot of loops to do something which I think can be done in a much easier way.

 

[Commander Bello]

Ok, I just ran a few tests with having this function deactivated (I am making use of my modmod, so you can appear in the west as well).
Result: in four different attempts (cache deactivated and game always started completely new, but of course same map size and therefore same number of players) I as the English always spawned in the SW corner of the map with the other Europeans randomly set somewhere else.
The SE corner is also chosen if the human player picks the French (fifth attempt). So one can assume that the human player indeed will always spawn in the same area if no randomization is enabled.

Now I just have to try to make the randomization a bit "leaner".

 

[Nightinggale]

Now I looked up the code in VC++ as it gives a better impression than just reading it on the forum. First of all it isn't in vanilla, but it doesn't say who wrote

// PatchMod: Random Start Locs

That mean it is a bit tricky to find the original author and ask. It's even harder if that person isn't active anymore.

The code is actually quite simple.

First loop counts the number of players with water start and store this in iWaterCount.

In second loop, for every player (A) having a water start, it does

1. find a random number between 0 and iWaterCount and store this in iRandStart
2. use inner loop to pass iRandStart number of players with water start
3. once this number have been reached, it points at player B. A and B then switch starting plots.
4. inner loop ends even though it haven't gone though all players

You argue that going through MAX_PLAYERS is slower than going through active players. You are right, but then you need to cache the highest playerID active in the game and then you have to look that one up every time you use it. MAX_PLAYERS is hardcoded by the preprocessor and is set at compile time, which is faster than looking up a cache. It really is a tradeoff and in this case a cache might improve a little. However it is very minor.

I have done quite a bit of profiling and I see no real performance loss regarding looping all players. It isn't something you should be concerned with.

The loop you just wrote can make a European switch starting plot with a native. Needless to say that wouldn't be good to have a land start with your caravel

 

[Commander Bello]

Now I looked up the code in VC++ as it gives a better impression than just reading it on the forum. First of all it isn't in vanilla, but it doesn't say who wrote

// PatchMod: Random Start Locs

That mean it is a bit tricky to find the original author and ask. It's even harder if that person isn't active anymore.

Well, I always thought that "PatchMod" was part of the 1.01f patch?

The code is actually quite simple.

First loop counts the number of players with water start and store this in iWaterCount.

In second loop, for every player (A) having a water start, it does

1. find a random number between 0 and iWaterCount and store this in iRandStart
2. use inner loop to pass iRandStart number of players with water start
3. once this number have been reached, it points at player B. A and B then switch starting plots.
4. inner loop ends even though it haven't gone though all players

You argue that going through MAX_PLAYERS is slower than going through active players. You are right, but then you need to cache the highest playerID active in the game and then you have to look that one up every time you use it. MAX_PLAYERS is hardcoded by the preprocessor and is set at compile time, which is faster than looking up a cache. It really is a tradeoff and in this case a cache might improve a little. However it is very minor.

I have done quite a bit of profiling and I see no real performance loss regarding looping all players. It isn't something you should be concerned with.

You may be right, as in the beginning I got confused with the result of the loops. Of course, the less iterations you have the less you have to be concerned.

The loop you just wrote can make a European switch starting plot with a native. Needless to say that wouldn't be good to have a land start with your caravel

No, it doesn't as you will see here:

		int iRandStart= 0;
		CvPlot* pTempPlot = NULL;
		for (int iI=0; iI < MAX_PLAYERS; iI++)
		{
			if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iI).getCivilizationType()).isWaterStart() && !GET_PLAYER((PlayerTypes)iI).isHuman())  // ensuring that only European players are taken into consideration
			{
				iRandStart = getSorenRandNum(iI, "");
				if (iRandStart < iI)
				{
					pTempPlot = GET_PLAYER((PlayerTypes)iI).getStartingPlot();
					GET_PLAYER((PlayerTypes)iI).setStartingPlot(GET_PLAYER((PlayerTypes)0).getStartingPlot(), true);
					GET_PLAYER((PlayerTypes)0).setStartingPlot(pTempPlot, true);
					iI = MAX_PLAYERS;
				}
			}
		}

This seems to do the same, as I ran three tests now and always came up in different places.

What makes my code different from the previous one is that it sometimes causes 3 players in the west and 5 in the east (gigantic maps), where previously it was always equally split: 4 in the West and 4 in the East.
I have to think about which is better... Anyway, it was a nice little exercise for me.

 

[raystuttgart]

// PatchMod: Random Start Locs

Well, I always thought that "PatchMod" was part of the 1.01f patch?

No, the code you are talking about (considering randomization of Starting locatios) was introduced to TAC (and thus also RaR) by me.
(That is years ago now though.)

However, I simply copied code from Dale, creator of AODII.
(Dale is one of the best modders this community ever had.)

As far as I remember, I did not modify the code myself though.
So I did not put my name on it but used the same comments as Dale had.

Edit:

I introduced that piece of code in TAC to fix a Vanilla bug:
"Human Player always starting at the same location."

The fix was desperately wanted in those days by the TAC team.


By the way:

The code is absolutely fine as it is.
(Everything in that code is important and has its purpose.)

* 3 programmers - Dale (when he wrote it), myself (when I merged it) and koma13 (when he reviewed) had taken a look
* It was tested very well in TAC
* It has proven to be working properly for years now

If at all, you might be able to improve performance (initial loading time) of that piece of code by about 0.1 seconds.
(Looping through a couple of players and doing such simple logic is incredibly harmless considering performance.)

That mean it is a bit tricky to find the original author and ask. It's even harder if that person isn't active anymore.

If it is code that was introduced in TAC or RaR, it isn't that difficult to find out.
Just ask me. In 99% I will know who introduced or created it.

 

[Nightinggale]

By the way:

The code is absolutely fine as it is.
(Everything in that code is important and has its purpose.)

I completely agree. Don't modify code unless you know it isn't working like you want to.

* 3 programmers - Dale (when he wrote it), myself (when I merged it) and koma13 (when he reviewed) had taken a look

Add me to the list. I didn't see anything wrong with it either.

 

[Commander Bello]

Here is another function which I don't get:

int [B]CvCityAI::AI_estimateYieldValue[/B](YieldTypes eYield, int iAmount) const
{
	int iValue = iAmount * GET_PLAYER(getOwnerINLINE()).AI_yieldValue(eYield);
	
	switch (eYield)
	{
		case YIELD_FOOD:
		case YIELD_LUMBER:
		case YIELD_SILVER:
		case YIELD_COTTON:
		case YIELD_FUR:
		case YIELD_SUGAR:
		case YIELD_TOBACCO:
		case YIELD_ORE:
		case YIELD_CLOTH:
		case YIELD_COATS:
		case YIELD_RUM:
		case YIELD_CIGARS:
		case YIELD_TOOLS:
		case YIELD_MUSKETS:
		case YIELD_HORSES:
		case YIELD_TRADE_GOODS:
		case YIELD_HAMMERS:
			break;
		case YIELD_BELLS:
			break;
		case YIELD_CROSSES:
			break;
		case YIELD_EDUCATION:
			break;
		default:
			FAssert(false);
	}
	
	return iValue;
}

All it does is to multiply iValue with iAmount. Nice, but for what is the switch-case command?
And for what are the multiple "breaks"? As far as I see it, a break after case YIELD_EDUCATION: would have been sufficient.

To me, it looks pretty much like something unfinished.

 

[Nightinggale]

The vanilla code:

Spoiler :

int CvCityAI::AI_estimateYieldValue(YieldTypes eYield, int iAmount) const
{
	int iValue = iAmount * GET_PLAYER(getOwnerINLINE()).AI_yieldValue(eYield);
	
	switch (eYield)
	{
		case YIELD_FOOD:
		case YIELD_LUMBER:
		case YIELD_SILVER:
		case YIELD_COTTON:
		case YIELD_FUR:
		case YIELD_SUGAR:
		case YIELD_TOBACCO:
		case YIELD_ORE:
		case YIELD_CLOTH:
		case YIELD_COATS:
		case YIELD_RUM:
		case YIELD_CIGARS:
		case YIELD_TOOLS:
		case YIELD_MUSKETS:
		case YIELD_HORSES:
		case YIELD_TRADE_GOODS:
		case YIELD_HAMMERS:
			break;
		case YIELD_BELLS:
			break;
		case YIELD_CROSSES:
			break;
		case YIELD_EDUCATION:
			break;
		default:
			FAssert(false);
	}
	
	return iValue;
}

It looks to me like it at one point during development did something, but then it was moved to CvPlayerAI::AI_yieldValue(). In fact the functions in that part of the file all have a switch case based on yields.

Like the last function you found, it is presumably better to leave it untouched than to mess with something where you don't know why it is written like that. You could add the return statement before the switch case, but it wouldn't change anything at all.

 

[Commander Bello]

(Just making use of the code sniplet from above ...)

		// PatchMod: Random Start Locs START
		// Randomise start locations amongst water starters (Human Europeans)

		int iWaterCount = 0;
		int iCounter = 0;
		[B][COLOR="Green"]for (iI = 0; iI < MAX_PLAYERS; iI++)
		{
			if (GET_PLAYER((PlayerTypes)iI).isAlive())
			{
				if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iI).getCivilizationType()).isWaterStart())[/COLOR][/B]
				{
					iWaterCount++;
				}
			}
		}
		int iRandStart = 0;
		CvPlot* pTempPlot = NULL;
		[B][COLOR="blue"]for (iJ = 0; iJ < MAX_PLAYERS; iJ++)
		{
			if (GET_PLAYER((PlayerTypes)iJ).isAlive())
			{
				if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iJ).getCivilizationType()).isWaterStart()[/COLOR][/B] && GET_PLAYER((PlayerTypes)iJ).isHuman())
				{
					iRandStart = getSorenRandNum(iWaterCount, "");
					iCounter = 0;
					[B][COLOR="Red"]for (iI = 0; iI < MAX_PLAYERS; iI++)
					{
						if (GET_PLAYER((PlayerTypes)iI).isAlive())
						{
							if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iI).getCivilizationType()).isWaterStart())[/COLOR][/B]
							{
								iCounter++;
							}
						}
						if (iCounter == iRandStart)
						{
							pTempPlot = GET_PLAYER((PlayerTypes)iJ).getStartingPlot();
							GET_PLAYER((PlayerTypes)iJ).setStartingPlot(GET_PLAYER((PlayerTypes)iI).getStartingPlot(), true);
							GET_PLAYER((PlayerTypes)iI).setStartingPlot(pTempPlot, true);
							iI = MAX_PLAYERS;
						}
					}
				}
			}

Sorry, guys, for bringing this example up again, but I think I can use it to demonstrate some things.

Let's have a look at the first (green) loop. We are iterating over all (in the case of RaR) 48 players to find out if they (a) are alive (which might be the case for all of them) and then (b) if they are starting on the water (colonies) to increment a counter.

This could have been avoided in this case by simply switching the second and third condition line:

		[B][COLOR="Green"]for (iI = 0; iI < MAX_PLAYERS; iI++)
		{
			if (GC.getCivilizationInfo(GET_PLAYER((PlayerTypes)iI).getCivilizationType()).isWaterStart())
			{
				if (GET_PLAYER((PlayerTypes)iI).isAlive())[/COLOR][/B]
				{
					iWaterCount++;
				}
			}
		}

Now we no longer have to check 48 times if the player has a water start, but only 8 times if the player is alive: By just switching the filter conditions, we have reduced the number of calculations to 1/6.

Now to the blue and red lines. Please notice that I am making use of those just as an example (I am very aware of the conditions .isHuman() / .isWaterStart() which in this case drastically reduce the iterations, but as I said, it's just an example. Similar constructions regarding loops inside loops you will find each and everywhere in the code).

In general we iterate two times over the whole number of players in the game (MAX_PLAYERS = 48) meaning that we have a total of 48² = 2304 iterations.

But let's have a look at what is really going on.
The respective conditions are:
for (iI = 0; iI < MAX_PLAYERS; iI++) and embedded in it
for (iJ = 0; iJ < MAX_PLAYERS; iJ++)

The consequence is to check Player(i=0) with Player(j=0,1,2....), Player(i=1) with Player(j=0,1,2....) and so forth...

But in fact we only have to do the following:
For Player(i=0) do 46 checks against other players, for Player(i=1) do 45 checks against other players and so forth. After all, whether you check Player(0) against Player(1) or Player(1) against Player(0) typically won't make a difference.

So, if we replace above conditions with:
for (iI = 0; iI < MAX_PLAYERS-1; iI++) and embedded in it
for (iJ = i+1; iJ < MAX_PLAYERS; iJ++)
we should reduce the number of iterations to almost the half: 1128

If we now calculate the worst case over all three loops and compare it with the changed loops, we see the difference:
48 + 48² = 2352 vs 1136 = 8 + 1128 iterations.

As I said, similar constructions you'll find each and everywhere. Might be worth the effort to have a closer look next time if one could save some time.

 

[Nightinggale]

If you really want to make the game run faster, try to profile the game first and then look at the code, which actually take at least 1% of the time spend in the DLL. You can completely remove the code you are looking at now and not be able to tell the speed difference when running the game.

Optimizing the game to make a difference is kind of hard as I already profiled and optimized the parts I discovered as slow. I reduced next turn event calculations by 40% in RaR and then I have run out of good candidates to optimize.

 

[raystuttgart]

@Commander Bello:

I am sorry, but I won't even try to read this post thoroughly.

The post is way too long and too confusing.
(Lots of code, pseudo code an explanations mixed.)

It would take me much too long to thoroughly think into what you are trying to explain.

And I am totally convinced that that time needed would be wasted (for me) because:

1. The code is working properly.
2. The possible performance improvment by rewritting the code is so minimal, it is not worth to bother.
3. Rewriting code without any real benefit is not necessarily a good strategy.

I wish you good luck with your programming though.
(It is simply too time consuming for me to follow and I want to spend the little time I have available for modding more useful.)

 

[orlanth]

Commander Bello, I have to agree that Nightingale and ray already invested a lot of efforts in profiling and optimizing the code for memory usage and speed, to the point where it now runs so quickly and efficiently that no player will perceive speed as being a real problem with this game. (when I reopened the old version of 2071 mod I was shocked at the slow turn times, whereas M:C now runs extremely quickly with a lot more game features. M:C/RaR now run already very efficiently, to the point that further efforts are likely to be wasted, with the results of a few less milliseconds never being perceived by anyone. Particularly in something like player setup, which runs only once per game rather than once per turn; so no one will care either way even if it took 500 entire milliseconds additionally to set up; running 1128 iterations will take so much less time that you can't even notice it.)

But, I know you elsewhere expressed an interest in improving the Civ4Col AI, and I think that would actually be an excellent goal. Since you have a lot of experience as a player and apparently also DLL coding skills, I think you could make a much bigger impact if you want to consider AI enhancements. Because vanilla AI was so poor and still needs improvement, instead of the few milliseconds you might gain through lots of hours spent on further speed rewrites, I think you could probably still make great progress by carefully considering how to improve AI planning/strategy/behavior. If this is done in a generalizable way (ie helping the AI to better plan strategy by comparing values found in Unit/Building/Terrain XML, rather than hidden DLL hardcoding specific to a single mod), it could have great benefits for all mods. What do you think?

 

[raystuttgart]

Improving AI (or generally coding good AI behaviour) is probably the most difficult aspect in DLL modding.
Functional coding of a feature - so it is working for human player - is usually much much simpler.

Especially if you dive into such important / elementary logic as economy or military strategy.

This is usually an area where only expert modders - that more or less know all important parts of the AI logic by heart - have ever achieved significant improvments.
(Impacts and side effects of more complicated changes are often very hard to predict even if you know the most important parts of AI logic already.)

1) Coding good AI logic for some independant feature
-> usually doable if you already have a some experience with AI
2) Coding good AI logic for adding new logic to a UnitAI (or to create a completely new UnitAI)
-> already moderatly difficult
3) significantly improving AI logic for PlayerAI on strategical level (economy / military)
-> very difficult

I have known only very few DLL modders that have ever achieved a good knowledge and really significant improvments considering core AI logic.
(koma13 was one of those)

Also, AI improvments are often extremely specific (and thus indeed often even hardcoded) to the mod, because of the different features.
AI improvments that have been created in TAC and RaR might even be harmful to another mod.
(Even in RaR, I have reverted a few TAC AI changes, because they caused problems.)

 

[Nightinggale]

Commander Bello, I have to agree that Nightingale and ray already invested a lot of efforts in profiling and optimizing the code for memory usage and speed, to the point where it now runs so quickly and efficiently that no player will perceive speed as being a real problem with this game.

Not entirely true. I still think RaR can be sluggish at times. However after the last time I improved RaR performance, I posted in the development thread that wait for next turn has now been reduced by 40% for all my additions combined and that after additional profiling I can't find anymore performance hotspot to fix. I don't expect to be able to improve performance anymore and I find it unlikely that anybody else will either, unless they are some performance gurus. Even then noteworthy improvements would be hard to achieve.

when I reopened the old version of 2071 mod I was shocked at the slow turn times

That is really important. I recall the timeline as follows:

• I state col2071 is a great mod, but it is killed by horrible performance
• orlanth states he want to fork the DLL from M:C or RaR because he has given up on his own DLL
• I propose to share the M:C DLL code for both mods
• orlanth and Kailric agrees
• The DLL is modified to allow col2071
• World History mod appears and exploits the col2071 DLL additions
• Colonization Mod Collection is born

Things would likely have looked a whole lot differently if Colonization 2071 had performed better. Performance is really important and as it turns out, horrible performance can be good

If this is done in a generalizable way (ie helping the AI to better plan strategy by comparing values found in Unit/Building/Terrain XML, rather than hidden DLL hardcoding specific to a single mod), it could have great benefits for all mods. What do you think?

That's far from trivial. I want to do something like that for yields in CMC. I have studied the source code and thought about it for many hours to plan this right and I have yet to write the first line of code. In fact I haven't even figured out what the first line should be. It's quite complex and even more complex to write generic code.

While it sounds simple, it quickly starts to get complicated. Right now the code assumes buildings to use YIELD_TOOLS and that is hardcoded into the DLL. Then we add a building, which needs YIELD_STONE. Getting the AI to produce stone for this task isn't trivial as it sounds. Next the we need to move the stone from production city into another city and... oh dear. We really need to plan this well or the AI will be really stupid.


Asking Commander Bello (or anybody else for that matter) to go make some generic AI improvements is seriously underestimating the task difficulty.


One AI improvement I would like to see in RaR is to prevent the AI from removing all forest. I saw an AI yesterday, which cut down every single forest within reach of its first colony. It actually did that quite fast too. It would be nice if it valued lumber a bit more than that.

Another AI stupidity committed by the same player. The second colony was inland and was more than 50 plots away and it had to go through 3 Europeans and a few native tribes to get to it. As I recall it never gained more than 2 colonies.

I don't know how to fix any of those issues, but human players would likely have done better.

 

[Commander Bello]

Following agnat86's observation about problems when hurrying I've found something which as I think could be worth being improved.

bool CvPlayer::canHurry(HurryTypes eHurry, int iIndex) const
{
	CvHurryInfo& kHurry = GC.getHurryInfo(eHurry);

	if (getHurryCount(eHurry) > 0)
	{
		if (kHurry.getGoldPerCross() > 0) 
		{
			if (canTradeWithEurope())
			{
				if (immigrationThreshold() > getCrossesStored())
				{
					if (getHurryGold(eHurry, iIndex) <= getGold())
					{
						return true;
					}
				}
			}
		}
		[COLOR="Red"]else
		{
			return true;
		}[/COLOR]
	}

	return false;
}

The red part should result in "false", as far as I see it.
Otherwise the whole statement doesn't make much sense and could be more easily expressed as

bool CvPlayer::canHurry(HurryTypes eHurry, int iIndex) const
{
	CvHurryInfo& kHurry = GC.getHurryInfo(eHurry);

	if (getHurryCount(eHurry) > 0)
	{
	return true;
	}

	return false;
}

Furthermore, I think the if (canTradeWithEurope()) should be the first check, as !canTradeWithEurope() would make anything else obsolete.

 

[Nightinggale]

Again this is code, which will not affect performance and should only be changed if it is to fix bugs.

The red part should result in "false", as far as I see it.

If the price is 0, then you can afford to buy all the needed crosses. I don't see the problem.

if (getHurryCount(eHurry) > 0)
	{
	return true;
	}

	return false;

That can be reduced to

return getHurryCount(eHurry) > 0;

I haven't looked closely, but I'm quite sure your proposed change will change game behavior. Personally I wouldn't touch this code in a cleanup at all.

 

[Commander Bello]

This is not RaRE related, so I put it in here.

Some functions which aren't used at all

int CvPlayerAI::AI_neededExplorers(CvArea* pArea)
(not that I would regard this function as being especially sophisticated, but this one might be worth to be improved and USED!)

int CvPlayerAI::AI_unitValuePercent(UnitTypes eUnit, UnitAITypes* peUnitAI, CvArea* pArea)
(not that I would regard this function as being especially sophisticated)

 

[Commander Bello]

In CvPlayerAI::AI_doEurope() we find

	if ((eBuyUnit != NO_UNIT) && ((iBuyUnitValue > iBuyProfessionValue)[B][COLOR="Red"] || (iUnitPrice < getGold()))[/COLOR][/B])
	{
		if (getGold() > iUnitPrice)
		{
			CvUnit* pUnit = buyEuropeUnit(eBuyUnit, 100);

			FAssert(pUnit != NULL);
			pUnit->AI_setUnitAIType(eBuyUnitAI);
			// TAC - AI Military Buildup - koma13 - START
			if (AI_isStrategy(STRATEGY_MILITARY_BUILDUP))
			{
				AI_clearStrategy(STRATEGY_MILITARY_BUILDUP);
			}
			// TAC - AI Military Buildup - koma13 - END

			AI_updateNextBuyUnit();
		}
	}

where the highlighted part seems to be a mistake as the necessary condition of having more money than the unit costs may override the original intention to check if iBuyUnitValue exceeds iBuyProfessionValue.

 

[Nightinggale]

Some functions which aren't used at all

int CvPlayerAI::AI_neededExplorers(CvArea* pArea)
int CvPlayerAI::AI_unitValuePercent(UnitTypes eUnit, UnitAITypes* peUnitAI, CvArea* pArea)

A quick check reveals that there are no calls to those functions in vanilla and that the lack of DLLexport mean they can't be called from the exe.

Theoretically there might be pointers to those functions which would allow them to be used in code, which do not specify the function names. However I can't think of a way to make such pointers without using the name. My guess is that the code was written, then redesigned and some of the obsolete code was left behind. This theory would be a warning that the code might not do as we expect it to do as there likely is a reason why it was abandoned.

I know some people are quick to delete unused functions like that as they slow down compilation and increase DLL filesize. Both are too minor for me to really care and I would keep them if there is at least a small chance that they could be useful at some point. It would make sense to add comments about them being unused.

Speaking of comments, it might make good sense to add comments to comply with doxygen, which would mean every single function should be commented on the purpose of the function and meaning of arguments and return value.