Skip to content

Commit 7b0e5ec

Browse files
minetoblendBartłomiej Dach
minetoblend
and
Bartłomiej Dach
authored
Code quality improvements for child/draw order handling in HandOfCards (ppy#37423)
Attempt at adressing the points made in ppy#37419 (comment) - `CardContainer` is now being sorted immediately instead of only doing it once per frame. Given that its only ever gonna have a handful of children there wasn't really a need to optimize that that in the first place. - `HandOfCards.Cards` now exposes `cardLookup.Values` as an `IEnumerable` instead of exposing the card container's children directly. - `HandOfCard` now exposes `GetCardsInDisplayOrder` which returns a copy of all cards in display order. Since it's making a copy I made sure this isn't called on any hot code paths. - `HandOfCard.Clear` previously didn't clear the `cardLookup` dictionary. Didn't cause any issues since we're not re-adding cards to the hand anywhere but not good regardless. Switched to looping over all cards and calling `RemoveCard` to make sure changes to the removal logic can't get overlooked there again. --------- Co-authored-by: Bartłomiej Dach <dach.bartlomiej@gmail.com>
1 parent 71b3d51 commit 7b0e5ec

3 files changed

Lines changed: 68 additions & 67 deletions

File tree

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

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,14 @@ public void TestSingleSelectionMode()
5656
});
5757
AddStep("single selection mode", () => handOfCards.SelectionMode = HandSelectionMode.Single);
5858

59-
AddStep("click first card", () => handOfCards.Cards.First().TriggerClick());
60-
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.First().Item]));
59+
AddStep("click first card", () => handOfCards.GetCardsInDisplayOrder()[0].TriggerClick());
60+
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
6161

62-
AddStep("click second card", () => handOfCards.Cards.ElementAt(1).TriggerClick());
63-
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(1).Item]));
62+
AddStep("click second card", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
63+
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[1].Item]));
6464

65-
AddStep("click second card again", () => handOfCards.Cards.ElementAt(1).TriggerClick());
66-
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(1).Item]));
65+
AddStep("click second card again", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
66+
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[1].Item]));
6767
}
6868

6969
[Test]
@@ -76,14 +76,14 @@ public void TestMultiSelectionMode()
7676
});
7777
AddStep("multi selection mode", () => handOfCards.SelectionMode = HandSelectionMode.Multiple);
7878

79-
AddStep("click first card", () => handOfCards.Cards.First().TriggerClick());
80-
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.First().Item]));
79+
AddStep("click first card", () => handOfCards.GetCardsInDisplayOrder().First().TriggerClick());
80+
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder().First().Item]));
8181

82-
AddStep("click second card", () => handOfCards.Cards.ElementAt(1).TriggerClick());
83-
AddAssert("both cards selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item, handOfCards.Cards.ElementAt(1).Item]));
82+
AddStep("click second card", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
83+
AddAssert("both cards selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item, handOfCards.GetCardsInDisplayOrder()[1].Item]));
8484

85-
AddStep("click second card again", () => handOfCards.Cards.ElementAt(1).TriggerClick());
86-
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item]));
85+
AddStep("click second card again", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
86+
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
8787
}
8888

8989
[Test]
@@ -131,20 +131,20 @@ public void TestKeyboardSelectionSingleSelection()
131131
Key key = Key.Number1 + i;
132132

133133
AddStep($"key {i + 1}", () => InputManager.Key(key));
134-
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(i1).Item]));
134+
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[i1].Item]));
135135
}
136136

137137
AddStep("right arrow", () => InputManager.Key(Key.Right));
138-
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item]));
138+
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
139139

140140
AddStep("right arrow", () => InputManager.Key(Key.Right));
141-
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(1).Item]));
141+
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[1].Item]));
142142

143143
AddStep("left arrow", () => InputManager.Key(Key.Left));
144-
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item]));
144+
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
145145

