Quick Modding Questions Thread

[f1rpo]

@Darkator: Diplo comment ID 97. By my count (in Civ4DiplomacyInfos.xml), that's USER_DIPLOCOMMENT_EXIT, the very last one. Strange, I would've expected a diplo comment added after EXIT to cause the error. At any rate, I don't see a DCM XML folder among your Files. I currently have only RevolutionDCM installed; that one has its own XML files, surely DCM does too. In particular, RevDCM has its own version of Civ4DiplomacyInfos.xml with a lot of new diplo comments. Could well be that merging the DCM XML files with your own will fix this problem.

(And I think I got my proposed workaround wrong in the previous post; should've been the inverse of the condition that I had posted. But, if a lot of XML from DCM is missing, there'll probably be more errors elsewhere, at the least bits of missing game text.)

Oh, and the .dmp files aren't generally useful.

 

[Darkator]

diplo file xml

 

[f1rpo]

Just checked your uploaded archives again. I guess the DCM XML was in there all along And DCM does not modify Civ4DiplomacyInfos.xml. Sorry, I'll try to take another look later, maybe I'm too sleepy. (Edit: I think I had gotten confused by the lack of a Python folder. Turns out that DCM doesn't have its own Python folder, so that's fine too.)

 

[Darkator]

a

 

[f1rpo]

Could you please upload a savegame where the crash occurs? I've compiled your code (DCM with 50 civ limit), but I'm not sure how to reproduce the crash.

 

[Darkator]

I have idea why doing that

Critical Error with Diplomacy Screen

In my Star Trek mod I have a critical bug in the diplo interface that makes it impossible to demand resources out of vassals. When I select one of their resources, I get the following python exception: Traceback (most recent call last): File "CvDiplomacyInterface", line 42, in refresh...

forums.civfanatics.com

Could you please upload a savegame where the crash occurs? I've compiled your code (DCM with 50 civ limit), but I'm not sure how to reproduce the crash.

 

[f1rpo]

Yes, it may not directly have to do with the DiploComment types. And the Python exception in the logs should not crash the whole program. So the debugger might provide more information, and I'd like to take a look, but I don't know how to bring about the crash. Do I need all 50 civs in the game? I've met two other civs in a 7-civ game, and there was no problem so far. And this is just some version of DCM with the civ limit increased, right?

I have idea why doing that

But, if you're already (possibly) solving the problem yourself, then all the better.

 

[Darkator]

I will send you a prefab of my mod and you will be able to see bug

 

[Guvnor]

Is there a way to set unit cycling so that non-combat units automatically get selected to move first?

It gets rather annoying on later turns when you have a sprawling empire to constantly have to hit / to cycle to the next worker manually. Obviously, it always makes sense to move your workers first before you move your troops so they can build roads and other infrastructure or make a dash for shelter when there's enemy combatants around.

Failing that, auto-cycling to the next unit with actions left by proximity would also be helpful, so it doesn't automatically catapult you across the map to some unit on another continent.

 

[f1rpo]

Through the DLL, yes. Most importantly, I guess, CvPlayer::updateGroupCycle and (called from there) isBeforeUnitCycle in CvGameCoreUtils. The former seems to handle the distance criterion, the latter the similarities between units. Looks like combat strength – 0 for workers, I assume – already is given a high priority for the ordering. But I guess noncombatants are selected last, not first. So perhaps just a matter of negating a single condition. Hm, or maybe isBeforeUnitCycle is actually only used for the ordering within a selection group? In that case, the iValue from plotDistance may need to be adjusted or overwritten based on an additional isBeforeUnitCycle call.

 

[f1rpo]

@Darkator: Thanks. I wasn't sure how much else there was – beside the DCM files you had posted; so I had assumed there might be a problem with DCM itself. Now I think it's probably your files that cause the problem. Having launched your full mod with the debugger attached, I got a crash as soon as I ended my starting turn. I think this was the result of some or all players having no initial civics set. Don't know how those civics normally get set; but I guess players mustn't ever be without civics. I already got warnings (failed assertions) about that when starting a new game. And I got some other warnings already during XML loading: "TRAIT_PROTECTIVE not found" (looks like this one comes from Civ4LeaderHeadInfos.xml), "UNIT_GALLEY not found" (Civ4UnitClassInfos.xml?), also Trireme and some other ships. ARTSTYLE_KISLEV, two times, Civ4CivilizationInfos.xml. These all sound like serious problems. I could also imagine that the civics issue causes the crashes involving the diplo screen – because the diplo screen normally shows the other side's current civics.

Just in case that it might be useful in the future: Selecting an "Assert" or "Debug" build from the Configuration Manager in Visual Studio will compile a DLL that causes those warning popups to be shown (though they may not appear properly when running in fullscreen mode). To see the specific cause of a failed assertion (in particular: which line causes an XML loading error), the debugger needs to be attached in addition, but the assertions alone (i.e. the warning popups) are sometimes already helpful.

 

[Leoreth]

I noticed something strange in CvCityAI::AI_specialistValue - at the end of calculating the specialist value from the GPP it produces, we divide by the great people rate modifier. That seems incorrect to me, it would make specialists less value the more GPP they generate. Can someone sanity check this for me? Is the intent that we need fewer specialists to reach a desired amount of GPP? I am unsure if this is actually a bug or not.

 

[f1rpo]

This ...

iTempValue *= getTotalGreatPeopleRateModifier();
iTempValue /= GET_PLAYER(getOwnerINLINE()).AI_averageGreatPeopleMultiplier();

... would make some sense to me – increase based on the local modifier, decrease based on the overall modifier (population-based weighted average over all cities). To avoid running specialists everywhere when all cities have high modifiers. That would be an understandable reason; although the measure would still seem too drastic ... As it is, with just the denominator, it seems that e.g. National Epic isn't taken into account at all – or only through the average multiplier, discouraging specialists. I mostly see just:

iGreatPeopleRate = GC.getSpecialistInfo(eSpecialist).getGreatPeopleRateChange();
// ...
int iGPPValue = 4;
// ...
iTempValue = (iGreatPeopleRate * iGPPValue);

In between those lines,

//iGreatPeopleRate = ((iGreatPeopleRate * getTotalGreatPeopleRateModifier()) / 100);

is commented out. I can even locate the Better AI revision that took this line out: r147
But this context doesn't really help me understand the reason. Unless the

I went mad and TORE OUT THE MULTIPLIER-ABUSING LOGIC! Bwahahahahah. Please leave it that way for now. Maybe they return one day.

refers (also) to that change.

K-Mod has largely rewritten the specialist evaluation, but it seems that a division by AI_averageGreatPeopleModifier survived the rewrite (CvCityAI::AI_jobChangeValue)

Spoiler :

			//iTempValue /= kOwner.AI_averageGreatPeopleMultiplier();
			// K-Mod note: ultimately, I don't think the value should be divided by the average multiplier.
			// because more great people points is always better, regardless of what the average multiplier is.
			// However, because of the flawed way that food is currently evaluated, I need to dilute the value of GPP
			// so that specialists don't get value more highly than food tiles. (I hope to correct this later.)
			iGPPValue *= 100;
			iGPPValue /= (300 + kOwner.AI_averageGreatPeopleMultiplier())/4;

A bit higher up, K-Mod applies the local GP rate modifier:

			iGPPValue *= getTotalGreatPeopleRateModifier();
			iGPPValue /= 100;

Maybe the concern regarding food is that the AI could fail to run a mix of specialists and food producers.

My best bet would be to try multiplying by getTotalGreatPeopleRateModifier and dividing only by a dilution of AI_averageGreatPeopleMultiplier, perhaps by (100 + kOwner.AI_averageGreatPeopleMultiplier())/2, to be conservative.

 

[Leoreth]

My best bet would be to try multiplying by getTotalGreatPeopleRateModifier and dividing only by a dilution of AI_averageGreatPeopleMultiplier, perhaps by (100 + kOwner.AI_averageGreatPeopleMultiplier())/2, to be conservative.

Thanks for investigating this and thinking it through, that seems like a good solution. I did not consider that you need a scaling factor on the GPP value to balance it against other yields, and this works better than using a constant value. I also like taking the local modifier into account, this seems to be an easy improvement to import.

Another wrinkle on this is that the average great people modifier can be 0. This is an edge case because it requires it to be zero (or negative-truncated-to-zero) in all cities, but it happened to me due to an unrelated bug - which is how I ran into this bit of code in the first place. Using the local modifier as a multiplier here also works as a great guard against this division by zero error: if it is zero, we can skip the entire calculation. If it is nonzero, the division by average should be safe.

 

[Nightinggale]

I just found a bug in We The People, which turned out to be a vanilla bug, which is also in BTS. Doesn't look fixed in the few mods I quickly looked into, so I better share what I found.

The issue is CvPlot::updateWorkingCity(). It has a line saying

if (pOldWorkingCity != pBestCity)

In other words update if there is a change. It looks so obvious that it seems that the bug in it has remained hidden in plain sight for around two decades.
I changed it to

if (pOldWorkingCity != pBestCity || (pOldWorkingCity == NULL && m_workingCity.iID != -1))

The problem is if a city is killed. This will cause getWorkingCity() to return NULL, hence pOldWorkingCity becomes NULL. If pBestCity happens to be NULL too, then nothing happens meaning m_workingCity now contains an invalid ID. getCity() returns NULL if an invalid ID is given as argument, so the code still works. The problem occurs if the original city owner happens to build a new city, which just happens to have the same ID as the lost one, something which can totally happen as the ID is now vacant. This will cause the plot to report the city as working city, but the city will fail to find the plot within range of the city plots. This in turn can push the code into undefined territory and other bugs may show up as a result.

 

[Leoreth]

Thanks for sharing!

 

[f1rpo]

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. It mustn't be delayed long enough for another CvCity to be created because that city's FFreeList ID could clash with the destroyed city. I guess the code is currently taking advantage of this by not clearing the working city directly in CvCity::kill. Instead, setWorkingCityOverride(NULL) is called directly, which in turn calls CvPlot::updateWorkingCity, which then clears m_workingCity. Except that it doesn't - which is the bug.

I feel that this whole semi-delayed update idea (if that even was the idea) is ill-conceived and that IDInfo data members should be replaced with CvCity*. IDInfo will suggest to programmers that they don't need to update upon kill, and that's -largely- incorrect. Also leads to unnecessary overhead for lookup (though much of that can be optimized away). And just for the sake of muddling update and reset into a single function. CvCity:kill already directly calls set-NULL on the other two IDInfo members (m_plotCity, m_workingCityOverride), should be easy enough to replace those with CvCity*. And a set function can be added for m_workingCity as well, to be called by CvCity::kill before setWorkingCityOverride:

Spoiler :

void CvPlot::resetWorkingCity() // public
{
	setWorkingCity(NULL);
}

