Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add edge highlight to "argon" slider repeat arrow (and improve all skins' reverse arrow animations) #24990

Merged
merged 11 commits into from
Oct 3, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Oct 2, 2023

Addresses concerns raised at #24984.

I think this is something which has to be fixed as a priority, as it was shockingly broken. So, priority given.


This addresses concerns about visibility of repeat arrows on short sliders by adding a border element which animated outwards.

I've also fixed multiple inconsistencies when compared to stable, including disabling beat sync and having the animation start as soon as the hit object appears. This helps massively with visibility, as otherwise the animation may not start until much later. It also looks better as every slider is not longer synchronised to the same animation cycle.

osu.2023-10-02.at.10.08.59.mp4
osu.Game.Rulesets.Osu.Tests.2023-10-02.at.11.12.22.mp4

@@ -38,6 +40,10 @@ private void load(ISkinSource skinSource)

InternalChild = arrow = (skin?.GetAnimation(lookupName, true, true, maxSize: OsuHitObject.OBJECT_DIMENSIONS) ?? Empty());
textureIsDefaultSkin = skin is ISkinTransformer transformer && transformer.Skin is DefaultLegacySkin;

drawableObject.ApplyCustomUpdateState += updateStateTransforms;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda weird that there are event handlers being registered both in BDL and LoadComplete().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The animation doesn't play without it being here. We could try moving the other one here though?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works, sure. If it doesn't, I guess I'll live with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I'm not sure I'd feel safe changing it in this PR, so overlooking may be best 😅.

@bdach
Copy link
Collaborator

bdach commented Oct 2, 2023

Made a comparison video (google drive because it doesn't embed for whatever reason): https://drive.google.com/file/d/1MziNMMSNVGQ1zRjigA-bqhZPAf-qBOrR/view?usp=sharing

I think the rotation animation on the v1 skins still looks a bit wonky, and is improved by applying

diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs
index 34bcf95e1d..5164dc3240 100644
--- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs
+++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs
@@ -39,7 +39,10 @@ private void load(ISkinSource skinSource)
 
             var skin = skinSource.FindProvider(s => s.GetTexture(lookupName) != null);
 
-            InternalChild = arrow = (skin?.GetAnimation(lookupName, true, true, maxSize: OsuHitObject.OBJECT_DIMENSIONS) ?? Empty());
+            InternalChild = arrow = (skin?.GetAnimation(lookupName, true, true, maxSize: OsuHitObject.OBJECT_DIMENSIONS) ?? Empty()).With(arr =>
+            {
+                arr.Anchor = arr.Origin = Anchor.Centre;
+            });
             textureIsDefaultSkin = skin is ISkinTransformer transformer && transformer.Skin is DefaultLegacySkin;
 
             drawableObject.ApplyCustomUpdateState += updateStateTransforms;

Before:

2023-10-02.22-36-20.mp4

After:

2023-10-02.22-36-57.mp4

@peppy
Copy link
Member Author

peppy commented Oct 3, 2023

Made a comparison video (google drive because it doesn't embed for whatever reason): drive.google.com/file/d/1MziNMMSNVGQ1zRjigA-bqhZPAf-qBOrR/view?usp=sharing

I think the rotation animation on the v1 skins still looks a bit wonky, and is improved by applying

diff --git a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs
index 34bcf95e1d..5164dc3240 100644
--- a/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs
+++ b/osu.Game.Rulesets.Osu/Skinning/Legacy/LegacyReverseArrow.cs
@@ -39,7 +39,10 @@ private void load(ISkinSource skinSource)
 
             var skin = skinSource.FindProvider(s => s.GetTexture(lookupName) != null);
 
-            InternalChild = arrow = (skin?.GetAnimation(lookupName, true, true, maxSize: OsuHitObject.OBJECT_DIMENSIONS) ?? Empty());
+            InternalChild = arrow = (skin?.GetAnimation(lookupName, true, true, maxSize: OsuHitObject.OBJECT_DIMENSIONS) ?? Empty()).With(arr =>
+            {
+                arr.Anchor = arr.Origin = Anchor.Centre;
+            });
             textureIsDefaultSkin = skin is ISkinTransformer transformer && transformer.Skin is DefaultLegacySkin;
 
             drawableObject.ApplyCustomUpdateState += updateStateTransforms;

Before:

2023-10-02.22-36-20.mp4

After:

2023-10-02.22-36-57.mp4

Nice find. I was wondering why this looked so wrong but it's such a rare occurrence in skinning that I didn't really care enough to fix immediately.

@bdach
Copy link
Collaborator

bdach commented Oct 3, 2023

Updated comparison: https://drive.google.com/file/d/12xAX1XJhYuoQ0xXO_omGouios1o7XEhj/view

Looks about as good as can get. Just need the resources change to go through and we can probably wrap this up.

bdach
bdach previously approved these changes Oct 3, 2023
@bdach bdach merged commit 9c1f5c3 into ppy:master Oct 3, 2023
@peppy peppy deleted the argon-better-reverse branch October 4, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants