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 minimal viable new screen for daily challenge feature #28440

Merged
merged 10 commits into from
Jun 12, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jun 10, 2024

Continuation of #28136

First of the PRs that originate from my UI experiments detailed in this discord thread.

The goal to this one is to just get a new screen in place that new things can be added to. All components that could possibly have been reused to make work, have been. New components & UI overhauls (if any) shall be PR'd separately. This is large enough as it is due to setup boilerplate required.

1718017973

This screen existing also makes it feasible to hide the daily challenge room from the playlist listing which was previously deemed potentially desirable. This has been done in 073ddce.

(The reason why this requires a new screen to work is that PlaylistsRoomSubScreen has a SelectionPollingComponent that will add the current room to the ambient RoomManager if missing, which is shared with the playlist listing screen. So it was not feasible to filter out the daily challenge room client-side while at the same time reusing the existing playlists screens verbatim.)

@bdach bdach self-assigned this Jun 10, 2024
@peppy peppy self-requested a review June 11, 2024 06:22

if (playlistItem.AllowedMods.Any())
{
footerButtons.Insert(0, new UserModSelectButton
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I'm guessing the goal here was to insert the button to the left of the "Start" button, but this doesn't seem to be working:

2024-06-11 21 37 50@2x

Not sure if this is the best fix, but it works:

diff --git a/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallenge.cs b/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallenge.cs
index bff58dbb58..c0a0d4221b 100644
--- a/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallenge.cs
+++ b/osu.Game/Screens/OnlinePlay/DailyChallenge/DailyChallenge.cs
@@ -257,7 +257,7 @@ [new MatchChatDisplay(room) { RelativeSizeAxes = Axes.Both }]
 
             if (playlistItem.AllowedMods.Any())
             {
-                footerButtons.Insert(0, new UserModSelectButton
+                var userModSelectButton = new UserModSelectButton
                 {
                     Text = "Free mods",
                     Anchor = Anchor.Centre,
@@ -265,7 +265,10 @@ [new MatchChatDisplay(room) { RelativeSizeAxes = Axes.Both }]
                     RelativeSizeAxes = Axes.Y,
                     Size = new Vector2(250, 1),
                     Action = () => userModsSelectOverlay.Show(),
-                });
+                };
+
+                footerButtons.Add(userModSelectButton);
+                footerButtons.SetLayoutPosition(userModSelectButton, -1);
 
                 var rulesetInstance = rulesets.GetRuleset(playlistItem.RulesetID)!.CreateInstance();
                 var allowedMods = playlistItem.AllowedMods.Select(m => m.ToMod(rulesetInstance));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm guessing the goal here was to insert the button to the left of the "Start" button

It was, not sure how I didn't notice that not working. Especially so that I knew about this footgun from before.

Not sure if this is the best fix, but it works:

FlowContainer.Insert() is actually a shorthand for the two operations you have in that patch there. Hence, 5e002fb suffices.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Weird, I tried that exact patch and it crashed my test setup. Probably the arm64 dynamic recompile "Program incorrect format" .NET issue which I just presumed to be me doing something wrong with the Insert call 🤷

@peppy
Copy link
Sponsor Member

peppy commented Jun 11, 2024

The leaderboard doesn't have much room to work with. I'm thinking as a separate PR we can probably reduce the size of elements in the leaderboard items as a temporary fix (they feel way too big for these displays).

@bdach
Copy link
Collaborator Author

bdach commented Jun 12, 2024

The leaderboard doesn't have much room to work with.

For whatever it's worth, it uses the exact same amount of room as it does on the playlists screen.

daily challenge playlists
1718175636 1718175625

Not using that to excuse this, but I guess if we're adjusting we should probably try to adjust both...?

Generally I'm not sure what to do with the leaderboard. It's quite fugly, but we don't have a good replacement for it. The gameplay leaderboard is not a suitable replacement. There's the new song select leaderboard that got merged recently, but the shearing on it is going to make reuse difficult (unless I reuse it and unshear it for this screen specifically).

The other thing design-wise that probably needs replacing but I didn't touch yet is the upscaled playlist item that serves as the "subheader" for this entire screen.

@peppy
Copy link
Sponsor Member

peppy commented Jun 12, 2024

Kinda hoping the shearing can be turned off. I'm 99% sure it suffers the same thing where shearing is applied but there's no horizontal offset, making it looks silly in a list.

@bdach
Copy link
Collaborator Author

bdach commented Jun 12, 2024

I'm 99% sure it suffers the same thing where shearing is applied but there's no horizontal offset, making it looks silly in a list.

The song select design figma has the entire list sheared, so it doesn't look wrong there:

1718176096

which obviously doesn't work for this screen, though.

@peppy
Copy link
Sponsor Member

peppy commented Jun 12, 2024

I do wonder if we can make the shear a setting for the whole panel. Worth a try.

@peppy peppy merged commit 94b7148 into ppy:master Jun 12, 2024
11 of 17 checks passed
@bdach bdach deleted the daily-challenge/new-screen branch June 12, 2024 09:15
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.

None yet

2 participants