K-Mod bugs

[vedg]

While playing K-Mod version 1.46 in single player mode, I encountered the following 3 bugs:
1. AI switches from Hereditary Rule to Despotism. This happened to 2 different non-spiritual AIs without a human request and not because of Pyramids loss (both civilizations had the Monarchy technology). As far as I know, Despotism has no advantages over Hereditary Rule, only disadvantages. Both AIs switched back to Hereditary Rule later. This mistake on their part cost them 1-2 turns of Anarchy, possible happiness issues, worse attitude from other AIs with Hereditary Rule as a favorite civic. More specifically, in the attached saves, Wang Kon switches to Despotism in 1435 and Willem in 1535.

2. After making peace, a Khmer stack was automatically transported from my territory to a small unsettled desert island. The stack remained unused there for more than 80 turns, until the destruction of Khmers. Suryavarman certainly could use these 9 units as Sitting Bull declared on him shortly after my war. And later he was about to launch a naval invasion on Babylon... At first Suryavarman had only 2 Galleys in Hariharalaya, though even they could move the stack in 2 runs. 80 turns later he got 4 Galleons close by in Cahokia, but never reclaimed the now outdated stack. I didn't even bother destroying these units - they disappeared automatically upon Khmer destruction. I suspect that some AI code mistake made Suryavarman forget about this stack. Attached 2 relevant screenshots and 3 saves: the stack is moved from the vicinity of Thebes to the nearby island at the end of the war in 1200. The stack is still there in 1630.

Spoiler Screenshots :

The Khmer stack near Thebes just before peace:


The stack remains on the island 80 turns since peace:

3. The History->Strategy section of Sevopedia for Galley, Galleon and Transport incorrectly specifies their cargo space as 2, 3 and 4 respectively, despite the recent K-Mod increase of their capacities by one, which is reflected in the primary Special Abilities section of Sevopedia. The numbers in the History section should either be updated or removed as redundant to avoid confusion.

Spoiler Screenshot :

By the way, I suggest further increasing Transport space to 6 in order to give this 1.5 times more expensive unit a more substantial advantage over Galleon. The current small difference in cargo space, a bit faster movement and more strength usually make the Galleon->Transport upgrade unappealing. I'd leave the East Indiaman cargo space at 5 to give the more expensive Transport at least some advantage over this unique unit.

 

[s.bernbaum]

I don't know if there was any effort made to fix the stack on an island issue but the problem is inherent in CIV. I nearly always play maps with some sort of islands on them, usually in BUG only modded BTS. I have seen stacks auto transported as you describe now and then with the same result… they stay on the small island that they landed on forever. Similarly, whenever the AI lands a stack on an island and is victorious, the stack stays there forever. The AI does not seem to handle stacks on islands at all well.

 

[Andera412]

Thank you for bug spotting! I'll try to analyse them.
1. Try to use BUG Autolog next time. AI can have finansical problems with civic maintance, so he chages to No-cost civics. When AI declares war and move a lot of units to enemy territory, he can make random anarchy to keep gold.
2. Try to use debug dll. Does AI_handlestranded FAssert appears when Khmer does the turn?
3. All is normal in my Sevopedia. Maybe, you have installed any mod translation and translator hadn't kept variable in localization tag.

 

[f1rpo]

I've run the savegames in the debugger.
1. The civics have the same upkeep class, so this can't be right. Looks like this line in CvPlayerAI.cpp is responsible:

iValue += (iCities * 6 * iS * AI_getHealthWeight(-iS*kCivic.getUnhealthyPopulationModifier(), 1, true)) / 100;

Although the two civics have an UnhealthyPopulationModifier of 0, iValue gets decreased by 1 in the case of the current civic (iS=-1) and increased by 1 otherwise (iS=1). I think there's some small error in the HealthWeight function. The easy solution would be to add a guard:

if(kCivic.getUnhealthyPopulationModifier() != 0)

This should be added in any case because AI_getHealthWeight treats iHealth=0 the same as iHealth=1. For the one civic that has a non-zero UnhealthyPopulationModifier, Environmentalism, the current code seems to work.

Another issue is that the AI is too quick to switch when a civic has a higher total value than the current civic. It seems that a 5% difference in value can be enough. Should be probabilistic, i.e. switch with a small probability if the current civic is only slightly worse.

Update (14 Oct): The civics change when there is only a 5% difference in utility is actually a bug. I've added the fix to my list of changes that K-Mod might want to adopt here (the last one from October).
Once this is corrected, switching probabilistically arguably wouldn't be an improvement. If the new civic isn't clearly (30%) better, then the AI should wait for another civics change or Golden Age rather than switching after some random amount of time through a dice roll.

2. The AI is aware of the stranded units, but the two Galleys in AD 1200 are both loaded, one with combat units, the other with a Settler. Sury has no war plans, and the only site he wants to settle is unreachable for Galleys, so he should really unload these units and pick up the castaways. I've experimented with that a bit, but had trouble unloading the units; somehow, a Longbowman was always onboard when the Galley headed out.

Perhaps units shouldn't be teleported to a landmass where their owner doesn't have any cities. That said, it wouldn't necessarily be easier for the AI to pick up units from an owned island than from an unowned one.

BBAI has added the CvUnitAI::AI_pickupStranded function and K-Mod has modified it, so there has been an effort to solve such problems. And if I simply hit End Turn in the 1630 save, Sury does send a Galleon that picks up 4 units.

3. Strange that you can't reproduce this, Andera412. Some other Civilopedia texts that are outdated:
TXT_KEY_CONCEPT_CULTURE_PEDIA
"The first time that the city revolts, it will eventually return to the original owner's control. The owner may then try to pump up the city's culture to avoid a second revolution: but if the owner is unsuccessful and the city revolts again, it will flip and join the dominant civilization's empire."
Cities only flip on the third revolt in K-Mod. (And, regardless of K-Mod, the proper short-term measure is not to "pump up" culture, but to strengthen the garrison.)

TXT_KEY_BUILDING_SPACE_ELEVATOR_STRATEGY
TXT_KEY_CONCEPT_SPACESHIP_PEDIA
Say that the Space Elevator provides a 50% production bonus.

TXT_KEY_CONCEPT_GREAT_PEOPLE_PEDIA
Says that Artist generates 2000-6000 culture depending on game speed, but, in K-Mod, it also depends on the era.

TXT_KEY_BUILDING_INDUSTRIAL_PARK_STRATEGY
Mentions "an extra free Engineer", but the building grants two free Engineers now.

Transport: The more units are loaded into a single Transport, the higher the stakes in combat. So I'd tend to lower the cost to 100 instead of adding a 6th cargo space. East Indiaman and, for that matter, Carrack should have the same cargo capacity as in BtS, i.e. 4 and 2 respectively instead of 3 and 5. But an extra space for Transport would still be better than the status quo.

 

[vedg]

Another issue is that the AI is too quick to switch when a civic has a higher total value than the current civic. It seems that a 5% difference in value can be enough. Should be probabilistic, i.e. switch with a small probability if the current civic is only slightly worse.

One more issue is that the AI price for switching to another civic is too low. The cost probably doesn't event compensate for one turn of Anarchy, not to mention the opportunity cost for at least 5 turns and a possible another turn of Anarchy to return to optimal civics. I often gift techs to avoid tech trades between AIs or at the last turn of their research. Often I ask the AI to switch to some sub-optimal civics as part of the deal. I'd increase the cost of this civic switching depending on the impact of the Anarchy and the usefulness rate between the current civic and the switched-to one.

And if I simply hit End Turn in the 1630 save, Sury does send a Galleon that picks up 4 units.

What a coincidence! I declared war on Khmers in 1630 and so didn't notice their long overdue expedition. They really could use those troops in their long and slow war with Native Americans earlier. But they probably haven't bothered to build another Galley/Galleon during the landmass war, or use the existing (already loaded) ones for that.

Even though the AI is so slow to pick up stranded units, it tends to keep soldiers in naval transports inside cities and sometimes sends its loaded transport units with little escort during a war. For example, at the beginning of this short war with Khmers (1630-1675), they sent their 4 loaded galleons from Cahokia, which where promptly destroyed by my frigates. And later in this same game I have destroyed practically entire Korean army and fleet in a surprise amphibious attack of Seoul, because they prematurely loaded their Galleons for an invasion leaving only 4 weak unloaded city defenders. The AI could load their transports at the same turn their navy moves out from the city to its destination (unless the destination is so close that it can be reached and attacked in one turn).

 

[f1rpo]

One more issue is that the AI price for switching to another civic is too low. The cost probably doesn't event compensate for one turn of Anarchy, not to mention the opportunity cost for at least 5 turns and a possible another turn of Anarchy to return to optimal civics. I often gift techs to avoid tech trades between AIs or at the last turn of their research. Often I ask the AI to switch to some sub-optimal civics as part of the deal. I'd increase the cost of this civic switching depending on the impact of the Anarchy and the usefulness rate between the current civic and the switched-to one.

The difference in utility is accounted for more or less correctly I think, but the cost counted for (double) anarchy is way too low. I have a function "anarchyTradeVal" for this in my mod-mod that K-Mod could adopt:
GitHub link (might load for a moment because CvPlayerAI.cpp is a 1MB file) [Edit: link fixed]

What a coincidence! I declared war on Khmers in 1630 and so didn't notice their long overdue expedition.

Too little too late.

Even though the AI is so slow to pick up stranded units, it tends to keep soldiers in naval transports inside cities and sometimes sends its loaded transport units with little escort during a war. For example, at the beginning of this short war with Khmers (1630-1675), they sent their 4 loaded galleons from Cahokia, which where promptly destroyed by my frigates.

Well, Galleons can be escorts and Sury didn't have Chemistry for Frigates. I don't think the AI checks if its escorts can match the ships of the war enemy. CvUnitAI::AI_assaultSeaTransport might be the right place for this. Difficult to estimate the strength of the enemy navy though – mustn't be deterred just by their technology.

And later in this same game I have destroyed practically entire Korean army and fleet in a surprise amphibious attack of Seoul, because they prematurely loaded their Galleons for an invasion leaving only 4 weak unloaded city defenders. The AI could load their transports at the same turn their navy moves out from the city to its destination (unless the destination is so close that it can be reached and attacked in one turn).

That sounds like the proper solution. Hard to say how difficult it would be to change this. I've gotten my code for unloading to work, but this only addresses the specific issue of units not getting picked up. I'm still attaching my edits (tagged with "f1rpo") as they also address a problem with Pillage AI units forgetting that they're stranded and units entering ships that are en route to pick up stranded units. I'm adding these changes to my mod-mod, too, to see if they cause problems in other situations.

 

[vedg]

One more bug: in the attached save I sell Guilds to Pericles and ask him to switch to Org. Religion and Taoism. If in the trade screen I click on Org. Religion first and on Taoism second, then Pericles only switches to Org. Religion and forgets that he promised to switch to Taoism. He doesn't even switch to Taoism on the next turn (or the turn after the next). If in the trade screen I click on Taoism, then Org. Religion, he switches both to Taoism and to Org. Religion on the same turn.

 

[f1rpo]

CvPlayer::revolution has a bForce parameter that forces the revolution to happen despite the anarchy from adopting Taoism. I think the best solution is to add such a parameter to CvPlayer::convert too. I'd prefer to disallow trades that change both civics and religion unless the AI leader is Spiritual or in a Golden Age, but, to my knowledge, mods can't see which items are being offered on the trade table, so such trades would have to be rejected via the AI ("I'm sorry to say that no such deal is possible"), which could confuse players and wouldn't help in human-to-human trades (multiplayer). The game could remember the promised civics and religion changes and carry them out as soon as possible, but this would be quite a bit of work to implement.

So, I'd suggest the following code changes:

Spoiler :

To avoid changing the signature of CvPlayer::convert (DLLExport), add a declaration
void convert_(ReligionTypes eReligion, bool bForce);
to CvPlayer.h. Replace the start of CvPlayer::convert in CvPlayer.cpp with this:

void CvPlayer::convert(ReligionTypes eReligion)
{
   convert_(eReligion, false);
}

void CvPlayer::convert_(ReligionTypes eReligion, bool bForce) {

   int iAnarchyLength;

   if (!bForce && !canConvert(eReligion))
   {
       return;
   }

And in CvDeal::startTrade (CvDeal.cpp) under the TRADE_RELIGION case, replace
//GET_PLAYER(eFromPlayer).convert((ReligionTypes)trade.m_iData);
with
GET_PLAYER(eFromPlayer).convert_((ReligionTypes)trade.m_iData, true);

Edit: I've just noticed that Pericles goes through 2 turns of anarchy when adopting Organized Religion and Taoism in a single revolution. So it's really not much of a rules violation.

 

[Leoreth]

Why write code like that when you can just add the bForce parameter with the default value false?

 

[f1rpo]

Why write code like that when you can just add the bForce parameter with the default value false?

If CvPlayer::convert is indeed called from the EXE, then even an optional additional parameter isn't safe. For example, I've once added an optional boolean param to CvDeal::isCancelable, and got a crash upon canceling a deal through the trade screen, which was nasty to debug. @Nightinggale recently wrote a little article explaining (under "Why this is a problem") that the EXE will "present garbage for the new argument."

Upon closer examination, I doubt that CvPlayer::convert is ever called from the EXE. E.g. if I change my religion via the Religion screen, the call comes through CvNetConvert::Execute (CvMessageData.cpp), and if I search the EXE with a text editor, "convert@CvPlayer" isn't found. I'm not sure if this guarantees that there is no call.

 

[Nightinggale]

Upon closer examination, I doubt that CvPlayer::convert is ever called from the EXE.

It's not. I just generated the lists of functions called by the exe and pitboss. You should most likely add them somewhere easier to find than here though.

Keep in mind that the exe can also call virtual function, like 5th virtual function of class X. Doing so is not included in this list. This means you should not add/remove or change order of virtual functions and the same "don't change arguments" apply too. It's really annoying because I haven't figured out a way to tell which virtual functions the exe calls, just that it can call at least some of them.

 

[DarkLunaPhantom]

Wow, this is really terrible and I had no idea. Thank you both for this information!
I will have to recheck some functions because adding a parameter like this has been my approach to solving these kind of issues.

 

[Leoreth]

Oh, that is good to know, I never considered interaction with the exe could mess with us here.

 

[Nightinggale]

One trick to get around the exe requirements for functions is to not mod the functions itself, but rather rename the functions and mod the renamed one. The original function can then call the new function.

Example:

int functionA(int A)

Becomes:

int functionA(int A)
{
    return functionAInternal(A);
}

functionAInternal is then a new function unreachable by the exe meaning we can add whatever default arguments we want.

Another example of modding what the exe is calling is something I added recently to WTP:

int CvGameText::getNumLanguages() const
{
    // The exe will use this function for two purposes:
    // 1: number of lines in the language menu
    // 2: setting a new current Language ID will be ignored if the value is >= the return value here

    // This is a problem for our modded system if one or more languages are skipped in Languages.xml.
    // Say we have English and Tag. That's 0 and 25, but the number of languages is 2, meaning requests to change to Tag are ignored.

    // The solution to this is when changing the language, the bool CHANGE_LANGUAGE is set.
    // This means we know if the exe calls for the menu length or for verifying Language ID.
    if (STATIC_bChangeLanguage)
    {
        // New language being tested.
        // All languages will be lower than MAX_INT, meaning returning this value disables the unwanted test.
        STATIC_bChangeLanguage = false;
        return MAX_INT;
    }

    // The exe asks for the number of languages, which should be put in the menu (from Languages.xml)
    return STATIC_iNumLanguages;
}

It's a simple get function, but it returns different values depending on game state, in this case the value of a static bool.

In short: just because the exe is calling some function doesn't mean it's unmodable. You just have to pay extra attention and perhaps be unusual creative.

 

[Leoreth]

Yeah, that's essentially the pattern that prompted my original question.

 

[f1rpo]

[...] I just generated the lists of functions called by the exe and pitboss. You should most likely add them somewhere easier to find than here though.

Thanks a lot! I've gone ahead and, through manual searches, started a list of functions that the DLLExport could be removed from. There are going to be a few hundred of these. I'm not sure if it's wise to remove them in the end, as any oversights would introduce critical and hard-to-find errors. Perhaps just in some frequently mod'ed classes like CvPlayer. But I'd better start a new thread for such questions once I'm through with the list.

Keep in mind that the exe can also call virtual function, like 5th virtual function of class X. Doing so is not included in this list. This means you should not add/remove or change order of virtual functions and the same "don't change arguments" apply too. It's really annoying because I haven't figured out a way to tell which virtual functions the exe calls, just that it can call at least some of them.

It seems that the EXE doesn't call any of the CvCity::AI_... functions – K-Mod has added and removed several pure virtual functions in CvCity.h without any apparent problems. CvPlayer and CvTeam are definitely dangerous; not sure about CvUnit and CvSelectionGroup.

 

[Nightinggale]

There is a quick way to test if you removed too many. Download http://www.dependencywalker.com/ (DW)
Copy *.exe and *.dll from the install dir to assets in your mod. Open DW and open the exe file in assets. The left menu will show the DLL files loaded by the exe. Click on our DLL. To the top right is now is a list of functions the exe will call. The first column (called PI) will be green if the function is there and red if it's missing. Click on the column header to make the arrow point up. This will sort the list to make the red functions appear on top.

Remove DllExport, close DW (not the app, just the close in the File menu) and then open the exe again (first in recent files, also in File). This will update the red/green icons.

I think this is the way to go if you want to spend the time removing the unused DllExports.

It seems that the EXE doesn't call any of the CvCity::AI_... functions – K-Mod has added and removed several pure virtual functions in CvCity.h without any apparent problems. CvPlayer and CvTeam are definitely dangerous; not sure about CvUnit and CvSelectionGroup.

I think it was CvPlayer, which caused me problems regarding virtual functions and then I just haven't touch virtual functions since. A related note is that virtual functions became hype in the late 90s and it was the recommended approach for years (including when civ4 was coded). They aren't anymore because not only do they slow down execution, they also turned out to make it harder to make bugfree code. Even worse touching one part of the code makes it more likely to introduce bugs in what at the time seems like unrelated code.

Just cast the this pointer to the AI class. It works because all instances are also of the AI instance.

class CvPlayerAI;
class CvPlayer
{
....
inline CvPlayerAI* AI() {return (CvPlayerAI*)this;}
inline const CvPlayerAI* AI() const {return (const CvPlayerAI*)this;}

Add that to CvPlayer and it should allow you to use AI()->AI_whatever() instead of using virtual functions. Same with other AI classes. It's just important to remember that this will only work if all instances of CvPlayer is of type CvPlayerAI. It's not a general C++ solution.

The only time virtual functions might make sense is in tree inheritance. CvBaseInfo has a virtual deconstructor. This means if say CvUnit is freed, it actually free CvBaseInfo, but because it's a virtual function, it will call the deconstructor of CvUnitInfo, hence not leak memory. That's one of the few places vanilla use virtual functions in a way virtual functions were actually designed to be used. Virtual functions were never meant to be used like the AI functions are used.

 

[f1rpo]

There is a quick way to test if you removed too many. [...]

That's great! The tool and your instructions have already helped me fix an old bug in my current code that I had pretty much given up on. And the bug was
.......... an optional parameter added to a DLLExport. I kind of knew it had to be that, but I had been searching for it in CvCity.h, however, the culprit was CvMap::findCity.

In that case, I think will actually remove the DLLExports – and only post the list (which is complete now) once I've verified it in this way. That won't be until next week though, so never mind what I wrote about making a new thread; I'll let our moderator worry about whether this thread is going too far off topic.

[...]I think this is the way to go if you want to spend the time removing the unused DllExports.

I'm still not so sure about constructors, destructors and other virtual funtions with DLLExport. Perhaps there could be implicit calls that wouldn't be covered by your interface list and Dependency Walker? Though a test with CvGame.h suggests that these DLLExports can be removed: I've done it for the constructor, destructor, read and write and some pure virtual functions that are overridden by CvGameAI. Creating a game, saving, autosaving, loading, quickloading still work fine.
(Some constructors, destructors and other virtual functions do appear in the list; naturally these can't be removed.)

Removing DLLExport from an entire struct and adding it only to those member functions that are accessed by the EXE also seems to work. I've tried it for IDInfo and CvBattleRound in CvStructs.h, and combat and some turns on Auto Play didn't produce any crash. Though there may not be a point in cleaning up CvStructs.h; I've never even changed anything in that file ...

Just cast the this pointer to the AI class.

That's a good idea, wrapping the cast in an inline function.

[...] Virtual functions were never meant to be used like the AI functions are used.

Generally, deriving a human player class and an AI player class from an abstract base is not an odd thought, but of course Civ 4 doesn't have an abstract base class. What bothers me most about the software design though is the enormous size of some of the classes. The BtS CvPlayerAI has close to 17000 lines (not lines of code, but still). Insofar I'm thankful that they split CvPlayer and CvPlayerAI somehow.

 

[Nightinggale]

I'll let our moderator worry about whether this thread is going too far off topic.

I think we are way past the threshold for off topic, but at the same time it's one of the most useful threads I have seen in a long time. It will affect workflow and results for multiple mods. I know it has made me consider some more options for how to move on from where I am now.

Generally, deriving a human player class and an AI player class from an abstract base is not an odd thought, but of course Civ 4 doesn't have an abstract base class. What bothers me most about the software design though is the enormous size of some of the classes. The BtS CvPlayerAI has close to 17000 lines (not lines of code, but still). Insofar I'm thankful that they split CvPlayer and CvPlayerAI somehow.

I'm in the progress of adding CivEffects, which is essentially a cache in CvPlayer, which tells the effect from xml on that player. Something like an array of allowed units generated from civ info and owned tech infos. Quick lookup at runtime, but it also adds a lot of other benefits (and take quite a while to add). I'm considering changing it to implement it like this:

CvPlayerCivEffect
CvPlayer
CvPlayerAI

CvPlayerAI can access all, CvPlayerCivEffect has the AI() function, which allows accessing everything CvPlayerAI can access. In other words everything has access to everything. If that's the case, then why split it? In addition to gaining smaller files (easier to read/navigate), the answer can be summed up in public, protected and private.

Public is for everything. Protected is for everything inside any CvPlayer*. Private is for that file only.

The result would be that CivEffect variables can be made private. There are no set functions, meaning only one (small) file can actually write to the cache, though there should be some update cache public function. In other words it unlocks the data encapsulation model way better than vanilla and it should be easier to read, write and bugfix. I haven't tested it yet, but I think it should just fine and there is no reason to stop at just 3 classes if going with a design like this. While using virtual functions to "glue" the classes together hurts at runtime, using the AI() typecast should let the compiler put the classes together, hence reaching runtime performance equal to not splitting the classes.

 

[Leoreth]

I agree this discussion is useful, and applies outside of K-Mod as well. A lot of what is written here can probably be moved to the tutorials subforum, but I don't have the time now to review how to best put it together. If someone has a selection of posts for me to move (especially @Nightinggale), please write me a PM about it.