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

Move Descriptions in battle #4152

Merged
merged 14 commits into from
May 31, 2024
Merged

Conversation

Bassoonian
Copy link
Collaborator

@Bassoonian Bassoonian commented Feb 7, 2024

This PR builds off Pawkkie's #3571 implementation of TheXaman's move description branch. They cleaned it up and made it work with modern, I did the following:

  • Add support for EFFECT_PLACEHOLDER's description override
  • Changed the button to the L button to avoid conflicting with Mega Evolution/Z-Moves
  • Replace priority with battle category (explicitly showing priority values is non-vanilla)
  • Reorder to category/power/accuracy per SV
  • Add an indicator that the L button can be used for move info
  • Replace values of 0 with —
  • Whatever else is needed (we should figure out a scope for this PR)

People who collaborated with me in this PR

@TheXaman
@Pawkkie
@ravepossum

Discord contact info

bassoonian

@AlexOn1ine
Copy link
Collaborator

Thoughts on extending the move desc to add all of those?
image
Similar to Unbound. If there is not enough space the box could be extended.

@Bassoonian
Copy link
Collaborator Author

Shouldn't we at that point completely overhaul the interface to support longer move names too? Type and PP are already in the bottom right, split is on the to-do list, and that'd only leave effectiveness which we could maybe also cram in the bottom right like Unbound.

@AlexOn1ine
Copy link
Collaborator

Shouldn't we at that point completely overhaul the interface to support longer move names too? Type and PP are already in the bottom right, split is on the to-do list, and that'd only leave effectiveness which we could maybe also cram in the bottom right like Unbound.

Hm, I don't know. Up to you but maybe it would be better to merge the lone move desc for now and think later about my suggestion?

@Bassoonian Bassoonian marked this pull request as ready for review May 30, 2024 12:00
@Bassoonian
Copy link
Collaborator Author

I won't be able to work on this in the foreseeable future, and I don't think the button indicator necessarily needs to be blocking. PCG provided assets on Discord, so a follow-up PR is always welcome. This is ready for review other than that (and considering it's a commonly requested feature, I think we should merge it in sooner than later).

Comment on lines +165 to 168
static EWRAM_DATA bool8 sDescriptionSubmenu = 0;

static EWRAM_DATA bool8 sAckBallUseBtn = FALSE;
static EWRAM_DATA bool8 sBallSwapped = FALSE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The battle debug menu uses some hack to avoid using an EWARM. Not for this PR but we should explore it at some point. Maybe we will be able to remove those. I'll open an issue once it is merged.

@Bassoonian Bassoonian requested a review from AlexOn1ine May 30, 2024 22:10
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