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

5 Parental Bond tests #3606

Closed
wants to merge 1 commit into from
Closed

Conversation

Bassoonian
Copy link
Collaborator

Adds five of the many Parental Bond tests that still need to be written

Issue(s) that this PR fixes

Partially fixes #3430

Discord contact info

bassoonian

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.

Could you also check a normal double target move like Icy wind so both target conditions are tested?

GIVEN {
ASSUME(gBattleMoves[MOVE_TRIPLE_KICK].split != SPLIT_STATUS);
ASSUME(gBattleMoves[MOVE_TRIPLE_KICK].strikeCount == 3);
ASSUME(gBattleMoves[MOVE_TRIPLE_KICK].effect == EFFECT_TRIPLE_KICK);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the assume for the effect needed here it shouldn't effect the test in any way?

GIVEN {
ASSUME(gBattleMoves[MOVE_EARTHQUAKE].split != SPLIT_STATUS);
ASSUME(gBattleMoves[MOVE_EARTHQUAKE].strikeCount < 2);
ASSUME(gBattleMoves[MOVE_EARTHQUAKE].effect == EFFECT_EARTHQUAKE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again not sure if assuming Earthquake effect matters here but I think the assume for MOVE_TARGET_FOES_AND_ALLY should be added.

u16 move;
PARAMETRIZE { move = MOVE_EARTHQUAKE; }
PARAMETRIZE { move = MOVE_ROCK_SLIDE; }
GIVEN {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assume for MOVE_TARGET_FOES_AND_ALLY

GIVEN {
ASSUME(gBattleMoves[MOVE_EARTHQUAKE].split != SPLIT_STATUS);
ASSUME(gBattleMoves[MOVE_EARTHQUAKE].strikeCount < 2);
ASSUME(gBattleMoves[MOVE_EARTHQUAKE].effect == EFFECT_EARTHQUAKE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again maybe remove effect EQ and add MOVE_TARGET_FOES_AND_ALLY

@Bassoonian
Copy link
Collaborator Author

Could you also check a normal double target move like Icy wind so both target conditions are tested?

That's why Rock Slide is there :)

@AlexOn1ine
Copy link
Collaborator

Could you also check a normal double target move like Icy wind so both target conditions are tested?

That's why Rock Slide is there :)

Oh sorry, mb. Though I would also remove the flinch effect assume.

@Bassoonian
Copy link
Collaborator Author

Could you also check a normal double target move like Icy wind so both target conditions are tested?

That's why Rock Slide is there :)

Oh sorry, mb. Though I would also remove the flinch effect assume.

The flinch assume is necessary in this case, since it does in fact make the two opponents flinch (and the messages depend on that). I could alternatively move the flinch check there and make it check for regular Celebrate usage if the effect is not set to flinching, though

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Nov 27, 2023

Could you also check a normal double target move like Icy wind so both target conditions are tested?

That's why Rock Slide is there :)

Oh sorry, mb. Though I would also remove the flinch effect assume.

The flinch assume is necessary in this case, since it does in fact make the two opponents flinch (and the messages depend on that). I could alternatively move the flinch check there and make it check for regular Celebrate usage if the effect is not set to flinching, though

In this case I would check the HP bar a second time but negated instead of the messages and celebrate.
You could also use a double target move that doesn't flinch if you want to keep the celebrate check but I don't see why it would be necessary in this case.

@DizzyEggg
Copy link
Collaborator

What's the status of this? 👀

@AlexOn1ine
Copy link
Collaborator

superseded by #3973

@AlexOn1ine AlexOn1ine closed this Jan 11, 2024
@Bassoonian Bassoonian deleted the parentaltests branch April 21, 2024 13:39
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.

Write missing Parental Bond tests
3 participants