Fixing broken Asserts

[EmperorFool]

You eliminated the "bool hideAssert" argument.

Oh, that was unintentional. I modified my copy of CvGlobals.cpp which doesn't include RevDCM. My point was only to have Afforess add the parts I bolded in my posting.

 

[Afforess]

But the idea is to search the entire XML folder structure for the RELIGION_HELLENISTIC string.

You mean RELIGION_HELLENISM, right?

Anyways, this time I searched for <Type>RELIGION_HELLENISM</Type> and only got one hit, the religioninfos file, where it's supposed to be. I searched my entire Assets folder.

 

[Afforess]

Okay, let's come back to that Assert Later. A duplicate type isn't that high on the list of errors one can have.

What about this error?

Assert Failed

File: c:\program files (x86)\firaxis games\sid meier's civilization 4\beyond the sword\cvgamecoredll\CvInfos.h
Line: 93
Expression: false
Message: Override this

I get this when loading saves or starting a new map. WoC added this assert in their code.

The code is here:

	void copyNonDefaults(CvInfoBase* pClassInfo = NULL );
	virtual void copyNonDefaultsReadPass2(CvInfoBase* pClassInfo = NULL ){ pClassInfo; FAssertMsg(false, "Override this"); }

 

[EmperorFool]

CvInfoBase::copyNonDefaultsReadPass2() is a function defined in the base class to require any subclass that allows it to be called to override it--provide its own definition. Sometimes a base class function is designed to be extended--called by any overriding function to augment its behavior. In this case, it is designed so that it must be replaced.

This is done when you want to provide a hook for specialization that subclasses can use only when they need it. If they don't need it, they must make sure the function isn't called.

What this means to me is that you have an info type that requires a 2nd read pass but it hasn't overridden the above function, and it must. Find all info types that ask for a 2nd read pass--those that pass true for bTwoPass in CvXMLLoadUtilitySet.cpp.

 

[Afforess]

Let's see if I understand. This part of the CvXMLLoadUtilitySet.cpp is what you are talking about?:

	LoadGlobalClassInfo(GC.getTechInfo(), "CIV4TechInfos", "Technologies", "Civ4TechInfos/TechInfos/TechInfo", true, &CvDLLUtilityIFaceBase::createTechInfoCacheObject);

And that true means that it has a readpass2?

Or am I way off...

Anyways, I downloaded the latest pure RevDCM sources and compiled a Debug DLL from them and their sources have this Assert too.

 

[EmperorFool]

Yes, that's the section I'm talking about, and that "true" means that Techs require a second read pass. For each one that has true, make sure that it has a copyNonDefaultsReadPass2() function for the object in CvInfos.cpp.

When you find one that doesn't have a function, define an empty function for it, for example:

void CvTechInfo::copyNonDefaultsReadPass2(CvTechInfo* pClassInfo)
{
    // nothing to copy
}

This will fix the assert, but the assert is an indication that there is something wrong. Why would an object type require a second read pass if nothing read in the second pass is going to be copied over the original values? At that point you need to do some investigation to see what is read in readpass2() and see if it's needed.

 

[Afforess]

That particular assert has vanished with RevDCM 2.6. I guess that's the price I pay for using the SDK of someone else's work. My codebase changes like the shifting sands of a desert...

However, I have a repeatable CTD with a final release DLL, but just get two asserts that I can ignore with a debug dll, and get past the problem.

The first assert is:

Assert Failed

File: CvCity.cpp
Line: 1430
Expression: iCount == getBaseYieldRate((YieldTypes)iI)
Message:

And the relevant code section:

void CvCity::doTurn()
{...
#ifdef _DEBUG
	{
		CvPlot* pPlot;
		int iCount;
		int iI, iJ;

		for (iI = 0; iI < NUM_YIELD_TYPES; iI++)
		{
			FAssert(getBaseYieldRate((YieldTypes)iI) >= 0);
			FAssert(getYieldRate((YieldTypes)iI) >= 0);

			iCount = 0;

/*************************************************************************************************/
/** JOOYO_ADDON, Added by Jooyo, 06/17/09                                                        */
/**                                                                                              */
/**                                                                                              */
/*************************************************************************************************/
			for (iJ = 0; iJ < getNumCityPlots(); iJ++)
/*************************************************************************************************/
/** JOOYO_ADDON                          END                                                     */
/*************************************************************************************************/
			{
				if (isWorkingPlot(iJ))
				{
					pPlot = getCityIndexPlot(iJ);

					if (pPlot != NULL)
					{
						iCount += pPlot->getYield((YieldTypes)iI);
					}
				}
			}

			for (iJ = 0; iJ < GC.getNumSpecialistInfos(); iJ++)
			{
				iCount += (GET_PLAYER(getOwnerINLINE()).specialistYield(((SpecialistTypes)iJ), ((YieldTypes)iI)) * (getSpecialistCount((SpecialistTypes)iJ) + getFreeSpecialistCount((SpecialistTypes)iJ)));
			}

			for (iJ = 0; iJ < GC.getNumBuildingInfos(); iJ++)
			{
				iCount += getNumActiveBuilding((BuildingTypes)iJ) * (GC.getBuildingInfo((BuildingTypes) iJ).getYieldChange(iI) + getBuildingYieldChange((BuildingClassTypes)GC.getBuildingInfo((BuildingTypes) iJ).getBuildingClassType(), (YieldTypes)iI));
			}

			iCount += getTradeYield((YieldTypes)iI);
			iCount += getCorporationYield((YieldTypes)iI);

			[B]FAssert(iCount == getBaseYieldRate((YieldTypes)iI));[/B]
		}

		for (iI = 0; iI < NUM_COMMERCE_TYPES; iI++)
		{
			FAssert(getBuildingCommerce((CommerceTypes)iI) >= 0);
			FAssert(getSpecialistCommerce((CommerceTypes)iI) >= 0);
			FAssert(getReligionCommerce((CommerceTypes)iI) >= 0);
			FAssert(getCorporationCommerce((CommerceTypes)iI) >= 0);
			FAssert(GET_PLAYER(getOwnerINLINE()).getFreeCityCommerce((CommerceTypes)iI) >= 0);
		}

		for (iI = 0; iI < GC.getNumBonusInfos(); iI++)
		{
			FAssert(isNoBonus((BonusTypes)iI) || getNumBonuses((BonusTypes)iI) >= ((isConnectedToCapital()) ? (GET_PLAYER(getOwnerINLINE()).getBonusImport((BonusTypes)iI) - GET_PLAYER(getOwnerINLINE()).getBonusExport((BonusTypes)iI)) : 0));
		}
	}
#endif
...}

