Correctly implement stable notelock in Classic mod #24280

tybug · 2023-07-18T04:05:44Z

As far as I can tell, stable notelock was never implemented correctly in lazer. This PR implements stable notelock effectively 1:1.

There are two obvious behavioral changes in this diff, though there may be more that are obscured by the details.

on lazer master, clicks on stacked hitobjects which are notelocked cause shakes, while on stable they don't (yes, despite the note being notelocked. stacks are special-cased on stable.)
stable has a weird extra 3 ms of leniency before hitobjects can be notelocked. Not sure why — the "unsnapped objects" comment doesn't make much sense to me — but it's back now.

Relevant stable code:

        internal virtual ClickAction CheckClickAction(HitObject checkObject)
        {
            if (checkObject.IsType(HitObjectType.Normal))
            {
                //check stack - ignore clicks on circles lower in stack.
                int index = hitObjectManager.hitObjectsMinimal.IndexOf(checkObject);

                if (index > 0 && hitObjectManager.hitObjectsMinimal[index - 1].StackCount > 0 &&
                    hitObjectManager.hitObjectsMinimal[index - 1].IsVisible &&
                    !hitObjectManager.hitObjectsMinimal[index - 1].IsHit)
                    return ClickAction.Ignore;
            }

            foreach (HitObject testObject in hitObjectManager.hitObjectsMinimal)
            {
                if (testObject.IsHit) continue;

                // if we found the object being checked, we can move on to the final timing test.
                if (testObject == checkObject)
                    break;

                // for all other objects, we check for validity and block the hit if any are still valid.
                // 3ms of extra leniency to account for slightly unsnapped objects.
                if (testObject.EndTime + 3 < checkObject.StartTime)
                    return ClickAction.Shake;
            }

            int autopilotAdjust = Player.Relaxing2 ? 200 : 0;
            return Math.Abs(checkObject.StartTime - AudioEngine.Time) < HitObjectManager.HITTABLE_RANGE - autopilotAdjust
                       ? ClickAction.Hit
                       : ClickAction.Shake;
        }

I'm very aware that a change this far reaching deserves tests...but I'm honestly not sure how to easily test this (particularly a note being shaken vs ignored). Pointers in that regard would be appreciated.

Tested to resolve Stacked Sliders behave differently with classic mod than how they do in stable #17675
...and probably more. I tested two (1, 2) random stable replays and both exhibited incorrect judgments fixed by this pr.

osu.Game/Rulesets/UI/IHitObjectContainer.cs

tybug · 2023-07-18T04:09:57Z

osu.Game.Rulesets.Osu/UI/IHitPolicy.cs

@@ -20,7 +20,7 @@ public interface IHitPolicy
        /// <param name="hitObject">The <see cref="DrawableHitObject"/> to check.</param>
        /// <param name="time">The time to check.</param>
        /// <returns>Whether <paramref name="hitObject"/> can be hit at the given <paramref name="time"/>.</returns>
-        bool IsHittable(DrawableHitObject hitObject, double time);
+        ClickAction CheckHittable(DrawableHitObject hitObject, double time);


Open to renamings of both CheckHittable and ClickAction. ClickAction was brought across from the stable naming.

tybug · 2023-07-18T04:11:11Z

osu.Game.Rulesets.Osu/UI/ObjectOrderedHitPolicy.cs

-                    yield break;
+                var previousHitObject = (DrawableOsuHitObject)HitObjectContainer.AliveObjects[index - 1];
+                if (previousHitObject.HitObject.StackHeight > 0 && !previousHitObject.AllJudged)
+                    return ClickAction.Ignore;


this case right here is why the ClickAction abstraction is necessary. There are times when a note should, by all other metrics, be shaken, but we don't want it to be (e.g. stacks).

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

tybug · 2023-07-18T05:09:39Z

I see the test failures (was accidentally only running osu.Game.Tests locally). Will address when I wake. Don't submit PRs right before sleep! 😄

stable actually allows for hitobjs to be hit in the middle of sliders, as long as it doesn't interfere with the end time of the slider.

tybug · 2023-07-19T06:17:54Z

I've fixed one test in 15af852 by adjusting expected judgments to the correct notelock: believe it or not (I didn't, and had to test against stable), stable allows hitobjects to be hit even while a slider head is the primary / next object, as long as that hitobject doesn't interfere with the end time of the slider. I guess that's another stable mismatch bug fixed by this pull that I didn't even know about.

The other two failing tests are a result of custom hitwindows. Since this pull (and stable) hardcodes the notelock check at 400ms instead of using hitwindow information - which happens to also be 400ms for HitResult.Miss - the notelock is wrong relative to the test hitwindow.

On one hand, the obvious solution here seems to be to respect hitwindows in ObjectOrderedHitPolicy. However, if the default lazer hitwindows change in the future, the classic mod should still use continue using stable hitwindows at 400. Maybe in the latter case the hit policy should respect hitwindows, but the classic mod would be responsible for setting stable hitwindows game-wide. Just thinking about potential future footguns here if adding that is forgotten. Interested in thoughts here.

bdach · 2023-07-19T18:35:51Z

osu.Game.Rulesets.Osu/UI/ClickAction.cs

As for rename suggestions - maybe HitResponse or something instead? Maybe overthought. What you have is probably fine.

osu.Game.Rulesets.Osu/UI/ObjectOrderedHitPolicy.cs

bdach · 2023-07-19T18:44:31Z

I'm very aware that a change this far reaching deserves tests...but I'm honestly not sure how to easily test this (particularly a note being shaken vs ignored). Pointers in that regard would be appreciated.

Tests would go a long way here yes. Loose sketch of how writing some may be possible (very untested, buyer beware):

Leverage existing setup in TestSceneObjectOrderedHitPolicy to get a player instance.
In order to observe the outputs of a hit policy, yank OsuPlayfield out of the player via ChildrenOfType<T>() and set its HitPolicy to an implementation which wraps ObjectOrderedHitPolicy and stores the results of CheckHittable to a list. Basically decorator pattern.
Use test setup as normal by setting up replay frames. At the end, rather than examine object judgements, check ClickActions stored to the list in the injected hit policy.

On one hand, the obvious solution here seems to be to respect hitwindows in ObjectOrderedHitPolicy. However, if the default lazer hitwindows change in the future, the classic mod should still use continue using stable hitwindows at 400. Maybe in the latter case the hit policy should respect hitwindows, but the classic mod would be responsible for setting stable hitwindows game-wide. Just thinking about potential future footguns here if adding that is forgotten. Interested in thoughts here.

Let's not try to future-proof too much and just hardcode the constant along with the rest of the legacy cruft. As far as I'm concerned we're not changing hitwindows anyway without a whole nother huge effort to somehow not break every leaderboard in existence. I don't even know why'd be changing hitwindows to be honest.

tybug · 2023-07-20T01:57:46Z

Let's not try to future-proof too much and just hardcode the constant along with the rest of the legacy cruft.

Well, hardcoding the constant is what's causing the tests to fail, since the tests use custom hitwindows. Should I remove/adjust the tests?

bdach · 2023-07-20T19:24:03Z

Well, hardcoding the constant is what's causing the tests to fail, since the tests use custom hitwindows

Oh. Right, I somehow had this backwards. In that case yeah adjusting the tests sounds best.

bdach · 2023-08-22T18:53:27Z

Just as a heads up: I have put a considerable amount of time into looking at what needs to happen to get this one merged, and I feel I've gathered enough to work with for one or two possible avenues of shipping this change. I'll leave this be for the time being, but tomorrow I'll come back to this with a fresh mind and hopefully I can get this unblocked and also provide testing for the behaviours this new implementation aims to deliver.

tybug · 2023-08-22T19:01:23Z

Thanks! To be honest, I've been putting off working on this because I know it'll take me a while to wrap my head around writing tests for it.

bdach · 2023-08-23T12:23:45Z

I believe I've got a version of this branch that I would consider ready lined up, but I'm waiting for the reaction to #24627 and #24628 as I want to build this one on them, so I'd rather want to avoid force pushing them out if it turns out they aren't going to get approval.

The WIP version can be seen here, if anyone cares: https://github.com/ppy/osu/compare/master...bdach:osu:stable-notelock-working?expand=1 - if the two above pulls get merged then I'll push those changes in here.

bdach · 2023-08-24T07:00:43Z

@ppy/team-client should be good for review now. @tybug would appreciate your cross-check as well that I haven't broken anything with my changes, if I can get it.

bdach · 2023-08-24T07:02:22Z

osu.Game.Rulesets.Osu.Tests/TestSceneLegacyHitPolicy.cs

@@ -191,7 +198,9 @@ public void TestClickSecondCircleBeforeFirstCircleTimeWithFirstCircleJudged()
            addJudgementAssert(hitObjects[0], HitResult.Meh);
            addJudgementAssert(hitObjects[1], HitResult.Meh);
            addJudgementOffsetAssert(hitObjects[0], -190); // time_first_circle - 190
-            addJudgementOffsetAssert(hitObjects[0], -90); // time_second_circle - first_circle_time - 90
+            addJudgementOffsetAssert(hitObjects[1], -190); // time_second_circle - first_circle_time - 90


I accidentally broke this assertion in #24628, but it was broken before I touched the test scene (note that it used to check hitObjects[0] twice). I'm just fixing here to reduce overall ceremony.

tybug · 2023-08-25T04:59:55Z

I checked out this branch to test, but it seems to have a renderer-level crash for me when entering song select:

runtime.log

Stacktrace

2023-08-25 04:50:52 [verbose]: 🌅 Global background change queued
2023-08-25 04:50:53 [error]: An unhandled error has occurred.
2023-08-25 04:50:53 [error]: Veldrid.VeldridException: Error(s) were encountered during the execution of OpenGL commands. See InnerException for more information.
2023-08-25 04:50:53 [error]: ---> Veldrid.VeldridException: Error linking GL program: WARNING: Could not find vertex shader attribute 'm_TexturePosition' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: WARNING: Could not find vertex shader attribute 'm_TextureRect' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: WARNING: Could not find vertex shader attribute 'm_maskingIndex' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: WARNING: Could not find vertex shader attribute 'm_backbufferDrawDepth' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: ERROR: Input of fragment shader 'vdspv_fsin0' not written by vertex shader
2023-08-25 04:50:53 [error]: 
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLPipeline.CreateGraphicsGLResources()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLPipeline.CreateGLResources()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLCommandExecutor.ActivateGraphicsPipeline()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLCommandExecutor.SetPipeline(Pipeline pipeline)
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.NoAllocEntryList.OpenGLNoAllocCommandEntryList.ExecuteAll(OpenGLCommandExecutor executor)
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLGraphicsDevice.ExecutionThread.ExecuteWorkItem(ExecutionThreadWorkItem workItem)
2023-08-25 04:50:53 [error]: --- End of inner exception stack trace ---
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLGraphicsDevice.ExecutionThread.CheckExceptions()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLGraphicsDevice.ExecutionThread.WaitForIdle()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLGraphicsDevice.SwapBuffersCore(Swapchain swapchain)
2023-08-25 04:50:53 [error]: at osu.Framework.Platform.GameHost.Swap()
2023-08-25 04:50:53 [error]: at osu.Framework.Platform.MacOS.MacOSGameHost.Swap()
2023-08-25 04:50:53 [error]: at osu.Framework.Platform.GameHost.DrawFrame()
2023-08-25 04:50:53 [error]: at osu.Framework.Threading.GameThread.processFrame()
2023-08-25 04:50:53 [error]: An unhandled error has occurred. (inner)
2023-08-25 04:50:53 [error]: Veldrid.VeldridException: Error linking GL program: WARNING: Could not find vertex shader attribute 'm_TexturePosition' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: WARNING: Could not find vertex shader attribute 'm_TextureRect' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: WARNING: Could not find vertex shader attribute 'm_maskingIndex' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: WARNING: Could not find vertex shader attribute 'm_backbufferDrawDepth' to match BindAttributeLocation request.
2023-08-25 04:50:53 [error]: ERROR: Input of fragment shader 'vdspv_fsin0' not written by vertex shader
2023-08-25 04:50:53 [error]: 
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLPipeline.CreateGraphicsGLResources()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLPipeline.CreateGLResources()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLCommandExecutor.ActivateGraphicsPipeline()
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLCommandExecutor.SetPipeline(Pipeline pipeline)
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.NoAllocEntryList.OpenGLNoAllocCommandEntryList.ExecuteAll(OpenGLCommandExecutor executor)
2023-08-25 04:50:53 [error]: at Veldrid.OpenGL.OpenGLGraphicsDevice.ExecutionThread.ExecuteWorkItem(ExecutionThreadWorkItem workItem)
2023-08-25 04:50:53 [verbose]: ⚠️ An unhandled error has occurred.
2023-08-25 04:50:53 [verbose]: Unhandled exception has been denied .

Does not occur with metal renderer. I'm on macos but am using opengl temporarily because of #23970 (which seems to be partially resolved by now; no startup crash anymore, at least).

I'm really confused here, because this does not occur on latest master, but does on this branch consistently (n=4 retries each). I can't see what changes in this branch are related to the renderer, which is why I'm hesitant to even post this here...have I hit a freak probabilistic crash that only appears to occur on this pr?

bdach · 2023-08-25T05:10:02Z

There should be no reason why this branch specifically would trigger any sort of renderer level crash. Especially not on song select of all places. That's what I'd like to think, at least.

tybug · 2023-08-25T05:19:07Z

There should be no reason why this branch specifically would trigger any sort of renderer level crash.

That's what I figured. Looked into it more and I forgot to sync my fork ;-; so it is actually crashing on master as well. I'll open another issue for that.

tybug · 2023-08-25T05:44:38Z

I've tested the replays linked in the PR description and one other problematic one, and all behave as I would expect, so things look good purely from a visual perspective.

Two comments, though - first, I don't think there's a test for the 3ms of leniency. This was one of the things that I didn't explicitly test myself with a replay, though I can't see why it wouldn't work as expected. If it's judged that case isn't worth a test then that's of course fine with me.

A separate comment is that with this PR, replay 1 in the description - which is an FC - consistently breaks combo right around ~2100x (~5:18), but only when fast-forwarding to a point past it. This doesn't happen on master. I believe there's still various "fast-forward playback can be wrong" issues, so this probably isn't a blocker, but I'll mention it regardless since it's new behavior introduced here.

bdach · 2023-08-25T05:47:32Z

first, I don't think there's a test for the 3ms of leniency.

Yeah there isn't. I decided not to bother with that one. Up to reviewers whether that's an issue I guess. It shouldn't be very difficult to come up with one if required.

replay 1 in the description - which is an FC - consistently breaks combo right around ~2100x (~5:18), but only when fast-forwarding to a point past it

I think we'd be looking to address that separately anyhow. The core takeaway is that it is more correct at 1x.

correct implementation of stable notelock

768d7b5

pull-request-size bot added the size/L label Jul 18, 2023

tybug commented Jul 18, 2023

View reviewed changes

osu.Game/Rulesets/UI/IHitObjectContainer.cs Outdated Show resolved Hide resolved

tybug commented Jul 18, 2023

View reviewed changes

osu.Game.Rulesets.Osu/Objects/Drawables/DrawableHitCircle.cs Outdated Show resolved Hide resolved

peppy added the area:gameplay label Jul 18, 2023

adjust test for correct stable notelock

15af852

stable actually allows for hitobjs to be hit in the middle of sliders, as long as it doesn't interfere with the end time of the slider.

convert AliveObjects to list in hit policy instead of globally

2c97ac7

bdach reviewed Jul 19, 2023

View reviewed changes

rename ObjectOrderedHitPolicy to LegacyHitPolicy

6a81230

tybug changed the title ~~Correctly implement stable notelock~~ Correctly implement stable notelock in Classic mod Jul 20, 2023

bdach self-assigned this Aug 22, 2023

This was referenced Aug 23, 2023

Possible difference between lazer and stable in input handling with overlapping sliders #24626

Closed

Rewrite TestSceneObjectOrderedHitPolicy to not rely on custom hitwindows #24627

Merged

bdach added 7 commits August 24, 2023 08:43

Merge branch 'master' into stable-notelock

b777b34

Merge branch 'master' into stable-notelock

1f3121b

Add assertions checking action type taken by legacy hit policy

a045cb7

Add test case covering overlapping slider fail case

9ffc6cd

Add test case covering stacks not shaking

48b3f7d

Rename test scene to match tested class

64b36a7

Contain entirety of legacy notelock badness inside the hit policy

ed2b1a0

bdach added 3 commits August 24, 2023 08:52

Add failing test case for special autopilot hittable range

a316036

Use reduced hittable range with autopilot active

7f215f1

Fix broken assertion

38e3459

bdach reviewed Aug 24, 2023

View reviewed changes

bdach requested a review from a team August 31, 2023 22:17

peppy self-requested a review September 1, 2023 07:51

Apply NRT to new LegacyHitPolicy class (and other implementations)

c75b14c

peppy previously approved these changes Sep 1, 2023

View reviewed changes

Merge branch 'master' into stable-notelock

9012181

peppy dismissed their stale review via e668938 September 1, 2023 08:19

Remove broken check from TestSceneHitCircleLateFade

ede9fae

peppy force-pushed the stable-notelock branch from e668938 to ede9fae Compare September 1, 2023 08:29

peppy approved these changes Sep 1, 2023

View reviewed changes

peppy merged commit 8d0f6df into ppy:master Sep 1, 2023
17 checks passed

tybug deleted the stable-notelock branch September 4, 2023 04:05

bdach mentioned this pull request Sep 5, 2023

Block input to objects lying under already-hit hitcircles when classic note lock is active #24720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly implement stable notelock in Classic mod #24280

Correctly implement stable notelock in Classic mod #24280

tybug commented Jul 18, 2023 •

edited

tybug Jul 18, 2023

tybug Jul 18, 2023

tybug commented Jul 18, 2023

tybug commented Jul 19, 2023 •

edited

bdach Jul 19, 2023

bdach commented Jul 19, 2023

tybug commented Jul 20, 2023

bdach commented Jul 20, 2023

bdach commented Aug 22, 2023 •

edited

tybug commented Aug 22, 2023

bdach commented Aug 23, 2023 •

edited

bdach commented Aug 24, 2023

bdach Aug 24, 2023

tybug commented Aug 25, 2023 •

edited

bdach commented Aug 25, 2023

tybug commented Aug 25, 2023

tybug commented Aug 25, 2023 •

edited

bdach commented Aug 25, 2023

Correctly implement stable notelock in Classic mod #24280

Correctly implement stable notelock in Classic mod #24280

Conversation

tybug commented Jul 18, 2023 • edited

tybug Jul 18, 2023

Choose a reason for hiding this comment

tybug Jul 18, 2023

Choose a reason for hiding this comment

tybug commented Jul 18, 2023

tybug commented Jul 19, 2023 • edited

bdach Jul 19, 2023

Choose a reason for hiding this comment

bdach commented Jul 19, 2023

tybug commented Jul 20, 2023

bdach commented Jul 20, 2023

bdach commented Aug 22, 2023 • edited

tybug commented Aug 22, 2023

bdach commented Aug 23, 2023 • edited

bdach commented Aug 24, 2023

bdach Aug 24, 2023

Choose a reason for hiding this comment

tybug commented Aug 25, 2023 • edited

bdach commented Aug 25, 2023

tybug commented Aug 25, 2023

tybug commented Aug 25, 2023 • edited

bdach commented Aug 25, 2023

tybug commented Jul 18, 2023 •

edited

tybug commented Jul 19, 2023 •

edited

bdach commented Aug 22, 2023 •

edited

bdach commented Aug 23, 2023 •

edited

tybug commented Aug 25, 2023 •

edited

tybug commented Aug 25, 2023 •

edited