Skip to content
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

Gen. 9 Move Effects, 2nd batch #2870

Merged

Conversation

LOuroboros
Copy link
Collaborator

@LOuroboros LOuroboros commented Mar 28, 2023

Description

Closes #2481
Closes #2482
Closes #2484
Closes #2485
Closes #2488

Discord contact info

Lunos#4026

@LOuroboros LOuroboros force-pushed the gen_9_move_effects_batch2 branch from 4fe7d24 to d19755d Compare March 28, 2023 09:41
-Made BattleScript_EffectTakeHeart use modifybattlerstatstage
-Optimized usage of BattleScript_EffectInfernalParade
-Tweaked the way in which Triple Arrows' increased crit. hit chance is handled
-Optimized syntax for case MOVE_EFFECT_TRIPLE_ARROWS in SetMoveEffect
-Optimized case EFFECT_INFERNAL_PARADE in CalcMoveBasePower
 -Infernal Parade is not known to interact with Comatose, so Eduardo and I will extrapolate that here
-Added AI conditionals for some of the effects
-Made Triage notice Jungle Healing and moves that use its effect
@LOuroboros LOuroboros force-pushed the gen_9_move_effects_batch2 branch from d19755d to ed9e860 Compare March 28, 2023 13:09
@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Mar 28, 2023

I am not sure but the SV description for Take Heart seems to suggest that the curing effect was kept when they modified the stat changes effect.

The user lifts its spirits, curing its own status conditions and boosting its Sp. Atk and Sp. Def stats.

/Edit: Made the tests for the moves. Just waiting for Take Heart confirmation.

@LOuroboros
Copy link
Collaborator Author

I am not sure but the SV description for Take Heart seems to suggest that the curing effect was kept when they modified the stat changes effect.

The user lifts its spirits, curing its own status conditions and boosting its Sp. Atk and Sp. Def stats.

/Edit: Made the tests for the moves. Just waiting for Take Heart confirmation.

Damn, I had that in the back of my mind but I forgot to do it in the end.

I'll get to it in a bit 👀

It was missing its "heals any status condition" effect because I'm dumb and I forgot to actually go and do it.
@LOuroboros LOuroboros force-pushed the gen_9_move_effects_batch2 branch from 5cb25d8 to 5bde987 Compare March 29, 2023 03:24
@LOuroboros
Copy link
Collaborator Author

Done 👀

@LOuroboros
Copy link
Collaborator Author

Question time.

In LA, Take Heart prints specific text strings for each non-volatile status condition it can heal.

Paralysis: "[Pokémon] was cured of paralysis!"
Burn: "[Pokémon]'s burn was healed!"
Poison: "[Pokémon] was cured of its poisoning!"
07 Frostbite: "[Pokémon]'s frostbite was healed!"

Right now, in this PR it just prints "[Pokémon] status returned to normal!" just like Jungle Healing.

Sadly, its behavior in SV cannot be confirmed because the move cannot be used.

So, how should I proceed? Should I bother adding specific messages for each status, or should I keep it as it is right now?

Compilers on Unix-based systems hate it when a file doesn't end with an empty line.
src/battle_ai_main.c Outdated Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
@LOuroboros LOuroboros force-pushed the gen_9_move_effects_batch2 branch from b92c015 to 318b1ce Compare April 15, 2023 13:49
-Added EFFECT_JUNGLE_HEALING to IsHealingMoveEffect
-Modified the code for EFFECT_TAKE_HEART inside AI_CheckViability and AI_CheckBadMove
data/battle_scripts_1.s Show resolved Hide resolved
data/battle_scripts_1.s Outdated Show resolved Hide resolved
data/battle_scripts_1.s Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
src/battle_script_commands.c Show resolved Hide resolved
-Optimized BattleScript_EffectTakeHeart
-Updated the checks for EFFECT_JUNGLE_HEALING and EFFECT_TAKE_HEART in AI_CheckBadMove
-Updated the code for MOVE_EFFECT_TRIPLE_ARROWS inside SetMoveEffect
Thanks to MGriffin.
src/battle_script_commands.c Outdated Show resolved Hide resolved
src/battle_script_commands.c Outdated Show resolved Hide resolved
src/battle_script_commands.c Outdated Show resolved Hide resolved
data/battle_scripts_1.s Outdated Show resolved Hide resolved
data/battle_scripts_1.s Outdated Show resolved Hide resolved
}
}
}
break;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole section is still incredibly hacky with a lot of duplicate code. Even if it works, it would be best if cleaned up.

