From 720e1cd206154fc40f8889ef813f1e31d01d1e0a Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Mon, 14 Mar 2022 03:35:23 +0300 Subject: [PATCH 1/5] Add failing test cases --- .../TestSceneDrawableStoryboardSprite.cs | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableStoryboardSprite.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableStoryboardSprite.cs index 4011a6bc82..34e6d1996d 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableStoryboardSprite.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableStoryboardSprite.cs @@ -5,6 +5,7 @@ using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; +using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Graphics; using osu.Framework.Testing; using osu.Framework.Utils; @@ -52,6 +53,49 @@ public void TestAllowLookupFromSkin() AddAssert("skinnable sprite has correct size", () => sprites.Any(s => Precision.AlmostEquals(s.ChildrenOfType().Single().Size, new Vector2(128, 128)))); } + [Test] + public void TestFlippedSprite() + { + const string lookup_name = "hitcircleoverlay"; + + AddStep("allow skin lookup", () => storyboard.UseSkinSprites = true); + AddStep("create sprites", () => SetContents(_ => createSprite(lookup_name, Anchor.TopLeft, Vector2.Zero))); + AddStep("flip sprites", () => sprites.ForEach(s => + { + s.FlipH = true; + s.FlipV = true; + })); + AddAssert("origin flipped", () => sprites.All(s => s.Origin == Anchor.BottomRight)); + } + + [Test] + public void TestNegativeScale() + { + const string lookup_name = "hitcircleoverlay"; + + AddStep("allow skin lookup", () => storyboard.UseSkinSprites = true); + AddStep("create sprites", () => SetContents(_ => createSprite(lookup_name, Anchor.TopLeft, Vector2.Zero))); + AddStep("scale sprite", () => sprites.ForEach(s => s.VectorScale = new Vector2(-1))); + AddAssert("origin flipped", () => sprites.All(s => s.Origin == Anchor.BottomRight)); + } + + [Test] + public void TestNegativeScaleWithFlippedSprite() + { + const string lookup_name = "hitcircleoverlay"; + + AddStep("allow skin lookup", () => storyboard.UseSkinSprites = true); + AddStep("create sprites", () => SetContents(_ => createSprite(lookup_name, Anchor.TopLeft, Vector2.Zero))); + AddStep("scale sprite", () => sprites.ForEach(s => s.VectorScale = new Vector2(-1))); + AddAssert("origin flipped", () => sprites.All(s => s.Origin == Anchor.BottomRight)); + AddStep("flip sprites", () => sprites.ForEach(s => + { + s.FlipH = true; + s.FlipV = true; + })); + AddAssert("origin back", () => sprites.All(s => s.Origin == Anchor.TopLeft)); + } + private DrawableStoryboardSprite createSprite(string lookupName, Anchor origin, Vector2 initialPosition) => new DrawableStoryboardSprite( new StoryboardSprite(lookupName, origin, initialPosition) From 0b8c89bfa81c88019f629c8941d542f15d06fc6d Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Mon, 14 Mar 2022 03:50:12 +0300 Subject: [PATCH 2/5] Fix drawable storyboard sprites not flipping origin on negative scale --- osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs b/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs index eb877f3dff..ad4402dcc4 100644 --- a/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs +++ b/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs @@ -79,7 +79,7 @@ public override Anchor Origin { var origin = base.Origin; - if (FlipH) + if ((FlipH || VectorScale.X < 0) && !(FlipH && VectorScale.X < 0)) { if (origin.HasFlagFast(Anchor.x0)) origin = Anchor.x2 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); @@ -87,7 +87,7 @@ public override Anchor Origin origin = Anchor.x0 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); } - if (FlipV) + if ((FlipV || VectorScale.Y < 0) && !(FlipV && VectorScale.Y < 0)) { if (origin.HasFlagFast(Anchor.y0)) origin = Anchor.y2 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); From 9cf05080da2bb0125eaf3483326d1ab4fa0ab66c Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Mon, 14 Mar 2022 04:40:24 +0300 Subject: [PATCH 3/5] Simplify conditionals to one XOR operations with comments --- osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs b/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs index ad4402dcc4..f3173497e8 100644 --- a/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs +++ b/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs @@ -79,7 +79,8 @@ public override Anchor Origin { var origin = base.Origin; - if ((FlipH || VectorScale.X < 0) && !(FlipH && VectorScale.X < 0)) + // Either flip horizontally or negative X scale, but not both. + if (FlipH ^ (VectorScale.X < 0)) { if (origin.HasFlagFast(Anchor.x0)) origin = Anchor.x2 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); @@ -87,7 +88,8 @@ public override Anchor Origin origin = Anchor.x0 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); } - if ((FlipV || VectorScale.Y < 0) && !(FlipV && VectorScale.Y < 0)) + // Either flip vertically or negative Y scale, but not both. + if (FlipV ^ (VectorScale.Y < 0)) { if (origin.HasFlagFast(Anchor.y0)) origin = Anchor.y2 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); From 740a72e16d07b380177c331096780043ded35da1 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Mon, 14 Mar 2022 05:44:34 +0300 Subject: [PATCH 4/5] Share origin adjustment logic between storyboard sprite and animation --- .../Drawables/DrawableStoryboardAnimation.cs | 30 +------------ osu.Game/Storyboards/StoryboardExtensions.cs | 43 +++++++++++++++++++ 2 files changed, 45 insertions(+), 28 deletions(-) create mode 100644 osu.Game/Storyboards/StoryboardExtensions.cs diff --git a/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs b/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs index 81623a9307..28ac83a25c 100644 --- a/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs +++ b/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs @@ -3,7 +3,6 @@ using System; using osu.Framework.Allocation; -using osu.Framework.Extensions.EnumExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Animations; using osu.Framework.Graphics.Textures; @@ -70,34 +69,9 @@ public Vector2 VectorScale public override bool RemoveWhenNotAlive => false; - protected override Vector2 DrawScale - => new Vector2(FlipH ? -base.DrawScale.X : base.DrawScale.X, FlipV ? -base.DrawScale.Y : base.DrawScale.Y) * VectorScale; + protected override Vector2 DrawScale => new Vector2(FlipH ? -base.DrawScale.X : base.DrawScale.X, FlipV ? -base.DrawScale.Y : base.DrawScale.Y) * VectorScale; - public override Anchor Origin - { - get - { - var origin = base.Origin; - - if (FlipH) - { - if (origin.HasFlagFast(Anchor.x0)) - origin = Anchor.x2 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); - else if (origin.HasFlagFast(Anchor.x2)) - origin = Anchor.x0 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); - } - - if (FlipV) - { - if (origin.HasFlagFast(Anchor.y0)) - origin = Anchor.y2 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); - else if (origin.HasFlagFast(Anchor.y2)) - origin = Anchor.y0 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); - } - - return origin; - } - } + public override Anchor Origin => StoryboardExtensions.AdjustOrigin(base.Origin, VectorScale, FlipH, FlipV); public override bool IsPresent => !float.IsNaN(DrawPosition.X) && !float.IsNaN(DrawPosition.Y) && base.IsPresent; diff --git a/osu.Game/Storyboards/StoryboardExtensions.cs b/osu.Game/Storyboards/StoryboardExtensions.cs new file mode 100644 index 0000000000..4e8251c9e7 --- /dev/null +++ b/osu.Game/Storyboards/StoryboardExtensions.cs @@ -0,0 +1,43 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using osu.Framework.Extensions.EnumExtensions; +using osu.Framework.Graphics; +using osuTK; + +namespace osu.Game.Storyboards +{ + public static class StoryboardExtensions + { + /// + /// Given an origin and a set of properties, adjust the origin to display the sprite/animation correctly. + /// + /// The current origin. + /// The vector scale. + /// Whether the element is flipped horizontally. + /// Whether the element is flipped vertically. + /// The adjusted origin. + public static Anchor AdjustOrigin(Anchor origin, Vector2 vectorScale, bool flipH, bool flipV) + { + // Either flip horizontally or negative X scale, but not both. + if (flipH ^ (vectorScale.X < 0)) + { + if (origin.HasFlagFast(Anchor.x0)) + origin = Anchor.x2 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); + else if (origin.HasFlagFast(Anchor.x2)) + origin = Anchor.x0 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); + } + + // Either flip vertically or negative Y scale, but not both. + if (flipV ^ (vectorScale.Y < 0)) + { + if (origin.HasFlagFast(Anchor.y0)) + origin = Anchor.y2 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); + else if (origin.HasFlagFast(Anchor.y2)) + origin = Anchor.y0 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); + } + + return origin; + } + } +} From c1697c7621358e02dccb710d79842696a983c669 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Mon, 14 Mar 2022 06:29:49 +0300 Subject: [PATCH 5/5] Update `DrawableStoryboardSprite` to use helper method --- .../Drawables/DrawableStoryboardAnimation.cs | 3 +- .../Drawables/DrawableStoryboardSprite.cs | 29 +------------------ 2 files changed, 3 insertions(+), 29 deletions(-) diff --git a/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs b/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs index 28ac83a25c..88cb5f40a1 100644 --- a/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs +++ b/osu.Game/Storyboards/Drawables/DrawableStoryboardAnimation.cs @@ -69,7 +69,8 @@ public Vector2 VectorScale public override bool RemoveWhenNotAlive => false; - protected override Vector2 DrawScale => new Vector2(FlipH ? -base.DrawScale.X : base.DrawScale.X, FlipV ? -base.DrawScale.Y : base.DrawScale.Y) * VectorScale; + protected override Vector2 DrawScale + => new Vector2(FlipH ? -base.DrawScale.X : base.DrawScale.X, FlipV ? -base.DrawScale.Y : base.DrawScale.Y) * VectorScale; public override Anchor Origin => StoryboardExtensions.AdjustOrigin(base.Origin, VectorScale, FlipH, FlipV); diff --git a/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs b/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs index f3173497e8..db10f13896 100644 --- a/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs +++ b/osu.Game/Storyboards/Drawables/DrawableStoryboardSprite.cs @@ -3,7 +3,6 @@ using System; using osu.Framework.Allocation; -using osu.Framework.Extensions.EnumExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Textures; @@ -73,33 +72,7 @@ public Vector2 VectorScale protected override Vector2 DrawScale => new Vector2(FlipH ? -base.DrawScale.X : base.DrawScale.X, FlipV ? -base.DrawScale.Y : base.DrawScale.Y) * VectorScale; - public override Anchor Origin - { - get - { - var origin = base.Origin; - - // Either flip horizontally or negative X scale, but not both. - if (FlipH ^ (VectorScale.X < 0)) - { - if (origin.HasFlagFast(Anchor.x0)) - origin = Anchor.x2 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); - else if (origin.HasFlagFast(Anchor.x2)) - origin = Anchor.x0 | (origin & (Anchor.y0 | Anchor.y1 | Anchor.y2)); - } - - // Either flip vertically or negative Y scale, but not both. - if (FlipV ^ (VectorScale.Y < 0)) - { - if (origin.HasFlagFast(Anchor.y0)) - origin = Anchor.y2 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); - else if (origin.HasFlagFast(Anchor.y2)) - origin = Anchor.y0 | (origin & (Anchor.x0 | Anchor.x1 | Anchor.x2)); - } - - return origin; - } - } + public override Anchor Origin => StoryboardExtensions.AdjustOrigin(base.Origin, VectorScale, FlipH, FlipV); public override bool IsPresent => !float.IsNaN(DrawPosition.X) && !float.IsNaN(DrawPosition.Y) && base.IsPresent;