national buildings disappear?

[davidlallen]

We have a problem in Dune Wars which I have duplicated as a bug in vanilla (BTS 3.19). Please find attached a standalone tiny mod to show the problem.

I have modified the Barracks building to be a national building, with limit 2. The game appears to enforce a limit of 1. Here is how you can see the problem.

1. Start a new game with this mod, and use WB to give yourself 3 cities.
2. Set one city to build a barracks and set the other cities to build something else, like a warrior.
3. I have adjusted the build cost of barracks to a tiny number so it will build in one turn. Click next turn and you will get a barracks.
4. Set the second city to build a barracks. You can do this; the game knows you are allowed two barracks.
5. Try to set the third city to build a barracks. The game prevents you, because there is one built and one in your queue for another city. This is fine, it also shows the game knows about national limits.
6. Click next turn, and your second city will build the barracks.

7. Now for the surprise. Where did the barracks go, in your first city? It has been silently removed.

Something in the game misunderstands the national limit, and deletes your first barracks when the second is built. Can anybody help track this down?

 

[Afforess]

I can't help, but I can confirm this. This is a problem Rise of Mankind has too, so I would say that this is a problem with BTS in general.

If you manage to fix this, could you request it added to the unofficial patch?

 

[davidlallen]

I traced out the logic; it was not as hard to find as I thought. Here are the few related lines.

bool CvPlayer::isBuildingClassMaxedOut(BuildingClassTypes eIndex, int iExtra) const
	return ((getBuildingClassCount(eIndex) + iExtra) [COLOR="Red"]>= [/COLOR](GC.getBuildingClassInfo(eIndex).getMaxPlayerInstances() + GC.getBuildingClassInfo(eIndex).getExtraPlayerInstances()));

void CvCity::popOrder(int iNum, bool bFinish, bool bChoose) ...
	case ORDER_CONSTRUCT:
		if (GET_PLAYER(getOwnerINLINE()).isBuildingClassMaxedOut(((BuildingClassTypes)(GC.getBuildingInfo(eConstructBuilding).getBuildingClassType())), 1))
			GET_PLAYER(getOwnerINLINE()).removeBuildingClass((BuildingClassTypes)(GC.getBuildingInfo(eConstructBuilding).getBuildingClassType()));

Suppose MaxPlayerInstances is 2 and I have one already built. When popOrder is called, iBCMO(type,1) will return true because its test is ">=". So the removeBuildingClass line is triggered.

This is wrong, and this is the cause of the bug. Now, there are two ways to change it which may have some effect elsewhere.

1. Change the test in iBCMO to ">"
2. Change the call in popOrder to (type, 0)

Which seems safer?

 

[Afforess]

I'd just change the >= to >, but I'm not familiar with that area of the code... Just try it, compile, and run with it.

 

[EmperorFool]

Given the name of the function, >= makes sense. You've maxed out that building class when you have the maximum number allowed (or more to allow for errors). However, it's being used in that context as if it were named isBuildingClassOverLimit().

First, I would do a global search for that function name to see where else it's called. If nowhere else, either fix works but I'd probably change the caller to pass in 0. If you find it being called elsewhere, see how those callers treat the function.

Tip: Every function should behave as its name implies.​

 

[davidlallen]

I have changed popOrder to call iBCMO(type, 0) and the original problem is fixed. I did not check globally, but hopefully there are no side effects.

 

[primordial stew]

I can't help, but I can confirm this. This is a problem Rise of Mankind has too, so I would say that this is a problem with BTS in general.

It is present in Warlords too, and very likely vanilla.

 

[jdog5000]

Problem: With this change, if you build a new palace your old palace is not removed. You then have two.

Cause of problem: While iMaxPlayerInstances for a Palace is set to 1, it also sets iExtraPlayerInstances to 1. As you can see in the code snippet in the third post, isBuildingClassMaxedOut actually compares against iMaxPlayerInstances + iExtraPlayerInstances (2 for the Palace). So, for the old Palace to be deleted, we do need to pass in 1 for iExtra to isBuildingClassMaxedOut.

Analysis: In davidlallen's barracks example at the start of this thread, I believe the early removal of the old barracks could also be blamed on not setting iExtraPlayerInstances > 0. It appears that Firaxis' intended method for handling limited buildings was to place a hard cap on the number of buildings with iMaxPlayerInstances but allow you to start production of one more with iExtraPlayerInstances. The 1 passed to isBuildingClassMaxedOut appears to be a hack which works fine so long as iExtraPlayerInstances is also always 1.

Solution: Change the call in popOrder to (bolded part replaced the 1 or 0):

if (GET_PLAYER(getOwnerINLINE()).isBuildingClassMaxedOut(((BuildingClassTypes)(GC.getBuildingInfo(eConstructBuilding).getBuildingClassType())), [B]GC.getBuildingClassInfo((BuildingClassTypes)(GC.getBuildingInfo(eConstructBuilding).getBuildingClassType())).getExtraPlayerInstances()[/B]))

and, for most buildings limited by iMaxPlayerInstances, also set iExtraPlayerInstances > 0 (like say 1).