From 1b9d54a6ad4bd555a965b769d7b7863c625d1509 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 16 Apr 2018 17:39:03 +0900 Subject: [PATCH 1/5] Fix various data races causing crashes or incorrect leaderboard states --- .../Select/Leaderboards/Leaderboard.cs | 79 +++++++++++-------- 1 file changed, 46 insertions(+), 33 deletions(-) diff --git a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs index 3a6ab8f84b..a6bbbce9b9 100644 --- a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs @@ -39,8 +39,9 @@ namespace osu.Game.Screens.Select.Leaderboards private readonly LoadingAnimation loading; - private IEnumerable scores; + private ScheduledDelegate showScoresDelegate; + private IEnumerable scores; public IEnumerable Scores { get { return scores; } @@ -59,29 +60,34 @@ namespace osu.Game.Screens.Select.Leaderboards // ensure placeholder is hidden when displaying scores PlaceholderState = PlaceholderState.Successful; - // schedule because we may not be loaded yet (LoadComponentAsync complains). - Schedule(() => + var flow = scrollFlow = new FillFlowContainer { - LoadComponentAsync(new FillFlowContainer - { - RelativeSizeAxes = Axes.X, - AutoSizeAxes = Axes.Y, - Spacing = new Vector2(0f, 5f), - Padding = new MarginPadding { Top = 10, Bottom = 5 }, - ChildrenEnumerable = scores.Select((s, index) => new LeaderboardScore(s, index + 1) { Action = () => ScoreSelected?.Invoke(s) }) - }, f => - { - scrollContainer.Add(scrollFlow = f); + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y, + Spacing = new Vector2(0f, 5f), + Padding = new MarginPadding { Top = 10, Bottom = 5 }, + ChildrenEnumerable = scores.Select((s, index) => new LeaderboardScore(s, index + 1) { Action = () => ScoreSelected?.Invoke(s) }) + }; - int i = 0; - foreach (var s in f.Children) - { - using (s.BeginDelayedSequence(i++ * 50, true)) - s.Show(); - } + // schedule because we may not be loaded yet (LoadComponentAsync complains). + showScoresDelegate?.Cancel(); + if (!IsLoaded) + showScoresDelegate = Schedule(showScores); + else + showScores(); - scrollContainer.ScrollTo(0f, false); - }); + void showScores() => LoadComponentAsync(flow, _ => + { + scrollContainer.Add(flow); + + int i = 0; + foreach (var s in flow.Children) + { + using (s.BeginDelayedSequence(i++ * 50, true)) + s.Show(); + } + + scrollContainer.ScrollTo(0f, false); }); } } @@ -249,26 +255,33 @@ namespace osu.Game.Screens.Select.Leaderboards PlaceholderState = PlaceholderState.Retrieving; loading.Show(); + var localBeatmap = Beatmap; + getScoresRequest = new GetScoresRequest(Beatmap, osuGame?.Ruleset.Value ?? Beatmap.Ruleset, Scope); - getScoresRequest.Success += r => + getScoresRequest.Success += r => Schedule(() => { + if (localBeatmap != Beatmap) + return; + Scores = r.Scores; PlaceholderState = Scores.Any() ? PlaceholderState.Successful : PlaceholderState.NoScores; - }; - getScoresRequest.Failure += onUpdateFailed; + }); + + getScoresRequest.Failure += e => Schedule(() => + { + if (localBeatmap != Beatmap) + return; + + if (e is OperationCanceledException) + return; + + PlaceholderState = PlaceholderState.NetworkFailure; + Logger.Error(e, @"Couldn't fetch beatmap scores!"); + }); api.Queue(getScoresRequest); } - private void onUpdateFailed(Exception e) - { - if (e is OperationCanceledException) - return; - - PlaceholderState = PlaceholderState.NetworkFailure; - Logger.Error(e, @"Couldn't fetch beatmap scores!"); - } - private void replacePlaceholder(Placeholder placeholder) { var existingPlaceholder = placeholderContainer.Children.LastOrDefault() as Placeholder; From b9220a1e293c9eef775d6606e398d012c50180dc Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 16 Apr 2018 17:39:55 +0900 Subject: [PATCH 2/5] Fix leaderboard placeholder sometimes disappearing indefinitely --- .../Screens/Select/Leaderboards/Leaderboard.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs index a6bbbce9b9..4d8d43f60a 100644 --- a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs @@ -282,24 +282,29 @@ namespace osu.Game.Screens.Select.Leaderboards api.Queue(getScoresRequest); } + private Placeholder currentPlaceholder; + private void replacePlaceholder(Placeholder placeholder) { - var existingPlaceholder = placeholderContainer.Children.LastOrDefault() as Placeholder; - - if (placeholder != null && placeholder.Equals(existingPlaceholder)) + if (placeholder != null && placeholder.Equals(currentPlaceholder)) return; - existingPlaceholder?.FadeOut(150, Easing.OutQuint).Expire(); + currentPlaceholder?.FadeOut(150, Easing.OutQuint).Expire(); if (placeholder == null) + { + currentPlaceholder = null; return; + } Scores = null; - placeholderContainer.Add(placeholder); + placeholderContainer.Child = placeholder; placeholder.ScaleTo(0.8f).Then().ScaleTo(1, fade_duration * 3, Easing.OutQuint); placeholder.FadeInFromZero(fade_duration, Easing.OutQuint); + + currentPlaceholder = placeholder; } protected override void Update() From 9af6ef18646d8700ab4be15a6748c8a6f9762c47 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 16 Apr 2018 17:48:49 +0900 Subject: [PATCH 3/5] Remove extra unneded safety --- osu.Game/Screens/Select/Leaderboards/Leaderboard.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs index 4d8d43f60a..429e6a405d 100644 --- a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs @@ -255,23 +255,15 @@ namespace osu.Game.Screens.Select.Leaderboards PlaceholderState = PlaceholderState.Retrieving; loading.Show(); - var localBeatmap = Beatmap; - getScoresRequest = new GetScoresRequest(Beatmap, osuGame?.Ruleset.Value ?? Beatmap.Ruleset, Scope); getScoresRequest.Success += r => Schedule(() => { - if (localBeatmap != Beatmap) - return; - Scores = r.Scores; PlaceholderState = Scores.Any() ? PlaceholderState.Successful : PlaceholderState.NoScores; }); getScoresRequest.Failure += e => Schedule(() => { - if (localBeatmap != Beatmap) - return; - if (e is OperationCanceledException) return; From a27f39a55575411584f7bd02c444cfcaece7d6fa Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Apr 2018 11:57:06 +0900 Subject: [PATCH 4/5] Add documentation explaining intertwining logic --- osu.Game/Screens/Select/Leaderboards/Leaderboard.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs index 429e6a405d..43f6f13db3 100644 --- a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs @@ -109,6 +109,10 @@ namespace osu.Game.Screens.Select.Leaderboards private PlaceholderState placeholderState; + /// + /// Update the placeholder visibility. + /// Setting this to anything other than PlaceholderState.Successful will cancel all existing retrieval requests and hide scores. + /// protected PlaceholderState PlaceholderState { get { return placeholderState; } From 188c8ce1e7582f3789827a7004831b35e932bc34 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 18 Apr 2018 11:58:55 +0900 Subject: [PATCH 5/5] Remove unnecessary score nulling (already happens in PlaceholderState_Set) --- osu.Game/Screens/Select/Leaderboards/Leaderboard.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs index 43f6f13db3..9dae8fb273 100644 --- a/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs +++ b/osu.Game/Screens/Select/Leaderboards/Leaderboard.cs @@ -293,8 +293,6 @@ namespace osu.Game.Screens.Select.Leaderboards return; } - Scores = null; - placeholderContainer.Child = placeholder; placeholder.ScaleTo(0.8f).Then().ScaleTo(1, fade_duration * 3, Easing.OutQuint);