Exposing AI method to Python

[Leoreth]

I currently want to expose CvPlayerAI::AI_bestCivic() to Python. Context is that I want to give the AI some free techs, then let it choose some civics and script it to switch to them for free.

The problem is the Python wrapper in CyPlayer.cpp, which wraps around a CvPlayer instance, not its CvPlayerAI subclass. So if I want to do m_pPlayer->AI_bestCivic(), the code won't compile. So far, so logical.

The thing is, the sources already come with some AI methods exposed to Python. From what I can tell, the only thing missing was to declare the AI method as a virtual method in CvPlayer.h (although I have no clue what that means) and define it as 0, i.e. "virtual CivicTypes AI_bestCivic() = 0".

Now the code will compile, but when I actually launch the game, I get a CTD during runtime, along with a "failed to compress game data" error message (I suppose when the game tries to autosave). Note that this is with only the declaration in CvPlayer.h added, without the method actually being called inside the CyPlayer wrapper.

I assume that my new declaration has now overridden all calls to AI_bestCivic() in CvPlayer with a null pointer, which explains the crash. But then, why does this work for all the methods already defined as virtual ... = 0 in CvPlayer.h, and similar corresponding AI classes? How can I actually correctly expose an AI method to Python?

 

[Nightinggale]

Say you add AI_bestCivic() to CvPlayer and CvPlayerAI. You then have a pointer to CvPlayer and call AI_bestCivic(). The following will happen:
1: no virtual keyword
the function will be determined at compile time. Since it's in CvPlayer, it will use the CvPlayer function because it has no knowledge of CvPlayerAI.

2: virtual keyword
The function to call is determined at runtime. The class will contain a function pointer, which is set in the constructor (behind the scenes). The CvPlayer class sets the pointer to CvPlayer::AI_bestCivic(). After that because CvPlayerAI builds on top of CvPlayer, CvPlayerAI runs the constructor and overwrites the function pointer to point at CvPlayerAI::AI_bestCivic().

This means even though you have a pointer to CvPlayer, calling AI_bestCivic() will make a call to CvPlayerAI, but only if it's virtual.

Note: today people are mainly against using virtual, both for code design concerns and it causes extra code to execute and the compiler loses some options for optimization.


I can't tell why you crash (you would have to debug to determine that), but instead of setting the function to =0, you could make a function, which returns a default value. That way it should not be able to downright crash if it ends up calling the wrong function. Somehow I feel like I have only given half an answer, but I can't really be more specific without knowing anything more specific about your code.

 

[Leoreth]

Okay, that explains how the virtual keyword works and how Firaxis designed their code in general. I was always a bit confused by the fact that the superclass (e.g. CvPlayer) could call methods from its subclass (e.g. CvPlayerAI), but I suspected some kind of C++ magic like that.

And I would agree that it's bad design. In fact I would question that CvPlayerAI is really a subclass of CvPlayer, composition would probably have been the better approach.

Now for my concrete problem: I have omitted my code because it is not relevant to the crash. It is never called. CvPlayerAI::AI_bestCivic() already is part of the base game sources, so it's not that I messed up its implementation. Really all I did was add the virtual declaration in the CvPlayer.h header. Having it in there will cause a crash (now that I know how virtual works I get why it's a runtime error), removing it again will prevent the crash. What makes this weird is that dozens of of AI methods are already declared in CvPlayer.h the same way.

 

[Nightinggale]

You could try to typecast the this pointer to CvPlayerAI and call the function that way.

CivicTypes AI_bestCivic() {return static_cast<CvPlayerAI*>(this)->AI_bestCivic();}

Or you could add a generic approach to call all AI contents.

CvPlayerAI* getAI() const {return static_cast<CvPlayerAI*>(this);}

This would then allow code like

m_pPlayer->getAI()->AI_bestCivic();

A 3rd approach would be

static_cast<CvPlayerAI*>(m_pPlayer)->AI_bestCivic()

All use the same idea: since you know the game doesn't allocate any CvPlayer in memory, only CvPlayerAI, then it will be safe to take a CvPlayer pointer and tell the compiler to use it as a CvPlayerAI pointer.

Maybe the issue is not AI_bestCivic() in itself, but rather the addition of a function pointer. If the exe for some reason assumes the start of CvPlayer to be something specific (that would be bad coding) and you add a function pointer at the top, then it would create an offset issue. I'm not aware that this should be a problem, but then again I'm not aware that adding a virtual function should crash the game.

Fun fact: using either of the solutions mentioned here will avoid allocating the pointer, hence save 4 bytes in each allocation of CvPlayer(AI). Also it's more friendly to the compiler and I would expect the code to be faster than when using virtual. Virtual is really designed to be used in base classes where multiple classes inherit it and overwrite it with their own code. Vanilla is a mess and the only virtual function I have seen being used as intended for the virtual keyword is the virtual deconstructor in CvInfoBase. The civ4 codebase is not the place you should look to get an idea of how to use virtual correctly.

 

[Tholal]

You need to include the parameter.

ie,

virtual CivicTypes AI_bestCivic(CivicOptionTypes eCivicOption) const = 0;

 

[Nightinggale]

virtual CivicTypes AI_bestCivic(CivicOptionTypes eCivicOption) const = 0;

Well spotted. C++ supports overloaded functions, but all the functions have to be in the same class, meaning you can't have AI_bestCivic isn CvPlayer and CvPlayerAI where they have different arguments. What happens if you do that is you hide the function from CvPlayer when you have an instance of CvPlayerAI, which seems ok in your case. However at the same time you made the (hidden) CvPlayer function virtual, meaning you now hide the function pointer.

My guess is that CvPlayer places all the member variables according to offsets when reading CvPlayer. When CvPlayerAI sets the offsets, it fail to see the function pointer, skips it when setting variable offsets and suddenly CvPlayer and CvPlayerAI no longer agrees on where in the memory each variable is set. Naturally this will never go well.

Now the code will compile, but when I actually launch the game, I get a CTD during runtime, along with a "failed to compress game data" error message (I suppose when the game tries to autosave).

My guess is that the offset issue means the length of some list is set incorrectly and suddenly it will read/write say the array telling if the player has techs. It should be a bool for each of the now 1273638 techs (or whatever is left over in memory due to using uninitialized memory).

What remains is the question of why the compiler didn't object to undefined behavior like this. All I can say is that it's an old compiler and it has far fewer checks than modern compilers. It's not the first time I have seen it accept something it shouldn't have accepted.

 

[Leoreth]

Oh, my declaration includes the parameter, I just omitted it here for simplicity.

I think the bottom line of this thread is that I'm trying to do something that has side effects I don't want. So I'll work around it somehow. Thanks for the help, at least I know I didn't do anything obviously wrong.

 

[Tholal]

Now for my concrete problem: I have omitted my code because it is not relevant to the crash. It is never called. CvPlayerAI::AI_bestCivic() already is part of the base game sources, so it's not that I messed up its implementation. Really all I did was add the virtual declaration in the CvPlayer.h header. Having it in there will cause a crash (now that I know how virtual works I get why it's a runtime error), removing it again will prevent the crash. What makes this weird is that dozens of of AI methods are already declared in CvPlayer.h the same way.

Did you also add the matching function into CvPlayer.cpp?

I think the bottom line of this thread is that I'm trying to do something that has side effects I don't want. So I'll work around it somehow. Thanks for the help, at least I know I didn't do anything obviously wrong.

You should be able to expose this function without cascading issues. It sounds like you're just having a code issue. That crash message is usually related to reading/writing the savegame, though I'm not sure how your change is causing it.

Do you have your work in progress available for viewing somewhere?

 

[Leoreth]

Did you also add the matching function into CvPlayer.cpp?

Nope, following the pattern established for the other AI methods, I set it to 0 in CvPlayer.h. I don't want to implement the function in CvPlayer.cpp, I want to make the already existing implementation in CvPlayerAI.cpp accessible.

You should be able to expose this function without cascading issues. It sounds like you're just having a code issue. That crash message is usually related to reading/writing the savegame, though I'm not sure how your change is causing it.

Do you have your work in progress available for viewing somewhere?

I could make a branch of the BtS DLL sources Git repo I created a while ago, give me a moment.

 

[Leoreth]

Here, this reproduces the issue with an otherwise empty mod. Clone this BtS sources repo into the Mods folder and check out the "aiexpose" branch. As you can see in this diff, I have only added that one line in CvPlayer.h. If you start the mod with a DLL built from this code, the game will start on crash. Delete the DLL (or comment out the line and recompile), and it will start normally.

 

[Tholal]

This sort of thing is not really my strong suit, but a here are a couple of suggestions to try.

1) Add a CivicTypes typedef line at the top of CvPlayer.h
2) Try doing a DLL Export (not really sure what that does, but the only other declaration in the header file that uses CivicType for the function value type is referenced that way)
"DllExport CivicTypes getCivics(CivicOptionTypes eIndex) const;"

 

[Nightinggale]

Just like the the DLL exposes some functions to python, the DLL exposes some functions to the EXE. The approach to do that is much simpler though. It just need to declare the function with the DllExport keyword. It makes no sense to add it to your own functions because the EXE has no code to access your function, meaning you gain nothing from exposing it.

I cloned the repository and it plays flawlessly. It's just my luck that I can't get it to crash when I want to. It's usually the other way around
The code looks fine to me. I disagree with the design with all the virtual functions, but that's another story. It should work.

 

[Leoreth]

Did you compile the code? I didn't commit my DLL.

 

[Nightinggale]

Did you compile the code? I didn't commit my DLL.

I noticed and it wouldn't matter anyway because I started out by compiling a debug DLL and attached the debugger to see what happened. No crash, no failed asserts, no nothing. Just regular gameplay. I then tried again with an optimized DLL thinking it could be a bug in the optimization code, but no, it still works just fine.

 

[Leoreth]

Then I remain confused.

 

[Nightinggale]

For some unknown reason you seem to have a problem adding a virtual function. Since the problem can't be located, the answer would be to write code to call AI_bestCivic without using virtual.

Try this:

//virtual CivicTypes AI_bestCivic(CivicOptionTypes eCivicOptionType) const = 0;
CivicTypes AI_bestCivic(CivicOptionTypes eCivicOptionType) const;

And in CvPlayer.cpp

CivicTypes CvPlayer::AI_bestCivic(CivicOptionTypes eCivicOptionType) const
{
   return static_cast<const CvPlayerAI*>(this)->AI_bestCivic(eCivicOptionType);
}

I didn't test it other than compiling it, but it should do precisely what you need: provide access to CvPlayerAI from a CvPlayer pointer.

 

[Leoreth]

Yeah, I think I'm going to do that. Thanks again!

 

[Nightinggale]

I just did a new test.

      CvPlayerAI* AI()       {return (      CvPlayerAI*)(this);}
const CvPlayerAI* AI() const {return (const CvPlayerAI*)(this);}

Adding this to CvPlayer.h allows accessing AI functions like this:

AI()->AI_bestCivic(eCivicOptionType)

The benefit of this approach is that you can add the AI()-> prefix to everything and that way access everything in CvPlayerAI without additional CvPlayer code and without messing with virtual. I'm now seriously considering replacing all the virtual functions to this myself since it saves around 200 bytes for each player and (more importantly) executes the code faster. Since the functions are inline and just returns a typecasted this pointer, odds are that the compiler can figure out to optimize the functions out completely and just make the call into CvPlayerAI.

Note: I haven't spent a long time testing this, but initial testing indicates that it works as intended. Clearly it needs to be well tested before starting to remove the vanilla virtual functions. Your level of confidence needs to be greater when replacing known working code than when you just add new code.

In case you are wondering about overloading with the same argument: non-const pointers/functions will call a non-const function. It will then call a const if it fails to find a non-const matching the name/arguments. Const pointers/functions will go strait for a const function and not check for non-const. Because of this the compiler can tell them apart and it will always pick the function, which makes the code work, though it picks the non-const when both will do.

 

[Leoreth]

That's great! Not sure when I will find the time to clean the DLL like this, but it definitely looks like it's worth the effort.

 

[Leoreth]

How did you manage to include CvPlayerAI.h in CvPlayer.h without cyclic dependencies? I cannot declare a method returning a CvPlayerAI* pointer without including its header (compiler fails with "missing storage-class or type specifiers").

What am I missing? I'm obviously not very good with C++ beyond surface level stuff.