Skip to content

Commit f1a9be4

Browse files
author
Bartłomiej Dach
authored
Refactor audio preview logic in ranked play cards to match expectations while hopefully not looking buggy anymore (ppy#37463)
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*.) This took way too long to write.
1 parent 7b0e5ec commit f1a9be4

9 files changed

Lines changed: 59 additions & 51 deletions

File tree

‎osu.Game.Tests/Visual/RankedPlay/TestSceneSongPreview.cs‎

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
using System.Linq;
55
using NUnit.Framework;
6-
using osu.Framework.Bindables;
76
using osu.Framework.Graphics;
87
using osu.Framework.Testing;
98
using osu.Game.Online.API;
@@ -15,10 +14,10 @@ namespace osu.Game.Tests.Visual.RankedPlay
1514
{
1615
public partial class TestSceneSongPreview : RankedPlayTestScene
1716
{
18-
private readonly Bindable<bool> previewEnabled = new BindableBool(true);
19-
2017
private readonly BeatmapRequestHandler requestHandler = new BeatmapRequestHandler();
2118

19+
private PlayerHandOfCards handOfCards = null!;
20+
2221
public override void SetUpSteps()
2322
{
2423
base.SetUpSteps();
@@ -27,8 +26,6 @@ public override void SetUpSteps()
2726

2827
AddStep("add cards", () =>
2928
{
30-
PlayerHandOfCards handOfCards;
31-
3229
Child = handOfCards = new PlayerHandOfCards
3330
{
3431
RelativeSizeAxes = Axes.Both,
@@ -38,41 +35,36 @@ public override void SetUpSteps()
3835
};
3936

4037
foreach (var beatmap in requestHandler.Beatmaps.Take(3))
41-
{
42-
handOfCards.AddCard(new RevealedRankedPlayCardWithPlaylistItem(beatmap), handCard =>
43-
{
44-
handCard.Card.SongPreviewEnabled.BindTarget = previewEnabled;
45-
});
46-
}
38+
handOfCards.AddCard(new RevealedRankedPlayCardWithPlaylistItem(beatmap));
4739
});
4840

49-
AddUntilStep("load tracks", () => this.ChildrenOfType<RankedPlayCard>().All(card => card.PreviewTrackLoaded));
41+
AddUntilStep("load tracks", () => this.ChildrenOfType<RankedPlayCard>().All(card => card.SongPreview.TrackLoaded));
5042
}
5143

5244
[Test]
5345
public void TestSongPreview()
5446
{
5547
AddStep("move mouse to first card", () => InputManager.MoveMouseTo(getCard(0)));
5648

57-
AddAssert("first track running", () => getCard(0).PreviewTrackRunning);
58-
AddAssert("only one track running", () => this.ChildrenOfType<RankedPlayCard>().Count(c => c.PreviewTrackRunning) == 1);
49+
AddAssert("first track running", () => getCard(0).SongPreview.IsRunning);
50+
AddAssert("only one track running", () => this.ChildrenOfType<RankedPlayCard>().Count(c => c.SongPreview.IsRunning) == 1);
5951

6052
AddStep("move mouse to second card", () => InputManager.MoveMouseTo(getCard(1)));
53+
AddAssert("second track running", () => getCard(1).SongPreview.IsRunning);
54+
AddAssert("only one track running", () => this.ChildrenOfType<RankedPlayCard>().Count(c => c.SongPreview.IsRunning) == 1);
6155

62-
AddAssert("second track running", () => getCard(1).PreviewTrackRunning);
63-
AddAssert("only one track running", () => this.ChildrenOfType<RankedPlayCard>().Count(c => c.PreviewTrackRunning) == 1);
64-
65-
AddStep("disable preview", () => previewEnabled.Value = false);
66-
67-
AddAssert("no tracks running", () => !this.ChildrenOfType<RankedPlayCard>().Any(c => c.PreviewTrackRunning));
56+
AddStep("disable preview", () => handOfCards.CurrentPlayingPreview.Value = null);
57+
AddAssert("no tracks running", () => !this.ChildrenOfType<RankedPlayCard>().Any(c => c.SongPreview.IsRunning));
6858

6959
AddStep("move mouse to third card", () => InputManager.MoveMouseTo(getCard(2)));
60+
AddAssert("third track running", () => getCard(2).SongPreview.IsRunning);
7061

71-
AddAssert("no tracks running", () => !this.ChildrenOfType<RankedPlayCard>().Any(c => c.PreviewTrackRunning));
62+
AddStep("move mouse away", () => InputManager.MoveMouseTo(Vector2.Zero));
63+
AddAssert("third track running", () => getCard(2).SongPreview.IsRunning);
7264

73-
AddStep("enable preview", () => previewEnabled.Value = true);
74-
75-
AddAssert("third track running", () => getCard(2).PreviewTrackRunning);
65+
AddStep("move mouse to second card", () => InputManager.MoveMouseTo(getCard(1)));
66+
AddAssert("second track running", () => getCard(1).SongPreview.IsRunning);
67+
AddAssert("only one track running", () => this.ChildrenOfType<RankedPlayCard>().Count(c => c.SongPreview.IsRunning) == 1);
7668
}
7769

7870
private RankedPlayCard getCard(int index) => this.ChildrenOfType<RankedPlayCard>().ElementAt(index);

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Card/RankedPlayCard.SongPreview.cs‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,6 @@ public partial class SongPreviewContainer : Container, IBeatSyncProvider
3131
{
3232
private const double minimum_beat_length = 800;
3333

34-
public readonly Bindable<bool> Enabled = new BindableBool(true);
35-
36-
public readonly Bindable<bool> CardHovered = new BindableBool(true);
37-
3834
public bool TrackLoaded => previewTrack?.TrackLoaded ?? false;
3935

4036
public bool IsRunning => previewTrack?.IsRunning ?? false;
@@ -51,6 +47,8 @@ public partial class SongPreviewContainer : Container, IBeatSyncProvider
5147
[Resolved]
5248
private OsuColour colours { get; set; } = null!;
5349

50+
public readonly IBindable<SongPreviewContainer?> CurrentPlayingPreview = new Bindable<SongPreviewContainer?>();
51+
5452
public SongPreviewContainer()
5553
{
5654
InternalChildren =
@@ -112,7 +110,7 @@ private void updatePlayingState()
112110
if (previewTrack?.IsLoaded != true)
113111
return;
114112

115-
bool shouldBePlaying = Enabled.Value && CardHovered.Value;
113+
bool shouldBePlaying = CurrentPlayingPreview.Value == this;
116114

117115
if (shouldBePlaying == previewTrack.IsRunning)
118116
return;

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Card/RankedPlayCard.cs‎

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -31,29 +31,19 @@ public partial class RankedPlayCard : CompositeDrawable
3131

3232
private readonly IBindable<MultiplayerPlaylistItem?> playlistItem;
3333

34-
public readonly Bindable<bool> SongPreviewEnabled = new BindableBool(true);
35-
3634
private readonly Container content;
3735
private readonly Container cardContent;
3836
private readonly Container shadow;
3937
private readonly SelectionOutline selectionOutline;
40-
private readonly SongPreviewContainer songPreviewContainer;
38+
public readonly SongPreviewContainer SongPreview;
4139

4240
public bool ShowSelectionOutline
4341
{
4442
set => selectionOutline.FadeTo(value ? 1 : 0, 50);
4543
}
4644

47-
public bool PlayAudioPreview
48-
{
49-
set => songPreviewContainer.CardHovered.Value = value;
50-
}
51-
5245
public float Elevation;
5346

54-
public bool PreviewTrackLoaded => songPreviewContainer.TrackLoaded;
55-
public bool PreviewTrackRunning => songPreviewContainer.IsRunning;
56-
5747
private Sample? cardFlipSample;
5848

5949
[Resolved]
@@ -67,9 +57,8 @@ public RankedPlayCard(RankedPlayCardWithPlaylistItem item)
6757

6858
playlistItem = item.PlaylistItem.GetBoundCopy();
6959

70-
InternalChild = songPreviewContainer = new SongPreviewContainer
60+
InternalChild = SongPreview = new SongPreviewContainer
7161
{
72-
Enabled = { BindTarget = SongPreviewEnabled },
7362
RelativeSizeAxes = Axes.Both,
7463
Anchor = Anchor.Centre,
7564
Origin = Anchor.Centre,
@@ -173,7 +162,7 @@ private void loadCardContentAsync(MultiplayerPlaylistItem playlistItem) => Task.
173162
Schedule(() =>
174163
{
175164
SetContent(new RankedPlayCardContent(beatmap));
176-
songPreviewContainer.LoadPreview(beatmap);
165+
SongPreview.LoadPreview(beatmap);
177166
});
178167
});
179168

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/DiscardScreen.cs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -253,13 +253,14 @@ private void playDiscardAnimation()
253253

254254
foreach (var item in discardedCards)
255255
{
256-
if (!playerHand.RemoveCard(item, out var card, out Quad drawQuad))
256+
if (!playerHand.DetachCard(item, out var card, out Quad drawQuad))
257257
return;
258258

259259
card.Anchor = Anchor.Centre;
260260
card.Origin = Anchor.Centre;
261261

262-
card.SongPreviewEnabled.Value = false;
262+
if (playerHand.CurrentPlayingPreview.Value == card.SongPreview)
263+
playerHand.CurrentPlayingPreview.Value = null;
263264

264265
card.MatchScreenSpaceDrawQuad(drawQuad, CenterRow);
265266

@@ -333,7 +334,7 @@ private void presentRemainingCards()
333334

334335
foreach (var item in matchInfo.PlayerCards)
335336
{
336-
if (playerHand.RemoveCard(item, out var card, out Quad drawQuad))
337+
if (playerHand.DetachCard(item, out var card, out Quad drawQuad))
337338
{
338339
card.MatchScreenSpaceDrawQuad(drawQuad, CenterRow);
339340

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandOfCards.HandCard.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,6 @@ protected virtual void OnStateChanged(ValueChangedEvent<RankedPlayCardState> sta
104104
handOfCards.OnCardStateChanged(this, state);
105105

106106
Card.ShowSelectionOutline = state.NewValue.Selected;
107-
Card.PlayAudioPreview = state.NewValue.Hovered;
108107

109108
switch (state.NewValue.Pressed, state.OldValue.Pressed)
110109
{

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/HandOfCards.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ public void Contract()
113113

114114
public void AddCard(RankedPlayCardWithPlaylistItem item, Action<HandCard>? setupAction = null) => AddCard(new RankedPlayCard(item), setupAction);
115115

116-
public void AddCard(RankedPlayCard card, Action<HandCard>? setupAction = null)
116+
public virtual void AddCard(RankedPlayCard card, Action<HandCard>? setupAction = null)
117117
{
118118
if (cardLookup.ContainsKey(card.Item.Card))
119119
return;
@@ -159,7 +159,7 @@ public bool RemoveCard(RankedPlayCardWithPlaylistItem item)
159159
/// <param name="card">Contained <see cref="RankedPlayCard"/></param>
160160
/// <param name="screenSpaceDrawQuad"><see cref="Drawable.ScreenSpaceDrawQuad"/> of the removed card</param>
161161
/// <returns>Whether a card was found for the provided item</returns>
162-
public bool RemoveCard(RankedPlayCardWithPlaylistItem item, [MaybeNullWhen(false)] out RankedPlayCard card, out Quad screenSpaceDrawQuad)
162+
public virtual bool DetachCard(RankedPlayCardWithPlaylistItem item, [MaybeNullWhen(false)] out RankedPlayCard card, out Quad screenSpaceDrawQuad)
163163
{
164164
if (!cardLookup.Remove(item.Card, out var drawable))
165165
{

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/Hand/PlayerHandOfCards.cs‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,13 @@
44
using System;
55
using System.Collections.Generic;
66
using System.Diagnostics;
7+
using System.Diagnostics.CodeAnalysis;
78
using System.Linq;
89
using osu.Framework.Allocation;
910
using osu.Framework.Audio;
1011
using osu.Framework.Audio.Sample;
1112
using osu.Framework.Bindables;
13+
using osu.Framework.Graphics.Primitives;
1214
using osu.Framework.Input.Events;
1315
using osu.Game.Audio;
1416
using osu.Game.Online.RankedPlay;
@@ -82,6 +84,17 @@ public Action? PlayCardAction
8284
/// </summary>
8385
public IEnumerable<RankedPlayCardWithPlaylistItem> Selection => selection.Select(it => it.Card.Item);
8486

87+
/// <summary>
88+
/// The currently-playing preview.
89+
/// </summary>
90+
/// <remarks>
91+
/// Note that due to how cards get detached and handed off between multiple <see cref="HandOfCards"/> instances and non-hand containers,
92+
/// DI cannot be reliably used to pass this bindable down to children
93+
/// because it'll only work for the first <see cref="PlayerHandOfCards"/> that <see cref="RankedPlayCard"/>s bind to.
94+
/// Instead, <see cref="AddCard"/> and <see cref="DetachCard"/> overrides in this class set up this bindable manually as cards are handed off between stages.
95+
/// </remarks>
96+
public readonly Bindable<RankedPlayCard.SongPreviewContainer?> CurrentPlayingPreview = new Bindable<RankedPlayCard.SongPreviewContainer?>();
97+
8598
private readonly BindableBool allowSelection = new BindableBool();
8699

87100
private const int select_samples = 1;
@@ -110,6 +123,19 @@ private void load(AudioManager audio)
110123
PlayAction = PlayCardAction,
111124
};
112125

126+
public override void AddCard(RankedPlayCard card, Action<HandCard>? setupAction = null)
127+
{
128+
base.AddCard(card, setupAction);
129+
card.SongPreview.CurrentPlayingPreview.BindTo(CurrentPlayingPreview);
130+
}
131+
132+
public override bool DetachCard(RankedPlayCardWithPlaylistItem item, [MaybeNullWhen(false)] out RankedPlayCard card, out Quad screenSpaceDrawQuad)
133+
{
134+
bool result = base.DetachCard(item, out card, out screenSpaceDrawQuad);
135+
card?.SongPreview.CurrentPlayingPreview.UnbindFrom(CurrentPlayingPreview);
136+
return result;
137+
}
138+
113139
private void cardClicked(PlayerHandCard card)
114140
{
115141
if (selectionMode == HandSelectionMode.Disabled)
@@ -145,6 +171,9 @@ protected override void OnCardStateChanged(HandCard card, ValueChangedEvent<Rank
145171
{
146172
StateChanged?.Invoke();
147173

174+
if (evt.NewValue.Hovered)
175+
CurrentPlayingPreview.Value = card.Card.SongPreview;
176+
148177
base.OnCardStateChanged(card, evt);
149178
}
150179

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/OpponentPickScreen.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ private void cardPlayed(RankedPlayCardWithPlaylistItem item) => Task.Run(async (
155155
{
156156
RankedPlayCard? card;
157157

158-
if (opponentHand.RemoveCard(item, out card, out var drawQuad))
158+
if (opponentHand.DetachCard(item, out card, out var drawQuad))
159159
{
160160
card.MatchScreenSpaceDrawQuad(drawQuad, CenterRow);
161161
}

‎osu.Game/Screens/OnlinePlay/Matchmaking/RankedPlay/PickScreen.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ private void cardPlayed(RankedPlayCardWithPlaylistItem item)
237237
{
238238
RankedPlayCard? card;
239239

240-
if (playerHand.RemoveCard(item, out card, out var drawQuad))
240+
if (playerHand.DetachCard(item, out card, out var drawQuad))
241241
{
242242
card.MatchScreenSpaceDrawQuad(drawQuad, CenterRow);
243243
}

0 commit comments

Comments
 (0)