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

Fix track restarting when trying to switch track change direction with shuffle active #30215

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Fix track restarting when trying to switch track change direction with shuffle active #30215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0882f1b
Add failing test case
bdach Oct 11, 2024
47cb696
Fix switching direction when changing tracks with shuffle on restarti…
bdach Oct 11, 2024
1a25e9d
Add another failing test case for crash
bdach Oct 11, 2024
4a16341
Fix crash when switching tracks back and forth with shuffle on
bdach Oct 11, 2024
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
109 changes: 109 additions & 0 deletions osu.Game.Tests/Visual/Menus/TestSceneMusicActionHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,114 @@ public void TestMusicNavigationActions()
trackChangeQueue.Peek().changeDirection == TrackChangeDirection.Next);
AddAssert("track actually changed", () => !trackChangeQueue.First().working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
}

[Test]
public void TestShuffleBackwards()
{
Queue<(IWorkingBeatmap working, TrackChangeDirection changeDirection)> trackChangeQueue = null!;

AddStep("enable shuffle", () => Game.MusicController.Shuffle.Value = true);

// ensure we have at least two beatmaps available to identify the direction the music controller navigated to.
AddRepeatStep("import beatmap", () => Game.BeatmapManager.Import(TestResources.CreateTestBeatmapSetInfo()), 5);
AddStep("ensure nonzero track duration", () => Game.Realm.Write(r =>
{
// this was already supposed to be non-zero (see innards of `TestResources.CreateTestBeatmapSetInfo()`),
// but the non-zero value is being overwritten *to* zero by `BeatmapUpdater`.
// do a bit of a hack to change it back again - otherwise tracks are going to switch instantly and we won't be able to assert anything sane anymore.
foreach (var beatmap in r.All<BeatmapInfo>().Where(b => b.Length == 0))
beatmap.Length = 60_000;
}));

AddStep("bind to track change", () =>
{
trackChangeQueue = new Queue<(IWorkingBeatmap, TrackChangeDirection)>();
Game.MusicController.TrackChanged += (working, changeDirection) => trackChangeQueue.Enqueue((working, changeDirection));
});

AddStep("seek track to 6 second", () => Game.MusicController.SeekTo(6000));
AddUntilStep("wait for current time to update", () => Game.MusicController.CurrentTrack.CurrentTime > 5000);

AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
AddAssert("no track change", () => trackChangeQueue.Count == 0);
AddUntilStep("track restarted", () => Game.MusicController.CurrentTrack.CurrentTime < 5000);

AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
AddUntilStep("track changed", () => trackChangeQueue.Count == 1);

AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
AddUntilStep("track changed", () => trackChangeQueue.Count == 2);

AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
AddUntilStep("track changed", () =>
trackChangeQueue.Count == 3 && !trackChangeQueue.ElementAt(1).working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
}

[Test]
public void TestShuffleForwards()
{
Queue<(IWorkingBeatmap working, TrackChangeDirection changeDirection)> trackChangeQueue = null!;

AddStep("enable shuffle", () => Game.MusicController.Shuffle.Value = true);

// ensure we have at least two beatmaps available to identify the direction the music controller navigated to.
AddRepeatStep("import beatmap", () => Game.BeatmapManager.Import(TestResources.CreateTestBeatmapSetInfo()), 5);
AddStep("ensure nonzero track duration", () => Game.Realm.Write(r =>
{
// this was already supposed to be non-zero (see innards of `TestResources.CreateTestBeatmapSetInfo()`),
// but the non-zero value is being overwritten *to* zero by `BeatmapUpdater`.
// do a bit of a hack to change it back again - otherwise tracks are going to switch instantly and we won't be able to assert anything sane anymore.
foreach (var beatmap in r.All<BeatmapInfo>().Where(b => b.Length == 0))
beatmap.Length = 60_000;
}));

AddStep("bind to track change", () =>
{
trackChangeQueue = new Queue<(IWorkingBeatmap, TrackChangeDirection)>();
Game.MusicController.TrackChanged += (working, changeDirection) => trackChangeQueue.Enqueue((working, changeDirection));
});

AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
AddUntilStep("track changed", () => trackChangeQueue.Count == 1);

AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
AddUntilStep("track changed", () => trackChangeQueue.Count == 2);

AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
AddUntilStep("track changed", () =>
trackChangeQueue.Count == 3 && !trackChangeQueue.ElementAt(1).working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
}

[Test]
public void TestShuffleBackAndForth()
{
Queue<(IWorkingBeatmap working, TrackChangeDirection changeDirection)> trackChangeQueue = null!;

AddStep("enable shuffle", () => Game.MusicController.Shuffle.Value = true);

// ensure we have at least two beatmaps available to identify the direction the music controller navigated to.
AddRepeatStep("import beatmap", () => Game.BeatmapManager.Import(TestResources.CreateTestBeatmapSetInfo()), 5);
AddStep("ensure nonzero track duration", () => Game.Realm.Write(r =>
{
// this was already supposed to be non-zero (see innards of `TestResources.CreateTestBeatmapSetInfo()`),
// but the non-zero value is being overwritten *to* zero by `BeatmapUpdater`.
// do a bit of a hack to change it back again - otherwise tracks are going to switch instantly and we won't be able to assert anything sane anymore.
foreach (var beatmap in r.All<BeatmapInfo>().Where(b => b.Length == 0))
beatmap.Length = 60_000;
}));

AddStep("bind to track change", () =>
{
trackChangeQueue = new Queue<(IWorkingBeatmap, TrackChangeDirection)>();
Game.MusicController.TrackChanged += (working, changeDirection) => trackChangeQueue.Enqueue((working, changeDirection));
});

AddStep("press next", () => globalActionContainer.TriggerPressed(GlobalAction.MusicNext));
AddUntilStep("track changed", () => trackChangeQueue.Count == 1);

AddStep("press previous", () => globalActionContainer.TriggerPressed(GlobalAction.MusicPrev));
AddUntilStep("track changed", () =>
trackChangeQueue.Count == 2 && !trackChangeQueue.First().working.BeatmapInfo.Equals(trackChangeQueue.Last().working.BeatmapInfo));
}
}
}
95 changes: 57 additions & 38 deletions osu.Game/Overlays/MusicController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public partial class MusicController : CompositeDrawable
private readonly Bindable<RandomSelectAlgorithm> randomSelectAlgorithm = new Bindable<RandomSelectAlgorithm>();
private readonly List<Live<BeatmapSetInfo>> previousRandomSets = new List<Live<BeatmapSetInfo>>();
private int randomHistoryDirection;
private int lastRandomTrackDirection;

[BackgroundDependencyLoader]
private void load(AudioManager audio, OsuConfigManager configManager)
Expand Down Expand Up @@ -371,54 +372,72 @@ private bool next(bool allowProtectedTracks)

private Live<BeatmapSetInfo>? getNextRandom(int direction, bool allowProtectedTracks)
{
Live<BeatmapSetInfo> result;

var possibleSets = getBeatmapSets().Where(s => !s.Value.Protected || allowProtectedTracks).ToArray();
try
{
Live<BeatmapSetInfo> result;

if (possibleSets.Length == 0)
return null;
var possibleSets = getBeatmapSets().Where(s => !s.Value.Protected || allowProtectedTracks).ToArray();

// condition below checks if the signs of `randomHistoryDirection` and `direction` are opposite and not zero.
// if that is the case, it means that the user had previously chosen next track `randomHistoryDirection` times and wants to go back,
// or that the user had previously chosen previous track `randomHistoryDirection` times and wants to go forward.
// in both cases, it means that we have a history of previous random selections that we can rewind.
if (randomHistoryDirection * direction < 0)
{
Debug.Assert(Math.Abs(randomHistoryDirection) == previousRandomSets.Count);
result = previousRandomSets[^1];
previousRandomSets.RemoveAt(previousRandomSets.Count - 1);
randomHistoryDirection += direction;
return result;
}
if (possibleSets.Length == 0)
return null;

// if the early-return above didn't cover it, it means that we have no history to fall back on
// and need to actually choose something random.
switch (randomSelectAlgorithm.Value)
{
case RandomSelectAlgorithm.Random:
result = possibleSets[RNG.Next(possibleSets.Length)];
break;
// condition below checks if the signs of `randomHistoryDirection` and `direction` are opposite and not zero.
// if that is the case, it means that the user had previously chosen next track `randomHistoryDirection` times and wants to go back,
// or that the user had previously chosen previous track `randomHistoryDirection` times and wants to go forward.
// in both cases, it means that we have a history of previous random selections that we can rewind.
if (randomHistoryDirection * direction < 0)
{
Debug.Assert(Math.Abs(randomHistoryDirection) == previousRandomSets.Count);

case RandomSelectAlgorithm.RandomPermutation:
var notYetPlayedSets = possibleSets.Except(previousRandomSets).ToArray();
// if the user has been shuffling backwards and now going forwards (or vice versa),
// the topmost item from history needs to be discarded because it's the *current* track.
if (direction * lastRandomTrackDirection < 0)
{
previousRandomSets.RemoveAt(previousRandomSets.Count - 1);
randomHistoryDirection += direction;
}

if (notYetPlayedSets.Length == 0)
if (previousRandomSets.Count > 0)
{
notYetPlayedSets = possibleSets;
previousRandomSets.Clear();
randomHistoryDirection = 0;
result = previousRandomSets[^1];
previousRandomSets.RemoveAt(previousRandomSets.Count - 1);
return result;
}
}

result = notYetPlayedSets[RNG.Next(notYetPlayedSets.Length)];
break;
// if the early-return above didn't cover it, it means that we have no history to fall back on
// and need to actually choose something random.
switch (randomSelectAlgorithm.Value)
{
case RandomSelectAlgorithm.Random:
result = possibleSets[RNG.Next(possibleSets.Length)];
break;

default:
throw new ArgumentOutOfRangeException(nameof(randomSelectAlgorithm), randomSelectAlgorithm.Value, "Unsupported random select algorithm");
}
case RandomSelectAlgorithm.RandomPermutation:
var notYetPlayedSets = possibleSets.Except(previousRandomSets).ToArray();

if (notYetPlayedSets.Length == 0)
{
notYetPlayedSets = possibleSets;
previousRandomSets.Clear();
randomHistoryDirection = 0;
}

result = notYetPlayedSets[RNG.Next(notYetPlayedSets.Length)];
break;

default:
throw new ArgumentOutOfRangeException(nameof(randomSelectAlgorithm), randomSelectAlgorithm.Value, "Unsupported random select algorithm");
}

previousRandomSets.Add(result);
randomHistoryDirection += direction;
return result;
previousRandomSets.Add(result);
return result;
}
finally
{
randomHistoryDirection += direction;
lastRandomTrackDirection = direction;
}
}

private void restartTrack()
Expand Down
Loading