From 5cc2721e9afaaf5476df4024cbd9b1e069449062 Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Wed, 21 Sep 2022 14:42:02 +0900
Subject: [PATCH 1/4] Add failing test showing layout failure in gameplay
leaderboard
---
.../Gameplay/TestSceneGameplayLeaderboard.cs | 74 +++++++++++++++----
1 file changed, 61 insertions(+), 13 deletions(-)
diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayLeaderboard.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayLeaderboard.cs
index 663e398c01..72656c29b1 100644
--- a/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayLeaderboard.cs
+++ b/osu.Game.Tests/Visual/Gameplay/TestSceneGameplayLeaderboard.cs
@@ -6,7 +6,9 @@
using System.Linq;
using NUnit.Framework;
using osu.Framework.Bindables;
+using osu.Framework.Extensions.PolygonExtensions;
using osu.Framework.Graphics;
+using osu.Framework.Graphics.Containers;
using osu.Framework.Testing;
using osu.Framework.Utils;
using osu.Game.Online.API.Requests.Responses;
@@ -18,37 +20,52 @@ namespace osu.Game.Tests.Visual.Gameplay
[TestFixture]
public class TestSceneGameplayLeaderboard : OsuTestScene
{
- private readonly TestGameplayLeaderboard leaderboard;
+ private TestGameplayLeaderboard leaderboard;
private readonly BindableDouble playerScore = new BindableDouble();
public TestSceneGameplayLeaderboard()
{
- Add(leaderboard = new TestGameplayLeaderboard
+ AddStep("toggle expanded", () =>
{
- Anchor = Anchor.Centre,
- Origin = Anchor.Centre,
- Scale = new Vector2(2),
+ if (leaderboard != null)
+ leaderboard.Expanded.Value = !leaderboard.Expanded.Value;
});
+
+ AddSliderStep("set player score", 50, 5000000, 1222333, v => playerScore.Value = v);
}
- [SetUpSteps]
- public void SetUpSteps()
+ [Test]
+ public void TestLayoutWithManyScores()
{
- AddStep("reset leaderboard", () =>
+ createLeaderboard();
+
+ AddStep("add many scores in one go", () =>
{
- leaderboard.Clear();
- playerScore.Value = 1222333;
+ for (int i = 0; i < 32; i++)
+ createRandomScore(new APIUser { Username = $"Player {i + 1}" });
+
+ // Add player at end to force an animation down the whole list.
+ playerScore.Value = 0;
+ createLeaderboardScore(playerScore, new APIUser { Username = "You", Id = 3 }, true);
});
- AddStep("add local player", () => createLeaderboardScore(playerScore, new APIUser { Username = "You", Id = 3 }, true));
- AddStep("toggle expanded", () => leaderboard.Expanded.Value = !leaderboard.Expanded.Value);
- AddSliderStep("set player score", 50, 5000000, 1222333, v => playerScore.Value = v);
+ // Gameplay leaderboard has custom scroll logic, which when coupled with LayoutDuration
+ // has caused layout to not work in the past.
+
+ AddUntilStep("wait for fill flow layout",
+ () => leaderboard.ChildrenOfType<FillFlowContainer<GameplayLeaderboardScore>>().First().ScreenSpaceDrawQuad.Intersects(leaderboard.ScreenSpaceDrawQuad));
+
+ AddUntilStep("wait for some scores not masked away",
+ () => leaderboard.ChildrenOfType<GameplayLeaderboardScore>().Any(s => leaderboard.ScreenSpaceDrawQuad.Contains(s.ScreenSpaceDrawQuad.Centre)));
}
[Test]
public void TestPlayerScore()
{
+ createLeaderboard();
+ addLocalPlayer();
+
var player2Score = new BindableDouble(1234567);
var player3Score = new BindableDouble(1111111);
@@ -73,6 +90,9 @@ namespace osu.Game.Tests.Visual.Gameplay
[Test]
public void TestRandomScores()
{
+ createLeaderboard();
+ addLocalPlayer();
+
int playerNumber = 1;
AddRepeatStep("add player with random score", () => createRandomScore(new APIUser { Username = $"Player {playerNumber++}" }), 10);
}
@@ -80,6 +100,9 @@ namespace osu.Game.Tests.Visual.Gameplay
[Test]
public void TestExistingUsers()
{
+ createLeaderboard();
+ addLocalPlayer();
+
AddStep("add peppy", () => createRandomScore(new APIUser { Username = "peppy", Id = 2 }));
AddStep("add smoogipoo", () => createRandomScore(new APIUser { Username = "smoogipoo", Id = 1040328 }));
AddStep("add flyte", () => createRandomScore(new APIUser { Username = "flyte", Id = 3103765 }));
@@ -89,6 +112,9 @@ namespace osu.Game.Tests.Visual.Gameplay
[Test]
public void TestMaxHeight()
{
+ createLeaderboard();
+ addLocalPlayer();
+
int playerNumber = 1;
AddRepeatStep("add 3 other players", () => createRandomScore(new APIUser { Username = $"Player {playerNumber++}" }), 3);
checkHeight(4);
@@ -103,6 +129,28 @@ namespace osu.Game.Tests.Visual.Gameplay
=> AddAssert($"leaderboard height is {panelCount} panels high", () => leaderboard.DrawHeight == (GameplayLeaderboardScore.PANEL_HEIGHT + leaderboard.Spacing) * panelCount);
}
+ private void addLocalPlayer()
+ {
+ AddStep("add local player", () =>
+ {
+ playerScore.Value = 1222333;
+ createLeaderboardScore(playerScore, new APIUser { Username = "You", Id = 3 }, true);
+ });
+ }
+
+ private void createLeaderboard()
+ {
+ AddStep("create leaderboard", () =>
+ {
+ Child = leaderboard = new TestGameplayLeaderboard
+ {
+ Anchor = Anchor.Centre,
+ Origin = Anchor.Centre,
+ Scale = new Vector2(2),
+ };
+ });
+ }
+
private void createRandomScore(APIUser user) => createLeaderboardScore(new BindableDouble(RNG.Next(0, 5_000_000)), user);
private void createLeaderboardScore(BindableDouble score, APIUser user, bool isTracked = false)
From 087ca59ebb0362a19202704cd4e94141839d476d Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Wed, 21 Sep 2022 14:48:53 +0900
Subject: [PATCH 2/4] Add extra margin space to flow equal to height of
leaderboard
This ensures the content is always on screen, but also accounts for the
fact that scroll operations without animation were actually forcing the
local score to a location it can't usually reside at.
Basically, the local score was in the scroll extension region (due to always trying
to scroll the local player to the middle of the display, but there being
no other content below the local player to scroll up by).
---
osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs | 1 +
1 file changed, 1 insertion(+)
diff --git a/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs b/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
index f21ce5e36a..62c8788396 100644
--- a/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
+++ b/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
@@ -96,6 +96,7 @@ namespace osu.Game.Screens.Play.HUD
int displayCount = Math.Min(Flow.Count, maxPanels);
Height = displayCount * (GameplayLeaderboardScore.PANEL_HEIGHT + Flow.Spacing.Y);
+ Flow.Margin = new MarginPadding { Bottom = Height };
requiresScroll = displayCount != Flow.Count;
return drawable;
From b04871f40a0060ac0474f5fc5cbcd416d5127ed2 Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Wed, 21 Sep 2022 14:50:23 +0900
Subject: [PATCH 3/4] Animate scroll for a better visual experience
---
osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs b/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
index 62c8788396..c4d81e7e2a 100644
--- a/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
+++ b/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
@@ -119,7 +119,7 @@ namespace osu.Game.Screens.Play.HUD
if (requiresScroll && trackedScore != null)
{
float scrollTarget = scroll.GetChildPosInContent(trackedScore) + trackedScore.DrawHeight / 2 - scroll.DrawHeight / 2;
- scroll.ScrollTo(scrollTarget, false);
+ scroll.ScrollTo(scrollTarget);
}
const float panel_height = GameplayLeaderboardScore.PANEL_HEIGHT;
From 8f7a306d81a721878c7be093e17445e57f4ed3de Mon Sep 17 00:00:00 2001
From: Dean Herbert <pe@ppy.sh>
Date: Thu, 22 Sep 2022 19:53:16 +0900
Subject: [PATCH 4/4] Inline comment regarding margin necessity
---
osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs b/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
index c4d81e7e2a..05a04c3bae 100644
--- a/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
+++ b/osu.Game/Screens/Play/HUD/GameplayLeaderboard.cs
@@ -96,6 +96,13 @@ namespace osu.Game.Screens.Play.HUD
int displayCount = Math.Min(Flow.Count, maxPanels);
Height = displayCount * (GameplayLeaderboardScore.PANEL_HEIGHT + Flow.Spacing.Y);
+ // Add extra margin space to flow equal to height of leaderboard.
+ // This ensures the content is always on screen, but also accounts for the fact that scroll operations
+ // without animation were actually forcing the local score to a location it can't usually reside at.
+ //
+ // Basically, the local score was in the scroll extension region (due to always trying to scroll the
+ // local player to the middle of the display, but there being no other content below the local player
+ // to scroll up by).
Flow.Margin = new MarginPadding { Bottom = Height };
requiresScroll = displayCount != Flow.Count;