Attempt to fix ranked play preview tracks not stopping when entering gameplay#37473
Closed
bdach wants to merge 3 commits intoppy:masterfrom
Closed
Attempt to fix ranked play preview tracks not stopping when entering gameplay#37473bdach wants to merge 3 commits intoppy:masterfrom
bdach wants to merge 3 commits intoppy:masterfrom
Conversation
Closes ppy#37471 probably. This is a dogshit change but I have no better. This regressed in ppy#37453 because of trying to work around the framework bug which makes attempting to start or stop the track in certain threading scenarios kill the game. Changing the playback logic to be `Update()`-based broke `.StopAnyPlaying()` calls. In particular this one. https://github.com/ppy/osu/blob/71356a9455e0738b942371e3f78134e8b14ec7ab/osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs#L361 So to fix attempt to manually check whether the ranked play screen is still current in the update loop jank so that it doesn't restart the track if it was stopped by the screen suspending. How confident am I that this won't break in some eldritch threading scenario? Not very! I can sprinkle a `StopAnyPlaying()` in `MultiplayerPlayerLoader` if it helps. That one *should* theoretically be safer. But also double the dogshit. Note that I previously tried to fix the framework bug that causes all this in ppy/osu-framework#6727. That PR was briefly reverted in ppy/osu-framework#6728 because it started failing CI game-side. Anyone thinking they can do better is cordially invited to try. The other choice is just reverting all of my attempts and just accepting that players on single thread shall be indiscriminately and randomly smited for trying to play on single thread.
peppy
added a commit
that referenced
this pull request
Apr 22, 2026
…ues (#37477) Thing to make release happen. Reverts #37453 Reverts #37463 Alternative to #37473 Not that I disagree with any of these but I'm just looking to return to what works so we can do a release because we're on a clock here for other reasons. Test which should work but doesn't, so I'm not adding: ```diff diff --git a/osu.Game.Tests/Visual/RankedPlay/TestSceneOpponentPickScreen.cs b/osu.Game.Tests/Visual/RankedPlay/TestSceneOpponentPickScreen.cs index f747004..eb8e360d1e 100644 --- a/osu.Game.Tests/Visual/RankedPlay/TestSceneOpponentPickScreen.cs +++ b/osu.Game.Tests/Visual/RankedPlay/TestSceneOpponentPickScreen.cs @@ -1,12 +1,17 @@ // 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 NUnit.Framework; using osu.Framework.Extensions; +using osu.Framework.Testing; using osu.Game.Online.API; using osu.Game.Online.Multiplayer; using osu.Game.Online.Multiplayer.MatchTypes.RankedPlay; using osu.Game.Online.Rooms; using osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay; +using osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Card; +using osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand; namespace osu.Game.Tests.Visual.RankedPlay { @@ -14,6 +19,8 @@ public partial class TestSceneOpponentPickScreen : RankedPlayTestScene { private RankedPlayScreen screen = null!; + private readonly BeatmapRequestHandler requestHandler = new BeatmapRequestHandler(); + public override void SetUpSteps() { base.SetUpSteps(); @@ -26,8 +33,6 @@ public override void SetUpSteps() AddStep("load screen", () => LoadScreen(screen = new RankedPlayScreen(MultiplayerClient.ClientRoom!))); AddUntilStep("screen loaded", () => screen.IsLoaded); - var requestHandler = new BeatmapRequestHandler(); - AddStep("setup request handler", () => ((DummyAPIAccess)API).HandleRequest = requestHandler.HandleRequest); AddStep("set pick state", () => MultiplayerClient.RankedPlayChangeStage(RankedPlayStage.CardPlay, state => state.ActiveUserId = 2).WaitSafely()); @@ -44,7 +49,11 @@ public override void SetUpSteps() }).WaitSafely(); } }); + } + [Test] + public void TestBasic() + { AddWaitStep("wait", 15); AddStep("play beatmap", () => MultiplayerClient.PlayUserCard(2, hand => hand[0]).WaitSafely()); @@ -54,5 +63,29 @@ public override void SetUpSteps() BeatmapID = requestHandler.Beatmaps[0].OnlineID }).WaitSafely()); } + + [Test] + public void TestPickPreviewPlayedOnOpponentPick() + { + RankedPlayCard.SongPreviewContainer? originalPreview = null; + + AddStep("hover first card", + () => InputManager.MoveMouseTo(this.ChildrenOfType<PlayerHandOfCards>().Single().Cards + .First(c => c.Item.PlaylistItem.Value != null && c.Item.PlaylistItem.Value.BeatmapID != requestHandler.Beatmaps[0].OnlineID))); + AddUntilStep("preview playing", () => originalPreview = this.ChildrenOfType<RankedPlayCard.SongPreviewContainer>().FirstOrDefault(p => p.IsRunning), () => Is.Not.Null); + + AddStep("play beatmap", () => MultiplayerClient.PlayUserCard(2, hand => hand[0]).WaitSafely()); + AddStep("reveal card", () => MultiplayerClient.RankedPlayRevealUserCard(2, hand => hand[0], new MultiplayerPlaylistItem + { + ID = 0, + BeatmapID = requestHandler.Beatmaps[0].OnlineID + }).WaitSafely()); + + AddUntilStep("wait for original preview stopped", () => originalPreview?.IsRunning, () => Is.False); + + AddUntilStep("preview playing is opponent's pick", + () => ((RankedPlayCard)this.ChildrenOfType<RankedPlayCard.SongPreviewContainer>().SingleOrDefault(p => p.IsRunning)?.Parent!).Item.PlaylistItem.Value?.BeatmapID, + () => Is.EqualTo(requestHandler.Beatmaps[0].OnlineID)); + } } } ``` --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
github-project-automation
Bot
moved this from Pending Review
to Done
in osu! team task tracker
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #37471 probably.
This is a dogshit change but I have no better.
This regressed in #37453 because of trying to work around the framework bug which makes attempting to start or stop the track in certain threading scenarios kill the game.
Changing the playback logic to be
Update()-based broke.StopAnyPlaying()calls. In particular this one.osu/osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/RankedPlayScreen.cs
Line 361 in 71356a9
So to fix attempt to manually check whether the ranked play screen is still current in the update loop jank so that it doesn't restart the track if it was stopped by the screen suspending.
How confident am I that this won't break in some eldritch threading scenario? Not very!
I can sprinkle aI realise now that that wouldn't even work.StopAnyPlaying()inMultiplayerPlayerLoaderif it helps. That one should theoretically be safer. But also double the dogshit.Note that I previously tried to fix the framework bug that causes all this in ppy/osu-framework#6727. That PR was briefly reverted in ppy/osu-framework#6728 because it started failing CI game-side due to some other audio-related safety issue that I haven't investigated yet but @smoogipoo apparently has vague awareness of.
Anyone thinking they can do better is cordially invited to try.
The other choice is just reverting all of my attempts and just accepting that players on single thread shall be indiscriminately and randomly smited for trying to play on single thread.