Set a rudimentary lifetime end to improve seek performance in scrolling rulesets #28170
Conversation
| { | ||
| double computedStartTime = computeDisplayStartTime(entry); | ||
|
|
||
| // always load the hitobject before its first judgement offset | ||
| entry.LifetimeStart = Math.Min(entry.HitObject.StartTime - entry.HitObject.MaximumJudgementOffset, computedStartTime); | ||
| entry.LifetimeEnd = entry.HitObject.GetEndTime() + timeRange.Value; |
There was a problem hiding this comment.
I think at the very least this should take account of entry.HitObject.MaximumJudgementOffset like the previous line. The case here is an object scrolls past the hit point and time range but is still within the hit window.
The further concern I have, and that I would like a test for, is what happens if a hitobject scrolls from t < LifetimeStart to t > LifetimeEnd in one frame - does it get its judgement updated prior to being killed?
There was a problem hiding this comment.
I'm also uncertain the usage of timeRange is safe here, in particular when the overlapping algorithm is active (e.g. taiko). I get the logic of "the time range is the visible range, so waiting one full time range after the end time should be safe", but that's not quite how it works using the overlapping algorithm, since an object's visible time on screen also depends on the MultiplierControlPoint in that case (see something like #21769, wherein a swell can be on screen for two thirds of the map).
Something more similar to how computeDisplayStartTime() does things may be warranted - or maybe this change can be shipped off to just catch locally or something.
There was a problem hiding this comment.
The logic I used to reason around that is that as long as the hitobject appears on the screen once, it will have an end time set by DrawableHitObject.updateState().
However, I now realise you're absolutely correct and that is a concern.
There was a problem hiding this comment.
I think at the very least this should take account of entry.HitObject.MaximumJudgementOffset like the previous line. The case here is an object scrolls past the hit point and time range but is still within the hit window.
I had that originally but couldn't find a scenario where it was visually required. Do you have an example I can work with?
I'm also uncertain the usage of timeRange is safe here, in particular when the overlapping algorithm is active (e.g. taiko).
I also had concerns, as touched on in the OP but I couldn't seem to break it even when I was intentionally trying.
There was a problem hiding this comment.
I also had concerns, as touched on in the OP but I couldn't seem to break it even when I was intentionally trying.
I wanted to see if I could produce an example for this but then couldn't, and started debugging, and found this.
This overwrites the lifetime from the entry that this PR is setting.
At this point, I have several questions.
- What is going on here?
- If the above is the case then why are we adding
timeRange.Valueat all? What's the significance of that extension at this point even? Could probably just do end time with no extension and call it a day? (which I realise is what @smoogipoo was getting at here and now am failing to understand what my initial point was?) - What the -------- is going on with all this????? How did we produce this?????????
There was a problem hiding this comment.
This overwrites the lifetime from the entry that this PR is setting.
The general process is that yes, the line you state overwrites the value, but is followed by a ruleset either adjusting the lifetime itself in the update state call:
osu/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNoteHead.cs
Lines 40 to 41 in 7bc8908
or subsequently automatically set based on transforms applied since the line you highlighted:
osu/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs
Lines 488 to 489 in 3b8b56c
The change I've made in this PR is for initial lifetime before a DHO is assigned to a HO model. In this state, it is used to decide whether to de-pool a DHO (and potentially incur overheads) at all.
If the above is the case then why are we adding timeRange.Value at all?
It needs to be lenient enough to ensure that if a hit object could be visible on screen, a DHO needs to be retrieved from a pool.
What the -------- is going on with all this????? How did we produce this?????????
There was a problem hiding this comment.
The change I've made in this PR is for initial lifetime before a DHO is assigned to a HO model. In this state, it is used to decide whether to de-pool a DHO (and potentially incur overheads) at all.
Thanks. As stupid as this entire thing is I believe this has helped me find an actual problem with this.
See patch below:
diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs
index e4d39bb6de..b891808ecb 100644
--- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs
+++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs
@@ -30,6 +30,7 @@
using osuTK;
using osuTK.Graphics;
using JetBrains.Annotations;
+using osu.Framework.Testing;
namespace osu.Game.Tests.Visual.Gameplay
{
@@ -106,6 +107,45 @@ public void TestLifetimeRecomputedWhenTimeRangeChanges(string pooled)
assertPosition(3, 1);
}
+ [Test]
+ public void TestLifetimeEnd()
+ {
+ var beatmap = new Beatmap<TestHitObject>
+ {
+ BeatmapInfo =
+ {
+ Difficulty = new BeatmapDifficulty
+ {
+ SliderMultiplier = 1
+ },
+ Ruleset = new OsuRuleset().RulesetInfo
+ }
+ };
+
+ for (int i = 0; i < 10; i++)
+ {
+ var h = new TestPooledHitObject();
+ h.StartTime = 100000 + i * time_range;
+ beatmap.HitObjects.Add(h);
+ }
+
+ beatmap.ControlPointInfo.Add(0, new TimingControlPoint { BeatLength = time_range / 2 });
+ beatmap.ControlPointInfo.Add(0, new EffectControlPoint { ScrollSpeed = 0.1 });
+
+ int counter = 0;
+ createTest(beatmap, d => d.RelativeScaleBeatLengthsOverride = true);
+ AddStep("hook into new result", () =>
+ {
+ var ruleset = this.ChildrenOfType<TestDrawableScrollingRuleset>().Single();
+ counter = 0;
+ ruleset.NewResult += _ => counter++;
+ });
+
+ setTime(95000);
+ setTime(999999);
+ AddAssert("all judged", () => counter, () => Is.EqualTo(10));
+ }
+
[Test]
public void TestRelativeBeatLengthScaleSingleTimingPoint()
{
@@ -463,6 +503,12 @@ public DrawableTestHitObject([CanBeNull] TestHitObject hitObject)
});
}
+ protected override void CheckForResult(bool userTriggered, double timeOffset)
+ {
+ if (timeOffset >= 0)
+ ApplyMaxResult();
+ }
+
protected override void Update() => LifetimeEnd = HitObject.EndTime;
}
This test passes on master and fails on this branch. It's related to the concern @smoogipoo brought up before:
The further concern I have, and that I would like a test for, is what happens if a hitobject scrolls from
t < LifetimeStarttot > LifetimeEndin one frame - does it get its judgement updated prior to being killed?
and it appears that the answer to that is "no it doesn't" with this change. This is significant as it could lead to gameplay not completing in rare cases (due to not all objects getting judged)
There was a problem hiding this comment.
Outside of tests, I can't seem to be able to make an example of this in gameplay. My initial concern is something like this:
diff --git a/osu.Game/Screens/Play/Player.cs b/osu.Game/Screens/Play/Player.cs
index 42ff1d74f3..81ff0c16bb 100644
--- a/osu.Game/Screens/Play/Player.cs
+++ b/osu.Game/Screens/Play/Player.cs
@@ -40,6 +40,7 @@
using osu.Game.Users;
using osu.Game.Utils;
using osuTK.Graphics;
+using osuTK.Input;
namespace osu.Game.Screens.Play
{
@@ -200,6 +201,13 @@ protected virtual void PrepareReplay()
DrawableRuleset.SetRecordTarget(Score);
}
+ protected override bool OnKeyDown(KeyDownEvent e)
+ {
+ if (e.Key == Key.Space)
+ Thread.Sleep(5000);
+ return true;
+ }
+
[BackgroundDependencyLoader(true)]
private void load(OsuConfigManager config, OsuGameBase game, CancellationToken cancellationToken)
{
diff --git a/osu.Game/Screens/Play/ReplayPlayer.cs b/osu.Game/Screens/Play/ReplayPlayer.cs
index ff60dbc0d0..d00027899f 100644
--- a/osu.Game/Screens/Play/ReplayPlayer.cs
+++ b/osu.Game/Screens/Play/ReplayPlayer.cs
@@ -6,6 +6,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
+using System.Threading;
using System.Threading.Tasks;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
@@ -115,11 +116,7 @@ public bool OnPressed(KeyBindingPressEvent<GlobalAction> e)
return true;
case GlobalAction.TogglePauseReplay:
- if (GameplayClockContainer.IsPaused.Value)
- GameplayClockContainer.Start();
- else
- GameplayClockContainer.Stop();
- return true;
+ return false;
}
return false;With the thinking being that this could arise naturally via e.g. a system-wide or GC event that causes a stutter at just an opportune moment so that the hitobject never gets updated. But this doesn't seem to actually trigger an issue.
Also, in replays I think FrameStabilityContainer will ensure that we have at most 16ms elapsed between frames, and TimeRange is unlikely to ever go below 16ms in any case.
And the editor is whatever - we don't really care about judgements there.
There was a problem hiding this comment.
Also, in replays I think FrameStabilityContainer will ensure that we have at most 16ms elapsed between frames, and TimeRange is unlikely to ever go below 16ms in any case.
Yeah, I went pretty deep in LifetimeManager logic and it's definitely not handling the scenario you brought up correctly, but frame stability is a saving grace.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
might regret this merge later but i haven't seen any actual non-test issue yet and am getting somewhat frustrated at how slow it is to get anything done lately so yolo merging
worst case scenario we pull the next build, if this fails it should do so pretty evidently ¯\_(ツ)_/¯
RFC.
Seems to work, I can't break it even though I feel like I should via some kind of editing operation of time range adjustment + missing refresh call of the method.
Closes #20221