DLL coding thread

[Nightinggale]

CvCity::doGrowth() loops civics to get random growth units. I'm not particularly happy with this code as I don't like how it handles randomness. If we have 50% change of a blacksmith and 2*25% chance of a farmer, then without calculations we would assume to get equal amount of each. However the odds for getting a blacksmith is 33,8% while getting a farmer is 38%. If we give the farmer 50% too, then the odds for each unit will be 37,5%. The second calculation is doable even without an advanced education, while the former (current setup) is far from trivial. It is far beyond anything I would even attempt in my head and while I would assume farmers to be more likely, I would not attempt to estimate how much based on a calculation in my head.
If we remove the blacksmith, then the odds for getting a farmer will be 50% if we stack, while it is 43,75% with the current code.

Because of this I propose caching the odds for each unit to allow civics to stack effects in a manner people is most likely to expect.

 

[Kailric]

That would be silly. I think I misunderstood you as I was talking of the player allowed cache. Now I took a closer look at what you actually did and it's a civic info code only. I don't actually see the problem. Maybe the problem is that you commented out the code

I commented it out because it did not work and I didn't want non working code in the dll.

I do have a few comments on the code, which is likely just confusing naming. iI and i aren't good variable names. iUnit and iUnitClass would be better. iI is defined elsewhere. It looks like Firaxis preferred to reuse the loop variables in multiple loops. However it helps the compiler if you declare the int each time like for (int iI = 0;. You looks for positive kCivicInfo.getAllowedUnitClassImmigration and then you print TXT_KEY_CIVIC_PREVENT_UNITCLASS_IMMIGRATION. Negative allow prevents and positive enables. All of this are minor and shouldn't prevent the code from working.

The problem is "if (kCivicInfo.getAllowedUnitClassImmigration(iI) > 0)" never returns positive as I tested this in debugger.

kCivicInfo.getAllowedUnitClassImmigration(iI) > 0 is not set up the same as the others as it just returns, return m_jaAllowedUnitClassImmigration.get(i);, and the others CivicInfos do a return m_aiAllowsUnitClasses ? m_aiAllowsUnitClasses : 0;. I tried using it the same as getAllowsUnitClasses(), but it doesn't seem to work the same.

EDIT: why change m_iCityPlotFoodBonus?...
As for simplicity, what you removed couldn't be simpler. It sets the variable to a default value, then it loops all civics and modifies the variable according to the player owned civics. Think about it.

I removed it because I was trying to test out that specific feature and was having those game freezes, and the Dock unit code wasn't working, and the updateInventionEffectCache() wasn't set up for swapping Civics, and it only takes a couple minutes to set up iCityPlotFoodBonus to work like vanilla code, and I was attempting to remove all problem causing variables, and I wasn't 100% sure how updateInventionEffectCache() worked (I see better now though).

Remember I am not a coder, but a modder

 

[Nightinggale]

I commented it out because it did not work and I didn't want non working code in the dll.

Oh the code is already committed. In that case I can take a look when I'm done with the growth stuff.

The problem is "if (kCivicInfo.getAllowedUnitClassImmigration(iI) > 0)" never returns positive as I tested this in debugger.

This might be a silly question, but did you ensure that you actually added a class to XML and that the player has the civic in question? When code doesn't work as expected, such trivial stuff should be the first to be checked.

kCivicInfo.getAllowedUnitClassImmigration(iI) > 0

is not set up the same as the others as it just returns, return m_jaAllowedUnitClassImmigration.get(i);, and the others CivicInfos do a return m_aiAllowsUnitClasses ? m_aiAllowsUnitClasses : 0;. I tried using it the same as getAllowsUnitClasses(), but it doesn't seem to work the same.

inline T get(int iIndex) const
{
	FAssert(iIndex >= 0);
	FAssert(iIndex < m_iLength);
	return tArray ? tArray[iIndex] : m_eDefault;
}

