Replace hard limits to highscore saving with configurable limits

[BWagener]

Currently only the top 16 scores on a song will be saved. If a player achieves below 16 already saved scores their score will not be saved.

What does this PR do?

• replaces the constants used as conditions to decide if a score should be saved with configurable values
  • specifically the limit of scores saved by song and the minimum required score for saving (unchanged from previous value of 2000 points)
• store all scores above 2000 by default
• fixes the maxLines logic that determines how many lines should be output to the list of scores

Motivation

This comes in handy when you are playing with players who score lower on average and currently can never save their scores, once a song has 16 scores saved for it.

How to test these changes

If you run the builds from this PR, the following new settings (settings->game) with defaults apply:

• Limit high-scores (false)
• High-scores limit (16, has no effect with Limit high-scores set to false)
• Minimum high-score (2000)

Also, up to the top 16 high scores should show up for each song.

• If you play with these defaults, you should be able to see more than 16 high scores can now be saved per song. To verify this, you can check the database.xml file.
• Change the Minimum high-score to verify scores below the setting will not be saved, while scores above it will be.
• Enable Limit high-scores to verify that no more scores will be saved once the limit is reached
  • You could set High-scores limit to 0 to disable high-scores being saved completely
  • While no more scores should be saved, old scores should be preserved when you lower the limit, to verify this you can enable the limit and set it lower than the amount of high-scores you have on a song. The scores should remain even after restarting the game.

[github-actions]

Download the artifacts for this pull request:

• Performous-1.3.1-983-7922f76-alpha-debian_11.deb.zip
• Performous-1.3.1-983-7922f76-alpha-ubuntu_22.04.deb.zip
• Performous-1.3.1-983-7922f76-alpha-fedora_35.rpm.zip
• Performous-1.3.1-983-7922f76-alpha.AppImage.zip
• Performous-1.3.1-983-7922f76-alpha-fedora_38.rpm.zip
• Performous-1.3.1-983-7922f76-alpha-debian_10.deb.zip
• Performous-1.3.1-983-7922f76-alpha-fedora_37.rpm.zip
• Performous-1.3.1-983-7922f76-alpha-debian_12.deb.zip
• Performous-1.3.1-983-7922f76-alpha-fedora_36.rpm.zip
• Performous-1.3.1-983-7922f76-alpha-fedora_39.rpm.zip
• Performous-1.3.1-983-7922f76-alpha-ubuntu_20.04.deb.zip
• Performous-1.3.1-983-7922f76-alpha-msvc.exe.zip
• Performous-1.3.1-983-7922f76-alpha.dmg.zip
• Performous-1.3.1-983-7922f76-alpha-mingw-w64.exe.zip

This service is provided by nightly.link. These artifacts will expire in 90 days and will not be available for download after that time.

[twollgam]

All in all it's good, but some minor changes are needed.

And there is one question belonging layout things.

[BWagener]

All in all it's good, but some minor changes are needed.

And there is one question belonging layout things.

Great, I updated the formatting and prefix notation related stuff. Let me know what you think about the replies regarding the layout and function name.

[twollgam]

All in all it's good, but some minor changes are needed.
And there is one question belonging layout things.

Great, I updated the formatting and prefix notation related stuff. Let me know what you think about the replies regarding the layout and function name.

Yes I would, but I see only the code changes. Did you save the comments?

[BWagener]

Do these links work?

#983 (comment)
#983 (comment)

[twollgam]

Do these links work?

#983 (comment) #983 (comment)

It's strange. In the popups of the links I see the start of your comment but a click doesn't lead me to them. And I cannot find them anywhere in the pr.

@Baklap4 could this be a rights problem?

[BWagener]

That's odd, I'm on mobile but I'll try to copy the comments content here:

#1 regarding the function name
I figured it would fit since this is only used when loading the songs initially from db, put and store sound more generic.

Which one do you prefer?

#2 regarding the length of the displayed high scores
It doesn't seem to change with screen size.

This is 640x480:
image
It's the same for example on 1080p and 1440p.

The last two lines could overlap with a long song title. The same is true for 14 song entries. With 12 it only very slightly reaches into the song title box:
image

Personally I don't mind the list reaching that far down but I understand if you prefer to keep it smaller. We could also make the list length configurable.

Or we could align the scores to the vertical top instead of center, then there would be quite a bit more space.

[BWagener]

My comments regarding some thoughts I had on specific sections of the code ended up being their own review, does that show up for you?

It shows up for me, almost at the top of this thread. And my answers to your comments are there as well, for some reason. I'm pretty sure I used the reply function to respond to your review comments for those, though. Quite strange.

[twollgam]

#1 regarding the function name I figured it would fit since this is only used when loading the songs initially from db, put and store sound more generic.

Which one do you prefer?

Ok I understand your point. But a function name should always describe what the function is doing, not where it should be used ;-) You'll never know who could use the function where in the future.

So I prefer addHiscore like the others. I cannot imagine why this function should not be called after the initially phase.

But maybe I miss a thing. If you (@BWagener) are able to relate to call it simple addHiscore.

[BWagener]

But maybe I miss a thing. If you (@BWagener) are able to relate to call it simple addHiscore.

Yeah I think it'd clash with the existing addHiscore function with the same signature. What do you think about addHiscoreUnconditionally?

[twollgam]

But maybe I miss a thing. If you (@BWagener) are able to relate to call it simple addHiscore.

Yeah I think it'd clash with the existing addHiscore function with the same signature. What do you think about addHiscoreUnconditionally?

Yes, I agree completely 👍 Good suggestion. Take it and I will finish the review. Can you write some test hints to the pr description? Then we search someone how will test it or I will test it on Windows 11 next week.

[BWagener]

But maybe I miss a thing. If you (@BWagener) are able to relate to call it simple addHiscore.

Yeah I think it'd clash with the existing addHiscore function with the same signature. What do you think about addHiscoreUnconditionally?

Yes, I agree completely 👍 Good suggestion. Take it and I will finish the review. Can you write some test hints to the pr description? Then we search someone how will test it or I will test it on Windows 11 next week.

Great, I updated the PR description and I updated the function name in the latest commit. I updated the function docs to reflect what the functions currently do - the addSong docs seemed a bit outdated so I updated that too.

[twollgam]

LGTM

[kt--]

I personally find the score minimum diminishes the fun of using performous, since for various reasons, I am typically unable to attain a score over 2000. It would be nice if I could at least track my progress from tragic up to terrible.

[BWagener]

I'd also be in favor of changing the default minimum score required for saving.

I can imagine removing it completely might not be desirable though, I haven't tested how the score saving screen behaves in that case.

How about 100?

[kt--]

I'd also be in favor of changing the default minimum score required for saving.

I can imagine removing it completely might not be desirable though, I haven't tested how the score saving screen behaves in that case.

How about 100?

Yes, something around that value, but larger than what one can score with line noise ;)
EDIT: maybe 500? 300?