Calling buildings in the DLL

[Inightshade]

Alright, so I'm finally stuck, unable to figure out where I've gone wrong.
And I think I need a fresh pair of eyes (or some random wisdom).

I have here a piece of code:

int CvPlayer::getBonusUnitHpFromBuildings() const
{
	int iTotalBonusHpFromBuildings = 0;
	const CvCity* pLoopCity;
	int iLoop;

	// DEBUG
	//return iTotalBonusHpFromBuildings;


	for (int jFCM = 0; jFCM < GC.getNumBuildingClassInfos(); jFCM++)
	{
		BuildingClassTypes eBuildingClass = (BuildingClassTypes)jFCM;

		CvBuildingClassInfo* pkBuildingClassInfo = GC.getBuildingClassInfo(eBuildingClass);
		if (!pkBuildingClassInfo)
		{
			continue;
		}

		CvCivilizationInfo& playerCivilizationInfo = getCivilizationInfo();
		BuildingTypes eBuilding = (BuildingTypes)playerCivilizationInfo.getCivilizationBuildings(eBuildingClass);

		CvBuildingEntry* pBuildingInfo = GC.getBuildingInfo(eBuilding);
		if (!pBuildingInfo || !pBuildingInfo->GetHpPer() > 0)
		{
			continue;
		}

		for (pLoopCity = firstCity(&iLoop); pLoopCity != NULL; pLoopCity = nextCity(&iLoop))
		{
			if (pLoopCity->GetCityBuildings()->GetNumBuilding(eBuilding) > 0)
			{
				iTotalBonusHpFromBuildings += pBuildingInfo->GetHpPer() * pLoopCity->GetCityBuildings()->GetNumBuilding(eBuilding);
			}
		}
	}

	return iTotalBonusHpFromBuildings;
}

That totally works. Except it doesn't.

So here is the gist, the hp increases correctly in game (and works as expected) but I crash (consistently) between turns 20 and 40.
"Disabling" this (by uncommenting that first return) allows me to play until turn 150ish (where I crash due to not having spy names for a few people )
But, but why? It always crashes on the barbs turn (so during post processing I think).
It doesn't run for barbs and minors (that is prevented here:

int CvPlayer::getBonusUnitHp() const
{
	// TODO: add Global value.
	int iTotalBonusHp = m_iBonusUnitHp;

	// Check buildings (Must have found a city for this to work!)
	// We don't care about Barbs or Minors for this. As this is special buildings only.
	if (!GetNumCitiesFounded() > 0 || isBarbarian() || isMinorCiv())
		iTotalBonusHp += getBonusUnitHpFromBuildings();

	//TODO: Add trait?

	//TODO: add promotions?

	//TODO: Add lua?
	
	return iTotalBonusHp;
}

)

-shrugs-

So some extra info, in code Civ calls for the global max hp, not the units max hp (which actually calls for the global, rather than being dynamic :/ ) so I went through and changed 99% of those. The few I left were AI calculations (half hp) and since I don't really think I'll end up with more than 120 I didn't bother.
However, given that this works (including performing combat! where 99% of the maxhp calls are) I have less than no clue why it's breaking stuff, but only in a delayed fashion. :/

Anyways, I have DnD today, so I won't get back to this until later. Thanks for any insight, wisdom, or logic questions. (Given that the last might help me break out of the stupor that is staring at one peice of code for hours on end).

 

[whoward69]

It doesn't run for barbs and minors (that is prevented here:

int CvPlayer::getBonusUnitHp() const
{
	int iTotalBonusHp = m_iBonusUnitHp;

	// Check buildings (Must have found a city for this to work!)
	// We don't care about Barbs or Minors for this. As this is special buildings only.
	if (!GetNumCitiesFounded() > 0 || isBarbarian() || isMinorCiv())
		iTotalBonusHp += getBonusUnitHpFromBuildings();

	return iTotalBonusHp;
}

Check the C++ operator precedence, your if condition isn't doing what you think it is

 

[Rajul]

Shouldn't it be:

if (![B]([/B]GetNumCitiesFounded() <= 0 || isBarbarian() || isMinorCiv()[B])[/B])

or

if (GetNumCitiesFounded() > 0 && !isBarbarian() && !isMinorCiv())

?

 

[Inightshade]

Check the C++ operator precedence, your if condition isn't doing what you think it is

I can't believe this is what I missed./

if (GetNumCitiesFounded() > 0 && !isBarbarian() && !isMinorCiv())

Thanks. :/

Think I was too much in the mentality of:

if (!pBuildingInfo || !pBuildingInfo->GetHpPer() > 0)
		{
			continue;
		}

To realise my mistake.

I'm curious as to why it waited to crash on turns 28ish rather than right away....
But perhaps best to not ask such questions
(And I doubt it was the minors, I had the crashes prior to that, thought it would help if it ran less often.)


Finally. Is it really necessary for me to run through buildingClasses as a loop? Why oh why can I not loop through the city buildings and check for building traits there?

 

[whoward69]

Why oh why can I not loop through the city buildings and check for building traits there?

In an OO world, the CvCityBuildings object would contain a set of CvCityBuilding objects that you could iterate. But this is C converted to compile under C++, so it uses arrays indexed by all possible building types to store off specific values. To compound these flaws, the programmer didn't provide accessor/iterator methods, so a lot of higher-level code has to duplicate these basic functions - welcome to the Firaxis hell that is copy-and-paste coding.

 

[Inightshade]

Ug. I still get the consistent crash. Though it's around turns 40-50 now. (6 tries in a row, same crash spot (listed as barbs turn)).

And, the 2 tries (2 to make sure it wasn't a fluke) with only the getBonusUnitHpFromBuildings being 'disabled' by immediatly returning a value of 0 as the change. (Well, I've been fixing up UI things also, but it was: crash, crash, crash, change, no crash, no crash, change back, crash, crash, crash.)

Mrph. Is it that I call a building loop every time there is combat? I've tried my best to limit the number of time this is called (skipped for barbs and minors, only called once (for each attker and defender) for each combat, especially when the hp value is used 6-20 times in one function)

???
EDIT: Spelling.

 

[whoward69]

Returning 0 at the top of the method is not terribly useful ... you need to identify the line that it's crashing on. So either add logging statements (see the end of the first of my DLL tutorials (link in my sig) for an example) or use the "mini dump" code and use the Visual Studio debugger to identify it.

YMMV as to which works best/consistently for you

EDIT: Unless you're doing/calling something that's leaking memory (always a possibility), calling the method many times won't cause the crash, just cause the system to slow down.

 

[Inightshade]

Returning 0 at the top of the method is not terribly useful

I know It was being used to determine if it was the statement or if I mussed up on one of the 9000 GC.getMAX_HIT_POINTS() replacements.

And a logger caused a rather funny issue. It crashed, and when I checked the log, it was 1.6gb in size just from running 3 times in the getBonusUnitHpFromBuildings() statement. :/

So this is the last call (for this crash): GetMaxHitPoints() called from GenerateMeleeCombatInfo for kAttk and pkDef:

Except it was called previously several times (like, several...) and those 'worked'.

All my logs activate before anything happens, so it called and crashed between here:

