-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
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.
f2ba5b0
to
eaea053
Compare
There was a problem hiding this 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.
I've applied requested changes, and also added a couple more cleanup commits which should be easy to accept without much brainpower. |
There was a problem hiding this 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
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.
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 removeBeatmapCarouselItem
conceptLess 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 drawablesBasically, 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.