DLL coding thread

[Nightinggale]

I discovered that they will only trade if they have an AI_isPort() City. Where is AI_isPort() set? I find nothing in the DLL so can only assume it is in the exe.

CvCityAI::AI_setPort(). However I can't find where it is supposed to be called. I just had an idea regarding this issue. If I understand DllExport correctly, those function names should be written in the exe in plain text. Grep or an hex editor should be able to find them if that is the case and then we can make a list of functions, which are known to be used inside the exe. Hopefully the list will be rather short.

Anyway, I am adding my own code, like when a City is created it checks if it is a NationState, checks if it can access Europe tiles, then sets that city as a Port city. Once I did that Rome started trading.

While we can't control when and what the exe calls, we can always make our own code like that. We should make our code as independent from the exe as possible. I looked at the function to get the bonus from a plot once. The exe calls it all the time, presumably the screen drawing code.

I realized that we should presumably make our own version of this function and make it return what the plot has while the vanilla function should return what should be drawn. That way we can hide bonuses before they are invented (at least I assume that). There are lots of options for the exe interaction, but figuring out what to actually do is tricky without access to the exe source code or at least documentation.

Also, I discovered that we need to add code to check whether a Product requires Two or more Yields in the AI_professionValue() code. If one Yield is Present and the other is not it will still assign Citizens to work Buildings that produce no Yields.

The entire AI yield behaviour seems a bit hackish and should be reviewed. We should make code to calculate how many yields a unit will make with a specific profession and use that everywhere, including GUI, AI and whereever needs this info. AI_professionValue() should then estimate value based on output, not input. That way AI_professionValue() will no longer care for input yields and it will be more resistant to bugs if we change something somewhere else later.

This is not just an issue with AI_professionValue(). We have lots of values, which are calculated in multiple locations and nobody knows if they are calculated to be the same everywhere.

 

[Nightinggale]

I finally managed to make a JIT array of a JIT array. They are called 2D JIT arrays in the code. It likely took me 2-3 hours to actually make and then 20 hours to get the backend memory allocation to work
In the end I couldn't get C++ to allocate an array or vector or similar to a template class using a non-default constructor. The answer is low level C where I manually allocate memory without using constructors and then I set the memory afterwards to make it appear like the correct constructor executed. It works, but it's not the level of clean coding I would have liked.

I'm quite happy with the result. It adds JustInTimeArray2D<T>, which is used just like the current JIT arrays where T is the variable type. The difference is that there is no default constructor and you have to give it arguments.

JustInTimeArray2D(JIT_ARRAY_TYPES eType, JIT_ARRAY_TYPES eSubType, T eDefault = (T)0);

eType sets the number of JIT arrays and eSubType sets the number of elements in each of those arrays. eDefault is passed on to each array.

Example:
m_ja2_iCultureRangeCities(JIT_ARRAY_PLAYER, JIT_ARRAY_CULTURE) will make a JIT array for each player, each with the length to store each culture type (I think there are 7 or something of those). Default is omitted and falls back to the default, which is 0.

You can replace eType with int, in which case you get the number of arrays you ask for. (4, JIT_ARRAY_YIELD) will give you 4 yield arrays.

Accessing is done by

T get(int iIndex, int iSubIndex) const;
void set(T eValue, int iIndex, int iSubIndex);
void add(T eValue, int iIndex, int iSubIndex);

Nothing completely new there either.

Saving is done by

m_ja2_iCultureRangeCities.readWrite(bRead, pStream);

The code this read/write code replaces couldn't handle XML changes and it is slightly longer

if (bRead)
{
	char cCount = 0;
	if (NULL != m_apaiCultureRangeCities)
	{
		for (int iI = 0; iI < MAX_PLAYERS; ++iI)
		{
			SAFE_DELETE_ARRAY(m_apaiCultureRangeCities[iI]);
		}
		SAFE_DELETE_ARRAY(m_apaiCultureRangeCities);
	}
	pStream->Read(&cCount);
	if (cCount > 0)
	{
		m_apaiCultureRangeCities = new char*[cCount];
		for (int iI = 0; iI < cCount; ++iI)
		{
			int iCount = 0;
			pStream->Read(&iCount);
			if (iCount > 0)
			{
				m_apaiCultureRangeCities[iI] = new char[iCount];
				pStream->Read(iCount, m_apaiCultureRangeCities[iI]);
			}
			else
			{
				m_apaiCultureRangeCities[iI] = NULL;
			}
		}
	}
}
else
{
	if (NULL == m_apaiCultureRangeCities)
	{
		pStream->Write((char)0);
	}
	else
	{
		pStream->Write((char)MAX_PLAYERS);
		for (int iI=0; iI < MAX_PLAYERS; ++iI)
		{
			if (NULL == m_apaiCultureRangeCities[iI])
			{
				pStream->Write((int)0);
			}
			else
			{
				pStream->Write((int)GC.getNumCultureLevelInfos());
				pStream->Write(GC.getNumCultureLevelInfos(), m_apaiCultureRangeCities[iI]);
			}
		}
	}
}

 

[Kailric]

An array in an Array, wasn't I asking about this a bit ago ? Sounds helpful!

 

[Nightinggale]

I merged the read and write functions for all the files where I planned to do so and it will now be much harder to make saving and loading go out of sync. Next is to implement an automated check to tell if all member variables are mentioned in readWrite(). This check should look into comments too as comments could tell that the variable shouldn't be saved. It will just have to be mentioned as it will catch the cases where a new variable is added and nobody remembers to add it to the savegame when it is supposed to be saved.

Now that we have a branch where I have broken savegame compatibility with more or less every single commit, we should consider code cleanup. I have a few things I would like to fix, but ideas are welcome.

 

[Nightinggale]

I finished a perl script to look through all source files and check if all variables mentioned in the header file are also mentioned in readWrite(). The way it works is that it goes through the header file and stores all words beginning with m_. It will then detect all words beginning with m_ in readWrite() and print those only present in the header.

I did encounter two potential issues, both in CvCity.
m_iCivicTreasuryBonus is not saved and there is nothing in CvCity.cpp to recalculate it on load. Bug or is it handle elsewhere?

m_paTradeCities is set to NULL in the constructor, freed in the deconstructor and saved. It's not mentioned anywhere where. Is there a plan with this variable or is it ok to remove it?

I managed to get the execution time of the script down to 0.15 seconds. With an execution time this low I will look into automatically executing it in a precommit hook. If it reports a failure, the commit fails. That should make it next to impossible to forget to save data in the future.

I would also like to check XML ndents when committing, but considering it takes 3 seconds to check, I will see if I can get it to check only those files, which are being committed. As a precommit hook can't actually change what is being committed, my goal is to correct indenting, fail the commit and then it will be possible to commit again without doing anything manually in between because the problem was fixed automatically when the check failed.

 

[Kailric]

I finished a perl script to look through all source files and check if all variables mentioned in the header file are also mentioned in readWrite(). The way it works is that it goes through the header file and stores all words beginning with m_. It will then detect all words beginning with m_ in readWrite() and print those only present in the header.

I did encounter two potential issues, both in CvCity.
m_iCivicTreasuryBonus is not saved and there is nothing in CvCity.cpp to recalculate it on load. Bug or is it handle elsewhere?

m_paTradeCities is set to NULL in the constructor, freed in the deconstructor and saved. It's not mentioned anywhere where. Is there a plan with this variable or is it ok to remove it?

I managed to get the execution time of the script down to 0.15 seconds. With an execution time this low I will look into automatically executing it in a precommit hook. If it reports a failure, the commit fails. That should make it next to impossible to forget to save data in the future.

I would also like to check XML ndents when committing, but considering it takes 3 seconds to check, I will see if I can get it to check only those files, which are being committed. As a precommit hook can't actually change what is being committed, my goal is to correct indenting, fail the commit and then it will be possible to commit again without doing anything manually in between because the problem was fixed automatically when the check failed.

Wow, you are going to make me out to be a better coder than I really am
m_iCivicTreasuryBonus should either be saved or recalculated in UpdateBuildingAffectedCache. I probably planned to do one or the other but neither was done

EDIT: actually m_iCivicTreasuryBonus in CvCity.cpp isn't even used any where, yet.

m_paTradeCities can safely be removed, and I think there is one similar in CvPlayer also that I didn't end up using but went a different route, but kept it just in case the different route didn't work

 

[Nightinggale]

EDIT: actually m_iCivicTreasuryBonus in CvCity.cpp isn't even used any where, yet.