void CvUnitCombat::GenerateMeleeCombatInfo(CvUnit& kAttacker, CvUnit* pkDefender, CvPlot& plot, CvCombatInfo* pkCombatInfo)
{
#if defined(JFCM_BUILDING_UNIT_HP)
	JFCM_LOG("GetMaxHitPoints() called from GenerateMeleeCombatInfo for kAttk and pkDef: ", kAttacker, pkDefender);
	int iAtkMaxHP = kAttacker.GetMaxHitPoints();
	int iDefMaxHP = pkDefender->GetMaxHitPoints();
#else
	int iMaxHP = GC.getMAX_HIT_POINTS();
#endif

and here

#if defined(JFCM_BUILDING_UNIT_HP)
int CvPlayer::getBonusUnitHp() const
{
	// TODO: add Global value.
	int iTotalBonusHp = /*0*/ m_iBonusUnitHp;

	// Check buildings (Must have found a city for this to work!)
	// We don't care about Barbs or Minors for this. As this is special buildings only.
	if (GetNumCitiesFounded() > 0 && !isBarbarian() && !isMinorCiv())
		iTotalBonusHp += getBonusUnitHpFromBuildings();

	//TODO: Add trait?

	//TODO: add promotions?

	//TODO: Add lua?
	
	return iTotalBonusHp;
}

int CvPlayer::getBonusUnitHpFromBuildings() const
{
	JFCM_LOG("GetMaxHitPoints() called");
	int iTotalBonusHpFromBuildings = 0;
	const CvCity* pLoopCity;
	int iLoop;

with the only idemitary being:

//	--------------------------------------------------------------------------------
int CvUnit::GetMaxHitPoints() const
{
	VALIDATE_OBJECT
#if defined(JFCM_BUILDING_UNIT_HP)
		int iTotalHp = GC.getMAX_HIT_POINTS();
	iTotalHp += GET_PLAYER(getOwner()).getBonusUnitHp();
	return iTotalHp;
#else
		return GC.getMAX_HIT_POINTS();
#endif
}

That it worked numerous times before crashing is, curious...
Is there a need to check kAttacker or pkDefender? is there a way?
Or did my log truncate at 33315240 lines and give up tracking?

The weird part is my consistency to play past 100 turns with this being only sudo disabled, which leaves me very confused.

I don't know anything about logging, I'm sure there is a way to not actually log 33 million times in 44 turns.

Final note: I couldn't use Notepad (or notepad++) to open the log lol. So I had to use something else. I don't get anything for the

", kAttacker, pkDefender);

part, nor do I for any others. -shrugs-


EDIT:

just from running 3 times in the getBonusUnitHpFromBuildings() statement. :/

It is worth note that it did appear to work right... :/ 99% of the logs are from just the first 2 logs in the main statement. and a few of what's left are the rare breakthroughs of the building actually existing.
So... It's crashing somewhere, for some reason, but not here. At least, I think.

Would it crash when trying to call a method with an empty object? I'm trying to figure out how it's breaking here, as I definitely recall sieging a city, attacking with a city, melee attacks, ranged attacks, boat fights, and capturing civies. Outside of nukes and Air I can't think of anything that I haven't seen work in game. :\

My next test will be 1v1 with no barbs or minors. there are too many variables in standard sized games.

 

[whoward69]

A quick search for calls to GenerateMeleeCombatInfo() reveals

		// We are doing a non-ranged attack on a city
		CvCombatInfo kCombatInfo;
		GenerateMeleeCombatInfo(kAttacker, NULL, plot, &kCombatInfo);

so

int iDefMaxHP = pkDefender->GetMaxHitPoints();

needs to check that pkDefender is not NULL

 

[Inightshade]

Thanks, there is a CvAssert that I glossed over later down in the method. I can just move the defender hp get to after that, I should also check other combat methods for similar issues. (So would this mean the crash is caused by meleeing a city? strange. I swear I captured 2 cities (and burned one) one of these tests (trying to see how fast I could kill my neighbor! > ))

I totally forgot that cities would be handled differently with melee (Cities have their own getMaxHitPoints() also) and since ranged had 2 different methods (one with cities, one without) and melee one, I made assumptions. Silly me. :/

Time to see if this works! I'm disabling logs in the loop though. holy crap does logging 33 million times cause lag.

Thanks for the help!
(I was kinda disheartened by a bug I couldn't figure out. too much progress recently lol.)

 

[whoward69]

Final note: I couldn't use Notepad (or notepad++) to open the log lol.

Try BareTail