RevolutionDCM for BTS

[Afforess]

Hi ya Phungi
I'm taking a look at what I might be able to do in the list. Afforess is really busy at the moment. I suspect that when he can, he will introduce his RevDCM options solution for multiplayer which is on the 2.8 list as the "AND" system.

It's done now, and will keep Global Defines in sync correctly - I just need to port it over.

 

[phungus420]

It would be really nice to get some help on fixing some of these bugs. If anyone has any ideas I may be able to tackle some issues with some help; however the problems seem to be deep in changePlayer and AIAutoplay. To be honest it feels like RevDCM is dead. Unfortunately I don't known where else to go though. The current version of RevDCM has some serious bugs in it, that make AIAuotplay unusable with certain game settings, and also alot of Revolutions functions simply aren't working (lead rebels for instance). Is there any chance for some help on this? Sorry to sound negative, but it's literally been months, since May actually, since any real bugs have been worked on by anyone other then myself (I know Afforess fixed some WoC issues, however my feelings for WoC are about the same as how I feel about the neighbor's dog's poo in my yard, maybe Afforess polished the poo, but it's still poo that I don't want in my yard anyway).

In case any help is possible, a debug dll seems to be pointing to the fact some python functions are trying to make calls on dead or non existent players. I've been considering adding some code to have python exceptions fire when a debug dll is running when CyPlayer would be making a call it shouldn't. Not really sure where to isolate this though, for reference it's pretty easy to do this, look at Fuyu's last post here:

http://forums.civfanatics.com/showthread.php?t=361439

Basically when you use the

throw new exception();

command, Python will throw an exception giving the line number and file name of the offending call. Pretty useful, did alot of bug fixing for 2.72 using this method. Unfortunately the current bugs aren't as visible, and seem hidden deep in the bowls of changePlayer and AIAutoplay, so I'm not sure what to zero in on. Debugging is telling me the call where things first go wrong with is getCivilzationDescriptionKey, but I"m sure things are getting messed up before this: it seems that for some reason AIAutoplay is making it so dead or non existent civs are being evaluated for things that they obviously shouldn't be.

Any help would be greatly appreciated, any ideas, anything really would help improve my spirits, cause right now I'm just pretty frustrated.

 

[Fimbulvetr]

I'm not sure I can help, as I speak no python and I've never looked at RevDCM sources; and I have little more interest in Revolutons than you have in WoC.

If you have an assert msg and want to dig deeper, first replace
FASSERT_BOUNDS(0, MAX_PLAYERS, eID, "CvInitCore::getCivDescriptionKey");
with
FAssertMsg(eID >= 0, "eID is expected to be non-negative");
FAssertMsg(eID < MAX_PLAYERS, "eID is expected to be smaller than MAX_PLAYERS");
FAssertMsg(eID >= -1, "WTH?"); //lower than -1 should not even be valid player ID at all
FAssertMsg(eID <= MAX_PLAYERS, "WTH???"); //same goes for higher than MAX_PLAYERS
so you know what you are dealing with (-1 is NO_PLAYER and MAX_PLAYERS is Barbarians), and then proceed to search for places where it is called from, add Assert statements there (if it's python you can add some raise Exception("text")), and one by one you should be able to get closer to the source.

My only idea:
CvPlot::setOwner(): I believe eNewValue could eventually be MAX_PLAYERS if a barbarian unit captures a city.

		[COLOR="Gray"](...)[/COLOR]
		if (pOldCity != NULL)
		{
			szBuffer = gDLL->getText("TXT_KEY_MISC_CITY_REVOLTED_JOINED", pOldCity->getNameKey(), GET_PLAYER(eNewValue).getCivilizationDescriptionKey());
			[COLOR="Gray"](...)[/COLOR]
			szBuffer = gDLL->getText("TXT_KEY_MISC_CITY_REVOLTS_JOINS", pOldCity->getNameKey(), GET_PLAYER(eNewValue).getCivilizationDescriptionKey());

but in BarbarianCiv.py's createMinorCiv, newPlayer can't cause that assert. And if pRevPlayer in Revolution.py can ever be -1 or MAX_PLAYERS, idk ..

 

[phungus420]

Thanks Fuyu for taking the time to offer up some ideas.

When things go wrong, the Civilization index is out of bounds in CvGlobals, and walking through the debugger makes me believe that it starts with the function:

const wchar* CvPlayer::getCivilizationDescription(uint uiForm) const

At least I know that some number that's outside the range 0-MAX_CIVS is being passed into CvGlobals asking for the civ description by this function.

Now I'm fairly sure that it's python that's triggering this, so looking at CyPlayer I'd like to add in some code to get it get Python to throw an exception when

std::wstring CyPlayer::getCivilizationDescription(int iForm)

is called on an invalid player. Since this function is in CyPlayer I'm not sure how to determine weather CyPlayer is out of bounds (ie not between 0 and MAX_PLAYERS). Do you know hwo I can write a control statement in CyPlayer to determine this?

also do you know what the argument iForm is referring to? I cannot figure it out,

 

[Fimbulvetr]

Note to self: So, even BARBARIAN_PLAYER has an ID < MAX_PLAYERS, so out of bould means either NO_PLAYER or bogus value.

CyPlayer check:

std::wstring CyPlayer::getCivilizationDescription(int iForm)
{
[B]	if (m_pPlayer)
	{
		FAssertMsg(m_pPlayer->getID() >= 0, "m_pPlayer should actually be a player");
		FAssertMsg(m_pPlayer->getID() < MAX_PLAYERS, "Player ID can never be greater than MAX_PLAYERS - 1");
		if (m_pPlayer->getID() < 0 || m_pPlayer->getID() >= MAX_PLAYERS)
		{
#ifdef _DEBUG
			throw new exception();
#endif
       		 return std::wstring();
		}
	}[/B]

	return m_pPlayer ? m_pPlayer->getCivilizationDescription((uint)iForm) : std::wstring();
}

.. and no, I have no clue what iForm/uiForm is.


edit: I don't think it's possible to have a valid m_pPlayer pointer with a bogus ID though.

 

[phungus420]

Thanks again for the help.

Unfortunately my hunch wasn't right, and adding those asserts did not catch anything new. For more specific information, this is the first place where I'm made aware something has gone wrong:

CvCivilizationInfo& CvGlobals::getCivilizationInfo(CivilizationTypes eCivilizationNum)
{
	[B]FAssert(eCivilizationNum > -1);[/B]
	FAssert(eCivilizationNum < GC.getNumCivilizationInfos());
	return *(m_paCivilizationInfo[eCivilizationNum]);
}

bolded assert that fires. If start stepping through the debugger the function that trips this is:

const wchar* CvPlayer::getCivilizationDescription(uint uiForm) const
{
/************************************************************************************************/
/* REVOLUTION_MOD                         01/01/08                                jdog5000      */
/*                                                                                              */
/* dynamic civ names                                                                            */
/************************************************************************************************/
/*
	if (GC.getInitCore().getCivDescription(getID(), uiForm).empty())
	{
		return GC.getCivilizationInfo(getCivilizationType()).getDescription(uiForm);
	}
	else
	{
		return GC.getInitCore().getCivDescription(getID(), uiForm);
	}
*/
	if( !(m_szCivDesc.empty()) )
	{
		return m_szCivDesc;
	}
	[B]else if (GC.getInitCore().getCivDescription(getID(), uiForm).empty())[/B]
	{
		return GC.getCivilizationInfo(getCivilizationType()).getDescription(uiForm);
	}
	else
	{
		return GC.getInitCore().getCivDescription(getID(), uiForm);
	}
/************************************************************************************************/
/* REVOLUTION_MOD                          END                                                  */
/************************************************************************************************/
}

Bolded the line of code that the debugger steps directly into after the assert.

Now here is what I know about the bug: It only pops up if you change player or go into AI autoplay, if you just play the game, this does not occur. If you ever changed player or went into AIAutoplay this assert will end up occuring, even if you're playing normal. (strange huh?). This assert happens when a minor civ is supposed to settle, ie it will happen if you're plaiying start as minors and a civ gets writing, and/or it'll fire when you have Barb Civs on, and the barb civ attempts to settle down into a full civ from a minor civ (the bug also causes the minor civ settling to fail, and the game can be played on, but the civs stay minors).

Because of the above behavior I figured the bug must be in the settleMinorCiv function, but I cannot find any issues with it. I also went through the BarbarianCiv.py and MinorCiv.py files and added a bunch of checks to ensure the Player is alive and their id# is between 0 and MAX_PLAYERs, but the same bug continues to occur. Because adding your code that ensures the call is being made on a real player, and the fact that adding the aforementioned code to ensure a valid ID number in the relevant functions in the Revolution python files, I'm pretty sure an invalid ID number is not the cause.

I'm including a RevDCM save (current SVN version) where the bug occurs within a couple turns, I would post the setterMinorCiv code, but it's too many characters long for a post, so I'm including my modified BarbarianCiv.py file with the added player ID number validation checks, for the original see the RevDCM SVN. If you (or anyone else) has any ideas of what else to check I'll work further on debugging this, unfortunately right now I have no idea what I should work on next, and also don't even have a hunch for what is causing the problem, so I'm just stumped with no direction to go.

 

[Fimbulvetr]

Unfortunately my hunch wasn't right, and adding those asserts did not catch anything new.

I'm not surprised.

edit: I don't think it's possible to have a valid m_pPlayer pointer with a bogus ID though.

FAssert(eCivilizationNum > -1);
eCivilizationNum is no playerID int, that's CivilizationTypes, something you get from CvPlayer::getCivilizationType().

 

[phungus420]

OK, but how is it possible to get a returned value of -1/NONE/NO_CIVILIZATION on a player.getCivilizationType() call on a valid player, if the player is a valid player (ie, they have an ID between -1 and MAX_PAYERS), how can they have NO_CIVILIZATION?

 

[Fimbulvetr]

A normal player will have a civilizationType but a new one might not. The key is a change_player component, void CvPlayer::changeCiv( CivilizationTypes eNewCiv ) - if this is not called for a new player (ie when barbs settle into a civ) before some other code tries to do getCivilizationDescription, then you get this assert failure.

 

[phungus420]

This is happening when a minor civ (You can set civs to minor in a WB save file, and minor civs have a specific civilization type, just a civ tagged as Minor cannot engage in diplo and is always at war with all other civs) settles into a full civ, ie it gets it's Minor status removed, I can't see how this would effect it's civilization type. I'm not trying to be argumentative, I'm just not following what I should look at next.

In terms of how RevDCM operates if a gameoption StartAsMinors is selected, all civs start out tagged as minor civs. Once they discover writing, the minor status is revoked, and they can engage in diplomacy. When a civ discovers writing is when this assert fires, and minor status is no longer revoked. This bug reared it's head after the changePlayer and AIAutoplay functions were supposed to be made functional in Multiplayer, but they obviously aren't as they don't work in single player, as noted. I didn't do any of the code for the MP fixes, so I can't comment specifically on what they could have changed, which is why this is so frustrating as I'm totally in the dark.

Unfortunately it appears that no one else wants to work on fixing these bugs though, so that's why I'm posting here, and am grateful that you're trying to help out. Based on the described behavior of this assert do you have any ideas of what could be causing it, and how I could go about ruling these ideas out? I have hit a brick wall (why I'm posting) and am out of ideas on what I should try next.

 

[Fimbulvetr]

Ok, I admit I have no clue how these Revolution features work. Still, if that assert is triggered, some civ obviously has no civilization type.


I hope this helps a bit..

Spoiler :

std::wstring CyPlayer::getCivilizationDescription(int iForm)
{
[B]	if (m_pPlayer)
	{
		if (m_pPlayer->getCivilizationType() < 0)
		{
		FAssertMsg(false, "m_pPlayer should have a civilizationType");
#ifdef _DEBUG
		throw new exception();
#endif
      	 	return std::wstring();
		}
	}[/B]

	return m_pPlayer ? m_pPlayer->getCivilizationDescription((uint)iForm) : std::wstring();
}

std::wstring CyPlayer::getCivilizationDescriptionKey()
{
[B]	if (m_pPlayer)
	{
		if (m_pPlayer->getCivilizationType() < 0)
		{
		FAssertMsg(false, "m_pPlayer should have a civilizationType");
#ifdef _DEBUG
		throw new exception();
#endif
      	 	return std::wstring();
		}
	}[/B]
	
	return m_pPlayer ? m_pPlayer->getCivilizationDescriptionKey() : std::wstring();
}

std::wstring CyPlayer::getCivilizationShortDescription(int iForm)
{
[B]	if (m_pPlayer)
	{
		if (m_pPlayer->getCivilizationType() < 0)
		{
		FAssertMsg(false, "m_pPlayer should have a civilizationType");
#ifdef _DEBUG
		throw new exception();
#endif
       		return std::wstring();
		}
	}[/B]

	return m_pPlayer ? m_pPlayer->getCivilizationShortDescription((uint)iForm) : std::wstring();
}

std::wstring CyPlayer::getCivilizationShortDescriptionKey()
{
[B]	if (m_pPlayer)
	{
		if (m_pPlayer->getCivilizationType() < 0)
		{
		FAssertMsg(false, "m_pPlayer should have a civilizationType");
#ifdef _DEBUG
		throw new exception();
#endif
       		return std::wstring();
		}
	}[/B]

	return m_pPlayer ? m_pPlayer->getCivilizationShortDescriptionKey() : std::wstring();
}

std::wstring CyPlayer::getCivilizationAdjective(int iForm)
{
[B]	if (m_pPlayer)
	{
		if (m_pPlayer->getCivilizationType() < 0)
		{
		FAssertMsg(false, "m_pPlayer should have a civilizationType");
#ifdef _DEBUG
		throw new exception();
#endif
       		return std::wstring();
		}
	}[/B]
	
	return m_pPlayer ? m_pPlayer->getCivilizationAdjective((uint)iForm) : std::wstring();
}

std::wstring CyPlayer::getCivilizationAdjectiveKey( )
{
[B]	if (m_pPlayer)
	{
		if (m_pPlayer->getCivilizationType() < 0)
		{
		FAssertMsg(false, "m_pPlayer should have a civilizationType");
#ifdef _DEBUG
		throw new exception();
#endif
       		return std::wstring();
		}
	}[/B]

	return m_pPlayer ? m_pPlayer->getCivilizationAdjectiveKey() : std::wstring();
}

This should at least reveal from where in the python mess this is coming from.

 

[phungus420]

Yeah, I added those right after you first that the error was civilizationIndex being out of bounds, and not the player. Python's errors aren't firing till after the assert in the dll, so this call must be happening in the dll. I wish there was a way to step back using the debugger... but unfortunately I do not know of any. Python does end up throwing some assert messages, but I think these are being triggered by BUG and then effecting RevDCM, and not the other way around, at least it seems this way, and I think the issue BUG is having is being caused by whatever is going wrong in the DLL. I'll look at CvTeam::setIsMinorCiv and see how it works, as this seems to be the trigger for everything, but unfortunately the calls to it seem to be correct in the python, so far as I can tell.

 

[Afforess]

FYI: I added the system I built to keep global defines in sync, Fixed the popups in AI Autoplay, and Fixed the CopyNonDefaults.

 

[Fimbulvetr]

At some point you'll catch it.

BarbarbianCiv.py: You could add a "playerI.getCivilizationDescription(0)" to the beginning of checkMinorCivs, maybe that can trigger the error earlier.

CyTeam.cpp: check setIsMinorCiv for civs without civilization type

void CyTeam::setIsMinorCiv( bool bNewValue, bool bDoBarbCivCheck )
{	if( m_pTeam )
	[B]{[/B]
		[B]for(int iI = 0; iI < MAX_CIV_PLAYERS; iI++)
		{
			if( GET_PLAYER((PlayerTypes)iI).getTeam() == m_pTeam->getID() )
			{
				if (GET_PLAYER((PlayerTypes)iI)->getCivilizationType() < 0)
				{
					FAssertMsg(false, "GET_PLAYER((PlayerTypes)iI) of m_pTeam should have a civilizationType");
#ifdef _DEBUG
					throw new exception();
#endif
				}
			}
		}[/B]
		
		m_pTeam->setIsMinorCiv(bNewValue, bDoBarbCivCheck);
	[B]}[/B]
}

But then I'm fresh out of ideas.

 

[phungus420]

That's a pretty clever way to go about it Fuyu, but no dice. I added some checks to make sure the minor civ has a civilization type, and it does, the assert message does not fire.

Strangely enough I'm getting an Unidentified C++ exception at the setIsMinorCiv line in BarbarbianCiv.py anyway, but it's not the fact the civ doesn't have a civilizationType (assert doesn't fire with the message, and I added a check a line above to make sure and it went fine). I don't understand how this function could be screwing up, the code is pretty direct and simple:

		# Convert team
		if( gc.getTeam(newPlayer.getTeam()).isOpenBordersTrading() or not gc.getGame().isOption(GameOptionTypes.GAMEOPTION_START_AS_MINORS) ) :
			newPlayer.getCivilizationType();
			newTeam.setIsMinorCiv(false, false)

Unfortunately I'm not seeing what could be wrong in that...

 

[Afforess]

Oh, yeah, I remember the issue you guys are trying to tackle it. I had it a few months back. I never figured out what caused it, but I did create a work-around. It only fixes the symptom - but I've never seen any other issues.

Here's the original code:

const wchar* CvPlayer::getCivilizationDescription(uint uiForm) const
{
/************************************************************************************************/
/* REVOLUTION_MOD                         01/01/08                                jdog5000      */
/*                                                                                              */
/* dynamic civ names                                                                            */
/************************************************************************************************/
/*
	if (GC.getInitCore().getCivDescription(getID(), uiForm).empty())
	{
		return GC.getCivilizationInfo(getCivilizationType()).getDescription(uiForm);
	}
	else
	{
		return GC.getInitCore().getCivDescription(getID(), uiForm);
	}
*/
	if( !(m_szCivDesc.empty()) )
	{
		return m_szCivDesc;
	}
	[COLOR="red"]else if (GC.getInitCore().getCivDescription(getID(), uiForm).empty())[/COLOR]
	{
		return GC.getCivilizationInfo(getCivilizationType()).getDescription(uiForm);
	}
	else
	{
		return GC.getInitCore().getCivDescription(getID(), uiForm);
	}
/************************************************************************************************/
/* REVOLUTION_MOD                          END                                                  */
/************************************************************************************************/
}

And here's it with my change:

const wchar* CvPlayer::getCivilizationDescription(uint uiForm) const
{
/************************************************************************************************/
/* REVOLUTION_MOD                         01/01/08                                jdog5000      */
/*                                                                                              */
/* dynamic civ names                                                                            */
/************************************************************************************************/
/*
	if (GC.getInitCore().getCivDescription(getID(), uiForm).empty())
	{
		return GC.getCivilizationInfo(getCivilizationType()).getDescription(uiForm);
	}
	else
	{
		return GC.getInitCore().getCivDescription(getID(), uiForm);
	}
*/
	if( !(m_szCivDesc.empty()) )
	{
		return m_szCivDesc;
	}
	
	[COLOR="Red"]else if (getCivilizationType() != NO_CIVILIZATION && GC.getInitCore().getCivDescription(getID(), uiForm).empty())[/COLOR]
	{
		return GC.getCivilizationInfo(getCivilizationType()).getDescription(uiForm);
	}
	else
	{
		return GC.getInitCore().getCivDescription(getID(), uiForm);
	}
/************************************************************************************************/
/* REVOLUTION_MOD                          END                                                  */
/************************************************************************************************/
}

BTW, any chance that you but a breakpoint at that return statement, and figure out what the ID value for the player was in Visual Studio? It would help track down which player the game was trying to find the description of.

 

[phungus420]

Awesome job afforess, that actually fixes all the issues with BarbarianCiv and StartAsMinors. This assert must have been messing up BUG somehow, which cascaded the problem into the Revolutions component. Fixing the check made that go away.

Now to see what's going wrong with Change player.

 

[Fimbulvetr]

If there are no other issues, which would mean that the civ that has no civilizationtype at one point fixes itself soon afterwards anyway, then there's no problem with just fixing the symptom. I admit I hadn't thought of that, nice work.

 

[phungus420]

Damn, was really hoping that this had worked. changePlayer even seemed to be working fine after this. However after running AIAutoplay for a few dozen more turns I get a crash to desktop. The debugger claims it is an access violation error in this function:

CivilizationTypes CvInitCore::getCiv(PlayerTypes eID) const
{
	FASSERT_BOUNDS(0, MAX_PLAYERS, eID, "CvInitCore::getCiv");
	if ( checkBounds(eID, 0, MAX_PLAYERS) )
	{
		return m_aeCiv[eID];
	}
	else
	{
		return NO_CIVILIZATION;
	}
}

That's pretty confusing, considering the function checks to ensure the player ID is within the correct bounds. I'm not sure how an access violation error is occuring. I tried adding some code to check if the civ was alive, as I thought that may cause the problem, but doing so throws a nearly unlimited number of asserts when the game first loads up, and causes an instant CTD in the disassembly when trying to load a game; so this function must be able to be called on non alive civs.

 

[Fimbulvetr]

Add a check for m_aeCiv at the beginning.

if (!m_aeCiv)
{
	FAssertMsg(false, "something odd is going on");

 	return NO_CIVILIZATION;
}

But I have no clue how that could be happening so this guess is probably is useless as the last one.