From dc595b83f12761a410240e404dc965a27258a37f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 1 Jun 2023 14:25:18 +0900 Subject: [PATCH 1/3] Remove unused `Dimension` specification from `StatisticItem` --- osu.Game/Screens/Ranking/Statistics/StatisticItem.cs | 10 +--------- osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs | 2 +- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs b/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs index 5bbd260d3fd0..77a40959aeb1 100644 --- a/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs +++ b/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs @@ -6,7 +6,6 @@ using System; using JetBrains.Annotations; using osu.Framework.Graphics; -using osu.Framework.Graphics.Containers; using osu.Framework.Localisation; namespace osu.Game.Screens.Ranking.Statistics @@ -26,11 +25,6 @@ public class StatisticItem /// public readonly Func CreateContent; - /// - /// The of this row. This can be thought of as the column dimension of an encompassing . - /// - public readonly Dimension Dimension; - /// /// Whether this item requires hit events. If true, will not be called if no hit events are available. /// @@ -42,13 +36,11 @@ public class StatisticItem /// The name of the item. Can be to hide the item header. /// A function returning the content to be displayed. /// Whether this item requires hit events. If true, will not be called if no hit events are available. - /// The of this item. This can be thought of as the column dimension of an encompassing . - public StatisticItem(LocalisableString name, [NotNull] Func createContent, bool requiresHitEvents = false, [CanBeNull] Dimension dimension = null) + public StatisticItem(LocalisableString name, [NotNull] Func createContent, bool requiresHitEvents = false) { Name = name; RequiresHitEvents = requiresHitEvents; CreateContent = createContent; - Dimension = dimension; } } } diff --git a/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs b/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs index 4c22afd8f708..c11c42e290e7 100644 --- a/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs +++ b/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs @@ -168,7 +168,7 @@ private void populateStatistics(ValueChangedEvent score) Origin = Anchor.Centre, }); - dimensions.Add(col.Dimension ?? new Dimension()); + dimensions.Add(new Dimension()); } rows.Add(new GridContainer From 985604fab5bb4a74ef7eacdcd842e8190c74047b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 1 Jun 2023 14:35:14 +0900 Subject: [PATCH 2/3] Return `StatisticItem`s rather than `StatisticRow`s from ruleset There were no usages of more than one column being provided per row, so it seemed like unnecessarily complexity. I'm currently trying to reduce complexity so we can improve the layout of the results screen, which currently has up to three levels of nested `GridContainer`s. Of note, I can't add backwards compatibility because the method signature has not changed in `Ruleset` (only the return type). If we do want to keep compatibility with other rulesets, we could designate a new name for the updated method. --- osu.Game.Rulesets.Mania/ManiaRuleset.cs | 44 ++++--------- osu.Game.Rulesets.Osu/OsuRuleset.cs | 58 +++++------------ osu.Game.Rulesets.Taiko/TaikoRuleset.cs | 44 ++++--------- .../Ranking/TestSceneStatisticsPanel.cs | 65 +++---------------- osu.Game/Rulesets/Ruleset.cs | 4 +- .../Statistics/SimpleStatisticTable.cs | 2 +- .../Ranking/Statistics/SoloStatisticsPanel.cs | 26 +++----- .../Ranking/Statistics/StatisticItem.cs | 2 +- .../Ranking/Statistics/StatisticRow.cs | 21 ------ .../Ranking/Statistics/StatisticsPanel.cs | 38 +++++------ 10 files changed, 82 insertions(+), 222 deletions(-) delete mode 100644 osu.Game/Screens/Ranking/Statistics/StatisticRow.cs diff --git a/osu.Game.Rulesets.Mania/ManiaRuleset.cs b/osu.Game.Rulesets.Mania/ManiaRuleset.cs index d324682989f3..e8fda3ec8038 100644 --- a/osu.Game.Rulesets.Mania/ManiaRuleset.cs +++ b/osu.Game.Rulesets.Mania/ManiaRuleset.cs @@ -389,41 +389,23 @@ public override LocalisableString GetDisplayNameForHitResult(HitResult result) return base.GetDisplayNameForHitResult(result); } - public override StatisticRow[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) => new[] + public override StatisticItem[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) => new[] { - new StatisticRow + new StatisticItem("Performance Breakdown", () => new PerformanceBreakdownChart(score, playableBeatmap) { - Columns = new[] - { - new StatisticItem("Performance Breakdown", () => new PerformanceBreakdownChart(score, playableBeatmap) - { - RelativeSizeAxes = Axes.X, - AutoSizeAxes = Axes.Y - }), - } - }, - new StatisticRow + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y + }), + new StatisticItem("Timing Distribution", () => new HitEventTimingDistributionGraph(score.HitEvents) { - Columns = new[] - { - new StatisticItem("Timing Distribution", () => new HitEventTimingDistributionGraph(score.HitEvents) - { - RelativeSizeAxes = Axes.X, - Height = 250 - }, true), - } - }, - new StatisticRow + RelativeSizeAxes = Axes.X, + Height = 250 + }, true), + new StatisticItem(string.Empty, () => new SimpleStatisticTable(3, new SimpleStatisticItem[] { - Columns = new[] - { - new StatisticItem(string.Empty, () => new SimpleStatisticTable(3, new SimpleStatisticItem[] - { - new AverageHitError(score.HitEvents), - new UnstableRate(score.HitEvents) - }), true) - } - } + new AverageHitError(score.HitEvents), + new UnstableRate(score.HitEvents) + }), true) }; public override IRulesetFilterCriteria CreateRulesetFilterCriteria() diff --git a/osu.Game.Rulesets.Osu/OsuRuleset.cs b/osu.Game.Rulesets.Osu/OsuRuleset.cs index 922594a93ae2..8ce55d78dd09 100644 --- a/osu.Game.Rulesets.Osu/OsuRuleset.cs +++ b/osu.Game.Rulesets.Osu/OsuRuleset.cs @@ -291,56 +291,32 @@ public override LocalisableString GetDisplayNameForHitResult(HitResult result) return base.GetDisplayNameForHitResult(result); } - public override StatisticRow[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) + public override StatisticItem[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) { var timedHitEvents = score.HitEvents.Where(e => e.HitObject is HitCircle && !(e.HitObject is SliderTailCircle)).ToList(); return new[] { - new StatisticRow + new StatisticItem("Performance Breakdown", () => new PerformanceBreakdownChart(score, playableBeatmap) { - Columns = new[] - { - new StatisticItem("Performance Breakdown", () => new PerformanceBreakdownChart(score, playableBeatmap) - { - RelativeSizeAxes = Axes.X, - AutoSizeAxes = Axes.Y - }), - } - }, - new StatisticRow + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y + }), + new StatisticItem("Timing Distribution", () => new HitEventTimingDistributionGraph(timedHitEvents) { - Columns = new[] - { - new StatisticItem("Timing Distribution", () => new HitEventTimingDistributionGraph(timedHitEvents) - { - RelativeSizeAxes = Axes.X, - Height = 250 - }, true), - } - }, - new StatisticRow + RelativeSizeAxes = Axes.X, + Height = 250 + }, true), + new StatisticItem("Accuracy Heatmap", () => new AccuracyHeatmap(score, playableBeatmap) { - Columns = new[] - { - new StatisticItem("Accuracy Heatmap", () => new AccuracyHeatmap(score, playableBeatmap) - { - RelativeSizeAxes = Axes.X, - Height = 250 - }, true), - } - }, - new StatisticRow + RelativeSizeAxes = Axes.X, + Height = 250 + }, true), + new StatisticItem(string.Empty, () => new SimpleStatisticTable(3, new SimpleStatisticItem[] { - Columns = new[] - { - new StatisticItem(string.Empty, () => new SimpleStatisticTable(3, new SimpleStatisticItem[] - { - new AverageHitError(timedHitEvents), - new UnstableRate(timedHitEvents) - }), true) - } - } + new AverageHitError(timedHitEvents), + new UnstableRate(timedHitEvents) + }), true) }; } diff --git a/osu.Game.Rulesets.Taiko/TaikoRuleset.cs b/osu.Game.Rulesets.Taiko/TaikoRuleset.cs index a35fdb890da3..d6824109b30a 100644 --- a/osu.Game.Rulesets.Taiko/TaikoRuleset.cs +++ b/osu.Game.Rulesets.Taiko/TaikoRuleset.cs @@ -229,45 +229,27 @@ public override LocalisableString GetDisplayNameForHitResult(HitResult result) return base.GetDisplayNameForHitResult(result); } - public override StatisticRow[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) + public override StatisticItem[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) { var timedHitEvents = score.HitEvents.Where(e => e.HitObject is Hit).ToList(); return new[] { - new StatisticRow + new StatisticItem("Performance Breakdown", () => new PerformanceBreakdownChart(score, playableBeatmap) { - Columns = new[] - { - new StatisticItem("Performance Breakdown", () => new PerformanceBreakdownChart(score, playableBeatmap) - { - RelativeSizeAxes = Axes.X, - AutoSizeAxes = Axes.Y - }), - } - }, - new StatisticRow + RelativeSizeAxes = Axes.X, + AutoSizeAxes = Axes.Y + }), + new StatisticItem("Timing Distribution", () => new HitEventTimingDistributionGraph(timedHitEvents) { - Columns = new[] - { - new StatisticItem("Timing Distribution", () => new HitEventTimingDistributionGraph(timedHitEvents) - { - RelativeSizeAxes = Axes.X, - Height = 250 - }, true), - } - }, - new StatisticRow + RelativeSizeAxes = Axes.X, + Height = 250 + }, true), + new StatisticItem(string.Empty, () => new SimpleStatisticTable(3, new SimpleStatisticItem[] { - Columns = new[] - { - new StatisticItem(string.Empty, () => new SimpleStatisticTable(3, new SimpleStatisticItem[] - { - new AverageHitError(timedHitEvents), - new UnstableRate(timedHitEvents) - }), true) - } - } + new AverageHitError(timedHitEvents), + new UnstableRate(timedHitEvents) + }), true) }; } } diff --git a/osu.Game.Tests/Visual/Ranking/TestSceneStatisticsPanel.cs b/osu.Game.Tests/Visual/Ranking/TestSceneStatisticsPanel.cs index fcd5f97fccbb..67211a3b729d 100644 --- a/osu.Game.Tests/Visual/Ranking/TestSceneStatisticsPanel.cs +++ b/osu.Game.Tests/Visual/Ranking/TestSceneStatisticsPanel.cs @@ -174,78 +174,33 @@ public TestBeatmapConverter(IBeatmap beatmap) private class TestRulesetAllStatsRequireHitEvents : TestRuleset { - public override StatisticRow[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) + public override StatisticItem[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) => new[] { - return new[] - { - new StatisticRow - { - Columns = new[] - { - new StatisticItem("Statistic Requiring Hit Events 1", - () => CreatePlaceholderStatistic("Placeholder statistic. Requires hit events"), true) - } - }, - new StatisticRow - { - Columns = new[] - { - new StatisticItem("Statistic Requiring Hit Events 2", - () => CreatePlaceholderStatistic("Placeholder statistic. Requires hit events"), true) - } - } - }; - } + new StatisticItem("Statistic Requiring Hit Events 1", () => CreatePlaceholderStatistic("Placeholder statistic. Requires hit events"), true), + new StatisticItem("Statistic Requiring Hit Events 2", () => CreatePlaceholderStatistic("Placeholder statistic. Requires hit events"), true) + }; } private class TestRulesetNoStatsRequireHitEvents : TestRuleset { - public override StatisticRow[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) + public override StatisticItem[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) { return new[] { - new StatisticRow - { - Columns = new[] - { - new StatisticItem("Statistic Not Requiring Hit Events 1", - () => CreatePlaceholderStatistic("Placeholder statistic. Does not require hit events")) - } - }, - new StatisticRow - { - Columns = new[] - { - new StatisticItem("Statistic Not Requiring Hit Events 2", - () => CreatePlaceholderStatistic("Placeholder statistic. Does not require hit events")) - } - } + new StatisticItem("Statistic Not Requiring Hit Events 1", () => CreatePlaceholderStatistic("Placeholder statistic. Does not require hit events")), + new StatisticItem("Statistic Not Requiring Hit Events 2", () => CreatePlaceholderStatistic("Placeholder statistic. Does not require hit events")) }; } } private class TestRulesetMixed : TestRuleset { - public override StatisticRow[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) + public override StatisticItem[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) { return new[] { - new StatisticRow - { - Columns = new[] - { - new StatisticItem("Statistic Requiring Hit Events", - () => CreatePlaceholderStatistic("Placeholder statistic. Requires hit events"), true) - } - }, - new StatisticRow - { - Columns = new[] - { - new StatisticItem("Statistic Not Requiring Hit Events", - () => CreatePlaceholderStatistic("Placeholder statistic. Does not require hit events")) - } - } + new StatisticItem("Statistic Requiring Hit Events", () => CreatePlaceholderStatistic("Placeholder statistic. Requires hit events"), true), + new StatisticItem("Statistic Not Requiring Hit Events", () => CreatePlaceholderStatistic("Placeholder statistic. Does not require hit events")) }; } } diff --git a/osu.Game/Rulesets/Ruleset.cs b/osu.Game/Rulesets/Ruleset.cs index a77068eb145c..490ec1475c95 100644 --- a/osu.Game/Rulesets/Ruleset.cs +++ b/osu.Game/Rulesets/Ruleset.cs @@ -321,8 +321,8 @@ protected Ruleset() /// /// The to create the statistics for. The score is guaranteed to have populated. /// The , converted for this with all relevant s applied. - /// The s to display. Each may contain 0 or more . - public virtual StatisticRow[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) => Array.Empty(); + /// The s to display. + public virtual StatisticItem[] CreateStatisticsForScore(ScoreInfo score, IBeatmap playableBeatmap) => Array.Empty(); /// /// Get all valid s for this ruleset. diff --git a/osu.Game/Screens/Ranking/Statistics/SimpleStatisticTable.cs b/osu.Game/Screens/Ranking/Statistics/SimpleStatisticTable.cs index d10888be43e4..d68df4558a0d 100644 --- a/osu.Game/Screens/Ranking/Statistics/SimpleStatisticTable.cs +++ b/osu.Game/Screens/Ranking/Statistics/SimpleStatisticTable.cs @@ -17,7 +17,7 @@ namespace osu.Game.Screens.Ranking.Statistics { /// /// Represents a table with simple statistics (ones that only need textual display). - /// Richer visualisations should be done with s and s. + /// Richer visualisations should be done with s. /// public partial class SimpleStatisticTable : CompositeDrawable { diff --git a/osu.Game/Screens/Ranking/Statistics/SoloStatisticsPanel.cs b/osu.Game/Screens/Ranking/Statistics/SoloStatisticsPanel.cs index 57d072b7de81..73b9897096fe 100644 --- a/osu.Game/Screens/Ranking/Statistics/SoloStatisticsPanel.cs +++ b/osu.Game/Screens/Ranking/Statistics/SoloStatisticsPanel.cs @@ -23,32 +23,26 @@ public SoloStatisticsPanel(ScoreInfo achievedScore) public Bindable StatisticsUpdate { get; } = new Bindable(); - protected override ICollection CreateStatisticRows(ScoreInfo newScore, IBeatmap playableBeatmap) + protected override ICollection CreateStatisticItems(ScoreInfo newScore, IBeatmap playableBeatmap) { - var rows = base.CreateStatisticRows(newScore, playableBeatmap); + var items = base.CreateStatisticItems(newScore, playableBeatmap); if (newScore.UserID > 1 && newScore.UserID == achievedScore.UserID && newScore.OnlineID > 0 && newScore.OnlineID == achievedScore.OnlineID) { - rows = rows.Append(new StatisticRow + items = items.Append(new StatisticItem("Overall Ranking", () => new OverallRanking { - Columns = new[] - { - new StatisticItem("Overall Ranking", () => new OverallRanking - { - RelativeSizeAxes = Axes.X, - Anchor = Anchor.Centre, - Origin = Anchor.Centre, - Width = 0.5f, - StatisticsUpdate = { BindTarget = StatisticsUpdate } - }) - } - }).ToArray(); + RelativeSizeAxes = Axes.X, + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + Width = 0.5f, + StatisticsUpdate = { BindTarget = StatisticsUpdate } + })).ToArray(); } - return rows; + return items; } } } diff --git a/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs b/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs index 77a40959aeb1..c5bdc6f6f5d5 100644 --- a/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs +++ b/osu.Game/Screens/Ranking/Statistics/StatisticItem.cs @@ -31,7 +31,7 @@ public class StatisticItem public readonly bool RequiresHitEvents; /// - /// Creates a new , to be displayed inside a in the results screen. + /// Creates a new , to be displayed in the results screen. /// /// The name of the item. Can be to hide the item header. /// A function returning the content to be displayed. diff --git a/osu.Game/Screens/Ranking/Statistics/StatisticRow.cs b/osu.Game/Screens/Ranking/Statistics/StatisticRow.cs deleted file mode 100644 index 9f5f44918e6f..000000000000 --- a/osu.Game/Screens/Ranking/Statistics/StatisticRow.cs +++ /dev/null @@ -1,21 +0,0 @@ -// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. -// See the LICENCE file in the repository root for full licence text. - -#nullable disable - -using JetBrains.Annotations; - -namespace osu.Game.Screens.Ranking.Statistics -{ - /// - /// A row of statistics to be displayed in the results screen. - /// - public class StatisticRow - { - /// - /// The columns of this . - /// - [ItemNotNull] - public StatisticItem[] Columns; - } -} diff --git a/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs b/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs index c11c42e290e7..31dd5df27a83 100644 --- a/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs +++ b/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs @@ -100,9 +100,9 @@ private void populateStatistics(ValueChangedEvent score) bool hitEventsAvailable = newScore.HitEvents.Count != 0; Container container; - var statisticRows = CreateStatisticRows(newScore, task.GetResultSafely()); + var statisticItems = CreateStatisticItems(newScore, task.GetResultSafely()); - if (!hitEventsAvailable && statisticRows.SelectMany(r => r.Columns).All(c => c.RequiresHitEvents)) + if (!hitEventsAvailable && statisticItems.All(c => c.RequiresHitEvents)) { container = new FillFlowContainer { @@ -144,32 +144,24 @@ private void populateStatistics(ValueChangedEvent score) bool anyRequiredHitEvents = false; - foreach (var row in statisticRows) + foreach (var item in statisticItems) { - var columns = row.Columns; - - if (columns.Length == 0) - continue; - var columnContent = new List(); var dimensions = new List(); - foreach (var col in columns) + if (!hitEventsAvailable && item.RequiresHitEvents) { - if (!hitEventsAvailable && col.RequiresHitEvents) - { - anyRequiredHitEvents = true; - continue; - } + anyRequiredHitEvents = true; + continue; + } - columnContent.Add(new StatisticContainer(col) - { - Anchor = Anchor.Centre, - Origin = Anchor.Centre, - }); + columnContent.Add(new StatisticContainer(item) + { + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + }); - dimensions.Add(new Dimension()); - } + dimensions.Add(new Dimension()); rows.Add(new GridContainer { @@ -219,11 +211,11 @@ private void populateStatistics(ValueChangedEvent score) } /// - /// Creates the s to be displayed in this panel for a given . + /// Creates the s to be displayed in this panel for a given . /// /// The score to create the rows for. /// The beatmap on which the score was set. - protected virtual ICollection CreateStatisticRows(ScoreInfo newScore, IBeatmap playableBeatmap) + protected virtual ICollection CreateStatisticItems(ScoreInfo newScore, IBeatmap playableBeatmap) => newScore.Ruleset.CreateInstance().CreateStatisticsForScore(newScore, playableBeatmap); protected override bool OnClick(ClickEvent e) From 602d5db3bb66e84b9ade2d99b004fbdd5e130be6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 3 Jun 2023 19:40:01 +0200 Subject: [PATCH 3/3] Simplify column dimensions code `dimensions` would always receive exactly one item, so might as well inline it. And yes, at this point the grid container is mostly a glorified `FillFlowContainer { Direction = FlowDirection.Vertical }`, but I am not touching that in this pull pending further decisions with respect to direction. --- osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs b/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs index 31dd5df27a83..c36d7726dc8c 100644 --- a/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs +++ b/osu.Game/Screens/Ranking/Statistics/StatisticsPanel.cs @@ -147,7 +147,6 @@ private void populateStatistics(ValueChangedEvent score) foreach (var item in statisticItems) { var columnContent = new List(); - var dimensions = new List(); if (!hitEventsAvailable && item.RequiresHitEvents) { @@ -161,8 +160,6 @@ private void populateStatistics(ValueChangedEvent score) Origin = Anchor.Centre, }); - dimensions.Add(new Dimension()); - rows.Add(new GridContainer { Anchor = Anchor.TopCentre, @@ -170,7 +167,7 @@ private void populateStatistics(ValueChangedEvent score) RelativeSizeAxes = Axes.X, AutoSizeAxes = Axes.Y, Content = new[] { columnContent.ToArray() }, - ColumnDimensions = dimensions.ToArray(), + ColumnDimensions = new[] { new Dimension() }, RowDimensions = new[] { new Dimension(GridSizeMode.AutoSize) } }); }