Bug discovered in BtS Odds Calculator

[PieceOfMind]

Would you believe it? There is a small bug in the Combat Odds calculator! Fortunately, it does not have anything to do with the spearmen-defeats-tank issue.
The bug can only occur when dealing with units that have a damage cap (eg. catapults, trebuchets, cannons etc.).

These units when attacking can never kill the defender - they must always withdraw. A catapult can not damage a unit past 25HP and a cannon can not damage a unit past 20HP.

The odd thing is, when these units can dish out damage such that the damage can end up exactly on the combatLimit after a hit, the attacking unit still has to deal one more blow before he is forced to withdraw.

For example, a catapult vs. a warrior in a forest will have a iDamageToDefender value of 25HP. This means each time the catapult wins a round the warrior will take 25HP of damage.

Now you might wonder to yourself, does the catapult need to hit the warrior three times or four times before it is forced to withdraw? According to the way combat is resolved, the answer is four times, but in the BtS odds calculator it is assumed only three times. This means the calculator overestimates the retreat odds.

To prove this is the case in-game, try setting up a bunch of catapults to attack warriors in separate forests. Do a bunch of such battles (maybe 10 battles). Check through the combat log and you will see sometimes the warrior took 3 blows of 25HP but was still able to deal another hit to the catapult. This proves they were still fighting while the warrior was on 25HP - the catapult's combat limit.

Note in these pictures the upper odds ("Retreat Odds") are those predicted by the game, the middle odds "Odds" are the odds of attacker winning a given round (this line was thanks to DanF), and the Withdraw Odds shown in green are what I have calculated using my method.

Another test can be done with cannons vs. cataphracts (both having str12.). The cannon has a combatLimit of 80HP so it cannot damage a cataphract below 20HP.

The game predicts retreat odds of 63.7%. However in this case the true retreat odds are exactly 50%.


I have verified the 50% odds with the WB with 185 battles giving 93:92 odds (very close to 50:50).

How do we fix this bug?
The error is in CvGameCoreUtils::getCombatOdds.

iNeededRoundsAttacker = (std::max(0, pDefender->currHitPoints() - iDefenderHitLimit) + iDamageToDefender - [COLOR="Red"]1[/COLOR] ) / iDamageToDefender;

Fixed (I hope)

iNeededRoundsAttacker = (std::max(0, pDefender->currHitPoints() - iDefenderHitLimit) + iDamageToDefender - [B](((pAttacker->combatLimit())==GC.getMAX_HIT_POINTS())?1:0)[/B] ) / iDamageToDefender;

The reason this bug does not present problems in ordinary battles, is that normally when a unit is brought to 0HP it dies straight away. You never have to battle with a defender while the defender is sitting on 0HP! But if the cap is 25HP then you do have to battle the defender if he has just reached 25HP - it is only when you land a blow that would send him below 25HP you are forced to retreat.

I have not looked much further to see if this bug happens in other parts of the code but I don't think it does. Also, I would imagine this bug is present in all earlier versions and expansions.

This thread's second purpose is to give a sneak preview to the Advanced Combat Odds interface I'm making. I stumbled on this bug in the process.

 

[dirtyparrot]

Interesting. I think I could have played this game for years with no clue that this was happening.

 

[TheMeInTeam]

I always felt the siege odds were off, as in my siege was dying way more than expected (and different from other battles). However, I just assumed it was human bias......wow. I can't believe it. I should have trusted my perception, rather than doubting it as irrational.

It should probably be patched .

 

[PieceOfMind]

Keep in mind the bug would only pop up fairly rarely. The unit dealing the damage has to be able to damage the unit to exactly the combatLimit.

To put it simply, the difference between the defender's current hitpoints and defender's hitpoint limit needs to be an exact multiple of the damage dealt per blow to the defender.

With the catapult/forest warrior example, (100-25) is a multiple of 25, and for the cataphract/cannon, (100-20) is an exact multiple of 20.

As you can see, the lower the damage being dealt by the attacking unit (typical for suicide cats) the more likely the bug is to occur, but at the same time the less obvious the effect is going to be. To be honest I think the cannon/cataphract example must be one of the most extreme cases of it being off.

TMIT in the majority of battles the odds are correct and even in the battles where it's not correct (maybe 10% to 15% of battles on average), the discrepency would usually be small (maybe 2% to 5%). I don't think you can completely discount any human bias you might have.

 

[TheMeInTeam]

That's why I normally ignore it and assume bias unless I can prove otherwise. Something felt out of place for siege though (keep in mind it's something I'd kind of stored off after playing well over 100 games in a single year with tons and tons of siege going. I've probably sacrificed thousands of siege units!).

I'm wondering where the distributions like, and what combinations of promotions make the odds wrong .

Of course it only matters for like the first 2-4 siege units attacking anyway. After that everything defending is toast.

 

[Yakk]

Ah, and it is standard that the 'last blow' of the catapult is wasted, or does it get applied partially? I thought it did get applied to the damage cap...

In that case, the combat _should_ have ended when the warrior hit 25 HP.

 

[PieceOfMind]

Ah, and it is standard that the 'last blow' of the catapult is wasted, or does it get applied partially? I thought it did get applied to the damage cap...

not sure what you mean exactly

In that case, the combat _should_ have ended when the warrior hit 25 HP.

This is what I thought originally too. It can certainly be viewed as a bug in the way combat is resolved instead. I think it's entirely reasonable to expect the catapult to withdraw the instant it hits the cap, rather than having do successfully deal another blow of 0HP!

If you look in CvUnit::resolveCombat, it is this line that is problematic...

if (std::min(GC.getMAX_HIT_POINTS(), pDefender->getDamage() + iDefenderDamage) > combatLimit())

This line shows that combat will only cease if the current blow is about to deal more damage than is allowed by the combat limit. It does not stop the battle when the combatLimit is reached exactly.

If the inequality ">" was changed to a ">=", you could look at that as the alternative way to fix the bug.

EDIT
Yakk, I just better understood what you were asking. No, the catapult does not damage the defender to below 25HP on the last hit. The HP of the defender can not end up on anything under 25HP after the combat. So often the last damaging blow dealt by a unit like a catapult will be reduced. For example, if a catapult is attacking a defender who starts with 100HP, and the cat can deal 24HP per blow, then the catapult can damage the defender down to 28HP, but then if he hits again he will deal only 3HP damage to bring the defender to 25HP and he will immediately withdraw.

 

[dirtyparrot]

Come to think of it, as long it's the same both ways, it's a bug that evens out. The only thing that you now know is that the odds are less than what is stated, so adjust accordingly. The fact that we know this and the AI doesn't, probably gives us a very minor advantage.

 

[Yakk]

So that means, for a 25 damage attacker with a 25 combat limit, the last attack "actually does" 0 damage.

And for a 24 damage attacker with a 25 combat limit, the last attack "actually does" 3 damage.

I would consider the case of the catapult doing 0 damage on a successful hit being a bug, because ... it is pretty dumb.

So -- have it quit at the combat limit, and the simulation lines up with the actual fight, and we don't have the strange case of the catapult having to do zero damage in order to disengage.

 

[PieceOfMind]

Yakk,

I'm not in a position to call the line in resolveCombat the bug, because the way the Retreat mechanics work is a bit similar. An attacking unit can only retreat when he is 1 hit away from being killed and the defender rolls a winning combat round and the attacker retreats taking no damage from that hit.

I can completely understand taking the decision either way, but I feel less than certain about changing the way the combat rules work. Initially I hadn't even thought it could be a bug in combat because it feels a bit weird to call the rules bugged. I was only intending to go through the code to improve the interface (odds calculator only) so I just didn't think of it.

Besides on the other side of the coin, if the defender has high odds, he has a good chance of killing the catapult even when the defender is down to 25HP. Letting the catapult get away just because it got three (or however many) lucky hits in could equally be viewed as problematic.

Basically in terms of being fair to both the attacker and defender it is not clear which way it should be. I definitely don't think it's obvious that it is bugged the way it is.

 

[Yakk]

Yes -- retreating, in every case, consists of the ability to have a percent chance to negate the damage from the blow that would have killed you, and retreat instead. This is a uniform effect of being able to retreat -- it is quite clearly supposed to negate damage to you survive.

