-
Notifications
You must be signed in to change notification settings - Fork 988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds Rage Fist #3573
Adds Rage Fist #3573
Conversation
Ready for review. I initially followed Bulbagarden but switched to Showdown because I think their implementation is correct and was tested by DaWoblefet |
You've got a lot of failing tests here, so probably best to fix those first :) |
Messed up merge 😅 Also I forgot to mention. I added callnative |
Applied review changes, ready for re-review! |
@@ -5537,6 +5537,16 @@ static void Cmd_moveend(void) | |||
} | |||
gBattleScripting.moveendState++; | |||
break; | |||
case MOVEEND_NUM_HITS: | |||
if (gBattlerAttacker != gBattlerTarget |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing TARGET_TURN_DAMAGED
and gBattleMons[gBattlerTarget].hp != 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TARGET_TURN_DAMAGED
As far as I understand it you don't need to take damage as long as you get hit by a damage dealing move (e.g. False Swipes).
gBattleMons[gBattlerTarget].hp != 0
What is the reason for this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand it you don't need to take damage as long as you get hit by a damage dealing move (e.g. False Swipes).
TARGET_TURN_DAMAGED
should still evaluate to true for False Swipe. If it doesn't that's a bug. You need it because otherwise this branch will be triggered by Pain Split (by virtue of the power being set to 1) even though it doesn't do direct damage.
What is the reason for this check?
Looked it up, and you actually shouldn't add it in the end. The counter's not reset upon fainting like with other counters.
Edit: You need three checks: The move isn't a status move, the move had effect, and the target took damage. No other checks are needed and you can comment if there is a check to be removed you'd like an explanation for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TARGET_TURN_DAMAGED
should still evaluate to true for False Swipe. If it doesn't that's a bug. You need it because otherwise this branch will be triggered by Pain Split (by virtue of the power being set to 1) even though it doesn't do direct damage.
Not sure what to do about TARGET_TURN_DAMAGED
. It checks for non zero damage so I would need a workaround(bug fix) for False Swipe, Endure and Focus Band since all of those can result in 0 damage, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should double check if it's actually not evaluating to true for False Swipe. Because if it is, certain branches of code like:
case ABILITY_STATIC:
if (!(gMoveResultFlags & MOVE_RESULT_NO_EFFECT)
&& gBattleMons[gBattlerAttacker].hp != 0
&& !gProtectStructs[gBattlerAttacker].confusionSelfDmg
&& TARGET_TURN_DAMAGED
&& CanBeParalyzed(gBattlerAttacker)
&& IsMoveMakingContact(move, gBattlerAttacker)
&& RandomWeighted(RNG_STATIC, 2, 1))
Will not be triggered in those cases you mentioned even though they very much should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if it is indeed not, look at how setting those values is handled in the CFRU. They're always set to at least 1
if the target is damaged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, Static and False Swipe interaction is also bugged.
As far as I can tell gHpDealt
is set to gBattleMoveDamage
and I don't see where physicalDmg
could be set to 1 if False Swipes deals 0 damage.
if (gBattleMons[battler].hp > gBattleMoveDamage)
{
gBattleMons[battler].hp -= gBattleMoveDamage;
gHpDealt = gBattleMoveDamage;
}
I looked into CFRU and unless I'm misunderstand it the only time hpDealt
can be set to 1 is if Raid Shields are up
if (HasRaidShields(gActiveBattler)) //Raid Shields can be used outside of Raid Battles now
hpDealt = MathMax(1, hpDealt); //Because damage can get heavily reduced to 0
Unless I'm looking at the wrong place but I don't know why gBattleMoveDamage
would be set to 1 in this case when Cmd_datahpupdate
is called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I checked again, and in the CFRU I've modified the macro used to checking damage was done:
#define TOOK_DAMAGE(bank) (gSpecialStatuses[bank].moveturnLostHP_physical || gSpecialStatuses[bank].moveturnLostHP_special || (gNewBS->enduredDamage & gBitTable[bank]))
Notice the gNewBS->enduredDamage & gBitTable[bank]
. That's how it overcomes the issue of the moveturnLostHP
values being 0
. You should implement this field as well to solve the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, porting that field solved the issue with False Swipe!
src/battle_script_commands.c
Outdated
@@ -5537,6 +5538,16 @@ static void Cmd_moveend(void) | |||
} | |||
gBattleScripting.moveendState++; | |||
break; | |||
case MOVEEND_NUM_HITS: | |||
if (gBattlerAttacker != gBattlerTarget | |||
&& GetBattlerSide(gBattlerAttacker) != GetBattlerSide(gBattlerTarget) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this and the line above. Confusion damage counts. The three checks after this are enough (although I would move the gBattleMoves[gCurrentMove].split != SPLIT_STATUS
to it's own line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confusion damage counts
Does it? I know Bulbagarden says it does but both Showdown and research from DaWoblefet say it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So a few things then.
- You should update Bulbapedia to reflect the details here: https://www.smogon.com/forums/threads/scarlet-violet-battle-mechanics-research.3709545/page-13#post-9409510
- You're missing the transform copying mentioned in the source above
GetBattlerSide(gBattlerAttacker) != GetBattlerSide(gBattlerTarget)
is still wrong because allies do increase the counter- You should test:
- Disguise busting counts towards damaging hits
- Damaging or breaking the user's Substitute doesn't count towards damaging hits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All done.
For Substitute I already had a test specifically to test the edge case.
include/battle.h
Outdated
@@ -732,6 +732,8 @@ struct BattleStruct | |||
bool8 transformZeroToHero[PARTY_SIZE][NUM_BATTLE_SIDES]; | |||
u8 pledgeMove:1; | |||
bool8 isSkyBattle:1; | |||
u8 timesGotHit[PARTY_SIZE][NUM_BATTLE_SIDES]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would put the battle side first and make the party size the subarray, but I see you're just following transformZeroToHero
which was also done this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity. Why do you think it's better? I wouldn't mind changing it though this is what all fields look like in the expansion if they need party and side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you'd be grouping related data. Currently the structure is:
[
[Player Party 0, Enemy Party 0],
[Player Party 1, Enemy Party 1],
...
]
But members of either party in the same slot have nothing to do with each other. By putting the [NUM_BATTLE_SIDES]
first, each party will be grouped:
[
[Player Party 0, Player Party 1, Player Party 2, ...],
[Enemy Party 1, Enemy Party 2, Enemy Party 3, ...]
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree and also changed it since it looks like there is no consistency right now anyway and transformZeroToHero
field is changed in #3614 so it should be fine. Thanks for the explanation!
* Adds Rage Fist * Fix initial implementation * fix merge * review changes * endure test * add field enduredDamage * rage fist: transform, disguise and field change * merge fix
Closes #2507