phoboslab / wipeout-rewrite

2.59k stars 198 forks source link

View highscores in options menu #100

Closed LuKP17 closed 8 months ago

LuKP17 commented 8 months ago

“Best times” button in the options menu leading to the first added page.

First page: page_options_highscores

Second page: page_options_highscores_viewer

Design choices:

It’s my first time contributing to real source code and I’m open to suggestions for improvements. I chose to work on this feature because I need an easy starting point and it has been really fun so far. My work can also end up being a temporary solution before the menu system gets rewritten. Looking forward to hearing your thoughts on this PR!

phoboslab commented 8 months ago

I have two remarks - the first one being the result of shortcomings of the "menu system" itself:

I'm not a fan of page_options_highscores_viewer_input_handler() being called from the page's draw function. I understand why it's there (and I've been using the same scheme for the highscore name entry), but modifying the menu state from within the draw() function can lead to some funky behavior.

Maybe modifying the menu system so that each page has a "default update function" (which you can override) and a separate "draw function" would make this a bit cleaner.

Another approach you could have taken here, is to put some invisible buttons/toggles on that page (with the text set to "") to control the race class/circut. This way you wouldn't need your input handler. Not saying that's the better solution. Just thinking aloud here.

In any case, this is fine for now – the menu system needs some rework first.

My second remark is the layout: It looks nice on higher resolutions and/or smaller UI scale but fails on 240p 1x or 480p 2x. I guess it could all be squeezed in with smaller font sizes for some of the lines?!

Anyway: nice work! I'll merge this as-is. Feel free to submit further PRs regarding the layout.

Edit:

Now that it is possible to view highscores during a race and in the options menu, do I need to make these screens share a function to retrieve and draw the highscores?

I don't have strong opinions here, but my hunch is that if we re-use the highscores layout for race end and the options menu it would lead to code that is harder to follow. I don't mind the duplication.