Catapult damage caps means that when you would have damaged the target under the damage cap, you instead reduce the target to the damage cap, and then retreat.

The case of "the damage you do is 0" is then a corner case. Doing 0 damage is neglected even in the combat display log. The doing 0 damage case is neglected in the odds calculation code. And 'hitting for 0 damage' is qualitatively different than every other 'catapult hits, does some damage, and then retreats' case.

Looking at the implemetation details, to make the change in the combat code is a 1 character change (having the catapult retreat when it reduces the target to the damage cap, instead of below the damage cap), which is an easy case to program the 'wrong way' regardless of which side you intended it (the same is true of the combat odds calculator). The case where it happens is only when DamagePerHit | (CurrentHP-DamageCap) exactly, so it would be easy for it to slip by any formal or informal QA process.

With all of that on my side -- I'd say it is a bug in the actual combat code. Which is, in a sense, far more important than a bug in the combat odds calculator.

 

[PieceOfMind]

Yes -- retreating, in every case, consists of the ability to have a percent chance to negate the damage from the blow that would have killed you, and retreat instead. This is a uniform effect of being able to retreat -- it is quite clearly supposed to negate damage to you survive.

Catapult damage caps means that when you would have damaged the target under the damage cap, you instead reduce the target to the damage cap, and then retreat.

The case of "the damage you do is 0" is then a corner case. Doing 0 damage is neglected even in the combat display log. The doing 0 damage case is neglected in the odds calculation code. And 'hitting for 0 damage' is qualitatively different than every other 'catapult hits, does some damage, and then retreats' case.

Looking at the implemetation details, to make the change in the combat code is a 1 character change (having the catapult retreat when it reduces the target to the damage cap, instead of below the damage cap), which is an easy case to program the 'wrong way' regardless of which side you intended it (the same is true of the combat odds calculator). The case where it happens is only when DamagePerHit | (CurrentHP-DamageCap) exactly, so it would be easy for it to slip by any formal or informal QA process.

With all of that on my side -- I'd say it is a bug in the actual combat code. Which is, in a sense, far more important than a bug in the combat odds calculator.

Well given that the way combat works is already pretty abstract, I wouldn't necessarily consider a successful combat roll by the catapult necessarily being an actual combat event. What I mean by this is, on the last roll you don't necessarily have to look at it as a hit with 0HP damage.

I look at the rule as simply being, "If I am about to damage the defender to less than 25HP then that will be my opportunity to withdraw".

I think you are assuming that catapult (or other unit with a combatLimit) has control of the combat. If a catapult is causing 7 damage per hit and the defender is causing 30 damage per hit, and the defender has just reached 25HP while the catapult is on, say, 50HP, I think it's reasonable to say the defender can keep taking shots at the catapult because he still has a good chance of killing it. No hit yet has made the defender risk having less than 25HP so there is no inconsistency there.

Although you put a good argument forward, I still don't consider it obvious that anything needs be changed in the way combat works, especially when fixing the odds display is just as easy.

You said it takes 1 character for one fix, but for the other fix it's still only two constants with a boolean expression that need be added. In fairness, the only reason the fix I suggested is a little more complicated is that it is accounting for two different possible situations - reaching the damage cap and reaching 0HP. I think it's obvious that a unit should die straight away at 0HP but not obvious at the damage cap. The change that would take 1 character in the CvUnit would only take 1 character because the checks for unit cap-withdraw and defender-death are done separately. To be precise, the check for death occurs after the check for cap-withdraw so the change is fairly simple because of that.

I might add, by the way, that the successful combat roll by the defender in the case of an attacker retreating is also omitted from the combat log, but I guess the fact the attacker retreats is enough to tell you that roll took place.

 

[PieceOfMind]

The Advanced Combat Odds mod I hinted at in the OP is finally up.

You can now rest easy knowing at least one of the values displayed is correct!

 

[H&K]

I think it quite reasonable that the unit only withdraws after its fired a shot that does "less damage" than normal.

 

[PieceOfMind]

This bug has not been fixed in BtS 3.19.

Spoiler :

So using Advanced Combat Odds will be mandatory.