h1or4dme8/osu-lazer
1
0
Fork 0
mirror of https://github.com/ppy/osu.git synced 2024-12-14 21:02:55 +08:00
Code Issues Releases Wiki Activity

Merge pull request #20171 from peppy/beatmap-carousel-nrt

Browse Source 
Fix missing null checks on `selectedBeatmap` fields in `BeatmapCarousel`
This commit is contained in:
Dan Balasescu 2022-09-07 17:15:00 +09:00 committed by GitHub
commit f8e37af2e0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 56 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs
View File

@ -173,7 +173,7 @@ namespace osu.Game.Tests.Visual.SongSelect
if (isIterating) if (isIterating)
AddUntilStep("selection changed", () => !carousel.SelectedBeatmapInfo?.Equals(selection) == true); AddUntilStep("selection changed", () => !carousel.SelectedBeatmapInfo?.Equals(selection) == true);
else else
AddUntilStep("selection not changed", () => carousel.SelectedBeatmapInfo.Equals(selection)); AddUntilStep("selection not changed", () => carousel.SelectedBeatmapInfo?.Equals(selection) == true);
} }
} }
} }
@ -382,7 +382,7 @@ namespace osu.Game.Tests.Visual.SongSelect
// buffer the selection // buffer the selection
setSelected(3, 2); setSelected(3, 2);
AddStep("get search text", () => searchText = carousel.SelectedBeatmapSet.Metadata.Title); AddStep("get search text", () => searchText = carousel.SelectedBeatmapSet!.Metadata.Title);
setSelected(1, 3); setSelected(1, 3);
@ -701,7 +701,7 @@ namespace osu.Game.Tests.Visual.SongSelect
setSelected(2, 1); setSelected(2, 1);
AddAssert("Selection is non-null", () => currentSelection != null); AddAssert("Selection is non-null", () => currentSelection != null);
AddStep("Remove selected", () => carousel.RemoveBeatmapSet(carousel.SelectedBeatmapSet)); AddStep("Remove selected", () => carousel.RemoveBeatmapSet(carousel.SelectedBeatmapSet!));
waitForSelection(2); waitForSelection(2);
AddStep("Remove first", () => carousel.RemoveBeatmapSet(carousel.BeatmapSets.First())); AddStep("Remove first", () => carousel.RemoveBeatmapSet(carousel.BeatmapSets.First()));
@ -804,7 +804,7 @@ namespace osu.Game.Tests.Visual.SongSelect
AddStep("filter to ruleset 0", () => AddStep("filter to ruleset 0", () =>
carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(0) }, false)); carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(0) }, false));
AddStep("select filtered map skipping filtered", () => carousel.SelectBeatmap(testMixed.Beatmaps[1], false)); AddStep("select filtered map skipping filtered", () => carousel.SelectBeatmap(testMixed.Beatmaps[1], false));
AddAssert("unfiltered beatmap not selected", () => carousel.SelectedBeatmapInfo.Ruleset.OnlineID == 0); AddAssert("unfiltered beatmap not selected", () => carousel.SelectedBeatmapInfo?.Ruleset.OnlineID == 0);
AddStep("remove mixed set", () => AddStep("remove mixed set", () =>
{ {
@ -854,7 +854,7 @@ namespace osu.Game.Tests.Visual.SongSelect
AddStep("Restore no filter", () => AddStep("Restore no filter", () =>
{ {
carousel.Filter(new FilterCriteria(), false); carousel.Filter(new FilterCriteria(), false);
eagerSelectedIDs.Add(carousel.SelectedBeatmapSet.ID); eagerSelectedIDs.Add(carousel.SelectedBeatmapSet!.ID);
}); });
} }
@ -899,10 +899,10 @@ namespace osu.Game.Tests.Visual.SongSelect
AddStep("Restore different ruleset filter", () => AddStep("Restore different ruleset filter", () =>
{ {
carousel.Filter(new FilterCriteria { Ruleset = rulesets.GetRuleset(1) }, false); carousel.Filter(new FilterCriteria { Ruleset = rulesets.GetRuleset(1) }, false);
eagerSelectedIDs.Add(carousel.SelectedBeatmapSet.ID); eagerSelectedIDs.Add(carousel.SelectedBeatmapSet!.ID);
}); });
AddAssert("selection changed", () => !carousel.SelectedBeatmapInfo.Equals(manySets.First().Beatmaps.First())); AddAssert("selection changed", () => !carousel.SelectedBeatmapInfo!.Equals(manySets.First().Beatmaps.First()));
} }
AddAssert("Selection was random", () => eagerSelectedIDs.Count > 2); AddAssert("Selection was random", () => eagerSelectedIDs.Count > 2);

80
osu.Game/Screens/Select/BeatmapCarousel.cs
View File

@ -1,8 +1,6 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence. // 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. // See the LICENCE file in the repository root for full licence text.
#nullable disable
using System; using System;
using System.Collections.Generic; using System.Collections.Generic;
using System.Diagnostics; using System.Diagnostics;
@ -49,31 +47,31 @@ namespace osu.Game.Screens.Select
/// <summary> /// <summary>
/// Triggered when the <see cref="BeatmapSets"/> loaded change and are completely loaded. /// Triggered when the <see cref="BeatmapSets"/> loaded change and are completely loaded.
/// </summary> /// </summary>
public Action BeatmapSetsChanged; public Action? BeatmapSetsChanged;
/// <summary> /// <summary>
/// The currently selected beatmap. /// The currently selected beatmap.
/// </summary> /// </summary>
public BeatmapInfo SelectedBeatmapInfo => selectedBeatmap?.BeatmapInfo; public BeatmapInfo? SelectedBeatmapInfo => selectedBeatmap?.BeatmapInfo;
private CarouselBeatmap selectedBeatmap => selectedBeatmapSet?.Beatmaps.FirstOrDefault(s => s.State.Value == CarouselItemState.Selected); private CarouselBeatmap? selectedBeatmap => selectedBeatmapSet?.Beatmaps.FirstOrDefault(s => s.State.Value == CarouselItemState.Selected);
/// <summary> /// <summary>
/// The currently selected beatmap set. /// The currently selected beatmap set.
/// </summary> /// </summary>
public BeatmapSetInfo SelectedBeatmapSet => selectedBeatmapSet?.BeatmapSet; public BeatmapSetInfo? SelectedBeatmapSet => selectedBeatmapSet?.BeatmapSet;
/// <summary> /// <summary>
/// A function to optionally decide on a recommended difficulty from a beatmap set. /// A function to optionally decide on a recommended difficulty from a beatmap set.
/// </summary> /// </summary>
public Func<IEnumerable<BeatmapInfo>, BeatmapInfo> GetRecommendedBeatmap; public Func<IEnumerable<BeatmapInfo>, BeatmapInfo>? GetRecommendedBeatmap;
private CarouselBeatmapSet selectedBeatmapSet; private CarouselBeatmapSet? selectedBeatmapSet;
/// <summary> /// <summary>
/// Raised when the <see cref="SelectedBeatmapInfo"/> is changed. /// Raised when the <see cref="SelectedBeatmapInfo"/> is changed.
/// </summary> /// </summary>
public Action<BeatmapInfo> SelectionChanged; public Action<BeatmapInfo?>? SelectionChanged;
public override bool HandleNonPositionalInput => AllowSelection; public override bool HandleNonPositionalInput => AllowSelection;
public override bool HandlePositionalInput => AllowSelection; public override bool HandlePositionalInput => AllowSelection;
@ -151,15 +149,15 @@ namespace osu.Game.Screens.Select
private CarouselRoot root; private CarouselRoot root;
private IDisposable subscriptionSets; private IDisposable? subscriptionSets;
private IDisposable subscriptionDeletedSets; private IDisposable? subscriptionDeletedSets;
private IDisposable subscriptionBeatmaps; private IDisposable? subscriptionBeatmaps;
private IDisposable subscriptionHiddenBeatmaps; private IDisposable? subscriptionHiddenBeatmaps;
private readonly DrawablePool<DrawableCarouselBeatmapSet> setPool = new DrawablePool<DrawableCarouselBeatmapSet>(100); private readonly DrawablePool<DrawableCarouselBeatmapSet> setPool = new DrawablePool<DrawableCarouselBeatmapSet>(100);
private Sample spinSample; private Sample? spinSample;
private Sample randomSelectSample; private Sample? randomSelectSample;
private int visibleSetsCount; private int visibleSetsCount;
@ -200,7 +198,7 @@ namespace osu.Game.Screens.Select
} }
[Resolved] [Resolved]
private RealmAccess realm { get; set; } private RealmAccess realm { get; set; } = null!;
protected override void LoadComplete() protected override void LoadComplete()
{ {
@ -215,7 +213,7 @@ namespace osu.Game.Screens.Select
subscriptionHiddenBeatmaps = realm.RegisterForNotifications(r => r.All<BeatmapInfo>().Where(b => b.Hidden), beatmapsChanged); subscriptionHiddenBeatmaps = realm.RegisterForNotifications(r => r.All<BeatmapInfo>().Where(b => b.Hidden), beatmapsChanged);
} }
private void deletedBeatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet changes, Exception error) private void deletedBeatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet? changes, Exception? error)
{ {
// If loading test beatmaps, avoid overwriting with realm subscription callbacks. // If loading test beatmaps, avoid overwriting with realm subscription callbacks.
if (loadedTestBeatmaps) if (loadedTestBeatmaps)
@ -228,7 +226,7 @@ namespace osu.Game.Screens.Select
removeBeatmapSet(sender[i].ID); removeBeatmapSet(sender[i].ID);
} }
private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet changes, Exception error) private void beatmapSetsChanged(IRealmCollection<BeatmapSetInfo> sender, ChangeSet? changes, Exception? error)
{ {
// If loading test beatmaps, avoid overwriting with realm subscription callbacks. // If loading test beatmaps, avoid overwriting with realm subscription callbacks.
if (loadedTestBeatmaps) if (loadedTestBeatmaps)
@ -266,8 +264,11 @@ namespace osu.Game.Screens.Select
foreach (int i in changes.InsertedIndices) foreach (int i in changes.InsertedIndices)
UpdateBeatmapSet(sender[i].Detach()); UpdateBeatmapSet(sender[i].Detach());
if (changes.DeletedIndices.Length > 0) if (changes.DeletedIndices.Length > 0 && SelectedBeatmapInfo != null)
{ {
// If SelectedBeatmapInfo is non-null, the set should also be non-null.
Debug.Assert(SelectedBeatmapSet != null);
// To handle the beatmap update flow, attempt to track selection changes across delete-insert transactions. // To handle the beatmap update flow, attempt to track selection changes across delete-insert transactions.
// When an update occurs, the previous beatmap set is either soft or hard deleted. // When an update occurs, the previous beatmap set is either soft or hard deleted.
// Check if the current selection was potentially deleted by re-querying its validity. // Check if the current selection was potentially deleted by re-querying its validity.
@ -304,7 +305,7 @@ namespace osu.Game.Screens.Select
} }
} }
private void beatmapsChanged(IRealmCollection<BeatmapInfo> sender, ChangeSet changes, Exception error) private void beatmapsChanged(IRealmCollection<BeatmapInfo> sender, ChangeSet? changes, Exception? error)
{ {
// we only care about actual changes in hidden status. // we only care about actual changes in hidden status.
if (changes == null) if (changes == null)
@ -367,7 +368,7 @@ namespace osu.Game.Screens.Select
// check if we can/need to maintain our current selection. // check if we can/need to maintain our current selection.
if (previouslySelectedID != null) if (previouslySelectedID != null)
select((CarouselItem)newSet.Beatmaps.FirstOrDefault(b => b.BeatmapInfo.ID == previouslySelectedID) ?? newSet); select((CarouselItem?)newSet.Beatmaps.FirstOrDefault(b => b.BeatmapInfo.ID == previouslySelectedID) ?? newSet);
} }
itemsCache.Invalidate(); itemsCache.Invalidate();
@ -384,7 +385,7 @@ namespace osu.Game.Screens.Select
/// <param name="beatmapInfo">The beatmap to select.</param> /// <param name="beatmapInfo">The beatmap to select.</param>
/// <param name="bypassFilters">Whether to select the beatmap even if it is filtered (i.e., not visible on carousel).</param> /// <param name="bypassFilters">Whether to select the beatmap even if it is filtered (i.e., not visible on carousel).</param>
/// <returns>True if a selection was made, False if it wasn't.</returns> /// <returns>True if a selection was made, False if it wasn't.</returns>
public bool SelectBeatmap(BeatmapInfo beatmapInfo, bool bypassFilters = true) public bool SelectBeatmap(BeatmapInfo? beatmapInfo, bool bypassFilters = true)
{ {
// ensure that any pending events from BeatmapManager have been run before attempting a selection. // ensure that any pending events from BeatmapManager have been run before attempting a selection.
Scheduler.Update(); Scheduler.Update();
@ -442,6 +443,9 @@ namespace osu.Game.Screens.Select
private void selectNextSet(int direction, bool skipDifficulties) private void selectNextSet(int direction, bool skipDifficulties)
{ {
if (selectedBeatmap == null || selectedBeatmapSet == null)
return;
var unfilteredSets = beatmapSets.Where(s => !s.Filtered.Value).ToList(); var unfilteredSets = beatmapSets.Where(s => !s.Filtered.Value).ToList();
var nextSet = unfilteredSets[(unfilteredSets.IndexOf(selectedBeatmapSet) + direction + unfilteredSets.Count) % unfilteredSets.Count]; var nextSet = unfilteredSets[(unfilteredSets.IndexOf(selectedBeatmapSet) + direction + unfilteredSets.Count) % unfilteredSets.Count];
@ -454,7 +458,7 @@ namespace osu.Game.Screens.Select
private void selectNextDifficulty(int direction) private void selectNextDifficulty(int direction)
{ {
if (selectedBeatmap == null) if (selectedBeatmap == null || selectedBeatmapSet == null)
return; return;
var unfilteredDifficulties = selectedBeatmapSet.Items.Where(s => !s.Filtered.Value).ToList(); var unfilteredDifficulties = selectedBeatmapSet.Items.Where(s => !s.Filtered.Value).ToList();
@ -483,7 +487,7 @@ namespace osu.Game.Screens.Select
if (!visibleSets.Any()) if (!visibleSets.Any())
return false; return false;
if (selectedBeatmap != null) if (selectedBeatmap != null && selectedBeatmapSet != null)
{ {
randomSelectedBeatmaps.Push(selectedBeatmap); randomSelectedBeatmaps.Push(selectedBeatmap);
@ -525,12 +529,14 @@ namespace osu.Game.Screens.Select
var beatmap = randomSelectedBeatmaps.Pop(); var beatmap = randomSelectedBeatmaps.Pop();
if (!beatmap.Filtered.Value) if (!beatmap.Filtered.Value)
{
if (selectedBeatmapSet != null)
{ {
if (RandomAlgorithm.Value == RandomSelectAlgorithm.RandomPermutation) if (RandomAlgorithm.Value == RandomSelectAlgorithm.RandomPermutation)
previouslyVisitedRandomSets.Remove(selectedBeatmapSet); previouslyVisitedRandomSets.Remove(selectedBeatmapSet);
if (selectedBeatmapSet != null)
playSpinSample(distanceBetween(beatmap, selectedBeatmapSet)); playSpinSample(distanceBetween(beatmap, selectedBeatmapSet));
}
select(beatmap); select(beatmap);
break; break;
@ -542,14 +548,18 @@ namespace osu.Game.Screens.Select
private void playSpinSample(double distance) private void playSpinSample(double distance)
{ {
var chan = spinSample.GetChannel(); var chan = spinSample?.GetChannel();
if (chan != null)
{
chan.Frequency.Value = 1f + Math.Min(1f, distance / visibleSetsCount); chan.Frequency.Value = 1f + Math.Min(1f, distance / visibleSetsCount);
chan.Play(); chan.Play();
}
randomSelectSample?.Play(); randomSelectSample?.Play();
} }
private void select(CarouselItem item) private void select(CarouselItem? item)
{ {
if (!AllowSelection) if (!AllowSelection)
return; return;
@ -561,7 +571,7 @@ namespace osu.Game.Screens.Select
private FilterCriteria activeCriteria = new FilterCriteria(); private FilterCriteria activeCriteria = new FilterCriteria();
protected ScheduledDelegate PendingFilter; protected ScheduledDelegate? PendingFilter;
public bool AllowSelection = true; public bool AllowSelection = true;
@ -593,7 +603,7 @@ namespace osu.Game.Screens.Select
} }
} }
public void Filter(FilterCriteria newCriteria, bool debounce = true) public void Filter(FilterCriteria? newCriteria, bool debounce = true)
{ {
if (newCriteria != null) if (newCriteria != null)
activeCriteria = newCriteria; activeCriteria = newCriteria;
@ -796,7 +806,7 @@ namespace osu.Game.Screens.Select
return (firstIndex, lastIndex); return (firstIndex, lastIndex);
} }
private CarouselBeatmapSet createCarouselSet(BeatmapSetInfo beatmapSet) private CarouselBeatmapSet? createCarouselSet(BeatmapSetInfo beatmapSet)
{ {
// This can be moved to the realm query if required using: // This can be moved to the realm query if required using:
// .Filter("DeletePending == false && Protected == false && ANY Beatmaps.Hidden == false") // .Filter("DeletePending == false && Protected == false && ANY Beatmaps.Hidden == false")
@ -962,7 +972,7 @@ namespace osu.Game.Screens.Select
/// </summary> /// </summary>
/// <param name="item">The item to be updated.</param> /// <param name="item">The item to be updated.</param>
/// <param name="parent">For nested items, the parent of the item to be updated.</param> /// <param name="parent">For nested items, the parent of the item to be updated.</param>
private void updateItem(DrawableCarouselItem item, DrawableCarouselItem parent = null) private void updateItem(DrawableCarouselItem item, DrawableCarouselItem? parent = null)
{ {
Vector2 posInScroll = Scroll.ScrollContent.ToLocalSpace(item.Header.ScreenSpaceDrawQuad.Centre); Vector2 posInScroll = Scroll.ScrollContent.ToLocalSpace(item.Header.ScreenSpaceDrawQuad.Centre);
float itemDrawY = posInScroll.Y - visibleUpperBound; float itemDrawY = posInScroll.Y - visibleUpperBound;
@ -990,13 +1000,13 @@ namespace osu.Game.Screens.Select
/// </summary> /// </summary>
private class CarouselBoundsItem : CarouselItem private class CarouselBoundsItem : CarouselItem
{ {
public override DrawableCarouselItem CreateDrawableRepresentation() => public override DrawableCarouselItem CreateDrawableRepresentation() => throw new NotImplementedException();
throw new NotImplementedException();
} }
private class CarouselRoot : CarouselGroupEagerSelect private class CarouselRoot : CarouselGroupEagerSelect
{ {
private readonly BeatmapCarousel carousel; // May only be null during construction (State.Value set causes PerformSelection to be triggered).
private readonly BeatmapCarousel? carousel;
public readonly Dictionary<Guid, CarouselBeatmapSet> BeatmapSetsByID = new Dictionary<Guid, CarouselBeatmapSet>(); public readonly Dictionary<Guid, CarouselBeatmapSet> BeatmapSetsByID = new Dictionary<Guid, CarouselBeatmapSet>();
@ -1017,7 +1027,7 @@ namespace osu.Game.Screens.Select
base.AddItem(i); base.AddItem(i);
} }
public CarouselBeatmapSet RemoveChild(Guid beatmapSetID) public CarouselBeatmapSet? RemoveChild(Guid beatmapSetID)
{ {
if (BeatmapSetsByID.TryGetValue(beatmapSetID, out var carouselBeatmapSet)) if (BeatmapSetsByID.TryGetValue(beatmapSetID, out var carouselBeatmapSet))
{ {