Burn damage calculation not fixed?

[fjodorvanveen@…]

The very same problem as last time. I was using cheats in an 8 player map, I played against 7 hard enemies all in team G. Screenshot seems irrelevant, just like last time. The enemies incendiary mortar kills all units in one shot, as well as it's fire.

[Daltx]

Added a savegame. (forgot to to change my name)

[cybersphinx]

That patch makes the code a. work and b. readable. The "if (damageToDo > 20)" block after that still looks strange, not sure if that works right, but at least we don't get "(uint)(- a bit) / 1000" damage anymore.

[Zarel]

Why'd you take out the cast to (SDWORD)? You can tell that it's Pumpkin code and not my code by the fact that it says SDWORD and not int32_t - they may have had good reason to cast to signed.

[Zarel]

This patch is cybersphinx's patch, plus a bit of cleanup, plus adding the cast to signed back, plus duplicating the code to the other place where burn damage is calculated. It is now ready for commit.

The if (damageToDo > 20) block is to limit rounding errors to +/- 5%, since damage is only done in lumps of 20. Back when damage was done in lumps of 1, rounding error was +/- 100% (since 1 damage, plus a 1.25x damage modifier, plus a 1.5x upgrade modifier, was still 1 damage after rounding down).

[Zarel]

(No, I will not be committing this fix personally; someone else should commit it, since I am on commit-sabbatical.)

[cybersphinx]

Replying to Zarel:

Why'd you take out the cast to (SDWORD)? You can tell that it's Pumpkin code and not my code by the fact that it says SDWORD and not int32_t - they may have had good reason to cast to signed.

Because casts are ugly, and it didn't seem to be needed. Is there a reason for burnDamage to be over ~2.000.000.000, which should be interpreted as negative in this case? Maybe there is a reason for it to be negative (do some units heal themselves when they burn?), but then the variable itself should be signed, and not casted.

Replying to Zarel:

This patch is cybersphinx's patch, plus a bit of cleanup, plus adding the cast to signed back, plus duplicating the code to the other place where burn damage is calculated.

Minus the testing it seems, since it doesn't compile.

It is now ready for commit.

...

The if (damageToDo > 20) block is to limit rounding errors to +/- 5%, since damage is only done in lumps of 20. Back when damage was done in lumps of 1, rounding error was +/- 100% (since 1 damage, plus a 1.25x damage modifier, plus a 1.5x upgrade modifier, was still 1 damage after rounding down).

But where is the 20 damage done? It looks like it reduces damageToDo by its %20, and then does damageToDo damage. But maybe I just don't understand it, I haven't looked that much at it.

[Zarel]

Replying to cybersphinx:

Because casts are ugly, and it didn't seem to be needed. Is there a reason for burnDamage to be over ~2.000.000.000, which should be interpreted as negative in this case? Maybe there is a reason for it to be negative (do some units heal themselves when they burn?), but then the variable itself should be signed, and not casted.

You don't use signed integers to prevent overflow, you use them to prevent underflow. It's unlikely that burnDamage would be over 2 billion. It's certainly possible that burnDamage could go negative. The fact that Pumpkin casted to signed suggests that there may have been at least one such underflow bug, and even if it were fixed, a future commit could reintroduce such a problem. Remove it if you want.

Minus the testing it seems, since it doesn't compile.

Gah, uploaded the wrong patch.

But where is the 20 damage done? It looks like it reduces damageToDo by its %20, and then does damageToDo damage. But maybe I just don't understand it, I haven't looked that much at it.

Reducing damageToDo by its %20 means that afterwards, damageToDo will be either 0 or 20. In other words, we accumulate until we have more than 20 damage, then we deal it in lumps of 20.

So (for some theoretical extremely high damage burn) damageToDo will look something like:

• 3
• 7
• 10
• 14
• 17
• 22 [deal 20 damage] 2
• 5
• 8

[cybersphinx]

Replying to Zarel:

It's certainly possible that burnDamage could go negative.

I don't see how, it's either set to zero or increased by damageToDo when that is positive.

[the_cybersphinx]

(In [11393]) Really fix burn damage calculations.

Closes #2045.