DLL coding thread

I decided to start a new thread on coding issues. This is not about anything related to the players. Instead it is announcements and discussions on how the internals of the DLL should work.

I pushed an update for the cache for getDefineINT(). Now that XML files are read differently we can set the cache in a more efficient way as we know everything is set before main menu.

I decided to take this one step further and reorder where in the array each int is. If a variable is often used together with another variable, then those two can benefit from being next to each other (hardware cache). We currently have 306 variables in the cache meaning analyzing all of them manually would mean spending way more time than I plan to do on this. Instead I made a simple counter, played a bit and then read the counter. This tells me how many times each variable is called during my gaming ad the plan was to sort by this list to group often called together. At least this will prevent often called ones from being together with really rarely called ones. This gave me a top 10:

This reveals something interesting. More than 96% of all calls to the cache goes to the same 3 variables
Also only 1% of the calls goes to those 296 variables, which isn't in top 10.

Now I have a better idea of how to optimize this. The top 2-3 variables get hardcoded into the DLL.

CIVICOPTION_INVENTIONS: this really needs to be hardcoded, but I'm not sure which would be the best approach yet.
DEFAULT_YIELD_ARMOR_TYPE can be part of the yield enum.
AUTORESEARCH_ALL could be a define set in Makefile.settings or a header as it is fairly rarely used with a non-default value.

If those are hardcoded, then I think the rest will be called rarely enough not to matter with order and we can keep the current one as it is human readable with comments about file of origin etc.


If you feel like knowing more about memory management optimization:

I hardcoded the first two and committed this change. I also updated col2071 to make the XML apply to these new conditions.

XML_DEFAULT_YIELD_ARMOR_TYPE is now set in yield enum while XML_CIVICOPTION_INVENTIONS is set in civicOption enum. The latter is still in XML as it minimizes the change, but it is assumed to be first and will assert on startup if it isn't.

Ok, on the issue I brought up in the Trade Screen thread about my new Widget. I did some investigating as I didn't know exactly how widgets are set up. I found the below code in CvStructs.h. Is this where we can add new arguments like an int m_iData3? Or better yet could we add a CvWString type? That way all we have to do in Python is pass the Screen text, such as "showSpiceRouteScreen", to the Widget code and we can thus pop up any screen we want with that one widget?

github said:

Fix the infinite loop in CvPlayer.cpp

Click to expand...

I changed that line and didn't notice the missing = (mainly due to search and replace). However it was missing before I touched the code meaning the function was already broken. It just managed to conceal it better when inventions was last.


CvWidgetDataStruct sounds like it is a place to store arguments, though you should notice DllExport. This mean the exe could expect the struct to look like it does. The problem is that if the exe reads from it, it get the pointer to the start and knowing the size of each variable it adds to this pointer to reach the variable it needs. This mean to make it work we need to keep the vanilla variables' offset. In other words we can add new variables below vanilla, but not between vanilla code. Some enums have the same issue. Often this isn't a problem, but I have run into problems with this and I know other people has as well.

What I think you are saying is that you want to add a variable to this struct. That's easy, but you need to add a variable to the arguments in the python interface as well. I see no reason why it shouldn't work. If you want to make this the easy way, then add a new python interface with the extra argument and keep the existing one untouched as this will ensure the existing python code will continue to work without modifications. However do consider what to do about the new variable in case it isn't set from python (presumably it should be set to 0).

As for type of argument, I would prefer a number rather than a string as C++ handles numbers a lot better. I can't think of a good reason to get a string from python as it is likely part of CvEuropeInfo and fetching it from there is better than sending it to python and back. However I'm not fully aware of what you are planning to do and a string might make sense.

Nightinggale said:

I changed that line and didn't notice the missing = (mainly due to search and replace). However it was missing before I touched the code meaning the function was already broken. It just managed to conceal it better when inventions was last.

Click to expand...

This is actually hilarious. This part of my code is old (update Update 1.1b), and I can remember I first put the new CIVICOPTION_INVENTIONS at the top of the list, but it caused the game to freeze. So, I moved it to the end and it worked fine. I didn't know how to do debug back then or I may have figured this out. I agree

Nightinggale said:

As for type of argument, I would prefer a number rather than a string as C++ handles numbers a lot better. I can't think of a good reason to get a string from python as it is likely part of CvEuropeInfo and fetching it from there is better than sending it to python and back. However I'm not fully aware of what you are planning to do and a string might make sense.

Click to expand...

I'll explain what I am wanting to do. In CvUnit.cpp when a unit reaches Europe it will auto show the Europe Screen, that way you don't forget about it. Anyway, it does this through the CvPopupQueue. I added a check for what type of Europe Screen should be popped up. In the EuropeInfos XML you can add the screen show name, like "showSpiceRouteScreen" and the code gets the CvWstring from that info and uses it to find that specific Python screen.

I am saying we can send the CvWstring info to the DLL of my new Widget that opens screens and then you wouldn't have to do anything else. So, unless we convert all new screen types to integers a string would work best here I believe.

Kailric said:

In the EuropeInfos XML you can add the screen show name, like "showSpiceRouteScreen" and the code gets the CvWstring from that info and uses it to find that specific Python screen.

Click to expand...

The int will then be the index of EuropeInfo and the receiving end can use this info to loop up the string. Perhaps the index is already present?

Nightinggale said:

The int will then be the index of EuropeInfo and the receiving end can use this info to loop up the string. Perhaps the index is already present?

Click to expand...

Right, that's the way I am doing it now. But if we use this Widget to open other None Europe Screens there will be no EuropeInfo. In fact there maybe no info at all. I am saying this Widget can bet used to open any new screen we want to add without having to add a new Control Type to the dll, "If" we can pass a CvWstring.

I'm looking into just-in-time arrays again. I managed to make length and default values const. This mean those two values are set in the line the jit array is declared (or initializer list if it is a class member) and then they can never be changed again. This prevents accidental changes (which could cause havoc or crashes). It also helps the compiler as it knows that once it read the variable, it can keep a local copy, which can be faster than checking the original as the original is known not to have changed.

I have to look into declaration of YieldEquipmentAmount cache as this is the only remaining code, which doesn't comply with the const declaration. This made me wonder. It stores an int for each yield telling the number of yields used for a profession. I wonder if it would be ok to change that to an unsigned char. This would allow 0-255 yields of each kind for a profession. Would it be realistic to assume more than 255 yields for a profession? We could make it a short if needed, which goes between -32k and 32k. However it uses twice as much memory.

I also noticed it uses YieldArray. I later added YieldCargoArray, which avoids the virtual yields. We could save a bit of memory here, but more importantly we would save time in often called functions if we can make the loops run fewer times. The question is if it would ever make sense to use virtual yields to make a profession, like using crosses for a missionary.

Maybe it would also make sense to reduce the list to only the yields being used. If we look at RaR it uses just weapons, horses and tools for professions, which is less than 10% of the total number of yields. This could be autogenerated and it could be set in a yield header file. The header file would provide the best performance, but it might need recompilations, which would otherwise be unneeded.

I also consider removing alternative equipment as it would simplify the code a great deal. I never really liked it as the alternative is to just make more professions.

The reason why I care about this part of the code is because the AI calls it a huge number of times. I once thought it was num units*num professions, but that appears to not even being close to the actual number of calls.

Nightinggale said:

I also consider removing alternative equipment as it would simplify the code a great deal. I never really liked it as the alternative is to just make more professions.

Click to expand...

Hold up, don't go there just yet, I am sure we can work something out The whole reason I created alternate equipment is because I didn't like the idea of having a multitude of professions. Should I do, Spearman in leather tunic, spearman in scale tunic, spearman in chain tunic, or spearman in plate tunic? That's just to many tunics. It made more sense to have your Light Spearman simply equip the best gear for his profession and the player has no worries about it. If this part of the code is really slowing things down that much I'm sure something can be worked out.

I'm not going to remove it right now and I will certainly not change something like this without a general agreement on goal. However I have wondered about this for a while. The current code is bad from a coding point of view, meaning it would be better to find another solution.

At runtime it would be good to have one profession => one yield cost. This mean each set of armor should be a profession of their own at runtime. Runtime is likely the key here. Runtime is how it should look inside the DLL after XML is loaded, which mean w have two places to look for a solution in.

Say we add a new XML tag with "parent profession". On load it will clone the parent and then read just a few selected tags yield cost (precisely what doesn't matter for the concept). This approach allows us to have virtually identical professions with just a few differences while we only need to update one of them. The "parent profession" should be stored in CvProfessionInfo as this allows merges in GUI, like in the pedia. The AI will view them as unrelated professions, which will likely be an improvement for vanilla or near vanilla code.

For this to work well we should likely make a template "overwrite this" profession to copy paste into the file, which is a dead giveaway that the fields will not be read. Comments would likely also be a good idea.

One bonus from this approach compared to alternate equipment is that there is no upper limit to how many children a profession can have. We can make a swordsman with no armor and then have 3 "sub professions" for 3 different kind of armors.

I thought about making a list in profession with a list of sub professions. However that appears to be a bit more tricky to code an XML loader for. Maybe if I sit down and read the code and think hard about it I can come up with something to allow this. The problem lies in the first XML run. It only loads BasicInfo, which mean it will not allocate room for the extra professions. Somehow it should read BasicInfo AND subtypes and allocate a new profession in the vector for each sub profession. This could be a bit tricky but not entirely impossible.

I didn't like the idea of having a multitude of professions. Should I do, Spearman in leather tunic, spearman in scale tunic, spearman in chain tunic, or spearman in plate tunic? That's just to many tunics. It made more sense to have your Light Spearman simply equip the best gear for his profession and the player has no worries about it. If this part of the code is really slowing things down that much I'm sure something can be worked out.

Click to expand...

At runtime it would be good to have one profession => one yield cost. This mean each set of armor should be a profession of their own at runtime. Runtime is likely the key here. Runtime is how it should look inside the DLL after XML is loaded, which mean w have two places to look for a solution in.

Click to expand...

How about going back to the idea of letting the equipment be a normal Promotion like its is in most other mods such as FfH and MoM, instead of intermingled with the whole profession system? You can already let Unitclasses and/or Professions be prerequisites for whatever Promotions you like, and can mod a variety of indivdual effects for Promotions a lot more flexibly, while not needing to worry about modding tons of possible combinations between Professions, sub-professions, and AlternateEquipments.

(BTW here's an awesome set of Promotions and Equipment-as-Promotions I came across while combing through platyping's python threads, its worth a look if you're interested in easily adding any of these kinds of effects. http://forums.civfanatics.com/showthread.php?t=482337 )

The problem with replacing professions with promotions is that different professions costs different yields (or at least they can) while one profession with different promotions always cost the same yields. We need a system which takes yields into account, which mean we can't use anything from yieldless civ4.

I couldn't decide how much memory we need for yield demand and added this

CvPlayer.h said:

// cache CvPlayer::getYieldEquipmentAmount - start - Nightinggale
// set amount of memory allocated to store yield cost for professions (each yield)
// unsigned char: 0 to 255
// unsigned short: 0-65535 (that should certainly be enough!)
typedef unsigned char ProfessionYieldCost;
// cache CvPlayer::getYieldEquipmentAmount - end - Nightinggale

Click to expand...

Now the code works and it isn't a problem if we need more memory later. In fact we can even make the size change according to mod. Alt equipment is always a short as it can be negative.

As for how to generate more professions on the fly:
We can add a bool argument to CvXMLLoadUtility::LoadGlobalClassInfo(), which it passes on to SetGlobalClassInfo() (which also needs to get a bool argument added). SetGlobalClassInfo() is then aware that the class can have subtypes and will not call CvInfoBase::read(). Instead it calls a new CvInfoBase function, which calls read and then it adds to the vector. BasicInfo and parent should be set to all. The parent should have a is_parent(-2?) set as parent.

CvProfessionInfo::read() should then function normally except it doesn't call CvInfoBase::read() (it already has those info).

Once the sub profession array appears (last), read does one of the two things:
1: if it is the parent, then for all profession infos, which has the current one as partent: call read with the same argument(XML pointer)
This will make the sub profession read from the very same place in the XML and make a complete clone, except for BasicInfo and parent.

2: if it isn't the parent, then look into the array, find the place where its own type is written and read those data. This should overwrite some of the already assigned data and the profession is no longer identical to the parent.

This approach can be used for other info classes as well.

The only missing part is the assert to prevent name clashes. The idea in the assert check will no longer work. However we can live without the check for professions if needed.

Well, I don't rightly understand all your saying, but the main thing is we want Units to be able to equip the new gear types or why even have them and at the same time we don't want to have to sort through a long list of professions.

I am guessing the code in question is the canHaveProfession as I would imagine the AI calls that a bunch. The Alt equipment is only for military so maybe that part of the code could be skipped if the AI isn't searching for a military profession.

The game plays fine as it is, even on my old system, so performance doesn't seem like an issue even with my not so conventional codding habits

The idea is like this:

In XML we have
PROFESSION_A
PROFESSION_B

In PROFESSION_A, there is two sub professions called
PROFESSION_A2
PROFESSION_A3

On read, it will read:
PROFESSION_A
PROFESSION_A2
PROFESSION_A3
PROFESSION_B

the sub professions will then clone PROFESSION_A on read, except it will overwrite specific parts with whatever is written in the sub profession array.

Kailric said:

Well, I don't rightly understand all your saying, but the main thing is we want Units to be able to equip the new gear types or why even have them and at the same time we don't want to have to sort through a long list of professions.

Click to expand...

This is why I said there should be a profession enum in the profession telling who the parent is or if it is a parent itself. This can be used to merge professions in the GUI while keeping them separate in game mechanics.

Kailric said:

The game plays fine as it is, even on my old system, so performance doesn't seem like an issue even with my not so conventional codding habits

Click to expand...

Have you ensured that the AI behaves correctly with alt equipment? How do you handle 3 types of armor for one profession? How big a game have you tested on?

Also having a simple game engine design makes it easier to write other expansions later.

Ok, I better see what you are talking about. So, game wise, subprofessions would not appear on the list when you choose a profession. You would simply select your profession then if the code detects a Subprofession that it can use it will use that one instead? That sounds like a good system to use. Subprofessions could be marked as such so they don't appear in the Choose Profession list. This sounds like a better way, and this way you could even have different Graphics for the Subprofessions as Graphics are tied to the Professions. Starting to like this idea

Nightinggale said:

Have you ensured that the AI behaves correctly with alt equipment? How do you handle 3 types of armor for one profession? How big a game have you tested on?

Click to expand...

Well, the code automatically applies the different equipments so the AI doesn't even have to think about it, nor does the player. I liked that part of it all because it does eliminate the micromanagement. But I do have several built in AI cheats when it comes to equipment until I can go back and actually make them think.

Great success. I managed to clone professions





I wrote this
Boom. I had 4 peasant professions, all alike, but with different descriptions.

The schema looks like this
We can copy paste more data in from professions as we like. It's perfectly happy with reusing the very same element types.

Loading in the DLL is like this:
Again lines can be copied directly from profession code as it knows within the {} it is the sub being read and not the main one. The red lines are needed as this makes it move back into the main one, ready for next profession. The green line is the stuff we modify. Notice that the unique type has already been set at this point as the only thing, which differs from the parent.

It turned out that coding this was harder than expected, but easier to add data to once it works. One really nice bonus is that I wrote this generic and the functions doing the actual work is in basic info. This mean we can add this to any info xml if we like. Imagine buildings. We have a blacksmith's house and then we make a sub house where we define production for each unit and max number of units. Other stuff like which yields to turn into which yields can then be reused.

Now I need to clean up the code and add some code to make professions aware of parent profession relationships.

So, in the XML PROFESSION_COLONIST2 would be an actual profession as well as a Subprofession correct? We would need a way to display this information. There are about 15 military professions at the moment, and when you look at the pedia it displays their alternate equipment, but this is suppose to replace alternate equipment right? So, how should this info be listed, or should we list all professions? I think that would be too much. We could have it listed as "Upgraded with 25(Leather Armor Symbol)?

On another note; I am excited thinking about how we can now set up what LeaderHeads can use what TradeScreens, and have Leader specific Trade Screens. I am so ready to be done with these "corrections" we are making so we can start to work on the fun stuff

Kailric said:

So, in the XML PROFESSION_COLONIST2 would be an actual profession as well as a Subprofession correct? We would need a way to display this information. There are about 15 military professions at the moment, and when you look at the pedia it displays their alternate equipment, but this is suppose to replace alternate equipment right? So, how should this info be listed, or should we list all professions? I think that would be too much. We could have it listed as "Upgraded with 25(Leather Armor Symbol)?

Click to expand...

Right now they are two unrelated professions. I plan to add a variable to change this. It should be:
-1: not parent and not a sub
ID of parent (indicating a sub)
something to indicate it's a parent. First I thought of -2, but then I wonder if we could add more information. I came up with this.
We use 16 bit for this variable.
9 bit for ID of first sub
1 alway true bit (is parent bool)
That leaves 6 bit to tell the number of subs. As this is signed we can use 5 with no problem. This allows room for 512 professions and 32 sub professions each (subs count in the 512 too). That should be enough. If not, then we can always use an int later. This will open up for using millions of professions

Using this info, we can from those 16 bits tell:
it's a parent
ID of first sub
ID of last sub (ID of first+num subs)

This translates into the functions:
isParent()
isSub()
getFirstSubID()
getLastSubID()
getParentID()

We know all IDs between first and last are also subs for the parent. This mean the last two functions will likely be used as init and end values for loops.

Those functions should be enough to make useful merges in the GUI (pedia etc).

I pushed the ability to make subclasses of professions. There aren't any getParent() or similar functions yet, but the data for those are generated. I decided to store this data in an unsigned int and I'm only using around half of it. Next step is to move all 11 bools in CvProfessionInfo into this int as it has enough room for it. Not only will it reduce the memory usage of the class, it will also make a hotspot in the allocated memory when looking at number of calls to read the data. Both are good for hardware cache.

To get this working I made new bitmask functions to store multiple bits in an int. We can now do stuff like storing a 3 bit variable on bits 4-6.

EDIT: I decided on a new goal. I split out CvProfessionInfo from CvInfos.h. This mean whenever it is changed, it will only recompile the cpp files actually using profession info, not all files using info (which is almost all files). In the long run I plan on splitting out buildings and units as well, possibly others. There are quite a lot of different kind of infos and I will only bother with those likely to be changed relatively often.

I pushed the improved (smaller) profession info. I introduced a new type of code, which is actually rather interesting:
It's an assert, but it works at compile time instead of runtime. Quite useful for a different kind of checks. In this particular case I verify that the code will not try to use more than 32 bit in an unsigned int. As the compiler has the info to detect this, then the compiler should complain instead of waiting for this to maybe trigger at runtime.

I noticed something else. Free Peasant is replaced by Peasant for certain civs. Looking at the two units it looks like they are very much alike except for the fact that they use different graphics. This gave me an idea: why not make sub units as well? This will allow us with very few lines to make the same unit for different civs and change settings for all such units at the same time to keep game balance.

This also gave me another idea. Adding prev/next in pedia to cycle sub professions (units). I wonder if this can be written in a generic way to allow the same code to be used for both units and professions and whatever we add sub classes to in the future.