spectral damage off by 1 #7021
spectral damage off by 1 #7021
Conversation
the maximum damage is max-1
qndel
changed the title
ikonomov
changed the title
Source/player.cpp
Outdated
| @@ -600,7 +600,7 @@ bool PlrHitMonst(Player &player, Monster &monster, bool adjacentDamage = false) | |||
| } | |||
|
|
|||
| if (gbIsHellfire && HasAllOf(player._pIFlags, ItemSpecialEffect::FireDamage | ItemSpecialEffect::LightningDamage)) { | |||
| int midam = player._pIFMinDam + GenerateRnd(player._pIFMaxDam - player._pIFMinDam); | |||
| int midam = player._pIFMinDam + GenerateRnd(player._pIFMaxDam - player._pIFMinDam + 1); | |||
There was a problem hiding this comment.
int midam = player._pIFMinDam + GenerateRnd((player._pIFMaxDam - player._pIFMinDam) + 1);
While the order of operations here will result in the same result regardless if you add these parentheses or not, it's best practice to make the order of operations explicit, for maintainability and readability, since max - (min + 1) would produce a different result.
It wouldn't hurt to also add these parentheses in other functions that use GenerateRnd() with min and max damage however maybe best in a separate PR.
Source/player.cpp
Outdated
| @@ -920,7 +920,7 @@ bool DoRangeAttack(Player &player) | |||
| mistype = MissileID::LightningArrow; | |||
| } | |||
| if (HasAllOf(player._pIFlags, ItemSpecialEffect::FireArrows | ItemSpecialEffect::LightningArrows)) { | |||
| dmg = player._pIFMinDam + GenerateRnd(player._pIFMaxDam - player._pIFMinDam); | |||
| dmg = player._pIFMinDam + GenerateRnd(player._pIFMaxDam - player._pIFMinDam + 1); | |||
Source/player.cpp
Outdated
| @@ -600,7 +600,7 @@ bool PlrHitMonst(Player &player, Monster &monster, bool adjacentDamage = false) | |||
| } | |||
|
|
|||
| if (gbIsHellfire && HasAllOf(player._pIFlags, ItemSpecialEffect::FireDamage | ItemSpecialEffect::LightningDamage)) { | |||
| int midam = player._pIFMinDam + GenerateRnd(player._pIFMaxDam - player._pIFMinDam); | |||
| int midam = player._pIFMinDam + GenerateRnd(player._pIFMaxDam - player._pIFMinDam + 1); | |||
There was a problem hiding this comment.
Isn't there a randomIntBetween function we can use here that already does this?
There was a problem hiding this comment.
Yeah, use RandomIntBetween(player._pIFMinDam, player._pIFMaxDam). Closed ranges (where you add one to the roll to get a result back that potentially includes the max damage) are the default
There was a problem hiding this comment.
Fixed. Thanks guys
|
Interesting that you can close issues across repos like that |
Oh I hadn't noticed that! That's new to me as well, good to know. |
the maximum damage is max-1
fixes diasurgical/devilution#64 (comment)