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

Improve editor performance for maps with many control points #27630

Merged
merged 7 commits into from Mar 20, 2024

Conversation

EVAST9919
Copy link
Contributor

@EVAST9919 EVAST9919 commented Mar 16, 2024

Similar implementation to TimelineTickDisplay.
Remaining overhead comes from the bottom part, but that's for another pr.

Map examples:
https://osu.ppy.sh/beatmapsets/982020
https://osu.ppy.sh/beatmapsets/1238185#mania/2574372

master pr
master pr
https://streamable.com/4pchzb https://streamable.com/t28jwh

Comment on lines 17 to 18
[Resolved]
private Timeline? timeline { get; set; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was using TimelineTickDisplay as a reference, will remove.

Comment on lines +48 to +49
(ToLocalSpace(timeline.ScreenSpaceDrawQuad.TopLeft).X - TopPointPiece.WIDTH) / DrawWidth * Content.RelativeChildSize.X,
(ToLocalSpace(timeline.ScreenSpaceDrawQuad.TopRight).X) / DrawWidth * Content.RelativeChildSize.X);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does the left bound subtract TopPointPiece.WIDTH and the right one subtract nothing, if TopPointPiece is anchored top centre? Shouldn't the bounds be symmetrical?

Copy link
Contributor Author

@EVAST9919 EVAST9919 Mar 18, 2024

Choose a reason for hiding this comment

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

The piece is top-centered, however the TimelineControlPointGroup (it's parent) is not, which is what we are accounting for here
flag

Comment on lines 65 to 72
// Remove groups outside the visible range
for (int i = Count - 1; i >= 0; i--)
{
var g = Children[i];

if (!shouldBeVisible(g.Group))
g.Expire();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason to iterate backwards here? It's not like this is removing anything from the list as it's iterating over it? All expiration does is set LifetimeEnd. Actual removal from composites happens in a separate pass elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All expiration does is set LifetimeEnd

Which will make ShouldBeAlive false and since RemoveWhenNotAlive isn't overriden - it will be removed.
... but later in a separate pass, so you are right.

@peppy peppy self-requested a review March 20, 2024 04:49
@peppy peppy merged commit 1f70019 into ppy:master Mar 20, 2024
9 of 11 checks passed
@EVAST9919 EVAST9919 deleted the editor-performance branch March 20, 2024 05:31
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