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 selection support to beatmap carousel v2 #31634

Merged
merged 13 commits into from
Jan 24, 2025

Conversation

peppy
Copy link
Member

@peppy peppy commented Jan 23, 2025

This aims to replicate how stable handles selection. It therefore has different behaviour to lazer – most importantly, keyboard selection is now a separate concept (up and down arrows) from actual carousel selection.

  • Pressing the select binding (enter) will propagate keyboard selection to an actual selection.
  • Using group selection keys (left and right arrows) immediately updates the current selection and keyboard selection. If keyboard selection doesn't match current selection, it will first propagate keyboard selection as above.

Personally I think this feels better, and no one has ever complained about it on stable. I think there will be some lazer users which are accustomed to what we have on master, but I suspect they will get used to the new selection style quite fast.

osu.Game.Tests.2025-01-23.at.09.33.17.mp4

Important concepts in this change:

Add set-difficulty tracking in BeatmapCarouselFilterGrouping

Rather than tracking inside individual items, let's just maintain a single dictionary which is refreshed every time we regenerate filters.

Make CarouselItem sealed and remove BeatmapCarouselItem concept

Less abstraction is better. As far as I can tell, we don't need a custom model for this. If there's any tracking to be done, it should be done within BeatmapCarousel's implementation (or a filter).

Move Selected status to drawables

Basically, I don't want bindables in CarouselItem. It means there needs to be a bind-unbind process on pooling. By moving these to the drawable and just updating every frame, we can simplify things a lot.

Performance

I've been testing with 200k beatmapsets to ensure performance is amicable. Changing selection on a release build is ~2 ms of overhead, due to updating Y positions of all panels (it is assumed that visbility of panels changes when a selection occurs for simplicity).

This is already better than master (well over 20 ms under the same conditions) and can be improved in the future. I've stopped myself from optimising further until we are feature complete. I suspect that adding the top level groupings that users are waiting on may break any optimisations I add at this point.

Tests

I've added back most of the selection related tests I could find in TestSceneBeatmapCarousel. There's probably more than could be added, but I don't want to go too crazy until things like recommendations and groupings are added, as the asserts will likely require updates as the behaviours are built upon.

peppy added 7 commits January 23, 2025 18:48
Basically, I don't want bindables in `CarouselItem`. It means there
needs to be a bind-unbind process on pooling. By moving these to the
drawable and just updating every frame, we can simplify things a lot.
Less abstraction is better. As far as I can tell, we don't need a custom
model for this. If there's any tracking to be done, it should be done
within `BeatmapCarousel`'s implementation (or a filter).
Rather than tracking inside individual items, let's just maintain a
single dictionary which is refreshed every time we regenerate filters.
Where possible I've tried to match the test and method names of
`TestSceneBeatmapCarousel` for easy coverage comparison.
@peppy peppy force-pushed the beatmap-carousel-v2-selection branch from f2ba5b0 to eaea053 Compare January 23, 2025 09:51
@bdach bdach self-requested a review January 23, 2025 10:16
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.

Initial pass. Having trouble following some of this.

@peppy
Copy link
Member Author

peppy commented Jan 23, 2025

I've applied requested changes, and also added a couple more cleanup commits which should be easy to accept without much brainpower.

@bdach bdach self-requested a review January 24, 2025 09:40
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.

Seems ok, let's keep rolling

@bdach bdach merged commit 2d46da1 into ppy:master Jan 24, 2025
10 checks passed
@peppy peppy deleted the beatmap-carousel-v2-selection branch January 28, 2025 13:09
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