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

Used the COMPOUND_STRING macro to unify item descriptions and item data #3432

Merged
merged 8 commits into from
Dec 20, 2023

Conversation

LOuroboros
Copy link
Collaborator

@LOuroboros LOuroboros commented Oct 17, 2023

Description

About 3 hours and a half ago, I was indirectly reminded of #3243.
Somehow, that made me think that having the text strings that compose an item's descriptions in an entirely separate file is a pita when you decide to add entirely new items, TM/HM or not.
The very first way to deal with that I thought was to use the COMPOUND_STRING macro that SBird added in #2733 to write the strings directly in the .description field of each item.
That worked.
Then I gauged the public opinion in the Discord server, and most of the people seemed to be on board with the idea.
So I proceeded to ditch every single dedicated text string for item descriptions and submit a PR to the expansion.

This PR will remain a Draft until #3284 and #3274 are merged. Personal choice. I don't wanna trouble kittenchilly 😆
Any early reviews would be appreciated.

Discord contact info

lunos4026

Misc. Changes:
-Corrected the Serious Mint's description.
-Moved the X_ITEM_STAGES and CONFUSE_BERRY_HEAL_FRACTION macros to the top of the file for consistency's sake.
-Corrected the Tapunium Z's description.
mrgriffin
mrgriffin previously approved these changes Oct 23, 2023
Copy link
Collaborator

@mrgriffin mrgriffin left a comment

Choose a reason for hiding this comment

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

I love this change.

I think I like your formatting better than the other obvious alternative:

        .description = COMPOUND_STRING(
            "A tool used for\n"
            "catching wild\n"
            "Pokémon."
        ),

But I suppose reasonable people could disagree about whether it's better to have 5 lines with little indentation, or 3 lines with a lot of indentation. I'll let people fight it out in the comments to this PR 🤣

DizzyEggg
DizzyEggg previously approved these changes Oct 31, 2023
@DizzyEggg
Copy link
Collaborator

This is a great PR Lunos. Formatting is better too. Is there something missing since you set it as a draft?

@LOuroboros
Copy link
Collaborator Author

This is a great PR Lunos. Formatting is better too. Is there something missing since you set it as a draft?

This PR will remain a Draft until #3284 and #3274 are merged. Personal choice. I don't wanna trouble kittenchilly 😆

@Bassoonian
Copy link
Collaborator

Both PRs you were waiting for have been merged in 👀

@LOuroboros
Copy link
Collaborator Author

Both PRs you were waiting for have been merged in 👀

Hell yeah!

Gonna pull and solve the merge conflicts then.

@LOuroboros LOuroboros dismissed stale reviews from DizzyEggg and mrgriffin via 5e41b02 November 24, 2023 06:39
@LOuroboros LOuroboros marked this pull request as ready for review November 24, 2023 06:42
@LOuroboros
Copy link
Collaborator Author

LOuroboros commented Nov 24, 2023

Alright, after some crazy developments on my end and Git moving me to a non-branch for reasons that I don't understand, this PR is ready for review.

I pulled upcoming, solved the conflicts and moved the definitions of CONFUSE_BERRY_HEAL_FRACTION back to src/data/items.h.

The definitions of CONFUSE_BERRY_HP_FRACTION which was introduced in b93dfb9 are kept in include/constants/items.h because the file in which it's used, src/battle_ai_switch_items.c, can't read preproc constants defined inside src/data/items.h.

@LOuroboros
Copy link
Collaborator Author

And now I had to move CONFUSE_BERRY_HEAL_FRACTION back for the same reasons.

I really don't like to have item data related constants split across 2 files like that, but I cba to think of a good solution to address that so I'll leave that to whoever else may care enough. It's not in-scope for this PR regardless.

@AsparagusEduardo AsparagusEduardo added this to the 1.8.0 milestone Nov 24, 2023
@Bassoonian
Copy link
Collaborator

Like what we do for moves and Pokédex entries that share a description, I think we should have some static declarations at the top of the file for shared descriptions (I'm thinking e.g. Poké Toy and Fluffy Tail) to avoid duplicate strings (that modern doesn't consolidate, as we have found out).

@LOuroboros
Copy link
Collaborator Author

Like what we do for moves and Pokédex entries that share a description, I think we should have some static declarations at the top of the file for shared descriptions (I'm thinking e.g. Poké Toy and Fluffy Tail) to avoid duplicate strings (that modern doesn't consolidate, as we have found out).

I don't like the inconsistency that creates at the time of editing the strings, and it's not like item description strings aren't duplicated even in vanilla, but I'll look into it.

@LOuroboros
Copy link
Collaborator Author

Ready for re-review.

@Bassoonian Bassoonian merged commit 5718d99 into rh-hideout:upcoming Dec 20, 2023
1 check passed
@LOuroboros LOuroboros deleted the itemDescriptions branch December 20, 2023 18:02
Pawkkie pushed a commit to Pawkkie/pokeemerald-expansion that referenced this pull request May 15, 2024
…ta (rh-hideout#3432)

* Used the COMPOUND_STRING macro to unify item descriptions and item data

Misc. Changes:
-Corrected the Serious Mint's description.
-Moved the X_ITEM_STAGES and CONFUSE_BERRY_HEAL_FRACTION macros to the top of the file for consistency's sake.
-Corrected the Tapunium Z's description.

* Oops. I forgot to delete the old .description of the X Attack

* Removed definition of CONFUSE_BERRY_HEAL_FRACTION from include/constants/items.h

* Moved CONFUSE_BERRY_HEAL_FRACTION back to include/constants/items.h

* Unified item description strings where possible

---------

Co-authored-by: Bassoonian <[email protected]>
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.

5 participants