BetterAI with Dales Ranged bombardment - Assert error

[cf_nz]

Recently I've been experiencing crashes with Civ4 and have made an assert build to try and figure out the cause, unfortunately my C++ skills are limited so I'm only vaguely aware of what I'm doing. I've posted in Dales thread to see what is likely to trigger this assert but I'll post here as well. Thanks to anyone for their assistance. (My game consists of a merge of BetterAI; Dales Ranged Bombardment; Influence driven War and Route Pillage).

----------------------

Assert Failed

File: CvUnitAI.cpp
Line: 13827
Expression: pDefender != NULL
Message:

----------------------

The offending section of code; I've bolded line 13827.

// Dale - Field Bombard: modified to allow AI to use new ranged bombardment method
// Returns true if a mission was pushed...
bool CvUnitAI::AI_bombardCity()
{
    PROFILE_FUNC();

    CvCity* pCity;
    CvCity* pBombardCity;
    CvPlot* pLoopPlot;
    CvPlot* pBestPlot;
    CvUnit* pDefender;
    int iSearchRange;
    int iPotentialAttackers;
    int iValue;
    int iDamage;
    int iBestValue;
    int iDX, iDY;

    if(!canBombard(plot()))
    {
        return false;
    }
    if(DCMFieldBombard)
    {
        iSearchRange = getDCMBombRange();
        iBestValue = 0;
        pBestPlot = NULL;
        for (iDX = -(iSearchRange); iDX <= iSearchRange; iDX++)
        {
            for (iDY = -(iSearchRange); iDY <= iSearchRange; iDY++)
            {
                pLoopPlot    = plotXY(getX_INLINE(), getY_INLINE(), iDX, iDY);
                if (pLoopPlot != NULL)
                {
                    if (canBombardAtRanged(plot(), pLoopPlot->getX_INLINE(), pLoopPlot->getY_INLINE()))
                    {
                        iValue = 0;
                        pCity = pLoopPlot->getPlotCity();
                        if (pCity != NULL)
                        {
                            iValue += max(0, (min((pCity->getDefenseDamage() + airBombCurrRate()), GC.getMAX_CITY_DEFENSE_DAMAGE()) - pCity->getDefenseDamage()));
                            iValue *= 5;
                            if (pCity->AI_isDanger())
                            {
                                iValue *= 2;
                            }
                            if (pCity == pCity->area()->getTargetCity(getOwnerINLINE()))
                            {
                                iValue *= 3;
                            }
                        }
                        iPotentialAttackers = GET_PLAYER(getOwnerINLINE()).AI_adjacentPotentialAttackers(pLoopPlot);//pLoopPlot->getNumVisibleEnemyDefenders(NO_PLAYER);
                        if (iPotentialAttackers > 0 || pLoopPlot->isAdjacentTeam(getTeam()))
                        {
                            pDefender = pLoopPlot->getBestDefender(NO_PLAYER, getOwnerINLINE(), this, true);
                            [B]FAssert(pDefender != NULL);[/B]
                            FAssert(pDefender->canDefend());
                            iDamage = GC.getGameINLINE().getSorenRandNum(bombardRate(), "AI Bombard");
//                            iValue = max(0, (min((pDefender->getDamage() + iDamage), bombardRate()) - pDefender->getDamage()));
                            iValue += ((((iDamage * collateralDamage()) / 100) * min((pLoopPlot->getNumVisibleEnemyDefenders(getOwnerINLINE()) - 1), collateralDamageMaxUnits())) / 2);
                            iValue *= (3 + iPotentialAttackers);
                            iValue /= 4;
                        }
                        iValue *= GC.getGameINLINE().getSorenRandNum(20, "AI Bombard");
                        if (iValue > iBestValue)
                        {
                            iBestValue = iValue;
                            pBestPlot = pLoopPlot;
                            FAssert(!atPlot(pBestPlot));
                        }
                    }
                }
            }
        }
        if (pBestPlot != NULL)
        {
            getGroup()->pushMission(MISSION_BOMBARD, pBestPlot->getX_INLINE(), pBestPlot->getY_INLINE());
            return true;
        }
        return false;
    } else {
        if (canBombard(plot()))
        {
            pBombardCity = bombardTarget(plot());
            FAssertMsg(pBombardCity != NULL, "BombardCity is not assigned a valid value");
            
            -------BetterAI addition start-------
            // do not bombard cities with no defenders
            int iDefenderStrength = pBombardCity->plot()->AI_sumStrength(NO_PLAYER, getOwnerINLINE(), DOMAIN_LAND, true, true, false);
            if (iDefenderStrength == 0)
            {
                return false;
            }
            
            // do not bombard cities if we have overwelming odds
            int iAttackOdds = getGroup()->AI_attackOdds(pBombardCity->plot(), true);
            if (iAttackOdds > 95)
            {
                return false;
            }
            
            // could also do a compare stacks call here if we wanted, the downside of that is that we may just have a lot more units
            // we may not want to suffer high casualties just to save a turn
            //getGroup()->AI_compareStacks(pBombardCity->plot(), true, true, true);
            //int iOurStrength = pBombardCity->plot()->AI_sumStrength(getOwnerINLINE(), NO_PLAYER, DOMAIN_LAND, false, false, false)
            ----------BetterAI addition end----------

            if (pBombardCity->getDefenseDamage() < ((GC.getMAX_CITY_DEFENSE_DAMAGE() * 3) / 4))
            {
                getGroup()->pushMission(MISSION_BOMBARD);
                return true;
            }
        }
        return false;
    }
}

 

