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

Add invalidation on draw size change in beatmap carousel v2 #31817

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

peppy
Copy link
Member

@peppy peppy commented Feb 6, 2025

Matching old implementation. Required because DrawHeight (and therefore visibleHalfHeight) changes on aspect ratio change.

Before:

osu.Game.Tests.2025-02-06.at.08.49.37.mp4

After:

osu.Game.Tests.2025-02-06.at.08.48.58.mp4

@@ -678,6 +679,15 @@ private static void expirePanelImmediately(Drawable panel)
carouselPanel.Expanded.Value = false;
}

protected override bool OnInvalidate(Invalidation invalidation, InvalidationSource source)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you've used this instead of a LayoutValue? Primary concern is you're not returning true here with means the drawnode MAY not get invalidated if the stars line up correctly and the base call doesn't do it for you, which is an easy thing to miss by using this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, it's copy-pasted from old carousel. can try using the newer structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a non blocker anyway, was more interested than anything.

@smoogipoo smoogipoo merged commit 736fe78 into ppy:master Feb 6, 2025
8 of 9 checks passed
@peppy peppy deleted the carousel-v2-invalidation branch February 10, 2025 04:08
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.

2 participants