Introduces Battler struct for the Battle Engine

[AlexOn1ine]

Description

Introduces a new struct called Battlers that lives in BattleStruct.

Since DisableStruct, SpecialStatuses and ProtectStruct are cleared at various points in the battle engine they can't always be used. Sometimes you want the flexibility to clear a flag during specific points. This is what usually the BattleStruct is used for. The downside here is that it is not convenient to use the struct when specific battlers were involved. The workaround used was a Bitfield but if you don't know the specific syntax was hard to read.

Introducing a Battler struct alleviates the problem and has the additional advantage of future proofing the engine for triple battles because each field that only used 4 bits would have to be changed. The Battler struct will automatically increase in size based on MAX_BATTLERS_COUNT.

The small downside here is that you want to set everything to 0 you have to loop over all battlers. This is usually not a problem because this is already done for various other field so the code can be reused. And generally wasn't much of a problem in this PR because it only applied to a few cases.

People who collaborated with me in this PR

@mrgriffin provided the framework for this change

Feature(s) this PR does NOT handle:

We can do the same for party members. This pr does not handle it to make the review easier

Things to note in the release changelog:

So, I don't think this should introduce any conflicts. The fields changed are controlling the flow of various game mechanics and using them anywhere else can cause bugs so a user should never touch them. If they did touch them then I expect them to know what they are doing.

If users used bitfields in the BattleStruct then nothing changes for them because this pr doesn't break them. If they want to migrate then this is how they would do it based on the following example.

Checking if the field is set:

gBattleStruct->commandingDondozo & (1u << battlerDef)) 
-> gBattleStruct->battlerState[battlerDef].commandingDondozo

Setting a field to true:

gBattleStruct->commandingDondozo |= 1u << battler 
-> gBattleStruct->battlerState[battler].commandingDondozo = TRUE

Setting a field to false:

gBattleStruct->commandingDondozo &= ~(1u << i) 
-> gBattleStruct->battlerState[i].commandingDondozo = FALSE;

[AsparagusEduardo]

Is there a reason why these can't be in gBattleMons?

[AlexOn1ine]

Is there a reason why these can't be in gBattleMons?

this isn't gBattleMon data

[AsparagusEduardo]

this isn't gBattleMon data

What's the difference? Battle-only data is stored in BattlePokemon, such as stat stages and types.

[AlexOn1ine]

this isn't gBattleMon data

What's the difference? Battle-only data is stored in BattlePokemon, such as stat stages and types.

BattlePokemon is used for attributes of a species. The stuff in this pr is used to control the flow of moves, items and abilities

With BattlePokemon you aren't controlling the flow but storing various data that can be accessed.

[AsparagusEduardo]

BattlePokemon is used for attributes of a species. The stuff in this pr is used to control the flow of moves, items and abilities

I guess my question is more in the line of "wouldn't it be better to have everything related to single battler in a single struct?"

[AlexOn1ine]

BattlePokemon is used for attributes of a species. The stuff in this pr is used to control the flow of moves, items and abilities

I guess my question is more in the line of "wouldn't it be better to have everything related to single battler in a single struct?"

No, because those are fully different things and shouldn't be mixed up with each other. You are potentially only introducing bugs because some BattleMon data was swapped somewhere.

e.g. switching. During switch ins that data is swapped with a party mon for the calculation.

1. This process is already slow. So by adding more data you would only make it worse
2. You are potentially swapping stuff around that has to be set during the calculations

[AsparagusEduardo]

No, because those are fully different things and shouldn't be mixed up with each other. You are potentially only introducing bugs because some BattleMon data was swapped somewhere

Can you give an example? I legit do not know what you mean in practical terms by "some BattleMon data is swapped somewhere".
I don't see how the fields in BattlePokemon are any different from the states from this new struct. Both are modified directly by the battle code flow. It's not like BattlePokemon is data that's only loaded once at the beginning of the battle and never touched.
Eg. ability,hp, status1, status2, species and item are all modified during battle.

[AlexOn1ine]

No, because those are fully different things and shouldn't be mixed up with each other. You are potentially only introducing bugs because some BattleMon data was swapped somewhere

Can you give an example? I legit do not know what you mean in practical terms by "some BattleMon data is swapped somewhere". I don't see how the fields in BattlePokemon are any different from the states from this new struct. Both are modified directly by the battle code flow. It's not like BattlePokemon is data that's only loaded once at the beginning of the battle and never touched. Eg. ability,hp, status1, status2, species and item are all modified during battle.

Here is an exmaple

I don't see how the fields in BattlePokemon are any different from the states from this new struct

Because it isn't Pokemon Data.

[AsparagusEduardo]

Here is an exmaple

Wouldn't it be more buggy if we detatch the battler information into different structs? I'm thinking that in this case, if AI_CalcDamage modifies data in this new struct, FreeRestoreBattleMons would not be able to restore it, as it wasn't part of BattlePokemon.

Because it isn't Pokemon Data.

Stat stages aren't Pokémon Data, they're also a state.

[ghoulslash]

We don't want to move to stuff to gBattleMons because it isnt allocated. I also recommend changing the struct name to BattlerFlags or something

[AlexOn1ine]

Made the suggested label changes

[AsparagusEduardo]

Suggested change

	u32 commandingDondozo:1;
	u32 absentBattlerFlags:1;
	u32 focusPunchBattlers:1;
	u32 multipleSwitchInBattlers:1;
	u32 alreadyStatusedMoveAttempt:1; // As bits for battlers; For example when using Thunder Wave on an already paralyzed Pokémon.
	u32 activeAbilityPopUps:1;
	u32 lastMoveFailed:1; // For Stomping Tantrum
	u32 forcedSwitch:1;
	u32 storedHealingWish:1;
	u32 storedLunarDance:1;
	u32 usedEjectItem:1;
	u32 sleepClauseEffectExempt:1; // Stores whether effect should be exempt from triggering Sleep Clause (Effect Spore)
	u32 usedMicleBerry:1;
	u32 pursuitTarget:1;
	bool32 commander:1;
	bool32 absent:1;
	bool32 focusPunch:1;
	bool32 multipleSwitchIn:1;
	bool32 alreadyStatusedMoveAttempt:1; // For example when using Thunder Wave on an already paralyzed Pokémon.
	bool32 activeAbilityPopUp:1;
	bool32 lastMoveFailed:1; // For Stomping Tantrum
	bool32 forcedSwitch:1;
	bool32 storedHealingWish:1;
	bool32 storedLunarDance:1;
	bool32 usedEjectItem:1;
	bool32 sleepClauseEffectExempt:1; // Stores whether effect should be exempt from triggering Sleep Clause (Effect Spore)
	bool32 usedMicleBerry:1;
	bool32 pursuitTarget:1;

[AlexOn1ine]

Can we use u32 for consistency reasons? Things sometimes need more then one bit and every struct uses u32's already, or most structs. That it is a true/false value is already indicated by the 1 bit usage.

[AsparagusEduardo]

bool32 is a synonym for u32, just used for documentation purposes. In this case, it's being used for flags, meaning they're being used as booleans. Because it's a synonym, you can mix and match them when needed without worrying about padding in the struct.

[AlexOn1ine]

I know they are synonyms to each to each other. This is why I'm asking to keep the u32 for constituency reasons.

Not everything will be using only one bit so so you end with a struct that might look like this:

bool32 field_1:1;
bool32 field_2:1;
u32 field_3:2;;
u32 field_4:3;
bool32 field_5:1;

it doesn't look good when you start mix and matching it,
That they are bools is already indicated by the usage of one bit. Also all important structs use u's instead of bools

[AsparagusEduardo]

it doesn't look good when you start mix and matching it, That they are bools is already indicated by the usage of one bit.

We shouldn't mislabel something for the sake of it looking good. Otherwise, all bool32 could be converted to u32 because it's less verbose.

Also all important structs use u's instead of bools

Those should be changed too, probably on pret's side even.

[Bassoonian]

If this is an issue even on pret's end then this isn't remotely close to blocking for this PR imo

[AlexOn1ine]

I personally don't think there is any mislabeling happening here because it is understandable that u32 field:1; uses one bit as a flag but for consistency reasons I would like to have Eggs / ghouls approval on this. So if they are fine with this change then I'm okay with it.

But if they agree to this change then it should happen in pret, and across the board.