Refactored SetMoveEffect Inner Focus Case

[lordraindance2]

Description

This fixes an issue where changing B_WAIT_TIME_MULTIPLIER to a value other than 16 (we tried 8) would break the inner focus case of SetMoveEffect. The default value of 16 "magically" ends up in the right string to print (use gdb debugger), as gBattlescriptCurrInstr++ ends up using (16 * 2 = 0x20) as the mapped function from gBattleScriptingCommandsTable.

Discord contact info

poetahto
me: accelgor

[AlexOn1ine]

There is a test file inner_focus.c. Would you be interested to check if there are tests missing and if that's the case, could you either write the complete test or make TO_DO_BATTLE_TEST so someone can do it later?

[poetahto]

The nature of the issue is undefined behaviour. Right now, the undefined instruction is dependent on the value of B_WAIT_TIME_SHORT and B_WAIT_TIME_MULTIPLIER, which with the default value of 16 happens to produce correct behavior. However, if you change B_WAIT_TIME_MULTIPLIER to any other value, you will get wildly different behavior, most likely failing the existing tests (a value of 8 fails on our project). If you change the first instruction in BattleScript_FlinchPrevention to anything other than pause B_WAIT_TIME_SHORT, you will get different undefined behavior. Thats why it might be difficult to write a test that captures this issue, since the observable cause and effects of it might change.

Rain saw this a couple times when testing; sometimes the test would timeout and fail, sometimes it would pass without side-effect, and sometimes it would pass but deal damage twice, all depending on the memory layout during BattleScript_FlinchPrevention.

*edited to be more clear about definitions

[AlexOn1ine]

...Rain saw this a couple times when testing; sometimes the test would timeout and fail, sometimes it would pass without side-effect, and sometimes it would pass but deal damage twice, all depending on the memory layout during BattleScript_FlinchPrevention.

Thanks for the explanation.