Add slider head circle late hit lenience #25776

smoogipoo · 2023-12-15T08:13:33Z

Resolves the second part of #25129
Resolves #25651

There are two parts to this, that I could split out:

The implementation of the lenience.
Refactoring all slider input (which has now become a bit more complex) to SliderInputManager. The first commit (599fdb0) can be reviewed on its own, with the refactoring in the follow up commit being an almost 1:1 move of all existing logic to a new class. All other commits from there on can be reviewed on their own.

This allows slider heads to be hit late. I've taken a little bit of liberty in the specifics here which may differ slightly from the discussion, but is hopefully amicable.

From a non-visual perspective, the intention here is as such:

The follow circle always starts enabled.
If a tick is passed and the cursor is not in the follow circle, then the follow circle is disabldd.
This allows the slider head to be hit late and for tracking to be gained even if the slider ball is no longer under the cursor.
If the follow circle remained active until the head hit-time, then all prior ticks are judged as hits.
If the follow circle was disabled at any point until the head hit-time, then all prior ticks are missed.

The discussion thread wanted to check if the follow circle had "at any point" been too far from the mouse position, but this is not feasible. I believe this implementation will behave better with a slider that has a single tick towards the end, but a point on tbe path that is far from the head.

There are quite a few test cases, but here are some that players would find interest in:

Late hit inside tracking radius

https://drive.google.com/file/d/10fjm9VSZ2qRSwpJRuPQ60IEKztrwzhwE/view?usp=share_link

Late hit outside tracking radius

https://drive.google.com/file/d/10gdU9WV2ze0jRVaDiqov1I-jpgJMO3q1/view?usp=share_link

Miss hit inside tracking radius

https://drive.google.com/file/d/10lNfyaR8-8FLpH3msXbvYuBPy0-l4wWV/view?usp=share_link

Sliders with required outside tracking point

https://drive.google.com/file/d/10llkF0XoKG0yJ_1Qc_M2a_BGkit_yHu-/view?usp=sharing

Short slider with repeats and ticks

https://drive.google.com/file/d/10iC_ZAsP8exFxpwu5EZqiKnzSAPFkeXP/view?usp=share_link

Zyfarok · 2023-12-16T23:15:33Z

osu.Game.Rulesets.Osu/Objects/Drawables/SliderInputManager.cs

+            if (!head.Judged || !head.Result.IsHit)
+                return;
+
+            if (!isMouseInFollowArea(true))


This is arguable, but I think it might make more sense to move this check to after the foreach ?
(Shouldn't you hit the first few ticks that are within range even if the ball is no longer within range ?)

Taking your example videos, I would say it doesn't make sense for "Late hit outside tracking radius" (2nd) to not hit the first few ticks when in "Sliders with outside extremums" (4th) it does. Either it should hit the first ticks in both or miss them in both.

Edit: Actually it might be better to leave that line here but miss all ticks if any is not in range. (so changing the 4th example with the circle-shaped slider to miss all ticks) see #25776 (comment) bellow

(Edit: Smoogi applied the suggestion from #25776 (comment) so you can ignore that point)

You disagree even with the fact that 2nd and 4th being treated differently is weird ? Not that it matters that much anyway, but I just felt like it was a tiny bit weird, and if you don't want to miss all ticks if any is not in range, you might as well move the isMouseInFollowArea after the foreach as I initially suggested to be more consistent.

Zyfarok · 2023-12-16T23:41:35Z

osu.Game.Rulesets.Osu/Objects/Drawables/SliderInputManager.cs

+                if (slider.Tracking.Value)
+                {
+                    // Attempt to preserve correct ordering of judgements as best we can by forcing an un-judged head to be missed when the user has clearly skipped it.
+                    slider.HeadCircle.MissForcefully();


What will happen if a player is still holding the key from the previous hit-object and is on his way to hit the slider-head late ?
Can it accidentally activate the slider-ball on the way and forcefully miss the slider-head because it detected a hit on a tick ?
The player might still have had the time to hit the head in time...
Not sure how realistic this scenario is though. (But I think it can happen a lot in slider-streams ?)

Simply returning might be better here. It would mean that the tick would be ignored until the head is hit or missed. (seems better to risk missing two elements instead of one in some cases than risk breaking combo when the player was about to hit the head in time in the other cases)

Or maybe a distinction can be made specifically for the case where the player didn't press any new key since the last object ? (might be better if doable)

Seems valid, although

Or maybe a distinction can be made specifically for the case where the player didn't press any new key since the last object ? (might be better if doable)

would be more complexity than I'm willing to include.

Disagree with this in the way that's it's proposed. It would mean someone that a pixel-missed slider would never gain tracking unless they realised in time and repressed either on the slider ball or slider head. I feel like that behaviour would not be intuitive.

There might be a better way of handling it, though.

It would mean someone that a pixel-missed slider would never gain tracking unless they realised in time and repressed either on the slider ball or slider head.

That's not fully correct, they would be able to gain tracking again once the head is missed. But the thing is they missed one element anyway (the head) so it's not that much cost to miss a few ticks along the way (while in the other scenario, it's breaking combo when it should not, because the player was on his way to the slider-head).

And if you really want to mitigate, then there's my suggestion about key-presses. It's a bit complex but very similar to what is done in the tracking code with timeToAcceptAnyKeyAfter ?

I applied this change in the recent commits.

Zyfarok · 2023-12-17T00:26:34Z

osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTail.cs

@@ -125,36 +121,7 @@ protected override void UpdateHitStateTransforms(ArmedState state)
            }
        }

-        protected override void CheckForResult(bool userTriggered, double timeOffset)
-        {
-            if (userTriggered)


Not sure what this test was doing* but better be safe than sorry:
Is this test no longer needed ?

(*: I looked at the doc of UpdateResult but still didn't get what userTriggered means exactly)

userTriggered = true comes from user input, if somewhere UpdateResult(true) was called.

It's irrelevant here because that's not called.

Zyfarok · 2023-12-17T00:31:59Z

I was looking just by curiosity and was not planning to code-review but ended up making a pass on everything except the test scene. I hope those comments can help. Everything else seemed sane.

(Edit: made some update after thinking more about my 1st comment)

Zyfarok · 2023-12-17T01:41:14Z

osu.Game.Rulesets.Osu/Objects/Drawables/SliderInputManager.cs

+                }
+                else
+                    nested.HitForcefully();
+            }


So, I asked what was preferred and it seems that they prefer always missing all ticks if any tick is not in range. Thus the suggested change would be something like this:

// Check if all past ticks are in range foreach (var nested in slider.NestedHitObjects.OfType<DrawableOsuHitObject>()) { // Skip nested objects that are already judged. if (nested.Judged) continue; // Stop the process when a nested object is reached that can't be hit before the current time. if (nested.HitObject.StartTime > Time.Current) break; float radius = getFollowRadius(true); double objectProgress = Math.Clamp((nested.HitObject.StartTime - slider.HitObject.StartTime) / slider.HitObject.Duration, 0, 1); Vector2 objectPosition = slider.HitObject.CurvePositionAt(objectProgress); // If any nested object that is further outside the follow area is reached, // forcefully miss all nested objects. if ((objectPosition - mousePositionInSlider).LengthSquared > radius * radius) { forceMiss = true; break; } } // Validate or miss all past ticks foreach (var nested in slider.NestedHitObjects.OfType<DrawableOsuHitObject>()) { if (nested.Judged) continue; if (nested.HitObject.StartTime > Time.Current) break; if (forceMiss) nested.MissForcefully(); else nested.HitForcefully(); }

Thanks for the feedback, but strongly disagree with this one, we'll go with what we have.

I don't think I mind much either way, but this is an extreme edge case imo.

(Edit: Sorry I just saw that you applied the change already)

Sorry if this was not clear, the reasoning for that was the same as here : #25776 (comment)
Basically, it doesn't make sense for the 2nd example above (Late hit outside tracking radius https://drive.google.com/file/d/10gdU9WV2ze0jRVaDiqov1I-jpgJMO3q1/view?usp=share_link) to be full misses but the 4th example above (Sliders with outside extremums https://drive.google.com/file/d/10hvIwpzszqceWP-5NI6-WMcFoJDZJ-gc/view) to be hitting the first ticks.
Because in both cases, the ball has left the follow-circle range at one point.

I applied this change in the recent commits.

Zyfarok · 2023-12-17T01:49:02Z

osu.Game.Rulesets.Osu/Objects/Drawables/SliderInputManager.cs

+            }
+
+            // Enable tracking, since the mouse is within the follow area (if it were expanded).
+            updateTracking(true);


I just realized that it might make more sense to not start tracking if forceMiss is true and the cursor is not on the ball. (we missed some tick so the ball should no longer be active)
So it should be:

updateTracking(!forceMiss || isMouseInFollowArea(false));

Which is actually also equivalent to

updateTracking(isMouseInFollowArea(!forceMiss));

@smoogipoo this seems valid, please check whether you agree.

Is there a specific reason why this makes more sense to you?

When it comes to these extreme edge cases, I tend to prefer to follow the easier-to-explain mechanic. In this case, that if you're within the tracking radius at the point where the head is hit, you gain tracking.

I feel like we are simulating the movement of the ball as if the ball was instantly reproducing the movement from the start to where it should be at the time. This means we start tracking on the head hit, and as soon as the ball is further than follow-circle away, the ball deactivates (which justifies the miss all the other ticks). If the ball finishes it's movement under the cursor it should reactivate though (which is why we have isMouseInFollowArea(false) in that case)

I applied this change in the recent commits.

smoogipoo · 2023-12-17T11:04:56Z

Gonna block this one for a bit. Will apply some of the suggestions above and think I might've discovered an issue with the tail.

smoogipoo · 2023-12-17T11:52:56Z

This is now once again ready to go. I've updated the OP to -- hopefully -- explain the new mechanics.

peppy · 2023-12-18T04:05:18Z

I'm going to merge this in as-is with the goal of getting it out for testing (and using the tests as a general guarantee that things are working). As discussed with smoogi IRL, there's going to need to be some refactoring on this code – for one I don't want SliderInputManager to exist – but in attempting to do this I managed to break tests, so will save it for post-release.

Karoo13 · 2023-12-19T05:43:11Z

The discussion thread wanted to check if the follow circle had "at any point" been too far from the mouse position, but this is not feasible.

What makes this not feasible? This is my main qualm with the implementation described, since its pretty common to have buzz sliders that leave the follow radius and re-enter.

While following the slider it is checked every frame whether the cursor is in the follow radius, so why doesn't that checking just start at the head time, and following is tracked with a growing/shrinking follow radius as normal (but the followcircle is invisible and holding click is not required), each individual tick is marked as followed or not, and then when the head is clicked all marked followed hits are scored, or if the head is missed they are missed? This will give much more accurate results in the edge cases, including when you failed to follow just some ticks before clicking, the late hit will give score for the rest you would have followed.

I am also a bit concerned about 2b scoring order, but I think it would be ok to reset 2b scores to fix mechanics edge cases later.

smoogipoo · 2023-12-19T05:51:32Z

It's something that can be worked on with a refactoring of this, sure.

peppy · 2023-12-19T07:06:47Z

I am also a bit concerned about 2b scoring order

Order should be enforced.

Please start by testing, then opening a discussion. We generally don't have discussions on merged PR branches, or deal with hypotheticals without evidence backing them up.

smoogipoo added 4 commits December 15, 2023 14:14

Add lenience for late-hit of slider heads

599fdb0

Refactor all slider input into SliderInputManager

6bd190c

Refactor test

3b8a73b

Use the cursor position to test nested object validity

1221001

smoogipoo added ruleset/osu! area:gameplay labels Dec 15, 2023

pull-request-size bot added the size/XL label Dec 15, 2023

Add some text to the test scene showing hits/misses

9ae3be8

peppy self-requested a review December 15, 2023 09:58

Remove unused using

acb7016

bdach added the next release Pull requests which are almost there. We'll aim to get them in the next release, but no guarantees! label Dec 16, 2023

Zyfarok reviewed Dec 16, 2023

View reviewed changes

Zyfarok reviewed Dec 17, 2023

View reviewed changes

peppy and others added 2 commits December 17, 2023 13:00

Merge branch 'master' into slider-late-hit-lenience

c0e9692

Only hit passed-through ticks if none were missed

f77884b

smoogipoo added the blocked Don't merge this. label Dec 17, 2023

smoogipoo added 3 commits December 17, 2023 20:11

Fix tail being missed too early

fbe48d7

Only track if in slider ball after any ticks missed

9b02bd7

Fix 1-frame issues due to referencing external value

fddfa33

smoogipoo removed the blocked Don't merge this. label Dec 17, 2023

Require slider head to be judged before ticks

2b33aec

pull-request-size bot added size/XXL and removed size/XL labels Dec 17, 2023

Fix assertions

04d5421

smoogipoo mentioned this pull request Dec 18, 2023

Late hits on short sliders can cause incorrect combo breaks #25651

Closed

peppy approved these changes Dec 18, 2023

View reviewed changes

peppy merged commit 2f28a92 into ppy:master Dec 18, 2023
15 of 17 checks passed

ppy locked and limited conversation to collaborators Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add slider head circle late hit lenience #25776

Add slider head circle late hit lenience #25776

smoogipoo commented Dec 15, 2023 •

edited

Loading

Zyfarok Dec 16, 2023 •

edited

Loading

peppy Dec 17, 2023

Zyfarok Dec 17, 2023 •

edited

Loading

Zyfarok Dec 16, 2023 •

edited

Loading

peppy Dec 17, 2023

smoogipoo Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

smoogipoo Dec 17, 2023

Zyfarok Dec 17, 2023 •

edited

Loading

smoogipoo Dec 17, 2023

Zyfarok commented Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

peppy Dec 17, 2023

smoogipoo Dec 17, 2023

Zyfarok Dec 17, 2023 •

edited

Loading

smoogipoo Dec 17, 2023

Zyfarok Dec 17, 2023 •

edited

Loading

peppy Dec 17, 2023

smoogipoo Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

smoogipoo Dec 17, 2023

smoogipoo commented Dec 17, 2023

smoogipoo commented Dec 17, 2023

peppy commented Dec 18, 2023

Karoo13 commented Dec 19, 2023 •

edited

Loading

smoogipoo commented Dec 19, 2023

peppy commented Dec 19, 2023

Add slider head circle late hit lenience #25776

Add slider head circle late hit lenience #25776

Conversation

smoogipoo commented Dec 15, 2023 • edited Loading

Late hit inside tracking radius

Late hit outside tracking radius

Miss hit inside tracking radius

Sliders with required outside tracking point

Short slider with repeats and ticks

Zyfarok Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zyfarok Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Zyfarok Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Zyfarok Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zyfarok Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zyfarok commented Dec 17, 2023 • edited Loading

Zyfarok Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zyfarok Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zyfarok Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Zyfarok Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoogipoo commented Dec 17, 2023

smoogipoo commented Dec 17, 2023

peppy commented Dec 18, 2023

Karoo13 commented Dec 19, 2023 • edited Loading

smoogipoo commented Dec 19, 2023

peppy commented Dec 19, 2023

smoogipoo commented Dec 15, 2023 •

edited

Loading

Zyfarok Dec 16, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

Zyfarok Dec 16, 2023 •

edited

Loading

smoogipoo Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

Zyfarok commented Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

smoogipoo Dec 17, 2023 •

edited

Loading

Zyfarok Dec 17, 2023 •

edited

Loading

Karoo13 commented Dec 19, 2023 •

edited

Loading