Unofficial Patch for 3.19

I found two bugs in Python/pyHelper/Popup.py:

1. Typo: X used instead of iX.

Code: 
	def addPythonButtonXY( self, strFunctionName, strButtonText, strHelpText, strArtPointer = "Art\Interface\Popups\PopupRadioButton.kfm", iData1 = -1, iData2 = -1, bOption = True, iX = -1, iY = -1 ):
		"adds a python button at XY"
		self.popup.addPythonButtonXY( strFunctionName, strButtonText, strHelpText, strArtPointer, iData1, iData2, bOption, [B][COLOR="Red"]X[/COLOR][/B], iY )

should be

Code: 
	def addPythonButtonXY( self, strFunctionName, strButtonText, strHelpText, strArtPointer = "Art\Interface\Popups\PopupRadioButton.kfm", iData1 = -1, iData2 = -1, bOption = True, iX = -1, iY = -1 ):
		"adds a python button at XY"
		self.popup.addPythonButtonXY( strFunctionName, strButtonText, strHelpText, strArtPointer, iData1, iData2, bOption, [B][COLOR="Red"]iX[/COLOR][/B], iY )

2. Unset variable "loopTitle" accessed in addTitleData().

I have no idea how to fix it since it isn't called by any vanilla code. The comments don't match the code, and I see no parameter that could be holding the image's extra size information. That also makes it pretty darn unimportant in my book. ;)
 
Version 1.5 has been released! You can download it here at CFC or check out the first post for more info and options. There will also be a discussion thread.

There are 24 fixes and tweaks in this version, special thanks to NotSoGood, EmperorFool, LunarMongoose, Sephi, Afforess, denev, jsbrigo, and God-Emperor for their bug hunting!
 
Can you please tag the 1.5 release in the repository?

Edit: I'm merging the trunk into BULL now and wasn't sure if there were any changes since the release, but I prefer to go off of tags as it's easier to track diffs and put it into the changelog.
 
There is a fix in CvPlayerAI.cpp to make decisions based on culture strategy also consider high culture slider values. There are two if() cases--one with a city and one without--and each has three if() cases based on the strategy or culture slider level, but the case with a city doesn't add the slider detection to the second culture level check.

Here is my proposed change which copies the code from the NULL city branch on line 1585 to line 1561:

Code: 
else if (AI_isDoStrategy(AI_STRATEGY_CULTURE2) [B][COLOR="Red"]|| getCommercePercent(COMMERCE_CULTURE) > 70[/COLOR][/B])

Also the culture percentages are different between the two cases (80/nothing/50 vs. 90/70/50). Is that intentional?
 
In CvTeam::doTurn(), line 891 of CvTeam.cpp, there is a fix to check isBarbarian(). However the loop is over MAX_CIV_TEAMS. Doesn't this enum exclude barbarian civs making the fix unnecessary?
 
EmperorFool said:
Can you please tag the 1.5 release in the repository?
Click to expand...

Done.

EmperorFool said:
There is a fix in CvPlayerAI.cpp to make decisions based on culture strategy also consider high culture slider values. There are two if() cases--one with a city and one without--and each has three if() cases based on the strategy or culture slider level, but the case with a city doesn't add the slider detection to the second culture level check.

Here is my proposed change which copies the code from the NULL city branch on line 1585 to line 1561:

Code: 
else if (AI_isDoStrategy(AI_STRATEGY_CULTURE2) [B][COLOR="Red"]|| getCommercePercent(COMMERCE_CULTURE) > 70[/COLOR][/B])

Also the culture percentages are different between the two cases (80/nothing/50 vs. 90/70/50). Is that intentional?
Click to expand...

Good catch. Perhaps 80/60/40 for all, since it's a > check this means setting the culture slider at 50 qualifies, that seems like a good starting point for special culture treatment.

EmperorFool said:
In CvTeam::doTurn(), line 891 of CvTeam.cpp, there is a fix to check isBarbarian(). However the loop is over MAX_CIV_TEAMS. Doesn't this enum exclude barbarian civs making the fix unnecessary?
Click to expand...

Yes, you are correct.
 
Thanks.

jdog5000 said:
Perhaps 80/60/40 for all, since it's a > check this means setting the culture slider at 50 qualifies, that seems like a good starting point for special culture treatment.
Click to expand...

Some mods allow setting the slider in 5% increments, but I don't know if that affects AIs or not. Regardless, use >= 50 if you want 50 to be included but not 45 or > 40 if you want 45 to be included. I typically prefer to use >= when specifying a cutoff to make it clear at exactly what point the effect should kick in.

By "all" are you saying to use the same values for the city vs. non-city case, or are you talking about the non-city case specifically?
 
Better BTS AI includes the Unofficial Patch, so it's your choice...
 
CvPlayerAI.cpp line 4116
Code: 
	if (kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) > 0)
	{
		bIsCultureBuilding = true;
	}
I believe this should be
Code: 
	if (kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) > 0[COLOR="RoyalBlue"] || kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE) > 0[/COLOR])
	{
		bIsCultureBuilding = true;
	}

Current BtS doesn't consider a monument as a cultural building.
 
There more ways a building could provide extra culture:
  • CommerceModifiers
  • GlobalCommerceModifiers
  • SpecialistExtraCommerces
  • StateReligionCommerces
  • FreeSpecialistCount (when the specialist produces culture)
  • FreeBuilding (when the civilizationBuilding of this buildingClass is a culturebuilding)
Not sure what CommerceFlexibles are supposed to be as that is not used in stock BTS.
Anyway you are right, just checking for commerceChange is a bit too simple there.

There are also some late wonders that don't change culture production, so those would be meaningless for cultural victories.
Code: 
									if (AI_isDoVictoryStrategy(AI_VICTORY_CULTURE1) [COLOR="Blue"]&& bIsCultureBuilding[/COLOR])
									{
										iValue += 400;
									}
 
Good point denev and Fuyu ... I think given what the code seems to be doing here it should read:

Code: 
	if (!isLimitedWonderClass((BuildingClassTypes)iJ))
	{
		if (kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) > 0 || kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE) > 0)
		{

There are two places this is needed in that function.

Then, if running for culture victory:

Code: 
if (AI_isDoVictoryStrategy(AI_VICTORY_CULTURE1))
{
	if (kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) > 2 || 
		kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE) > 2 ||
		kLoopBuilding.getCommerceModifier(COMMERCE_CULTURE) > 10)
	{
		iValue += 400;
	}
}

How does that look?
 
Looks good.
Whereever it says
Code: 
(kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) > 0)
it should say
Code: 
(kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) > 0 || kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE) > 0)

Just one more place, replacing "kLoopBuilding.getCommerceChange(COMMERCE_CULTURE)" with "(kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) + kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE))"
Code: 
                if (AI_isDoVictoryStrategy(AI_VICTORY_CULTURE2))
                {
                    int iMultiplier = (isLimitedWonderClass((BuildingClassTypes)iJ) ? 1 : 3);
                    iBuildingValue += (150 * [COLOR="Blue"][B]([/B][/COLOR]kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) [COLOR="Blue"][B]+ kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE))[/B][/COLOR]) * iMultiplier;
                    iBuildingValue += kLoopBuilding.getCommerceModifier(COMMERCE_CULTURE) * 4 * iMultiplier ;
                }

In CvPlayerAI::AI_cultureVictoryTechValue() there is a similar line
Code: 
				iValue += (150 * [COLOR="Blue"][B]([/B][/COLOR]kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) [COLOR="Blue"][B]+ kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE))[/B][/COLOR]) * 20;

---

You are catching all wonders of the base game at least, because both Eiffel Tower and Sistine Chapel produce base culture too.
I'd use >= as comparison operator since you aren't comparing to anything special but only arbitrary numbers. At least 3 culture, or at least 11%, is that what you mean? 3 is good but I'd include 10% too.
Code: 
if (AI_isDoVictoryStrategy(AI_VICTORY_CULTURE1))
{
	if (kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) >[COLOR="Blue"]= 3[/COLOR] || 
		kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE) >[COLOR="Blue"]= 3[/COLOR] ||
		kLoopBuilding.getCommerceModifier(COMMERCE_CULTURE) >[COLOR="Blue"]=[/COLOR] 10)
	{
		iValue += 400;
	}
}
 
Looks good Fuyu, thanks.

One other question ... does that line you pointed out in CvPlayerAI::AI_cultureVictoryTechValue look bugged to you?

