With judgements occurring forcing emission of important frames, there's
the question of emitting redundant frames. In a majority of cases
still, a judgement *will* be tied to a specific user input, which means
that there's a high likelihood of duplicate frames, as one will be
emitted by the input change, and another by the judgement.
To a degree this is a pre-existing issue. The replay recording code
responds to both mouse movement and key presses by recording frames, so
depending on the intrinsic (basically undefined) ordering between
handling mouse move and key presses, it was possible for multiple frames
to be emitted with the same timestamp to the replay, each containing
partial input state changes (e.g. first frame with mouse position
changed, and the second with a key pressed).
The extent to which this increases the frame count will obviously depend
on the beatmap, but in my ballpark testing across all rulesets on a
single "relatively standard" beatmap (think no aspire chicanery), around
4-10% of frames can end up being duplicates / not meaningful. This has
implications both on replay size and on playback performance.
This commit therefore performs de-duplication of the frames based on
timestamp - only the last frame with a given timestamp ends up in the
final replay.
The CPU cost of this is negligible to the point of not being useful to
profile - it's a single condition gated behind a single float comparison
- Closes https://github.com/ppy/osu/issues/4287
- Probably closes https://github.com/ppy/osu/issues/25405 (but not
retroactively)
Up until now, whether or not a replay frame is emitted depended solely
on the user's input, i.e. mouse movement or key presses/releases. This,
intersected with the replay playback system which is given allowance to
perform interpolation between replay frames, leads to potential
situations wherein a replay can play inaccurately when a judgement takes
place without user input meaningfully changing. One such case is slider
ends with their 36ms of judgement leniency; see
https://github.com/ppy/osu/issues/25405#issuecomment-2879031106 for
details on that.
To that end, this commit aims to counteract that issue by *forcing* an
important replay frame to be emitted on every new judgement recorded
during gameplay. This will only benefit rulesets wherein judgements can
occur that are not inherently tied to user input changing, which are
going to be osu! as mentioned above, and maybe possibly catch. I don't
foresee this doing anything relevant for taiko or mania.
This fell out of my work on hit window-related replay issues. In my WIP
branch (that is probably going to get PR'd as draft soon) I refactored
`HitWindows` and found a few straggling test failures due to some places
reading hit windows for results that were not actually supported by the
underlying `HitWindows` implementations, leading to returning garbage
(mostly zeroes).
Importantly, there is one actual usage in game code with impact here -
`TaikoModSingleTap` was attempting to read the hit window for MEH, when
MEH was never actually a valid hit result in taiko (OK is). This was a
result of a copy-paste oversight from osu!, specifically from
https://github.com/ppy/osu/blob/51cf835fb6300aca53b5b98143d606f64d7a4d49/osu.Game.Rulesets.Osu/Mods/InputBlockingMod.cs#L58
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.