Good because a closer look reveals that it is broken. It is only changed in CvCity:rocessBuilding() and it relies on which civics the player has.

If player does

1. get needed civic
2. get building

then the civic is present when calling processBuilding() and the bonus is applied. If the order is reversed, then the civic isn't present when calling processBuilding() and the bonus is lost even after the civic is enabled. Fixing this is a bit complicated, but I will think of something. I think we will need two functions for this, one which loops all civics when a building is added/removed and one which loops all present buildings when a civic is added/removed.

EDIT: found code to handle when adding/removing civics. I'm still not sure it works 100% as intended through. Looks a bit like if we gain civic A, which enables some buildings and civic B, which increases from some buildings (possibly the same), the bonus from A is overwritten.

The line just before it is also broken. CvCity::changeMarauderDetection() will set detection range to the range of the best building for that task. It works by setting the range to the one from the processed building if that building has a higher range than the city. The number will not go down if the building is lost because there is no code to lower the range. The correct solution would be to reset the number in CvCity::UpdateBuildingAffectedCache() and then call changeMarauderDetection() in the loop in that function. That way it will be able to go both up and down according to currently available buildings.

I'm considering introducing a loop after loading where CvCity:rocessBuilding() is called for each present building. It will then work like promotions for units where the effect is applied from XML on load instead of from cached savegame data. It looks like it would just work with the entire function except for the two variables already mentioned, which would have to be modified anyway.

 

[Kailric]

m_iGoldIncome is the float in CvPlayer that uses the treasury bonus info, m_iCivicTreasuryBonus does nothing. I probably had some intention of using that to display info per city but haven't done it yet. When the Building is Processed it adds to the current Player Value whether positive or negative change depending if the building is added or taken away. When a Civic is added it does the same thing using the formula iGoldIncome += kCivicInfo.getCivicTreasuryBonus(iI) * getBuildingClassCount((BuildingClassTypes)kCivicInfo.getCivicTreasury(iI)); for each building class that gets the bonus.

I tested it for adding/removing saving/loading buildings/civics and all seemed to work at the time. Buildings are all processed at the start of a turn (or end which ever) and the Civic Changes happen during the players turn, so hopefully they will not go out of sync, I may have done something wrong though, wouldn't be the first time

Edit: I do see one potential problem. When a building is processed it can add the bonus for getGoldReservesBonus() which is different than getCivicTreasuryBonus(iC). Both are added to the player. Then if a you change Civics it does not factor in the getGoldReservesBonus() and will remove it. So, I need to adjust this so that it is not over written.

 

[Nightinggale]

I rebased save branch onto develop and then rebased develop. The result is the same as merging save into develop, but with a much nicer log (all save revisions together).

This mean most read() and write() functions have been replaced with readWrite() which handles both. It will hopefully get a well documented wiki page eventually, but for now just ask if something isn't clear.

I added Fullerene branches to M:C DLL and python. They are based on head revisions of develop and should ideally only be used by one person (guess who ).

 

[Kailric]

I have a question, what is called and what is happening with the TradeData item? It is sent into the setTradeItem as &item which sets up the variable to be used in other parts of the code. What is this process called?

TradeData item;
setTradeItem(&item, TRADE_WAR, eWarTeam, NULL);
AttitudeTypes eAttitute = AI_getAttitude(eWarPlayer);
if (kPlayer.canTradeItem(getID(), item, true) && eAttitute <= ATTITUDE_ANNOYED)

Also, I have discovered some more parts that are tied to the exe. I was attempting to piggy back on the DiplomacyInfo "AI_DIPLOCOMMENT_JOIN_WAR" by letting Pope's use this for a Holy War. The idea was that they would ask the player to go on a Crusade against another player but the Pope wouldn't actually be at war or go to war, only the Player would go to war. Normally this DiplomacyInfo is used for Players that are already at war and they want another player to Join with them in that war.

When I tested things, the Diplomacy event was executed in the dll but the popup was never sent to the player. So, I thought hmm, maybe there is something in the exe that makes another check to see if the Pope is actually at war. So, I used WouldBuilder to set the Pope at War with the targeted Civ and sure enough the Diplomacy event popup would execute everytime. So, that tells me that AI_DIPLOCOMMENT_JOIN_WAR and possibly others are double checked in the exe. Most of the Diplomacy code is handled in the exe anyway.