Copy link
Collaborator Author

@LOuroboros LOuroboros Apr 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't keep it like that because it worked, but because I couldn't come up with something better and that worked.

I've been staring really hard and scratching my peanut sized brain some more, and I think this is a nice improvement and it still seems to work. Probably the farthest I can personally go.

https://diffy.org/diff/9f85000bb97c8

EDIT: Ignore the double semicolon in the call at Cmd_seteffectwithchance. Fixed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you're giving yourself enough credit 🙂
Take it slow, and take a break if you need to. I'm sure after enough time you'll be able to figure it out! Hacky code can often lead to unwanted bugs down the line. It's better to take it slow and write something that if modified in the future will stay bug-free.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stand by my earlier statement, this section has still not been fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that, that's why I haven't resolved the conversation.

Still, I can't do more than I did here, so we're at an impasse.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just do

u8 randomLowerDefenseChance = RandomPercentage(RNG_TRIPLE_ARROWS_DEFENSE_DOWN, GetMoveSecondaryEffectChance(gBattlerAttacker, 50));
u8 randomFlinchChance = RandomPercentage(RNG_TRIPLE_ARROWS_FLINCH, GetMoveSecondaryEffectChance(gBattlerAttacker, 30));

if (randomFlinchChance
&& battlerAbility != ABILITY_INNER_FOCUS
&& GetBattlerTurnOrderNum(gEffectBattler) > gCurrentTurnActionNumber)
{
    gBattleMons[gEffectBattler].status2 |= sStatusFlagsForMoveEffects[MOVE_EFFECT_FLINCH];
}

if (randomLowerDefenseChance)
{
    BattleScriptPush(gBattlescriptCurrInstr + 1);
    gBattlescriptCurrInstr = BattleScript_DefDown;
}
else
    gBattlescriptCurrInstr++;

If your tests fail because of this change, might be worth reworking those.

Also added a function to get a move's secondaryEffectChance, meant to handle abilities, side statuses and the like that modify it.
@LOuroboros
Copy link
Collaborator Author

Solved the conflicts, pushed new tests added on last April, and also added a function to centralize the second effect chances as Skeli suggested.

I'll wait for more reviews. It's been so long and the chain of corrections has gotten so huge that it's hard to tell what else there's left to fix, update or try-to-improve besides Axe Kick.

I have no idea why is this working now when I'm pretty sure it didn't work before, but I consider this a win all the same.
jumpifstat BS_ATTACKER, CMP_LESS_THAN, STAT_SPDEF, MAX_STAT_STAGE, BattleScript_CalmMindStatRaise
goto BattleScript_CantRaiseMultipleStats

BattleScript_EffectTakeHeart_TryToRaiseStats:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed. Jump directly to Calm Mind after the ppreduce.


BattleScript_ReduceDefenseAndFlinch::
modifybattlerstatstage BS_TARGET, STAT_DEF, DECREASE, 1, BattleScript_DefDownAndFlinch_Ret, ANIM_ON
jumpifability BS_TARGET, ABILITY_INNER_FOCUS, BattleScript_DefDownAndFlinch_Ret

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not needed. Should already be covered in the flinch move effect itself.

&& !(gBattleMons[battlerAtk].status1 & STATUS1_ANY)
&& !(gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY))
score -= 10;
else if (AI_DATA->hpPercents[battlerAtk] >= 90 || AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90)
Copy link

@Skeli789 Skeli789 Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what if either the attacker or partner has a status condition? It would be better if that was checked for as well. Take for instance both the attacker and partner are at full HP but are paralyzed. The AI would choose not to use Jungle Healing because of the current logic.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what

             && !(gBattleMons[battlerAtk].status1 & STATUS1_ANY)
             && !(gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY))

is for. if they're both at max health AND both it and its partner have no status condition, reduce 10 from the score.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's how it reads currently. If both the attacker and partner are all full health and neither the attacker nor the partner have a status condition, do score - 10. So far so good. But I'm commenting on the next block that is entered if either mon isn't at full health or either mon has a status condition. This second block is saying that if either mon has more than 90% of their HP remaining, do score - 10.

So take for instance another case. Attacker is at 1 HP remaining while the partner is at full health. The AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90 condition will evaluate to TRUE causing score - 9. This is clearly not what's wanted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha, I misread it. An && would be needed for the HP percent, but it's not checking status in the second block. Do you think that's ok?

src/battle_ai_main.c Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
}
}
}
break;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stand by my earlier statement, this section has still not been fixed.

src/battle_util.c Outdated Show resolved Hide resolved
@@ -12753,29 +12753,30 @@ const struct BattleMove gBattleMoves[MOVES_COUNT_Z] =
{
#if B_UPDATED_MOVE_DATA >= GEN_9

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...This move has only been in a main series game since Gen 9? Why is this needed? (PLA is not considered a main series game in the same sense as all of the other ones).

Copy link
Collaborator Author

@LOuroboros LOuroboros Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Nintendo does seem to consider it a main game.

Regardless, as it's explained in some of the comments at include/config/battle.h, we're considering LA a Gen. 8 Game (so we can have a configuration like this one for Triple Arrows because adding sub-version labels like GEN_9_LA was rejected), and in PLA, Triple Arrows had a power of 50 and a base amount of PP of 15.

"Why is it [the config for Triple Arrows] needed?" It isn't. I highly doubt someone will want to make use of Triple Arrows' LA data which makes the move weaker, but I have to cover my bases. We have configurations for a lot of different things, and the different aspects of move data is one of them.

I'm just going with the flow, and for better or worse, the expansion wants to have configurations for every little thing possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be better in that case to invert the condition and make a special define for PLA moves (such as #ifdef PLA_MOVE_EFFECTS) since they don't belong to any gen. That way if someone wants the moves to do what they did in PLA, they'll get all moves that existed in PLA to do what they did.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since they don't belong to any gen

PLA is Gen 8, but I get what you mean. Since we're not planning on adding PLA's combat mechanics, having the PLA data for these moves and not others is a bit redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you want me to proceed then, @AsparagusEduardo?

Should I just remove the move config for the PLA moves?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say so, yeah.

@@ -12753,29 +12753,30 @@ const struct BattleMove gBattleMoves[MOVES_COUNT_Z] =
{
#if B_UPDATED_MOVE_DATA >= GEN_9

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be better in that case to invert the condition and make a special define for PLA moves (such as #ifdef PLA_MOVE_EFFECTS) since they don't belong to any gen. That way if someone wants the moves to do what they did in PLA, they'll get all moves that existed in PLA to do what they did.

.secondaryEffectChance = 100,
.target = MOVE_TARGET_SELECTED,
.priority = 0,
.flags = FLAG_PROTECT_AFFECTED | FLAG_MIRROR_MOVE_AFFECTED | FLAG_KINGS_ROCK_AFFECTED | FLAG_SHEER_FORCE_BOOST,
.flags = FLAG_PROTECT_AFFECTED | FLAG_MIRROR_MOVE_AFFECTED | FLAG_KINGS_ROCK_AFFECTED | FLAG_SHEER_FORCE_BOOST | FLAG_HIGH_CRIT,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All moves that have a flinch chance are not affected by King's Rock.

}
}
}
break;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just do

u8 randomLowerDefenseChance = RandomPercentage(RNG_TRIPLE_ARROWS_DEFENSE_DOWN, GetMoveSecondaryEffectChance(gBattlerAttacker, 50));
u8 randomFlinchChance = RandomPercentage(RNG_TRIPLE_ARROWS_FLINCH, GetMoveSecondaryEffectChance(gBattlerAttacker, 30));

if (randomFlinchChance
&& battlerAbility != ABILITY_INNER_FOCUS
&& GetBattlerTurnOrderNum(gEffectBattler) > gCurrentTurnActionNumber)
{
    gBattleMons[gEffectBattler].status2 |= sStatusFlagsForMoveEffects[MOVE_EFFECT_FLINCH];
}

if (randomLowerDefenseChance)
{
    BattleScriptPush(gBattlescriptCurrInstr + 1);
    gBattlescriptCurrInstr = BattleScript_DefDown;
}
else
    gBattlescriptCurrInstr++;

If your tests fail because of this change, might be worth reworking those.

src/battle_ai_main.c Outdated Show resolved Hide resolved
percentChance = gBattleMoves[gCurrentMove].secondaryEffectChance * 2;
else
percentChance = gBattleMoves[gCurrentMove].secondaryEffectChance;
u32 percentChance = GetMoveSecondaryEffectChance(gBattlerAttacker, gBattleMoves[gCurrentMove].secondaryEffectChance);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be cleaner to pass gCurrentMove instead of secondaryEffectChance.

Copy link
Collaborator Author

@LOuroboros LOuroboros Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably fit the function's own label better. I'll make the change.

EDIT: Is what I'd say if I didn't make this for Triple Arrows which has 2 different effects with different chances, lel.

Uh, suggestions for a new label?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about CalcMoveSecondaryEffectChance? To make it similar to CalcMoveBasePower.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. In the Discord server, Cancer Fairy and I settled for CalculateMoveSecondaryEffectChance, so we were all on pretty much the same wavelength, I'll rename and push for another review.

@@ -12753,29 +12753,30 @@ const struct BattleMove gBattleMoves[MOVES_COUNT_Z] =
{
#if B_UPDATED_MOVE_DATA >= GEN_9
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since they don't belong to any gen

PLA is Gen 8, but I get what you mean. Since we're not planning on adding PLA's combat mechanics, having the PLA data for these moves and not others is a bit redundant.

&& !(gBattleMons[battlerAtk].status1 & STATUS1_ANY)
&& !(gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY))
score -= 10;
else if (AI_DATA->hpPercents[battlerAtk] >= 90 || AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what

             && !(gBattleMons[battlerAtk].status1 & STATUS1_ANY)
             && !(gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY))

is for. if they're both at max health AND both it and its partner have no status condition, reduce 10 from the score.

Co-authored-by: Eduardo Quezada D'Ottone <eduardo602002@gmail.com>
|| gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY
|| AI_DATA->hpPercents[battlerAtk] >= 90
|| AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90)
score -= 9; // No point in healing, but should at least do it if there's nothing better or if it's afflicted by a status ailment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting a sense I already commented something like this before, but this logic is wrong. Going line by line:

gBattleMons[battlerAtk].status1 & STATUS1_ANY: This is saying if the attacker has any status condition do score -9. This completely defeats the purpose of the move since the attacker will basically try to never heal its own status condition.

gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY: Same as above except now if the partner has a status condition the AI won't use the move.

AI_DATA->hpPercents[battlerAtk] >= 90: So what happens if the partner is at 1 HP but the attacker is at full health? The AI wouldn't use this move because one of the mons on the field is already near max HP.

AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90: Same as above except now the AI won't heal itself if its partner is near max HP.

Copy link
Collaborator Author

@LOuroboros LOuroboros Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is located in a function that demotivates the AI from using the move in question if any of the conditions are met 👀

After the initial if right above which makes sure to completely demotivate the AI from using the move if the attacker or their partner are at full HP or if they don't have any status ailments, the AI would be a bit more inclined to use the move if either of them did have any status ailment or if they lacked only a bit of HP.

EDIT: To further elaborate, the function that makes them more prone to using Jungle Healing under normal circumstances would be AI_CheckViability. There, the code for Jungle Healing's effect as you can see it in this PR right now is the following:

    case EFFECT_JUNGLE_HEALING:
        if (ShouldRecover(battlerAtk, battlerDef, move, 25)
         || ShouldRecover(BATTLE_PARTNER(battlerAtk), battlerDef, move, 25)
         || gBattleMons[battlerAtk].status1 & STATUS1_ANY
         || gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY)
            score += 3;
        break;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is located in a function that demotivates the AI from using the move in question if any of the conditions are met

Yes, but let me explain how you're supposed to use AI_CheckBadMove. This function is supposed to tell the AI when it's a bad idea to do something. As in, the move won't work so the AI shouldn't use it. You've already covered the case where none of the conditions needed for Jungle Healing work (in the condition with score - 10), so fundamentally it is not a "bad idea" to use the move anymore once it's been determined it will work. Only AI_CheckViability is supposed to be used for "smart AI logic". If you don't want the AI using the move because there would be better options, you just don't give a plus score there.

There, the code for Jungle Healing's effect as you can see it in this PR right now is the following

Yes, and look what happens in this case, for instance. Say the attacker has a status condition, is at low HP, and the partner also has a status condition (but is at full HP). That's +3. But oh, wait, in AI_CheckBadMove you have gBattleMons[battlerAtk].status1 & STATUS1_ANY gives -9. That's a net -6 and the AI won't use the move (not to mention you also have AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90 so that would cause a -9 as well). Do you see what the problem is now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think I do. Thanks. I'll remove the -9 check completely.

In regards to the original -10 check, what do you think about a change like this?

        case EFFECT_JUNGLE_HEALING:
-           if (AtMaxHp(battlerAtk)
-            && AtMaxHp(BATTLE_PARTNER(battlerAtk))
-            && !(gBattleMons[battlerAtk].status1 & STATUS1_ANY)
-            && !(gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY))
+           if ((AtMaxHp(battlerAtk) && AtMaxHp(BATTLE_PARTNER(battlerAtk)))
+            || (!(gBattleMons[battlerAtk].status1 & STATUS1_ANY) && !(gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY)))
                score -= 10;
            break;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave what you had before; it was good. That's now saying if they're both at full HP or both don't have status conditions then don't use the move. You don't want that because in the case where they're both at 1 HP but still both don't have status conditions, the AI will choose not to use the move.

src/battle_ai_main.c Show resolved Hide resolved
BattleScript_DefDown_Ret:
return

BattleScript_ReduceDefenseAndFlinch::

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This battle script isn't in use anymore.

@@ -236,5 +236,6 @@ bool8 ChangeTypeBasedOnTerrain(u8 battlerId);
void RemoveConfusionStatus(u8 battlerId);
u8 GetBattlerGender(u8 battlerId);
bool8 AreBattlersOfOppositeGender(u8 battler1, u8 battler2);
u32 CalcMoveSecondaryEffectChance(u8 battlerId, u8 secondaryEffectChance);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, either rename it to CalcSecondaryEffectChance or pass in u16 move as the second argument. Right now the API is confusing because someone unfamiliar with the engine would think they had to pass in a move.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter is not viable, so I'll do the former.

|| gBattleMons[BATTLE_PARTNER(battlerAtk)].status1 & STATUS1_ANY
|| AI_DATA->hpPercents[battlerAtk] >= 90
|| AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90)
score -= 9; // No point in healing, but should at least do it if there's nothing better or if it's afflicted by a status ailment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is located in a function that demotivates the AI from using the move in question if any of the conditions are met

Yes, but let me explain how you're supposed to use AI_CheckBadMove. This function is supposed to tell the AI when it's a bad idea to do something. As in, the move won't work so the AI shouldn't use it. You've already covered the case where none of the conditions needed for Jungle Healing work (in the condition with score - 10), so fundamentally it is not a "bad idea" to use the move anymore once it's been determined it will work. Only AI_CheckViability is supposed to be used for "smart AI logic". If you don't want the AI using the move because there would be better options, you just don't give a plus score there.

There, the code for Jungle Healing's effect as you can see it in this PR right now is the following

Yes, and look what happens in this case, for instance. Say the attacker has a status condition, is at low HP, and the partner also has a status condition (but is at full HP). That's +3. But oh, wait, in AI_CheckBadMove you have gBattleMons[battlerAtk].status1 & STATUS1_ANY gives -9. That's a net -6 and the AI won't use the move (not to mention you also have AI_DATA->hpPercents[BATTLE_PARTNER(battlerAtk)] >= 90 so that would cause a -9 as well). Do you see what the problem is now?

src/battle_ai_main.c Show resolved Hide resolved
@Skeli789
Copy link

Well, Lunos, you powered through and it looks good to me now. I knew you could do it 😎

Copy link
Collaborator

@AsparagusEduardo AsparagusEduardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, Lunos, you powered through and it looks good to me now. I knew you could do it 😎

Thank you @Skeli789 for the throrough review, and thank you Lunos for your patience.
It also looks good to me, so we're ready to merge.

We can remove the PLA-specific data in a later PR.

@AsparagusEduardo AsparagusEduardo merged commit 0d86ab0 into rh-hideout:upcoming Jun 19, 2023
@LOuroboros LOuroboros deleted the gen_9_move_effects_batch2 branch June 20, 2023 01:15
@AsparagusEduardo AsparagusEduardo mentioned this pull request Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants