Implement new components for song select wedge redesign #28063
Conversation
Tests are mostly the same as `TestSceneAdvancedStats`.
I wanna stop reading already at this point honestly. Do you realise what reviewing of a 1.7k line diffstat feels like? Do you realise the bikeshed fiasco you're opening yourself up here for by getting this entire code blob stunlocked on minute design changes? I dunno, this may be "less work" for you, but it is more work for me / other reviewers, and I see there being a 95% chance of this getting stuck in review hell again. This is especially important for me because I would like to pay very close attention to how individual components are structured to avoid similar fiascos that plague the current song select and make implementing stuff like correct beatmap invalidation basically infeasible. Please take a look at how I replaced the mod overlay for this. It was not a giant 5k line blob. |
|
Maybe I'm biased by wanting to make quick forward progress, but even with this many lines diff, if it's mostly drawable code and hardly any modified lines (all new lines) I'm willing to take it on – at least an initial pass to see how things read and feel. Will loop back after doing a pass. |
|
Sure if you want to try and stomach it I guess. But I will want to have a look too before merge even if you end up being okay with it. An important code design constraint is that every drawable component in here must support the beatmap changing under it for the invalidation flow. Preferably with a way to force a refresh manually too. If that is not the case, then I'm gonna have a problem with accepting this. |
|
@Joehuu what is the significance of the diff in the OP? It doesn't look to cleanly apply. |
|
@arflyte how much of this change is going to be thrown away in the "next" iteration? |
|
@Joehuu is this ready for a full code review and/or should it be taken out of draft? |
|
This is ready for review, yes. Forgot to take this out of draft. |
osu.Game.Tests/Visual/SongSelectV2/SongSelectComponentsTestScene.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace osu.Game.Screens.Select | ||
| { | ||
| public partial class DifficultySettingsContent : FillFlowContainer, IHasCustomTooltip<AdjustedAttributesTooltip.Data> |
There was a problem hiding this comment.
Settings is a weird term to use here? I think you're looking for Attributes.
There was a problem hiding this comment.
If you look at https://osu.ppy.sh/wiki/en/Beatmap/Circle_size and the rest of the wiki, there is no mention of "Difficulty Attributes". Theres also https://github.com/ppy/osu/blob/f2436a5ecb96b7e4aa9a38d76caacc5e4687d879/osu.Game/Rulesets/Difficulty/DifficultyAttributes.cs.
| public const float WEDGE_CORNER_RADIUS = 10; | ||
| public const float SHEAR_X = 0.175f; | ||
| public static readonly Vector2 WEDGED_CONTAINER_SHEAR = new Vector2(SHEAR_X, 0); |
There was a problem hiding this comment.
I would probably just place this in BeatmapInfoWedgeV2, or at the very least in SongSelectV2 if they really need to be in the main screen class.
| { | ||
| InternalChildren = new Drawable[] | ||
| { | ||
| // TODO: add metadata column |
There was a problem hiding this comment.
I suppose this will come in a follow-up PR? Probably a better idea since it's just additional details that don't necessarily have to exist in this diff.
There was a problem hiding this comment.
Yeah, I initially had the details tab implemented in the diff but it had too many LOC. Also the design was cluttered with it / gonna be redone anyway.
| // the cached ruleset bindable might be a decoupled bindable provided by SongSelect, | ||
| // which we can't rely on in combination with the game-wide selected mods list, | ||
| // since mods could be updated to the new ruleset instances while the decoupled bindable is held behind, | ||
| // therefore resulting in performing difficulty calculation with invalid states. | ||
| gameRuleset = game.Ruleset.GetBoundCopy(); |
There was a problem hiding this comment.
As far as I'm concerned, the goal of the song select redesign task isn't only to update the visuals of the existing song select screen, but rewrite the entirety of the screen into a state where workarounds like these don't have to exist (since the current code in SongSelect is too convoluted/fragile to touch). cc @peppy to confirm on this direction.
Bindable decoupling is not implemented in SongSelectV2 yet, so I would suggest at least commenting workarounds like these until a point where we have SongSelectV2 in a finalised state and revisit if these workarounds are still necessary.
There was a problem hiding this comment.
That was copied verbatim in AdvancedStats but was removed 5c049fe#diff-7537e2f2818506dce8cc62f117ec907f9cb69ca88513b5224cc524474447e654. Will see if this workaround is needed and will try to make extensive testing on rulesets whenever I get the chance next time.
osu.Game.Tests/Visual/SongSelectV2/SongSelectComponentsTestScene.cs
Outdated
Show resolved
Hide resolved
| switch (beatmapInfo.Value) | ||
| { | ||
| case BeatmapInfo: | ||
| // TODO: consider mods which apply variable rates. | ||
| double rate = 1; | ||
| foreach (var mod in mods.Value.OfType<IApplicableToRate>()) | ||
| rate = mod.ApplyToRate(0, rate); | ||
|
|
||
| var beatmap = workingBeatmap.Value.Beatmap; |
There was a problem hiding this comment.
I don't really know about this flow. Having this method being triggered by a change in beatmapInfo but then read from workingBeatmap inside the callback sounds like a recipe for disaster to me.
Reiterating what I mentioned in the flow presented by TestSceneSongSelectComponents, for UI components that are meant to be shared between song select and beatmap info overlay, let them accept the direct values that they require, and move this code to a general component that's tied directly to song select (i.e. BeatmapInfoWedgeV2).
This also means removing the APIBeatmap handling entirely. They will be handled in the new beatmap info overlay efforts.
| Child = starCounter = new StarCounter | ||
| RelativeSizeAxes = Axes.X, | ||
| Height = WEDGE_HEIGHT, | ||
| Shear = SongSelect.WEDGED_CONTAINER_SHEAR, |
| new InfoWedgeBackground | ||
| { | ||
| Child = new BasicBeatmapInfoContent | ||
| { | ||
| Padding = new MarginPadding | ||
| { | ||
| Left = text_margin, | ||
| Right = 20, | ||
| Vertical = 10 | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
diff --git a/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs b/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs
index fd3b6e73cc..88690ecc7f 100644
--- a/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs
+++ b/osu.Game/Screens/Select/BeatmapInfoWedgeV2.cs
@@ -165,6 +165,12 @@ private void load()
},
new InfoWedgeBackground
{
+ Padding = new MarginPadding
+ {
+ Top = 10,
+ Left = -WEDGE_CORNER_RADIUS,
+ Right = SHEAR_WIDTH + COLOUR_BAR_WIDTH
+ },
Child = new BasicBeatmapInfoContent
{
Padding = new MarginPadding
@@ -177,6 +183,12 @@ private void load()
},
new InfoWedgeBackground
{
+ Padding = new MarginPadding
+ {
+ Top = 5,
+ Left = -WEDGE_CORNER_RADIUS,
+ Right = SHEAR_WIDTH + COLOUR_BAR_WIDTH + 8f,
+ },
Child = new ExtendedBeatmapInfoContent
{
Padding = new MarginPadding
diff --git a/osu.Game/Screens/Select/InfoWedgeBackground.cs b/osu.Game/Screens/Select/InfoWedgeBackground.cs
index 6f505f99b4..8af0c8f9da 100644
--- a/osu.Game/Screens/Select/InfoWedgeBackground.cs
+++ b/osu.Game/Screens/Select/InfoWedgeBackground.cs
@@ -23,15 +23,6 @@ public InfoWedgeBackground()
{
RelativeSizeAxes = Axes.X;
AutoSizeAxes = Axes.Y;
-
- Padding = new MarginPadding
- {
- Top = 10,
- Left = -SongSelect.WEDGE_CORNER_RADIUS,
-
- // TODO: should account top wedge's shear width for alignment (hard to do as this auto-sizes height right now)
- Right = BeatmapInfoWedgeV2.SHEAR_WIDTH + BeatmapInfoWedgeV2.COLOUR_BAR_WIDTH
- };
}
[BackgroundDependencyLoader]Pushes the second InfoWedgeBackground further to the left for overall alignment, and also decreases the spacing between the two wedges to match design.
There was a problem hiding this comment.
I had this hardcoded like you do (the 8, but different), but I guess it's fine as it should have a fixed height.
|
|
||
| namespace osu.Game.Screens.Select | ||
| { | ||
| public partial class LengthAndBPMStatisticPill : CompositeDrawable |
There was a problem hiding this comment.
In response to the last point in #28063 (comment), I would like to know how everyone feels about having this instead:
CleanShot.2024-07-02.at.09.22.00.mp4
|
@Joehuu Are you able to look into the points raised above? |
|
Will get a chance today. |
|
For reference what's the blocking part here? |
|
This being too big to review, and the bindable flow would need to be rewritten like the recent PR. This merely serves as a reference for me now, so closing. |
Where's the individual component PRs?
Considered PRing individual components (and replacing old ones) more effort because:
My original proposal for the process was just implementing the component and not replacing the old one, but also may be harder to review as it'll just be a single component (content) in a test with no context.
The goal of this PR is preparation for just replacing the old wedge. It does the bare minimum: no skinning (e.g. collapsing), no new density graph, old details tab w/ metadata and online stuff is still a thing (newest design iteration may not have it displayed by default). Also had beatmap info overlay / online in mind when I did this long before, so there are
APIBeatmapcases with some testing.Code follows the
Contentterminology like results screen components when the component contains text and has no background. The LOC is probably daunting, but If you don't count the tests / header and usings, it is probably under 1000 with some reusing of old components' code.The interim design is following the very first figma iteration for the difficulty name / length and bpm content, and for object count / difficulty settings content, it follows the current layout but has three columns because the left side expanded. The header is not implemented as I believe that takes too much height room (it's still in multi though). Also ignores the latest figma design linked above that removes bars entirely.
The old-to-new wedge replacement diff is below. Didn't apply it for now to ease LOC.
There are some TODOs, and I wrote this after midnight, so opening as draft for general feedback for now:
TODOs (some todos in code not listed):