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

De-dupe displayed hits in judgement counter #26540

Merged
merged 2 commits into from
Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion osu.Game.Tests/Visual/Gameplay/TestSceneJudgementCounter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,16 @@ public void TestMaxValueHiddenOnModeChange()
AddAssert("Assert max judgement hidden", () => counterDisplay.CounterFlow.ChildrenOfType<JudgementCounter>().First().Alpha == 0);
}

[Test]
public void TestNoDuplicates()
{
AddStep("create counter", () => Child = counterDisplay = new TestJudgementCounterDisplay());
AddStep("Show all judgements", () => counterDisplay.Mode.Value = JudgementCounterDisplay.DisplayMode.All);
AddAssert("Check no duplicates",
() => counterDisplay.CounterFlow.ChildrenOfType<JudgementCounter>().Count(),
() => Is.EqualTo(counterDisplay.CounterFlow.ChildrenOfType<JudgementCounter>().Select(c => c.ResultName.Text).Distinct().Count()));
}

[Test]
public void TestCycleDisplayModes()
{
Expand All @@ -163,7 +173,7 @@ public void TestCycleDisplayModes()

private int hiddenCount()
{
var num = counterDisplay.CounterFlow.Children.First(child => child.Result.Type == HitResult.LargeTickHit);
var num = counterDisplay.CounterFlow.Children.First(child => child.Result.Types.Contains(HitResult.LargeTickHit));
return num.Result.ResultCount.Value;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using osu.Framework.Allocation;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
using osu.Framework.Localisation;
using osu.Game.Rulesets;
using osu.Game.Rulesets.Judgements;
using osu.Game.Rulesets.Scoring;
Expand All @@ -21,18 +22,30 @@ public partial class JudgementCountController : Component
[Resolved]
private ScoreProcessor scoreProcessor { get; set; } = null!;

public List<JudgementCount> Results = new List<JudgementCount>();
private readonly Dictionary<HitResult, JudgementCount> results = new Dictionary<HitResult, JudgementCount>();

public IEnumerable<JudgementCount> Counters => counters;

private readonly List<JudgementCount> counters = new List<JudgementCount>();

[BackgroundDependencyLoader]
private void load(IBindable<RulesetInfo> ruleset)
{
foreach (var result in ruleset.Value.CreateInstance().GetHitResults())
// Due to weirdness in judgements, some results have the same name and should be aggregated for display purposes.
// There's only one case of this right now ("slider end").
foreach (var group in ruleset.Value.CreateInstance().GetHitResults().GroupBy(r => r.displayName))
{
Results.Add(new JudgementCount
var judgementCount = new JudgementCount
{
Type = result.result,
DisplayName = group.Key,
Types = group.Select(r => r.result).ToArray(),
ResultCount = new BindableInt()
});
};

counters.Add(judgementCount);

foreach (var r in group)
results[r.result] = judgementCount;
}
}

Expand All @@ -46,13 +59,20 @@ protected override void LoadComplete()

private void updateCount(JudgementResult judgement, bool revert)
{
foreach (JudgementCount result in Results.Where(result => result.Type == judgement.Type))
result.ResultCount.Value = revert ? result.ResultCount.Value - 1 : result.ResultCount.Value + 1;
if (!results.TryGetValue(judgement.Type, out var count))
return;

if (revert)
count.ResultCount.Value--;
else
count.ResultCount.Value++;
}

public struct JudgementCount
{
public HitResult Type { get; set; }
public LocalisableString DisplayName { get; set; }

public HitResult[] Types { get; set; }

public BindableInt ResultCount { get; set; }
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

using System.Linq;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
Expand Down Expand Up @@ -44,14 +45,14 @@ private void load(OsuColour colours, IBindable<RulesetInfo> ruleset)
{
Alpha = 0,
Font = OsuFont.Numeric.With(size: 8),
Text = ruleset.Value.CreateInstance().GetDisplayNameForHitResult(Result.Type)
Text = Result.DisplayName,
}
}
};

var result = Result.Type;
var result = Result.Types.First();

Colour = result.IsBasic() ? colours.ForHitResult(Result.Type) : !result.IsBonus() ? colours.PurpleLight : colours.PurpleLighter;
Colour = result.IsBasic() ? colours.ForHitResult(result) : !result.IsBonus() ? colours.PurpleLight : colours.PurpleLighter;
}

protected override void LoadComplete()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// See the LICENCE file in the repository root for full licence text.

using System;
using System.Linq;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
using osu.Framework.Graphics;
Expand Down Expand Up @@ -49,7 +50,7 @@ private void load()
AutoSizeAxes = Axes.Both
};

foreach (var result in judgementCountController.Results)
foreach (var result in judgementCountController.Counters)
CounterFlow.Add(createCounter(result));
}

Expand Down Expand Up @@ -88,7 +89,9 @@ bool shouldShow(int index, JudgementCounter counter)
if (index == 0 && !ShowMaxJudgement.Value)
return false;

if (counter.Result.Type.IsBasic())
var hitResult = counter.Result.Types.First();

if (hitResult.IsBasic())
Comment on lines +92 to +94
Copy link
Collaborator

Choose a reason for hiding this comment

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

this sort of stuff is pretty dodgy, but i guess it's also isolated display logic, so i can sort of pretend to not see this for now...

Copy link
Member Author

Choose a reason for hiding this comment

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

Haven't changed from what was there 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

To expound on the above, the .First() is what I take issue with specifically. There is nothing guaranteeing that the "chosen" result from the potential multiple in the given "result type grouping" represents the rest in terms of these .IsBasic() / .IsBonus() checks.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, right. yeah, that is what it is, i have no better ideas.

return true;

switch (Mode.Value)
Expand All @@ -97,7 +100,7 @@ bool shouldShow(int index, JudgementCounter counter)
return false;

case DisplayMode.Normal:
return !counter.Result.Type.IsBonus();
return !hitResult.IsBonus();

case DisplayMode.All:
return true;
Expand Down
Loading