Updated Serene Grace checks in AI_CheckViability

[LOuroboros]

Description

It doesn't look like the original author of this suggestion wants to submit a PR themselves, so I'm just gonna do it myself.

Originally brought up by Skeli#3917 in Pret's Discord server.
The checks that encourage the AI to use certain moves with secondary effects if their Pokémon has the ability Serene Grace could use an extra check, so it won't give preferential treatment to moves like Rock Tomb, which despite having a secondary effect, already have a 100% chance of activating said effect.

I wanted to add the check in a small function since that was nicer to read, something like:

bool32 DoesMoveHaveSecondaryEffectChance(u16 moveId)
{
    if (gBattleMoves[moveId].secondaryEffectChance > 0 && gBattleMoves[moveId].secondaryEffectChance < 100)
        return TRUE;
    else
        return FALSE;
}

From what I understand though, the AI calculations are pretty heavy as is, so maybe adding extra function calls without a good reason wouldn't be wise. Then again, surely, a single cheap function call wouldn't do harm. Any thoughts on that?

Also, there was double score addition for EFFECT_SPEED_DOWN_HIT using the same checks for some reason, so I removed one of them and raised the score addition for the check within if (ShouldLowerSpeed(battlerAtk, battlerDef, AI_DATA->abilities[battlerDef])) by 1.

Discord contact info

Lunos#4026

[ghoulslash]

might be cleaner to replace the u8 secondaryEffectChance to bool32 sereneGraceBoost = (AI_DATA->abilities[battlerAtk] == ABILITY_SERENE_GRACE && (gBattleMoves[move].secondaryEffectChance && gBattleMoves[move].secondaryEffectChance < 100); and then replace all of these instances with just if (sereneGraceBoost)`

[ghoulslash]

Another thing that came up in the pret server is that we don't necessarily want to boost Bubble, for example, since serene grace will only give a 20% chance, and bubble isn't that good of a move. So maybe the sereneGraceBoost check should only be true if the secondary effect chance is like 20<=x<100?

[LOuroboros]

20<=x<100

Just to make sure that I'm understanding the point correctly, you mean >= 20 && < 100, right? 👀

[ghoulslash]

20<=x<100

Just to make sure that I'm understanding the point correctly, you mean >= 20 && < 100, right? 👀

Yep!

[LOuroboros]

20<=x<100

Just to make sure that I'm understanding the point correctly, you mean >= 20 && < 100, right? 👀

Yep!

Done 👀

[AsparagusEduardo]

Now that I think about it, we should probably add a comment as to why it only checks from 20% onwards. I got confused until I read @ghoulslash's comment in this thread.

[ghoulslash]

Now that I think about it, we should probably add a comment as to why it only checks from 20% onwards. I got confused until I read @ghoulslash's comment in this thread.

Agreed. @LOuroboros can you add a comment?

[LOuroboros]

Now that I think about it, we should probably add a comment as to why it only checks from 20% onwards. I got confused until I read @ghoulslash's comment in this thread.

Agreed. @LOuroboros can you add a comment?

I meeean... sure, I don't mind, but it's probably going to look ugly since sereneGraceBoost occupies most of the line.

[LOuroboros]

Done 👀

[AsparagusEduardo]

going to look ugly since sereneGraceBoost occupie

Yeah, in those cases what I do is put the comment above the line of code needed.

[LOuroboros]

going to look ugly since sereneGraceBoost occupie

Yeah, in those cases what I do is put the comment above the line of code needed.

Alrighty.

Should I rebase and force push? This doesn't look like something that should take 5 commits, lol.

[AsparagusEduardo]

going to look ugly since sereneGraceBoost occupie

Yeah, in those cases what I do is put the comment above the line of code needed.

Alrighty.

Should I rebase and force push? This doesn't look like something that should take 5 commits, lol.

Neither? Just add another commit to your branch and normal push

[LOuroboros]

going to look ugly since sereneGraceBoost occupie

Yeah, in those cases what I do is put the comment above the line of code needed.

Alrighty.
Should I rebase and force push? This doesn't look like something that should take 5 commits, lol.

Neither? Just add another commit to your branch and normal push

I mean, I already did push the commit moving the comment.

I was asking if I should rebase and force push to clean up this PR before it's merged.

[AsparagusEduardo]

I've never seen the point of "cleaning up" PRs. If you want to see the overall changes of a PR in a single commit, that's what the merge commit is for imo

[LOuroboros]

I've never seen the point of "cleaning up" PRs.

A cleaner Git history.

[StephenLynx]

Its easier to understand a change the fewer commits it is spread across.

[AsparagusEduardo]

Alright, I'll allow this time :P

[LOuroboros]

Alright, I'll allow this time :P

k.

Done.

Ghoul's suggested changes have already been implemented, dismissing to merge.