-
Notifications
You must be signed in to change notification settings - Fork 211
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
Summary screen #978
base: master
Are you sure you want to change the base?
Summary screen #978
Conversation
Emi, this already looks great. Thank you for putting all your work into it! ^^ |
Also, I'd be happy to merge this as just a summary screen replacement. Meaning that the Moves screen and the move-learning page could stay, no need to implement move swapping in this for the initial merge. ( A couple of graphic observations:
The original mockups for comparison: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the engine/pokemon/summary* files yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is still a work-in-progress, and looks great!
My only comment is there are a lot of magic numbers.. particularly related to tile IDs and tile palettes, ect.
I know the other codebase is similar; but when we have the opportunity to rewrite systems; personally i'd like to define constants for things like the tile ID's that way if we need to reorder graphics we can update the tile ID defined in the constant.
Thoughts @Rangi42 ? I'm talking about like what I did at the top of engine/overworld/weather.asm
I definitely agree about using named constants in general. I haven't reviewed the "meat" of this PR yet so can't comment on how many magic numbers there are. But yeah, you can see how other engine files define constants for meaningful tile IDs. |
Finally got around to resolving these issues and adding graphics constants. @Rangi42 re the met tab contents, as a restriction of seamless page switching, the bottom of the screen's vertical aligns can't be adjusted iirc. The pokerus change would require some further palette adjustments that I'll look into. |
I've updated the PR to include a fix for the hblank jittering, and feature inclusions for status, shiny and pokerus palettes, red/blue nature indicators on stats, and some other small things. The main blocking issue at this point is the graphical issues in the pc/in battle. Adding move swapping would also allow the moves screen to be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick cursory review of the code. Overall, it looks really clean. Will still need to do in-game reviews too.
ld e, a | ||
ld b, 0 | ||
.loop | ||
ld c, $10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is $10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ByteFill amount, b is cleared before the loop and remains cleared
Very close to ready to go, all that's left is removing the move swap screen in the party menu, adding move swapping checks for battle etc, and removing an artifact I just can't figure out when exiting the PC. |
Alright, all ready to go! There's nothing outstanding that I know of that would prevent this from going live in a release version right now. All optimized, documented, and shiny. |
Replaces the old stats screen with the new and improved summary screen. Ready to go!
summary.mp4
Outstanding TODOs in the summary screen file, copied for posterityAll complete! Ready to go.Some things I'd like to further do or seen done for the summary screen code wise that aren't necessarily functional