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

Reduce number of redundant control points displayed on summary timeline #16446

Merged

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Jan 14, 2022

As pointed out in #16435, beatmaps with too many control points (usually added via external automation apps) could cause the lazer editor to grind to a halt.

The overheads here are mostly from the GL side. An eventual goal would be to render this in a smarter way, rather than using thousands of drawables. Until that, this optimisation should help reduce the overhead by omitting control points in close proximity that are redundant for display purposes.

I've tried to contain this in the display logic directly, with the goal that it can be ripped out as fast as it was added. Certainly required more changes than I hoped for, but I don't think it's too ugly.

Testing with this beatmap

Before:

20220114 171307 (osu!)

After:

20220114 171056 (osu!)

As pointed out in ppy#16435, beatmaps
with too many control points (usually added via external automation
apps) could cause the lazer editor to grind to a halt.

The overheads here are mostly from the GL side. An eventual goal would
be to render this in a smarter way, rather than using thousands of
drawables. Until that, this optimisation should help reduce the overhead
by omitting control points in close proximity that are redundant for
display purposes.

I've tried to contain this in the display logic directly, with the goal
that it can be ripped out as fast as it was added. Certainly required
more changes than I hoped for, but I don't think it's too ugly.
@peppy
Copy link
Sponsor Member Author

peppy commented Jan 14, 2022

Note that with the 1000ms setting, there are a few holes in the timeline. This can be adjusted down to 500ms to still resolve the issue amicably in the linked beatmap if that's seen as preferable. A more optimal implementation would consider the display pixels or combine the points at a pre-drawable level, but not willing to spend time there for now, as this is a "best effort" display that is generally not used for precise purposes.

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

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

Seems good in general.

@frenzibyte
Copy link
Member

Note that with the 1000ms setting, there are a few holes in the timeline. This can be adjusted down to 500ms to still resolve the issue amicably in the linked beatmap if that's seen as preferable. A more optimal implementation would consider the display pixels or combine the points at a pre-drawable level, but not willing to spend time there for now, as this is a "best effort" display that is generally not used for precise purposes.

I was slightly thinking of it being a setting configurable by the mapper at first, but given that such cases can only happen via external tools I think going with constant intervals sounds good. I think 500ms is a better choice, but would like to hear what others say.

@peppy
Copy link
Sponsor Member Author

peppy commented Jan 15, 2022

Negative. It will not be a setting. We do not add settings for stuff like this.

Comment on lines 72 to 73
// kiai sections display duration, so are required to be visualised.
public bool IsRedundant(ControlPoint other) => (other as EffectControlPoint)?.KiaiMode == effect.KiaiMode;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no type check? so any other point type can hide this? may be especially relevant for scroll speed changes, which are handled by effect points on scrolling rulesets now

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I don't quite follow. The existing as check will cause a null/false if the type doesn't match.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh. i was defeated by nullable tri-state boolean logic

@bdach
Copy link
Collaborator

bdach commented Jan 15, 2022

Does appear to cover the general framerate, but seeks are still sort of dreadful on maps where this was reproduced. Out of scope of this though.

Also going to note that the redundancy checks aren't still super great perf wise (quadratic lookups due to checking every pair of groups and every pair of points in groups) but it's slow once per every change to the set of control points rather than slow every frame so definitely an improvement.

bdach
bdach previously approved these changes Jan 15, 2022
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.

I'm ok with this as is. If you think it's a good idea to split the check mentioned in review conversation above into its parts to read better then I'm all for it but I don't think it's a huge deal.

bdach
bdach previously approved these changes Jan 16, 2022
@smoogipoo smoogipoo merged commit 9920ff5 into ppy:master Jan 19, 2022
@peppy peppy deleted the summary-timeline-control-point-optimisation branch January 19, 2022 07:33
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.

None yet

4 participants