Interior City Confirmation

[OrionVeteran]

I'm attempting to convert a Python function to SDK. The function compiles OK, but the game hangs, like it's stuck in a never ending loop. Can someone examine this code and recommend any improvements?

Spoiler :

/************************************************************************************************/
/* Mine Warfare MOD                      Start                                OrionVeteran      */
/************************************************************************************************/
bool CvCity::isInteriorCity() const
{
	CvPlot* pCityPlot = plot();
	CvPlot* pLoopPlot;
	int iI, iDist;
	PlayerTypes eOwner = getOwnerINLINE();
	bool bFoundBorderCity = false;
	
	for (iI = 0; iI < GC.getMapINLINE().numPlotsINLINE(); iI++)
	{
		pLoopPlot = GC.getMapINLINE().plotByIndexINLINE(iI);
		
		iDist = GC.getMapINLINE().calculatePathDistance(pCityPlot, pLoopPlot);
		
		if (iDist < 6)
		{
			if (pLoopPlot != NULL)
			{
				if (!pLoopPlot->isCity())
				{
					if (!pLoopPlot->isImpassable())
					{
						//If pLoopPlot is unowned or is not owned by the City owner
						if (!(pLoopPlot->isOwned()) || (pLoopPlot->getOwnerINLINE() != eOwner))
						{
							bFoundBorderCity = true;
							break;
						}
					}
				}
			}
		}
	}
	if (!bFoundBorderCity)
	{
		return true;
	}
	return false;
}
/************************************************************************************************/
/* Mine Warfare MOD                      End                                  OrionVeteran      */
/************************************************************************************************/

 

[The_J]

I have no clue, but I'd check the value for iDist.
I mean, I don't know what calculatePathDistance will return if there's no path available.

 

[God-Emperor]

I don't see an infinite loop in the function. I do see a problem. It could lead to an infinite loop in the function that calls this function.

What does GC.getMapINLINE().calculatePathDistance(pCityPlot, pLoopPlot) return if there is no path to the plot? It returns -1. This is less than 6.

What will cause it to return -1? A variety of things. If it pLoopPlot is on different continent from the city that would do it. I think any water plot will as well, unless maybe the city on pCityPlot is on the coast of that body of water. It apparently only returns values that are not -1 if it can find a path that is all land or all water.

If there is a single 1 plot lake that is not inside your territory then all of your cities will always get a return value of False. It is a race between possible things that cause it to return -1 to see which one gets to make the function return False. If there is a single plot of land that is not impassible which you do not own that is on a different continent from the city and that happens to be plot 0 then the very first plot will make the function return false. Odds are good that on most full planet maps plot 0 is some water ice plot - that is likely to make the function return False immediately since you can't get there from most (or possibly all) cities.

What does the function that calls this function do if it can't find a city that returns True? That could be the location of an infinite loop.

The fix for the problem: Instead if just checking for "< 6", check for "< 6" and "> 0".
if ((iDist < 6) && (iDist > 0))

Also, if you happen to be testing this with a new just started game, you will not own every plot within 6 of your one city so it will always return False for that one and only city your own too.

Therefore you should also check what the calling function does. Even if the above fix fixes everything, which it probably won't since it does nothing for the only "have 1 city with borders closer than 6" case, there is still a potential infinite loop in there somewhere which should probably be dealt with.

Oh, you are also checking pLoopPlot to make sure it is not NULL after you have already used it to get the path distance. Normally you do the validation before you use the data... Fortunately the calculatePathDistance also checks it for validity - of course, it returns the infamous -1 if either argument is not valid.

 

[OrionVeteran]

I don't see an infinite loop in the function. I do see a problem. It could lead to an infinite loop in the function that calls this function.

What does GC.getMapINLINE().calculatePathDistance(pCityPlot, pLoopPlot) return if there is no path to the plot? It returns -1. This is less than 6.

What will cause it to return -1? A variety of things. If it pLoopPlot is on different continent from the city that would do it. I think any water plot will as well, unless maybe the city on pCityPlot is on the coast of that body of water. It apparently only returns values that are not -1 if it can find a path that is all land or all water.

If there is a single 1 plot lake that is not inside your territory then all of your cities will always get a return value of False. It is a race between possible things that cause it to return -1 to see which one gets to make the function return False. If there is a single plot of land that is not impassible which you do not own that is on a different continent from the city and that happens to be plot 0 then the very first plot will make the function return false. Odds are good that on most full planet maps plot 0 is some water ice plot - that is likely to make the function return False immediately since you can't get there from most (or possibly all) cities.

What does the function that calls this function do if it can't find a city that returns True? That could be the location of an infinite loop.

The fix for the problem: Instead if just checking for "< 6", check for "< 6" and "> 0".
if ((iDist < 6) && (iDist > 0))

Also, if you happen to be testing this with a new just started game, you will not own every plot within 6 of your one city so it will always return False for that one and only city your own too.

Therefore you should also check what the calling function does. Even if the above fix fixes everything, which it probably won't since it does nothing for the only "have 1 city with borders closer than 6" case, there is still a potential infinite loop in there somewhere which should probably be dealt with.

Oh, you are also checking pLoopPlot to make sure it is not NULL after you have already used it to get the path distance. Normally you do the validation before you use the data... Fortunately the calculatePathDistance also checks it for validity - of course, it returns the infamous -1 if either argument is not valid.

As always, your recommendations prove to put me on the right track. I implemented your suggestions for the Null check and added a min range for iDist instead of just a max range. Then, I ran several message box tests in each of the python functions that call this SDK function. Result: This SDK function ran successfully and flawlessly.

However, my tests revealed one of two other source functions for an infinite loop. The first one had to do with the deployment of the mine sweeper. It literally had no place to go. With a one line change, the sweeper is now deploying under the condition, which it failed to do so before. It should not be long before more testing reveals the second freeze up, which occurred some 50 turns latter. With each function, I move to SDK, the turn performance is improving, even on a huge map with over 200 cities and 12 players. I truly believe the SDK version of Mine Warfare is going to be a huge success.

I can't thank you enough.