From 25ab3a5feafcd037bc475aeea1c3d8119185e70b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Oct 2020 22:10:37 +0900 Subject: [PATCH 1/6] Construct replay after being sure a ruleset is available to avoid nullrefs --- osu.Game/Screens/Play/Spectator.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Play/Spectator.cs b/osu.Game/Screens/Play/Spectator.cs index 51cd5b59aa..6a11aeb0e9 100644 --- a/osu.Game/Screens/Play/Spectator.cs +++ b/osu.Game/Screens/Play/Spectator.cs @@ -160,8 +160,6 @@ namespace osu.Game.Screens.Play if (userId != targetUser.Id) return; - replay ??= new Replay { HasReceivedAllFrames = false }; - this.state = state; Schedule(attemptStart); @@ -197,6 +195,8 @@ namespace osu.Game.Screens.Play return; } + replay ??= new Replay { HasReceivedAllFrames = false }; + var scoreInfo = new ScoreInfo { Beatmap = resolvedBeatmap, From 5d02de29ca1476a4989ecf74ee6583e32dd12005 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Oct 2020 22:50:45 +0900 Subject: [PATCH 2/6] Fix attempt to change ruleset/beatmap bindables while screen is not active --- osu.Game/Screens/Play/Spectator.cs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/Play/Spectator.cs b/osu.Game/Screens/Play/Spectator.cs index 6a11aeb0e9..2f65dc06d0 100644 --- a/osu.Game/Screens/Play/Spectator.cs +++ b/osu.Game/Screens/Play/Spectator.cs @@ -63,6 +63,11 @@ namespace osu.Game.Screens.Play private IBindable> managerUpdated; + /// + /// Becomes true if a new state is waiting to be loaded (while this screen was not active). + /// + private bool newStatePending; + public Spectator([NotNull] User targetUser) { this.targetUser = targetUser ?? throw new ArgumentNullException(nameof(targetUser)); @@ -162,7 +167,21 @@ namespace osu.Game.Screens.Play this.state = state; - Schedule(attemptStart); + if (this.IsCurrentScreen()) + Schedule(attemptStart); + else + newStatePending = true; + } + + public override void OnResuming(IScreen last) + { + base.OnResuming(last); + + if (newStatePending) + { + attemptStart(); + newStatePending = false; + } } private void userFinishedPlaying(int userId, SpectatorState state) From 8bbcb9be8a7a8bd11b8192f7ee0c0722fe96b62d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Oct 2020 22:50:57 +0900 Subject: [PATCH 3/6] Always use imported beatmap in tests --- .../Visual/Gameplay/TestSceneSpectator.cs | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 5a2230dd64..b4ab22cfad 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -36,13 +36,21 @@ namespace osu.Game.Tests.Visual.Gameplay private int nextFrame; + private BeatmapSetInfo importedBeatmap; + + private int importedBeatmapId; + public override void SetUpSteps() { base.SetUpSteps(); AddStep("reset sent frames", () => nextFrame = 0); - AddStep("import beatmap", () => ImportBeatmapTest.LoadOszIntoOsu(game, virtualTrack: true).Wait()); + AddStep("import beatmap", () => + { + importedBeatmap = ImportBeatmapTest.LoadOszIntoOsu(game, virtualTrack: true).Result; + importedBeatmapId = importedBeatmap.Beatmaps.First(b => b.RulesetID == 0).OnlineBeatmapID ?? -1; + }); AddStep("add streaming client", () => { @@ -115,6 +123,7 @@ namespace osu.Game.Tests.Visual.Gameplay start(); sendFrames(); + start(); sendFrames(); } @@ -157,7 +166,7 @@ namespace osu.Game.Tests.Visual.Gameplay private void waitForPlayer() => AddUntilStep("wait for player", () => Stack.CurrentScreen is Player); - private void start(int? beatmapId = null) => AddStep("start play", () => testSpectatorStreamingClient.StartPlay(beatmapId)); + private void start(int? beatmapId = null) => AddStep("start play", () => testSpectatorStreamingClient.StartPlay(beatmapId ?? importedBeatmapId)); private void checkPaused(bool state) => AddAssert($"game is {(state ? "paused" : "playing")}", () => player.ChildrenOfType().First().IsPaused.Value == state); @@ -179,18 +188,21 @@ namespace osu.Game.Tests.Visual.Gameplay internal class TestSpectatorStreamingClient : SpectatorStreamingClient { - [Resolved] - private BeatmapManager beatmaps { get; set; } - public readonly User StreamingUser = new User { Id = 1234, Username = "Test user" }; - public void StartPlay(int? beatmapId = null) => sendState(beatmapId); + private int beatmapId; - public void EndPlay() + public void StartPlay(int beatmapId) + { + this.beatmapId = beatmapId; + sendState(beatmapId); + } + + public void EndPlay(int beatmapId) { ((ISpectatorClient)this).UserFinishedPlaying((int)StreamingUser.Id, new SpectatorState { - BeatmapID = beatmaps.GetAllUsableBeatmapSets().First().Beatmaps.First(b => b.RulesetID == 0).OnlineBeatmapID, + BeatmapID = beatmapId, RulesetID = 0, }); } @@ -212,7 +224,7 @@ namespace osu.Game.Tests.Visual.Gameplay ((ISpectatorClient)this).UserSentFrames((int)StreamingUser.Id, bundle); if (!sentState) - sendState(); + sendState(beatmapId); } public override void WatchUser(int userId) @@ -220,18 +232,18 @@ namespace osu.Game.Tests.Visual.Gameplay if (sentState) { // usually the server would do this. - sendState(); + sendState(beatmapId); } base.WatchUser(userId); } - private void sendState(int? beatmapId = null) + private void sendState(int beatmapId) { sentState = true; ((ISpectatorClient)this).UserBeganPlaying((int)StreamingUser.Id, new SpectatorState { - BeatmapID = beatmapId ?? beatmaps.GetAllUsableBeatmapSets().First().Beatmaps.First(b => b.RulesetID == 0).OnlineBeatmapID, + BeatmapID = beatmapId, RulesetID = 0, }); } From 1d499ec15d5ce31cd05337f9ead39a33562f267e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Oct 2020 22:51:35 +0900 Subject: [PATCH 4/6] Change beatmap not existing test to specify a beatmap ID that can't possibly exist --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index b4ab22cfad..0b530a303f 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -158,7 +158,7 @@ namespace osu.Game.Tests.Visual.Gameplay { loadSpectatingScreen(); - start(88); + start(-1234); sendFrames(); AddAssert("screen didn't change", () => Stack.CurrentScreen is Spectator); From 7cc4a7cb5cdd419af637f6bcd1e631bc8321f6ad Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Oct 2020 22:59:54 +0900 Subject: [PATCH 5/6] Add more accurate fail scenario test logic --- .../Visual/Gameplay/TestSceneSpectator.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 0b530a303f..7dde493b1a 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -94,7 +94,7 @@ namespace osu.Game.Tests.Visual.Gameplay start(); waitForPlayer(); - AddUntilStep("game is paused", () => player.ChildrenOfType().First().IsPaused.Value); + checkPaused(true); sendFrames(); @@ -134,8 +134,13 @@ namespace osu.Game.Tests.Visual.Gameplay loadSpectatingScreen(); start(); - sendFrames(); + waitForPlayer(); + checkPaused(true); + + finish(); + + checkPaused(false); // TODO: should replay until running out of frames then fail } @@ -168,8 +173,10 @@ namespace osu.Game.Tests.Visual.Gameplay private void start(int? beatmapId = null) => AddStep("start play", () => testSpectatorStreamingClient.StartPlay(beatmapId ?? importedBeatmapId)); + private void finish(int? beatmapId = null) => AddStep("end play", () => testSpectatorStreamingClient.EndPlay(beatmapId ?? importedBeatmapId)); + private void checkPaused(bool state) => - AddAssert($"game is {(state ? "paused" : "playing")}", () => player.ChildrenOfType().First().IsPaused.Value == state); + AddUntilStep($"game is {(state ? "paused" : "playing")}", () => player.ChildrenOfType().First().IsPaused.Value == state); private void sendFrames(int count = 10) { From 6c2cee7b3fb187348fc099617bbdde37966404a0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Oct 2020 23:13:42 +0900 Subject: [PATCH 6/6] Avoid cross-pollution between tests of current playing state --- osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs index 7dde493b1a..6485cbdad3 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneSpectator.cs @@ -57,6 +57,8 @@ namespace osu.Game.Tests.Visual.Gameplay Remove(testSpectatorStreamingClient); Add(testSpectatorStreamingClient); }); + + finish(); } private OsuFramedReplayInputHandler replayHandler => @@ -212,6 +214,8 @@ namespace osu.Game.Tests.Visual.Gameplay BeatmapID = beatmapId, RulesetID = 0, }); + + sentState = false; } private bool sentState;