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

Gimmick Refactor #4449

Merged
merged 41 commits into from
Jun 22, 2024
Merged

Conversation

AgustinGDLV
Copy link
Collaborator

@AgustinGDLV AgustinGDLV commented Apr 26, 2024

The goal of this PR is to unify common features of generational mechanics in the codebase, provide an easy to use interface, and hopefully remove and simplify a lot of redundant code. The graphics behind a gimmick are now probably as simple as it would be to add an icon for an item, and I was able to remove a lot of redundant data in gBattleStruct.

Ideally, gimmicks become nearly as easy to implement as abilities: potentially difficult depending on complexity, but very specific to the ability itself.

New Gimmick Interface

  • Defined general struct gBattleStruct->gimmick for common variables belonging to each generational gimmick (allowing the complete removal of MegaData, BurstData, and TeraData):
struct BattleGimmickData
{
    u8 usableGimmick[MAX_BATTLERS_COUNT];
    bool8 playerSelect;
    u8 triggerSpriteId;
    u8 toActivate;
    u8 activeGimmick[NUM_BATTLE_SIDES][PARTY_SIZE];
    bool8 activated[MAX_BATTLERS_COUNT][GIMMICKS_COUNT];
};
  • Defined info struct (organized in gGimmicksInfo in data/gimmicks.h) to quickly implement common features of generational gimmicks:
struct GimmickInfo
{
    const struct SpritePalette * triggerPal;
    const struct SpriteSheet * triggerSheet;
    const struct SpriteTemplate * triggerTemplate;
    const struct SpritePalette * indicatorPal;
    const struct SpriteSheet * indicatorSheet;
    bool32 (*CanActivate)(u32 battler);
    void (*ActivateGimmick)(u32 battler);
};
  • Defined set of common functions to interact with gimmicks:
bool32 CanActivateGimmick(u32 battler, enum Gimmick gimmick);
bool32 IsGimmickSelected(u32 battler, enum Gimmick gimmick);
void SetActiveGimmick(u32 battler, enum Gimmick gimmick);
enum Gimmick GetActiveGimmick(u32 battler);
bool32 ShouldTrainerBattlerUseGimmick(u32 battler, enum Gimmick gimmick);
bool32 HasTrainerUsedGimmick(u32 battler, enum Gimmick gimmick);
void SetGimmickAsActivated(u32 battler, enum Gimmick gimmick);
  • Consolidated RET_MEGA_EVO, RET_ULTRA_BURST, etc. into RET_GIMMICK (we were out of bits, lol)
  • Consolidated triggers into one set of functions, choosing to display the first usable gimmick.
  • Consolidated indicators into one set of functions that work with the refactor.

Other Changes

  • Refactored Z-Move logic to simplify the code and eliminate the majority of the fields in ZMoveData.
  • Updated test runner to work with new interface and with Z-Moves.
  • Updated AI and trainer_proc to work with new interface.

Issues Fixed

  • Resolves Add Z-Move support in battle tests #4393 by implementing battle controller functionality for Z-Moves.
  • [TBD] Resolves Z-Moves desync Recorded Battles #3332 for the same reason.
  • Writes tests for all of the untested Z-Move status effects in Missing Battle Tests #2706.
  • Fixes Z_EFFECT_ALL_STATS_UP functionality, which had not been raised as an issue yet.
  • Fixes Z-Nature Power, Z-Copycat, Z-Me First, and Z-Sleep Talk incorrectly using a normal move and not a Z-Move, which had not been raised as an issue yet. (some of these were caught recently!)
  • Fixes Breakneck Blitz incorrectly being affected by -ate abilities, which had not been raised as an issue yet.
  • Fixes Instruct incorrectly not failing if the target last used a Z-Move, which had not been raised as an issue yet.
  • On the order of ~500-1000 lines of redundant / duplicate code removed! :^)
  • Fixes Guardian of Alola incorrectly doing 50% of the target's HP instead of 75%.

Videos

These videos are a bit out-of-date (as of 5/20/24), but nothing other than graphics should have changed.

Here!

Ultra Burst

gimmick.refactor.1.mp4

Mega

gimmick.refactor.2.mp4

Dynamax and Terastallization

gimmick.refactor.3.mp4

@Bassoonian Bassoonian marked this pull request as draft April 27, 2024 19:52
@AgustinGDLV AgustinGDLV marked this pull request as ready for review May 20, 2024 22:42
include/config.h Outdated
@@ -6,7 +6,7 @@
// still has them in the ROM. This is because the developers forgot
// to define NDEBUG before release, however this has been changed as
// Ruby's actual debug build does not use the AGBPrint features.
#define NDEBUG
// #define NDEBUG
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reminder to redefine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget this :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// #define NDEBUG
#define NDEBUG

@Bassoonian Bassoonian added this to the 1.9.0 milestone May 28, 2024
@Bassoonian
Copy link
Collaborator

There's a bunch of conflicts for you to solve before I can start reviewing :)

Copy link
Collaborator

@Bassoonian Bassoonian left a comment

Choose a reason for hiding this comment

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

Preliminary code review. @AlexOn1ine will take care of trying to break things :)

include/battle_controllers.h Outdated Show resolved Hide resolved
include/battle_gimmick.h Show resolved Hide resolved
include/battle_interface.h Show resolved Hide resolved
include/battle_interface.h Outdated Show resolved Hide resolved
include/config.h Outdated
@@ -6,7 +6,7 @@
// still has them in the ROM. This is because the developers forgot
// to define NDEBUG before release, however this has been changed as
// Ruby's actual debug build does not use the AGBPrint features.
#define NDEBUG
// #define NDEBUG
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget this :)

@@ -10247,7 +10245,7 @@ static inline uq4_12_t CalcTypeEffectivenessMultiplierInternal(u32 move, u32 mov

// Thousand Arrows ignores type modifiers for flying mons
if (!IsBattlerGrounded(battlerDef) && (gMovesInfo[move].ignoreTypeIfFlyingAndUngrounded)
&& (gBattleMons[battlerDef].type1 == TYPE_FLYING || gBattleMons[battlerDef].type2 == TYPE_FLYING || gBattleMons[battlerDef].type3 == TYPE_FLYING))
&& (GetBattlerType(battlerDef, 0, FALSE) == TYPE_FLYING || GetBattlerType(battlerDef, 1, FALSE) == TYPE_FLYING || GetBattlerType(battlerDef, 2, FALSE) == TYPE_FLYING))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of scope, but this makes me wonder if a BattlerHasType function would be useful for cases like these

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, would be useful for other stuff like type immunity to moves, such as Grass types being immune to powder moves. Let's write it down so we don't forget.

src/battle_util.c Outdated Show resolved Hide resolved
src/battle_z_move.c Outdated Show resolved Hide resolved
src/battle_z_move.c Show resolved Hide resolved
tools/trainerproc/main.c Outdated Show resolved Hide resolved
test/test_runner_battle.c Outdated Show resolved Hide resolved
test/battle/gimmick/dynamax.c Outdated Show resolved Hide resolved
test/battle/ability/ate_abilities.c Show resolved Hide resolved
src/data/graphics/gimmicks.h Outdated Show resolved Hide resolved
src/battle_controller_player_partner.c Show resolved Hide resolved
Comment on lines +12 to +13
TURN { MOVE(opponent, MOVE_TACKLE);
MOVE(player, MOVE_QUICK_ATTACK, gimmick: GIMMICK_Z_MOVE); }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
TURN { MOVE(opponent, MOVE_TACKLE);
MOVE(player, MOVE_QUICK_ATTACK, gimmick: GIMMICK_Z_MOVE); }
TURN { MOVE(opponent, MOVE_TACKLE); MOVE(player, MOVE_QUICK_ATTACK, gimmick: GIMMICK_Z_MOVE); }

test/battle/gimmick/zmove.c Outdated Show resolved Hide resolved
TURN { MOVE(opponent, MOVE_ION_DELUGE); MOVE(player, MOVE_TACKLE, gimmick: GIMMICK_Z_MOVE); }
} SCENE {
ANIMATION(ANIM_TYPE_GENERAL, B_ANIM_ZMOVE_ACTIVATE, player);
ANIMATION(ANIM_TYPE_MOVE, MOVE_BREAKNECK_BLITZ, player);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If Ion Deluge changes type, shouldn't it change the move animation to be Gigavolt Havoc?

We might need to test in USUM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bulbapedia specifically says that Breakneck Blitz is transformed into an Electric-type move, so I want to say that it stays as Breakneck Blitz, but we may need to test to confirm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If that's the case that should be fine, we can check post-merge :)

test/battle/gimmick/zmove.c Show resolved Hide resolved
test/battle/gimmick/zmove.c 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.

Just some style changes left to do. After that, should be ready to merge :)

src/battle_controller_player_partner.c Outdated Show resolved Hide resolved
src/battle_controller_opponent.c Outdated Show resolved Hide resolved
tools/trainerproc/main.c Outdated Show resolved Hide resolved
src/battle_gimmick.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.

Thanks for the change. Just maybe a minor optimization?

src/battle_terastal.c Outdated Show resolved Hide resolved
@AlexOn1ine
Copy link
Collaborator

@AsparagusEduardo Can you merge? There are two minor things that need to be addressed but they aren't urgent. I will open an issue for it.

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.

4 participants