Closes https://github.com/ppy/osu/issues/35651.
The reproduction steps provided in the issue are too complex even. In my
testing all you need to do is go into editor, replace the background via
external editing, and exit out to song select; you'll immediately see
loss of selection on the carousel, the set panel still using the old
background, and eventually a crash when you attempt to re-select any of
the difficulties of the edited set.
`HandleItemsChanged()` - an optimisation aiming to reduce the number
of redundant re-filters due to minor changes to realm models that aren't
visible to the user anyway - ignoring changes to `BeatmapInfo.ID` after
re-entering song select post-external edit meant that song select would
retain stale beatmap models that no longer existed in the realm
database, thus failing refetch attempts via `GetWorkingBeatmap()` or
https://github.com/ppy/osu/blob/8f6f859c15bdfc9f10d5754c254fca7b9dd9bc9b/osu.Game/Screens/SelectV2/FooterButtonOptions.cs#L56-L57
This was primarily written to fix
https://github.com/ppy/osu/issues/35538, but also incidentally targets
some other scenarios, such as:
- When switching from artist filtering to title filtering, selection
sometimes would stay at the group under which the selection's artist
was filed, rather than moving to the group under which the selection's
title is filed (in other words, the group that *the selection is
currently under*).
- When simply assigning a beatmap to a collection such that it would
be moved out of the current group, the selection will now follow to
the new collection's group rather than staying at its previous
position.
Whether this is desired is highly likely to be extremely situational,
but I don't want to introduce complications unless it's absolutely
necessary.
This has a significant performance overhead because
`CheckModelEquality()` isn't free, but it doesn't seem horrible in
profiling.
Calling `HandleItemActivated()` rather than its intended 'parent method'
of `Activate()` meant that selection state was not correctly
invalidated:
https://github.com/ppy/osu/blob/819da1bc38dbc9676f8f1ee3924bfd8d9cb50347/osu.Game/Graphics/Carousel/Carousel.cs#L157
which in turn meant that carousel item Y positions would not be
recalculated correctly after the group was expanded, which meant that
the items would become
- visible,
- stuck to the bottom of the expanded group,
- one on top of another.
Which is not something that's going to perform well.
Certified OOP moment.
Closes https://github.com/ppy/osu/issues/35003.
Bit dodgy to use `CurrentSelectionItem` for this. Ideally I would use
the global `Beatmap.IsDefault`, but I kind of don't want to violate the
rule that `BeatmapCarousel` shouldn't have direct access to the global
beatmap. And this seems to work, so... maybe fine to use until it
doesn't?
The equality check that was supposed to replace the read of
`CurrentSelectionItem` showed up as a hotspot in profiling.
The selection updating code looks a little stupid after this, but all in
the name of performance...
I'm not exactly sure on the reproduction scenario here, but I have had
switching ruleset with converts disabled crash on me a few times
today. It appears to happen sometimes when after the switch the expanded
group no longer exists in the set mapping, because a filter just ran and
removed that group from set of possible groups because there'd be no
beatmaps under it.
I tried to manufacture a quick test but it's not a quick one to test
because filtering intereference is required to reproduce, I think.
I will try again on request but I mostly just want to get this fix out
ASAP before I finish up for the day.
Addresses https://github.com/ppy/osu/issues/33443, maybe.
I considered adding tests but they'd likely be janky and take a long
time to write, so decided against until there's a demand for it.
Closes https://github.com/ppy/osu/issues/35010.
The issue here does not reproduce consistently, and is more or less
random in presentation. That said, using a large enough realm database
more or less ensures that the issue will present itself (in testing on a
large realm db, the failure rate is around ~50%).
This actually regressed in https://github.com/ppy/osu/pull/34842. The
core failure in this case is here:
https://github.com/ppy/osu/blob/fd412618dba7399b1778b0902b73ffbae21a2399/osu.Game/Screens/SelectV2/BeatmapCarousel.cs#L161
The `CheckModelEquality()` call above is comparing two `BeatmapInfo`s,
but a84c364e44 changed the
`BeatmapInfo`-comparing path of `CheckModelEquality()` to use
`GroupedBeatmap` instead. Due to this, `CheckModelEquality()` falls back
to reference equality comparison for `BeatmapInfo`s. When that reference
comparison fails, the carousel stops detecting that the current
selection was deleted from under it correctly, and therefore the
proximity-based selection logic never runs.
Due to the human-obvious mechanism of failure and relatively easy
manual reproduction I've decided not to try and add tests for this,
as they are likely to take a long time to write due to the mechanism
of failure being incorrect use of reference equality specifically. That
said, I can try on request.
11 is excessive. There can ever be at most 9 of these panels, ever,
because there are at most 9 possible letter grades at this time (F, D,
C, B, A, S, S+, SS, SS+).
d4b357dfa0 contains a sneaky regression.
The previous code read:
if (CurrentSelection != null && CheckModelEquality(beatmap, CurrentSelection))
RequestSelection(matchingNewBeatmap);
and the new one reads:
if (CurrentSelection is GroupedBeatmap currentBeatmapUnderGrouping)
{
var candidateSelection = currentBeatmapUnderGrouping with { Beatmap = beatmap };
if (CheckModelEquality(candidateSelection, CurrentSelection))
RequestSelection(candidateSelection);
}
The point is that we want to reselect `matchingNewBeatmap` here, not the
old selection. The `CheckModelEquality()` check's purpose is to check
whether *the current selection needs updating*.
I'm not sure why tests just wonderfully passed despite this, but my
suspicion is that it was because of accidental copying of realm guids
that obscured this problem.
This is probably where things get a little controversial.
There are some song select flows wherein song select just wants to
ensure sanity by authoritatively setting the global beatmap. The goal is
to change the beatmap immediately and instantly. Therefore it should
kind of be the carousel's job to figure out its grouping complications.
To that end, `CurrentSelection` is made virtual, and overridden in
`BeatmapCarousel` to perform a sort of reconciliation logic. If an
external component sets `CurrentSelection` to a `BeatmapInfo`, one of
the two following things happen:
- Nothing, if the current `GroupedBeatmap` is already a copy of the
beatmap that needs to be selected, or
- The carousel looks at its items, finds any first copy which matches
the beatmap that the external consumer wanted selected, and changes
selection to that instead.