Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect ordering of items at song select when certain sort modes are used #25888

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Dec 19, 2023

This is quite an unfortunate one, but I can't see a good way of fixing beyond a "revert" for now. Which is to say, it reverts behaviour for the sort modes which will obviously fail due to oversights, but keeps the optimisation in place for the others. Unfortunately this affects most of the commonly used sort modes.

Basically, some sort modes rely on aggregating a "max" value for each CarouselBeatmapSet, which is then used to sort the full carousel. The value of this "max" will change as a constraining filter is applied, since it only aggregates visible beatmaps:

/// <summary>
/// All beatmaps which are not filtered and valid for display.
/// </summary>
protected IEnumerable<BeatmapInfo> ValidBeatmaps => Beatmaps.Where(b => !b.Filtered.Value || b.State.Value == CarouselItemState.Selected).Select(b => b.BeatmapInfo);
private int compareUsingAggregateMax(CarouselBeatmapSet other, Func<BeatmapInfo, double> func)
{
bool ourBeatmaps = ValidBeatmaps.Any();
bool otherBeatmaps = other.ValidBeatmaps.Any();
if (!ourBeatmaps && !otherBeatmaps) return 0;
if (!ourBeatmaps) return -1;
if (!otherBeatmaps) return 1;
return ValidBeatmaps.Max(func).CompareTo(other.ValidBeatmaps.Max(func));
}

The optimisation applied in #25679 assumed that sorting was applied to all sets (even filtered ones), but the above smashes a hole in this theory. Here are two failing cases:


#25820

  • Ruleset is changed while difficulty sort is enabled

In this case, we actually need to run a full sort, specifically for the difficulty sort type. I haven't special-cased this for now, but if we ever re-apply optimisation in the future it will need to be considered.


#25820 (comment)

  • Difficulty sort is enabled
  • Text criteria entered, filtering (hiding) some panels
  • Current beatmap is updated, causing a Carousel.UpdateBeatmapSet – this is common when playing the beatmap as the LastPlayed will be updated

In this case, the beatmap will be added back to the carousel group using BinarySearch:

if (lastCriteria != null)
{
i.Filter(lastCriteria);
int index = items.BinarySearch(i, criteriaComparer);
if (index < 0) index = ~index; // BinarySearch hacks multiple return values with 2's complement.
items.Insert(index, i);
}

But due to some items being hidden when the last sort was applied, the binary search will not run successfully. I believe this case can be improved with some further thinking (sort could be run more comprehensively on filtered-away sets), but another more amicable path may be to cache the aggregate max value on a BeatmapSet (and only invalidating when count of difficulties changes), reducing the overhead of these filter operations massively.

Closes #25820.

Note that I haven't added tests for now. I might get to it later or someone else might beat me. It's not going to be easy to test either of these – one needs fake star rating changes based on ruleset, which as far as i can tell won't be easy, the other needs a test crafted specifically to break BinarySearch, which I couldn't make happpen on a quick attempt.

@peppy peppy requested a review from bdach December 19, 2023 06:37
@bdach bdach added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Dec 19, 2023
@bdach
Copy link
Collaborator

bdach commented Dec 19, 2023

Seems to work.

I gave writing tests a weak attempt but it is as annoying as you say, so in the interest of hotfixing this...

@bdach bdach enabled auto-merge December 19, 2023 08:52
@bdach bdach disabled auto-merge December 19, 2023 09:37
@bdach bdach merged commit c078d63 into ppy:master Dec 19, 2023
13 of 17 checks passed
@peppy peppy deleted the fix-sort-failures branch December 19, 2023 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:song-select next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difficulty sort breaks when switching rulesets
2 participants