[Iustus]

It seems clear that AI_adjacentPotentialAttackers is saying that there are attackers adjacent to the potential bombard plot, however, there likely are no potential enemies on the actual bombard plot pLoopPlot->getBestDefender.

This code will cause a crash, because it is dereferencing pDefender when it is NULL.

At a bare minimum, you need to do a check for NULL before using the defender, but not understanding completely how this bombard logic is supposed to work, I hesitate to suggest an exact fix.

With this new code, can you bombard a plot and it hits every/some units in every plot next to it?

-Iustus

 

[Iustus]

Doing some further analysis, it is clear that this change is what caused your problem:

Old code:

iPotentialAttackers = pLoopPlot->getNumVisibleEnemyDefenders(NO_PLAYER);

New code:

iPotentialAttackers = GET_PLAYER(getOwnerINLINE()).AI_adjacentPotentialAttackers(pLoopPlot);

The code inside that block assumes that getNumVisibleEnemyDefenders is greater than zero, which would ensure that getBestDefender does not return NULL.

This code seems to me to be flawed in a couple senses. Does it really want to call random in this case? I would think it would be smarter to just pick the best target to bombard. In which case you would want to do this (to find the average damage):

current code:

iDamage = GC.getGameINLINE().getSorenRandNum(bombardRate(), "AI Bombard");

suggested fix:

iDamage = (bombardRate() + 1) / 2;

Of course, you could clean it up a bit more, by moving the whole "(iDamage * collateralDamage()) / 100)" term from below into this variable, probably renaming it.

It may make sense to call getBestDefender on every adjacent plot, either to pick the best of all of them, or perhaps it is not needed, and the count of the total attackers in that 3 by 3 area is sufficient to determine the best place to bombard.

Also, the new code does not actually use pDefender except in the asserts. Remove these lines, and you will fix part of the problem:

pDefender = pLoopPlot->getBestDefender(NO_PLAYER, getOwnerINLINE(), this, true);
FAssert(pDefender != NULL);
FAssert(pDefender->canDefend());

I still think there is some bad AI logic, but that will fix the asserts and crashes.

-Iustus

 

[cf_nz]

Thanks Iustus, that was a fast reply.

I've made the suggested fix and commented out the instances of pDefender.

Fingers crossed I can actually finish a game now.

Thanks again.

 

[Dale]

Actually, the problems with that posted code go a lot deeper than you think. It's got nothing to do with how AI_BombardCity() (my one) works, but calls to it from other areas of the AI.

I post the CvUnitAI.cpp changes required here:

Add the following to (near the top, under the Heal() call):

		// Dale - Field Bombard: START
		if (AI_bombardCity())
		{
			return;
		}
		// Dale - Field Bombard: END

- AI_attackMove
- AI_attackCityMove
- AI_cityDefenseMove
- AI_cityDefenseExtraMove
- AI_attackSeaMove
- AI_assaultSeaMove

In AI_anyAttack comment out the line: if (iValue >= AI_finalOddsThreshold(pLoopPlot, iOddsThreshold))

