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

Use classic score in total score processor #157

Merged
merged 5 commits into from
Oct 4, 2023

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Sep 28, 2023

As per #134 (comment)

@bdach bdach self-assigned this Sep 28, 2023
@pull-request-size pull-request-size bot added size/S and removed size/M labels Sep 29, 2023
@bdach bdach mentioned this pull request Oct 3, 2023
2 tasks
@peppy peppy self-requested a review October 4, 2023 06:39
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 4, 2023
@peppy peppy merged commit b191351 into ppy:master Oct 4, 2023
3 checks passed
@bdach bdach deleted the total-score-classic branch October 4, 2023 07:28
@smoogipoo
Copy link
Contributor

smoogipoo commented Oct 5, 2023

Should we not be increasing ScoreStatisticsQueueProcessor.VERSION as a result of this? And shouldn't this support revert of both versions? Though I believe it is the first time VERSION is being used in this way, I can't imagine any other purpose for it.

Edit: Actually it already has a check for previousVersion >= 2, so it's not the first time this is being done.

@peppy
Copy link
Sponsor Member

peppy commented Oct 5, 2023

Nah, I won't be running any kind of revert operation on already applied scores. Is what it is at this point.

@smoogipoo
Copy link
Contributor

As long as you understand the implication is not only that you will not be doing it, but that if you ever decide to do it it will become extremely difficult to do and blindly doing it will cause those users to have negative total scores.

@bdach
Copy link
Collaborator Author

bdach commented Oct 5, 2023

Edit: Actually it already has a check for previousVersion >= 2, so it's not the first time this is being done.

That's the typical "this processor did not exist on prior versions" check, isn't it? Slightly different.

As long as you understand the implication is not only that you will not be doing it, but that if you ever decide to do it it will become extremely difficult to do and blindly doing it will cause those users to have negative total scores.

I mean... if we're bringing this up as a concern, then any change to any scoring algorithm is potentially a concern. If the scoring algorithm silently changes on the game side and then the processor version gets incremented, and the processors begin to reprocess scores, everything that touches score is now potentially not reverting what it originally applied. For this to work in a 100% controlled manner you would need a score versioning scheme etc. Which I will do, if you think that's best, I'm just not sure that it is.

@smoogipoo
Copy link
Contributor

It's exactly the same thing at the end of the day.

switch (previousVersion)
{
    case < 2:
        return; // Processor didn't exist before this version.
    case < 7:
        totalScore -= standardisedScore;
        break;
    default:
        totalScore -= classicScore;
        break;
}

Anywhoo... #159

@peppy
Copy link
Sponsor Member

peppy commented Oct 5, 2023

With added context and IRL discussion, I think the above is a valid concern (should increase VERSION and add per-version revert logic).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants