Unofficial Patch for 3.19

It sounds like I could reduce it to a single flag (HIDDEN), with an optional second flag to toggle the entire mod on and off.

RANK could be replaced by a test for all contacts.
 
Yes, a single SHOW_HIDDEN_ATTITUDES flag would be fine.

For BULL I'll need to hook into its way of checking runtime options for the overall enabled flag, so you can omit this for your mod. Anyone merging in your source code directly clearly wants it to show.
 
CvPlot::isVisibleEnemyDefender
CvPlot::getNumVisibleEnemyDefenders
CvPlot::getNumVisiblePotentialEnemyDefenders
CvPlot::isVisibleEnemyUnit

These functions say "pUnit->isAlwaysHostile(this)", but "this" plot is not a position of "pUnit", but a opponent unit.
Code:
bool CvPlot::isVisibleEnemyUnit(const CvUnit* pUnit) const
{
	return (plotCheck(PUF_isEnemy, pUnit->getOwnerINLINE(), [COLOR="Blue"]pUnit->isAlwaysHostile(this)[/COLOR], NO_PLAYER, NO_TEAM, PUF_isVisible, pUnit->getOwnerINLINE()) != NULL);
}

Therefore, Privateer(= pUnit) with a command "Sentry" can not find neutral unit within a city, even if Privateer is "always hostile".
Because,
  1. Privateer's "always hostile" ability is disabled in a city.
  2. "this" plot is a city.
  3. Then, Privateer assumes that it loses "always hostile" even if it doesn't stay in a city.
This is my approach for solution. New blockade action for naval units
How do you think?
 
While debugging a CTD in RoM with AND I ran into the following problem:

http://forums.civfanatics.com/showpost.php?p=8819748&postcount=285

These (and possibly other) mods added units with combat value and having NO_UNITCOMBAT (-1) combat type (or no CombatType tag in the xml). This causes a silent heap corruption in CvPlayerAI.cpp at

Code:
for (iI = 0; iI < GC.getNumUnitClassInfos(); iI++)
	{
		if (m_aiUnitClassWeights[iI] > 0)
		{
			UnitTypes eUnit = (UnitTypes)GC.getUnitClassInfo((UnitClassTypes)iI).getDefaultUnitIndex();
->			m_aiUnitCombatWeights[GC.getUnitInfo(eUnit).getUnitCombatType()] += m_aiUnitClassWeights[iI];
		}
	}

I recommend guarding this against a buffer under/overrun, for example with

Code:
	int ctype = 0;
	for (iI = 0; iI < GC.getNumUnitClassInfos(); iI++)
	{
		if (m_aiUnitClassWeights[iI] > 0)
		{
			UnitTypes eUnit = (UnitTypes)GC.getUnitClassInfo((UnitClassTypes)iI).getDefaultUnitIndex();
			ctype = GC.getUnitInfo(eUnit).getUnitCombatType();
			// FAssert(ctype >= 0 && ctype < GC.getNumUnitCombatInfos());
			if (ctype >= 0 && ctype < GC.getNumUnitCombatInfos())
				m_aiUnitCombatWeights[ctype] += m_aiUnitClassWeights[iI];
		}
	}

as it was creating load/save issues and random CTDs later which are quite hard to trace down. I understand that the bug was probably introduced by xml modders, who were not aware of the fact that careless use of NO_UNITCOMBAT may cause crashes, but as it has serious consequences silently, I think the dll should be protected against this kind of abuse. Probably uncomment the assertion if you want the xml modders warned.

The other point which concerns vanilla BTS is in CyGlobalContext.cpp, where

Code:
CvCommerceInfo* CyGlobalContext::getCommerceInfo(int i) const
{
	return (i>=0 && i<NUM_COMMERCE_TYPES) ? &GC.getCommerceInfo((CommerceTypes) i) : NULL;
}

is used to have a call on GC's zero length vector at start-up in these mods. It evaluates correctly in the end, but I would add

Code:
CvCommerceInfo* CyGlobalContext::getCommerceInfo(int i) const
{
	if (GC.getCommerceInfo().size() == 0) return NULL;
	return (i>=0 && i<NUM_COMMERCE_TYPES) ? &GC.getCommerceInfo((CommerceTypes) i) : NULL;
}

just to stop the debugger complaining. Probably another problem introduced by xml modding, but again, the dll should be aware that it may happen.
 
Version 1.30 changes:
- CvPlot::doFeature - Plot feature (Forest/jungle) appearance now scales by game speed
- CvPlot::doImprovement - Bonus appearance in mines now scales by game speed
Nuclear plants blowing up still doesn't.

CvCity::doMeltdown() changes:
Code:
[COLOR="Green"]/************************************************************************************************/
/* UNOFFICIAL_PATCH                                                               jdog5000      */
/*                                                                                              */
/* Gamespeed scaling                                                                            */
/************************************************************************************************/
/* original bts code[/COLOR]
[COLOR="DimGray"]			if (GC.getBuildingInfo((BuildingTypes)iI).getNukeExplosionRand() != 0)
			{
				if (GC.getGameINLINE().getSorenRandNum(GC.getBuildingInfo((BuildingTypes)iI).getNukeExplosionRand(), "Meltdown!!!") == 0)
				{[/COLOR]
[COLOR="Green"]*/[/COLOR]
			int iOdds = GC.getBuildingInfo((BuildingTypes)iI).getNukeExplosionRand();
			iOdds *= GC.getGameSpeedInfo(GC.getGameINLINE().getGameSpeedType()).getResearchPercent();
			iOdds /= 100;

			if( iOdds > 0 )
			{
				if( GC.getGameINLINE().getSorenRandNum(iOdds, "Meltdown!!!") == 0)
				{
[COLOR="Green"]/************************************************************************************************/
/* UNOFFICIAL_PATCH                        END                                                  */
/************************************************************************************************/[/COLOR]
though the chance is so low to begin with (1 in 2000) I doubt anyone will notice
 
I'm (finally) updating the UP in BULL to 1.4 and I noticed what may be a bug, but I'm not certain.

One change in CvCity is in popOrder() for buildings. The code calls CvPlayer::isBuildingClassMaxedOut(); if so it calls CvPlayer::removeBuildingClass(). The UP change is to pass CvBuildingClassInfo::getExtraPlayerInstances() instead of 1 for iExtra to isBBuildingClassMaxedOut(). My question isn't about that change, however.

In removeBuildingClass() the cities are scanned for the first one with at least 1 real matching building, but it removes all of them. Now, for vanilla that's fine since you can only have one of each type of building, but some mods allow multiples. Should this be changed to remove just one of the buildings since only one is being added elsewhere?

Here's the change if you think so:

Code:
void CvPlayer::removeBuildingClass(BuildingClassTypes eBuildingClass)
{
	CvCity* pLoopCity;
	BuildingTypes eBuilding;
	int iLoop;

	eBuilding = ((BuildingTypes)(GC.getCivilizationInfo(getCivilizationType()).getCivilizationBuildings(eBuildingClass)));

	if (eBuilding != NO_BUILDING)
	{
		for (pLoopCity = firstCity(&iLoop); pLoopCity != NULL; pLoopCity = nextCity(&iLoop))
		{
			if (pLoopCity->getNumRealBuilding(eBuilding) > 0)
			{
				pLoopCity->setNumRealBuilding(eBuilding, [B][COLOR="Red"]pLoopCity->getNumRealBuilding(eBuilding) - 1[/COLOR][/B]);
				break;
			}
		}
	}
}

Now, what is up with extra player instances? The Palace is 1 but all the national wonders are 0 (all of those have 1 max). What is this field used for? It seems that by passing in extra player instances to isBuildingClassMaxedOut() which adds extra and max for comparison, it's a wash.

Ah, does this control allowing a building with a max to be replaced by building it elsewhere? canConstruct() passes in the default 0 for iExtra which allows the Palace to be built in another city but not a national wonder. So extraPlayerInstances seems to say how many you can add to city queues once you've maxed out the class.
 
I submitted a fix for the canFoundCitiesOnWater() callback, but I see that the code is different from what I submitted. As it is now, if the checks above make the plot valid for a city, the plot being water will no longer make it invalid as it did in the original code. This will happen if the callback is disabled (as it is by default) or if it returns False.

The bug in the original code was that it ignored the result of the callback, but the fix in 1.4 actually makes water plots valid locations for cities by default. Is this intended?

Here's the start of my patch:

Code:
	if (pPlot->isWater())
	{
		bValid = false;
		...

Here is 1.4:

Code:
	if ([B][COLOR="Red"]!bValid &&[/COLOR][/B] pPlot->isWater())
	{
		...

And for comparison, here is the original code:

Code:
	[B]// lResult is 0 here[/B]
	if(GC.getUSE_CAN_FOUND_CITIES_ON_WATER_CALLBACK())
	{
		... [B]// and gets set to 1 if the callback says it's okay[/B]
	}

	if (lResult == 1)
	{
		bValid = true;
	}
	else
	{
		[B]// if the callback is disabled or returns False, cannot found on water[/B]
		if (pPlot->isWater())
		{
			return false;
		}
	}

So while the original code ignores isWater() if the callback returns True, it correctly blocks founding on water otherwise.

Given the checks above the code it probably won't come up in practice, but it might affect mods that allow founding on water or add more rules above this check.

Edit: Meh, I'm not really sure which way it should go. The three checks above are of the style "if it's not okay but if the terrain makes it okay, it's okay." As soon as one check says it's okay, the other checks are ignored. Leaving the !bValid check in as 1.4 has it matches that form. But 1.4 definitely differs from vanilla 3.19. That there's a definite bug in 3.19 makes it harder to pick a winner.
 
Minor nitpick: In the fix at the start of CvPlayer::canDoCivics() there's a check and early return.

Code:
	// Circumvents second crash bug in simultaneous turns MP games
	if( eCivic == NO_CIVIC )
	{
		return 1;
	}

The function returns bool, though, so I recommend putting true there instead of 1. Yes, it's the same, but I did say it was a nitpick. ;)

As long as I'm nit-picking, my name doesn't have a space in it. :mischief:
 
In CvPlayerAI::AI_civicValue() there's a fix that looks wrong given what's around it, but I am very unfamiliar with the AI code.

Code:
/************************************************************************************************/
/* UNOFFICIAL_PATCH                       10/21/09                                jdog5000      */
/*                                                                                              */
/* Bugfix                                                                                       */
/************************************************************************************************/
/* orginal bts code
		iValue += (getNumCities() * 6 * AI_getHealthWeight(isCivic(eCivic) ? -kCivic.getExtraHealth() : kCivic.getExtraHealth(), 1)) / 100;
*/
		iValue += (getNumCities() * 6 * AI_getHealthWeight(kCivic.getExtraHealth(), 1)) / 100;
/************************************************************************************************/
/* UNOFFICIAL_PATCH                        END                                                  */
/************************************************************************************************/

The change makes it so the value is always positive instead of being negative if the AI is already running the civic. Many of the surrounding checks keep the isCivic() check, swapping their value negative if so. Why is this one changed yet military police, war weariness, etc. are left as-is?

Edit: Never mind. Pays to complete the merge of a function before asking questions! I just saw that the next three checks that do this have been changed.
 
I'm pretty sure already I talked to someone (jdog?) about the canFoundCitiesOnWater() callback, don't remember where though so I have to rely on my brain's memory and re-reading the code..

!bValid should always be true because bValid is false by default and only 3 checks can turn it to true before the callback: isFound(), isFoundCoast() with isCoastalLand(), and isFoundFreshWater() with isFreshWater(). A Water tile can never be coastal land or with fresh water (both isCoastalLand() and isFreshWater() check if the plot is water and immediately return false), and if isFound() returned true for a water tile (<bFound>1</bFound> for that terrain in CIV4TerrainInfos.xml), then the callback is simply not needed anymore and can be skipped .. iirc.
Does that make sense?
 
Yes, that makes sense which is why I don't think this issue is black or white. I'm only thinking of some modder coming along and adding a 4th check, say "can found on any tile with ice or snow next to it", but they don't think to check if the plot is already valid or not.

Code:
for each adjacent tile
    if it has ice or snow
        bValid = true
        break

Suddenly ocean tiles next to icebergs or snowy land will become available for settling. Yes, it's an error on the part of the modder, but removing the bValid check has such a tiny price--one extra check to isWater() that usually returns false--that it seems worth it. The callback won't be called unnecessarily.

In cases like these I tend to prefer preventative measures (defensive coding).
 
Gamespeed scaling again: The current way of scaling works fine for all events that can happen multiple times per tile, for those that can only happen once (doImprovement: a mine can only once pop a resource, right?) things are a bit more complicated.

Code:
[COLOR="Green"]/************************************************************************************************/
/* UNOFFICIAL_PATCH                       10/22/09                                jdog5000      */
/*                                                                                              */
/* Gamespeed scaling                                                                            */
/************************************************************************************************/
/* original bts code[/COLOR]
					[COLOR="Silver"]if (GC.getImprovementInfo(getImprovementType()).getImprovementBonusDiscoverRand(iI) > 0)
					{
						if (GC.getGameINLINE().getSorenRandNum(GC.getImprovementInfo(getImprovementType()).getImprovementBonusDiscoverRand(iI), "Bonus Discovery") == 0)
						{[/COLOR]
[COLOR="Green"]*/[/COLOR]
					int iOdds = GC.getImprovementInfo(getImprovementType()).getImprovementBonusDiscoverRand(iI);
					int iResearchPercent = GC.getGameSpeedInfo(GC.getGameINLINE().getGameSpeedType()).getResearchPercent();
					[B]iOdds = static_cast<int>([U]math::round[/U](1.0 / (1.0 - (math[I]::[/I]pow((1.0 - (1.0 / iOdds)),(100.0 / iResearchPercent))))));[/B]

					if( iOdds > 0 )
					{
						if( GC.getGameINLINE().getSorenRandNum(iOdds, "Bonus Discovery") == 0)
						{
[COLOR="Green"]/************************************************************************************************/
/* UNOFFICIAL_PATCH                        END                                                  */
/************************************************************************************************/[/COLOR]

I imagine the error from not doing it this way is pretty small though, I don't know if single precision is enough here or how much more CPU this would cost, trading 2 integer operations for 5 floating point operations plus math::pow() and a few type casts .. but yeah, I thought I should still mention it.

Error is too small, forget it.
 
Floodplains handling suggestion from EF whichI think belongs here:
I bet the argument went something like this:

  1. Assume the choice by Firaxis to remove features was
    • Settling in forests and jungles gives an uncounterable +50% defense.
    • This is not fair/realistic/whatever.
    • Remove features when settling.
    • Problem solved.
  2. Flood plains don't provide a defense bonus.
  3. Therefore Firaxis didn't mean to remove them.
  4. It's a bug; fix it!
If the assumed logic of 1 is correct, then the change is actually a fix. If not, it's a gameplay change and doesn't belong in the UP. Since we cannot know if 1 is correct, I'd err on the side of caution and remove it from the UP.

I'm all for it being in PIG, however since that is what PIG is about: small gameplay changes designed to improve the game.

If you do leave it in, I recommend removing all features that add a defensive bonus rather than special-casing Flood Plains.
No (other feature that provides defence but forest and jungle) in vanilla, but it's always nice to consider the modding community when adding features, especially in this case where you'd need to exchange one line of code for another.

Code:
if (pPlot->getFeatureType() != NO_FEATURE && GC.getFeatureInfo(pPlot->getFeatureType()).getDefenseModifier() != 0)
 
Now, what is up with extra player instances? The Palace is 1 but all the national wonders are 0 (all of those have 1 max). What is this field used for? It seems that by passing in extra player instances to isBuildingClassMaxedOut() which adds extra and max for comparison, it's a wash.

Ah, does this control allowing a building with a max to be replaced by building it elsewhere? canConstruct() passes in the default 0 for iExtra which allows the Palace to be built in another city but not a national wonder. So extraPlayerInstances seems to say how many you can add to city queues once you've maxed out the class.

I think you've summarized extraPlayerInstances perfectly, that's how I understand it and it worked that way in testing. Definitely a bit confusing.

Your code for fixing the remove function looks perfect, will fold in for 1.5.
 
I submitted a fix for the canFoundCitiesOnWater() callback, but I see that the code is different from what I submitted. As it is now, if the checks above make the plot valid for a city, the plot being water will no longer make it invalid as it did in the original code. This will happen if the callback is disabled (as it is by default) or if it returns False.

The bug in the original code was that it ignored the result of the callback, but the fix in 1.4 actually makes water plots valid locations for cities by default. Is this intended?

Here's the start of my patch:

Code:
	if (pPlot->isWater())
	{
		bValid = false;
		...

Here is 1.4:

Code:
	if ([B][COLOR="Red"]!bValid &&[/COLOR][/B] pPlot->isWater())
	{
		...

And for comparison, here is the original code:

Code:
	[B]// lResult is 0 here[/B]
	if(GC.getUSE_CAN_FOUND_CITIES_ON_WATER_CALLBACK())
	{
		... [B]// and gets set to 1 if the callback says it's okay[/B]
	}

	if (lResult == 1)
	{
		bValid = true;
	}
	else
	{
		[B]// if the callback is disabled or returns False, cannot found on water[/B]
		if (pPlot->isWater())
		{
			return false;
		}
	}

So while the original code ignores isWater() if the callback returns True, it correctly blocks founding on water otherwise.

Given the checks above the code it probably won't come up in practice, but it might affect mods that allow founding on water or add more rules above this check.

Edit: Meh, I'm not really sure which way it should go. The three checks above are of the style "if it's not okay but if the terrain makes it okay, it's okay." As soon as one check says it's okay, the other checks are ignored. Leaving the !bValid check in as 1.4 has it matches that form. But 1.4 definitely differs from vanilla 3.19. That there's a definite bug in 3.19 makes it harder to pick a winner.

I see your point, and it's probably best to have consistent behavior here. If the mod make has turned on the canFoundCitiesOnWater callback, they probably want that used for all water tiles and might feel like they should set bFound in XML for ocean to make that possible. With that usage, your approach works better.

We can still get fast (python callbacks are slow) cities built in water by putting the bValid = false line inside the if( GC.getUSE_ ) check. Then, modders could use the bFound XML setting and not the python callback if settling anywhere in water is good for their mod.

How does that sound?

So the code would read:

Code:
	if (pPlot->isWater())
	{
		if(GC.getUSE_CAN_FOUND_CITIES_ON_WATER_CALLBACK())
		{
			bValid = false;

			CyArgsList argsList2;
			argsList2.add(iX);
			argsList2.add(iY);
			lResult=0;
			gDLL->getPythonIFace()->callFunction(PYGameModule, "canFoundCitiesOnWater", argsList2.makeFunctionArgs(), &lResult);

			if (lResult == 1)
			{
				bValid = true;
			}
		}
	}
 
Minor nitpick: In the fix at the start of CvPlayer::canDoCivics() there's a check and early return.

Code:
	// Circumvents second crash bug in simultaneous turns MP games
	if( eCivic == NO_CIVIC )
	{
		return 1;
	}

The function returns bool, though, so I recommend putting true there instead of 1. Yes, it's the same, but I did say it was a nitpick. ;)

As long as I'm nit-picking, my name doesn't have a space in it. :mischief:

Fixed and fixed :)
 
Jdog, I found something that doesn't scale for gamespeed; memory decays. See here in CvPlayerAI:

Code:
	for (iI = 0; iI < MAX_PLAYERS; iI++)
	{
		if (GET_PLAYER((PlayerTypes)iI).isAlive())
		{
			for (iJ = 0; iJ < NUM_MEMORY_TYPES; iJ++)
			{
				if (AI_getMemoryCount(((PlayerTypes)iI), ((MemoryTypes)iJ)) > 0)
				{
					if (GC.getLeaderHeadInfo(getPersonalityType()).getMemoryDecayRand(iJ) > 0)
					{
						if (GC.getGameINLINE().getSorenRandNum(GC.getLeaderHeadInfo(getPersonalityType()).getMemoryDecayRand(iJ), "Memory Decay") == 0)
						{
							AI_changeMemoryCount(((PlayerTypes)iI), ((MemoryTypes)iJ), -1);
						}
					}
				}
			}
		}
	}

It should really scale, so Marathon leaders are less forgetful.
 
Gamespeed scaling again: The current way of scaling works fine for all events that can happen multiple times per tile, for those that can only happen once (doImprovement: a mine can only once pop a resource, right?) things are a bit more complicated.

Code:
[COLOR="Green"]/************************************************************************************************/
/* UNOFFICIAL_PATCH                       10/22/09                                jdog5000      */
/*                                                                                              */
/* Gamespeed scaling                                                                            */
/************************************************************************************************/
/* original bts code[/COLOR]
					[COLOR="Silver"]if (GC.getImprovementInfo(getImprovementType()).getImprovementBonusDiscoverRand(iI) > 0)
					{
						if (GC.getGameINLINE().getSorenRandNum(GC.getImprovementInfo(getImprovementType()).getImprovementBonusDiscoverRand(iI), "Bonus Discovery") == 0)
						{[/COLOR]
[COLOR="Green"]*/[/COLOR]
					int iOdds = GC.getImprovementInfo(getImprovementType()).getImprovementBonusDiscoverRand(iI);
					int iResearchPercent = GC.getGameSpeedInfo(GC.getGameINLINE().getGameSpeedType()).getResearchPercent();
					[B]iOdds = static_cast<int>(1.0 / (1.0 - (math[I]::[/I]pow((1.0 - (1.0 / iOdds)),(100.0 / iResearchPercent)))));[/B]

					if( iOdds > 0 )
					{
						if( GC.getGameINLINE().getSorenRandNum(iOdds, "Bonus Discovery") == 0)
						{
[COLOR="Green"]/************************************************************************************************/
/* UNOFFICIAL_PATCH                        END                                                  */
/************************************************************************************************/[/COLOR]

I imagine the error from not doing it this way is pretty small though, I don't know if single precision is enough here or how much more CPU this would cost, trading 2 integer operations for 5 floating point operations plus math::pow() and a few type casts .. but yeah, I thought I should still mention it.

Yeah, you're right ... this snippet gets called a lot though, I'm just not sure it's worth it. We can make the original code (and your code if you use it) faster by moving the multiplies inside the iOdds > 0 check though.
 
Floodplains handling suggestion from EF whichI think belongs here:

Thanks, looks like a good idea. The one issue is that fallout won't be cleared in this case if a city is founded on it ... could easily be fixed with an xml change so that cities can't be founded on fallout, which I think makes good sense.

Thoughts?
 
I think it's going to take having a modder tweak this function to see what really makes sense. If someone does something that makes all tiles adjacent to ice foundable, they'll figure it out soon enough. ;)
 
Top Bottom