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/34062.
The root cause of the issue is that `OnEntering()` calls
`onArrivingAtScreen()`, which calls `ensureGlobalBeatmapValid()`, which
would call `checkBeatmapValidForSelection()` with a `FilterCriteria`
instance retrieved from the `FilterControl`.
The problem with that is at the time that this call chain is happening,
`FilterControl` is not yet loaded, which means in particular that it has
not bound itself to the config bindable, as that happens on
`LoadComplete()`:
https://github.com/ppy/osu/blob/bff07010d1f9874125baf2918f02c5cf61a5ea60/osu.Game/Screens/SelectV2/FilterControl.cs#L198
To resolve this, retrieve the bindable in `SongSelect` itself, which
ensures it is valid for reading at the time the above call chain
happens.
A relatively recent regression. It's maybe not a huge one, in that it
probably doesn't matter all that much, but it is somewhat important to
keep the "locally modified" status of the set for as long as possible.
One reason for that is that keeping the "locally modified" status will
pull up a dialog when the user attempts to update the beatmap, warning
them that they will lose their local changes - this dialog will not show
if the online lookup flow is allowed to overwrite the map status with
something else.
Closes https://github.com/ppy/osu/issues/34810.
The reason why I touched it in this direction and not the other is only
because the standalone panel positioning of the button was touched last
in 92ed964627, thus I changed the set
panel to match that.
Closes https://github.com/ppy/osu/issues/35113.
Regressed in dfed564bda - setting
`Carousel.CurrentSelection` was not all that
`requestRecommendedSelection()` was doing there...
A potential point of discussion is whether this global beatmap switch
should be debounced or instant. I'm not sure I have a particularly
well-formed opinion on that. One argument in favour of not debouncing is
that if you look closely at the left side of the screen while the
debounce is in progress, you can still sort of see the broken behaviour
happen - it just doesn't stay there forever.
Thankfully `ensureGlobalBeatmapValid()` being called in every scenario
on screen suspension prevented this bug from being any worse than it is
right now.
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+).
I'm doing this silently to see if any users complain without being told
about the change. Without this, when holding left arrow for short
bursts, precisely *one* beatmap change happens before actual key repeat
kicks in, which feels really weird (updates the leaderboard / background
unexpectedly).
This is the simplest way to resolve the issue, so if users aren't
offended by it I think we should commit to it.
Personally it's still fast enough to not annoy me at all.
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.