Skip to content

Change the Highscore List from a static Top 5 to a dynamic Top N#1009

Open
dgruss wants to merge 5 commits into
UltraStar-Deluxe:masterfrom
dgruss:topscorecount
Open

Change the Highscore List from a static Top 5 to a dynamic Top N#1009
dgruss wants to merge 5 commits into
UltraStar-Deluxe:masterfrom
dgruss:topscorecount

Conversation

@dgruss
Copy link
Copy Markdown
Contributor

@dgruss dgruss commented May 30, 2025

The number is configured via a new setting in the ini file:
[Advanced]
TopScreenSize=10

for example. I tested values up to 15. There is nothing preventing setting of higher values but depending on the theme the entries will start to overlap in the highscore.

I did not add it to the in-game configuration as it would require an immediate restart anyways (or adjust the memory allocation).

@barbeque-squared
Copy link
Copy Markdown
Member

NB: the list below became a bit longer than I expected. It's something I also wanted to do at some point, so obviously I have some ideas on how I would do it, but I've tried to not let any of that bias leak into this.

Couple of issues I have with this:

  • there are traces of unrelated changes, mostly DuetScore things in UIni
  • it deletes a whole bunch of stuff from the theming (TopX 3, 4, 5 mostly) but I presume it's still trying to load them regardless
  • function AddText(X, Y: real; ThemeText: TThemeText; Replacement: string = ''): integer; overload; I am really not a fan of slapping on random extra parameters that have random defaults. Doubly so because other language elements like SONG_JUMPTO_SONGSFOUND do not appear to need this
  • going back to SONG_JUMPTO_SONGSFOUND, that (and other language elements) appear to use %d and %s things in a string, whereas this uses <N>
  • TopScreenSize I don't like this variable name. Not sure if TopNScreenSize is better though, because it's really more something like HighscoreScreenNumEntries or something
  • It's still using old theme element names (the first two of the old ones) but they now have no relation whatsoever with what they actually do anymore. They have "Top5" in the name, and they aren't for the first two entries anymore. While I can (kinda) forgive the Top5 thing (I really don't understand why it wasn't just called "Highscores" way back in the day), the themes are hard enough to understand as they are and Top5TextName2 is obviously not being interpreted as a text element, so why does it look like a text element in the themes. IIRC text elements can actually have Width and Height, you could have 1 element for the column header and then another for the entire column and then name them HighscoresPlayernameColumnHeader and HighscoresPlayernameColumnData for example?
  • You could also 'cheat' and instead of doing difficult things with Top X players just rename the entire screen to Highscores (maybe in a preceding and separate PR though -- I really like split PR's where one does a more-or-less no-op preparation and then the actual feature is like 5 lines) and also rename the actual files etc (and in that case it's ok to leave the actual theme elements as they are -- see for example what I'm doing with the P3R or SixP theme hell but in the code it's all Solo3PP3 or Duet6PP4). The screen already numbers the entries anyway.
  • is this really an advanced setting or is it better placed in the [Theme] section?
  • is there really no way to at least prepare this for being changed run-time without having to restart the entire game? Because right now it's just kicking that can down the road for when the options screens get overhauled. I'd rather it be the other way around: it can be changed run-time, it just isn't actually done anywhere yet.

I'm not against the feature itself, it's just taking shortcuts and doing things slightly different from how most/everything else is doing it. Which are (among) the two things I've been working to slowly get rid of the last few years (and isn't finished by a long shot) which means in its current state, this PR will not be accepted.

@dgruss
Copy link
Copy Markdown
Contributor Author

dgruss commented Jul 6, 2025

that makes sense. i tried to minimize the edits.... a bigger overhaul would allow to make this nicer. i'm currently still using this on my builds as it's better than nothing for me

@dgruss
Copy link
Copy Markdown
Contributor Author

dgruss commented May 25, 2026

@barbeque-squared i have rewritten the entire PR based on your feedback. the code is now much cleaner and more generic, and split up into meaningful separate commits

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.

2 participants