Quick Modding Questions Thread

[Nightinggale]

I am awaiting the conclusion of this detective story with bated breath and am ready to add whatever final fixes come out of it. Unfortunately, I can't meaningfully contribute to the discussion, as all that is nuclear physics to me.

 

[f1rpo]

I don't think anything better than Nightinggale's bugfix is forthcoming. I might fix the same issue more elaborately, but that won't be as easy to adopt.

My guess is that init is not simply called from the CvUnit constructor (guaranteeing full initialization) due to the close interaction between CvUnit and the FFreeListThrashArray class (I'll abbreviate that as FLTA). It's a pretty strange design pattern to me. Maybe there was also a desire for uniformity with CvPlayer and CvTeam, which exist already on the opening menu and get reset to a blank state when returning there. CvUnit (and CvCity, CvSelectionGroup) are not reusable like that, but still have reset functions (which FLTA does not require and which I've merged into the constructors in AdvCiv). Anyway, I believe the intention is that every unit gets initialized immediately after its creation, and that kill initiates every unit's destruction, even when returning to the opening menu, probably even when returning to the desktop. Within the DLL, nothing will stop a programmer from calling the CvUnitAI constructor directly. CvUnit itself can't be instantiated at all; having to go through CvUnitAI is arguably already counterintuitive enough to make this an unlikely accident. Deleting a CvUnit (or CvUnitAI) pointer is perhaps a slightly more likely mishap. For Python, there's afaik only CyPlayer::initUnit for properly creating a unit. For any game state changes, I'd say it's safe to rely on init and kill; if those aren't executed, the game state will break anyway.

I have some old notes about the life cycle of CvUnit, but they might be too technical:

Spoiler :

The creation of a city, unit or group is always initiated from within the DLL. CvPlayer provides factory functions initUnit, initCity and addSelectionGroup, which in turn call FLTA::add. add calls the respective default constructor, which calls reset functions for a blank state.

add also stores a pointer to the (unit, city or group) element and stores an id at the element by calling its setID function. FLTA::getAt can map that id very efficiently to the element pointer. The id is only guaranteed to be unique within the given FLTA. As a globally unique id, the IDInfo struct is used (most importantly in serialized CLLinkLists), which couples the FLTA-internal id with the id of the player that owns the FLTA. Lookup of an IDInfo happens through e.g. getCity (CvGameCoreUtils) via CvPlayer::getCity.

Once add returns, the factory function (or, in the case of CvSelectionGroup: CvUnit::joinGroup) calls init on the blank instance, providing some crucial data such as plot coordinates.

The destruction of a unit, city or group gets initiated through a kill call on that instance. kill calls a delete... helper function on CvPlayer (deleteSelectionGroup, deleteUnit, deleteCity), which calls removeAt on the FLTA. The FLTA calls the element's destructors, which call uninit and AI_uninit. Finally, the FLTA removes the element pointer from its internal array.

 

[Walter Hawkwood]

My guess is that init is not simply called from the CvUnit constructor (guaranteeing full initialization) due to the close interaction between CvUnit and the FFreeListThrashArray class (I'll abbreviate that as FLTA). It's a pretty strange design pattern to me. Maybe there was also a desire for uniformity with CvPlayer and CvTeam, which exist already on the opening menu and get reset to a blank state when returning there. CvUnit (and CvCity, CvSelectionGroup) are not reusable like that, but still have reset functions (which FLTA does not require and which I've merged into the constructors in AdvCiv). Anyway, I believe the intention is that every unit gets initialized immediately after its creation, and that kill initiates every unit's destruction, even when returning to the opening menu, probably even when returning to the desktop. Within the DLL, nothing will stop a programmer from calling the CvUnitAI constructor directly. CvUnit itself can't be instantiated at all; having to go through CvUnitAI is arguably already counterintuitive enough to make this an unlikely accident. Deleting a CvUnit (or CvUnitAI) pointer is perhaps a slightly more likely mishap. For Python, there's afaik only CyPlayer::initUnit for properly creating a unit. For any game state changes, I'd say it's safe to rely on init and kill; if those aren't executed, the game state will break anyway.

I have some old notes about the life cycle of CvUnit, but they might be too technical:

No, thank you, that is the exact right amount of technical! While I'm still a bit fuzzy on creation (I think addUnit always calls init, and one can't create a unit without addUnit), I am now reasonably sure that deleteUnit is indeed only called from one place.

 

[f1rpo]

Oh, right, there is also CvPlayer::addUnit; I've eliminated that in AdvCiv and therefore probably didn't mention it in my notes. That one looks hazardous - adds a new unit to the FLTA without calling init afterwards. Not exposed to Python, but public visibility in the DLL. initUnit should be the only call location; I don't find others with a full-text search on the original source code. (There are other functions of the same name that take a CvUnit pointer as an argument.) And the only deleteUnit call that I see is indeed in CvUnit::kill.

 

[Nightinggale]

So it seems that the only potential advantage of an IDInfo data member over a CvCity pointer is that the invalidation of the IDInfo member can be delayed a little bit when a CvCity is destroyed.

That's actually not entirely true. Ignoring the savegame issue, there is also the issue that storing pointers everywhere means everything depends on everything. Isolating each function/class as much as possible makes it easier to do automated tests. Most modders ignore this, partly (or mainly) because the vanilla design is test hostile.

 

[f1rpo]

I don't see myself using numeric IDs with a lookup mechanism instead of a simple C++ reference or pointer just for the sake of testing; but I can see how that could help solve problems with testing.

As for splitting up updateWorkingCity, I've realized that the current pattern is to call that function for each plot of the map whenever a city is created or destroyed. Additionally clearing the IDInfo in a separate call seems to add more confusion than it removes, so I'll confine myself to adding reproachful comments.

 

[PPQ_Purple]

That's actually not entirely true. Ignoring the savegame issue, there is also the issue that storing pointers everywhere means everything depends on everything. Isolating each function/class as much as possible makes it easier to do automated tests. Most modders ignore this, partly (or mainly) because the vanilla design is test hostile.

The issue with transferring parameters by value as opposed to pointer is that it leads to memory duplication. And that is a big problem with CIV4 because of its hardcoded 32 bit memory limit. Something that mods already but up against. If moders stopped using pointers like you suggest the game would become less coupled but would also flat out not work.

 

[Nightinggale]

The issue with transferring parameters by value as opposed to pointer is that it leads to memory duplication. And that is a big problem with CIV4 because of its hardcoded 32 bit memory limit. Something that mods already but up against. If moders stopped using pointers like you suggest the game would become less coupled but would also flat out not work.

A pointer is 4 bytes. An enum type instance is 4 bytes, less with bitfields. IDInfo is 8 bytes. I don't think this is where the memory is being eaten up.

If you want to lower memory usage, one thing which has been on my mind for ages is CvInfoBase. In most cases it contains nearly no information whatsoever, yet it always use 320 bytes, not counting external memory for the text in the strings. Take for instance PlayerColorInfo. All it use the base class for is the Type string. This can be rewritten to being just a single char pointer meaning it would take up 4 bytes. Storing the xml data seems to be the most wasteful part of the code in terms of memory usage.

 

[PPQ_Purple]

A pointer is 4 bytes. An enum type instance is 4 bytes, less with bitfields. IDInfo is 8 bytes. I don't think this is where the memory is being eaten up.

If you want to lower memory usage, one thing which has been on my mind for ages is CvInfoBase. In most cases it contains nearly no information whatsoever, yet it always use 320 bytes, not counting external memory for the text in the strings. Take for instance PlayerColorInfo. All it use the base class for is the Type string. This can be rewritten to being just a single char pointer meaning it would take up 4 bytes. Storing the xml data seems to be the most wasteful part of the code in terms of memory usage.

What I am saying is that pointers are the memory saving mechanism that keeps it from being eaten up. The CIV code is full of various structures passing around data about cities, units etc. And those will all add up if you start passing them by value. Especially in larger mods that are already screaming for memory because everything is loaded into memory anyway.

I wonder how far we could go with loading stuff on demand. Like, say if the game is in the medieval period there is no need to load assets or XML for modern units.

 

[Nightinggale]

What I am saying is that pointers are the memory saving mechanism that keeps it from being eaten up. The CIV code is full of various structures passing around data about cities, units etc. And those will all add up if you start passing them by value. Especially in larger mods that are already screaming for memory because everything is loaded into memory anyway.

I think there is a misunderstanding here. I didn't say everything should be return by value. Instead I said something like CvPlot should contain TerrainTypes, FeatureTypes etc rather than a pointer to each. It can still use get*Info and get the references. Without the pointers, it becomes easier to make a setup for unit testing where each class/function can be automatically tested. It also in general makes it easier to isolate bugs when tracking them down. It's fine to store a reference/pointer on the stack, but ideally we shouldn't have a lot of them as persistent class member variables.

Also say CvPlot has a list of unit pointers, what will happen if a unit dies and there is a bug, which doesn't tell the plot? It will crash while an IDInfo giving an invalid ID will result in a NULL pointer to a unit, which can be tested for. This makes it easier to write code, which is less prone to crashing. Having such test conditions also makes it easier to add good assert checks.

I wonder how far we could go with loading stuff on demand. Like, say if the game is in the medieval period there is no need to load assets or XML for modern units.

If anybody can figure out how to make the exe read graphics on demand like that, then I'm all ears.

 

[wli4]

Hi, I'm wondering if there's a way I could make it so that a wonder gives more culture for every fortified unit in the city?

 

[f1rpo]

If you just want to add the unit count to the city's total culture once per turn - and thus to contribute to the city's culture level, that should be doable in any Python function that gets called once per player turn. doCulture would seem like the most appropriate one. You would then check for the presence of the building by its XML type string, e.g. "BUILDING_SISTINE_CHAPEL". For the number of garrisoned units, CyCity.getMilitaryHappinessUnits could be used, I think that's also used (by the DLL) for Hereditary Rule. However, this approach will keep AI and UI entirely unaware of the additional culture. Also will only partially affect the culture spread to tiles. (This can also be handled in Python, but it's going to be elaborate; see CvCity::doPlotCulture in the DLL.)

The clean way to implement such a mechanism would be through CvCity::getBaseCommerceRateTimes100 in the DLL and with a new XML tag, perhaps named "CommercePerMilitaryUnit". (Will be easier to implement for all commerce types at once.)

 

[wli4]

How would the code look for the doCulture function?

 

[f1rpo]

Spoiler :

	def doCulture(self, argsList):
		pCity = argsList[0]
		print "doCulture called for city at " + str(pCity.getX()) + ", " + str(pCity.getY())
		# For example; actual building type string from XML needs to be used here.
		iMilitaryCultureBuilding = gc.getInfoTypeForString("BUILDING_SISTINE_CHAPEL")
		if iMilitaryCultureBuilding < 0:
			print "Military Culture building type not found"
			return False
		else: print "Military Culture building ID: " + str(iMilitaryCultureBuilding)
		iBuildings = pCity.getNumActiveBuilding(iMilitaryCultureBuilding)
		if iBuildings <= 0:
			print "No Military Culture buildings present (or obsolete)"
			return False
		else: print "Military Culture building present: " + str(iBuildings)
		iMilitaryUnits = pCity.getMilitaryHappinessUnits()
		print "Military Happiness units present: " + str(iMilitaryUnits)
		iCultureChange = iBuildings * iMilitaryUnits
		print "Adding " + str(iCultureChange) + " city culture"
		pCity.changeCulture(pCity.getOwner(), iCultureChange, False)
		return False

Not tested, which is why I've inserted a lot of print statements; so that one can easily track where the code goes wrong. Should be removed/ commented out later. print writes to PythonDbg.log if LoggingEnabled is set in CivilizationIV.ini. Adding this to a BUG-based mod may require placing the code in a separate module and registering a handler for the doCulture function. I'd expect that some Python-based mod components exist that increase a city's culture rate, maybe something by Platyping. Perhaps someone has already figured out how to get the plot culture spread right through Python and also how to add some form of UI feedback.

 

[wli4]

Okay thanks it worked
However, is there a way for me to append to the additional culture to the culture interface screen?
Like this one

 

[f1rpo]

Through the DLL - easily, in Python - no. One could probably add a new tooltip, but then the old one would have to be reimplemented in Python. I doubt that two separate tooltips can be shown. I don't know what the least bad Python-only solution is to this problem.

 

[wli4]

Okay how would the DLL code look?

 

[f1rpo]

Since I claimed that it's easy ...

Spoiler :

New code tagged with "milc" (short for "military culture/ commerce")

// CvGameTextMgr.cpp
void CvGameTextMgr::setCommerceHelp(CvWStringBuffer &szBuffer, CvCity& city, CommerceTypes eCommerceType)
{
	// ...

	int iBuildingCommerce = city.getBuildingCommerce(eCommerceType);
	if (0 != iBuildingCommerce)
	{
		szBuffer.append(gDLL->getText("TXT_KEY_MISC_HELP_BUILDING_COMMERCE", iBuildingCommerce, info.getChar()));
		szBuffer.append(NEWLINE);
		iBaseCommerceRate += 100 * iBuildingCommerce;
	}
	// <milc>
	int iMilitaryCommerce = city.getMilitaryCommerce(eCommerceType);
	if (iMilitaryCommerce != 0)
	{
		szBuffer.append(gDLL->getText("TXT_KEY_MISC_HELP_MILITARY_COMMERCE", iMilitaryCommerce, info.getChar()));
		szBuffer.append(NEWLINE);
		// This would be needed if the rule change were also implemented through the DLL
		//iBaseCommerceRate += 100 * iMilitaryCommerce;
	} // </milc>
	int iFreeCityCommerce = owner.getFreeCityCommerce(eCommerceType);
	// ...
}

// CvCity.h
	void changeCommerceRateModifier(CommerceTypes eIndex, int iChange);

	int getMilitaryCommerce(CommerceTypes eCommerce) const; // milc

	int getCommerceHappinessPer(CommerceTypes eIndex) const;

// CvCity.cpp
void CvCity::changeCommerceRateModifier(CommerceTypes eIndex, int iChange)
{
	//
}

// milc:
int CvCity::getMilitaryCommerce(CommerceTypes eCommerce) const												 
{
	// Proper DLL implementation (not involving Python) would do something like this:
	//return getNumMilitaryUnits() * getCommercePerMilitaryUnit(eCommerce);
	if (eCommerce != COMMERCE_CULTURE)
		return 0;
	// Same as the Python code (doCulture callback)
	// For example; actual building type string from XML needs to be used here.
	static BuildingTypes const eMilitaryCultureBuilding = (BuildingTypes)
			GC.getInfoTypeForString("BUILDING_SISTINE_CHAPEL");
	if (eMilitaryCultureBuilding == NO_BUILDING)
	{
		FAssertMsg(eMilitaryCultureBuilding != NO_BUILDING, "Military Culture building type not found);
		return 0;
	}
	return getNumActiveBuilding(eMilitaryCultureBuilding) * getNumMilitaryUnits();
}

int CvCity::getCommerceHappinessPer(CommerceTypes eIndex) const

// New game text file
<?xml version="1.0" encoding="ISO-8859-1"?>

<Civ4GameText xmlns="http://www.firaxis.com">
	<!-- Based on TXT_KEY_MISC_HELP_BUILDING_COMMERCE, translations on
		 TXT_KEY_CIVIC_MILITARY_PRODUCTION. -->
	<TEXT>
		<Tag>TXT_KEY_MISC_HELP_MILITARY_COMMERCE</Tag>
		<English>[ICON_BULLET]%D1%F2 from Military Units</English>
		<French>[ICON_BULLET]%D1%F2 en raison d'unit&#233;s militaires</French>
		<German>[ICON_BULLET]%D1%F2 durch Milit&#228;reinheiten</German>
		<Italian>[ICON_BULLET]%D1%F2 dagli unit&#224; militari</Italian>
		<Spanish>[ICON_BULLET]%D1 %F2 por unidades militares</Spanish>
	</TEXT>
</Civ4GameText>

getMilitaryCommerce would essentially repreat the Python implementation, including the hardcoded XML building type string. You'd need to recompile with these changes, of course. And I have, again, not tested it.

 

[wli4]

hmmmm, something's not working, but hopefully I'll figure that out. Anyways thanks

 

[f1rpo]

Sure. I was surprised that the Python just worked. (Or maybe it didn't, and you also had to figure some things out.) Please feel free to post errors that you can't resolve; your thread with the same question could still find a use. Well, if the issue is with compilation in general, then the "easiest way to compile thread" is the better place. (I'm assuming that you're new to that because you asked for a Python/XML solution.)