In AI_anyAttack change the pBestPlot check to:

	if (pBestPlot != NULL)
	{
		// Dale - Field Bombard: If odds are right, attack, else field bombard
		if(iBestValue > iOddsThreshold)
		{
			FAssert(!atPlot(pBestPlot));
			getGroup()->pushMission(MISSION_MOVE_TO, pBestPlot->getX_INLINE(), pBestPlot->getY_INLINE(), ((bFollow) ? MOVE_DIRECT_ATTACK : 0));
			return true;
		// Dale - Field Bombard: new method
		} else if(DCMFieldBombard) {
			if(canBombardAtRanged(pBestPlot, pBestPlot->getX_INLINE(), pBestPlot->getY_INLINE()))
			{
				getGroup()->pushMission(MISSION_BOMBARD, pBestPlot->getX_INLINE(), pBestPlot->getY_INLINE());
				return true;
			}
		// Dale - Field Bombard: old method
		} else if(!DCMFieldBombard) {
			if(canBombard(pBestPlot))
			{
				getGroup()->pushMission(MISSION_BOMBARD);
				return true;
			}
		}
	}

In AI_followBombard change the bombardCity check to: (this is the one that causes the crash)

	// Dale - Field Bombard: different AI checks under each method START
	if(DCMFieldBombard)
	{
        if (AI_bombardCity())
		{
			return true;
		}
	} else if(canBombard(plot())) {
		getGroup()->pushMission(MISSION_BOMBARD);
		return true;
	}
	// Dale - Field Bombard: different AI checks under each method END

That'll do it. Just so you know, I've successfully merged ranged bombard with Better AI and it works seamlessly with no errors. Just need those fixes.

Dale

 

[cf_nz]

Iustus

Having read your latest comments in Dales ranged bombardment thread and ways it could be improved with regards AI, I have a question.

Would you consider it to be incompatable (I use the term loosely) with BetterAI in regards the AI's ability to use it's units for maximum effect?

 

[Iustus]

Iustus

Having read your latest comments in Dales ranged bombardment thread and ways it could be improved with regards AI, I have a question.

Would you consider it to be incompatable (I use the term loosely) with BetterAI in regards the AI's ability to use it's units for maximum effect?

I would not go that far. I would recommend running under chipotle, ctrl-z and watch all the AI moves. See what it actually does. Use worldbuilder to generate some situations and see how they develop. (It is a bit tricky to get the AI to form the groups you want it to form, sometimes it may take a couple turns, but if you hold down the ctrl key, and watch what it does, you will get the hang of it).

I strongly suspect that the AI will not use these units as effectively as a human play will, or as effectively as it could with those suggested changes, but that is not the same as saying it is totally ineffective.

I predict that it does reasonably well already, the problem is that those units could be a lot more effective if they are not used directly on the attack (at least I suspect that is the case). The real question is, as a human, is it more effective to not suicide the units, but rather to always use them for ranged bombardment. If this is the case, then how often does the AI follow this strategy?

The AI is not going to do anything worse than what it would do without this mod, the issue is that by adding this feature, you may be giving a bigger advantage to the human player, because he will be able to use this new feature better than the AI will.

Let me give another completely different example to illustrate what I mean. Lets say that someone does a mod (I believe it has been done) that allows aircraft to destroy roads and railroads. Well, that would make it much more effective to use aircraft to cut off a city before attacking it, so that no defenders can stream in to either reinforce or retake a city. Now, human players will see right away the value of this technique, and use it. AI players may stumble onto it, when they decide to call in an airstrike, but they will not do it in any directed fashion.

Now the ranged bombard case is no where near as extreme. Dale has already added AI code to make the AI know about ranged bombardment, and to use it in many cases. My concern, without any testing to back it up, is that the overall grouping and flow of units will not be quite right, even if they basically are bombarding when they should.

The two things I would look for are the following:
(1) do non-bombard units in a bombard stack take action when they should? (Or are they often doing nothing when they should be attacking, or attacking when they should be doing nothing) When they do attack, is the best unit attacking, is the correct sacrifice unit being used?
(2) do bombard units range bombard instead of suiciding when they should?

The dynamics of stacks of units in Civ4 is quite complicated and convoluted, so it is something hard to track what is going on and when changes will have unintended effects.

My gut feel without any testing to back it up, is that ranged field bombardment is different enough that it might even deserve its own UnitAI type, allowing different behavior of bombardment stacks, which would allow for some more sophisticated tactics.

Another option is to repurpose the collateral unit ai, but BetterAI already repurposed that to be for defensive collateral units. I suppose you could change collateral to mean attacking bombard units, and add a city collateral to mean the defensive version of the same. Having one type for both offensive and defensive would work, but is problematic because of how floating defenders work in BetterAI. You could change it to only count collateral units that are within your cultural borders, but that would be a bit more work.

I think the bottom line is that any time you add a feature which results in new tactics, then AIs will perform better, the more you let it know about the new feature and use new tactics.

-Iustus

 

[cf_nz]

Thanks for the feedback Iustus.

The real question is, as a human, is it more effective to not suicide the units, but rather to always use them for ranged bombardment. If this is the case, then how often does the AI follow this strategy?

As part of my use of this mod I've made all siege units 'only defensive', in theory that should negate this issue.

Funny you should mention aircraft destroying roads etc, thats the Route Pillage mod that I also use. I've never really thought a lot about how they impact on the AI (not knowing how it works).

Priority one for me at this stage is to figure out why the game keeps crashing. I've only just found that by using BetterAI as the base and merging into it (rather than the reverse) I can build the assert version. I'm not convinced at this stage that the Ranged Bombardment mod is the only cause of my crash problems (I've had crashes without it), it's just the first assert (and crash) that came up.

Anyway, thanks again for the feedback and tips, I will persist with my current selection of mods and try to get things working.

 

[Iustus]

As part of my use of this mod I've made all siege units 'only defensive', in theory that should negate this issue.

This has other ramifications. With the units marked 'only defensive', I am not sure for which UnitAIs they will be built, so whether or not they will be sent out on city attack missions.

Now, someone was reporting that MGs were being build for city attack stacks (although they did not report what unit AI they were being given), so this may not be that big a problem. Of course, this may be a bug in BetterAI that we will fix, as excessive MGs should not be sent on city attack missions.

Use chipotle mode to investigate this, to see what unitAIs the AI is assigning to your units, and what groups they are getting grouped with. You will want to see them in stacks that have a head unit of type UnitAI_CityAttack (or sometimes listed as just "city attack").

I suspect you will not be getting enough bombard units getting built if they are only defensive, but I am not certain. I do not know how/if the strength of the unit is used for field bombardment, but if it is not used, you could also make the units low strength with a large defensive strength bonus. Of course, that would make them perfect candidates for sacrifice, so that would not be an ideal fix.

-Iustus

 

[cf_nz]

Good points re: defensive only units not being sent out on city attack missions. Could a work-around be to include a clause such that any defensive only siege units that can perform ranged bombardment are included in city attack missions regardless of their defensive only status?

On the subject of defensive only units, I've also made carriers and transports defensive only as I didn't want them units as offensive units. What impact will this have on their AI roles? Is it necessary under BetterAI to do this, will carriers and transports ever by used to attack?

In my current game catapults and Trebuchet are labelled as being UnitAI_Attack_City units (see screen shot). I don't know whether it's the best example as the stack is not attacking; Mehmed did declare war on me so I reckon they were destined for city attack.
-----------------

I 've had a couple more Asserts today. I'm now running under Ctl + z. I've been able to continue in both cases.

-----------------

Assert Failed

File: CvSelectionGroupAI.cpp
Line: 179
Expression: false
Message:

This is a mix of Warlords 2.08 code and BetterAI, I've tagged each line as such. I don't recall what I was doing to trigger this.

bool bFailedAlreadyFighting = false; #BetterAI addition#
    while (!bFailedAlreadyFighting && (m_bGroupAttack || readyToMove())) #BetterAI addition#
    {
        iTempHack++; #Warlords 2.08#
        if (iTempHack > 100) #Warlords 2.08#
        {
            FAssert(false); #Warlords 2.08#
            if ((pHeadUnit = getHeadUnit()) != NULL) { pHeadUnit->finishMoves(); } #BetterAI modification#
            break;
        }

-----------------
Assert Failed

File: CvGame.cpp
Line: 4052
Expression: ((pOurList->getLength() > 0) || (pTheirList->getLength() > 0))
Message: Empty trade implemented?!

This occurred while signing a cease fire. Not an issue as far as I can tell as it's only going to occur if in debug mode (ctl + z), but will mention it anyway. This is a BetterAI addition to the code.

// if debug mode (ctrl-z under chipotle), then notify the human that a deal was made
    if (GC.getGameINLINE().isDebugMode())
    {
        FAssertMsg(((pOurList->getLength() > 0) || (pTheirList->getLength() > 0)), "Empty trade implemented?!");

        PlayerTypes iActivePlayer = GC.getGameINLINE().getActivePlayer();
        
        if (iActivePlayer != eWho && iActivePlayer != eOtherWho)
        {
            bool bReportTrade = true;
            
            // special check for the triple open borders issue
            CLLNode<TradeData>* pNode = pOurList->head();
            if (pNode != NULL)
            {
                if (pNode->m_data.m_eItemType == TRADE_OPEN_BORDERS && pNode->m_data.m_iData != 0)
                {
                    bReportTrade = false;
                }
            }
            
            if (bReportTrade)
            {
                CvWString szString;
                CvWString szDealString;

                szDealString.clear();
                GAMETEXT.getDealString(szDealString, eWho, eOtherWho, pOurList, pTheirList, iActivePlayer);
                szString.Format(L"&#37;s", szDealString.GetCString());
                gDLL->getInterfaceIFace()->addMessage(iActivePlayer, true, GC.getDefineINT("EVENT_MESSAGE_TIME"), szString);
            }
        }
    }

 

[Iustus]

Good points re: defensive only units not being sent out on city attack missions. Could a work-around be to include a clause such that any defensive only siege units that can perform ranged bombardment are included in city attack missions regardless of their defensive only status?

You could do something like this, but it is not going to be as straightforward as you might think.

On the subject of defensive only units, I've also made carriers and transports defensive only as I didn't want them units as offensive units. What impact will this have on their AI roles? Is it necessary under BetterAI to do this, will carriers and transports ever by used to attack?

This should probably be fine, you might want to check to make sure that all the functions called by AI_assaultSeaMove and AI_carrierSeaMove check to make sure the unit can attack before doing some useless work. And also confirm that those units are still built for those unitAIs.

In my current game catapults and Trebuchet are labelled as being UnitAI_Attack_City units (see screen shot). I don't know whether it's the best example as the stack is not attacking; Mehmed did declare war on me so I reckon they were destined for city attack.

Try holding ctrl key over a stack when looking at it, you will get more concentrated information of the sort you want for this type of thing. It does appear that you are still building them as city attack units (although this might be a bug in BetterAI that needs to be fixed). Was the AI in question using the Crush strategy? Because that converts unit types. Look for a stack of city attack units when a civ is at peace to really be sure.

Assert Failed

File: CvSelectionGroupAI.cpp
Line: 179
Expression: false
Message:

This is a tricky one. This is one of the common bugs that occurs when something with grouping or other unit actions has been messed up. Basically you have a unit that cannot seem to push a mission. I would suggest doing a #define DEBUG_AI_UPDATE_GROUPS, and if you cannot run under the debugger, change DEBUGLOG to do a log like CvRandom.get does logging, so that you can get at what is going on. This will be a tricky one to solve.

-----------------
Assert Failed

File: CvGame.cpp
Line: 4052
Expression: ((pOurList->getLength() > 0) || (pTheirList->getLength() > 0))
Message: Empty trade implemented?!

This one you can probably ignore. It is the code that displays AI-AI trades when you have ctrl-z active. Apparently there is something strange about the cease fire trade, I am probably not logging it correctly, but it should be harmless.

-Iustus

 

[cf_nz]

This is a tricky one. This is one of the common bugs that occurs when something with grouping or other unit actions has been messed up. Basically you have a unit that cannot seem to push a mission. I would suggest doing a #define DEBUG_AI_UPDATE_GROUPS, and if you cannot run under the debugger, change DEBUGLOG to do a log like CvRandom.get does logging, so that you can get at what is going on. This will be a tricky one to solve.

Hmm, I'm very quickly getting out of my depth here. I'll see how I get on.

The main reason I'm using this mod is because I dislike the suicidal nature of the current siege units. Do you have thoughts as to how you could change siege units to not be suicidal but still fuction properly under BetterAI.

BetterAI mod is easily my favorite mod, I truly hope Firaxis incorporates it into the next expansion.

 

[Iustus]

Hmm, I'm very quickly getting out of my depth here. I'll see how I get on.

The main reason I'm using this mod is because I dislike the suicidal nature of the current siege units. Do you have thoughts as to how you could change siege units to not be suicidal but still fuction properly under BetterAI.

BetterAI mod is easily my favorite mod, I truly hope Firaxis incorporates it into the next expansion.

It sounds like you are taking the right approach, it is just not a trivial thing to do. I suggest you work with Dale, as he is the most familiar with the changes for ranged bombardment.

-Iustus

 

[Sine Nomen]

I hope you've worked out whatever problems you were having reconcilling the two mods - have you put this up for download? Please do if you haven't! I would merge them myself but I have absolutely no idea how to start

 

[Chazcon]

This thread is ancient but I found it on a search as I throw the same error as one of the posters, on the assert in this: bool CvSelectionGroupAI::AI_update()

iTempHack++;
		if (iTempHack > 100)
		{
			FAssert(false);// ChazModTEMP bug not found
			CvUnit* pHeadUnit = getHeadUnit();
			if (NULL != pHeadUnit)
			{
				if (GC.getLogging())
				{

Iustus is right it's a mother bear to figure out. Anyone have any good ideas/methods to track this down?