146146
AddStep("left arrow", () => InputManager.Key(Key.Left));
147-
AddAssert("last card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(^1).Item]));
147+
AddAssert("last card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[^1].Item]));
148148

149149
AddStep("space", () => InputManager.Key(Key.Space));
150150
AddAssert("play action triggered", () => playActionTriggered);
@@ -166,11 +166,11 @@ public void TestKeyboardSelectionMultiSelection()
166166
Key key = Key.Number1 + i;
167167

168168
AddStep($"key {i + 1}", () => InputManager.Key(key));
169-
AddAssert("card hovered", () => handOfCards.Cards.ElementAt(i1).CardHovered);
169+
AddAssert("card hovered", () => handOfCards.GetCardsInDisplayOrder()[i1].CardHovered);
170170

171-
AddAssert("card not selected", () => !handOfCards.Selection.Contains(handOfCards.Cards.ElementAt(i1).Card.Item));
171+
AddAssert("card not selected", () => !handOfCards.Selection.Contains(handOfCards.GetCardsInDisplayOrder()[i1].Card.Item));
172172
AddStep("space", () => InputManager.Key(Key.Space));
173-
AddAssert("card selected", () => handOfCards.Selection.Contains(handOfCards.Cards.ElementAt(i1).Card.Item));
173+
AddAssert("card selected", () => handOfCards.Selection.Contains(handOfCards.GetCardsInDisplayOrder()[i1].Card.Item));
174174
}
175175
}
176176

@@ -201,9 +201,9 @@ public void TestRemoveCardsWhileDragging()
201201
for (int i = 0; i < 5; i++)
202202
handOfCards.AddCard(new RankedPlayCardWithPlaylistItem(new RankedPlayCardItem()));
203203
});
204-
AddStep("hover card", () => InputManager.MoveMouseTo(handOfCards.Cards.First()));
204+
AddStep("hover card", () => InputManager.MoveMouseTo(handOfCards.GetCardsInDisplayOrder()[0]));
205205
AddStep("start drag", () => InputManager.PressButton(MouseButton.Left));
206-
AddStep("move card", () => InputManager.MoveMouseTo(handOfCards.Cards[3]));
206+
AddStep("move card", () => InputManager.MoveMouseTo(handOfCards.GetCardsInDisplayOrder()[3]));
207207
AddStep("remove cards", () =>
208208
{
209209
foreach (var card in handOfCards.Cards.ToArray())

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

Lines changed: 27 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
using System.Linq;
88
using osu.Framework.Allocation;
99
using osu.Framework.Bindables;
10-
using osu.Framework.Caching;
1110
using osu.Framework.Graphics;
1211
using osu.Framework.Graphics.Containers;
1312
using osu.Framework.Graphics.Primitives;
@@ -29,7 +28,18 @@ public abstract partial class HandOfCards : CompositeDrawable
2928

3029
private const float card_spacing = -15;
3130

32-
public IReadOnlyList<HandCard> Cards => cardContainer.Children;
31+
/// <summary>
32+
/// Cards currently present in this <see cref="HandOfCards"/>
33+
/// </summary>
34+
/// <remarks>
35+
/// Entries are not sorted by display order
36+
/// </remarks>
37+
public IEnumerable<HandCard> Cards => cardLookup.Values;
38+
39+
/// <summary>
40+
/// Returns a list of the cards present in this <see cref="HandOfCards"/> ordered by the cards' <see cref="HandCard.Order"/>
41+
/// </summary>
42+
public List<HandCard> GetCardsInDisplayOrder() => Cards.OrderBy(static c => c.Order).ToList();
3343

3444
/// <summary>
3545
/// How far a card slides upwards when hovered.
@@ -66,12 +76,6 @@ protected override void Update()
6676
{
6777
base.Update();
6878

69-
if (!drawOrderBacking.IsValid)
70-
{
71-
cardContainer.Sort();
72-
drawOrderBacking.Validate();
73-
}
74-
7579
if (!layoutBacking.IsValid)
7680
{
7781
updateLayout();
@@ -126,23 +130,25 @@ public void AddCard(RankedPlayCard card, Action<HandCard>? setupAction = null)
126130
drawable.Order = cardContainer.Max(c => c.Order) + 1;
127131

128132
cardContainer.Add(drawable);
129-
InvalidateLayout(drawOrder: true);
133+
cardContainer.Sort();
134+
InvalidateLayout();
130135

131136
setupAction?.Invoke(drawable);
132137
}
133138

134-
public void Clear() => cardContainer.Clear();
139+
public void Clear()
140+
{
141+
foreach (var card in Cards.ToArray())
142+
RemoveCard(card.Item);
143+
}
135144

136145
public bool RemoveCard(RankedPlayCardWithPlaylistItem item)
137146
{
138147
if (!cardLookup.Remove(item.Card, out var drawable))
139148
return false;
140149

141-
// child order is only updated once per frame so ordering can change between that and the card getting removed
142-
// which can mess when doing a binary-search for the child during removal
143-
cardContainer.Sort();
144150
cardContainer.Remove(drawable, true);
145-
InvalidateLayout(drawOrder: true);
151+
InvalidateLayout();
146152
return true;
147153
}
148154

@@ -165,11 +171,8 @@ public bool RemoveCard(RankedPlayCardWithPlaylistItem item, [MaybeNullWhen(false
165171
screenSpaceDrawQuad = drawable.ScreenSpaceDrawQuad;
166172
card = drawable.Detach();
167173

168-
// child order is only updated once per frame so ordering can change between that and the card getting removed
169-
// which can mess when doing a binary-search for the child during removal
170-
cardContainer.Sort();
171174
cardContainer.Remove(drawable, true);
172-
InvalidateLayout(drawOrder: true);
175+
InvalidateLayout();
173176

174177
return true;
175178
}
@@ -178,7 +181,9 @@ public bool RemoveCard(RankedPlayCardWithPlaylistItem item, [MaybeNullWhen(false
178181

179182
protected virtual void OnCardStateChanged(HandCard card, ValueChangedEvent<RankedPlayCardState> evt)
180183
{
181-
InvalidateLayout(drawOrder: affectsDrawOrder(evt));
184+
InvalidateLayout();
185+
if (affectsDrawOrder(evt))
186+
cardContainer.Sort();
182187

183188
// hovered state can be caused by keyboard focus, in which case we have to clean up after the other cards manually
184189
if (evt.NewValue.Hovered)
@@ -198,30 +203,23 @@ private static bool affectsDrawOrder(ValueChangedEvent<RankedPlayCardState> evt)
198203
#region Layout
199204

200205
private readonly LayoutValue layoutBacking = new LayoutValue(Invalidation.DrawSize | Invalidation.MiscGeometry);
201-
private readonly Cached drawOrderBacking = new Cached();
202206

203207
/// <summary>
204208
/// Invalidates the layout of the hand of cards, causing a relayout to occur.
205209
/// </summary>
206-
/// <param name="drawOrder">If set to true, also invalidates the draw order of the cards.</param>
207-
protected void InvalidateLayout(bool drawOrder = false)
208-
{
209-
layoutBacking.Invalidate();
210-
if (drawOrder)
211-
drawOrderBacking.Invalidate();
212-
}
210+
protected void InvalidateLayout() => layoutBacking.Invalidate();
213211

214212
private void updateLayout()
215213
{
216214
if (Contracted)
217215
return;
218216

219217
// card container draws dragged card on top so we need to sort those separately
220-
var cards = cardContainer.Children.OrderBy(static c => c.State.Order).ToArray();
218+
var cards = GetCardsInDisplayOrder();
221219

222220
int activeCardIndex = GetActiveCardIndex(cards);
223221

224-
for (int i = 0; i < cards.Length; i++)
222+
for (int i = 0; i < cards.Count; i++)
225223
{
226224
var card = cards[i];
227225

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

Lines changed: 19 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -161,10 +161,15 @@ protected override bool OnKeyDown(KeyDownEvent e)
161161
switch (e.Key)
162162
{
163163
case >= Key.Number1 and <= Key.Number9:
164-
focusCard(e.Key - Key.Number1);
164+
{
165+
int index = e.Key - Key.Number1;
166+
if (GetCardsInDisplayOrder().ElementAtOrDefault(index) is HandCard card)
167+
focusCard(card);
165168
return true;
169+
}
166170

167171
case Key.Space:
172+
{
168173
if (selectionMode == HandSelectionMode.Disabled)
169174
return false;
170175

@@ -177,6 +182,7 @@ protected override bool OnKeyDown(KeyDownEvent e)
177182
card.TriggerClick();
178183

179184
return true;
185+
}
180186

181187
case Key.Left:
182188
moveCardFocus(-1);
@@ -192,30 +198,27 @@ protected override bool OnKeyDown(KeyDownEvent e)
192198

193199
private void moveCardFocus(int direction)
194200
{
195-
int currentIndex = Cards.ToList().FindIndex(c => c.HasFocus);
201+
var cards = GetCardsInDisplayOrder();
202+
203+
int currentIndex = cards.FindIndex(c => c.HasFocus);
196204

197205
// default behaviour is to start from either end of the cards if no card is focused currently
198206
// in single-selection mode we can however use the current selection as a fallback index if there's no focus
199207
if (selectionMode == HandSelectionMode.Single && currentIndex == -1)
200-
currentIndex = Cards.ToList().FindIndex(c => c.Selected);
208+
currentIndex = cards.FindIndex(c => c.Selected);
201209

202210
int newIndex = currentIndex + direction;
203211

204212
if (newIndex < 0)
205-
newIndex = Cards.Count - 1;
206-
else if (newIndex >= Cards.Count)
213+
newIndex = cards.Count - 1;
214+
else if (newIndex >= cards.Count)
207215
newIndex = 0;
208216

209-
focusCard(newIndex);
217+
focusCard(cards[newIndex]);
210218
}
211219

212-
private void focusCard(int index)
220+
private void focusCard(HandCard card)
213221
{
214-
var card = Cards.ElementAtOrDefault(index);
215-
216-
if (card == null)
217-
return;
218-
219222
GetContainingFocusManager()?.ChangeFocus(card);
220223

221224
if (SelectionMode == HandSelectionMode.Single && !card.Selected)
@@ -224,7 +227,7 @@ private void focusCard(int index)
224227

225228
private void cardDragged(PlayerHandCard card, Vector2 screenSpacePosition)
226229
{
227-
var cards = Cards.OrderBy(static c => c.Order).ToArray();
230+
var cards = GetCardsInDisplayOrder();
228231

229232
int newIndex = cardIndexInLayout(cards, card.ScreenSpaceDrawQuad.Centre);
230233

@@ -247,9 +250,9 @@ private void cardDragged(PlayerHandCard card, Vector2 screenSpacePosition)
247250
c.Item.DisplayOrder = c.Order;
248251
}
249252

250-
private int cardIndexInLayout(HandCard[] cards, Vector2 screenSpacePosition)
253+
private int cardIndexInLayout(IReadOnlyList<HandCard> cards, Vector2 screenSpacePosition)
251254
{
252-
Debug.Assert(cards.Length > 0);
255+
Debug.Assert(cards.Count > 0);
253256

254257
var position = ToLocalSpace(screenSpacePosition) - DrawSize / 2;
255258

@@ -258,7 +261,7 @@ private int cardIndexInLayout(HandCard[] cards, Vector2 screenSpacePosition)
258261
int minIndex = 0;
259262
float minDistance = float.MaxValue;
260263

261-
for (int i = 0; i < cards.Length; i++)
264+
for (int i = 0; i < cards.Count; i++)
262265
{
263266
float distance = MathF.Abs(GetCardX(i, activeIndex) - position.X);
264267

0 commit comments

Comments
 (0)