So, I just created a new DiplomacyInfo "AI_DIPLOCOMMENT_ACCEPT_HOLY_WAR" and now my function works as intended. Which is great

 

[Nightinggale]

I have a question, what is called and what is happening with the TradeData item? It is sent into the setTradeItem as &item which sets up the variable to be used in other parts of the code. What is this process called?

setTradeItem() appears to be an init function. Nothing special there and I don't think it will cause you problems. CvPlayer::canTradeItem() on the other hand appears to be very important to what you do.

now my function works as intended. Which is great

It's usually a good thing when the code works as intended

 

[Kailric]

I just tried to push my latest change to a new sub module and I received the following error messages, never seen it before?

Push: refs/remotes/origin/Fullerene does not point to a valid object! refs/remotes/origin/GameFont does not point to a valid object! refs/remotes/origin/Fullerene does not point to a valid object! refs/remotes/origin/GameFont does not point to a valid object! Aborting.
$ git.exe push --porcelain --progress --recurse-submodules=check origin refs/heads/LimitedResources:refs/heads/LimitedResources
refs/remotes/origin/Fullerene does not point to a valid object!
refs/remotes/origin/GameFont does not point to a valid object!
refs/remotes/origin/Fullerene does not point to a valid object!
refs/remotes/origin/GameFont does not point to a valid object!
The following submodule paths contain changes that can
sourceDLL
not be found on any remote:
Please try
git push --recurse-submodules=on-demand
or cd to the path and use
git push
to push them to a remote.
Aborting.

 

[Nightinggale]

I'm not quite sure what is going on, but try pulling before pushing. The most common reason for failure to push is because you are out of date compared to what is on origin. Also do try to only push the active branch, not everything. I can't see a reason for your client to even be concerned with those branches.

 

[Fullerene]

I haven't pushed or pulled anything for long time. I pulled now, maybe that fixed the errors?

 

[Kailric]

Yeah, I pulled before I did anything. Then created a new branch for my changes and tried to push that. It went through on my second attempt although it had those errors again.

 

[Kailric]

I am wanting to add a new attribute to ProfessionsInfo, that being bProspector but not sure how to go about it with the new setup, please inform me

Is it safe to add a new enum to PROFESSION_INFO_BM?

pXML->GetChildXmlValByName(&bBoolBuffer, "bProspector ");
if (bBoolBuffer)
{
SetBit(m_bfA, PROFESSION_INFO_BM_PROSPECTOR);
}

 

[Nightinggale]

Is it safe to add a new enum to PROFESSION_INFO_BM?

pXML->GetChildXmlValByName(&bBoolBuffer, "bProspector ");
if (bBoolBuffer)
{
	SetBit(m_bfA, PROFESSION_INFO_BM_PROSPECTOR);
}

That looks ok. The only thing, which can go wrong would be to run out of bits in m_bfA. However I intentionally added code, which fails at compiletime if that happens. If it compiles, it should work.

Do note that you added a space after bProspector in GetChildXmlValByName(). Fixing that now will save you some frustrating time later.

 

[Kailric]

That looks ok. The only thing, which can go wrong would be to run out of bits in m_bfA. However I intentionally added code, which fails at compiletime if that happens. If it compiles, it should work.

Do note that you added a space after bProspector in GetChildXmlValByName(). Fixing that now will save you some frustrating time later.

Ok, I'll give it a go when I get the chance. The space was a forum bug only

Anyway, still trying to push my changes. I refetched Gamefont and Fullerene and pushed them still no go. Makes no sense.

 

[Nightinggale]

Anyway, still trying to push my changes. I refetched Gamefont and Fullerene and pushed them still no go. Makes no sense.

That's quite odd. Try to make a new clone, copy your modified files into that one and commit & push. See if that makes any difference. I can't really tell what's wrong as I have never encountered that issue myself.

 

[Kailric]

That's quite odd. Try to make a new clone, copy your modified files into that one and commit & push. See if that makes any difference. I can't really tell what's wrong as I have never encountered that issue myself.

Yep, just tried that. Created a LimitedYields branch, pushed an test change with no problems but as soon as I merged in LimitedResources I get the same error. Only thing I can think that I have done different is I have been using your pearl script "move to source", any chance that is causing an issue?