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

[Ability] [Move] Implement Magic Bounce and Magic Coat #5225

Open
wants to merge 22 commits into
base: beta
Choose a base branch
from

Conversation

SirzBenjie
Copy link
Contributor

@SirzBenjie SirzBenjie commented Jan 31, 2025

What are the changes the user will see?

Magic Bounce and magic coat are both fully implemented, no longer marked as (N), and all interactions (other than extremely specific niche scenarios) should work as expected.

Why am I making these changes?

Magic bounce is on several pokemon, yet does not do anything at all. Mega Absol and Mega Sableye end up not having any ability.

What are the changes from a developer perspective?

Okay, there's a decent amount here, as there's quite a lot of components that need to be considered to get magic bounce working properly.

src/phases/move-effect-phase.ts

Lots of changes in this file that are needed for magic bounce. I won't go through every line here, but rather the overall.

Added a new field reflected that is true if the initiating move was reflected by the effects of magic bounce, to prevent infinite magic bounce loops, and modified the constructor to take this new parameter and set the flag accordingly.

Extracted out some logic that is also needed for magic bounce from the hitCheck method to their own functions, to adhere to DRY principles.

Modified start to properly handle magic bounce in all of the cases. See the newly added unit tests for an exhaustive list of the behaviors.
Notably, removed the Prevent ENEMY_SIDE targeted moves from occurring twice in double battles logic from the per target loop to occur prior to looping over the targets, as we need to know which target, if any, will bounce the enemy side move.
Moved some other variable definitions around (such as `isCommanding) without modifying them as they are needed earlier than their original place.

Added a queuedPhases array that will store new phases that should be added (in order) after the next Move End phase, in order to insert new move phases initiated by magic bounce. This queudPhases structure should probably be used for other moves that need to add phases after the move is ended, though that is outside the scope of this PR.

src/phases/move-phase.ts

Added a new field reflected that is true if the initiating move was reflected by the effects of magic bounce. This is checked before initiating dancer, as Feather Dance does not trigger dancer if magic bounce was bounced.

Modified the showMoveText method to check the reflected tag when determining the message in order to display the "bounced the move back" text instead of the "used the move" text.
Modified the constructor call to MoveEffectPhase to pass along the reflected field.

src/battle-scene.ts

Refactored unshiftPhase to mirror javascript's array.unshift method, allowing it to take a variable list of phases to unshift.
Refactored prependToPhase and appendToPhase to also allow taking a Phase [] instead of just Phase.

Necessary in order to insert the move-phases initiated from magic bounce after the move-end phase

src/data/ability.ts

Applied the new ReflectStatusMoveAbAttr and removed the unimplemented marker on Magic Bounce. Added an edgeCase marker on it as well, as there are some niche interactions that are currently not handled (and due to issues with moves outside the scope of this PR).

src/data/battler-tags.ts

  • Added the MagicCoatTag class for the Magic Coat move that lasts until the end of the turn and initiates the battlerTags:magicCoatOnAdd message when added.
  • Added a new case for getBattlerTag to construct and return this when getting the Magic_Coat tag.

src/data/move.ts

Removed unimplemented from Magic Coat, and have it add the new MAGIC_COAT battler tag. Gave it the failIfLastCondition condition. Marked it as edgeCase for the same reasons as the Magic Bounce ability.

Added a new move flag, REFLECTABLE and the reflectable() method which adds it when called.
Modified getTagTargetBenefitScore to mark the Magic_Coat tag have a benefit score of 3 in line with similar move effects.

Added the new REFLECTABLE tag to each move listed on bulbapedia.
Note that Sappy Seed is marked as having the reflectable flag in the game code of scarlet/violet, but not in pokerogue, as per Kev's response:

image

src/enums/battler-tag-type.ts

Added a new enum member for Magic Coat.

Tests

Added src/test/abilities/magic_bounce.test.ts with many tests for magic bounce. 4 of them are marked as TODO, related to edge cases. Despite these TODO tests, these are incredibly niche circumstances that do not warrant marking the move as partial.

Likewise added src/test/moves/magic_coat.test.ts that repeats most of the checks for magic bounce, along with a few things specific to magic coat as a move.

Screenshots/Videos

There are way, way too many specific scenarios to give evidence for every one. Every case has an automated test added.
Here are videos showing magic bounce working properly.

Demonstration of magic bounce properly bouncing growl back to both opponents in a double battle, per target that has magic bounce. Also only bouncing back spikes once:

Magic.Bounce.growl.spikes.mp4

Demonstration of magic bounce bouncing normally, respecting the immunities, and doing evasion / accuracy check against new target

Magic.Bounce.Immunity.Accuracy.mp4

Demonstration of magic coat

Magic.Coat.Basic.mp4

Demonstration of magic coat bypassing move immunities

magic.coat.bypass.immunity.and.pp.reduction.mp4

How to test the changes?

npx vitest run src/test/moves/magic_coat.test.ts src/test/abilities/magic_bounce.test.ts

Overrides I used for various test videos:
soundproof / magic coat

const overrides = {
  OPP_ABILITY_OVERRIDE: Abilities.SOUNDPROOF,
  OPP_MOVESET_OVERRIDE: [ Moves.MAGIC_COAT ],
  MOVESET_OVERRIDE: [ Moves.SPORE, Moves.GROWL ]
} satisfies Partial<InstanceType<typeof DefaultOverrides>>;

Fly / spikes

const overrides = {
  OPP_MOVESET_OVERRIDE: [ Moves.FLY ],
  OPP_ABILITY_OVERRIDE: Abilities.MAGIC_BOUNCE,
  OPP_SPECIES_OVERRIDE: Species.REGIELEKI,
  STARTING_LEVEL_OVERRIDE: 100,
  MOVESET_OVERRIDE: [ Moves.SPIKES, Moves.CHARM, Moves.SPLASH ],
  ABILITY_OVERRIDE: Abilities.STALL,
} satisfies Partial<InstanceType<typeof DefaultOverrides>>;

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR?
  • The PR is self-contained and cannot be split into smaller PRs? (see below)
  • Have I provided a clear explanation of the changes?
  • Have I tested the changes manually?
  • Are all unit tests still passing? (npm run test)
    • Have I created new automated tests (npm run create-test) or updated existing tests related to the PR's changes?
  • Have I provided screenshots/videos of the changes (if applicable)?
    • [ ] Have I made sure that any UI change works for both UI themes (default and legacy)?

Are there any localization additions or changes? If so:

Additional Notes

I didn't want to split this into multiple PRs even though there is a sizeable code change. The logic for magic coat and magic bounce is exactly the same, (other than magic coat adding its own tag). Decoupling these into separate PRs would require one change being merged before the other.

I saw a few bugs not related to magic bounce, but that will be triggered with this magic bounce, that I was tempted to fix, but opted not to in order to keep the PR self contained. One of these is the erroneous behavior for lock on and kin.

@SirzBenjie SirzBenjie changed the title Add unit tests for magic bounce [Ability] [Move] Implement Magic Bounce and Magic Coat Jan 31, 2025
@Madmadness65 Madmadness65 added Move Affects a move Ability Affects an ability labels Jan 31, 2025
@SirzBenjie SirzBenjie marked this pull request as ready for review February 2, 2025 22:45
@SirzBenjie SirzBenjie requested a review from a team as a code owner February 2, 2025 22:45
This was based on smogon's incorrect handling of this situation
Wlowscha
Wlowscha previously approved these changes Feb 4, 2025
@SirzBenjie
Copy link
Contributor Author

Needs to be re-reviewed with new fix for:
https://bsky.app/profile/f-buffalo.bsky.social/post/3lhd6oo55zc2a

@SirzBenjie SirzBenjie requested a review from Wlowscha February 5, 2025 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ability Affects an ability Move Affects a move
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants