Mister-e - Nov 23, 2014 15:13
Masao has brought it to my attention that if you use Frost Driver with Autospell, the damage output would be significantly high than normal.
[color=rgb(40,40,40)][font=helvetica, arial, sans-serif]in battle.c line 5897:[/font][/color]
[code=auto:0]
if (sc->data[SC_SOULLINK] && sc->data[SC_SOULLINK]->val2 == SL_SAGE)
i = 0; //Max chance, no skill_lv reduction. [Skotlex]
if (i >= 50) skill_lv -= 2;
else if (i >= 15) skill_lv--;
if (skill_lv < 1) skill_lv = 1;
sp = skill->get_sp(skill_id,skill_lv) * 2 / 3;
[/code]
Should be:
[code=auto:0]
if (sc->data[SC_SPIRIT] && sc->data[SC_SPIRIT]->val2 == SL_SAGE)
i = 0; //Max chance, no skill_lv reduction. [Skotlex]
//reduction only for skill_lv > 1
if (skill_lv > 1) {
if (i >= 50) skill_lv -= 2;
else if (i >= 15) skill_lv--;
}
[/code]
Haru - Nov 23, 2014 16:48
I guess the removal of these two lines isn't intentional, right?[code=auto:0]
if (skill_lv < 1) skill_lv = 1;
sp = skill->get_sp(skill_id,skill_lv) * 2 / 3;
[/code]The fix seems flawed though, if skill_lv == 2, and i >= 50, you get a skill_lv of 0, which I don't think is what you want there, or is it?
Wouldn't it be better to fix it by changing skill_lv to a proper, signed, type (i.e. int16 instead of uint16), so the underflow doesn't happen?
Playtester - Nov 23, 2014 18:27
Well if you make sure the level isn't dropped below 1 in the first place then you don't need that check.
Could easily be fixed by using "skill_lv /= 2;" instead of "skill_lv -= 2;". Then it doesn't drop to 0. 2 -> 1. 3 -> 1 (rounded down).
Haru - Nov 23, 2014 19:26
How is /= 2 equivalent to -= 2 (assuming we want maintainable code that doesn't make assumptions on the input values)?
What's wrong with using signed data types, and avoiding the problem altogether?
Playtester - Nov 23, 2014 21:26
Well you can do that too, I'm always unsure why anybody started using unsigned types for anything really, because signed is usually always better in regards of possible overflows. I always think "if someone want through the trouble and define something unsigned, he must have known what he was doing", so I try to get around it like that.
/= 2 makes sure that 2 becomes 1 and 3 also becomes 1. What should happen with higher input values is not defined anyway. Half level or two levels lower both would be reasonable approaches.
Mister-e - Dec 7, 2014 14:50
No idea. I'm just passing info along~ So don't quote me on the source changes xD