Specific Bug Reports

[Afforess]

Found a cause of an infinite loop, in CvSelectionGroup, from Better AI:

/************************************************************************************************/
/* Afforess	                  Start		 07/27/10                                               */
/*                                                                                              */
/* Infinite Loop Fix                                                                            */
/************************************************************************************************/
/*
				if( pLoopUnit->canMove() && pLoopUnit->canPillage(plot()) )
*/
				if( pLoopUnit->canMove() && pLoopUnit->canPillage(pLoopUnit->plot()) )
/************************************************************************************************/
/* Afforess	                     END                                                            */
/************************************************************************************************/

Your code appears twice in CvSelectionGroup.cpp, so this fix will need to be applied twice.

 

[Fimbulvetr]

I could have never found this one. I always thought the units inside a SelectionGroup are all standing on the same plot so plot() and pLoopUnit->plot() should, without exception, return a pointer to the same plot.
If you know why plot() alone is failing, please share the wisdom.

 

[Afforess]

I could have never found this one. I always thought the units inside a SelectionGroup are all standing on the same plot so plot() and pLoopUnit->plot() should, without exception, return a pointer to the same plot.
If you know why plot() alone is failing, please share the wisdom.

plot() returns the plot of the head unit; not where the selectiongroup is. Of course, until a few days ago, I thought selectiongroups were always on the same tile too. In fact, I think they *should* always be on the same tile; I have no idea what lead to the circumstances that created a selectiongroup over two tiles (the tiles were diagonally adjacent, IIRC); but what happen was a unit was trying to pillage, but the head unit was on a tile that couldn't be pillaged, so the pillage() call was returning false, so the loop never ended.

 

[Fimbulvetr]

Of course, until a few days ago, I thought selectiongroups were always on the same tile too.

Until this very moment, I was 100% sure of it. So much for that.
Maybe some xUPT issue, like unit gets expelled from a tile but not removed from its selectiongroup?

plot() returns the plot of the head unit;

Which I thought had to be identical, but yes I saw the difference. And in any case, if we already have a unit pointer pLoopUnit it's faster/simpler to just use that, no need to look up the head unit again each time.
It's a good change, I just couldn't imagine it woud ever make a difference. Even less a big one like that.

gj

 

[Afforess]

I don't think it's an XUPT issue, I just call jumpToNearestValidPlot() for full cities...

 

[Cybah]

Do we need both lines or just

if( pLoopUnit->canMove() && pLoopUnit->canPillage(pLoopUnit->plot()) )

instead of the other one?

 

[Fimbulvetr]

I don't think it's an XUPT issue, I just call jumpToNearestValidPlot() for full cities...

I wasn't really suspecting it was, just very wildly guessing because I still hope selectiongroups spreading over multiple tiles is not part of stock BTS.

@Cybah: except for that line, everything is commented out. So yes, that's all you need in the 2 places inside startMission.

 

[Afforess]

Fuyu, I'm adding this line here and hoping it's never triggered:

void CvSelectionGroup::doTurn()
...
while (pUnitNode != NULL)
		{
			pLoopUnit = ::getUnit(pUnitNode->m_data);
			pUnitNode = nextUnitNode(pUnitNode);
/************************************************************************************************/
/* Afforess	                  Start		 07/30/10                                               */
/*                                                                                              */
/*                                                                                              */
/************************************************************************************************/
			FAssertMsg((pLoopUnit->plot() == plot()), "Selectiongroup is not on the same tile!");
/************************************************************************************************/
/* Afforess	                     END                                                            */
/************************************************************************************************/
			pLoopUnit->doTurn();

			if (pLoopUnit->isHurt())
			{
				bHurt = true;
			}
		}

 

[Fimbulvetr]

Perfect Adding it too.

 

[EmperorFool]

I'm pretty sure looking at the SDK that a selection group must not span multiple plots. If it does, it's a bug. I've seen more than a few places that assume all units in the group are on the same plot.

 

[Afforess]

I'm pretty sure looking at the SDK that a selection group must not span multiple plots. If it does, it's a bug. I've seen more than a few places that assume all units in the group are on the same plot.

That's what I thought. My solution is logical though, and I added the assert so I can catch it next time I see it happening.

 

[sjodster]

In CvUnitAI::AI_leaveAttack()

				if (AI_plotValid(pLoopPlot))
				{
					if (pLoopPlot->isVisibleEnemyUnit(this) || (pLoopPlot->isCity() && AI_potentialEnemy(pLoopPlot->getTeam(), pLoopPlot)))
					{
/************************************************************************************************/
/* BETTER_BTS_AI_MOD                      02/22/10                                jdog5000      */
/*                                                                                              */
/* Efficiency                                                                                   */
/************************************************************************************************/
						if (pLoopPlot->getNumVisibleEnemyDefenders(this) > 0)
						{
							if (!atPlot(pLoopPlot) && (generatePath(pLoopPlot, 0, true, &iPathTurns) && (iPathTurns <= iRange)))
							{
/************************************************************************************************/
/* BETTER_BTS_AI_MOD                       END                                                  */
/************************************************************************************************/
						
								iValue = getGroup()->AI_attackOdds(pLoopPlot, true);

The isVisibleEnemyUnit() and getNumVisibleEnemyDefenders() calls are redundant (in a segment labelled "Efficiency" heh), and more significantly the latter call invalidates the isCity() second half of the "or" condition before it, which may or may not be causing a problem (depending on whether the city-related conditions were really meant to allow this code to run, which I'm not sure about).

The same situation exists in CvUnitAI::AI_anyAttack() btw, although here the getNumVisibleEnemyDefenders() call is compared against the iMinStack argument with a >= condition, so if a zero were ever passed in it would still allow the city conditions to function (and it would allow the isVisibleEnemyUnit() check to function for that matter, since it could return a Worker or something that wouldn't show up as a defender).

That code is not my doing ... the efficiency label is wrong, the only change I made in that part of AI_leaveAttack was to enforce that path distances to the new target were in fact less than the max range. So it's a bug fix.

Firaxis uses that nested structure of defense tests in a number of places, so there's probably a reason to it.

If I remember correctly (too tired to check atm) you also inverted the nest order from vanilla, which is probably why you tagged it Efficiency.

LunarMongoose is correct. I'm in the process of merging BetterAI into the Realism mod and the iRange check was already there. The order of the if statements was the modification and it looks to me like it was made solely to avoid the generatePath call. The original efficiency label was correct.

However my point stands; there is absolutely no Firaxian reason that could explain this code lol. VisibleEnemyDefenders is a more restrictive version of VisibleEnemyUnit which nulls the first half of the outer level condition outright, and b/c it's alone it also totally wastes the "|| (pLoopPlot->isCity() && AI_potentialEnemy(pLoopPlot->getTeam(), pLoopPlot)))" check, which can be true or false it doesn't matter; it can only get you through the first if.

I agree. Potential enemies are ultimately ignored due to the second conditional; and isVisibleEnemyUnit just checks for visible enemy units (could be non-military), whereas getNumVisibleEnemyDefenders checks for visible enemy military units. The entire first conditional is superfluous.

I mean, the fix to improve speed and maintain current functionality is just to completely delete the outer if with both of its irrelevant conditions, but I was hoping you could figure out whether the "|| (isCity() && AI_potentialEnemy())" thing was supposed to be there or not, b/c if it was then it needs to be written back into the function.

AI_potentialEnemy takes the players that the AI has warplans for into consideration as enemies. Including the AI_potentialEnemy check essentially means that a unit calling this function could start a war.

AI_leaveAttack is called from the following 4 methods:

1. AI_collateralMove
2. AI_reserveMove
3. AI_cityDefenseMove
4. AI_cityDefenseExtraMove

It seems to me that a unit that is supposed to be defending a city shouldn't be starting wars, even if such a war is in the AI's plan. So potential defenders should be ignored in at least the last two circumstances.

It is probably appropriate for units with the first two AIs to start wars, but they can start wars in their AI_anyAttack or AI_cityAttack calls. So my vote is to ignore potential enemies altogether in AI_leaveAttack.

-Josh

(P.S. Thank you both for all of your hard work!)

 

[sjodster]

There is an unmarked modification in CvUnitAI.cpp near line 9877. The definition of CvUnitAI::AI_shadow has been changed in BetterAI. The change is correctly marked in the .h file but not the .cpp file.

It should be something like this:

/************************************************************************************************/
/* BETTER_BTS_AI_MOD                      04/01/10                                jdog5000      */
/*                                                                                              */
/* Unit AI                                                                                      */
/************************************************************************************************/
/* original bts code
bool CvUnitAI::AI_shadow(UnitAITypes eUnitAI, int iMax, int iMaxRatio, bool bWithCargoOnly)
*/
bool CvUnitAI::AI_shadow(UnitAITypes eUnitAI, int iMax, int iMaxRatio, bool bWithCargoOnly, bool bOutsideCityOnly, int iMaxPath)
/************************************************************************************************/
/* BETTER_BTS_AI_MOD                       END                                                  */
/************************************************************************************************/

-Josh

 

[denev]

m_iStrategyRand isn't saved at initial save file.
I have added one line as below.

void CvPlayerAI::AI_init()
{
	AI_reset(false);

	//--------------------------------
	// Init other game data
	if ((GC.getInitCore().getSlotStatus(getID()) == SS_TAKEN) || (GC.getInitCore().getSlotStatus(getID()) == SS_COMPUTER))
	{
		FAssert(getPersonalityType() != NO_LEADER);
		AI_setPeaceWeight(GC.getLeaderHeadInfo(getPersonalityType()).getBasePeaceWeight() + GC.getGameINLINE().getSorenRandNum(GC.getLeaderHeadInfo(getPersonalityType()).getPeaceWeightRand(), "AI Peace Weight"));
		AI_setEspionageWeight(GC.getLeaderHeadInfo(getPersonalityType()).getEspionageWeight());
		//AI_setCivicTimer(((getMaxAnarchyTurns() == 0) ? (GC.getDefineINT("MIN_REVOLUTION_TURNS") * 2) : CIVIC_CHANGE_DELAY) / 2);
		AI_setReligionTimer(1);
		AI_setCivicTimer((getMaxAnarchyTurns() == 0) ? 1 : 2);
	}

	[COLOR="Blue"]AI_getStrategyRand();[/COLOR]
}

I don't assure that this is best method.
But I hope that it helps someone.

 

[Fimbulvetr]

Without a new random seed on reload, the numbers will be the same anyway. No need to save them.

 

[denev]

Without a new random seed on reload, the numbers will be the same anyway. No need to save them.

Initial autosave file keeps 0 (initial value of m_iStrategyRand) as m_iStrategyRand of each player.
These may be fixed after first player ends his first turn.
This means that if first player changes his action, second (or below) player's m_iStrategyRand is changed too.

I can't make an assertion that it is a bug, but I believe it is funny.

 

[LunarMongoose]

With the "New Random Seed on Reload" game option OFF, combat result chains are preserved across loads in single player universally, and they were also preserved in multiplayer UP TO A CERTAIN POINT which was years ago. Somewhere in the Warlords period, or maybe when BTS came out I don't remember, it stopped working in multiplayer and reloads always triggered new random results regardless of the game option setting. This was on my really-long-longterm list of stuff to look into and try to fix, but I wrote it off as an issue with the PRN generator code which I know zilch about and which makes my brain hurt when I try to read the mathematics involved. I know, I know... *embarassed*

ANYWAY... If Denev's fix fixes this then I will hug Denev so hard he suffocates. I don't have time to test it yet - still playing Starcraft 2 every waking minute heh - but I should be back on here soon-ish-ly. Just checking in atm heh. And thanks for your comments, sjodster.

 

[sjodster]

I found an unmarked change in CvUnitAI::AI_settlerSeaTransport() circa line 17119. The original BTS code is this:

	if (iAreaBestFoundValue > iOtherAreaBestFoundValue)
	{
		//let the settler walk.
		unloadAll();
		return true;
	}

In BetterAI the code was changed to this:

	if (iAreaBestFoundValue > iOtherAreaBestFoundValue)
	{
		//let the settler walk.
		getGroup()->unloadAll();
		getGroup()->pushMission(MISSION_SKIP);
		return true;
	}

I don't know if this was an unofficial patch or BetterAI modification.

 

[sjodster]

Found another unmarked change in CvUnitAI::AI_specialSeaTransportSpy(). The 02/23/10 efficiency modification needs to have the lower comment moved down so that it contains the entire modification.

The original code was this:

				if (iValue > 0)
				{
					if (GET_PLAYER(getOwnerINLINE()).AI_plotTargetMissionAIs(pLoopPlot, MISSIONAI_ATTACK_SPY, getGroup(), 4) == 0)
					{
						if (generatePath(pLoopPlot, 0, true, &iPathTurns))
						{
							iValue *= 1000;

							iValue /= (iPathTurns + 1);

The current BetterAI code is this:

/************************************************************************************************/
/* BETTER_BTS_AI_MOD                      02/23/10                                jdog5000      */
/*                                                                                              */
/* Efficiency                                                                                   */
/************************************************************************************************/
				iValue *= 1000;

				if (iValue > iBestValue)
				{
/************************************************************************************************/
/* BETTER_BTS_AI_MOD                       END                                                  */
/************************************************************************************************/
					if (GET_PLAYER(getOwnerINLINE()).AI_plotTargetMissionAIs(pLoopPlot, MISSIONAI_ATTACK_SPY, getGroup(), 4) == 0)
					{
						if (generatePath(pLoopPlot, 0, true, &iPathTurns))
						{
							iValue /= (iPathTurns + 1);

This line was moved above the conditional:

iValue *= 1000;

However, the deletion of the original line is unmarked. The comment should be moved down so that the code looks like this:

/************************************************************************************************/
/* BETTER_BTS_AI_MOD                      02/23/10                                jdog5000      */
/*                                                                                              */
/* Efficiency                                                                                   */
/************************************************************************************************/
				iValue *= 1000;

				if (iValue > iBestValue)
				{
					if (GET_PLAYER(getOwnerINLINE()).AI_plotTargetMissionAIs(pLoopPlot, MISSIONAI_ATTACK_SPY, getGroup(), 4) == 0)
					{
						if (generatePath(pLoopPlot, 0, true, &iPathTurns))
						{
/************************************************************************************************/
/* BETTER_BTS_AI_MOD                       END                                                  */
/************************************************************************************************/

 

[sjodster]

There's an unmarked change in bool CvUnitAI::AI_pickup(UnitAITypes eUnitAI, bool bCountProduction, int iMaxPath) circa line 19838.

The original BTS code is this:

	if ((pBestPlot != NULL) && (pBestPickupPlot != NULL))
	{
		FAssert(!atPlot(pBestPlot));
		getGroup()->pushMission(MISSION_MOVE_TO, pBestPlot->getX_INLINE(), pBestPlot->getY_INLINE(), 0, false, false, MISSIONAI_PICKUP, pBestPickupPlot);
		return true;
	}

The BetterAI code is this:

	if ((pBestPlot != NULL) && (pBestPickupPlot != NULL))
	{
		FAssert(!atPlot(pBestPlot));
		getGroup()->pushMission(MISSION_MOVE_TO, pBestPlot->getX_INLINE(), pBestPlot->getY_INLINE(), MOVE_AVOID_ENEMY_WEIGHT_3, false, false, MISSIONAI_PICKUP, pBestPickupPlot);
		return true;
	}

	return false;
}

A '0' was changed to 'MOVE_AVOID_ENEMY_WEIGHT_3'.