anonymous uploaded file InBug.7z (75.0 KiB)

Daltx commented

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

cybersphinx uploaded file burnfix.patch (1.3 KiB)

cybersphinx commented

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 commented

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 uploaded file burnfix-2.patch (2.2 KiB)

Zarel commented

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 commented

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

cybersphinx commented

Replying to Warzone2100/old-trac-import#2045 (comment:3):

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 Warzone2100/old-trac-import#2045 (comment:4):

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 uploaded file burnfix-3.patch (2.2 KiB)

Zarel commented

Replying to Warzone2100/old-trac-import#2045 (comment:6):

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 commented

Replying to Warzone2100/old-trac-import#2045 (comment:7):

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 changed status from new to closed

the_cybersphinx changed resolution from `` to fixed

the_cybersphinx commented

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

Closes #2045.