It's original form was:

Code: 
				iValue += (150 * kLoopBuilding.getCommerceChange(COMMERCE_CULTURE)) * 20;

Which is ludicrously out of proportion with the surrounding values ... a building granting 1 culture/turn give a value of +3000! Compared to a building boosting culture by 50% giving +100. That's nuts.

I'm thinking it was supposed to be / 20 at the end there.
 
Well it's either *3000 or *7,5.

Comparing
Code: 
iValue += (150 * kLoopBuilding.getCommerceChange(COMMERCE_CULTURE)) * 20;
iValue += kLoopBuilding.getCommerceModifier(COMMERCE_CULTURE) * [B]2[/B];
to
Code: 
if (AI_isDoVictoryStrategy(AI_VICTORY_CULTURE2))
{
   int iMultiplier = (isLimitedWonderClass((BuildingClassTypes)iJ) ? 1 : 3);
   iBuildingValue += (150 * (kLoopBuilding.getCommerceChange(COMMERCE_CULTURE) + kLoopBuilding.getObsoleteSafeCommerceChange(COMMERCE_CULTURE))) * iMultiplier;
   iBuildingValue += kLoopBuilding.getCommerceModifier(COMMERCE_CULTURE) * [B]4[/B] * iMultiplier ;
}
I can only assume they meant *75 because in the other case it's
150 * x * iMultiplier - for commerceChange, and
x * 4 * iMultiplier - for commerceModifier

and in this case it's
x * 2 - for commerceModifier

I have no idea how a /2 became a *20 though.

edit: The only idea that comes to mind is that the 150* part was never meant to be there, just the *20 one. At least if I had to select between the multipliers 7,5 , 20 , 75 and 3000, 20 would make even more sense than 75 since in that function it's specifically for cultural victory, where commerceModifiers should have a lot more weight.

edit2: You're probably right about the /20. Cities that go legendary need a lot of base culture, that easily makes the commerceModifiers worth 10 times more than normally.
 
I looked into the code and it seems you are correct.

  1. Add two Warriors to the queue.
  2. Put at least 1:hammers: into the first.
  3. Insert anything else into the queue before the Warriors, e.g. Barracks, Warrior, Warrior.
  4. Let the Barracks complete (must take more than ten turns on Normal; if not insert something else).
  5. With BULL, hover over the Warrior and you'll see that it will decay if you don't put :hammers: into it this turn.
  6. Let the first Warrior complete. Make sure the second Warrior is immediately after it in the queue.
  7. With BULL, hover over the second Warrior (now first in queue) and you'll see that it is still subject to decay. The timer has not been reset.
Warning: The above scenario is based on my reading of the code. I'll set up a test to verify the behavior some time this weekend and report back. I didn't see any other place setting the counter to zero, so I'm pretty confident the bug exists. Good catch, jesusin!

Each turn decay is applied after production. But doDecay() will only clear a unit's decay timer if it isn't first in the queue.

Code: 
for each unit:
    if unit is not first in queue:
        if unit has at least 1 hammer:
            increment counter
            if counter > game-speed-limit
                decay
        else:
            reset counter to zero

As you can see the idea is that decay should only affect items not first in the queue, but the counter should be reset when a unit is completed. The fixes go into popOrder() and are trivial.

