Song Select: Add title as fallback for certain sorting modes

[SneakyKenny]

Greetings

Hello!

This is my first PR for osu!, I hope you'll welcome me with as much enthusiasm as I have opening this!

The following PR comes from some frustration on my end when using song select. I have not found issues or opened/closed PRs that address this topic, so I thought I'd shoot my shot!

To give some more context, I've been pretty inactive on osu! and playing almost exclusively songs I imported from stable. From what I understand, this makes all my beatmapsets have the same DateAdded. This triggers the ID fallback comparison, which, as noted in the code comments, is essentially random.

Writing this, I realize that I haven't checked if this addresses a behavior that changes from stable to lazer, or something that was always here!

This PR aims at making a small part of song-select sorting more intuitive by using the song's title as a fallback sorting method.

My feeling is that DateAdded "looks more random" than the title, especially in cases like mine where the DateAdded fallback-comparison outputs 0.

The implementation is the same as Artist with Title fallback.

Screenshots

On my installed osu version: sorting by BPM puts "Asymetry" in the middle of two mapsets of "Snow halation"

After the patch, mapsets with the same BPM will also be sorted by song title, for example at 173 BPM:

And further below:

[SneakyKenny]

Note:

This test passes before my changes to the sorting algorithm. I've validated through Rider's debugger that the test passes because of my changes and not because of the previous implementation.

If some other testing method / more intensive testing is required, do tell, I'll look for another way to test this better!

[bdach]

This being applied only to BPM sorting specifically makes zero sense.

I'll leave the judgement of whether this makes sense in any mode to @peppy, but I definitely do not understand why this would be behaviour specific to BPM sorting.

[peppy]

Probably makes sense to use everywhere instead of ID? Anything is better for the end user than literal random GUID comparisons.

(have not read code or assessed if this is feasible, just commenting rationale)

[bdach]

The fallback order on ties in all sort modes is date added, then guid. Author is trying to jam title somewhere in there I guess.

[SneakyKenny]

This being applied only to BPM sorting specifically makes zero sense.

I'll leave the judgement of whether this makes sense in any mode to @peppy, but I definitely do not understand why this would be behaviour specific to BPM sorting.

Hi,

I agree that this change being solely applied to BPM is odd. I indicated in the initial commit that I'm looking for feedback on your end of which sorting modes would benefit from this behavior.

The fallback order on ties in all sort modes is date added, then guid. Author is trying to jam title somewhere in there I guess.

Correct. In my initial commit I added title comparison before the both of them.

In this commit, I created a list of modes which would use this fallback, with the idea that there would be more modes added with your feedback. After a friend's review, he pointed out that allocating a list in a comparison operator would not be friendly on the allocator, so I moved the fallback to the switch statement above.

I asked a friend to review the PR, he pointed out that my implementation was not too friendly on the allocator, so I moved the fallback behavior to the switch statement (therefore a second commit). This change however would not be as maintainable if we were to use the fallback for every sort modes.

Probably makes sense to use everywhere instead of ID? Anything is better for the end user than literal random GUID comparisons.

That did cross my mind when initially looking for a way to make sorting "more intuitive". I decided to keep the behavior changes limited to the use-case I'm familiar with, but with the intent of expanding the scope after you'd given your opinion on it.

assessed if this is feasible

Sure is!

[SneakyKenny]

I've added a new commit that applies the fallback for all cases!