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

Remove slider head circle movement (and remove setting from "classic" mod) #24810

Merged
merged 8 commits into from
Sep 19, 2023

Conversation

peppy
Copy link
Member

@peppy peppy commented Sep 14, 2023

Closes #24639.

I've gone with the most simple approach animation wise, which feels quite good to me: the snaking path will only begin snaking after the head circle is hit. I can see that others came to roughly the same consensus in the original discussion, so I don't think there will be too much contention over this change.

Things to note:

  • I considered having the slider ball not appear until the head is hit, but I don't think a change like this would sit well as it would potentially reduce readability
  • I intentionally made the snaking only happen on a successful hit. So if you miss the slider head completely, the outwards snaking will never complete. This could be a good visual cue.
  • The original plan I had animation wise was to have the portion of path before the slider ball's current position fade out on slider head hit, but this would mean duplicating the slider path. And I don't think this is a good idea from a performance perspective for such an edge case.

Here's how it looks in slow motion:

osu.Game.Rulesets.Osu.Tests.2023-09-14.at.08.59.08.mp4

It's important to note that in the majority of cases, these kinds of sliders are so fast you can't even catch this visually. It's usually hidden under the head / tail. Even when it's not, it's hard to catch:

osu.Game.Rulesets.Osu.Tests.2023-09-14.at.09.01.22.mp4

Finally, here's what missing a slider head completely looks like:

osu.Game.Rulesets.Osu.Tests.2023-09-14.at.09.01.51.mp4

(basically, as if snaking was not enabled)

@monochrome22
Copy link

monochrome22 commented Sep 14, 2023

It's unfortunate that fading can't be used. If it's not an issue to undermine snaking sliders or have jarring visuals in edge cases, then this is probably the next best solution.
We did consider disconnected slider heads to be less jarring than teleporting, but with disconnected slider heads, it wouldn't be possible to reduce the amount of poor visuals by not snaking when the slider is missed, and slider heads in the middle of nowhere can look weird.

Would it be possible to have the slider body fade out quickly, like over 40ms, instead of immediately disappearing, or would that also require duplicating the slider path?

@bdach
Copy link
Collaborator

bdach commented Sep 14, 2023

The failing snaking test here probably needs to be removed or just adjusted to check that on miss the slider repeat stays in place:

diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneSliderSnaking.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderSnaking.cs
index 630049f408..aef7dcaa59 100644
--- a/osu.Game.Rulesets.Osu.Tests/TestSceneSliderSnaking.cs
+++ b/osu.Game.Rulesets.Osu.Tests/TestSceneSliderSnaking.cs
@@ -135,9 +135,9 @@ public void TestSnakingDisabled(int sliderIndex)
         }
 
         [Test]
-        public void TestRepeatArrowDoesNotMoveWhenHit()
+        public void TestRepeatArrowDoesNotMove([Values] bool useAutoplay)
         {
-            AddStep("enable autoplay", () => autoplay = true);
+            AddStep($"set autoplay to {useAutoplay}", () => autoplay = useAutoplay);
             setSnaking(true);
             CreateTest();
             // repeat might have a chance to update its position depending on where in the frame its hit,
@@ -145,15 +145,6 @@ public void TestRepeatArrowDoesNotMoveWhenHit()
             addCheckPositionChangeSteps(() => 16600, getSliderRepeat, positionAlmostSame);
         }
 
-        [Test]
-        public void TestRepeatArrowMovesWhenNotHit()
-        {
-            AddStep("disable autoplay", () => autoplay = false);
-            setSnaking(true);
-            CreateTest();
-            addCheckPositionChangeSteps(() => 16600, getSliderRepeat, positionDecreased);
-        }
-
         private void retrieveSlider(int index)
         {
             AddStep("retrieve slider at index", () => slider = (Slider)beatmap.HitObjects[index]);

I don't see how that can be reconciled with the new behaviour here (if slider head missed then don't snake out).

However, something that is quite noticeable is that if a slider has its slider head missed, but then begins being tracked halfway through, it fades out much faster than if it is fully missed (to the point of appearing like it fades out instantly):

2023-09-14.13-38-16.mp4

This probably wasn't visible previously because of a combination of 2 things:

  • With classic mod off, the snaking combined with slider head movement led to the fact that at the end of the slider the body was always mostly obscured by the moving head, hiding the abrupt fade
  • With classic mod on, snaking out was just off.

Now it's in a weird state wherein a slider with a missed head will not snake (because of UpdateProgress() always being called with 0), but this still kicks in:

case ArmedState.Hit:
if (SliderBody?.SnakingOut.Value == true)
Body.FadeOut(40); // short fade to allow for any body colour to smoothly disappear.
break;

Would it be possible to have the slider body fade out quickly, like over 40ms, instead of immediately disappearing, or would that also require duplicating the slider path?

This may be both difficult to make happen, and I'm unsure if it's even worth it, since I had to slow down the animation to 1-2% to even notice the instant slider fadeout.

@bdach
Copy link
Collaborator

bdach commented Sep 14, 2023

Also this probably fixes #24464 (should verify)

@peppy
Copy link
Member Author

peppy commented Sep 14, 2023

Would it be possible to have the slider body fade out quickly, like over 40ms, instead of immediately disappearing, or would that also require duplicating the slider path?

See my opening post.

@monochrome22
Copy link

See my opening post.

I misread, I thought it was talking about the idea of fading the part between the slider ball and slider head, which I assume was rejected for the same performance reasons, since it would solve problems that teleporting has.

@peppy
Copy link
Member Author

peppy commented Sep 19, 2023

Now it's in a weird state wherein a slider with a missed head will not snake (because of UpdateProgress() always being called with 0), but this still kicks in

Good catch.

@peppy peppy requested a review from bdach September 19, 2023 05:52
@bdach bdach merged commit 92cafe2 into ppy:master Sep 19, 2023
Walavouchey added a commit to cdwcgt/osu-wiki that referenced this pull request Sep 21, 2023
@peppy peppy deleted the remove-slider-head-movement-allowance branch September 26, 2023 07:41
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.

Remove slider head movement entirely
3 participants