Add star rating to the beatmap metadata in player loading screen #12717

frenzibyte · 2021-05-08T08:52:59Z

Depends on Allow changing current star difficulty of a StarRatingDisplay #12747

Went with the simplest way of adding a StarRatingDisplay, as desired in the issue thread, looks harmonious enough.

CleanShot.2021-05-08.at.11.33.54.mp4

(in case it isn't obvious, this is from a test scene, not from game)

Follows the way working beatmap is passed, not sure why mods are passed as a bindable though, don't wanna bother too much with that.

osu.Game/Screens/Play/PlayerLoader.cs

bdach · 2021-05-08T12:37:49Z

the test scene is showing a one-frame flicker when the mods are starting to display.

2021-05-08.14-34-06.mp4

you can slow down to 0.1x or something to see it better.

osu.Game.Tests/Visual/Ranking/TestSceneStarRatingDisplay.cs

osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapMetadataDisplay.cs

osu.Game/Screens/Play/BeatmapMetadataDisplay.cs

bdach · 2021-05-08T12:43:22Z

osu.Game/Screens/Ranking/Expanded/StarRatingDisplay.cs

+            background.Colour = difficulty.DifficultyRating == DifficultyRating.ExpertPlus
+                ? ColourInfo.GradientVertical(Color4Extensions.FromHex("#C1C1C1"), Color4Extensions.FromHex("#595959"))
+                : (ColourInfo)colours.ForDifficultyRating(difficulty.DifficultyRating);


not super sure about the lack of any sort of fade out transition here

to clarify, you mean fading colours of the background?

that, and the text sprites

I've added proper transitioning along with rolling counter for the star rating display, I can imagine cases where the difficulty would need time to calculate before its shown, which would look bad on the player UX if it hides and shows back, moving the entire metadata flow back and forward.

CleanShot.2021-05-08.at.23.10.03.mp4

Not sure on the design of the placeholder though.

I'm not sure why this was added in this PR, but it's weird due to the font being right aligned and not fixed width.

Using fixed-width made it look really weird so I refrained from doing that, and I thought it wasn't that big of a change to split it, but can do.

Well it's affecting a global component and you've updated it in a local usage. There's nothing to say you actually checked how this works in every other usage.

I'm not saying it should be fixed width, but if anything it should not be right aligned and should definitely not affect the size of the parent container to the point it's bouncing around.

I'm not really sure about how to fix this, I've checked fixed-width one more time to record a video of it and the results are indeed not appealing: (especially with 1.23 due to font being right-aligned as you mentioned)

CleanShot.2021-05-10.at.07.37.51.mp4

Also fixing the width of the star rating display to 60px makes it look a bit questionable:

CleanShot.2021-05-10.at.07.49.16.mp4

The StarRatingDisplay in general looks to be designed to not be changeable and make the numbers roll around, not sure how to proceed forward without leaving this as-is (or going with the second option of fixing width to 60px).

Fixed width looks okay I think. If it's decided it doesn't then the numbers should not roll in the first place.

It shouldn't update all the time, only when the difficulty calculation is heavy, so I think it's fine to remove the rolling altogether and call it a day for this design.

Avoids first frame discrepancies from appearing in the test scene, those can be delt with later on, if needed.

frenzibyte · 2021-05-08T20:17:57Z

the test scene is showing a one-frame flicker when the mods are starting to display.

2021-05-08.14-34-06.mp4

you can slow down to 0.1x or something to see it better.

Resolved with initial fade in on test scenes, the underlying issues can be dealt with later on. (if I were to suggest a temporary resolution, the fading container in ModDisplay should have AlwaysPresent = true)

peppy · 2021-05-10T03:31:28Z

osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapMetadataDisplay.cs

+        }
+
+        [Test]
+        public void TestRandomFromDatabase()


really not sure about adding more test scenes which depend on the local beatmaps to function correctly

added it to check how metadata would look on real local beatmaps, and I imagined it to be useful for others (especially when it comes to tweaking the design).

unless you think it's better not to rely on local databases all the time and always go with the option of adding programmatically written beatmaps.

Makes no sense to add a test intended to test visual behaviour with one of the main elements missing. Not sure how you would be able to test the flow with the logo's presence.

peppy · 2021-05-10T03:44:38Z

I've made some preliminary changes to the test scene to make it behave more like how it does within the game. Also added back the logo.

frenzibyte · 2021-05-10T03:48:30Z

FWIW I've added the ruleset steps to be able to see the difficulty changing in the beatmap metadata display, simulates the case where star difficulty for a certain beatmap may take some time before the calculation finishes.

But it's a small benefit and can only be useful in the TestRandomFromDatabase test case, so no problem in removing it.

peppy · 2021-05-10T03:50:57Z

Make a test specifically for the star difficulty display, in the same PR where you split that functionality out.

Also make sure it's done in a way it doesn't cause the test scene to display nothing on first load. You probably want one [Test] per ruleset or something like that.

frenzibyte · 2021-05-10T07:34:30Z

Just to be extra clear, can you elaborate more on that? do you mean writing a new test scene that hooks up a star rating display to a bindable difficulty from the cache, on a written beatmap, to ensure it works?

bdach · 2021-05-10T20:41:35Z

simulates the case where star difficulty for a certain beatmap may take some time before the calculation finishes

That's the same thing as not setting difficulty initially and then setting it later after a delay, right? In that case, why not just skip interfacing with any beatmaps or difficulty caches whatsoever - or am I misunderstanding the intent?

frenzibyte · 2021-05-10T21:06:20Z

That is right, but I don't want to hack around with the BeatmapMetadataDisplay to allow the tests to change/set the star difficulty when they want (currently the display gets them directly from the cache, and nothing else), but I could make that happen with an altered difficulty cache that waits for TaskCompletionSource, probably the easiest solution.

peppy · 2021-05-11T01:38:35Z

It's fine as is, but this PR still needs the removal of the animated counter.

peppy

seems fine for now

Shouldn't guard against that here.

frenzibyte added 3 commits May 8, 2021 11:47

Add star rating display underneath the beatmap metadata

0f08c2a

Allow null logo facade

7b7e7a8

Add visual test scene

169a283

frenzibyte added type:cosmetic type:behavioural labels May 8, 2021

pull-request-size bot added the size/L label May 8, 2021

Pass ruleset info to constructor instead

b4801fa

Follows the way working beatmap is passed, not sure why mods are passed as a bindable though, don't wanna bother too much with that.

Game4all reviewed May 8, 2021

View reviewed changes

osu.Game/Screens/Play/PlayerLoader.cs Outdated Show resolved Hide resolved

frenzibyte added 4 commits May 8, 2021 12:55

Refactor StarRatingDisplay to be mutable with a current bindable

0410ede

Use BeatmapDifficultyCache.GetBindableDifficulty(...) instead

4309006

Remove no longer necessary ruleset info requirement

dca5efc

Fix test not handling 0 beatmap sets

26c0010

bdach removed the type:behavioural label May 8, 2021

bdach requested changes May 8, 2021

View reviewed changes

frenzibyte added 8 commits May 8, 2021 18:18

Remove test scene description

d9605e8

Use regular test steps rather than one-time set up and scheduling

3575d98

Remove nullable facade logic

a75347c

Pass empty facade and replace random property with method instead

ca55287

Remove pattern-matching on nullable with simple .HasValue/.Value

655e8d3

Apply further refactoring to star rating display UX-wise

c52f173

Add initial fade in to the metadata display

f701c33

Avoids first frame discrepancies from appearing in the test scene, those can be delt with later on, if needed.

Allow changing ruleset during test

8fba655

peppy self-requested a review May 10, 2021 03:25

peppy reviewed May 10, 2021

View reviewed changes

peppy added 4 commits May 10, 2021 12:34

Tidy up test scene

0c973fe

Remove unnecessary ruleset switching steps

2b90bc4

Make test work without manually clicking things

b7acf9d

Add the osu! logo to the test scene

9ba412d

Makes no sense to add a test intended to test visual behaviour with one of the main elements missing. Not sure how you would be able to test the flow with the logo's presence.

Merge branch 'master' into player-loader-star-rating

52ce16f

frenzibyte mentioned this pull request May 11, 2021

Allow changing current star difficulty of a StarRatingDisplay #12747

Merged

frenzibyte and others added 7 commits May 11, 2021 09:53

Merge branch 'current-star-rating' into player-loader-star-rating

004ce95

Make BeatmapDifficultyCache.GetDifficultyAsync virtual

e0728a6

Add test beatmap difficulty cache with calc. blocking support

db361ef

Apply simple transforms to star rating display when ready if not

0dc3bfd

Merge branch 'current-star-rating' into player-loader-star-rating

93007c1

Update usage due to nullability removal

7c2fc9b

Merge branch 'master' into player-loader-star-rating

dc56250

peppy previously approved these changes May 15, 2021

View reviewed changes

Remove null conditional

34d1490

Shouldn't guard against that here.

frenzibyte dismissed peppy’s stale review via 34d1490 May 15, 2021 07:00

peppy approved these changes May 15, 2021

View reviewed changes

peppy merged commit 50a3775 into ppy:master May 15, 2021

frenzibyte deleted the player-loader-star-rating branch May 15, 2021 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add star rating to the beatmap metadata in player loading screen #12717

Add star rating to the beatmap metadata in player loading screen #12717

frenzibyte commented May 8, 2021 •

edited by peppy

Loading

bdach commented May 8, 2021

bdach May 8, 2021

frenzibyte May 8, 2021

bdach May 8, 2021

frenzibyte May 8, 2021 •

edited

Loading

peppy May 10, 2021

frenzibyte May 10, 2021

peppy May 10, 2021

frenzibyte May 10, 2021

peppy May 10, 2021

frenzibyte May 10, 2021

frenzibyte commented May 8, 2021 •

edited

Loading

peppy May 10, 2021

frenzibyte May 10, 2021

peppy commented May 10, 2021

frenzibyte commented May 10, 2021

peppy commented May 10, 2021 •

edited

Loading

frenzibyte commented May 10, 2021

bdach commented May 10, 2021

frenzibyte commented May 10, 2021 •

edited

Loading

peppy commented May 11, 2021

peppy left a comment

Add star rating to the beatmap metadata in player loading screen #12717

Add star rating to the beatmap metadata in player loading screen #12717

Conversation

frenzibyte commented May 8, 2021 • edited by peppy Loading

bdach commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte May 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frenzibyte commented May 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peppy commented May 10, 2021

frenzibyte commented May 10, 2021

peppy commented May 10, 2021 • edited Loading

frenzibyte commented May 10, 2021

bdach commented May 10, 2021

frenzibyte commented May 10, 2021 • edited Loading

peppy commented May 11, 2021

peppy left a comment

Choose a reason for hiding this comment

frenzibyte commented May 8, 2021 •

edited by peppy

Loading

frenzibyte May 8, 2021 •

edited

Loading

frenzibyte commented May 8, 2021 •

edited

Loading

peppy commented May 10, 2021 •

edited

Loading

frenzibyte commented May 10, 2021 •

edited

Loading