Fix breaks not showing unless already ordered in the beatmap file

[frenzibyte]

• Closes Break not showing in lazer #28717

BreakOverlay shows and hides by populating transforms at the designated periods using BeginAbsoluteSequence. The logic expects the breaks list to be ordered chronologically, otherwise the transforms of the earlier break periods will overwrite any transforms made later (and therefore make later break periods not have the overlay visible as seen in #28717).

This can be solved locally in BreakOverlay by reordering the break periods before applying the transforms, but I feel it's generally natural to expect such lists to be ordered beforehand, so I solved this differently by changing IBeatmap.Breaks to be a SortedList instead (similar to ControlPointInfo etc.).

Before:

CleanShot.2024-07-08.at.15.49.48-converted.mp4

After:

CleanShot.2024-07-08.at.14.30.25-converted.mp4

[bdach]

Did you look into whether this can be tested (the part where breaks do not display, specifically)? I'd like it to be tested if at all possible.

[smoogipoo]

I have slight concerns about the amount of extra stuff we're doing every beatmap conversion. This is effectively doing an O(n log n) operation every convert, on top of what I'd already consider to be wasteful.

As I've mentioned previously, I'd like Beatmap to be treated as a plain old data model, whereas this is adding (albeit hidden) logic to it. The same is done at parse time for HitObjects.

Maybe it's not an issue here since it's breaks so N is expected to be very low, though.

I'll hand this off to another reviewer to evaluate whether my concerns are worthwhile at this point.

[bdach]

This is effectively doing an O(n log n) operation every convert

Given I'd realistically expect n<10 in this case then I see this as a moot concern. I'll profile if you're concerned but I'm 99% sure this won't even show up.

A local sort could work but I think this is a good invariant to put down. We already enforce start time ordering on hitobjects and control points.

[frenzibyte]

Did you look into whether this can be tested (the part where breaks do not display, specifically)? I'd like it to be tested if at all possible.

The problem with them not displaying is due to BreakOverlay taking an unordered list of break periods, i.e. due to unexpected behaviour. I'm not sure how I can write a test case for that. The best I can do is to write a test case that does all of the steps in #28717 (comment) and then check if the break periods are ordered or not, but I felt that would be too much.

I meant to add some videos showcasing the issue and compare between master & PR, but I had to leave desk abruptly. It's attached now.

As I've mentioned previously, I'd like Beatmap to be treated as a plain old data model, whereas this is adding (albeit hidden) logic to it. The same is done at parse time for HitObjects.

I wouldn't mind following the path of simplicity myself. At first, I've had some concerns about using SortedList, but as mentioned above, it doesn't really matter since it can't be that large. I think SortedList just makes it more convenient to expose the data in an ordered fashion, and simplify code in many places. It doesn't include logic that could end up backfiring like bindables would.

Not using SortedList is also possible. Break periods are mutated in limited places, and we can make such places responsible for reordering the breaks.

[bdach]

I'm not sure how I can write a test case for that

Surely you can imagine a way? Something like a player instance, with a manual backing clock and 2 breaks specified out of order? Seek to the middle of first break, assert that the overlay is showing?

To me this is very close to "basic correctness" so to not have a test for this sort of core behaviour is hard to swallow.

[frenzibyte]

A realistic test case is what I mean, but I guess intentionally adding break periods out of order is technically possible to do in game (as one could do so in the .osu file), so sure, can do that.

[bdach]

Profiling (release build, captured using tracing configuration)

On https://osu.ppy.sh/beatmapsets/972#osu/9007 (55 breaks, which I consider to be close to, if not the peak of sane edge cases):

On user's beatmap from the issue thread (3 breaks):

Based on these results I personally conclude that performance worries here are, as I suspected, unsubstantiated and not worthy of further scrutiny.