Possible empty Army bug

[instafluff0]

haluu has been kind enough to help test custom mod districts for me using R26. haluu sent a SAV file to me that had an immediate crash, which using the debugger I identified as a divide by zero error.

The stack trace from the debugger ends with FUN_005bce60, specifically the line local_20 = param_1 / iVar11;.

My read is that iVar11 is the count of units currently contained in the Army, but for some reason this is zero - not sure how that could be, and I could be wrong. Anyway I wanted to get your take and bring this to your attention. I'm attaching the file here, though I think you may need ToC installed & haluu's specific scenario setup to make it work - let me know if you want me to send you the rest of the files.

Conquests Autosave 680 AD - Crash.SAV.zip

[maxpetul]

Agreed, iVar11 is the count of contained units. It's not obvious what FUN_005bce60 is for. It's called as part of the combat logic, it's a method on the Unit class, its first parameter is the unit's remaining HP, and it returns a Unit *. My guess is it's finding which unit in the army actually participates in the fight.

You don't have to send me the files. I guessed that the crash is caused by an empty army entering combat and created a little scenario that confirmed it. Normally armies can't enter combat because they've zero attack and defense strength. The Firaxis editor enforces that by disabling the attack and defense boxes when you put the Army flag on a unit type, but Quintillus's editor does not. The crash could probably be fixed by patching FUN_005bce60 to return this when it's run on an empty army unit. It already returns this for non-army units so that should be valid.

[instafluff0]

Got it, thanks for taking a look. Do you want to patch or shall I? Either way is fine.

[maxpetul]

It doesn't matter to me either. Go ahead and patch it now if you have the time otherwise I'll do it later today.

[instafluff0]

Ok, implemented in my PR here 23807e0. I confirmed this prevents the crash in haluu's save. Let me know if you want to rework anything or feel free to.

[maxpetul]

Looks alright except for a few small things:

1. Did you verify that FUN_005bce60 for sure selects an army member for combat?
2. To keep the pattern, the title of the bug in the description in all caps should be the same as the config setting that controls its fix. You have "PREVENT EMPTY ARMY FROM COMBAT" which is a bit different from "patch_empty_army_combat". Also I'd like the setting named "patch_empty_army_combat_crash" so it's more clear what the bug is.
3. Please add a line to the changelog before we forget. For simple bug fixes like this, the changelog entries are generally short and toward the end of the list.

[instafluff0]

Ok, 2 and 3 are updated 012671c.

For (1) I can't tell because I see no armies actually deployed. I ran through several more turns and it seems just normal units in combat, many (all?) of which trigger if (unit_count == 0) return this;. Which makes sense as FUN_005bce60 seems to be called for every unit but is gated by that Unit::has_ability(Army) call but - I'm guessing - ToC has the Army ability enabled for many units. I don't have a solid grasp of the combat mechanics under the hood though.

If you have your test scenario handy it may be safer to verify on your end; I'm not sure how you set it up.

[instafluff0]

Clarification from haluu:

Yeah i reviewed the save game file, and the only 'real' army that is in the game (the Skaven's Rat Tank) was not in the save file.
However, there are 3 other units I have the "Army" flag too, in order to limit them to only 1 per 3 cities (before Flintlock's amazing unit limit system). I haven't converted them to Flintlock's system yet, so I'm thinking they are the culprits, especially the one for the Andori that is actually in the save game file I shared with you for turn 680. These units cannot load units, the Army flag was used strictly as a unit limiting tool.
Here are the current units with Army flags in Tides of Crimson.
Orb Cannon (Andori) - x
Sprite (Wood Elves) - x
Fishing Boat (Archons) - x
Rat Tank (Skaven)
Army (Default vanilla Army, not available in ToC, but I was afraid to touch it in the Editor, just in case anything was hard-coded)
My hesitation with converting them to C3X's unit limit feature has to do with unit upgrades. For instance, the Sprite has a standard version "Sprite" and an upgraded version "Sprite <>". Using the Army flag, I'm able to ensure that a player can only build 1 Sprite (regardless of upgraded status) for every 3 cities. (see image below).
However, if I use the C3X unit limit functionality (unit_limits setting), each 'upgraded' version is treated as a separate unit. So i can limit each 'version' of the Sprite to 1 per 3 cities, but that opens the door to a player being able to build 1 "Sprite" for every 3 cities and an additional "Sprite <>" for every 3 cities.
With the bug fix you put in for C3X, I can still leave these units with the 'Army' flag and there should be no crashes right?

[instafluff0]

I was able to observe the Orb Cannon (Andori) unit in combat that haluu referenced - I think that is the one that was causing the crash in this case - and confirmed that it was able to enter combat normally, as far as I can tell. All other combat also seemed to be functioning normally.

[maxpetul]

I wonder how this bug wasn't discovered sooner if haluu has a bunch of different army-flagged combat units. Must be that none of them have ever actually been in combat until just now.

I confirmed that FUN_005bce60 does select army members for combat. To do that, I simply put a break point on the first call to that function then attacked with an army to see when it's called and what it returns. It's called before each round of combat and, as expected, the unit that it returns is the one that fights for that round.

One last thing: In the changelog you wrote "Fix crash caused by empty armies entering combat despite having zero attack and defense strength", but that's not what caused the crash. When army units have zero attack & defense like usual, the game doesn't crash because then empty armies can never enter combat. If they're attacked, they just get destroyed, probably by the method that handles capturing units. It's only when they're modded to have non-zero attack & defense that they can end up in combat and then the game crashes when it tries to divide by the number of units they contain.