Bug Reports

[davidlallen]

Ah, thank you! Good detective work.

If you could write a short description in the "For Modders and Mergers" thread, that would be helpful I think.

Done. Thanks for your help.

 

[jdog5000]

phungus -

Thanks for the code, got the new DLL built and am trying that out now. I do think I have to build the DLL myself to use my debug setup.

Alright, here's what I've got:

Assert in CvSelectionGroupAI, line 200 (this is the place which catches units whose CvUnitAI move function doesn't actually give them an order).
UNITAI_MERCHANT
X: 48
Y: 33
Owner: 2

Assert in CvSelectionGroupAI, line 200 (
UNITAI_MERCHANT
X: 81
Y: 27
Owner: 3

Assert in CvSelectionGroupAI, line 200 (
UNITAI_MERCHANT
X: 73
Y: 56
Owner: 5

These aren't critical, but something weird is going on with merchants ...

Then, the hang during player 14's turn which goes on forever begins. The offending group has 2 units and is led by:
UNITAI_ATTACK
X: 19
Y: 42

In CvUnitAI::AI_attackMove, the unit goes through down to line 2116 where there is new logic I inserted for the Revolution mod. The unit then decides it is going to merge with a UNITAI_ATTACK_CITY group within two tiles and does so. Turns out, this group is actually just a single unit in the same plot.

Then, the cycle goes round again ... now, a UNITAI_ATTACK group in the same location comes up, but this time it has 3 units. This is where the bug rears its head. When the UNITAI_ATTACK group decided to join the UNITAI_ATTACK_CITY group, the result is supposed to be a group led by a unit with UNITAI_ATTACK_CITY ... that's how CvSelectionGroup::mergeIntoGroup is supposed to work.

However, this will only work if the UNITAI_ATTACK_CITY unit has a value greater than 0 to to the AI. Verifying your hunch, a unit which cannot capture a city is not considered a valid recipient of UNITAI_ATTACK_CITY (makes sense ... this can also be overridden in XML if you set UNITAI_ATTACK_CITY as an explicit option for a unit). So as a result of this, the merged stack is led by a unit with UNITAI_ATTACK in this case.

The AI would never have assigned UNITAI_ATTACK_CITY to the unit or built it as a UNITAI_ATTACK_CITY unit. The bug stems from the fact that in Revolution, I sometimes force UNITAI_ATTACK_CITY when creating rebel units without checking whether they can capture cities. I was not properly checking whether UNITAI_ATTACK_CITY is considered a valid UNITAI for the unit.

Now, the reason you only saw this starting with BBAI 0.7 is that in Feb I introduced some logic which causes a stack led by a UNITAI_ATTACK unit but containing a UNITAI_ATTACK_CITY unit to split apart ... this situation is never supposed to happen, the logic in AI_attackCityMove is much better for offense stack operations. So, the new 3 unit UNITAI_ATTACK stack splits into three separate stacks. Each unit then runs its own update, the two UNITAI_ATTACK units group first and then they try to merge with the UNITAI_ATTACK_CITY unit again ... rinse and repeat.

There are a couple of things I will do to fix this issue so it doesn't crop up again:
- In Revolution.py, checking whether a unit has value to the AI with a particular UNITAI in addition to the XML check.
- In CvUnitAI.cpp, changing the logic for splitting groups a bit so it can't lead to infinite hangs (group->split->group->split-> ... is one source of infinite hangs).

 

[jdog5000]

Just got a save uploaded with a critical bug from LoR (not the test build this time, it's the 0.8.2 release). Fatal hang, here is the failed assert:


This assert will keep popping up as long as I say ignore once, and then if I choose always ignore, fatal hang.

Going to work with this a bit, see if I can isolate it. It's doubful that I can convert this to a pure RevDCM save, since it's AI related, but I'll see what I can do.

There are issues with the CvUnitAI::AI_guardCity logic ... I've never seen it cause a hang before, but I've gotten this Assert several times and seen other highly unfortunate behavior (unit cycling into and out of cities). In 0.75 (I think) I already muted this assert and forced the game to do what it should in this case (not count the current unit as a defender).

Edit:
When cycling through the civs in an effort to narrow down the cause of the fatal bug to the civilization it occurs in, and hopefully locate the offending unit(s), I ran into this failed assert, I've never seen before:

Just hit "ignore" and it went away. But figured I'd note it. Also this assert was not thrown when I hit enter to simply end the turn to reproduce the fatal hang reported above. Seems odd this would trip when cycling through the civs one by one, but not when the game was running through an entire turn. (also how is it possible to return -1 when trying to retrieve the number of units?)

I have seen this before, though it's really rare. The AI keeps count of how many of each UNITAI type it has whenever a unit is initialized, killed, or has its UNITAI changed.

Looking into it ... it looks like this might happen when leaders change for a civ in the game, the player's AI variables are all reset so they don't have hold over grudges, but this also includes resetting the number of UNITAI types they think they have. Good spot!

 

[glider1]

There is also a problem with the apollo program reported at the RevolutionDCM forum post #1207. Will chase when have time but that may not be for some days.
Cheers.

 

[phungus420]

Thanks for checking up on those jdog. That's awesome you were able to figure out the exact cause.

BTW this assert:
CvSelectionGroupAI, line 200

Happens quite frequently. When I'm using the debug dll I never get the exact UnitAI type, like you are displaying. Anyway though, since you're showing it's UnitAI merchant, it's a good bet it's caused by inquisitors. One thing though, I've seen this assert happen in ancient times, before inquisitors are available.... So I don't think it's entirely caused by inquisitor logic.

Overall I apreciate the feedback. I really want to update LoR, even got glider to update RevDCM yesterday so I could do it, but now he's found a new bug related to the spacerace, and also I should probably wait on those fixes anyway. So looking forward to those fixes and the next release.

Edit:
One more thing, in the fatal hang I reported above, that you state is related to city defender logic, I can't figure out how to fix the game for the user (since I can't isolate it to the offending unit). Any ideas how I could do that? He's a few turns from winning the game, and I feel bad leaving him hanging (nice pun ).

 

[Ninja2]

Now, the reason you only saw this starting with BBAI 0.7 is that in Feb I introduced some logic which causes a stack led by a UNITAI_ATTACK unit but containing a UNITAI_ATTACK_CITY unit to split apart ... this situation is never supposed to happen, the logic in AI_attackCityMove is much better for offense stack operations. So, the new 3 unit UNITAI_ATTACK stack splits into three separate stacks. Each unit then runs its own update, the two UNITAI_ATTACK units group first and then they try to merge with the UNITAI_ATTACK_CITY unit again ... rinse and repeat.

There are a couple of things I will do to fix this issue so it doesn't crop up again:
- In Revolution.py, checking whether a unit has value to the AI with a particular UNITAI in addition to the XML check.
- In CvUnitAI.cpp, changing the logic for splitting groups a bit so it can't lead to infinite hangs (group->split->group->split-> ... is one source of infinite hangs).

Great find, jdog. I have one question: The CTD that I reported earlier in this thread had a similar situation (i.e., a group of one UNITAI_ATTACK_CITY catapult merging with a group of one UNITAI_ATTACK Keshik (horse archer). This caused a CTD, but according to your explanation I should have seen an infinite hang, not a CTD. Unless it's because I am on Vista 64. I've read that Vista is more sensitive to minor programming errors (such as infinite loops). What system are you experiencing this on? I seem to recall that phungus is on XP...

 

[jdog5000]

@phungus:

Hmmm ... two ideas:

I assume you know the offending player. If you can't find the unit for that save, you could try destroying cities. Since the problem is with guard city logic, that should be a quick path to a cure. Sure, taking out a city arbitrarily makes the game feel hacked or something, but it'll let him finish. Or, it might help you find the unit.

The cleaner approach would be to try compiling a new DLL. The offending lines are around 9866 in CvUnitAI, change them to read:

		//FAssert(pLoopUnit != this);
		if( pLoopUnit != this )
		{
			iCount++;
		}

This is what I've got in the next release of BBAI. If you go this route, please let me know whether it solves the problem (it should ...).

 

[phungus420]

OK, will try that.

And no, I couldn't find the offending player... The assert failed to trip when I cycled through the civs using the Ctrl + Shift + L switching function.

 

[jdog5000]

Great find, jdog. I have one question: The CTD that I reported earlier in this thread had a similar situation (i.e., a group of one UNITAI_ATTACK_CITY catapult merging with a group of one UNITAI_ATTACK Keshik (horse archer). This caused a CTD, but according to your explanation I should have seen an infinite hang, not a CTD. Unless it's because I am on Vista 64. I've read that Vista is more sensitive to minor programming errors (such as infinite loops). What system are you experiencing this on? I seem to recall that phungus is on XP...

The problems sound highly related, but it's tough to be sure. With only two units the particular infinite cycling I described can't occur, the group has to have more than 2 before the splitting logic bothers checking to split the group.

Do the Catapult or Keshik have any XML modifications? If not, I should be running into this issue in my BBAI tests, unless there's some weird deeper interference happening with the other components ... which happens.

I'm using XP, 32-bit.

 

[Ninja2]

Nope, those two units are pretty vanilla. They have a few changes, like upgrade options, changed power ratings, more flankingmods for the keshik, but that's pretty much it. No changes to the UNITAI's, or their ability to capture cities (totally unrelated: Should siege weapons perhaps be excluded from capturing cities altogether?). Switching the cat's UNITAI in the worldbuilder from city attack to collateral prevented the crash.

What was so interesting about it, though, was that it was so early in the game, two players, duel map, no ocean, no wars, very few units, that very little of the entire game code was being used, and certainly very little of the other dll components I have merged. I thought it would have been relatively easy to identify the crashbug, but I'm not skilled enough to do it.

Regardless, I might just have seen a fluke from the compiling of the dll. I saw this behaviour when I merged BetterAI 0.70, but after switching to 0.75, I have run maybe 25 games on AutoAI with identical settings to the save I have with the crash, and I haven't seen it since. If I do see it again, I'll report back.

 

[phungus420]

@phungus:
...

The cleaner approach would be to try compiling a new DLL. The offending lines are around 9866 in CvUnitAI, change them to read:

		//FAssert(pLoopUnit != this);
		if( pLoopUnit != this )
		{
			iCount++;
		}

This is what I've got in the next release of BBAI. If you go this route, please let me know whether it solves the problem (it should ...).

Negative. All this does is not trip the assert. The fatal hang still occurs.

Edit: I've tried WB saving the game, but the assert no longer trips (big surprise). If it had I could have exported it to the 0.9.2x test build for you to take a look at. If you do decide to download and install LoR 0.8.2 (the "official" install) to debug this, the CvDefines.h needs to be changed to 50 civs... It still has the 34 civ listing there, all the rest of the source is good to go. You may be able to use the standard RevDCM 1.02 gamecore though, the only modification to it was adding in the barbarian World challenge option, which might not have anything that would effect save games. And once again the save is here:
http://rapidshare.com/files/240677764/BeforeBlock.CivBeyondSwordSave

And again sorry about bugging you to debug stuff from a different mod from pure RevDCM, but these are the critical bug reports I get, and I don't see any showing up for RevDCM pure. I also think it's a pretty safe assumption this is caused by RevDCM, BetterAI, or default BtS code, and not something specific to LoR.

 

[jdog5000]

Negative. All this does is not trip the assert. The fatal hang still occurs.

Edit: I've tried WB saving the game, but the assert no longer trips (big surprise). If it had I could have exported it to the 0.9.2x test build for you to take a look at. If you do decide to download and install LoR 0.8.2 (the "official" install) to debug this, the CvDefines.h needs to be changed to 50 civs... It still has the 34 civ listing there, all the rest of the source is good to go. You may be able to use the standard RevDCM 1.02 gamecore though, the only modification to it was adding in the barbarian World challenge option, which might not have anything that would effect save games. And once again the save is here:
http://rapidshare.com/files/240677764/BeforeBlock.CivBeyondSwordSave

And again sorry about bugging you to debug stuff from a different mod from pure RevDCM, but these are the critical bug reports I get, and I don't see any showing up for RevDCM pure. I also think it's a pretty safe assumption this is caused by RevDCM, BetterAI, or default BtS code, and not something specific to LoR.

Drat, okay. I might get a chance to try it in 0.8.2 ... getting 0.9test going was a bit easier than expected, and I have a better sense of what goes into it now.

Don't worry about bugging me ... these things certainly seem likely to be cause by components in RevDCM and its great to have such clearly presented bugs to hunt down!

 

[davidlallen]

Recently I posted that revdcm must do some hard-coding of the features in featureinfos, because if you add a new one into the file, the game immediately deletes it from the map. I thought I could trick it by adding a new one into the middle of the file and updating NUM_GAME_FEATURES.

In fact this is not working because the game will crash, every time, after 150-200 turns. (This is in the Dune Wars mod.)

I suspect this is because the number of game features and battlefield features must match, but I would like to prove this.

Where, in the sdk code or python code, do the battlefield features get used? A quick search for "battle" in the python and sdk code did not give me any leads.

 

[Ninja2]

DCM has "Battle Effects" in CvGlobals.cpp, CvPlot.cpp, CvSelectionGroup.cpp, CvUnit.cpp, and CyGlobalContextInterface2.cpp.

 

[davidlallen]

OK ... what sources do I look in, to see that? The first post in this thread says it has the "latest sources", but they are from 2008 and searching for battle, any case, in CvPlot.cpp finds nothing. There isn't any kind of version information in the file itself. I also hunted around some other threads but I am not able to find any "canonical" source location.

(EDIT: rechecked revdcm thread, downloaded version 2.10, it does not include the sources. No links to the source I can see, even though I am sure they must be somewhere obvious.)

(EDIT: crawling more threads, I came across a link to sourceforge. Downloaded the latest source from there (March 2009). In CvPlot::doFeature() we find setBattleFeature() which does:

		{
			setBattleFeaturePrev(getFeatureType());
			setBattleFeatureVariety(getFeatureVariety());
			ivalue = getFeatureType() + GC.getNUM_GAME_FEATURES();
			ivalue2 = GC.getGameINLINE().getSorenRandNum(GC.getNUM_BATTLE_FEATURES(), "Feature type");
			setFeatureType((FeatureTypes)ivalue, ivalue2);
		}

This very strongly relies on NUM_GAME_FEATURES and NUM_BATTLE_FEATURES being set correctly, and the featureinfos entries being in the right order. If something is out of sync, setFeatureType can be passed an illegal value, resulting in a *crash*.

At a minimum, a check for an illegal value should be put here. I spent more than a day banging my head against my own code, never thinking that somebody else's code might be causing the crash.

 

[Ninja2]

I was refering to the original DCM source code. I don't know if RevDCM even includes the battlefield features, it looks like it doesn't -- otherwise, all of Dale's comments in the source may have been deleted.

 

[davidlallen]

The revdcm source code on sourceforge definitely includes battlefield effects. I highly recommend to add the following into globalaltdefines.xml:

<!-- Global Defines -->
<Civ4Defines xmlns="x-schema:CIV4GlobalDefinesSchema.xml">
	<Define>
		<!-- Set to zero to disable battlefield features -->
		<!-- Battlefield features affect the order of items in the xml/terrain/civ4featureinfos.xml file.  If you have X number of real features, they must go first in the file (before any battlefield features), and make sure NUM_GAME_FEATURES below is set to X.  If you have Y number of battlefield features, make sure NUM_BATTLE_FEATURES is set to Y.  Note the default file does this incorrectly since there are 6 real features and 7 battlefield.  Also note that if the number of features in the file is less than X + Y, the game may crash anytime a battle completes, as a nonexistent feature is referenced. -->
		<DefineName>DCM_BATTLE_EFFECTS</DefineName>
		<iDefineIntVal>1</iDefineIntVal>
	</Define>

It would also be helpful to add a comment to featureinfos, which points to this variable. The lack of a check *will* lead to unexpected crashes if people edit featureinfos in a standard way, adding a new feature to the end of the file.

(EDIT: corrections below in the thread.)

 

[jdog5000]

Alright, thanks for the hunting davidlallen. We'll try to make the DCM battle effects system a little more robust.

 

[davidlallen]

Sigh. My comment is incorrect. I read the code in CvPlot::setBattleFeature more carefully. The actual requirement is:

* If you have N real features, you must have N battlefield features, because the new feature ID is set to N + NUM_GAME_FEATURES, without checking if this is valid.

* The variable NUM_BATTLE_FEATURES actually refers to the number of varieties in the battlefield, not the number of battlefield features. The battlefield feature has 5 varieties. It would also probably cause crashes if different battlefield features, had different numbers of varieties.

So, you must have exactly N battle features, each of which must have the same number of varieties.

I am just going to turn this off by setting DCM_BATTLE_EFFECTS to zero in globaldefinesalt. I do not wish to keep all these dummy features in sync.

 

[glider1]

Johny Smith from WoC Lite has also found that battle effects seems to cause the scenario option of BTS to crash with RevDCM.
Cheers.