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

Fixes Dancer, adds Revelation Dance interactions with Z-Move, Roost and typeless mons #5133

Merged
merged 13 commits into from
Aug 11, 2024

Conversation

PhallenTree
Copy link

@PhallenTree PhallenTree commented Aug 10, 2024

Adds Tests for Revelation Dance.

The following Revelation Dance interactions were added:

  • If a Pokémon is Typeless, Revelation Dance will have no type (in which case Revelation Dance will not receive STAB). If a Pokémon has lost its primary Flying type due to having used Roost earlier this turn and uses Revelation Dance due to Dancer, Revelation Dance's type will match its current type: its secondary type if it has any, or else be Normal-type (regardless of type additions by Forest's Curse or Trick-or-Treat).
  • Typeless Revelation Dance does not activate Color Change and is blocked by Wonder Guard. Conversion 2 will fail if the last move used by the target was typeless Revelation Dance.
  • Regardless of the user's types, Revelation Dance is always treated as Normal-type for Z-Move mechanics, and can be upgraded to Breakneck Blitz by a Normalium Z.

Also fixes Dancer-called moves not updating their type according to the new user of the move.

Feature(s) this PR does NOT handle:

When merging to upcoming, will be necessary to modify the checks for CheckDynamicMoveType.

Discord contact info

PhallenTree

@AlexOn1ine
Copy link
Collaborator

The CI fails.

@PhallenTree
Copy link
Author

PhallenTree commented Aug 11, 2024

tests

I've run out of ideas on how to fix this. Fails on CI, passes on manual build

@AsparagusEduardo
Copy link
Collaborator

AsparagusEduardo commented Aug 11, 2024

I've run out of ideas on how to fix this. Fails on CI, passes on manual build

Are you able to make it fail locally by running at 4 threads? make -j4 check?

@PhallenTree
Copy link
Author

PhallenTree commented Aug 11, 2024

Are you able to make it fail locally by running at 4 threads? make -j4 check?

Yes, while running locally at 4 threads the test fails

@AsparagusEduardo
Copy link
Collaborator

Yes, while running locally at 4 threads the test fails

What does the replay looks like? (you can open the elf file on mGBA)

@AsparagusEduardo
Copy link
Collaborator

AsparagusEduardo commented Aug 11, 2024

Yes, while running locally at 4 threads the test fails

What does the replay looks like? (you can open the elf file on mGBA)

Wait, that's too many tests. Let's see if it fails just with make -j4 check TESTS="Revelation Dance".
EDIT: should be make -j4 check TESTS="Dancer"

@AsparagusEduardo
Copy link
Collaborator

Ok, finally made it fail locally with just Dancer tests: make -j3 check TESTS="Dancer"

@PhallenTree
Copy link
Author

PhallenTree commented Aug 11, 2024

The Dancer test that fails seems to only fail if handled by thread 0. Maybe a problem with the test runner?

I tried moving the Dancer test to above the previous one and it passes after doing so (with 4 threads).
Should I just do that and add a comment to let others know that test fails if executed on thread 0?

@AsparagusEduardo
Copy link
Collaborator

AsparagusEduardo commented Aug 11, 2024

In some cases, it seems that the battler sent to AbilityBattleEffects is changed. I'm not sure why it's happening.
image

But @AlexOn1ine identified that MOVEEND_DANCER's loop should use gBattlersCount instead of MAX_BATTLERS_COUNT, which fixes the issue. Can you apply this change?

-                   for (battler = 0; battler < MAX_BATTLERS_COUNT; battler++)
+                   for (battler = 0; battler < gBattlersCount; battler++)
                    {
                        if (GetBattlerAbility(battler) == ABILITY_DANCER && !gSpecialStatuses[battler].dancerUsedMove)
                        {
                            if (turnOnHitmarker)
                                gHitMarker |= HITMARKER_ATTACKSTRING_PRINTED;
                            if (!nextDancer || (gBattleMons[battler].speed < gBattleMons[nextDancer & 0x3].speed))
                                nextDancer = battler | 0x4;
                        }
                    }
                    if (nextDancer && AbilityBattleEffects(ABILITYEFFECT_MOVE_END_OTHER, nextDancer & 0x3, 0, 0, 0))
                        effect = TRUE;

@PhallenTree
Copy link
Author

PhallenTree commented Aug 11, 2024

It seems to me that it still had the speeds of the battlers from the previous test ("Dancer triggers from slowest to fastest") hence it using battler = 3 (which is the slowest in that test).

But now with the fix, it should pick the right battler.

@AsparagusEduardo AsparagusEduardo merged commit 779cedd into rh-hideout:master Aug 11, 2024
1 check passed
@hedara90 hedara90 added category: battle-mechanic Pertains to battle mechanics bugfix Bugfixes labels Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bugfixes category: battle-mechanic Pertains to battle mechanics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants