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.

Sign up for GitHub

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

Jump to bottom

Fix advanced stats in beatmap info overlay showing "key count" on non-mania beatmaps #27380

Merged
merged 3 commits into from Feb 28, 2024
Merged

Fix advanced stats in beatmap info overlay showing "key count" on non-mania beatmaps #27380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c049fe
Fix advanced stats in beatmap info overlay showing "key count" on non…
frenzibyte Feb 25, 2024
b30a6d5
Adjust existing test coverage
frenzibyte Feb 25, 2024
8e33661
Add xmldoc explaining `Ruleset` bindable's usage
peppy Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 5 additions & 44 deletions osu.Game.Tests/Visual/SongSelect/TestSceneAdvancedStats.cs
View file
Open in desktop
Expand Up @@ -37,15 +37,9 @@ public partial class TestSceneAdvancedStats : OsuTestScene
[SetUp]
public void Setup() => Schedule(() => Child = advancedStats = new TestAdvancedStats
{
Width = 500
Width = 500,
});

[SetUpSteps]
public void SetUpSteps()
{
AddStep("reset game ruleset", () => Ruleset.Value = new OsuRuleset().RulesetInfo);
}

private BeatmapInfo exampleBeatmapInfo => new BeatmapInfo
{
Ruleset = rulesets.AvailableRulesets.First(),
Expand Down Expand Up @@ -74,45 +68,12 @@ public void TestNoMod()
}

[Test]
public void TestManiaFirstBarTextManiaBeatmap()
public void TestFirstBarText()
{
AddStep("set game ruleset to mania", () => Ruleset.Value = new ManiaRuleset().RulesetInfo);

AddStep("set beatmap", () => advancedStats.BeatmapInfo = new BeatmapInfo
{
Ruleset = rulesets.GetRuleset(3) ?? throw new InvalidOperationException("osu!mania ruleset not found"),
Difficulty = new BeatmapDifficulty
{
CircleSize = 5,
DrainRate = 4.3f,
OverallDifficulty = 4.5f,
ApproachRate = 3.1f
},
StarRating = 8
});

AddAssert("first bar text is correct", () => advancedStats.ChildrenOfType<SpriteText>().First().Text == BeatmapsetsStrings.ShowStatsCsMania);
}

[Test]
public void TestManiaFirstBarTextConvert()
{
AddStep("set game ruleset to mania", () => Ruleset.Value = new ManiaRuleset().RulesetInfo);

AddStep("set beatmap", () => advancedStats.BeatmapInfo = new BeatmapInfo
{
Ruleset = new OsuRuleset().RulesetInfo,
Difficulty = new BeatmapDifficulty
{
CircleSize = 5,
DrainRate = 4.3f,
OverallDifficulty = 4.5f,
ApproachRate = 3.1f
},
StarRating = 8
});

AddStep("set ruleset to mania", () => advancedStats.Ruleset.Value = new ManiaRuleset().RulesetInfo);
AddAssert("first bar text is correct", () => advancedStats.ChildrenOfType<SpriteText>().First().Text == BeatmapsetsStrings.ShowStatsCsMania);
AddStep("set ruleset to osu", () => advancedStats.Ruleset.Value = new OsuRuleset().RulesetInfo);
AddAssert("first bar text is correct", () => advancedStats.ChildrenOfType<SpriteText>().First().Text == BeatmapsetsStrings.ShowStatsCs);
}

[Test]
Expand Down
30 changes: 19 additions & 11 deletions osu.Game/Overlays/BeatmapSet/Details.cs
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
using osu.Game.Beatmaps;
using osu.Game.Online.API.Requests.Responses;
using osu.Game.Overlays.BeatmapSet.Buttons;
using osu.Game.Rulesets;
using osu.Game.Screens.Select.Details;
using osuTK;

Expand All @@ -33,10 +34,10 @@ public APIBeatmapSet BeatmapSet
{
if (value == beatmapSet) return;

beatmapSet = value;
basic.BeatmapSet = preview.BeatmapSet = beatmapSet = value;

basic.BeatmapSet = preview.BeatmapSet = BeatmapSet;
updateDisplay();
if (IsLoaded)
updateDisplay();
}
}

Expand All @@ -50,13 +51,10 @@ public IBeatmapInfo BeatmapInfo
if (value == beatmapInfo) return;

basic.BeatmapInfo = advanced.BeatmapInfo = beatmapInfo = value;
}
}

private void updateDisplay()
{
Ratings.Ratings = BeatmapSet?.Ratings;
ratingBox.Alpha = BeatmapSet?.Status > 0 ? 1 : 0;
if (IsLoaded)
updateDisplay();
}
}

public Details()
Expand Down Expand Up @@ -101,12 +99,22 @@ public Details()
};
}

[BackgroundDependencyLoader]
private void load()
[Resolved]
private RulesetStore rulesets { get; set; }

protected override void LoadComplete()
{
base.LoadComplete();
updateDisplay();
}

private void updateDisplay()
{
Ratings.Ratings = BeatmapSet?.Ratings;
ratingBox.Alpha = BeatmapSet?.Status > 0 ? 1 : 0;
advanced.Ruleset.Value = rulesets.GetRuleset(beatmapInfo?.Ruleset.OnlineID ?? 0);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I dunno. I would have just changed AdvancedStats to have a ctor flag of something like "showConvertedRulesetInfo" rather than changing this whole flow.

The new flow reads very shoddy and is confusing. Like when do you hook up ruleset to beamap and when do you use game? It's not explained anywhere and bound to get used wrong in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't agree with that. I think boolean flags are shoddy and should only be used as a last resort. This seems better because it makes this a more "pure" component in that it has explicit inputs that it's reading rather some implicit resolution process as previously.

On a quick skim the only thing that makes me wince in this diff is the SongSelect change and it looks like that one is caused by the typical song select debounce hell.

Copy link
Sponsor Member

@peppy peppy Feb 26, 2024 •
edited

Choose a reason for hiding this comment

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

To confirm you're understand this: Ruleset not being set (and thus null) in the online beatmap set overlay is the fix here.

Zero xmldoc explaining this, just a wild ruleset bindable? I don't know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uhhhh... I guess I missed that part. null fixing it is dodgy indeed.

Should be explicitly set in all instances.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

If it's explicitly set on the beatmap overlay to anything that isn't null, it will regress the fix. I'm open to a better structure or xmldoc trying to explain this, but this is why I proposed the ctor argument instead (it's explicit and it gives us a place to document the modes of operation). 🤷

Copy link
Sponsor Member

@peppy peppy Feb 26, 2024 •
edited

Choose a reason for hiding this comment

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

My API proposal is for AdvancedStats which is used at song select, and bound to song select's local ruleset.

Maybe if we can't easily agree on this the class should just be copypasted into two versions. Because I guarantee the next issue is going to be that it's reading from global mods and displaying "unexpected" on the overlay.

Copy link
Collaborator

@bdach bdach Feb 26, 2024 •
edited

Choose a reason for hiding this comment

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

I'm not sure why this conversation is going in the places it's going so I might just recuse myself from it.

the next issue is going to be that it's reading from global mods and displaying "unexpected" on the overlay

I never wanted that. It should not be reading from global mods. It should be given the correct ruleset to show explicitly in every usage. No inferences.

Hell give it a ruleset bindable instead but one that is explicltly hooked to by every usage. Song select can bind it to the game-global bindable and beatmap info overlay can do its local one.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

There's more issues with AdvancedStats ruleset – it doesn't have knowledge of whether a conversion can happen, so right now it will prefer the bound ruleset even if the conversion is impossible (ie. mania beatmap and osu! ruleset being set). Not sure if this is okay or not. Should it be up to the point of construction to get this right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My vision for it was for it to be a dumb component that displays what it's told to. At the point it's told to display an impossible conversion something has gone deeply wrong somewhere higher above I'd say.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Fair enough.

}

private partial class DetailBox : Container
{
private readonly Container content;
Expand Down
36 changes: 16 additions & 20 deletions osu.Game/Screens/Select/Details/AdvancedStats.cs
View file
Open in desktop
Expand Up @@ -38,11 +38,6 @@ public partial class AdvancedStats : Container, IHasCustomTooltip<AdjustedAttrib
[Resolved]
private IBindable<IReadOnlyList<Mod>> mods { get; set; }

[Resolved]
private OsuGameBase game { get; set; }

private IBindable<RulesetInfo> gameRuleset;

protected readonly StatisticRow FirstValue, HpDrain, Accuracy, ApproachRate;
private readonly StatisticRow starDifficulty;

Expand All @@ -64,6 +59,15 @@ public IBeatmapInfo BeatmapInfo
}
}

/// <summary>
/// Ruleset to be used for certain elements of display.
/// When set, this will override the set <see cref="Beatmap"/>'s own ruleset.
/// </summary>
/// <remarks>
/// No checks are done as to whether the ruleset specified is valid for the currently <see cref="BeatmapInfo"/>.
/// </remarks>
public Bindable<RulesetInfo> Ruleset { get; } = new Bindable<RulesetInfo>();

public AdvancedStats(int columns = 1)
{
switch (columns)
Expand Down Expand Up @@ -137,12 +141,7 @@ protected override void LoadComplete()
{
base.LoadComplete();

// the cached ruleset bindable might be a decoupled bindable provided by SongSelect,
// which we can't rely on in combination with the game-wide selected mods list,
// since mods could be updated to the new ruleset instances while the decoupled bindable is held behind,
// therefore resulting in performing difficulty calculation with invalid states.
gameRuleset = game.Ruleset.GetBoundCopy();
gameRuleset.BindValueChanged(_ => updateStatistics());
Ruleset.BindValueChanged(_ => updateStatistics());

mods.BindValueChanged(modsChanged, true);
}
Expand All @@ -169,8 +168,6 @@ private void updateStatistics()
IBeatmapDifficultyInfo baseDifficulty = BeatmapInfo?.Difficulty;
BeatmapDifficulty adjustedDifficulty = null;

IRulesetInfo ruleset = gameRuleset?.Value ?? beatmapInfo.Ruleset;

if (baseDifficulty != null)
{
BeatmapDifficulty originalDifficulty = new BeatmapDifficulty(baseDifficulty);
Expand All @@ -180,24 +177,24 @@ private void updateStatistics()

adjustedDifficulty = originalDifficulty;

if (gameRuleset != null)
if (Ruleset.Value != null)
{
double rate = 1;
foreach (var mod in mods.Value.OfType<IApplicableToRate>())
rate = mod.ApplyToRate(0, rate);

adjustedDifficulty = ruleset.CreateInstance().GetRateAdjustedDisplayDifficulty(originalDifficulty, rate);
adjustedDifficulty = Ruleset.Value.CreateInstance().GetRateAdjustedDisplayDifficulty(originalDifficulty, rate);

TooltipContent = new AdjustedAttributesTooltip.Data(originalDifficulty, adjustedDifficulty);
}
}

switch (ruleset.OnlineID)
switch (Ruleset.Value?.OnlineID)
{
case 3:
// Account for mania differences locally for now.
// Eventually this should be handled in a more modular way, allowing rulesets to return arbitrary difficulty attributes.
ILegacyRuleset legacyRuleset = (ILegacyRuleset)ruleset.CreateInstance();
ILegacyRuleset legacyRuleset = (ILegacyRuleset)Ruleset.Value.CreateInstance();

// For the time being, the key count is static no matter what, because:
// a) The method doesn't have knowledge of the active keymods. Doing so may require considerations for filtering.
Expand All @@ -206,7 +203,6 @@ private void updateStatistics()

FirstValue.Title = BeatmapsetsStrings.ShowStatsCsMania;
FirstValue.Value = (keyCount, keyCount);

break;

default:
Expand Down Expand Up @@ -240,8 +236,8 @@ private void updateStatistics()

starDifficultyCancellationSource = new CancellationTokenSource();

var normalStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, gameRuleset.Value, null, starDifficultyCancellationSource.Token);
var moddedStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, gameRuleset.Value, mods.Value, starDifficultyCancellationSource.Token);
var normalStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, Ruleset.Value, null, starDifficultyCancellationSource.Token);
var moddedStarDifficultyTask = difficultyCache.GetDifficultyAsync(BeatmapInfo, Ruleset.Value, mods.Value, starDifficultyCancellationSource.Token);

Task.WhenAll(normalStarDifficultyTask, moddedStarDifficultyTask).ContinueWith(_ => Schedule(() =>
{
Expand Down
7 changes: 6 additions & 1 deletion osu.Game/Screens/Select/SongSelect.cs
View file
Open in desktop
Expand Up @@ -286,7 +286,7 @@ private void load(AudioManager audio, OsuColour colours, ManageCollectionsDialog
AutoSizeAxes = Axes.Y,
Anchor = Anchor.Centre,
Origin = Anchor.Centre,
Padding = new MarginPadding(10)
Padding = new MarginPadding(10),
},
}
},
Expand Down Expand Up @@ -585,6 +585,11 @@ private void performUpdateSelected()
beatmapInfoPrevious = beatmap;
}

// we can't run this in the debounced run due to the selected mods bindable not being debounced,
// since mods could be updated to the new ruleset instances while the decoupled bindable is held behind,
// therefore resulting in performing difficulty calculation with invalid states.
advancedStats.Ruleset.Value = ruleset;

void run()
{
// clear pending task immediately to track any potential nested debounce operation.
Expand Down