Another Gen 9 move batch #3450

kittenchilly · 2023-10-22T20:10:30Z

Description

EFFECT_SPICY_EXTRACT
EFFECT_DOODLE
EFFECT_FILLET_AWAY
EFFECT_SHED_TAIL
EFFECT_TIDY_UP

Issue(s) that this PR fixes

Closes #2491
Closes #2498
Closes #2499
Closes #2504
Closes #2506

Discord contact info

kittenchilly

…_FILLET_AWAY, MOVE_RAGING_BULL, MOVE_SHED_TAIL, MOVE_TIDY_UP

mrgriffin

A good start!

Just a note, it might be worth configuring your text editor to not trim trailing whitespace from lines because it makes the diffs a little more annoying to read.

src/battle_script_commands.c

mrgriffin · 2023-10-23T10:04:28Z

asm/macros/battle_script.inc

+	.macro tidyupclear battler:req, clear:req, failInstr:req
+	various \battler, VARIOUS_TIDY_UP
+	.byte \clear
+	.4byte \failInstr
+	.endm


I think it would make sense to separate this into two commands (idk what the names should be, but obviously they need to be different):

.macro tidyupclear battler:req, failInstr:req various \battler, VARIOUS_TIDY_UP .byte FALSE .4byte \failInstr .endm .macro tidyupclear battler:req various \battler, VARIOUS_TIDY_UP .byte TRUE .4byte NULL .endm

Because failInstr isn't actually required in the clear=TRUE case.

But also we never actually use the clear=FALSE case in the code as-written. Why does it exist?

I copied it from defogclear, which did have that. Forgot to remove it, but it might be useful...

Ahhh I think I see what clear does, if TRUE then it actually does the clearing, and if FALSE then it checks if there's anything that can be cleared and if there isn't it branches to failInstr?

I think it'd be fine to keep them both, and I suppose the names could be tidyupclear and cantidyupclear or something like that?

mrgriffin · 2023-10-23T10:07:12Z

src/battle_script_commands.c

@@ -9640,6 +9669,25 @@ static void Cmd_various(void)
        }
        return;
    }
+    case VARIOUS_TIDY_UP:


I think I might have seen somebody comment about this on #expansion-dev, but we've moved away from defining new variouses and now define callnatives because variouses are harder to read, and can be a merge conflict magnet for downstream users.

src/battle_util.c

src/data/battle_moves.h

mrgriffin · 2023-10-23T10:18:57Z

test/battle/move_effect/doodle.c

+    ASSUME(gBattleMoves[MOVE_DOODLE].effect == EFFECT_DOODLE);
+}
+
+DOUBLE_BATTLE_TEST("Doodle gives the target's ability to user and ally")


Can you write a/some tests that show Doodle failing if any three of the Pokémon involved have a forbidden ability? PARAMETRIZE will be useful for this.

Also can we get a SINGLE_BATTLE_TEST for whatever the expected behavior of Doodle in singles is.

Doodle in singles is exactly the same as Role Play.

Cool! So in that case I'd copy Role Play's applicable single test(s) and change them to use Doodle.

Or alternatively I suppose you could PARAMETRIZE those applicable tests over both MOVE_ROLE_PLAY and MOVE_DOODLE.

mrgriffin · 2023-10-23T10:20:24Z

test/battle/move_effect/fillet_away.c

+    ASSUME(gBattleMoves[MOVE_FILLET_AWAY].effect == EFFECT_FILLET_AWAY);
+}
+
+SINGLE_BATTLE_TEST("Fillet Away cuts the user's HP in half")


This should test that the stats have been boosted.

Can you test that this move fails if the HP is too low, or all the stats are at +6 already, and that it succeeds if only some but not all are at +6.

mrgriffin · 2023-10-23T10:21:39Z

test/battle/move_effect/shed_tail.c

+TO_DO_BATTLE_TEST("Shed Tail sets up substitute");
+TO_DO_BATTLE_TEST("Shed Tail transfers substitute to switch in");


Can you write these tests?

New effects should always have their full tests.

mrgriffin · 2023-10-23T10:27:41Z

test/battle/move_effect/tidy_up.c

+TO_DO_BATTLE_TEST("Tidy Up raises attack and speed by 1 and removes Substitute")
+TO_DO_BATTLE_TEST("Tidy Up raises attack and speed by 1 and removes Stealth Rock")
+TO_DO_BATTLE_TEST("Tidy Up raises attack and speed by 1 and removes Toxic Spikes")
+TO_DO_BATTLE_TEST("Tidy Up raises attack and speed by 1 and removes Sticky Web")


Can you write these tests?

AsparagusEduardo · 2023-10-23T10:31:36Z

Just a note, it might be worth configuring your text editor to not trim trailing whitespace from lines because it makes the diffs a little more annoying to read.

GitHub has an option to hide whitespace differences to make it easier on that front

asm/macros/battle_script.inc

src/battle_script_commands.c

data/battle_scripts_1.s

Skeli789 · 2023-10-23T11:41:26Z

data/battle_scripts_1.s

+	jumpifbattletype BATTLE_TYPE_ARENA, BattleScript_ButItFailed
+	jumpifcantswitch SWITCH_IGNORE_ESCAPE_PREVENTION | BS_ATTACKER, BattleScript_ButItFailed
+	setsubstitute
+	jumpifbyte CMP_EQUAL, cMULTISTRING_CHOOSER, B_MSG_SUBSTITUTE_FAILED, BattleScript_ButItFailed


Should this not jump to BattleScript_NotEnoughHPForSubstitute?

That doesn't exist. The way the substitute message works is different.

So then you should jump to the part of the substitute battle script where it prints the failure string. That way it'll print the correct string using the multistring chooser.

Skeli789 · 2023-10-23T11:42:18Z

data/battle_scripts_1.s

+	healthbarupdate BS_ATTACKER
+	datahpupdate BS_ATTACKER
+	printstring STRINGID_SHEDITSTAIL
+	waitmessage B_WAIT_TIME_LONG


At this point you need to call the moveend effects so things like HP restoring berries activate before the switchout.

Does this happen? Do moveend effects happen before the baton pass or u-turn switch out?

Each switchout move is its own case. U-Turn and Volt Switch for example can activate contact Abilities, while Baton Pass doesn't need to at all. In the case of Shed Tail, the user's HP is restored via a held Berry before switching out.

Skeli789 · 2023-10-23T11:42:49Z

data/battle_scripts_1.s

+	datahpupdate BS_ATTACKER
+	printstring STRINGID_SHEDITSTAIL
+	waitmessage B_WAIT_TIME_LONG
+	openpartyscreen BS_ATTACKER, BattleScript_ButItFailed


You should jump to a preexisting battle script for pivoting out. I'm sure plenty exist to choose from given the number of different pivot moves.

There isn't one. They all individually do the code for a switch out.

You should consolidate them then because there's a lot of reusable code from the different pivoting moves.

data/battle_scripts_1.s

Skeli789 · 2023-10-23T11:44:21Z

data/battle_scripts_1.s

+	jumpifbyte CMP_EQUAL, cMULTISTRING_CHOOSER, B_MSG_STAT_WONT_INCREASE, BattleScript_TidyUpTryHazards
+	printfromtable gStatUpStringIds
+	waitmessage B_WAIT_TIME_LONG
+BattleScript_TidyUpTryHazards:


The hazards are supposed to be removed before the stat changes. Once the order is fixed, you can also just jump to the Dragon Dance battle script to avoid duplicating code.

Where did you get this from? Defog raises evasiveness before clearing hazards.

https://youtu.be/n0_8i17k1RE?si=Cclst_BjTd9869pB&t=410
It's a dangerous trap to equate two similar yet unrelated effects. You can't rely on GameFreak to do things consistently.

data/battle_scripts_1.s

Skeli789 · 2023-10-23T11:46:18Z

data/battle_scripts_1.s

+	playstatchangeanimation BS_TARGET, BIT_ATK, STAT_CHANGE_BY_TWO
+	setstatchanger STAT_ATK, 2, FALSE
+	statbuffchange STAT_CHANGE_ALLOW_PTR | STAT_CHANGE_NOT_PROTECT_AFFECTED, BattleScript_SpicyExtractDefDown
+	jumpifbyte CMP_EQUAL, cMULTISTRING_CHOOSER, B_MSG_STAT_WONT_INCREASE, BattleScript_SpicyExtractDefDown


This should be when greater than. The message that the stat can't be raised should print if necessary.

I don't see any other stat change that uses greater than for this.
Also it does print the message that the stat can't be raised? The printfromtable command right below does it.

But you're skipping it. How can the printfromtable command be reached if you're jumping to BattleScript_SpicyExtractDefDown when the message is B_MSG_STAT_WONT_INCREASE?

Skeli789 · 2023-10-23T11:46:44Z

data/battle_scripts_1.s

+	playstatchangeanimation BS_TARGET, BIT_DEF, STAT_CHANGE_NEGATIVE | STAT_CHANGE_BY_TWO
+	setstatchanger STAT_DEF, 2, TRUE
+	statbuffchange STAT_CHANGE_ALLOW_PTR | STAT_CHANGE_NOT_PROTECT_AFFECTED, BattleScript_EffectSpicyExtract_End
+	jumpifbyte CMP_EQUAL, cMULTISTRING_CHOOSER, B_MSG_STAT_WONT_DECREASE, BattleScript_EffectSpicyExtract_End


Same here as the above comment.

Co-authored-by: Martin Griffin <martinrgriffin@gmail.com>

…omization

AsparagusEduardo · 2023-11-03T17:39:05Z

Dude, stop pushing commits without at testing them.
If we switch to a self-hosted runner, this will choke other other actions.

kittenchilly · 2023-11-27T21:39:22Z

Decided I am going to split this into separate PRs. Will be easier to debug and review that way.

kittenchilly added 6 commits October 20, 2023 23:39

MOVE_TERA_BLAST, MOVE_ORDER_UP, MOVE_SPICY_EXTRACT, MOVE_DOODLE, MOVE…

2e3a572

…_FILLET_AWAY, MOVE_RAGING_BULL, MOVE_SHED_TAIL, MOVE_TIDY_UP

Update battle_moves.h

f6cb68f

Merge remote-tracking branch 'upstream/upcoming' into gen9moves

1e4d70d

Merge remote-tracking branch 'upstream/upcoming' into gen9moves

be8c381

Tests

15333c0

Update battle_moves.h

98c3b30

mrgriffin requested changes Oct 23, 2023

View reviewed changes

mrgriffin reviewed Oct 23, 2023

View reviewed changes

asm/macros/battle_script.inc Outdated Show resolved Hide resolved

mrgriffin reviewed Oct 23, 2023

View reviewed changes

src/battle_script_commands.c Outdated Show resolved Hide resolved

Skeli789 suggested changes Oct 23, 2023

View reviewed changes

kittenchilly and others added 13 commits October 23, 2023 11:30

Merge branch 'upcoming' into gen9moves

cd27ee4

Apply suggestions from code review

79d1b11

Co-authored-by: Martin Griffin <martinrgriffin@gmail.com>

Update banned abilities for ability changing moves

1ba584f

Split Entrainment and Simple Beam banned abilities list for more cust…

500f27a

…omization

IsRolePlayDoodleBannedAbility

47ba3b4

Update battle_moves.h

6c20974

Merge branch 'upcoming' into gen9moves

7cf5635

Update battle_util.h

2bb02dd

Update battle_moves.h

2bbc4eb

Address a few reviews

a9652eb

Update battle_script_commands.c

be4e58c

Update battle_script_commands.c

e1c084d

Merge branch 'upcoming' into gen9moves

31dbcb6

kittenchilly closed this Nov 27, 2023

AlexOn1ine mentioned this pull request Jan 17, 2024

Adds Move Shed Tail #4016

Merged

AlexOn1ine mentioned this pull request Feb 4, 2024

Adds Tidy Up + minor Dragon Cheer follow up #4136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another Gen 9 move batch #3450

Another Gen 9 move batch #3450

kittenchilly commented Oct 22, 2023

mrgriffin left a comment

mrgriffin Oct 23, 2023

kittenchilly Oct 23, 2023

mrgriffin Oct 23, 2023

mrgriffin Oct 23, 2023

mrgriffin Oct 23, 2023

kittenchilly Oct 30, 2023

mrgriffin Oct 30, 2023

mrgriffin Oct 23, 2023

mrgriffin Oct 23, 2023

AsparagusEduardo Nov 3, 2023

mrgriffin Oct 23, 2023

AsparagusEduardo commented Oct 23, 2023

Skeli789 Oct 23, 2023

kittenchilly Oct 30, 2023

Skeli789 Oct 30, 2023

Skeli789 Oct 23, 2023

kittenchilly Oct 30, 2023

Skeli789 Oct 30, 2023

Skeli789 Oct 23, 2023

kittenchilly Oct 30, 2023

Skeli789 Oct 30, 2023

Skeli789 Oct 23, 2023

kittenchilly Oct 30, 2023

Skeli789 Oct 30, 2023

Skeli789 Oct 23, 2023

kittenchilly Oct 30, 2023

Skeli789 Oct 30, 2023

Skeli789 Oct 23, 2023

AsparagusEduardo commented Nov 3, 2023 •

edited

Loading

kittenchilly commented Nov 27, 2023

		TO_DO_BATTLE_TEST("Shed Tail sets up substitute");
		TO_DO_BATTLE_TEST("Shed Tail transfers substitute to switch in");

Another Gen 9 move batch #3450

Another Gen 9 move batch #3450

Conversation

kittenchilly commented Oct 22, 2023

Description

Issue(s) that this PR fixes

Discord contact info

mrgriffin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsparagusEduardo commented Oct 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsparagusEduardo commented Nov 3, 2023 • edited Loading

kittenchilly commented Nov 27, 2023

AsparagusEduardo commented Nov 3, 2023 •

edited

Loading