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

Secondary/primary effects overhaul #3577

Merged

Conversation

cfmnephrite
Copy link

@cfmnephrite cfmnephrite commented Nov 18, 2023

Revamping the way move secondary/primary effects are stored and handled by adding a compound literal of AdditionalEffects to the BattleMove struct.

Description

Each AdditionalEffect looks like this and lets you define primary/secondary effects and ones that target the opponent or the attacker::

struct AdditionalEffect
{
    u8 self:1;
    u8 onlyIfTargetRaisedStats:1;
    u8 onChargeTurnOnly:1;
    u8 chance; // 0% = effect certain, primary effect
    u16 moveEffect;
};

The ADDITIONAL_EFFECTS macro let you define an array of them in a compound literal that also counts them:

#define ADDITIONAL_EFFECTS(...) EFFECTS_ARR( __VA_ARGS__ ), .numAdditionalEffects = ARRAY_COUNT(EFFECTS_ARR( __VA_ARGS__ ))

#define EFFECTS_ARR(...) (const struct AdditionalEffect[]) { __VA_ARGS__ }

Example:

[MOVE_TRIPLE_ARROWS] =
{
    #if B_UPDATED_MOVE_DATA >= GEN_9
        .power = 90,
        .pp = 10,
    #else
        .power = 50,
        .pp = 15,
    #endif
    .effect = EFFECT_HIT,
    .type = TYPE_FIGHTING,
    .accuracy = 100,
    .criticalHitStage = 1,
    .target = MOVE_TARGET_SELECTED,
    .priority = 0,
    .category = BATTLE_CATEGORY_PHYSICAL,
    .sheerForceBoost = TRUE,
    .additionalEffects = ADDITIONAL_EFFECTS({
        .moveEffect = MOVE_EFFECT_DEF_MINUS_1,
        .chance = 50,
    },
    {
        .moveEffect = MOVE_EFFECT_FLINCH,
        .chance = 30,
    }),
},

In order to call an array of multiple effects, we modify the seteffectwithchance macro (renamed to setadditionaleffects) to loop if we set the move effect to MOVE_EFFECT_CONTINUE (see below).

.macro setadditionaleffects
+1:
.byte 0x15
+jumpifhalfword CMP_EQUAL, sMOVE_EFFECT, MOVE_EFFECT_CONTINUE, 1b
.endm

As we loop through, it's necessary to track the index in gBattleStruct->additionalEffectsCounter - if this value is less than the total number of effects this move has in additionalEffects, increment the counter, set gBattlescripting.moveEffect to MOVE_EFFECT_CONTINUE and the macro calls the function again.

if (gBattleMoves[gCurrentMove].numAdditionalEffects > gBattleStruct->additionalEffectsCounter)
{
    u32 percentChance;
    const u8 *currentPtr = gBattlescriptCurrInstr;
    const struct AdditionalEffect *additionalEffect = &gBattleMoves[gCurrentMove].additionalEffects[gBattleStruct->additionalEffectsCounter];

    // Check additional effect flags
    // self-targeting move effects cannot occur multiple times per turn
    // only occur on the last setmoveeffect when there are multiple targets
    if (!(gBattleMoves[gCurrentMove].additionalEffects[gBattleStruct->additionalEffectsCounter].self
        && GetNextTarget(gBattleMoves[gCurrentMove].target, TRUE) != MAX_BATTLERS_COUNT)
        && !(additionalEffect->onlyIfTargetRaisedStats && !gProtectStructs[gBattlerTarget].statRaised)
        && additionalEffect->onChargeTurnOnly == gProtectStructs[gBattlerAttacker].chargingTurn)
    {
        percentChance = CalcSecondaryEffectChance(gBattlerAttacker, GetBattlerAbility(gBattlerAttacker), additionalEffect);

        // Activate effect if it's primary (chance == 0) or if RNGesus says so
        if ((percentChance == 0) || RandomPercentage(RNG_SECONDARY_EFFECT + gBattleStruct->additionalEffectsCounter, percentChance))
        {
            gBattleScripting.moveEffect = additionalEffect->moveEffect | (MOVE_EFFECT_AFFECTS_USER * (additionalEffect->self));

            SetMoveEffect(
                percentChance == 0, // a primary effect
                percentChance >= 100 // certain to happen
            );
        }
    }

    // Move script along if we haven't jumped elsewhere
    if (gBattlescriptCurrInstr == currentPtr)
        gBattlescriptCurrInstr = cmd->nextInstr;

    // Call setadditionaleffects again in the case of a move with multiple effects
    gBattleStruct->additionalEffectsCounter++;
    if (gBattleMoves[gCurrentMove].numAdditionalEffects > gBattleStruct->additionalEffectsCounter)
        gBattleScripting.moveEffect = MOVE_EFFECT_CONTINUE;
    else
        gBattleScripting.moveEffect = gBattleStruct->additionalEffectsCounter = 0;
}
else
{
    gBattleScripting.moveEffect = 0;
    gBattlescriptCurrInstr = cmd->nextInstr;
}

Out of scope:

  • AI improvements (much has been changed to check for move_effects rather than effects but still more to be done)
  • an issue where moves with self-targeting effects that hit multiple mons and are only supposed to activate once may not activate if the move fails to connect with the last target

Discord contact info

thechurchofcage

@cfmnephrite cfmnephrite marked this pull request as draft November 18, 2023 18:58
cfmnephrite and others added 7 commits November 19, 2023 05:05
Sorry, Lunos
Pass tests too
Now has the ability to loop over multiple effects without causing problems - requires a maybe controversial macro modification...
All work fine - all tests pass
Also tidied up paralysis/burn scripts; updated Barb Barrage
Jaw Lock needs a test but I'm too lazy to write it
Added move effect FREEZE_OR_FROSTBITE macro so that we only need that if statement once...
Two turn moves (Bounce, Freeze Shock, Sky Attack, Shadow/Phantom Force), Dire Claw, Stone Axe, Ceaseless Edge, Wicked Torque, Relic Song, Fake Out,
Added check to prevent loops in cmd_seteffectwithchance. Updated Pay Day, Tri Attack, Spectral Thief, Clear Smog, V Create, Core Enforcer
Poison Fang, Knock Off, Thunder, Hurricane, Snore; also Thief, hit_and_escape moves, recharge moves, (but didn't remove effect)
Includes Grav Apple - test that checks the AI can see its power bump needs fixing
@cfmnephrite cfmnephrite changed the title Additional effects overhaul (BattleMove refactor) Secondary/primary effects overhaul Nov 28, 2023
src/data/battle_moves.h Outdated Show resolved Hide resolved
To do: moves like CC, Overheat that LOWER stats; did NOT remove the effect for raising all stats due to an AI function
Also, Syrup Bomb (really weird script)
Modified tests to use MoveHasMoveEffect
Some util updates
@cfmnephrite
Copy link
Author

Please solve conflicts :)

Done. I've had to do this like three or four times already because by the nature of this PR, pretty much any change in upcoming to battle_scripts, battle_move_effects, or gBattleMoves will cause a conflict. If there are conflicts next time you check, please disregard and check anyway and I'll merge once more when it's approved.

data/battle_scripts_1.s 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_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_util.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@AlexOn1ine AlexOn1ine left a comment

Choose a reason for hiding this comment

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

A couple more things. Why do some effects have a 100%, some don't? E.g Thousand Waves and Spirit Shackle. Additionally can you write a test for Payday where Player and AI use them on the same turn?

Also I've noticed some minor details when going over the moves. Mighty Cleave doesn't break Protect and Sky Attack has a high critical ratio.

PR looks clean and so far I haven't found a broken move though I noticed some inconsistencies for Payday but high chance I'm wrong here.

test/battle/move_effect/throat_chop.c Outdated Show resolved Hide resolved
test/battle/move_effect/relic_song.c Outdated Show resolved Hide resolved
test/battle/move_effect/rapid_spin.c Show resolved Hide resolved
test/battle/move_effect/pledge.c Show resolved Hide resolved
test/battle/move_effect/mortal_spin.c Outdated Show resolved Hide resolved
test/battle/ai_check_viability.c Outdated Show resolved Hide resolved
test/battle/ability/leaf_guard.c Show resolved Hide resolved
Numerous AI updates and test fixes; added a test for Aura Wheel
@cfmnephrite
Copy link
Author

A couple more things. Why do some effects have a 100%, some don't? E.g Thousand Waves and Spirit Shackle. Additionally can you write a test for Payday where Player and AI use them on the same turn?

Also I've noticed some minor details when going over the moves. Mighty Cleave doesn't break Protect and Sky Attack has a high critical ratio.

PR looks clean and so far I haven't found a broken move though I noticed some inconsistencies for Payday but high chance I'm wrong here.

Thanks for checking! For anyone else checking, we discussed Thousand Waves vs Spirit Shackle here. Mighty Cleave was given EFFECT_FEINT when I started so I simply gave it the move effect - I didn't think to check what its correct effect was. I don't know what happened with Sky Attack... Both fixed now

src/battle_util.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 Show resolved Hide resolved
data/battle_scripts_1.s Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
data/battle_scripts_1.s Show resolved Hide resolved
test/battle/ai_check_viability.c Outdated Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
Not the ideal solution but Fling now has a hardcoded check for Shield Dust and acts accordingly - a better long term solution inolves making a bunch of reusable MOVE_EFFECTS and rejigging attackcanceler but I didn't feel like doing that today...
test/battle/move_effect/aura_wheel.c Outdated Show resolved Hide resolved
src/battle_script_commands.c Outdated Show resolved Hide resolved
src/battle_script_commands.c Show resolved Hide resolved
include/config/battle.h Outdated Show resolved Hide resolved
include/pokemon.h Outdated Show resolved Hide resolved
src/battle_tv.c Outdated Show resolved Hide resolved
test/battle/ability/sheer_force.c Outdated Show resolved Hide resolved
test/battle/move_effect/burn_hit.c Outdated Show resolved Hide resolved
include/random.h Show resolved Hide resolved
test/battle/move_effect/fling.c Outdated Show resolved Hide resolved
Thanks, Edu

Co-authored-by: Eduardo Quezada D'Ottone <eduardo602002@gmail.com>
src/battle_util.c Outdated Show resolved Hide resolved
src/battle_ai_main.c Outdated Show resolved Hide resolved
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.

So close 👀

src/battle_script_commands.c Outdated Show resolved Hide resolved
test/battle/move_effect/fling.c Outdated Show resolved Hide resolved
@AsparagusEduardo AsparagusEduardo merged commit 22b9337 into rh-hideout:upcoming Jan 11, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants