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

Added new conditions for the trainer slide-in system #2713

Merged
merged 10 commits into from
Apr 24, 2023

Conversation

LOuroboros
Copy link
Collaborator

@LOuroboros LOuroboros commented Feb 21, 2023

Description

Addresses something brought up in #1655; missing conditionals that are present in the official games.

mGBA_20230302_061420748

The new conditions on which a trainer NPC is now allowed to say something are as follows:
-When their last Pokémon's HP is between 25% and 50%.
-When a critical hit is dealt to their first Pokémon for the first time.
-When a super effective hit is dealt to their first Pokémon for the first time.
-When a STAB move is used against (but doesn't faint) the opponent's Pokémon.
-When the Player's Pokémon's move doesn't affect the opponent's Pokémon.

Additionally, I added 2 conditions that are not present in the official games (as far as I remember anyway.)
-When they're about to trigger a Mega Evolution.
-When they're about to trigger a Z-Move.
These 2 are extrapolated from the fact that Dynamax/Gigantamax does have a slide-in message in the game where that gimmick is used.
If that one has slide-in messages, why should Megas or Z-Moves not have one too?

Discord contact info

Lunos#4026

Now they can say something:
-When their last Pokémon's HP is between 25% and 50%.
-When a critical hit is dealt to their first Pokémon for the first time.
-When a super effective hit is dealt to their first Pokémon for the first time.
-When a STAB move is used against (but doesn't faint) the opponent's Pokémon.
-When the Player's Pokémon's move doesn't affect the opponent's Pokémon.
-When they're about to trigger a Mega Evolution.
-When they're about to trigger a Z-Move.

Misc. changes:
-Split GetEnemyMonCount for readability's sake.
-Optimized the size allocated to trainerSlideLowHpMsgDone.
@Jaizu
Copy link

Jaizu commented Feb 26, 2023

Please fix conflicts :D

@LOuroboros
Copy link
Collaborator Author

LOuroboros commented Feb 27, 2023

Please fix conflicts :D

Done. I also fixed the order of the variouses. For whatever reason, VARIOUS_JUMP_IF_SHELL_TRAP wasn't added to Cmd_various in the same order as they're defined at include/constants/battle_script_commands.h.

Copy link
Collaborator

@DizzyEggg DizzyEggg left a comment

Choose a reason for hiding this comment

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

Please fix conflicts.

@LOuroboros
Copy link
Collaborator Author

Please fix conflicts.

Done. Needs testing due to the extra changes from #2699 that got in the way of the changes I made in this PR.

If no one tests things, I'll do it myself once I wake up later.

@Jaizu
Copy link

Jaizu commented Mar 2, 2023

Checking some Scarlet/Violet videos I saw that they have a trainer message just as they send their first Pokémon. Do you plan to add those too?
This video contains spoilers of the elite 4 and what not: https://www.youtube.com/watch?v=JzXGavc83zE

@LOuroboros
Copy link
Collaborator Author

Checking some Scarlet/Violet videos I saw that they have a trainer message just as they send their first Pokémon. Do you plan to add those too?

I'll look into it.

@LOuroboros
Copy link
Collaborator Author

Done.

@LOuroboros
Copy link
Collaborator Author

I recorded a .gif because why not, which I forgot to drop because I'm dumb.

mGBA_20230302_061420748

Now a trainer NPC can also say something before the first turn of a battle starts for good.
@Jaizu Jaizu requested a review from DizzyEggg March 21, 2023 09:28
include/battle.h Outdated
@@ -622,7 +622,7 @@ struct BattleStruct
struct MegaEvolutionData mega;
struct ZMoveData zmove;
const u8 *trainerSlideMsg;
bool8 trainerSlideLowHpMsgDone;
bool8 trainerSlideLowHpMsgDone:1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

u8 trainerSlideLowHpMsgDone:1;

it also still takes up a byte currently, unless its combined with other bit-field field definitions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why is that, and what do you suggest me to do? 👀

Copy link
Collaborator

Choose a reason for hiding this comment

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

because of struct alignment. you can't align the next field in the middle of a byte, so it will fill in the remaining 7 bits with padding. you can move the field to another bit-field area. If you don't care about saying the space then its fine as is. I just made this comment in the event you were trying to save the 7 bits

Copy link
Collaborator Author

@LOuroboros LOuroboros Apr 23, 2023

Choose a reason for hiding this comment

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

I was. In this situation, I rather remove the bit allocation and keep the variable a bool8 than to use the u8 data type for what's a bool variable honestly. I'll do that, if you don't mind.

include/battle.h Outdated
@@ -663,6 +663,14 @@ struct BattleStruct
u8 storedHealingWish:4; // Each battler as a bit.
u8 storedLunarDance:4; // Each battler as a bit.
u16 supremeOverlordModifier[MAX_BATTLERS_COUNT];
bool8 trainerSlideHalfHpMsgDone:1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

u8 trainerSlideHalfHpMsgDone:1;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why? It's only used as a bool.

Copy link
Collaborator

Choose a reason for hiding this comment

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

to be explicit that its sharing the byte with the fields below

Copy link
Collaborator Author

@LOuroboros LOuroboros Apr 23, 2023

Choose a reason for hiding this comment

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

Is it though? The 4 variables below conform a byte on their own, don't they?
Also, a few lines above there's hitSwitchTargetFailed, also a bool8 variable that is allocated a size of 1 bit.

include/battle.h Outdated
u8 trainerSlideFirstSuperEffectiveHitMsgState:2;
u8 trainerSlideFirstSTABMoveMsgState:2;
u8 trainerSlidePlayerMonUnaffectedMsgState:2;
bool8 trainerSlideMegaEvolutionMsgDone:1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

u8 instead of bool8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why though.

else
return FALSE;
}

u32 ShouldDoTrainerSlide(u32 battlerId, u32 which)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we clean up some of the code below by calling frequently-used functions e.g. GetEnemyMonCount(firstId, lastId, TRUE) , or initializing variables like currHp, maxHp for the hp threshold checks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GetEnemyMonCount is already called.

As for the thresholds, I suppose I could, yeah. I'll see what I can come up with.

Copy link
Collaborator Author

@LOuroboros LOuroboros Apr 23, 2023

Choose a reason for hiding this comment

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

Check up my ping in the Discord server later. I need your opinion on a function that I wrote in an attempt to make those maxHp checks more readable.

@@ -11176,6 +11178,39 @@ static void Cmd_various(void)
gBattlescriptCurrInstr = cmd->nextInstr;
return;
}
case VARIOUS_JUMP_IF_SHELL_TRAP:
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this included here?

Copy link
Collaborator Author

@LOuroboros LOuroboros Apr 23, 2023

Choose a reason for hiding this comment

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

Because I moved it from its original position.

It was written above VARIOUS_ACTIVATE_TERRAIN_CHANGE_ABILITIES, even though it's defined below VARIOUS_HIT_SWITCH_TARGET_FAILED, so I moved it below the case VARIOUS_HIT_SWITCH_TARGET_FAILED in Cmd_various to keep consistency.

-Undeclared size for certain trainer slide-in variables at struct BattleStruct for reasons.
-Added a BattlerHPPercentage function to make HP threshold checks in ShouldDoTrainerSlide a bit more readable.
@LOuroboros
Copy link
Collaborator Author

Ready for re-review 👀

@ghoulslash ghoulslash merged commit 19ec7e2 into rh-hideout:upcoming Apr 24, 2023
@LOuroboros LOuroboros deleted the trainerSlideMsgUpdate branch April 24, 2023 12:54
@AsparagusEduardo AsparagusEduardo mentioned this pull request May 31, 2023
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.

4 participants