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

Implement Customizable NPC Trainer Parties #2733

Merged
merged 1 commit into from
Mar 26, 2023

Conversation

SBird1337
Copy link
Collaborator

@SBird1337 SBird1337 commented Feb 23, 2023

Description

Implements fully customizable trainer parties for enemy trainers according to the following structure:

struct TrainerMonCustomized
{
    const u8 *nickname;
    const u8 *ev;
    u32 iv;
    u16 moves[4];
    u16 species;
    u16 heldItem;
    u16 ability;
    u8 lvl;
    u8 ball;
    u8 friendship;
    u8 nature : 5;
    bool8 gender : 2;
    bool8 isShiny : 1;
};

Discord contact info

Karathan#1337

src/battle_main.c Outdated Show resolved Hide resolved
src/battle_main.c Show resolved Hide resolved
@SBird1337 SBird1337 force-pushed the feature/trainer-control branch 2 times, most recently from 0ff7930 to 5fc5659 Compare March 16, 2023 09:48
@SBird1337
Copy link
Collaborator Author

added a COMPOUND_STRING macro to define strings as compound literals:

.nickname = COMPOUND_STRING("asdf")

@SBird1337 SBird1337 marked this pull request as ready for review March 18, 2023 11:41
@SBird1337
Copy link
Collaborator Author

rebased to upcoming, squashed and addressed some changes. Could still need some tests.

@Jaizu
Copy link

Jaizu commented Mar 18, 2023

Stats need to be recalculated when using evs/ivs, needs a CalculateMonStats(&party[i]);

@Jaizu
Copy link

Jaizu commented Mar 18, 2023

The macro for mon evs doesn't compile, please change it to #define TRAINER_MON_EVS(hp, atk, def, speed, spatk, spdef) ((const u8[6]){hp,atk,def,spatk,spdef,speed})

@SBird1337
Copy link
Collaborator Author

addressed the reviews and added a TRAINER_PARTY_NATURE(nature) macro to account for natures being off-by-one.

@SBird1337 SBird1337 force-pushed the feature/trainer-control branch 2 times, most recently from 8f106be to ed76749 Compare March 18, 2023 15:35
@SBird1337
Copy link
Collaborator Author

added some tests, fixed some issues found by the tests.

Ready for review :)

fix nature related bug, fix hash generation, add tests
Copy link
Collaborator

@ghoulslash ghoulslash left a comment

Choose a reason for hiding this comment

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

I think this is a perfect opportunity to remove the TrainerMonPtr union, since its completely unnecessary and I've seen many posts expressing confusion about trying to assign items, etc, without the proper pointer definition or party flag. It would be much simpler from a user and developer to have just struct TrainerMonCustomized *party, and in CreateNPCTrainerParty, only assign fields if the field value is non-zero rather than checking e.g. F_TRAINER_PARTY_HELD_ITEM

@SBird1337
Copy link
Collaborator Author

I think this is a perfect opportunity to remove the TrainerMonPtr union, since its completely unnecessary and I've seen many posts expressing confusion about trying to assign items, etc, without the proper pointer definition or party flag. It would be much simpler from a user and developer to have just struct TrainerMonCustomized *party, and in CreateNPCTrainerParty, only assign fields if the field value is non-zero rather than checking e.g. F_TRAINER_PARTY_HELD_ITEM

I kinda agree, the reason I left it in is because the new structure is considerably larger than the old ones. So it would increase the ROM size. I have not made accurate calculations, but its not neglectable

Copy link
Collaborator

@ghoulslash ghoulslash left a comment

Choose a reason for hiding this comment

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

As per discord discussion, trainer party union will be removed at a later date

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