Fix editor test play not marking hit objects before its start time as judged #26465

LeNitrous · 2024-01-10T07:28:12Z

Fixes #17395 and #23029 using the suggestion from #17395 (comment).

osu._2024.01.10-15.24.mp4

osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs

frenzibyte

osu!catch has automagically been working prior to this PR due to the nature of its hit object judgement logic (judging all object prior to current time as miss, instead of ignoring).

With this PR, now the judged hits count goes over the maximum value, as all previous objects increase the judged hits counter with "miss" judgements after the score processor has already been set to the calculated judged hits in this PR. This results in the osu!catch editor test player not being able to exit at finish (whereas on master it works).

An alternative method I have in mind would be to give maximum results to the actual hit objects of the test session themselves (which are assigned as entries to the drawable hit objects), so that they're not judged with a "miss" later on. I tried doing that, but I cannot seem to get it work at first attempt...something along the lines of the following diff:

diff

diff --git a/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs b/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs
index e2b2b067e0..f8fdb1f809 100644
--- a/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs
+++ b/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs
@@ -1,6 +1,7 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using osu.Framework.Allocation;
@@ -8,6 +9,7 @@
 using osu.Game.Beatmaps;
 using osu.Game.Online.Spectator;
 using osu.Game.Overlays;
+using osu.Game.Rulesets.Judgements;
 using osu.Game.Rulesets.Objects;
 using osu.Game.Rulesets.Replays;
 using osu.Game.Rulesets.Scoring;
@@ -21,19 +23,17 @@ public partial class EditorPlayer : Player
     {
         private readonly Editor editor;
         private readonly EditorState editorState;
-        private readonly IBeatmap playableBeatmap;
 
         protected override UserActivity InitialActivity => new UserActivity.TestingBeatmap(Beatmap.Value.BeatmapInfo, Ruleset.Value);
 
         [Resolved]
         private MusicController musicController { get; set; } = null!;
 
-        public EditorPlayer(Editor editor, IBeatmap playableBeatmap)
+        public EditorPlayer(Editor editor)
             : base(new PlayerConfiguration { ShowResults = false })
         {
             this.editor = editor;
             editorState = editor.GetState();
-            this.playableBeatmap = playableBeatmap;
         }
 
         protected override GameplayClockContainer CreateGameplayClockContainer(WorkingBeatmap beatmap, double gameplayStart)
@@ -52,21 +52,16 @@ protected override void LoadComplete()
         {
             base.LoadComplete();
 
-            var frame = new ReplayFrame { Header = new FrameHeader(new ScoreInfo(), new ScoreProcessorStatistics()) };
-
-            foreach (var hitObject in enumerateHitObjects(playableBeatmap.HitObjects.Where(h => h.StartTime < editorState.Time)))
+            foreach (var hitObject in enumerateHitObjects(DrawableRuleset.Objects))
             {
-                var judgement = hitObject.CreateJudgement();
-
-                if (!frame.Header.Statistics.ContainsKey(judgement.MaxResult))
-                    frame.Header.Statistics.Add(judgement.MaxResult, 0);
+                if (hitObject.StartTime >= editorState.Time)
+                    continue;
 
-                frame.Header.Statistics[judgement.MaxResult]++;
+                var judgement = hitObject.CreateJudgement();
+                HealthProcessor.ApplyResult(new JudgementResult(hitObject, judgement) { Type = judgement.MaxResult });
+                ScoreProcessor.ApplyResult(new JudgementResult(hitObject, judgement) { Type = judgement.MaxResult });
             }
 
-            HealthProcessor.ResetFromReplayFrame(frame);
-            ScoreProcessor.ResetFromReplayFrame(frame);
-
             ScoreProcessor.HasCompleted.BindValueChanged(completed =>
             {
                 if (completed.NewValue)

osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs

frenzibyte · 2024-01-11T03:55:33Z

In addition, there also seem to be specific scenarios at which editor test play still fails to exit on finish:

CleanShot.2024-01-11.at.06.53.57.mp4

At this point it may seem more ideal to add local code somewhere to mark gameplay as "completed" when being 1000ms ahead of the last hit object end time. Something to keep in mind as a better alternative.

bdach · 2024-01-11T08:21:26Z

At this point it may seem more ideal to add local code somewhere to mark gameplay as "completed" when being 1000ms ahead of the last hit object end time. Something to keep in mind as a better alternative.

disagree. i would like an explanation as to why this method is failing before resorting to such hacks

frenzibyte · 2024-01-11T10:19:22Z

Hence why I mentioned it as an alternative.

bdach · 2024-01-11T10:23:22Z

i don't consider it a valid alternative unless significant extenuating circumstances are provided.

frenzibyte · 2024-01-11T11:46:39Z

I will wait for a response to the issues I pointed above to get the current method in a fully working state (specifically #26465), and then discuss it from there.

LeNitrous · 2024-01-11T12:03:13Z

At this point it may seem more ideal to add local code somewhere to mark gameplay as "completed" when being 1000ms ahead of the last hit object end time. Something to keep in mind as a better alternative.

disagree. i would like an explanation as to why this method is failing before resorting to such hacks

It was caused by not accounting for hit windows and hit objects with a duration causing hit objects to be judged twice making JudgedHits > MaxHits in JudgementProcessor.

osu!catch has automagically been working prior to this PR due to the nature of its hit object judgement logic (judging all object prior to current time as miss, instead of ignoring).

Other modes were working as well. It's because in other modes other than standard, they mark all hit objects after a certain time as missed causing JudgedHits * 2 ≈ MaxHits in this PR.

After a bit more digging its likely that ApplyResult from hit objects is not being called to properly update JudgedHits for whatever reason and this solution is incorrect.

…ement count mismatch

…duration

peppy · 2024-05-29T14:33:22Z

I've fixed multiple edge cases and applied @frenzibyte's proposed changes above (applying results instead of using replay frame) as it solves the issue of DHOs currently on screen duplicating judgements (this could not be resolved with the replay frame approach).

Not sure if we want to try and craft tests for this but before expending the effort in doing so I'd like to see if anyone can break this.

bdach · 2024-05-29T15:01:04Z

this could not be resolved with the replay frame approach

Can you elaborate on this any further?

I guess I'll have to have a look myself at the end of the day at this point but I'm really loath to be inventing another way to do this. Especially so that I don't see how the replay frame mechanism can work for multi but not for editor. I will need to find out (myself or not) why that is the case before passing any judgement (whether there are some special editor circumstances at play, or whether the multi spectator mechanism is broken e.g. for catch and needs to be either fixed or replaced).

peppy · 2024-05-29T15:26:33Z

Can you elaborate on this any further?

This is correctly touched on by frenzi above, but I'll attempt.

Basically, assume there are multiple DHOs on screen, some before test_start_time and some after. When only using HitObjects to create the replay frame max judgement count (as previously attempted), we have no easy way of telling which DHOs are still on screen but before test_start_time (and will trigger their own ApplyResult(miss)).

We would need to use some concoction of PreEmpt or transforms or something else to get a correct count for the current frame of entry (and even then I'm not sure we could be 100% accurate).

In a simple scenario where past objects are not mapped to DHOs – ie. only objects after test_start_time exist – there will be no issue.

Which is to say, I honestly don't know how this works in replays (it probably doesn't always).

bdach · 2024-05-30T07:23:37Z

osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs

+            foreach (var hitObject in enumerateHitObjects(DrawableRuleset.Objects, editorState.Time))
+            {
+                var judgement = hitObject.Judgement;
+
+                HealthProcessor.ApplyResult(new JudgementResult(hitObject, judgement) { Type = judgement.MaxResult });
+                ScoreProcessor.ApplyResult(new JudgementResult(hitObject, judgement) { Type = judgement.MaxResult });
+            }


If the problem with the replay frame-based approach is flawed because it relied on using hitobjects and hitobject-based cutoffs, then how is this fine? This is still hitobject-based logic no? There are no DHOs here? Why is there no attempt to compensate for preempt or transforms or whatever here either?

That explanation isn't making much sense to me in light of this.

The hookup flow with ScoreProcessor via Player means that applying result here will have the results end up on the DHOs, seemingly. It's a long chain of events that bugs me every time i have to traverse it.

bdach · 2024-05-30T07:38:48Z

I'd like to see if anyone can break this.

Started https://osu.ppy.sh/beatmapsets/310607#mania/710980 on 01:37:623 and it kicked me out of test play before I actually completed the map:

2024-05-30.09-37-38.mp4

Another mania diff on the same set I tried, gameplay still stuck after end.

So as above, I don't see how this solution is any better than the previous one. It seems to be subject to the same exact issues.

peppy · 2024-05-30T08:09:16Z

👍 safe to say this is all doomed then, and go back to the drawing board.

bdach · 2024-06-26T11:03:08Z

Coming back to this: As far as I can tell, the assertions from this comment that going through ScoreProcessor does something with DHOs are pretty clearly false as far as I can tell. The two are not connected.

However, how do you feel about the following?

diff --git a/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs b/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs
index 2028094964..9a7c1822a3 100644
--- a/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs
+++ b/osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs
@@ -48,6 +48,7 @@ protected override void LoadComplete()
             base.LoadComplete();
 
             markPreviousObjectsHit();
+            markVisibleDrawableObjectsHit();
 
             ScoreProcessor.HasCompleted.BindValueChanged(completed =>
             {
@@ -67,9 +68,10 @@ private void markPreviousObjectsHit()
             foreach (var hitObject in enumerateHitObjects(DrawableRuleset.Objects, editorState.Time))
             {
                 var judgement = hitObject.Judgement;
+                var result = new JudgementResult(hitObject, judgement) { Type = judgement.MaxResult };
 
-                HealthProcessor.ApplyResult(new JudgementResult(hitObject, judgement) { Type = judgement.MaxResult });
-                ScoreProcessor.ApplyResult(new JudgementResult(hitObject, judgement) { Type = judgement.MaxResult });
+                HealthProcessor.ApplyResult(result);
+                ScoreProcessor.ApplyResult(result);
             }
 
             static IEnumerable<HitObject> enumerateHitObjects(IEnumerable<HitObject> hitObjects, double cutoffTime)
@@ -88,6 +90,40 @@ static IEnumerable<HitObject> enumerateHitObjects(IEnumerable<HitObject> hitObje
             }
         }
 
+        private void markVisibleDrawableObjectsHit()
+        {
+            if (!DrawableRuleset.Playfield.IsLoaded)
+            {
+                Schedule(markVisibleDrawableObjectsHit);
+                return;
+            }
+
+            foreach (var drawableObjectEntry in enumerateDrawableEntries(
+                         DrawableRuleset.Playfield.AllHitObjects
+                                        .Select(ho => ho.Entry)
+                                        .Where(e => e != null)
+                                        .Cast<HitObjectLifetimeEntry>(), editorState.Time))
+            {
+                drawableObjectEntry.Result = new JudgementResult(drawableObjectEntry.HitObject, drawableObjectEntry.HitObject.Judgement)
+                    { Type = drawableObjectEntry.HitObject.Judgement.MaxResult };
+            }
+
+            static IEnumerable<HitObjectLifetimeEntry> enumerateDrawableEntries(IEnumerable<HitObjectLifetimeEntry> entries, double cutoffTime)
+            {
+                foreach (var entry in entries)
+                {
+                    foreach (var nested in enumerateDrawableEntries(entry.NestedEntries, cutoffTime))
+                    {
+                        if (nested.HitObject.GetEndTime() < cutoffTime)
+                            yield return nested;
+                    }
+
+                    if (entry.HitObject.GetEndTime() < cutoffTime)
+                        yield return entry;
+                }
+            }
+        }
+
         protected override void PrepareReplay()
         {
             // don't record replays.

Is it pretty cursed? Yeah. Does it appear to work? In my (somewhat brief) testing, also yeah. It should also scale to non-pooled rulesets, thankfully, because of that whole SyntheticHitObjectEntry shebang that is present on non-pooled objects.

As far as I can tell the issue happens because the DHOs do not get the Result on their lifetime entry adjusted in line with what was faked on the score/HP processors. Therefore, especially in cases like osu! sliders or mania holds, it can occur that the DHO is not aware that it has already been accounted for in the judgement processors, and as such will issue judgement results itself, which leads to either ending gameplay early (if it causes the processors to hit JudgedHits == MaxHits prematurely), or not ending at all (if it causes the processors' JudgedHits to exceed MaxHits).

peppy · 2024-06-28T03:24:37Z

I think the diff looks okay.

- Covers fail case that wasn't covered before - Removes arbitrary wait step that was inevitably going to cause intermittent test failures

…or test play

resolved

LeNitrous added 2 commits January 10, 2024 15:19

Make all hit objects before the start time marked as hit.

b74c3b1

Enumerate nested hit objects

72e302d

pull-request-size bot added the size/M label Jan 10, 2024

add test case

aa83ac1

peppy added the area:editor label Jan 10, 2024

turbedi reviewed Jan 10, 2024

View reviewed changes

osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs Outdated Show resolved Hide resolved

frenzibyte self-requested a review January 11, 2024 02:43

Contains + Add to TryAdd

6cd255f

frenzibyte previously requested changes Jan 11, 2024

View reviewed changes

osu.Game/Screens/Edit/GameplayTest/EditorPlayer.cs Outdated Show resolved Hide resolved

peppy self-requested a review May 29, 2024 12:06

peppy added 5 commits May 29, 2024 21:08

Merge branch 'master' into fix/editor-test-exit

16e2fa2

Avoid passing beatmap in from editor when it's already present

d2c86b0

Fix the majority of cases where gameplay stil doesn't end due to judg…

3b5b7b2

…ement count mismatch

Apply results rather than fake a replay frame

126837f

Fix incorrect handling of nested objects when inside parent object's …

fdb47f8

…duration

bdach reviewed May 30, 2024

View reviewed changes

Merge branch 'master' into fix/editor-test-exit

b36e0f0

bdach added 3 commits June 28, 2024 09:30

Expand test coverage

ace6427

- Covers fail case that wasn't covered before - Removes arbitrary wait step that was inevitably going to cause intermittent test failures

Ensure past drawable objects also get their results populated in edit…

7ac5bd4

…or test play

Fix formatting

a3ea36d

bdach approved these changes Jun 28, 2024

View reviewed changes

peppy approved these changes Jun 28, 2024

View reviewed changes

bdach enabled auto-merge June 28, 2024 10:11

bdach disabled auto-merge June 28, 2024 10:12

bdach merged commit 81c6da9 into ppy:master Jun 28, 2024
13 of 17 checks passed

bdach changed the title ~~Fix editor test play not marking hit objects before its start time as judged.~~ Fix editor test play not marking hit objects before its start time as judged Jun 28, 2024

Joehuu mentioned this pull request Jun 28, 2024

Playtesting in the editor should start from 100% accuracy regardless of start position to match stable #23029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix editor test play not marking hit objects before its start time as judged #26465

Fix editor test play not marking hit objects before its start time as judged #26465

LeNitrous commented Jan 10, 2024

frenzibyte left a comment •

edited

Loading

frenzibyte commented Jan 11, 2024 •

edited

Loading

bdach commented Jan 11, 2024

frenzibyte commented Jan 11, 2024

bdach commented Jan 11, 2024 •

edited

Loading

frenzibyte commented Jan 11, 2024 •

edited

Loading

LeNitrous commented Jan 11, 2024 •

edited

Loading

peppy commented May 29, 2024

bdach commented May 29, 2024 •

edited

Loading

peppy commented May 29, 2024 •

edited

Loading

bdach May 30, 2024 •

edited

Loading

peppy May 30, 2024

bdach commented May 30, 2024 •

edited

Loading

peppy commented May 30, 2024

bdach commented Jun 26, 2024 •

edited

Loading

peppy commented Jun 28, 2024

Fix editor test play not marking hit objects before its start time as judged #26465

Fix editor test play not marking hit objects before its start time as judged #26465

Conversation

LeNitrous commented Jan 10, 2024

frenzibyte left a comment • edited Loading

Choose a reason for hiding this comment

frenzibyte commented Jan 11, 2024 • edited Loading

bdach commented Jan 11, 2024

frenzibyte commented Jan 11, 2024

bdach commented Jan 11, 2024 • edited Loading

frenzibyte commented Jan 11, 2024 • edited Loading

LeNitrous commented Jan 11, 2024 • edited Loading

peppy commented May 29, 2024

bdach commented May 29, 2024 • edited Loading

peppy commented May 29, 2024 • edited Loading

bdach May 30, 2024 • edited Loading

Choose a reason for hiding this comment

peppy May 30, 2024

Choose a reason for hiding this comment

bdach commented May 30, 2024 • edited Loading

peppy commented May 30, 2024

bdach commented Jun 26, 2024 • edited Loading

peppy commented Jun 28, 2024

frenzibyte left a comment •

edited

Loading

frenzibyte commented Jan 11, 2024 •

edited

Loading

bdach commented Jan 11, 2024 •

edited

Loading

frenzibyte commented Jan 11, 2024 •

edited

Loading

LeNitrous commented Jan 11, 2024 •

edited

Loading

bdach commented May 29, 2024 •

edited

Loading

peppy commented May 29, 2024 •

edited

Loading

bdach May 30, 2024 •

edited

Loading

bdach commented May 30, 2024 •

edited

Loading

bdach commented Jun 26, 2024 •

edited

Loading