Refactor audio preview logic in ranked play cards to match expectations while hopefully not looking buggy anymore#37463
Merged
peppy merged 2 commits intoppy:masterfrom Apr 22, 2026
Conversation
bdach
added
code quality
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ns while hopefully not looking buggy anymore RFC. See ppy#37453 (comment) for why. Of note: - To facilitate mutual exclusivity of playback `PlayerHandOfCards` maintains a bindable pointing at the currently playing song preview. - Because of how card drawables are passed between multiple parenting drawables, some of which are and some of which are not `PlayerHandOfCards` instances, DI fails horribly at working with this bindable unless it is manually managed. See relevant overrides in `PlayerHandOfCards`. - I renamed one of the overloads of `HandOfCards.RemoveCard()` to `DetachCard()` because I found the fact that there are two overloads of one method that do WILDLY DIFFERENT THINGS utterly *asinine*. (One overload scrapes the `RankedPlayCard` out for you to plop elsewhere. One *drops it on the floor entirely*.)
bdach
force-pushed
the
rp-card-preview-behaviour
branch
from
d565812 to
09428ad
Compare
peppy
reviewed
Apr 22, 2026
| public override void AddCard(RankedPlayCard card, Action<HandCard>? setupAction = null) | ||
| { | ||
| base.AddCard(card, setupAction); | ||
| card.SongPreview.CurrentPlayingPreview.BindTo(CurrentPlayingPreview); |
peppy
approved these changes
Apr 22, 2026
| card.Origin = Anchor.Centre; | ||
|
|
||
| card.SongPreviewEnabled.Value = false; | ||
| if (playerHand.CurrentPlayingPreview.Value == card.SongPreview) |
Member
There was a problem hiding this comment.
I guess I get the reason for this existing here and not in DetachCard (there are cases we want to keep playing the preview even after detaching) but it's not so pretty to read. A lot of two level deep bindable mutation and binding by multiple components does my head in.
peppy
approved these changes
Apr 22, 2026
github-project-automation
Bot
moved this from Pending Review
to Done
in osu! team task tracker
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>
Copilot AI
pushed a commit
to winnerspiros/osu
that referenced
this pull request
Apr 22, 2026
…37477) - Merge upstream ppy/osu master through commit a4f79f7 (PR ppy#37477: ranked play single-thread audio fix, supersedes/reverts ppy#37463). - Resolve conflict in RankedPlayCard.SongPreview.cs in favour of our fork's existing late-bind approach (Enabled/CardHovered subscribed inside LoadComponentAsync callback so they never fire pre-load). - Pick up the new TestPreviewStopsOnEnteringGameplay regression test. Co-authored-by: winnerspiros <1675249+winnerspiros@users.noreply.github.com>
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
3 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.
RFC.
See #37453 (comment) for why.
Of note:
PlayerHandOfCardsmaintains a bindable pointing at the currently playing song preview.PlayerHandOfCardsinstances, DI fails horribly at working with this bindable unless it is manually managed. See relevant overrides inPlayerHandOfCards.HandOfCards.RemoveCard()toDetachCard()because I found the fact that there are two overloads of one method that do WILDLY DIFFERENT THINGS utterly asinine. (One overload scrapes theRankedPlayCardout for you to plop elsewhere. One drops it on the floor entirely.)This took way too long to write.