Add support for grouping by artist to beatmap carousel v2 #31801
Conversation
|
Additionally, when clicking on a group header when beatmap set headers are also active, the selection doesn't actually change to the first beatmap of the first beatmap set of the group, which leads to you getting scrolled away from the group header that you just clicked: 2025-02-05.14-43-34.mp4 |
Yeah I was leaving this to fix the scroll part later, rather than make it select. In stable it doesn't automatically select so I'm leaning towards keeping that behaviour. |
Should I be expect the fix to be in this PR or no? To me this is the bigger blocker than the double iteration discussed above I think. This should behave correctly first and foremost and we can optimise later as required. |
peppy
force-pushed
the
carousel-v2-artist-grouping
branch
from
ba25671 to
6e93790
Compare
I had a bit of a struggle getting header traversal logic to work well. The constraints I had in place were a bit weird: - Group panels should toggle or potentially fall into the prev/next group - Set panels should just traverse around them The current method of using `CheckValidForGroupSelection` return type for traversal did not mesh with the above two cases. Just trust me on this one since it's quite hard to explain in words. After some re-thinking, I've gone with a simpler approach with one important change to UX: Now when group traversing with a beatmap set header currently keyboard focused, the first operation will be to reset keyboard selection to the selected beatmap, rather than traverse. I find this non-offensive – at most it means a user will need to press their group traversal key one extra time. I've also changed group headers to always toggle expansion when doing group traversal with them selected. To make all this work, the meaning of `Activation` has changed somewhat. It is now the primary path for carousel implementations to change selection of an item. It is what the `Drawable` panels call when they are clicked. Selection changes are not performed implicitly by `Carousel` – an implementation should decide when it actually wants to change the selection, usually in `HandleItemActivated`. Having less things mutating `CurrentSelection` is better in my eyes, as we see this variable as only being mutated internally when utmost required (ie the user has requested the change). With this change, `CurrentSelection` can no longer become of a non-`T` type (in the beatmap carousel implementation at least). This might pave a path forward for making `CurrentSelection` typed, but that comes with a few other concerns so I'll look at that as a follow-up.
peppy
force-pushed
the
carousel-v2-artist-grouping
branch
from
6e93790 to
024fbde
Compare
|
Apologies for the size increase of this PR, but it's going to be hard to split out the new logic (and a lot is tests anyway). Please read over 024fbde carefully. |
|
@frenzibyte could you also check the changes to see if they work okay in your eyes? |
| case BeatmapSetInfo set: | ||
| // Case where there are set headers, header should be visible | ||
| // and items should use the set's expanded state. | ||
| i.IsVisible = true; | ||
| setExpansionStateOfSetItems(set, i.IsExpanded); | ||
| break; | ||
|
|
||
| default: | ||
| // Case where there are no set headers, all items should be visible. | ||
| if (!grouping.BeatmapSetsGroupedTogether) | ||
| i.IsVisible = true; | ||
| break; |
There was a problem hiding this comment.
This comment does not mean to block this PR or request changes in any way shape or form, but is basically a general question and fruit for thought.
Seeing this kind of logic really makes me wonder just how bad would it really be if CarouselItem acts as a basic tree model and this logic becomes recursive instead, removing the need for BeatmapSetsGroupedTogether or manual lookup of the containing group in which a beatmap is at.
I can imagine multiple things being tackled with this idea of a tree:
- Removing the newly introduced
CarouselItem.DepthLayerand/or computing it appropriately based on how deep an item is in the hierarchy. - Making the constant X offsets applied in design PR become based on the
CarouselItem's depth in the hierarchy instead. - Allowing
BeatmapCarouselto easily determine whether to choose a small beatmap difficulty panel or a "standalone" beatmap single-diff panel, by simply checking whether the beatmap carousel item is a parent of aBeatmapSetPanelmodel or aGroupDefinition/no parent.
Of course all three points above can be solved by totally different means, but I found a basic tree model approach to be the natural solution, so I may as well raise it up for discussion.
There was a problem hiding this comment.
It's something i've considered and will try in the future. It has potential to optimise things if we hit a brick wall with the current structure.
So far the code complexity with flat list is very manageable and I'm not sure it can be maintained with tree.
There was a problem hiding this comment.
For reference I was party to an irl conversation on this exact question in December and my vague unsubstantiated feeling was also that a flat list was going to be more manageable than a hierarchical model - flat list does make some things more complicated, but also it will make some less so (e.g. I don't really want to think how the management of Y would look position if the backing model was no longer a flat list but hierarchical instead).
So far I see no strong indicators to change my opinion on this.
|
Code-wise this seems okay, but I cannot hold myself from remarking on how little sense the "set/group traversal" operation - as in, left/right arrow - makes to me at this stage. It basically only seems to do the sensible thing I expect it to do when there is no keyboard selection. When there is keyboard selection, many completely different things can happen:
Now that I typed it all up, I guess that makes "traversal" equivalent to... selection/enter? but only specifically when keyboard selection is present? That feels really weird to me. Unless this is all matching weird stable UX at which point I will have to concede to precedent. In addition, the issue mentioned earlier about how the carousel can scroll away from what the user selected still does not seem to be resolved. Should I expect it to be? I didn't get an answer to #31801 (comment). |
|
Fixing the scrolling will be a separate endeavour. Group traversal to me follows a simple rule: in the normal case you are using it to traverse beatmap sets (or difficulties where not grouping by sets). It should traverse disregarding other grouping. When you interrupt it, in the case it's not at a difficulty, it will mostly behave like a "select" press until it reverts to the standard behaviour. This is how stable works, and personally I think it feels good once you are used to it. The only thing i'm neutral on is the changed behaviour when at a set header because this case doesn't exist in stable and is a bit undefined. The resultant behaviour from this PR makes it align with "just being a select press" if you can get behind that. |
One of the final pieces to the puzzle – an example of how groups and sets can interact together.
Note that selection behaviour still needs some thought when for instance opening a group via click then hitting left/right arrows, but this isn't any new breakage so I will address separately.
osu.Game.Tests.2025-02-05.at.10.50.50.mp4