Code: 
void CvCity::popOrder(int iNum, bool bFinish, bool bChoose)
{
	...
	
	case ORDER_TRAIN:
		eTrainUnit = ((UnitTypes)(pOrderNode->m_data.iData1));
		...
		if (bFinish)
		{
			...
			setUnitProduction(eTrainUnit, 0);
			[B][COLOR="Red"]setUnitProductionTime(eTrainUnit, 0);[/COLOR][/B]

	...
	
	case ORDER_CONSTRUCT:
		eConstructBuilding = ((BuildingTypes)(pOrderNode->m_data.iData1));
		...
		if (bFinish)
		{
			...
			setBuildingProduction(eConstructBuilding, 0);
			[B][COLOR="Red"]setBuildingProductionTime(eConstructBuilding, 0);[/COLOR][/B]
 
I noticed that once I had built the six Hospitals that the Red Cross requires I still couldn't build it in one of my cities. Hovering over the Red Cross gave no indication that it couldn't be built. However, hovering over the Hospital showed that it was a prereq of the Red Cross.

The fix is easy. The code checks if the building has a minimum number of other buildings that must be built by the player (e.g. six Hospitals), and if it does the general prereq building class check is skipped. Just remove the "else" and you're done.

Code: 
void CvGameTextMgr::buildBuildingRequiresString(CvWStringBuffer& szBuffer, BuildingTypes eBuilding, bool bCivilopediaText, bool bTechChooserText, const CvCity* pCity)
{
	...

	if (NULL == pCity || !pCity->canConstruct(eBuilding))
	{
		...

		for (int iI = 0; iI < GC.getNumBuildingClassInfos(); ++iI)
		{
			if (ePlayer == NO_PLAYER && kBuilding.getPrereqNumOfBuildingClass((BuildingClassTypes)iI) > 0)
			{
				...
			}
			else if (ePlayer != NO_PLAYER && GET_PLAYER(ePlayer).getBuildingClassPrereqBuilding(eBuilding, ((BuildingClassTypes)iI)) > 0)
			{
				...
			}
// BUG - Unofficial Patch - start
			[s][B][COLOR="Red"]else[/COLOR][/B][/s] if (kBuilding.isBuildingClassNeededInCity(iI))
// BUG - Unofficial Patch - end
			{
				...
			}
		}

		...
	}
}

Edit: Naw, that doesn't look very nice. We want to hide the second requirement line if we've already added the x/y requirement. The following does just that:

Code: 
		for (int iI = 0; iI < GC.getNumBuildingClassInfos(); ++iI)
		{
// BUG - Unofficial Patch - start
			[B][COLOR="Red"]// EF: show "Requires Hospital" if "Requires Hospital (x/5)" requirement has been met[/COLOR][/B]
			[B][COLOR="Red"]bool bShowedPrereq = false;[/COLOR][/B]

			if (ePlayer == NO_PLAYER && kBuilding.getPrereqNumOfBuildingClass((BuildingClassTypes)iI) > 0)
			{
				eLoopBuilding = (BuildingTypes)GC.getBuildingClassInfo((BuildingClassTypes)iI).getDefaultBuildingIndex();
				szTempBuffer.Format(L"%s%s", NEWLINE, gDLL->getText("TXT_KEY_BUILDING_REQUIRES_NUM_SPECIAL_BUILDINGS_NO_CITY", GC.getBuildingInfo(eLoopBuilding).getTextKeyWide(), kBuilding.getPrereqNumOfBuildingClass((BuildingClassTypes)iI)).c_str());

				szBuffer.append(szTempBuffer);
				[B][COLOR="Red"]bShowedPrereq = true;[/COLOR][/B]
			}
			else if (ePlayer != NO_PLAYER && GET_PLAYER(ePlayer).getBuildingClassPrereqBuilding(eBuilding, ((BuildingClassTypes)iI)) > 0)
			{
				if ((pCity == NULL) || (GET_PLAYER(ePlayer).getBuildingClassCount((BuildingClassTypes)iI) < GET_PLAYER(ePlayer).getBuildingClassPrereqBuilding(eBuilding, ((BuildingClassTypes)iI))))
				{
					eLoopBuilding = ((BuildingTypes)(GC.getCivilizationInfo(GET_PLAYER(ePlayer).getCivilizationType()).getCivilizationBuildings(iI)));

					if (eLoopBuilding != NO_BUILDING)
					{
						if (pCity != NULL)
						{
							szTempBuffer.Format(L"%s%s", NEWLINE, gDLL->getText("TXT_KEY_BUILDING_REQUIRES_NUM_SPECIAL_BUILDINGS", GC.getBuildingInfo(eLoopBuilding).getTextKeyWide(), GET_PLAYER(ePlayer).getBuildingClassCount((BuildingClassTypes)iI), GET_PLAYER(ePlayer).getBuildingClassPrereqBuilding(eBuilding, ((BuildingClassTypes)iI))).c_str());
						}
						else
						{
							szTempBuffer.Format(L"%s%s", NEWLINE, gDLL->getText("TXT_KEY_BUILDING_REQUIRES_NUM_SPECIAL_BUILDINGS_NO_CITY", GC.getBuildingInfo(eLoopBuilding).getTextKeyWide(), GET_PLAYER(ePlayer).getBuildingClassPrereqBuilding(eBuilding, ((BuildingClassTypes)iI))).c_str());
						}

						szBuffer.append(szTempBuffer);
						[B][COLOR="Red"]bShowedPrereq = true;[/COLOR][/B]
					}
				}
			}
			[s][B][COLOR="Red"]else[/COLOR][/B][/s] if ([B][COLOR="Red"]!bShowedPrereq && [/COLOR][/B]kBuilding.isBuildingClassNeededInCity(iI))
// BUG - Unofficial Patch - end

Edit2: Related to the above, here are two XML <Text> fixes so that the "Requires Hospital (x/y)" lines make the building into a link in the Civilopedia:

Code: 
	<!-- General BTS Fixes -->
	
	<TEXT>
		<Tag>TXT_KEY_BUILDING_REQUIRES_NUM_SPECIAL_BUILDINGS</Tag>
		<!-- %s1_Name = Can be Buildings, Units, etc -->
		<English>[COLOR_WARNING_TEXT]Requires [B][COLOR="Red"][LINK=literal][/COLOR][/B]%s1_Name[B][COLOR="Red"][\LINK][/COLOR][/B] (%d2_NumCurr/%d3_NumTotal Total)[COLOR_REVERT]</English>
		<French>[COLOR_WARNING_TEXT]Requiert : [LINK=literal]%s1_Name[\LINK] (%d2_NumCurr/%d3_NumTotal au total)[COLOR_REVERT]</French>
		<German>[COLOR_WARNING_TEXT]Erfordert: [LINK=literal]%s1_Name[\LINK] (%d2_NumCurr/%d3_NumTotal gesamt)[COLOR_REVERT]</German>
		<Italian>[COLOR_WARNING_TEXT]Richiede [LINK=literal]%s1_Name[\LINK] (%d2_NumCurr/%d3_NumTotal totali)[COLOR_REVERT]</Italian>
		<Spanish>[COLOR_WARNING_TEXT]Requiere [LINK=literal]%s1_Name[\LINK] (%d2_NumCurr de un total de %d3_NumTotal)[COLOR_REVERT]</Spanish>
	</TEXT>
	<TEXT>
		<Tag>TXT_KEY_BUILDING_REQUIRES_NUM_SPECIAL_BUILDINGS_NO_CITY</Tag>
		<!-- %s1_Name = Can be Buildings, Units, etc -->
		<English>[COLOR_WARNING_TEXT]Requires [LINK=literal]%s1_Name[\LINK] (%d2_NumTotal Total)[COLOR_REVERT]</English>
		<French>[COLOR_WARNING_TEXT]Requiert : [LINK=literal]%s1_Name[\LINK] (%d2_NumTotal au total)[COLOR_REVERT]</French>
		<German>[COLOR_WARNING_TEXT]Erfordert: [LINK=literal]%s1_Name[\LINK] (%d2_NumTotal gesamt)[COLOR_REVERT]</German>
		<Italian>[COLOR_WARNING_TEXT]Richiede [LINK=literal]%s1_Name[\LINK] (%d2_NumTotal totali)[COLOR_REVERT]</Italian>
		<Spanish>[COLOR_WARNING_TEXT]Requiere [LINK=literal]%s1_Name[\LINK] (%d2_NumTotal en total)[COLOR_REVERT]</Spanish>
	</TEXT>
 
You must log in or register to reply here.

Similar threads

Lionic
[SCENARIO] The Cold War (TCW) Unofficial update by Lionic
2 3
Replies
52
Views
3K
Delta_Strife
Delta_Strife
B
Problem with the combat log in another language.
Replies
2
Views
844
BristonXII
B
DynamicSpirit
  • Sticky
[BTS] News: BOTM 271 - Babylon, Deity! Starts 5 April
Replies
3
Views
551
antimony
A
kcd_swede
  • Sticky
[BTS] News: BOTM 270 - Charlemagne, Monarch - starts March 15, 2024
Replies
10
Views
940
lymond
lymond
DynamicSpirit
  • Sticky
[BTS] News: BOTM 269 - Julius Caesar, Immortal - Starts 1 March
Replies
8
Views
994
Qgqqqqq
Q
Top Bottom