mirror of
https://github.com/ppy/osu.git
synced 2026-06-03 15:04:26 +08:00
Code quality improvements for child/draw order handling in HandOfCards (#37423)
Attempt at adressing the points made in https://github.com/ppy/osu/pull/37419#issuecomment-4279123323 - `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>
This commit is contained in:
maarvin
committed by
GitHub
Unverified
GitHub
parent
71b3d51ed5
commit
7b0e5ecc13
@@ -56,14 +56,14 @@ namespace osu.Game.Tests.Visual.RankedPlay
|
||||
});
|
||||
AddStep("single selection mode", () => handOfCards.SelectionMode = HandSelectionMode.Single);
|
||||
|
||||
AddStep("click first card", () => handOfCards.Cards.First().TriggerClick());
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.First().Item]));
|
||||
AddStep("click first card", () => handOfCards.GetCardsInDisplayOrder()[0].TriggerClick());
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
|
||||
|
||||
AddStep("click second card", () => handOfCards.Cards.ElementAt(1).TriggerClick());
|
||||
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(1).Item]));
|
||||
AddStep("click second card", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
|
||||
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[1].Item]));
|
||||
|
||||
AddStep("click second card again", () => handOfCards.Cards.ElementAt(1).TriggerClick());
|
||||
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(1).Item]));
|
||||
AddStep("click second card again", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
|
||||
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[1].Item]));
|
||||
}
|
||||
|
||||
[Test]
|
||||
@@ -76,14 +76,14 @@ namespace osu.Game.Tests.Visual.RankedPlay
|
||||
});
|
||||
AddStep("multi selection mode", () => handOfCards.SelectionMode = HandSelectionMode.Multiple);
|
||||
|
||||
AddStep("click first card", () => handOfCards.Cards.First().TriggerClick());
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.First().Item]));
|
||||
AddStep("click first card", () => handOfCards.GetCardsInDisplayOrder().First().TriggerClick());
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder().First().Item]));
|
||||
|
||||
AddStep("click second card", () => handOfCards.Cards.ElementAt(1).TriggerClick());
|
||||
AddAssert("both cards selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item, handOfCards.Cards.ElementAt(1).Item]));
|
||||
AddStep("click second card", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
|
||||
AddAssert("both cards selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item, handOfCards.GetCardsInDisplayOrder()[1].Item]));
|
||||
|
||||
AddStep("click second card again", () => handOfCards.Cards.ElementAt(1).TriggerClick());
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item]));
|
||||
AddStep("click second card again", () => handOfCards.GetCardsInDisplayOrder()[1].TriggerClick());
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
|
||||
}
|
||||
|
||||
[Test]
|
||||
@@ -131,20 +131,20 @@ namespace osu.Game.Tests.Visual.RankedPlay
|
||||
Key key = Key.Number1 + i;
|
||||
|
||||
AddStep($"key {i + 1}", () => InputManager.Key(key));
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(i1).Item]));
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[i1].Item]));
|
||||
}
|
||||
|
||||
AddStep("right arrow", () => InputManager.Key(Key.Right));
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item]));
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
|
||||
|
||||
AddStep("right arrow", () => InputManager.Key(Key.Right));
|
||||
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(1).Item]));
|
||||
AddAssert("second card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[1].Item]));
|
||||
|
||||
AddStep("left arrow", () => InputManager.Key(Key.Left));
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(0).Item]));
|
||||
AddAssert("first card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[0].Item]));
|
||||
|
||||
AddStep("left arrow", () => InputManager.Key(Key.Left));
|
||||
AddAssert("last card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.Cards.ElementAt(^1).Item]));
|
||||
AddAssert("last card selected", () => handOfCards.Selection.SequenceEqual([handOfCards.GetCardsInDisplayOrder()[^1].Item]));
|
||||
|
||||
AddStep("space", () => InputManager.Key(Key.Space));
|
||||
AddAssert("play action triggered", () => playActionTriggered);
|
||||
@@ -166,11 +166,11 @@ namespace osu.Game.Tests.Visual.RankedPlay
|
||||
Key key = Key.Number1 + i;
|
||||
|
||||
AddStep($"key {i + 1}", () => InputManager.Key(key));
|
||||
AddAssert("card hovered", () => handOfCards.Cards.ElementAt(i1).CardHovered);
|
||||
AddAssert("card hovered", () => handOfCards.GetCardsInDisplayOrder()[i1].CardHovered);
|
||||
|
||||
AddAssert("card not selected", () => !handOfCards.Selection.Contains(handOfCards.Cards.ElementAt(i1).Card.Item));
|
||||
AddAssert("card not selected", () => !handOfCards.Selection.Contains(handOfCards.GetCardsInDisplayOrder()[i1].Card.Item));
|
||||
AddStep("space", () => InputManager.Key(Key.Space));
|
||||
AddAssert("card selected", () => handOfCards.Selection.Contains(handOfCards.Cards.ElementAt(i1).Card.Item));
|
||||
AddAssert("card selected", () => handOfCards.Selection.Contains(handOfCards.GetCardsInDisplayOrder()[i1].Card.Item));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -201,9 +201,9 @@ namespace osu.Game.Tests.Visual.RankedPlay
|
||||
for (int i = 0; i < 5; i++)
|
||||
handOfCards.AddCard(new RankedPlayCardWithPlaylistItem(new RankedPlayCardItem()));
|
||||
});
|
||||
AddStep("hover card", () => InputManager.MoveMouseTo(handOfCards.Cards.First()));
|
||||
AddStep("hover card", () => InputManager.MoveMouseTo(handOfCards.GetCardsInDisplayOrder()[0]));
|
||||
AddStep("start drag", () => InputManager.PressButton(MouseButton.Left));
|
||||
AddStep("move card", () => InputManager.MoveMouseTo(handOfCards.Cards[3]));
|
||||
AddStep("move card", () => InputManager.MoveMouseTo(handOfCards.GetCardsInDisplayOrder()[3]));
|
||||
AddStep("remove cards", () =>
|
||||
{
|
||||
foreach (var card in handOfCards.Cards.ToArray())
|
||||
|
||||
@@ -7,7 +7,6 @@ using System.Diagnostics.CodeAnalysis;
|
||||
using System.Linq;
|
||||
using osu.Framework.Allocation;
|
||||
using osu.Framework.Bindables;
|
||||
using osu.Framework.Caching;
|
||||
using osu.Framework.Graphics;
|
||||
using osu.Framework.Graphics.Containers;
|
||||
using osu.Framework.Graphics.Primitives;
|
||||
@@ -29,7 +28,18 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
|
||||
private const float card_spacing = -15;
|
||||
|
||||
public IReadOnlyList<HandCard> Cards => cardContainer.Children;
|
||||
/// <summary>
|
||||
/// Cards currently present in this <see cref="HandOfCards"/>
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Entries are not sorted by display order
|
||||
/// </remarks>
|
||||
public IEnumerable<HandCard> Cards => cardLookup.Values;
|
||||
|
||||
/// <summary>
|
||||
/// Returns a list of the cards present in this <see cref="HandOfCards"/> ordered by the cards' <see cref="HandCard.Order"/>
|
||||
/// </summary>
|
||||
public List<HandCard> GetCardsInDisplayOrder() => Cards.OrderBy(static c => c.Order).ToList();
|
||||
|
||||
/// <summary>
|
||||
/// How far a card slides upwards when hovered.
|
||||
@@ -66,12 +76,6 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
{
|
||||
base.Update();
|
||||
|
||||
if (!drawOrderBacking.IsValid)
|
||||
{
|
||||
cardContainer.Sort();
|
||||
drawOrderBacking.Validate();
|
||||
}
|
||||
|
||||
if (!layoutBacking.IsValid)
|
||||
{
|
||||
updateLayout();
|
||||
@@ -126,23 +130,25 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
drawable.Order = cardContainer.Max(c => c.Order) + 1;
|
||||
|
||||
cardContainer.Add(drawable);
|
||||
InvalidateLayout(drawOrder: true);
|
||||
cardContainer.Sort();
|
||||
InvalidateLayout();
|
||||
|
||||
setupAction?.Invoke(drawable);
|
||||
}
|
||||
|
||||
public void Clear() => cardContainer.Clear();
|
||||
public void Clear()
|
||||
{
|
||||
foreach (var card in Cards.ToArray())
|
||||
RemoveCard(card.Item);
|
||||
}
|
||||
|
||||
public bool RemoveCard(RankedPlayCardWithPlaylistItem item)
|
||||
{
|
||||
if (!cardLookup.Remove(item.Card, out var drawable))
|
||||
return false;
|
||||
|
||||
// child order is only updated once per frame so ordering can change between that and the card getting removed
|
||||
// which can mess when doing a binary-search for the child during removal
|
||||
cardContainer.Sort();
|
||||
cardContainer.Remove(drawable, true);
|
||||
InvalidateLayout(drawOrder: true);
|
||||
InvalidateLayout();
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -165,11 +171,8 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
screenSpaceDrawQuad = drawable.ScreenSpaceDrawQuad;
|
||||
card = drawable.Detach();
|
||||
|
||||
// child order is only updated once per frame so ordering can change between that and the card getting removed
|
||||
// which can mess when doing a binary-search for the child during removal
|
||||
cardContainer.Sort();
|
||||
cardContainer.Remove(drawable, true);
|
||||
InvalidateLayout(drawOrder: true);
|
||||
InvalidateLayout();
|
||||
|
||||
return true;
|
||||
}
|
||||
@@ -178,7 +181,9 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
|
||||
protected virtual void OnCardStateChanged(HandCard card, ValueChangedEvent<RankedPlayCardState> evt)
|
||||
{
|
||||
InvalidateLayout(drawOrder: affectsDrawOrder(evt));
|
||||
InvalidateLayout();
|
||||
if (affectsDrawOrder(evt))
|
||||
cardContainer.Sort();
|
||||
|
||||
// hovered state can be caused by keyboard focus, in which case we have to clean up after the other cards manually
|
||||
if (evt.NewValue.Hovered)
|
||||
@@ -198,18 +203,11 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
#region Layout
|
||||
|
||||
private readonly LayoutValue layoutBacking = new LayoutValue(Invalidation.DrawSize | Invalidation.MiscGeometry);
|
||||
private readonly Cached drawOrderBacking = new Cached();
|
||||
|
||||
/// <summary>
|
||||
/// Invalidates the layout of the hand of cards, causing a relayout to occur.
|
||||
/// </summary>
|
||||
/// <param name="drawOrder">If set to true, also invalidates the draw order of the cards.</param>
|
||||
protected void InvalidateLayout(bool drawOrder = false)
|
||||
{
|
||||
layoutBacking.Invalidate();
|
||||
if (drawOrder)
|
||||
drawOrderBacking.Invalidate();
|
||||
}
|
||||
protected void InvalidateLayout() => layoutBacking.Invalidate();
|
||||
|
||||
private void updateLayout()
|
||||
{
|
||||
@@ -217,11 +215,11 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
return;
|
||||
|
||||
// card container draws dragged card on top so we need to sort those separately
|
||||
var cards = cardContainer.Children.OrderBy(static c => c.State.Order).ToArray();
|
||||
var cards = GetCardsInDisplayOrder();
|
||||
|
||||
int activeCardIndex = GetActiveCardIndex(cards);
|
||||
|
||||
for (int i = 0; i < cards.Length; i++)
|
||||
for (int i = 0; i < cards.Count; i++)
|
||||
{
|
||||
var card = cards[i];
|
||||
|
||||
|
||||
@@ -161,10 +161,15 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
switch (e.Key)
|
||||
{
|
||||
case >= Key.Number1 and <= Key.Number9:
|
||||
focusCard(e.Key - Key.Number1);
|
||||
{
|
||||
int index = e.Key - Key.Number1;
|
||||
if (GetCardsInDisplayOrder().ElementAtOrDefault(index) is HandCard card)
|
||||
focusCard(card);
|
||||
return true;
|
||||
}
|
||||
|
||||
case Key.Space:
|
||||
{
|
||||
if (selectionMode == HandSelectionMode.Disabled)
|
||||
return false;
|
||||
|
||||
@@ -177,6 +182,7 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
card.TriggerClick();
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
case Key.Left:
|
||||
moveCardFocus(-1);
|
||||
@@ -192,30 +198,27 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
|
||||
private void moveCardFocus(int direction)
|
||||
{
|
||||
int currentIndex = Cards.ToList().FindIndex(c => c.HasFocus);
|
||||
var cards = GetCardsInDisplayOrder();
|
||||
|
||||
int currentIndex = cards.FindIndex(c => c.HasFocus);
|
||||
|
||||
// default behaviour is to start from either end of the cards if no card is focused currently
|
||||
// in single-selection mode we can however use the current selection as a fallback index if there's no focus
|
||||
if (selectionMode == HandSelectionMode.Single && currentIndex == -1)
|
||||
currentIndex = Cards.ToList().FindIndex(c => c.Selected);
|
||||
currentIndex = cards.FindIndex(c => c.Selected);
|
||||
|
||||
int newIndex = currentIndex + direction;
|
||||
|
||||
if (newIndex < 0)
|
||||
newIndex = Cards.Count - 1;
|
||||
else if (newIndex >= Cards.Count)
|
||||
newIndex = cards.Count - 1;
|
||||
else if (newIndex >= cards.Count)
|
||||
newIndex = 0;
|
||||
|
||||
focusCard(newIndex);
|
||||
focusCard(cards[newIndex]);
|
||||
}
|
||||
|
||||
private void focusCard(int index)
|
||||
private void focusCard(HandCard card)
|
||||
{
|
||||
var card = Cards.ElementAtOrDefault(index);
|
||||
|
||||
if (card == null)
|
||||
return;
|
||||
|
||||
GetContainingFocusManager()?.ChangeFocus(card);
|
||||
|
||||
if (SelectionMode == HandSelectionMode.Single && !card.Selected)
|
||||
@@ -224,7 +227,7 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
|
||||
private void cardDragged(PlayerHandCard card, Vector2 screenSpacePosition)
|
||||
{
|
||||
var cards = Cards.OrderBy(static c => c.Order).ToArray();
|
||||
var cards = GetCardsInDisplayOrder();
|
||||
|
||||
int newIndex = cardIndexInLayout(cards, card.ScreenSpaceDrawQuad.Centre);
|
||||
|
||||
@@ -247,9 +250,9 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
c.Item.DisplayOrder = c.Order;
|
||||
}
|
||||
|
||||
private int cardIndexInLayout(HandCard[] cards, Vector2 screenSpacePosition)
|
||||
private int cardIndexInLayout(IReadOnlyList<HandCard> cards, Vector2 screenSpacePosition)
|
||||
{
|
||||
Debug.Assert(cards.Length > 0);
|
||||
Debug.Assert(cards.Count > 0);
|
||||
|
||||
var position = ToLocalSpace(screenSpacePosition) - DrawSize / 2;
|
||||
|
||||
@@ -258,7 +261,7 @@ namespace osu.Game.Screens.OnlinePlay.Matchmaking.RankedPlay.Hand
|
||||
int minIndex = 0;
|
||||
float minDistance = float.MaxValue;
|
||||
|
||||
for (int i = 0; i < cards.Length; i++)
|
||||
for (int i = 0; i < cards.Count; i++)
|
||||
{
|
||||
float distance = MathF.Abs(GetCardX(i, activeIndex) - position.X);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user