The second assert is:

Assert Failed

File: CvCity.cpp
Line: 12179
Expression: eIndex >= 0
Message: eIndex expected to be >= 0

----------------------------------------------------------

And the relevant code:

int CvCity::getUnitCombatFreeExperience(UnitCombatTypes eIndex) const
{
	FAssertMsg(eIndex >= 0, "eIndex expected to be >= 0");
	FAssertMsg(eIndex < GC.getNumUnitCombatInfos(), "eIndex expected to be < GC.getNumUnitCombatInfos()");
	return m_paiUnitCombatFreeExperience[eIndex];
}

Any Ideas on what the cause could be?

 

[EmperorFool]

For the first it seems that the member variables in CvCity are getting set incorrectly such that they no longer track that actual values calculated by adding up the relevant tiles.

The second one is definitely a problem as it causes the code to read outside the valid array bounds--never a good thing. Check where that function is being called with an invalid UnitCombatTypes value (probably NO_UNITCOMBAT from a Worker or Settler).

 

[Afforess]

The second one is definitely a problem as it causes the code to read outside the valid array bounds--never a good thing. Check where that function is being called with an invalid UnitCombatTypes value (probably NO_UNITCOMBAT from a Worker or Settler).

Oh, I bet I know... Settlers and Workers have a unitcombat in RoM. They are all "UNITCOMBAT_SETTLER", could that be causing an issue?

Also, the only units with <COMBAT>NONE</COMBAT> are Missionaries, Execs, GP, Trade Caravans, and Wild Animals...

 

[EmperorFool]

If it has a valid UnitCombatTypes it shouldn't cause a problem as it will be inside the array bounds. Only -1 (UNITCOMBAT_NONE) or values higher than the maximum value will cause trouble if it's the array access that is causing problems.

You must check all code that calls this function to ensure that it checks there is a valid combat type before making the call. If they all do, the problem is somewhere else.

 

[Afforess]

If it has a valid UnitCombatTypes it shouldn't cause a problem as it will be inside the array bounds. Only -1 (UNITCOMBAT_NONE) or values higher than the maximum value will cause trouble if it's the array access that is causing problems.

You must check all code that calls this function to ensure that it checks there is a valid combat type before making the call. If they all do, the problem is somewhere else.

I just checked, all the functions that call it (And there aren't very many), all check for or cast to UnitCombatTypes first.

 

[davidlallen]

I just checked, all the functions that call it (And there aren't very many), all check for or cast to UnitCombatTypes first.

Casting isn't the same as range protection. "(UnitCombatTypes) -1" still causes an illegal access violation. If there is some call which casts, but does not check the value, this could be the problem.

 

[Afforess]

Okay, but I have never even touched this function, in my SDK, or even added any new calls to it. I suppose I'm just counting on Firaxis's code to be correct, which probably is an invalid assumption...

For code like this that casts it to UnitCombatTypes, would I need to change anything?

for (iI = 0; iI < GC.getNumUnitCombatInfos(); iI++)
		{
			changeUnitCombatFreeExperience(((UnitCombatTypes)iI), GC.getBuildingInfo(eBuilding).getUnitCombatFreeExperience(iI) * iChange);
		}

 

[EmperorFool]

No because you can validate that iI will always be a valid UnitCombatTypes value by a) looking at the for() line and b) seeing that iI isn't changed in the loop other than in the for() line itself. It will never be UNITCOMBAT_NONE (-1) and is thus safe for that call.

As for trusting the Firaxis code is flawless, no code is flawless. Every program of sufficient complexity (more than one hundred lines?) will have bugs. I believe that if you build a Debug version of the stock BTS code and run it with the stock XML you'll get failed assertions, but don't quote me on that. I am nearly positive that WoC has failed assertions out-of-the-box.

An assertion failure can sometimes indicate that the code has drifted from the assertions. Normally an assertion failure should mean the imminent demise of the program or at least invalid results, but if the code has been fixed to handle the assertion error gracefully, the assertion becomes erroneous. An example would be a function that could not tolerate a NULL parameter that is later changed to handle it properly, but the assertion isn't removed.

 

[Afforess]

As for trusting the Firaxis code is flawless, no code is flawless. Every program of sufficient complexity (more than one hundred lines?) will have bugs. I believe that if you build a Debug version of the stock BTS code and run it with the stock XML you'll get failed assertions, but don't quote me on that. I am nearly positive that WoC has failed assertions out-of-the-box.

It does. So does RevDCM. If you go into AI AutoPlay mode, you get 4 asserts because you just asked for a ID for a human player...