void setWorkingCity(CvCity* pCity) // private
{
	CvCity* pOldCity = getWorkingCity();
	if (pOldCity == pCity)
		return;
	FAssert(pCity == NULL || isOwned());
	FAssert(pCity == NULL || !isBeingWorked());	
	m_pWorkingCity = pCity;

	if (pOldCity != NULL)
		pOldCity->setWorkingPlot(this, false);
	if (pOldCity != NULL)
		pOldCity->AI_setAssignWorkDirty(true);
	if (getWorkingCity() != NULL)
		getWorkingCity()->AI_setAssignWorkDirty(true);

	updateYield();
	updateFog();
	updateShowCitySymbols();

	if (getOwner() == GC.getGame(.getActivePlayer() &&
		gDLL->getGraphicOption(GRAPHICOPTION_CITY_RADIUS) &&
		gDLL->getInterfaceIFace()->canSelectionListFound())
	{
		gDLL->getInterfaceIFace()->setDirty(ColoredPlots_DIRTY_BIT, true);
	}
}

void updateWorkingCity() // public
{	
	CvCity* pBestCity = getPlotCity();
	if (pBestCity == NULL)
	{
		pBestCity = getWorkingCityOverride();
		FAssert(pBestCity == NULL || pBestCity->getOwner() == getOwner());
	}
	if (pBestCity == NULL && isOwned())
		pBestCity = defaultWorkingCity(); // moved into new function
	setWorkingCity(pBestCity);
}

CvCity* CvPlot::defaultWorkingCity() const // public, I guess; cut from updateWorkingCity
{
	CvCity* pBestCity = NULL;
	int iBestPriority = MAX_INT;
	for (int i = 0; i < NUM_CITY_PLOTS; i++)
	{
		CvPlot* pPlot = plotCity(getX(), getY(), i);
		if (pPlot == NULL)
			continue;
		CvCity* pCity = pPlot->getPlotCity();
		if (pCity == NULL || pCity->getOwner() != getOwner())
			continue;
		int const iPriority = GC.getCityPlotPriority()[i];
		if (iPriority < iBestPriority ||
			(iPriority == iBestPriority &&
			// XXX use getGameTurnAcquired() instead???
			(pCity->getGameTurnFounded() < pBestCity->getGameTurnFounded() ||
			(pCity->getGameTurnFounded() == pBestCity->getGameTurnFounded() &&
			pCity->getID() < pBestCity->getID()))))
		{
			iBestPriority = iPriority;
			pBestCity = pCity;
		}
	}
	return pBestCity;
}

Not yet tested; I thought I'd first check whether someone here sees a problem with this approach.

We also have ...

	IDInfo m_combatUnit;
	IDInfo m_transportUnit;

... at CvUnit, and one IDInfo at each CvCityAI and CvSelectionGroupAI:
IDInfo m_routeToCity;
IDInfo m_missionAIUnit;
When a unit is killed, it can't easily remove itself as everyone else's combat unit because that would require looping through all units, which is somewhat costly. My impression is that the code guarantees that m_combatUnit gets cleared at the end of combat resolution, so there is probably no problem here, i.e. also no problem with replacing IDInfo with a CvUnit*. m_transportUnit is handled by CvUnit::kill – conveniently, all units that have the dying unit as their transport unit get killed themselves.

m_routeToCity gets updated once per player turn (in CvCityAI::AI_doTurn):

Spoiler :

void CvCityAI::AI_updateRouteToCity()
{

	gDLL->getFAStarIFace()->ForceReset(&GC.getRouteFinder());

	int iBestValue = MAX_INT;
	CvCity pBestCity = NULL;

	for (int iI = 0; iI < MAX_PLAYERS; iI++)
	{
		// ...

							if (iValue < iBestValue)
							{
								iBestValue = iValue;
								pBestCity = pLoopCity;
							}
		// ...
	}

	if (pBestCity != NULL)
		m_routeToCity = pBestCity->getIDInfo();
	else m_routeToCity.reset();
}

No pBestCity == AI_getRouteToCity check, so this update should succeed even when there has been a collision of FFreeList IDs. However, a collision could perhaps lead to incorrect AI behavior for some portion of a game turn. Probably not possible, but, to be on the safe side and to be consistent, CvCity::kill should arguably just loop through all cities of the dying city's owner and explicitly reset the route-to-city.

m_missionAIUnit is a tough case. Seems to get used exclusively for MISSION_MOVE_TO_UNIT in BtS (K-Mod has a couple of additional uses), and only seems to get updated (overwritten) when a new mission is pushed. Meaning that, when a unit dies, AI units that were moving toward that unit will rely for several turns on the FFreeList ID not having been reassigned. Seems that the only clean remedy is a costly loop over all (friendly?) selection groups whenever a unit dies (which, of course, isn't such a frequent occurrence) or a bidirectional link, i.e. units being aware of all groups that are flocking to them. That might even perform worse due to increased memory use. Given that a clash of IDs is going to be very rare and that the consequences aren't catastrophic, perhaps it's best to leave this case alone - apart from some warnings in comments.

 

[Nightinggale]

I thought I'd first check whether someone here sees a problem with this approach.

Savegames will not work with this approach. CvMap is read prior to CvPlayer. Map reads plots while players read units and cities. In other words CvPlot::read() can't set pointers to units or cities as those haven't been allocated yet.

Obviously since the savegame code is in the DLL, it will be possible to write code to get around this limitation, but that will require some extra planning.

For the record, Colonization (and presumably BTS too) makes exe calls in the following order:

1. CvInitCore (called twice for some reason)
2. CvGameAI
3. CvMap
4. CvTeamAI (once for each instance)
5. CvPlayerAI (once for each instance)

All other classes are called from those 5 so it is possible to add code to the end of CvPlayerAI read/write that if ID == (MAX_PLAYERS - 1), then custom savegame code, which is read after everything has been allocated. I added code at that location in WTP, not for extra data, but to set a bunch of caches. Works flawlessly.


EDIT: I suppose IDInfo could be put in a union with a pointer. That way the savegame will remain unchanged, but after loading, loop all plots read the IDInfo and write the pointer instead. Other than while loading, the IDInfo will not be used. This way savegame read order will not matter.

 

[f1rpo]

Well, I'm glad that I asked. Apparently, that's the true rationale for those IDInfo members. With maybe some comments added in that vein, my point that the use of IDInfo is misleading no longer stands. If performance were a concern, the idea of caching the CvCity* in a union would be interesting, but I've already largely eliminated the relevant overhead in my own mod. But I think I'll still break up the updateWorkingCity function for better readability. Not sure about clearing m_routeToCity in CvCity:kill. Maybe one can argue that risking ID collisions is generally acceptable when they can at worst lead to nonsensical AI microdecisions for a few turns.

Edit: Oh, I see I've already applied the idea with the union to an area ID member of CvPlot:

	// CvPlot.h
	// <advc> m_pArea is enough - except while loading a savegame.
	union
	{
		CvArea* m_pArea; // This acted as a cache in BtS (was mutable)
		int m_iArea;
	}; // </advc>

 

[Walter Hawkwood]

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.

An unrelated question: are there any stock ways of creating units bypassing CvUnit::init and/or removing units bypassing CvUnit::kill, or are those functions guaranteed to be called when a unit is created/destroyed, respectively? To clarify, I am not seeking to bypass but rather looking for potential issues when a command inserted into one of those might end up not being executed (whereas it is supposed to be executed every time).