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

DoBattleIntro state documentation #5231

Conversation

AsparagusEduardo
Copy link
Collaborator

Description

Slight documentation to make it easier to modify intro states.
Also removed unused state.

Issue(s) that this PR fixes

Fixes #4501

People who collaborated with me in this PR

@ShinyDragonHunter

Discord contact info

AsparagusEduardo

src/battle_main.c Outdated Show resolved Hide resolved
Co-authored-by: Alex <93446519+AlexOn1ine@users.noreply.github.com>
@AsparagusEduardo
Copy link
Collaborator Author

Applied change

@AlexOn1ine
Copy link
Collaborator

correct me if I'm wrong but I think the variable declared on line 3452 can be changed to enum BattleIntroStates instead of u8

@AsparagusEduardo
Copy link
Collaborator Author

correct me if I'm wrong but I think the variable declared on line 3452 can be changed to enum BattleIntroStates instead of u8

For that, it seems that I need to change gBattleStruct->introState's type to enum BattleIntroStates too, and for that, move the enum to be next to BattleStruct's definition.
Should we start doing this for enums used in structs?

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 20, 2024

correct me if I'm wrong but I think the variable declared on line 3452 can be changed to enum BattleIntroStates instead of u8

For that, it seems that I need to change gBattleStruct->introState's type to enum BattleIntroStates too, and for that, move the enum to be next to BattleStruct's definition. Should we start doing this for enums used in structs?

When you make the change in the Battle Struct could you check the space usage in the struct before and after? Also you could try to change the enum to a u8 and see how that goes.

@AsparagusEduardo
Copy link
Collaborator Author

When you make the change in the Battle Struct could you check the space usage in the struct?

If you mean EWRAM usage, no difference:

Memory region         Used Size  Region Size  %age Used
           EWRAM:      244554 B       256 KB     93.29%
           IWRAM:       30436 B        32 KB     92.88%
             ROM:    25696068 B        32 MB     76.58%

Weirdly enough, there's a 20-byte increase in ROM when setting the last enum to 300 or 70000.

    STATE_PRINT_PLAYER_2_SEND_OUT_TEXT,
-   STATE_SET_DEX_AND_BATTLE_VARS,
+   STATE_SET_DEX_AND_BATTLE_VARS = 300,
Memory region         Used Size  Region Size  %age Used
           EWRAM:      244554 B       256 KB     93.29%
           IWRAM:       30436 B        32 KB     92.88%
-            ROM:    25696068 B        32 MB     76.58%
+            ROM:    25696088 B        32 MB     76.58%

@AsparagusEduardo
Copy link
Collaborator Author

(I'll have to wait until I get home to check the actual struct size)

@AlexOn1ine
Copy link
Collaborator

AlexOn1ine commented Aug 20, 2024

When you make the change in the Battle Struct could you check the space usage in the struct?

If you mean EWRAM usage, no difference:

no I meant sizoof(struct BattleStruct) but rom space is something tertu brought up before. It looks like changing the type of enum increases the rom size. See: #5106

@ShinyDragonHunter
Copy link

ShinyDragonHunter commented Aug 20, 2024

A few things were missed btw. I'm not sure how to quote particular lines but...

        if (!gBattleControllerExecFlags)
        {
            if (++gBattleCommunication[1] == gBattlersCount)
                (*state)++;
            else
                *state = 0;
        }
        if (B_FAST_INTRO == TRUE
          && !(gBattleTypeFlags & (BATTLE_TYPE_RECORDED | BATTLE_TYPE_RECORDED_LINK | BATTLE_TYPE_RECORDED_IS_MASTER | BATTLE_TYPE_LINK)))
            *state = 15; // Print at the same time as trainer sends out second mon.
        else
            (*state)++;
        break;

@AsparagusEduardo
Copy link
Collaborator Author

no I meant sizoof(struct BattleStruct) but rom space is something tertu brought up before. It looks like changing the type of enum increases the rom size. See: #5106

Finally was able to check. The change increased the size of the struct from 1144b to 1152b

@AlexOn1ine
Copy link
Collaborator

no I meant sizoof(struct BattleStruct) but rom space is something tertu brought up before. It looks like changing the type of enum increases the rom size. See: #5106

Finally was able to check. The change increased the size of the struct from 1144b to 1152b

hm, then since both changes would lead to either increased rom size or increased BattleStruct size I'm leaning towards it being not important enough to justify the change.

@AsparagusEduardo
Copy link
Collaborator Author

hm, then since both changes would lead to either increased rom size or increased BattleStruct size I'm leaning towards it being not important enough to justify the change.

You're talking about using an enum for gBattleStruct->introState, right? Or using an enum in general?

@AlexOn1ine
Copy link
Collaborator

hm, then since both changes would lead to either increased rom size or increased BattleStruct size I'm leaning towards it being not important enough to justify the change.

You're talking about using an enum for gBattleStruct->introState, right?

yes

AlexOn1ine
AlexOn1ine previously approved these changes Aug 21, 2024
@AlexOn1ine
Copy link
Collaborator

no I meant sizoof(struct BattleStruct) but rom space is something tertu brought up before. It looks like changing the type of enum increases the rom size. See: #5106

Finally was able to check. The change increased the size of the struct from 1144b to 1152b

could you try limit it to 8 bits. will this prevent to increase the size? so enum BattleInfoStates introState:8?

@AsparagusEduardo
Copy link
Collaborator Author

could you try limit it to 8 bits. will this prevent to increase the size? so enum BattleInfoStates introState:8?

Doesn't compile ^^;
image

@AsparagusEduardo
Copy link
Collaborator Author

I was able to remove the pointer reference alongside the bit limit change and it was kept as 1144b! :D

Since the enum will be in include/battle.h, should I rename the prefix from STATE_ to BATTLE_INTRO_STATE_?

@AlexOn1ine
Copy link
Collaborator

I was able to remove the pointer reference alongside the bit limit change and it was kept as 1144b! :D

Since the enum will be in include/battle.h, should I rename the prefix from STATE_ to BATTLE_INTRO_STATE_?

oh, that's nice.

I would rename it

@AsparagusEduardo
Copy link
Collaborator Author

oh, that's nice.

I would rename it

Done 👍

@AlexOn1ine AlexOn1ine merged commit 77148f8 into rh-hideout:upcoming Aug 21, 2024
1 check passed
@AsparagusEduardo AsparagusEduardo deleted the _RHH/pr/upcoming/battleIntroState branch August 21, 2024 14:08
@AlexOn1ine AlexOn1ine added the type: documentation Improvements or additions to documentation label Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants