Enforce real time difficulty for mod combinations which are not stored in the database

[tsunyoku]

Old discussion about the idea available on Discord: https://discord.com/channels/188630481301012481/380598781432823815/1245654906761777267.

Talked with smoogi about the idea and seems like a good way to get the ball rolling. This will allow for awarding pp on rates for double time other than the default (1.5x) and/or awarding pp for non-legacy mods which have difficulty changes. There's technically some edge cases here when a mod has settings which won't effect difficulty changes and will still go through realtime, but I don't think this is a major issue (and doesn't have a pretty solution). Classic is excluded from the "non-legacy mods" because default CL settings is treated the same as non-CL for now - this might become messy once difficulty calculation changes that effect non-CL specifically are deployed.

I've renamed the REALTIME_DIFFICULTY env var to ALWAYS_REALTIME_DIFFICULTY to try and reduce confusion over when realtime calculations will be used. When merging this'll need to be accounted for to avoid accidentally enabling realtime difficulty always.

To help smoogi/the team with profiling and the implications of realtime calculations on production, I've added a calculate-realtime-difficulty-attributes datadog timer with tags for the mods and ruleset. It's worth noting that the time will include the .osu file download time. This should help figuring out if realtime is too bad to use always, since long-term that would be the better option.

isRankedLegacyMod exists as an easy solution to figure out if the mod is going to be cached in the database. I don't really like this method in general but I don't know of a better way to do it...

There's an added test using OsuModMuted and OsuModTraceable as these are the only 2 mods which are non-legacy, so would be expected to go down the realtime path.

[bdach]

Talked with smoogi about the idea

Was this in DMs or something? I'd rather not proliferate that form of communication.

I don't disagree with the decision to do it, but I'd rather not play telephone game over such matters.

It's worth noting that the time will include the .osu file download time

I would hope that production would not download beatmaps. Or at least hope that it caches them if it has to download.

I wanted to write tests for these cases, however with this diff alone it's not really possible as in all cases mustUseRealtimeDifficulty would be true the mod will have Ranked as false so it'll never reach this block. Tests can be added in the future when this is merged.

I don't understand. How does merging this help write tests? Either tests can be written for this now and after merge, or neither now or after merge until something else happens?

[smoogipoo]

Was this in DMs or something? I'd rather not proliferate that form of communication.

Only briefly. It was not discussed in depth but I said it's a good path to go down.

[tsunyoku]

I don't understand. How does merging this help write tests? Either tests can be written for this now and after merge, or neither now or after merge until something else happens?

It would allow for setting Ranked on a mod that relies on the realtime path. Merging won't immediately make tests writable, I'm just highlighting that reaching this code with the current set of ranked mods is impossible.

I would hope that production would not download beatmaps. Or at least hope that it caches them if it has to download.

This is existing logic, the download path is settable with an env var so I'm not sure if there's some internal service communication that has some caching or something but if not then I'm open to replacement suggestions but I don't know how the infra works at that level.

[bdach]

It would allow for setting Ranked on a mod that relies on the realtime path. Merging won't immediately make tests writable, I'm just highlighting that reaching this code with the current set of ranked mods is impossible.

This makes much more sense as an explanation.

This is existing logic, the download path is settable with an env var so I'm not sure if there's some internal service communication that has some caching or something but if not then I'm open to replacement suggestions but I don't know how the infra works at that level.

Sure, it is, it has also never been ran on production, which means that figuring out whether or not the live version should download beatmaps or not is a valid review concern in my eyes.

[tsunyoku]

figuring out whether or not the live version should download beatmaps or not is a valid review concern in my eyes.

Yeah I 100% agree, just not sure if it's something I can personally weigh in on. I don't know enough about the infrastructure to comment.

[tsunyoku]

It would allow for setting Ranked on a mod that relies on the realtime path. Merging won't immediately make tests writable, I'm just highlighting that reaching this code with the current set of ranked mods is impossible.

I realise this isn't technically true because mods like muted and traceable are ranked which do not affect difficulty calculations, but fit under the conditions for going through realtime calculations. Should I add tests for those cases? Is it a concern that these mods are going through realtime calculations when they technically have no effect on difficulty (yet)?

[bdach]

Should I add tests for those cases?

I'll always take tests if it is possible to write them sanely. Not sure it is going to be in this case, but you could try.

[bdach]

I'm not even sure how to cross-check that this list is correct / complete right now.

Also I'd probably slightly prefer if this was listing leaf classes in the inheritance classes (don't like the // this also catches ManiaModFadeIn comment, and worried about future accidental inheritance), but probably not as important as not being to tell if this is correct.

[bdach]

There are two places to check (that I won't link to, for hopefully obvious reasons). So at least I know that now.

[peppy]

I think we can push this out for testing if @bdach is on board with the applied changes.

[bdach]

Not sure how that happened but the envvar had a space in name. Removed in 8b57913.

[bdach]

Well I've done some testing and this appears to do what it says I guess. That said, @peppy If you're going to try this:

• VERY IMPORTANT: Make sure to update the envvar from REALTIME_DIFFICULTY to ALWAYS_USE_REALTIME_DIFFICULTY. It defaults to 1 (and did previously). On production it must currently be set to 0. If you don't update the envvar name (or probably, more likely, specify both in the interim), this is going to start using realtime difficulty for ALL scores which WILL die of death.
• The realtime difficulty process does a web request for EVERY score (to fetch the .osu file). There is NO caching instituted. This is not news, but since we're wanting to switch things on, this is a consideration that this PR introduces. Specifying file:// protocol to bypass that WILL NOT work:

[network] 2024-08-07 09:33:59 [verbose]: Request to file:///home/dachb/Documents/opensource/diffcalc-sheet-generator/beatmaps/869310.osu failed with System.NotSupportedException: The 'file' scheme is not supported.
[network] 2024-08-07 09:33:59 [verbose]: at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
[network] 2024-08-07 09:33:59 [verbose]: at osu.Framework.IO.Network.WebRequest.internalPerform(CancellationToken cancellationToken).

• Realtime difficulty WILL immediately be used after this is deployed, for scores that use a few mods which did not exist in stable, and thus far worked because we basically went "this mod does not affect difficulty attributes, so we can pretend it does not exist for the sake of difficulty attribute lookup / treat it as nomod". As far as I know, this will be the case for mods such as Muted and Traceable. Possibly others that I'm forgetting.

[bdach]

Under the premises above

[developomp]

If it's not a huge hassle, there should probably be a partial pp recalc for old lazer scores so they are retroactively rewarded pp.

[bdach]

If it's not a huge hassle, there should probably be a partial pp recalc for old lazer scores so they are retroactively rewarded pp.

We haven't done this thus far when switching mods to ranked, so I dunno about that.

[stanriders]

If it's not a huge hassle, there should probably be a partial pp recalc for old lazer scores so they are retroactively rewarded pp.

We haven't done this thus far when switching mods to ranked, so I dunno about that.

It's probably gonna be covered by a pp deploy anyway, so it's not really necessary

[peppy]

Well I've done some testing and this appears to do what it says I guess. That said, @peppy If you're going to try this:

• VERY IMPORTANT: Make sure to update the envvar from REALTIME_DIFFICULTY to ALWAYS_USE_REALTIME_DIFFICULTY. It defaults to 1 (and did previously). On production it must currently be set to 0. If you don't update the envvar name (or probably, more likely, specify both in the interim), this is going to start using realtime difficulty for ALL scores which WILL die of death.
• The realtime difficulty process does a web request for EVERY score (to fetch the .osu file). There is NO caching instituted. This is not news, but since we're wanting to switch things on, this is a consideration that this PR introduces. Specifying file:// protocol to bypass that WILL NOT work:

[network] 2024-08-07 09:33:59 [verbose]: Request to file:///home/dachb/Documents/opensource/diffcalc-sheet-generator/beatmaps/869310.osu failed with System.NotSupportedException: The 'file' scheme is not supported.
[network] 2024-08-07 09:33:59 [verbose]: at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
[network] 2024-08-07 09:33:59 [verbose]: at osu.Framework.IO.Network.WebRequest.internalPerform(CancellationToken cancellationToken).

• Realtime difficulty WILL immediately be used after this is deployed, for scores that use a few mods which did not exist in stable, and thus far worked because we basically went "this mod does not affect difficulty attributes, so we can pretend it does not exist for the sake of difficulty attribute lookup / treat it as nomod". As far as I know, this will be the case for mods such as Muted and Traceable. Possibly others that I'm forgetting.

Thanks for the summary. This all sounds in line with my understanding. I'll look to deploy this tomorrow.

[peppy]

Reasoning for not deploying this yet: if it was to be deployed, scores set with non-precomputed difficulty attributes will get slightly different calculation methods. We are postponing deploy of this until after all pp is handled by osu-queue-score-statistics an we have a clean slate with the same algorithm used everywhere.

[smoogipoo]

Something that's been on my mind, but I've been weary of mentioning it because it'll get pushback ("release this ASAP and see if it breaks first!!!"), is that we already have https://github.com/ppy/osu-beatmap-difficulty-lookup-cache which is duplicating this functionality.

It may be worthwhile to hook that up (it's a web request that's being done anyway so overhead should be about the same) and optimising the caching local to that project.