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

Reset mod multiplier to 1 for classic mod in rulesets other than osu! #27816

Closed
wants to merge 4 commits into from

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Apr 9, 2024

Note that this PR doesn't mean that the osu! classic multiplier isn't changing. It means that I'm not generally convinced by any alternatives (and I don't believe I'm alone in that either).

Reprocessing job included, written in the simplest way possible (without persisting pre-modifier score anywhere, mirroring the server-side approach in ppy/osu-queue-score-statistics#246). Please scrutinise the logic therein closely as it's not obvious. It is sorta designed for easy reusability in the future but I dunno.

@peppy peppy self-requested a review April 12, 2024 07:18
var refetched = r.Find<ScoreInfo>(score.ID)!;
if (newTotalScore != null)
refetched.TotalScore = (long)newTotalScore.Value;
refetched.TotalScoreVersion = LegacyScoreEncoder.LATEST_VERSION;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're doing this in three different methods in this processor class now, alongside local queries for current version. This means if two migrations need to happen, only the first to execute will actually get run, unless I'm missing something.

May need to re-think how these are being applied. Either applying all total score changes in one method, or updating the TotalScoreVersion as a final step instead of locally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be changing the version value to be incremented once per change (similar to realm versioning) and specifically apply migrations based on the imminent version number (then incrementing by one on application), but that's a bit of an annoying structural/logic change to make initially.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct but this is also very annoying to handle properly.

I tried 004eaaf I guess, but also that commit is unsafe because if you happen to kill the game when it's updating the total score versions, it'll apply the mod multiplier change twice.

The big problem here is that the migration is not idempotent, i.e. there is unrecoverable data loss in a way. This is yet another argument to store the raw score pre-multipliers again I guess.

I dunno. I'd probably personally just close this PR and re-do from scratch properly start-to-end with storing the pre-multiplier value to client db and to server db, but you seem insistent on pushing this out so if you have better ideas then feel free to take a shot.

@bdach
Copy link
Collaborator Author

bdach commented Apr 12, 2024

closing due to wrong approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants