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 break generation not accounting for concurrent hitobjects correctly #28630

Merged
merged 5 commits into from
Jun 27, 2024

Conversation

peppy
Copy link
Member

@peppy peppy commented Jun 27, 2024

Closes #28622.

@peppy peppy changed the title Fix long note break woes Fix break generation not accounting for concurrent hitobjects correctly Jun 27, 2024
@frenzibyte
Copy link
Member

frenzibyte commented Jun 27, 2024

This part should also be changed to get the maximum end time (i.e. Beatmap.GetLastObjectTime()) instead of the last object's end time, right?

CleanShot 2024-06-27 at 08 06 56

I cannot easily imagine a test case that warrants that to be applied though (I don't understand the logic quite well).

@bdach bdach self-requested a review June 27, 2024 05:24
@bdach
Copy link
Collaborator

bdach commented Jun 27, 2024

I don't understand the logic quite well

The check is supposed to remove manually-adjusted breaks which are placed after all objects so that they don't hang around.

This part should also be changed to get the maximum end time (i.e. Beatmap.GetLastObjectTime()) instead of the last object's end time, right?

In theory yes, in practice it probably doesn't matter because of two circumstances:

  • Objects are assumed to be sorted by start time implicitly. This is also the reason why this pull even sort of works; if objects weren't guaranteed to be sorted by start time, this approach would be breakable, and instead you'd have to do something like pull start/end times of all objects into a flat list, sort them, and do a sweeping algorithm like the "balanced brackets" algo to actually determine which time intervals have no overlapping hitobjects.
  • The particular check in question here also checks if the break overlaps any objects, which means that even if the last object's end time is inaccurate, the overlapping check is going to remove the break anyway.

Due to the above I'm pretty sure it's not possible to manufacture a test case that exercises this. I'll add one for posterity at least but yeah.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't seem to find a way to break it further so probably fine until it's not

@peppy peppy merged commit f07a635 into ppy:master Jun 27, 2024
13 of 17 checks passed
@peppy peppy deleted the fix-long-note-break-woes branch June 28, 2024 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

osu! automatically insert break in between long note
3 participants