- Part of https://github.com/ppy/osu/issues/37818
During review, I would like to direct particular attention to the
following changes:
## [Migrate song select to new score multiplier
API](https://github.com/ppy/osu/commit/945fd78539da3ae57d1550a5bbfb0f859d153cc4)
This was a confusing change to write because of the way song selects
hook their mod overlays up to global bindables. In particular different
things happen in different circumstances.
- When going through `SongSelect.CreateModOverlay()`, which is called by
the base `SongSelect`, the mod overlay is automatically bound to global
bindables via `SongSelect.on{ArrivingAt,Leaving}Screen()`.
- For multiplayer user mod select overlays, which are bolted on by
subclasses of `SongSelect`, manual hook-up is required.
- As for free mod select overlays, they don't show mod multipliers at
all, and don't have easy access to the ruleset, and thus the hookup is
skipped entirely as redundant.
## [Fix score multiplier registrations being shared between
implementations via superclass static
fields](https://github.com/ppy/osu/commit/ba0a7ad421e0c84c2d8162b6bbdd3a0683f5a6a6)
Revealed by `ScoreMultiplierCalculatorTest` starting to fail due to
interference from `OsuScoreMultiplierCalculator`.
It's not ideal from a performance standpoint but it's the simplest
choice for now. Tricks could be pulled to salvage the static. One is
```csharp
public class ScoreMultiplierCalculator<T>
where T : ScoreMultiplierCalculator<T>
{
}
```
This works because of generics internals; static instance members are
not shared between different specialisations of a generic class. It is
also very unintuitive, so I would rather not. (It trips a ReSharper
inspection too, which would have to be silenced.)
From a performance standpoint this is not ideal, but a significant chunk
of migrated usages already precede the construction of the calculator
via the known-expensive `RulesetInfo.CreateInstance()`, and the paths
that actually construct the calculator do not appear to be that hot. If
need be, this can be handled by actually caching ruleset instances and
their derivative subcomponents.
## [Introduce passing of context to score multiplier
calculator](https://github.com/ppy/osu/pull/37845/changes/9e9242b3221dddacd226f4b3b9c5632d7350e998)
This is required for two reasons:
- The upcoming mod rebalance will require out-of-band supplementary
information that is not available for reading from the mod instances
themselves for calculating the multiplier.
- This context, namely passing of `ScoreInfo`, will be used for
implementing backwards compatibility with old scores and their score
multipliers. This is required because it has turned out under inspection
that all server-side lazer replays recorded until now are missing
`TotalScoreWithoutMods` due to an omission of not sending it across the
wire to spectator server.
Because the score import flow uses replays, filtered through
`LegacyScoreDecoder`, to populate total score in the realm database, it
is basically impossible to ignore scores that are missing
`TotalScoreWithoutMods`, because that will result in bug reports that
the scores do not have the new score multipliers applied.
Thus, passing of `ScoreInfo` will facilitate implementation of
versioning score multipliers, which should result in less breakage than
not doing so.
An example of this is added in 341b2d6e55,
which should handle the case of mania mod multipliers having been
changed without any attempt to facilitate for it in
https://github.com/ppy/osu/pull/30506.
---------
Co-authored-by: Dean Herbert <pe@ppy.sh>
It's been a while.
Notes:
- `SharpCompress` usages changed a bit. Manually adjusted these, mostly
just renames or adjusted parameters.
- nUnit 3 -> 4 migrated using
https://gist.github.com/peppy/07994386d793a117350cb5f24b156585. there's
a mode in this script to update to the newer `Assert.That` syntax but it
requires fixes and couldn't really be bothered.
- DeepEqual nuked as the only usage was on a disabled test. The reason
it's disabled has been merged upstream, but it's failing for other
(realm) reasons which I don't think is worthwhile to investigate for
now.
- This bumps Moq. I think the author is back in a sensible headspace and
the new version has the stupid shit removed, so probably okay? Nice to
be on a level playing field with packages for once in a long time.
- Automapper is silly, but we've discussed this elsewhere.
- `TestRealmKeyBindingStore` failures are a wildcard, but fixed by using
a more standardised testing method. Dunno why, don't care.
---------
Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
Closes https://github.com/ppy/osu/issues/22086
While this *is* a case of aspire-tier breakage, I hesitate to dismiss it
as wontfix, because the fix is somewhat easy, and probably results in
better accuracy across the board.
The issue in question here manifests when there is a significant amount
of lead-in time, like what the beatmap linked in the aforementioned
issue (https://osu.ppy.sh/beatmapsets/948643#osu/1981090) does.
Both stable and lazer record replay frames using absolute timestamps,
*but* the legacy replay format after the first frame uses time *deltas*,
i.e. amounts of time elapsed since the previous frame. This means that
the decoding process of the replay has to reconstruct absolute timestamps
by doing cumulative summation starting from the first frame.
When the very first frame has a timestamp that is very large in
magnitude (say, like the negative 2 billion that the beatmap in question
uses), this will lead to error if the cumulative summation is using
floating point numbers, because it will be adding a small magnitude
frame delta to a large magnitude cumulative absolute time. Which means
that sometimes adding the frame delta to the cumulative time *will not
change the cumulative time*, leading to the loss of time and thus replay
de-synchronisation.
Knowing that the legacy replay format only deals in integral time
values, however, this can be circumvented by just using a large enough
integral number type for the cumulative time tracking instead. I think
`long` in this case can be safely used "large enough" for our purposes:
> Console.WriteLine(long.MaxValue);
9223372036854775807
9 223 372 036 854 775 807 ms equals 292 277 024,6269277149 years
Closes https://github.com/ppy/osu/issues/32420.
The failure cause here is that in editor the beatmap version for the
beatmap affected (or... any beatmap, really), is 0 (ZERO). That is
probably a regression from https://github.com/ppy/osu/pull/32315, but
like... can we universally agree that calling that change "a regression"
in any capacity is dumb? Like what was that code *doing* playing dumb
reference games and copying stuff into an arbitrary instance that could
get or not get used later on? And now you have a 50/50 chance of
accessing the *correct* model's field, depending on whether you go via
`BeatmapInfo` or `Beatmap.BeatmapInfo`?
Moving the field to `IBeatmap`, i.e. what is by now - by consensus,
since https://github.com/ppy/osu/pull/28473 - supposed to be the "decoded
and materialised" beatmap, fixes this issue.
I probably should have done this as part of
https://github.com/ppy/osu/pull/28473 but it slipped my mind. Probably
for the better too because this change has rather large chances of
breaking stuff so maybe better to examine it in isolation (via diffcalc
runs or whatever).
For added humour points, you'd say that the field on `BeatmapInfo` was
not `[Ignore]`d, so this is a realm schema change, right? No. As far as
I can tell, it's not. I opened realm studio and `BeatmapVersion` *is not
a listed column` on `Beatmap` models.
I'm also not gonna get into the fact that I think `EditorBeatmap` doing
dumb games with juggling two `BeatmapInfo` references since
https://github.com/ppy/osu/pull/15075 is bad, because I don't think I
have the mental capacity to hotfix this by going down that train of
thought.
This subtle detail was messing with server-side score import flows.
Server-side, legacy total score will *already* be in `LegacyTotalScore`
from the start, and `TotalScore` will be zero until recomputed via
`StandardisedScoreMigrationTools.UpdateFromLegacy()` - so in that
context, attempting to move it across is incorrect.
Closes https://github.com/ppy/osu/issues/24061.
The gist of this change is that if the `LegacyReplaySoloScoreInfo`
bolt-on is present in the replay, then it can (and is) used to recompute
the accuracy, and rank is computed based on that.
This was the missing part of
https://github.com/ppy/osu/issues/24061#issuecomment-1888438151.
The accuracy would change on import before that because the encode
process is _lossy_ if the `LegacyReplaySoloScoreInfo` bolt-on is not
used, as the legacy format only has 6 fields for encoding judgement
counts, and some judgements that affect accuracy in lazer do not fit
into that.
Note that this _only_ fixes _relatively_ new lazer scores looking wrong
after reimport.
- Very old lazer scores, i.e. ones that don't have the
`LegacyReplaySoloScoreInfo` bolt-on, obviously can't use it
to repopulate. There's really not much good that can be done there,
so the stable pathways are used as a fallback that always works.
- For stable replays, `ScoreImporter` recalculates the accuracy of
the score _again_ in
https://github.com/ppy/osu/blob/15a5fd7e4c8e8e3c38382cfd3d5d676d107d7908/osu.Game/Scoring/ScoreImporter.cs#L106-L110
as `StandardisedScoreMigrationTools.UpdateFromLegacy()` recomputes
_both_ total score and accuracy.
This makes a _semblance_ of sense as it attempts to make the accuracy
of stable and lazer replays comparable. In most cases it also won't
matter, as the only ruleset where accuracy changed between the legacy
implementation and current lazer accuracy is mania.
But it is also an inaccurate process (as, again, some of the required
data is not in the replay, namely judgement counts of ticks
and so on).
For whatever's worth, a similar thing happens server-side in
https://github.com/ppy/osu-queue-score-statistics/blob/106c2948dbe695efcad5972d32cd46f4b36005cc/osu.Server.Queues.ScoreStatisticsProcessor/Commands/Queue/BatchInserter.cs#L319
- However, _ranks_ of stable scores will still use the local stable
reimplementation of ranks, i.e. a 1-miss stable score in osu! ruleset
will be an A rather than an S. See importer:
https://github.com/ppy/osu-queue-score-statistics/blob/106c2948dbe695efcad5972d32cd46f4b36005cc/osu.Server.Queues.ScoreStatisticsProcessor/Commands/Queue/BatchInserter.cs#L237
(it's the same method which is renamed
to `PopulateLegacyAccuracyAndRank()` in this commit).
That is all a bit of a mess honestly, but I'm not sure where to even
begin there...