No asserts is a good start. The last line looks like a copy paste of the line you wrote, except it returns m_eDefault instead of 0, but m_eDefault is set to 0 as default. It should produce the same result. This function is well tested and it works everywhere else. It returns the value it has stored. If it returns something incorrectly, then the problem lies in writing to the array, not reading from it.

I can't tell from the code why the code with getAllowsUnitClasses() works and your copy of it doesn't. Looks fine to me, which makes me wonder if XML or reading the XML is the problem.

I removed it because I was trying to test out that specific feature and was having those game freezes, and the Dock unit code wasn't working, and the updateInventionEffectCache() wasn't set up for swapping Civics, and it only takes a couple minutes to set up iCityPlotFoodBonus to work like vanilla code, and I was attempting to remove all problem causing variables, and I wasn't 100% sure how updateInventionEffectCache() worked (I see better now though).

Remember I am not a coder, but a modder

Not fully understanding the code... wouldn't that be a good reason to ask before changing something? The same goes for reverting a change made by somebody else. We need a bit of discipline and agreeing on design/goal when touching the same code area or it will end in chaos, bugs and commits changing design back and forth like a pendulum.

Do we agree that removing cache from savegames is a goal?

Another thing. You didn't increase the savegame version and broke old savegames.

 

[Kailric]

This might be a silly question, but did you ensure that you actually added a class to XML and that the player has the civic in question? When code doesn't work as expected, such trivial stuff should be the first to be checked.

The below is what I have in the XML, is it set up correctly?

<AllowsUnitClassesImmigration>
        <AllowsUnitClass>
          <AllowsUnit>UNITCLASS_INDENTURED_SERVANT</AllowsUnit>
          <iChange>-1</iChange>
        </AllowsUnitClass>
        <AllowsUnitClass>
          <AllowsUnit>UNITCLASS_COTTER</AllowsUnit>
          <iChange>-1</iChange>
        </AllowsUnitClass>
</AllowsUnitClassesImmigration>

Not fully understanding the code... wouldn't that be a good reason to ask before changing something? The same goes for reverting a change made by somebody else. We need a bit of discipline and agreeing on design/goal when touching the same code area or it will end in chaos, bugs and commits changing design back and forth like a pendulum.

Well, if you was next door I'd walk over and ask you about it or If I could call you I would. Since delays in conversations can be hours, if not days at a time, when I have the time to code I code, and ask questions later

Do we agree that removing cache from savegames is a goal?

I am not exactly sure what that means?

Another thing. You didn't increase the savegame version and broke old savegames.

I am not concerned with breaking saved games with the Civics Branch. Over the pass couple weeks I've added multiple new variables and functions that could use testing individually, but not within a complete game setting. I keep mentioning it, but my coding time is limited, and my coding skills are basic. Attempting to preserve saved games, that for the most part do not even exist, seems counter productive to me as I would probably create more issues than fix since I am not familiar with doing that.

 

[Nightinggale]

The below is what I have in the XML, is it set up correctly?

<AllowsUnitClassesImmigration>
        <AllowsUnitClass>
          <AllowsUnit>UNITCLASS_INDENTURED_SERVANT</AllowsUnit>
          <iChange>[B][COLOR="red"]-1[/COLOR][/B]</iChange>
        </AllowsUnitClass>
        <AllowsUnitClass>
          <AllowsUnit>UNITCLASS_COTTER</AllowsUnit>
          <iChange>[B][COLOR="Red"]-1[/COLOR][/B]</iChange>
        </AllowsUnitClass>
</AllowsUnitClassesImmigration>

if (kCivicInfo.getAllowedUnitClassImmigration(iI) > 0)

The code only display units when they are enabled. Change it to trigger on values != 0 and then branch display string on the value should make it possible to display both enable and disable of units for immigration.

Well, if you was next door I'd walk over and ask you about it or If I could call you I would. Since delays in conversations can be hours, if not days at a time, when I have the time to code I code, and ask questions later

Good point and it isn't like this is something major. However generally speaking it would be a good idea if we at least try to post something.

I am not exactly sure what that means?

It mean that we move variable calculation from processCivic() to updateInventionEffectCache() and NOT save the output of the calculations.

By removing the data from the savegames, modifying XML will take effect right away when loading savegames. When the calculations are handled by processCivic, the savegames will keep their old values. The result will be that it will likely be easier to fix XML setup bugs as you can load the same savegame over and over and see the results. It also mean less need to be concerned with savegame versions. Good for DLL modders and likely even better for XML modders.

I am not concerned with breaking saved games with the Civics Branch. Over the pass couple weeks I've added multiple new variables and functions that could use testing individually, but not within a complete game setting. I keep mentioning it, but my coding time is limited, and my coding skills are basic. Attempting to preserve saved games, that for the most part do not even exist, seems counter productive to me as I would probably create more issues than fix since I am not familiar with doing that.

That's a very valid point. I'm not really concerned about it right now, but you have done the same to master a few times and then realized that you forgot about it again. I can't tell if not updating savegames "by the book" is intended or not.

 

[Kailric]

if (kCivicInfo.getAllowedUnitClassImmigration(iI) > 0)

The code only display units when they are enabled. Change it to trigger on values != 0 and then branch display string on the value should make it possible to display both enable and disable of units for immigration.

Duh, totally missed that.

Good point and it isn't like this is something major. However generally speaking it would be a good idea if we at least try to post something.

Well, I did at least post and let you know what I did. The only reason I changed it at the time was because of all the issues I was having. Normally I would not have touched it. It can be added back with ease.

It mean that we move variable calculation from processCivic() to updateInventionEffectCache() and NOT save the output of the calculations.

By removing the data from the savegames, modifying XML will take effect right away when loading savegames. When the calculations are handled by processCivic, the savegames will keep their old values. The result will be that it will likely be easier to fix XML setup bugs as you can load the same savegame over and over and see the results. It also mean less need to be concerned with savegame versions. Good for DLL modders and likely even better for XML modders.

Ah, kk, I know we talked about this before it seems, but I forget easily if it has been a while. How many of these have you set up? For Player and City, right? And what about games like WHM that will have a ton of civics to cycle through, at what point does this system become less efficient, or does it ever, as the developers didn't go this route.

That's a very valid point. I'm not really concerned about it right now, but you have done the same to master a few times and then realized that you forgot about it again. I can't tell if not updating savegames "by the book" is intended or not.

I do realize though that when we merge in the Civic Screen stuff it will break all saved games. I really don't think there are any saved games worth saving at this point, however. We can name the number of testers on less than one hand and I don't get attached to saved games and they prolly don't either. With all the new Civics and features we are adding no saved games could be actually played out anyway without throwing things out of balance.

 

[Kailric]

Hmm, I added a if (kCivicInfo.getAllowedUnitClassImmigration(iI) < 0) to the code but still it is not showing up. I ran debugger and the expression is still returning false every time. I pushed the changes to Git, so if you can check into that, that be great!

 

[Nightinggale]

Ah, kk, I know we talked about this before it seems, but I forget easily if it has been a while. How many of these have you set up? For Player and City, right? And what about games like WHM that will have a ton of civics to cycle through, at what point does this system become less efficient, or does it ever, as the developers didn't go this route.

I don't think the number of civics will cause performance issues, regardless of number of civics. They are looped when a player gains one and for GUI like pedia. The often called functions like CvUnit::canHaveProfession() relies on cache only, which is the important part. I think I will try to make code to skip untouched arrays though. The cache update is already bigger than planned and I assume it will become bigger and bigger as time passes. We might as well fix potential performance issues before they appear.

Yeah they are set up for cities (build effects) and players (civics). They are fairly easy to add meaning we can have more if we feel like it.

Hmm, I added a if (kCivicInfo.getAllowedUnitClassImmigration(iI) < 0) to the code but still it is not showing up. I ran debugger and the expression is still returning false every time. I pushed the changes to Git, so if you can check into that, that be great!

I wonder how many months it will take before I can just sit down and code something brand new without being interrupted because somebody needs help. I will take a look at this.

 

[Kailric]

I wonder how many months it will take before I can just sit down and code something brand new without being interrupted because somebody needs help.

The answer to that expression is NULL

Hey, I worked out that infinite loop problem, I could have asked for help there too. I'm just unfamiliar with your way of setting up that function or I would delve further into.

 

[Kailric]

CvCity::doGrowth() loops civics to get random growth units. I'm not particularly happy with this code...

Because of this I propose caching the odds for each unit to allow civics to stack effects in a manner people is most likely to expect.

I can tell that you and I just ain't gonna get along

Just now noticed your comment on this. When I wrote that code I thought, "I wonder what Nightinggale will think of this?", now I know

Well, I wasn't thinking that anyone was going to actually stop and try to figure how accurate the calculations were. If a new growth unit had a chance and won the first contest then it moved on to the next tournament which it has equal odds with all who passed before. Seemed fair to me But if you want to fix this up then go right ahead.

 

[Nightinggale]

If a new growth unit had a chance and won the first contest then it moved on to the next tournament which it has equal odds with all who passed before.

That's not the problem. I'm fine with that part. The problem is if multiple civics adds a chance for the same unit because it stacks chances in a horrible and somewhat unpredictable way. If we have two civics giving 20% chance of a unit, we should have 40% chance for that unit, not 34% like right now. It isn't of great importance though.

 

[Nightinggale]

While looking at DoaNE I noticed this thread: http://forums.civfanatics.com/showthread.php?t=454061

I like the concept that adding a new unit will not break savegames. I looked at the code in question and it turns out to be 130 kB of code not including all the read() and write() functions, which needs updating. I don't want to do that.

However it gave me an idea. We just save a list of types for each in CvGame::write(). When reading this, we make a list of conversions to current XML, like UNIT_FARMER is 2 in the list, but 5 in XML, then the list of conversion will contain 5 at index 2. Next we use this list where it is easiest to implement: JustInTimeArray::read(). This way we will only have to update a single function and not a whole lot of functions.

It will likely be good to convert m_eUnitType in units as well.

This will not be the great solution that C2C came up with, but it should allow adding and reordering context in XML files and still load. It will sometimes also work if something is removed from XML. It will only work for those XML files, which are linked to a JustInTimeArray. However a closer inspection of those reveals that we are talking about units, buildings, promotions and other often modified files. Civics aren't part of it now, but it would be a good candidate to add as well.

 

[Kailric]

I like the concept that adding a new unit will not break savegames.

Ink-teresting, this would come in handy perhaps when I start adding all those Sub-Profession units perhaps and the like.

 

[Kailric]

By the way, in my latest push I mentioned about the CIVICOPTION_INVENTION... I attempted to do as you mentioned with adding GC.setInfoTypeFromString("CIVICOPTION_INVENTIONS", -1); to CvXMLlLoadUntilitySet.cpp but I got errors. First in the DLL all CIVICOPTION_INVENTIONS showed up being undefined after I removed the CvEnum.cpp for that, which this makes since. I added a New:

 #define CIVICOPTION_INVENTIONS		(-1)///Tks Civics

to CvDefines.h. All seems to be working as intended in that regards.

But, when I left CIVICOPTION_INVENTION in the CivicInfos.xml and loaded a game I got errors saying CIVICOPTION_INVENTION wasn't recognized, so I removed them all and just put a NONE and again, all seems to be working well.

 

[Nightinggale]

I can't see why GC.setInfoTypeFromString("CIVICOPTION_INVENTIONS", -1) didn't work
Maybe it was added at the wrong location or something.

Don't define CIVICOPTION_INVENTIONS in CvDefines.h. We have enums for that in CvEnums.h. There should be one for civics as well.

 

[Kailric]

I can't see why GC.setInfoTypeFromString("CIVICOPTION_INVENTIONS", -1) didn't work
Maybe it was added at the wrong location or something.

Don't define CIVICOPTION_INVENTIONS in CvDefines.h. We have enums for that in CvEnums.h. There should be one for civics as well.

I added it just below the "NONE" one I wasn't sure how to add it to enums. I guess you would just do the same as they do with NO_CIVICOPTION and they both = -1?

I should just do a system wide swap of CIVICOPTION_INVENTIONS with NO_CIVICOPTION so as to remove all confusion in the dll. Should only take seconds.

 

[Nightinggale]

By the way, in my latest push I mentioned about the CIVICOPTION_INVENTION... I attempted to do as you mentioned with adding GC.setInfoTypeFromString("CIVICOPTION_INVENTIONS", -1); to CvXMLlLoadUntilitySet.cpp but I got errors. First in the DLL all CIVICOPTION_INVENTIONS showed up being undefined after I removed the CvEnum.cpp for that, which this makes since. I added a New:

 #define CIVICOPTION_INVENTIONS		(-1)///Tks Civics

to CvDefines.h. All seems to be working as intended in that regards.

But, when I left CIVICOPTION_INVENTION in the CivicInfos.xml and loaded a game I got errors saying CIVICOPTION_INVENTION wasn't recognized, so I removed them all and just put a NONE and again, all seems to be working well.

I better look into this. The commit, which changes inventions to civic option to NONE completely broke civic pedia. Without the pedia I can't quickly tell if the growth unit appears as expected (yet another delay). At first I thought I broke it and as a result I now have two branches for growth unit to get back to once this problem is fixed. It's like growth unit concept is cursed to take forever.

 

[Kailric]

Oh yeah, the pedia. I had to fix the Tech Screen, the Trade Perk Screen, and other suff with that change. I was going to fix pedia too but forgot. I want to seperate the techs, civics, trade perks, censors, and anything else into its own pedia screen.

 

[Nightinggale]

If you plan on fixing the pedia, then I will put my work on hold and wait for you. Using the high number of local branches gives me the possibility to easily work on something completely different while keeping the changes I already made.

 

[Kailric]

Tech Categories need to be added as globals in the DLL so their placement does not matter.

Hmm, I stated this the other day but I don't think this would be very modder friendly. orlanth wants to bring back the Tech Categories. Right now in order to get the Pedia to work for Techs, Civics, Censures, and Trade Perks I've hardcoded values into python. Which, python is much easier to mod, still, there needs to be an easier set up to differenciate the different types.

For Civics it is easy as I use if gc.getCivicInfo(j).getCivicOptionType() != -1:, and since all Civics have an CivicOption and Techs and the like do not it is easy to find them.

For the techs I have used the order of Tech Categories, with Inventions being set to 1 since MEDIEVAL_TECH is 1 in the CivicOptions infos, NATIVE_TECH being 0 of course. And then use if gc.getCivicInfo(j).getInventionCategory() == 1: to find all the Inventions. And then for Trade Perks is 2, Censures, is 3.

Now, when orlanth expands the Tech Categories all these numbers will be thrown off. So, we need a mod friendly way to do this. Of course we can just simply add a if gc.getCivicInfo(j).getInventionCategory() < #: with # being the number of Tech Categories. But each time a new Category is added Python would have to be edited.

So, just thinking out loud, if we added a new xml file for Tech Categories that we can simply add new categories to, and then the <InventionCategory> would be used for this. Then in the Pedia any entry with an Invention Category would be considered a Tech.

Now for the other two Categories that are under a separate system, Trade Perks and Censures, we would need anther xml file, perhaps called CivicInfoCategory, which we can have the Techs, Trade Perks, and Censures. And then in the future we can add new categories to this if we wanted to add other effects.

What would be your ideas on this?

EDIT: Normally I would not even worry about this but Nightinggale has rubbed off on me and in my thinking now I think about other modders. Thanks, Night, for giving me a modder friendly heart ...