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

Only draw path visualiser when hovered or single slider is selected #20965

Merged
merged 6 commits into from
Oct 28, 2022

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Oct 27, 2022

This matches stable editor behaviour.

Now, slider path visualisations will only be shown when

  • A single slider is selected
  • A slider in a multi-selection is hovered
osu.Game.Tests.2022-10-27.at.05.54.22.mp4

This improves performance when selecting all objects by around 25% (on Disco Prince, update 44 / draw 22, up from update 33 / draw 18). Nothing too big but it's a starting point.

@@ -100,6 +105,8 @@ public override bool HandleQuickDeletion()
return true;
}

private bool hasSingleObjectSelected => editorBeatmap == null || selectedObjects.Count == 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did this change come to be? hasSingleObjectSelected being true if editorBeatmap is null blows my mind a bit. If the purpose of this property has changed somehow, can we at least rename it to read less WTF?

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.

It's a bit of an edge case to make tests work.. I just added it to guarantee existing behaviour didn't change, but may be worth removing and fixing any tests themselves to match (if any actually break).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds like there's a nonzero chance that addressing the other review comment I left may render this change unnecessary. Probably worth a go.

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.

Seems that's the case, I've removed the weird null check here now.

@smoogipoo smoogipoo merged commit 74f3b9b into ppy:master Oct 28, 2022
@peppy peppy deleted the reduce-slider-blueprint-overhead branch November 2, 2022 02:50
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

3 participants