-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
SirzBenjie
wants to merge
22
commits into
pagefaultgames:beta
Choose a base branch
from
SirzBenjie:magic-bounce
base: beta
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,005
−130
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SirzBenjie
changed the title
Add unit tests for magic bounce
[Ability] [Move] Implement Magic Bounce and Magic Coat
Jan 31, 2025
SirzBenjie
force-pushed
the
magic-bounce
branch
from
February 2, 2025 18:06
3f0aa8a
to
666f8d3
Compare
SirzBenjie
commented
Feb 2, 2025
This was based on smogon's incorrect handling of this situation
SirzBenjie
force-pushed
the
magic-bounce
branch
from
February 2, 2025 22:47
6e1230f
to
726179a
Compare
Wlowscha
previously approved these changes
Feb 4, 2025
Needs to be re-reviewed with new fix for: |
DayKev
reviewed
Feb 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 nextMove End
phase, in order to insert new move phases initiated by magic bounce. ThisqueudPhases
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 thereflected
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 thereflected
field.src/battle-scene.ts
Refactored
unshiftPhase
to mirror javascript'sarray.unshift
method, allowing it to take a variable list of phases to unshift.Refactored
prependToPhase
andappendToPhase
to also allow taking aPhase []
instead of justPhase
.Necessary in order to insert the move-phases initiated from magic bounce after the
move-end
phasesrc/data/ability.ts
Applied the new
ReflectStatusMoveAbAttr
and removed theunimplemented
marker onMagic Bounce
. Added anedgeCase
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
MagicCoatTag
class for the Magic Coat move that lasts until the end of the turn and initiates thebattlerTags:magicCoatOnAdd
message when added.getBattlerTag
to construct and return this when getting theMagic_Coat
tag.src/data/move.ts
Removed
unimplemented
fromMagic Coat
, and have it add the newMAGIC_COAT
battler tag. Gave it thefailIfLastCondition
condition. Marked it asedgeCase
for the same reasons as the Magic Bounce ability.Added a new move flag,
REFLECTABLE
and thereflectable()
method which adds it when called.Modified
getTagTargetBenefitScore
to mark theMagic_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:
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 aspartial
.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
Fly / spikes
Checklist
beta
as my base branchnpm run test
)npm run create-test
) or updated existing tests related to the PR's changes?[ ] 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.