From df3844cdbb833efc609355f1104c5983b1d00d7b Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 12:50:37 +0900 Subject: [PATCH 01/27] Add failing tests for pooling scrolling playfield --- .../TestSceneDrawableScrollingRuleset.cs | 182 ++++++++++++++++-- 1 file changed, 165 insertions(+), 17 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index 1a1babb4a8..ff3d152090 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -21,13 +21,13 @@ using osu.Game.Rulesets.Difficulty; using osu.Game.Rulesets.Mods; using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Objects.Drawables; -using osu.Game.Rulesets.Objects.Legacy; using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Osu; using osu.Game.Rulesets.UI; using osu.Game.Rulesets.UI.Scrolling; using osuTK; using osuTK.Graphics; +using JetBrains.Annotations; namespace osu.Game.Tests.Visual.Gameplay { @@ -46,6 +46,65 @@ namespace osu.Game.Tests.Visual.Gameplay [SetUp] public void Setup() => Schedule(() => testClock.CurrentTime = 0); + [Test] + public void TestHitObjectPooling() + { + var beatmap = createBeatmap(_ => new TestPooledHitObject()); + beatmap.ControlPointInfo.Add(0, new TimingControlPoint { BeatLength = time_range }); + createTest(beatmap); + + assertPosition(0, 0f); + assertDead(3); + + setTime(3 * time_range); + assertPosition(3, 0f); + assertDead(0); + + setTime(0 * time_range); + assertPosition(0, 0f); + assertDead(3); + } + + [TestCase("pooled")] + [TestCase("non-pooled")] + public void TestNestedHitObject(string pooled) + { + var beatmap = createBeatmap(i => + { + var h = pooled == "pooled" ? new TestPooledParentHitObject() : new TestParentHitObject(); + h.Duration = 300; + h.ChildTimeOffset = i % 3 * 100; + return h; + }); + beatmap.ControlPointInfo.Add(0, new TimingControlPoint { BeatLength = time_range }); + createTest(beatmap); + + assertPosition(0, 0f); + assertHeight(0); + assertChildPosition(0); + + setTime(5 * time_range); + assertPosition(5, 0f); + assertHeight(5); + assertChildPosition(5); + } + + private void assertDead(int index) => AddAssert($"hitobject {index} is dead", () => getDrawableHitObject(index) == null); + + private void assertHeight(int index) => AddAssert($"hitobject {index} height", () => + { + var d = getDrawableHitObject(index); + return d != null && Precision.AlmostEquals(d.DrawHeight, yScale * (float)(d.HitObject.Duration / time_range), 0.1f); + }); + + private void assertChildPosition(int index) => AddAssert($"hitobject {index} child position", () => + { + var d = getDrawableHitObject(index); + return d is DrawableTestParentHitObject && Precision.AlmostEquals( + d.NestedHitObjects.First().DrawPosition.Y, + yScale * (float)((TestParentHitObject)d.HitObject).ChildTimeOffset / time_range, 0.1f); + }); + [Test] public void TestRelativeBeatLengthScaleSingleTimingPoint() { @@ -147,8 +206,21 @@ namespace osu.Game.Tests.Visual.Gameplay assertPosition(1, 1); } + /// + /// Get a corresponding to the 'th . + /// When a pooling is used and the hit object is not alive, `null` is returned. + /// + [CanBeNull] + private DrawableTestHitObject getDrawableHitObject(int index) + { + var hitObject = drawableRuleset.Beatmap.HitObjects.ElementAt(index); + return (DrawableTestHitObject)drawableRuleset.Playfield.HitObjectContainer.Objects.FirstOrDefault(obj => obj.HitObject == hitObject); + } + + private float yScale => drawableRuleset.Playfield.HitObjectContainer.DrawHeight; + private void assertPosition(int index, float relativeY) => AddAssert($"hitobject {index} at {relativeY}", - () => Precision.AlmostEquals(drawableRuleset.Playfield.AllHitObjects.ElementAt(index).DrawPosition.Y, drawableRuleset.Playfield.HitObjectContainer.DrawHeight * relativeY)); + () => Precision.AlmostEquals(getDrawableHitObject(index)?.DrawPosition.Y ?? -1, yScale * relativeY)); private void setTime(double time) { @@ -160,12 +232,16 @@ namespace osu.Game.Tests.Visual.Gameplay /// The hitobjects are spaced milliseconds apart. /// /// The . - private IBeatmap createBeatmap() + private IBeatmap createBeatmap(Func createAction = null) { - var beatmap = new Beatmap { BeatmapInfo = { Ruleset = new OsuRuleset().RulesetInfo } }; + var beatmap = new Beatmap { BeatmapInfo = { Ruleset = new OsuRuleset().RulesetInfo } }; for (int i = 0; i < 10; i++) - beatmap.HitObjects.Add(new HitObject { StartTime = i * time_range }); + { + var h = createAction?.Invoke(i) ?? new TestHitObject(); + h.StartTime = i * time_range; + beatmap.HitObjects.Add(h); + } return beatmap; } @@ -225,7 +301,14 @@ namespace osu.Game.Tests.Visual.Gameplay TimeRange.Value = time_range; } - public override DrawableHitObject CreateDrawableRepresentation(TestHitObject h) => new DrawableTestHitObject(h); + public override DrawableHitObject CreateDrawableRepresentation(TestHitObject h) => + h switch + { + TestPooledHitObject _ => null, + TestPooledParentHitObject _ => null, + TestParentHitObject p => new DrawableTestParentHitObject(p), + _ => new DrawableTestHitObject(h), + }; protected override PassThroughInputManager CreateInputManager() => new PassThroughInputManager(); @@ -265,6 +348,9 @@ namespace osu.Game.Tests.Visual.Gameplay } } }); + + RegisterPool(1); + RegisterPool(1); } } @@ -277,30 +363,46 @@ namespace osu.Game.Tests.Visual.Gameplay public override bool CanConvert() => true; - protected override IEnumerable ConvertHitObject(HitObject original, IBeatmap beatmap, CancellationToken cancellationToken) - { - yield return new TestHitObject - { - StartTime = original.StartTime, - Duration = (original as IHasDuration)?.Duration ?? 100 - }; - } + protected override IEnumerable ConvertHitObject(HitObject original, IBeatmap beatmap, CancellationToken cancellationToken) => + throw new NotImplementedException(); } #endregion #region HitObject - private class TestHitObject : ConvertHitObject, IHasDuration + private class TestHitObject : HitObject, IHasDuration { public double EndTime => StartTime + Duration; - public double Duration { get; set; } + public double Duration { get; set; } = 100; + } + + private class TestPooledHitObject : TestHitObject + { + } + + private class TestParentHitObject : TestHitObject + { + public double ChildTimeOffset; + + protected override void CreateNestedHitObjects(CancellationToken cancellationToken) + { + AddNested(new TestHitObject { StartTime = StartTime + ChildTimeOffset }); + } + } + + private class TestPooledParentHitObject : TestParentHitObject + { + protected override void CreateNestedHitObjects(CancellationToken cancellationToken) + { + AddNested(new TestPooledHitObject { StartTime = StartTime + ChildTimeOffset }); + } } private class DrawableTestHitObject : DrawableHitObject { - public DrawableTestHitObject(TestHitObject hitObject) + public DrawableTestHitObject([CanBeNull] TestHitObject hitObject) : base(hitObject) { Anchor = Anchor.TopCentre; @@ -326,6 +428,52 @@ namespace osu.Game.Tests.Visual.Gameplay } } + private class DrawableTestPooledHitObject : DrawableTestHitObject + { + public DrawableTestPooledHitObject() + : base(null) + { + InternalChildren[0].Colour = Color4.LightSkyBlue; + InternalChildren[1].Colour = Color4.Blue; + } + + protected override void Update() => LifetimeEnd = HitObject.EndTime; + } + + private class DrawableTestParentHitObject : DrawableTestHitObject + { + private readonly Container container; + + public DrawableTestParentHitObject([CanBeNull] TestHitObject hitObject) + : base(hitObject) + { + InternalChildren[0].Colour = Color4.LightYellow; + InternalChildren[1].Colour = Color4.Yellow; + + AddInternal(container = new Container + { + RelativeSizeAxes = Axes.Both, + }); + } + + protected override DrawableHitObject CreateNestedHitObject(HitObject hitObject) => + new DrawableTestHitObject((TestHitObject)hitObject); + + protected override void AddNestedHitObject(DrawableHitObject hitObject) => container.Add(hitObject); + + protected override void ClearNestedHitObjects() => container.Clear(false); + } + + private class DrawableTestPooledParentHitObject : DrawableTestParentHitObject + { + public DrawableTestPooledParentHitObject() + : base(null) + { + InternalChildren[0].Colour = Color4.LightSeaGreen; + InternalChildren[1].Colour = Color4.Green; + } + } + #endregion } } From 5c743adbae77f5447d35be352aa1c87834f18f93 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 12:57:39 +0900 Subject: [PATCH 02/27] Support hit object pooling scrolling playfield --- .../Scrolling/ScrollingHitObjectContainer.cs | 40 ++++--------------- .../UI/Scrolling/ScrollingPlayfield.cs | 6 +++ 2 files changed, 14 insertions(+), 32 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index bf64175468..802204900b 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -48,33 +48,8 @@ namespace osu.Game.Rulesets.UI.Scrolling timeRange.ValueChanged += _ => layoutCache.Invalidate(); } - public override void Add(DrawableHitObject hitObject) - { - combinedObjCache.Invalidate(); - hitObject.DefaultsApplied += onDefaultsApplied; - base.Add(hitObject); - } - - public override bool Remove(DrawableHitObject hitObject) - { - var result = base.Remove(hitObject); - - if (result) - { - combinedObjCache.Invalidate(); - hitObjectInitialStateCache.Remove(hitObject); - - hitObject.DefaultsApplied -= onDefaultsApplied; - } - - return result; - } - public override void Clear(bool disposeChildren = true) { - foreach (var h in Objects) - h.DefaultsApplied -= onDefaultsApplied; - base.Clear(disposeChildren); combinedObjCache.Invalidate(); @@ -173,18 +148,19 @@ namespace osu.Game.Rulesets.UI.Scrolling } } - private void onDefaultsApplied(DrawableHitObject drawableObject) + /// + /// Invalidate the cache of the layout of this hit object. + /// + public void InvalidateDrawableHitObject(DrawableHitObject drawableObject) { - // The cache may not exist if the hitobject state hasn't been computed yet (e.g. if the hitobject was added + defaults applied in the same frame). - // In such a case, combinedObjCache will take care of updating the hitobject. if (hitObjectInitialStateCache.TryGetValue(drawableObject, out var state)) - { - combinedObjCache.Invalidate(); state.Cache.Invalidate(); - } + + combinedObjCache.Invalidate(); } - private float scrollLength; + // Use a nonzero value to prevent infinite results + private float scrollLength = 1; protected override void Update() { diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs index 9dac3f4de1..9b21a3f0a9 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs @@ -24,6 +24,12 @@ namespace osu.Game.Rulesets.UI.Scrolling Direction.BindTo(ScrollingInfo.Direction); } + protected override void OnNewDrawableHitObject(DrawableHitObject drawableHitObject) + { + drawableHitObject.HitObjectApplied += + ((ScrollingHitObjectContainer)HitObjectContainer).InvalidateDrawableHitObject; + } + /// /// Given a position in screen space, return the time within this column. /// From 8f39b54e58dbd673a124875da57935acd6a7a7c0 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 13:34:09 +0900 Subject: [PATCH 03/27] Simplify ScrollingHitObjectContainer logic --- .../TestSceneDrawableScrollingRuleset.cs | 2 +- .../Scrolling/ScrollingHitObjectContainer.cs | 76 +++++-------------- 2 files changed, 22 insertions(+), 56 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index ff3d152090..cebe0394c7 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -445,7 +445,7 @@ namespace osu.Game.Tests.Visual.Gameplay private readonly Container container; public DrawableTestParentHitObject([CanBeNull] TestHitObject hitObject) - : base(hitObject) + : base(hitObject) { InternalChildren[0].Colour = Color4.LightYellow; InternalChildren[1].Colour = Color4.Yellow; diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 802204900b..11a7665f0f 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -2,13 +2,10 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; -using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; -using osu.Framework.Caching; using osu.Framework.Graphics; using osu.Framework.Layout; -using osu.Framework.Threading; using osu.Game.Rulesets.Objects.Drawables; using osu.Game.Rulesets.Objects.Types; using osuTK; @@ -19,7 +16,6 @@ namespace osu.Game.Rulesets.UI.Scrolling { private readonly IBindable timeRange = new BindableDouble(); private readonly IBindable direction = new Bindable(); - private readonly Dictionary hitObjectInitialStateCache = new Dictionary(); [Resolved] private IScrollingInfo scrollingInfo { get; set; } @@ -27,9 +23,7 @@ namespace osu.Game.Rulesets.UI.Scrolling // Responds to changes in the layout. When the layout changes, all hit object states must be recomputed. private readonly LayoutValue layoutCache = new LayoutValue(Invalidation.RequiredParentSizeToFit | Invalidation.DrawInfo); - // A combined cache across all hit object states to reduce per-update iterations. - // When invalidated, one or more (but not necessarily all) hitobject states must be re-validated. - private readonly Cached combinedObjCache = new Cached(); + private readonly HashSet invalidHitObjects = new HashSet(); public ScrollingHitObjectContainer() { @@ -52,8 +46,7 @@ namespace osu.Game.Rulesets.UI.Scrolling { base.Clear(disposeChildren); - combinedObjCache.Invalidate(); - hitObjectInitialStateCache.Clear(); + invalidHitObjects.Clear(); } /// @@ -150,17 +143,18 @@ namespace osu.Game.Rulesets.UI.Scrolling /// /// Invalidate the cache of the layout of this hit object. + /// A hit object should be invalidated after all its nested hit objects are invalidated. /// public void InvalidateDrawableHitObject(DrawableHitObject drawableObject) { - if (hitObjectInitialStateCache.TryGetValue(drawableObject, out var state)) - state.Cache.Invalidate(); + invalidHitObjects.Add(drawableObject); - combinedObjCache.Invalidate(); + // Remove children as nested hit objects will be recursively updated. + foreach (var nested in drawableObject.NestedHitObjects) + invalidHitObjects.Remove(nested); } - // Use a nonzero value to prevent infinite results - private float scrollLength = 1; + private float scrollLength; protected override void Update() { @@ -168,17 +162,11 @@ namespace osu.Game.Rulesets.UI.Scrolling if (!layoutCache.IsValid) { - foreach (var state in hitObjectInitialStateCache.Values) - state.Cache.Invalidate(); - combinedObjCache.Invalidate(); + foreach (var obj in Objects) + invalidHitObjects.Add(obj); scrollingInfo.Algorithm.Reset(); - layoutCache.Validate(); - } - - if (!combinedObjCache.IsValid) - { switch (direction.Value) { case ScrollingDirection.Up: @@ -191,24 +179,16 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } - foreach (var obj in Objects) - { - if (!hitObjectInitialStateCache.TryGetValue(obj, out var state)) - state = hitObjectInitialStateCache[obj] = new InitialState(new Cached()); - - if (state.Cache.IsValid) - continue; - - state.ScheduledComputation?.Cancel(); - state.ScheduledComputation = computeInitialStateRecursive(obj); - - computeLifetimeStartRecursive(obj); - - state.Cache.Validate(); - } - - combinedObjCache.Validate(); + layoutCache.Validate(); } + + foreach (var obj in invalidHitObjects) + { + computeInitialStateRecursive(obj); + computeLifetimeStartRecursive(obj); + } + + invalidHitObjects.Clear(); } private void computeLifetimeStartRecursive(DrawableHitObject hitObject) @@ -247,7 +227,7 @@ namespace osu.Game.Rulesets.UI.Scrolling return scrollingInfo.Algorithm.GetDisplayStartTime(hitObject.HitObject.StartTime, originAdjustment, timeRange.Value, scrollLength); } - private ScheduledDelegate computeInitialStateRecursive(DrawableHitObject hitObject) => hitObject.Schedule(() => + private void computeInitialStateRecursive(DrawableHitObject hitObject) { if (hitObject.HitObject is IHasDuration e) { @@ -272,7 +252,7 @@ namespace osu.Game.Rulesets.UI.Scrolling // Nested hitobjects don't need to scroll, but they do need accurate positions updatePosition(obj, hitObject.HitObject.StartTime); } - }); + } protected override void UpdateAfterChildrenLife() { @@ -304,19 +284,5 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } } - - private class InitialState - { - [NotNull] - public readonly Cached Cache; - - [CanBeNull] - public ScheduledDelegate ScheduledComputation; - - public InitialState(Cached cache) - { - Cache = cache; - } - } } } From cabc8aa63b4b3e3cb7f59046054f8b87071b651a Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 13:57:20 +0900 Subject: [PATCH 04/27] Revert "Simplify ScrollingHitObjectContainer logic" This reverts commit b4cc39149c117e6a0e95ee917a67cec8ba723d06. --- .../TestSceneDrawableScrollingRuleset.cs | 2 +- .../Scrolling/ScrollingHitObjectContainer.cs | 74 ++++++++++++++----- 2 files changed, 55 insertions(+), 21 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index cebe0394c7..ff3d152090 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -445,7 +445,7 @@ namespace osu.Game.Tests.Visual.Gameplay private readonly Container container; public DrawableTestParentHitObject([CanBeNull] TestHitObject hitObject) - : base(hitObject) + : base(hitObject) { InternalChildren[0].Colour = Color4.LightYellow; InternalChildren[1].Colour = Color4.Yellow; diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 11a7665f0f..802204900b 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -2,10 +2,13 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; +using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; +using osu.Framework.Caching; using osu.Framework.Graphics; using osu.Framework.Layout; +using osu.Framework.Threading; using osu.Game.Rulesets.Objects.Drawables; using osu.Game.Rulesets.Objects.Types; using osuTK; @@ -16,6 +19,7 @@ namespace osu.Game.Rulesets.UI.Scrolling { private readonly IBindable timeRange = new BindableDouble(); private readonly IBindable direction = new Bindable(); + private readonly Dictionary hitObjectInitialStateCache = new Dictionary(); [Resolved] private IScrollingInfo scrollingInfo { get; set; } @@ -23,7 +27,9 @@ namespace osu.Game.Rulesets.UI.Scrolling // Responds to changes in the layout. When the layout changes, all hit object states must be recomputed. private readonly LayoutValue layoutCache = new LayoutValue(Invalidation.RequiredParentSizeToFit | Invalidation.DrawInfo); - private readonly HashSet invalidHitObjects = new HashSet(); + // A combined cache across all hit object states to reduce per-update iterations. + // When invalidated, one or more (but not necessarily all) hitobject states must be re-validated. + private readonly Cached combinedObjCache = new Cached(); public ScrollingHitObjectContainer() { @@ -46,7 +52,8 @@ namespace osu.Game.Rulesets.UI.Scrolling { base.Clear(disposeChildren); - invalidHitObjects.Clear(); + combinedObjCache.Invalidate(); + hitObjectInitialStateCache.Clear(); } /// @@ -143,18 +150,17 @@ namespace osu.Game.Rulesets.UI.Scrolling /// /// Invalidate the cache of the layout of this hit object. - /// A hit object should be invalidated after all its nested hit objects are invalidated. /// public void InvalidateDrawableHitObject(DrawableHitObject drawableObject) { - invalidHitObjects.Add(drawableObject); + if (hitObjectInitialStateCache.TryGetValue(drawableObject, out var state)) + state.Cache.Invalidate(); - // Remove children as nested hit objects will be recursively updated. - foreach (var nested in drawableObject.NestedHitObjects) - invalidHitObjects.Remove(nested); + combinedObjCache.Invalidate(); } - private float scrollLength; + // Use a nonzero value to prevent infinite results + private float scrollLength = 1; protected override void Update() { @@ -162,11 +168,17 @@ namespace osu.Game.Rulesets.UI.Scrolling if (!layoutCache.IsValid) { - foreach (var obj in Objects) - invalidHitObjects.Add(obj); + foreach (var state in hitObjectInitialStateCache.Values) + state.Cache.Invalidate(); + combinedObjCache.Invalidate(); scrollingInfo.Algorithm.Reset(); + layoutCache.Validate(); + } + + if (!combinedObjCache.IsValid) + { switch (direction.Value) { case ScrollingDirection.Up: @@ -179,16 +191,24 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } - layoutCache.Validate(); - } + foreach (var obj in Objects) + { + if (!hitObjectInitialStateCache.TryGetValue(obj, out var state)) + state = hitObjectInitialStateCache[obj] = new InitialState(new Cached()); - foreach (var obj in invalidHitObjects) - { - computeInitialStateRecursive(obj); - computeLifetimeStartRecursive(obj); - } + if (state.Cache.IsValid) + continue; - invalidHitObjects.Clear(); + state.ScheduledComputation?.Cancel(); + state.ScheduledComputation = computeInitialStateRecursive(obj); + + computeLifetimeStartRecursive(obj); + + state.Cache.Validate(); + } + + combinedObjCache.Validate(); + } } private void computeLifetimeStartRecursive(DrawableHitObject hitObject) @@ -227,7 +247,7 @@ namespace osu.Game.Rulesets.UI.Scrolling return scrollingInfo.Algorithm.GetDisplayStartTime(hitObject.HitObject.StartTime, originAdjustment, timeRange.Value, scrollLength); } - private void computeInitialStateRecursive(DrawableHitObject hitObject) + private ScheduledDelegate computeInitialStateRecursive(DrawableHitObject hitObject) => hitObject.Schedule(() => { if (hitObject.HitObject is IHasDuration e) { @@ -252,7 +272,7 @@ namespace osu.Game.Rulesets.UI.Scrolling // Nested hitobjects don't need to scroll, but they do need accurate positions updatePosition(obj, hitObject.HitObject.StartTime); } - } + }); protected override void UpdateAfterChildrenLife() { @@ -284,5 +304,19 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } } + + private class InitialState + { + [NotNull] + public readonly Cached Cache; + + [CanBeNull] + public ScheduledDelegate ScheduledComputation; + + public InitialState(Cached cache) + { + Cache = cache; + } + } } } From ec92545d7a4a2270d90682a3108c1936164d8e45 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 14:13:57 +0900 Subject: [PATCH 05/27] fix indent --- .../Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index ff3d152090..cebe0394c7 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -445,7 +445,7 @@ namespace osu.Game.Tests.Visual.Gameplay private readonly Container container; public DrawableTestParentHitObject([CanBeNull] TestHitObject hitObject) - : base(hitObject) + : base(hitObject) { InternalChildren[0].Colour = Color4.LightYellow; InternalChildren[1].Colour = Color4.Yellow; From ce57e8ddfb883e16dc99bfea54145c48b22a71a7 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 16:06:01 +0900 Subject: [PATCH 06/27] Separate Lifetime computation and layout update --- .../Scrolling/ScrollingHitObjectContainer.cs | 92 ++++++------------- 1 file changed, 30 insertions(+), 62 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 802204900b..1845094c44 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -2,13 +2,10 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; -using JetBrains.Annotations; using osu.Framework.Allocation; using osu.Framework.Bindables; -using osu.Framework.Caching; using osu.Framework.Graphics; using osu.Framework.Layout; -using osu.Framework.Threading; using osu.Game.Rulesets.Objects.Drawables; using osu.Game.Rulesets.Objects.Types; using osuTK; @@ -19,7 +16,12 @@ namespace osu.Game.Rulesets.UI.Scrolling { private readonly IBindable timeRange = new BindableDouble(); private readonly IBindable direction = new Bindable(); - private readonly Dictionary hitObjectInitialStateCache = new Dictionary(); + + // If a hit object is not in this set, the position and the size should be updated when the hit object becomes alive. + private readonly HashSet layoutComputedHitObjects = new HashSet(); + + // Used to recompute all lifetime when `layoutCache` becomes invalid + private readonly HashSet lifetimeComputedHitObjects = new HashSet(); [Resolved] private IScrollingInfo scrollingInfo { get; set; } @@ -27,10 +29,6 @@ namespace osu.Game.Rulesets.UI.Scrolling // Responds to changes in the layout. When the layout changes, all hit object states must be recomputed. private readonly LayoutValue layoutCache = new LayoutValue(Invalidation.RequiredParentSizeToFit | Invalidation.DrawInfo); - // A combined cache across all hit object states to reduce per-update iterations. - // When invalidated, one or more (but not necessarily all) hitobject states must be re-validated. - private readonly Cached combinedObjCache = new Cached(); - public ScrollingHitObjectContainer() { RelativeSizeAxes = Axes.Both; @@ -52,8 +50,7 @@ namespace osu.Game.Rulesets.UI.Scrolling { base.Clear(disposeChildren); - combinedObjCache.Invalidate(); - hitObjectInitialStateCache.Clear(); + layoutComputedHitObjects.Clear(); } /// @@ -150,13 +147,15 @@ namespace osu.Game.Rulesets.UI.Scrolling /// /// Invalidate the cache of the layout of this hit object. + /// A hit object should be invalidated after all its nested hit objects are invalidated. /// - public void InvalidateDrawableHitObject(DrawableHitObject drawableObject) + public void InvalidateDrawableHitObject(DrawableHitObject hitObject) { - if (hitObjectInitialStateCache.TryGetValue(drawableObject, out var state)) - state.Cache.Invalidate(); + // lifetime is computed before update + hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); - combinedObjCache.Invalidate(); + lifetimeComputedHitObjects.Add(hitObject); + layoutComputedHitObjects.Remove(hitObject); } // Use a nonzero value to prevent infinite results @@ -168,17 +167,14 @@ namespace osu.Game.Rulesets.UI.Scrolling if (!layoutCache.IsValid) { - foreach (var state in hitObjectInitialStateCache.Values) - state.Cache.Invalidate(); - combinedObjCache.Invalidate(); + // this.Objects cannot be used as it doesn't contain nested objects + foreach (var hitObject in lifetimeComputedHitObjects) + hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); + + layoutComputedHitObjects.Clear(); scrollingInfo.Algorithm.Reset(); - layoutCache.Validate(); - } - - if (!combinedObjCache.IsValid) - { switch (direction.Value) { case ScrollingDirection.Up: @@ -191,32 +187,18 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } - foreach (var obj in Objects) - { - if (!hitObjectInitialStateCache.TryGetValue(obj, out var state)) - state = hitObjectInitialStateCache[obj] = new InitialState(new Cached()); - - if (state.Cache.IsValid) - continue; - - state.ScheduledComputation?.Cancel(); - state.ScheduledComputation = computeInitialStateRecursive(obj); - - computeLifetimeStartRecursive(obj); - - state.Cache.Validate(); - } - - combinedObjCache.Validate(); + layoutCache.Validate(); } - } - private void computeLifetimeStartRecursive(DrawableHitObject hitObject) - { - hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); + foreach (var obj in AliveObjects) + { + if (layoutComputedHitObjects.Contains(obj)) + continue; - foreach (var obj in hitObject.NestedHitObjects) - computeLifetimeStartRecursive(obj); + updateLayoutRecursive(obj); + + layoutComputedHitObjects.Add(obj); + } } private double computeOriginAdjustedLifetimeStart(DrawableHitObject hitObject) @@ -247,7 +229,7 @@ namespace osu.Game.Rulesets.UI.Scrolling return scrollingInfo.Algorithm.GetDisplayStartTime(hitObject.HitObject.StartTime, originAdjustment, timeRange.Value, scrollLength); } - private ScheduledDelegate computeInitialStateRecursive(DrawableHitObject hitObject) => hitObject.Schedule(() => + private void updateLayoutRecursive(DrawableHitObject hitObject) { if (hitObject.HitObject is IHasDuration e) { @@ -267,12 +249,12 @@ namespace osu.Game.Rulesets.UI.Scrolling foreach (var obj in hitObject.NestedHitObjects) { - computeInitialStateRecursive(obj); + updateLayoutRecursive(obj); // Nested hitobjects don't need to scroll, but they do need accurate positions updatePosition(obj, hitObject.HitObject.StartTime); } - }); + } protected override void UpdateAfterChildrenLife() { @@ -304,19 +286,5 @@ namespace osu.Game.Rulesets.UI.Scrolling break; } } - - private class InitialState - { - [NotNull] - public readonly Cached Cache; - - [CanBeNull] - public ScheduledDelegate ScheduledComputation; - - public InitialState(Cached cache) - { - Cache = cache; - } - } } } From d5f082e5fb2cfa609d4e9f384d29812229c0914b Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 16:25:22 +0900 Subject: [PATCH 07/27] Comment about lifetime assumption --- .../Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 1845094c44..e750727764 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -147,11 +147,13 @@ namespace osu.Game.Rulesets.UI.Scrolling /// /// Invalidate the cache of the layout of this hit object. - /// A hit object should be invalidated after all its nested hit objects are invalidated. /// public void InvalidateDrawableHitObject(DrawableHitObject hitObject) { - // lifetime is computed before update + // Lifetime is computed once early and + // layout (Width/Height if `IHasDuration`, and nested object positions) will be computed when the object becomes alive. + // An assumption is that a hit object layout update (setting `Height` or `Width`) won't affect its lifetime. + // This is satisfied in practice because otherwise the hit object won't be aligned to its `StartTime`. hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); lifetimeComputedHitObjects.Add(hitObject); From 7f6e4d5b217ae563b64893f9e194b7e5c2733d5e Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 16:46:10 +0900 Subject: [PATCH 08/27] Delay lifetime computation until loaded --- .../Scrolling/ScrollingHitObjectContainer.cs | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index e750727764..150ed16bab 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -21,7 +21,7 @@ namespace osu.Game.Rulesets.UI.Scrolling private readonly HashSet layoutComputedHitObjects = new HashSet(); // Used to recompute all lifetime when `layoutCache` becomes invalid - private readonly HashSet lifetimeComputedHitObjects = new HashSet(); + private readonly HashSet allHitObjects = new HashSet(); [Resolved] private IScrollingInfo scrollingInfo { get; set; } @@ -150,18 +150,19 @@ namespace osu.Game.Rulesets.UI.Scrolling /// public void InvalidateDrawableHitObject(DrawableHitObject hitObject) { - // Lifetime is computed once early and - // layout (Width/Height if `IHasDuration`, and nested object positions) will be computed when the object becomes alive. - // An assumption is that a hit object layout update (setting `Height` or `Width`) won't affect its lifetime. - // This is satisfied in practice because otherwise the hit object won't be aligned to its `StartTime`. - hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); + // Lifetime computation is delayed to the next update because `scrollLength` may not be valid here. + // Layout computation will be delayed to when the object becomes alive. + // An assumption is that a hit object layout update (setting `Height` or `Width`) won't affect its lifetime but + // this is satisfied in practice because otherwise the hit object won't be aligned to its `StartTime`. + + layoutCache.Invalidate(); + + allHitObjects.Add(hitObject); - lifetimeComputedHitObjects.Add(hitObject); layoutComputedHitObjects.Remove(hitObject); } - // Use a nonzero value to prevent infinite results - private float scrollLength = 1; + private float scrollLength; protected override void Update() { @@ -170,7 +171,7 @@ namespace osu.Game.Rulesets.UI.Scrolling if (!layoutCache.IsValid) { // this.Objects cannot be used as it doesn't contain nested objects - foreach (var hitObject in lifetimeComputedHitObjects) + foreach (var hitObject in allHitObjects) hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); layoutComputedHitObjects.Clear(); From e34a2051044d2901df39328051a4dbdadd094802 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 18:52:15 +0900 Subject: [PATCH 09/27] Rewrite hit object management, take three --- osu.Game/Rulesets/UI/HitObjectContainer.cs | 8 +-- osu.Game/Rulesets/UI/Playfield.cs | 4 +- .../Scrolling/ScrollingHitObjectContainer.cs | 63 ++++++++++++------- .../UI/Scrolling/ScrollingPlayfield.cs | 6 -- 4 files changed, 45 insertions(+), 36 deletions(-) diff --git a/osu.Game/Rulesets/UI/HitObjectContainer.cs b/osu.Game/Rulesets/UI/HitObjectContainer.cs index 5fbda305c8..b10a6efaf2 100644 --- a/osu.Game/Rulesets/UI/HitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/HitObjectContainer.cs @@ -48,7 +48,7 @@ namespace osu.Game.Rulesets.UI /// /// If this uses pooled objects, this represents the time when the s become alive. /// - internal event Action HitObjectUsageBegan; + internal event Action HitObjectUsageBegan; /// /// Invoked when a becomes unused by a . @@ -56,7 +56,7 @@ namespace osu.Game.Rulesets.UI /// /// If this uses pooled objects, this represents the time when the s become dead. /// - internal event Action HitObjectUsageFinished; + internal event Action HitObjectUsageFinished; /// /// The amount of time prior to the current time within which s should be considered alive. @@ -115,7 +115,7 @@ namespace osu.Game.Rulesets.UI bindStartTime(drawable); AddInternal(drawableMap[entry] = drawable, false); - HitObjectUsageBegan?.Invoke(entry.HitObject); + HitObjectUsageBegan?.Invoke(drawable); } private void removeDrawable(HitObjectLifetimeEntry entry) @@ -132,7 +132,7 @@ namespace osu.Game.Rulesets.UI unbindStartTime(drawable); RemoveInternal(drawable); - HitObjectUsageFinished?.Invoke(entry.HitObject); + HitObjectUsageFinished?.Invoke(drawable); } #endregion diff --git a/osu.Game/Rulesets/UI/Playfield.cs b/osu.Game/Rulesets/UI/Playfield.cs index 82ec653f31..e27ab7fda5 100644 --- a/osu.Game/Rulesets/UI/Playfield.cs +++ b/osu.Game/Rulesets/UI/Playfield.cs @@ -91,8 +91,8 @@ namespace osu.Game.Rulesets.UI { h.NewResult += (d, r) => NewResult?.Invoke(d, r); h.RevertResult += (d, r) => RevertResult?.Invoke(d, r); - h.HitObjectUsageBegan += o => HitObjectUsageBegan?.Invoke(o); - h.HitObjectUsageFinished += o => HitObjectUsageFinished?.Invoke(o); + h.HitObjectUsageBegan += o => HitObjectUsageBegan?.Invoke(o.HitObject); + h.HitObjectUsageFinished += o => HitObjectUsageFinished?.Invoke(o.HitObject); })); } diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 150ed16bab..c8afe76f19 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -17,11 +17,16 @@ namespace osu.Game.Rulesets.UI.Scrolling private readonly IBindable timeRange = new BindableDouble(); private readonly IBindable direction = new Bindable(); - // If a hit object is not in this set, the position and the size should be updated when the hit object becomes alive. - private readonly HashSet layoutComputedHitObjects = new HashSet(); + // Tracks all `DrawableHitObject` (nested or not) applied a `HitObject`. + // It dynamically changes based on approximate lifetime when a pooling is used. + private readonly HashSet hitObjectApplied = new HashSet(); - // Used to recompute all lifetime when `layoutCache` becomes invalid - private readonly HashSet allHitObjects = new HashSet(); + // The lifetime of a hit object in this will be computed in next update. + private readonly HashSet toComputeLifetime = new HashSet(); + + // The layout (length if IHasDuration, and nested object positions) of a hit object *not* in this set will be computed in next updated. + // Only objects in `AliveObjects` are considered, to prevent a massive recomputation when scrolling speed or something changes. + private readonly HashSet layoutComputed = new HashSet(); [Resolved] private IScrollingInfo scrollingInfo { get; set; } @@ -34,6 +39,9 @@ namespace osu.Game.Rulesets.UI.Scrolling RelativeSizeAxes = Axes.Both; AddLayout(layoutCache); + + HitObjectUsageBegan += onHitObjectUsageBegin; + HitObjectUsageFinished += onHitObjectUsageFinished; } [BackgroundDependencyLoader] @@ -50,7 +58,9 @@ namespace osu.Game.Rulesets.UI.Scrolling { base.Clear(disposeChildren); - layoutComputedHitObjects.Clear(); + hitObjectApplied.Clear(); + toComputeLifetime.Clear(); + layoutComputed.Clear(); } /// @@ -145,21 +155,20 @@ namespace osu.Game.Rulesets.UI.Scrolling } } - /// - /// Invalidate the cache of the layout of this hit object. - /// - public void InvalidateDrawableHitObject(DrawableHitObject hitObject) + private void onHitObjectUsageBegin(DrawableHitObject hitObject) { - // Lifetime computation is delayed to the next update because `scrollLength` may not be valid here. - // Layout computation will be delayed to when the object becomes alive. - // An assumption is that a hit object layout update (setting `Height` or `Width`) won't affect its lifetime but - // this is satisfied in practice because otherwise the hit object won't be aligned to its `StartTime`. + // Lifetime computation is delayed until next update because + // when the hit object is not pooled this container is not loaded here and `scrollLength` cannot be computed. + hitObjectApplied.Add(hitObject); + toComputeLifetime.Add(hitObject); + layoutComputed.Remove(hitObject); + } - layoutCache.Invalidate(); - - allHitObjects.Add(hitObject); - - layoutComputedHitObjects.Remove(hitObject); + private void onHitObjectUsageFinished(DrawableHitObject hitObject) + { + hitObjectApplied.Remove(hitObject); + toComputeLifetime.Remove(hitObject); + layoutComputed.Remove(hitObject); } private float scrollLength; @@ -170,11 +179,10 @@ namespace osu.Game.Rulesets.UI.Scrolling if (!layoutCache.IsValid) { - // this.Objects cannot be used as it doesn't contain nested objects - foreach (var hitObject in allHitObjects) - hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); + foreach (var hitObject in hitObjectApplied) + toComputeLifetime.Add(hitObject); - layoutComputedHitObjects.Clear(); + layoutComputed.Clear(); scrollingInfo.Algorithm.Reset(); @@ -193,14 +201,21 @@ namespace osu.Game.Rulesets.UI.Scrolling layoutCache.Validate(); } + foreach (var hitObject in toComputeLifetime) + hitObject.LifetimeStart = computeOriginAdjustedLifetimeStart(hitObject); + + toComputeLifetime.Clear(); + + // An assumption is that this update won't affect lifetime, + // but this is satisfied in practice because otherwise the hit object won't be aligned to its `StartTime`. foreach (var obj in AliveObjects) { - if (layoutComputedHitObjects.Contains(obj)) + if (layoutComputed.Contains(obj)) continue; updateLayoutRecursive(obj); - layoutComputedHitObjects.Add(obj); + layoutComputed.Add(obj); } } diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs index 9b21a3f0a9..9dac3f4de1 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs @@ -24,12 +24,6 @@ namespace osu.Game.Rulesets.UI.Scrolling Direction.BindTo(ScrollingInfo.Direction); } - protected override void OnNewDrawableHitObject(DrawableHitObject drawableHitObject) - { - drawableHitObject.HitObjectApplied += - ((ScrollingHitObjectContainer)HitObjectContainer).InvalidateDrawableHitObject; - } - /// /// Given a position in screen space, return the time within this column. /// From b8a5cd94f741f98453bdd8118e95dc40369012f4 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 24 Nov 2020 19:46:57 +0900 Subject: [PATCH 10/27] Invoke HitObjectUsageFinished before removal --- osu.Game/Rulesets/UI/HitObjectContainer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Rulesets/UI/HitObjectContainer.cs b/osu.Game/Rulesets/UI/HitObjectContainer.cs index b10a6efaf2..fb91108605 100644 --- a/osu.Game/Rulesets/UI/HitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/HitObjectContainer.cs @@ -129,10 +129,10 @@ namespace osu.Game.Rulesets.UI drawableMap.Remove(entry); + HitObjectUsageFinished?.Invoke(drawable); + unbindStartTime(drawable); RemoveInternal(drawable); - - HitObjectUsageFinished?.Invoke(drawable); } #endregion From 0414e5c5502effefb56416d58d26645683179805 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Wed, 25 Nov 2020 23:38:47 +0900 Subject: [PATCH 11/27] Add MaximumJudgementOffset to DrawableHitObject, use in more places --- .../Objects/Drawables/DrawableHoldNoteTail.cs | 2 ++ .../Objects/Drawables/DrawableOsuHitObject.cs | 8 ------- .../Objects/Drawables/DrawableSpinnerTick.cs | 12 ++++++++++ osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs | 2 ++ .../Objects/Drawables/DrawableDrumRollTick.cs | 2 ++ .../Objects/Drawables/DrawableHitObject.cs | 24 +++++++++++-------- 6 files changed, 32 insertions(+), 18 deletions(-) diff --git a/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNoteTail.cs b/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNoteTail.cs index a4029e7893..3a00933e4d 100644 --- a/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNoteTail.cs +++ b/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNoteTail.cs @@ -30,6 +30,8 @@ namespace osu.Game.Rulesets.Mania.Objects.Drawables public void UpdateResult() => base.UpdateResult(true); + protected override double MaximumJudgementOffset => base.MaximumJudgementOffset * release_window_lenience; + protected override void CheckForResult(bool userTriggered, double timeOffset) { Debug.Assert(HitObject.HitWindows != null); diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs index a26db06ede..94bce53b12 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs @@ -11,7 +11,6 @@ using osu.Game.Rulesets.Osu.Judgements; using osu.Game.Graphics.Containers; using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Osu.UI; -using osu.Game.Rulesets.Scoring; using osuTK; namespace osu.Game.Rulesets.Osu.Objects.Drawables @@ -61,13 +60,6 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables PositionBindable.BindTo(HitObject.PositionBindable); StackHeightBindable.BindTo(HitObject.StackHeightBindable); ScaleBindable.BindTo(HitObject.ScaleBindable); - - // Manually set to reduce the number of future alive objects to a bare minimum. - LifetimeStart = HitObject.StartTime - HitObject.TimePreempt; - - // Arbitrary lifetime end to prevent past objects in idle states remaining alive in non-frame-stable contexts. - // An extra 1000ms is added to always overestimate the true lifetime, and a more exact value is set by hit transforms and the following expiry. - LifetimeEnd = HitObject.GetEndTime() + HitObject.HitWindows.WindowFor(HitResult.Miss) + 1000; } protected override void OnFree(HitObject hitObject) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinnerTick.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinnerTick.cs index fc9a7c00e6..f37d933e11 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinnerTick.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSpinnerTick.cs @@ -1,6 +1,8 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using osu.Game.Rulesets.Objects.Drawables; + namespace osu.Game.Rulesets.Osu.Objects.Drawables { public class DrawableSpinnerTick : DrawableOsuHitObject @@ -17,6 +19,16 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables { } + private DrawableSpinner drawableSpinner; + + protected override void OnParentReceived(DrawableHitObject parent) + { + base.OnParentReceived(parent); + drawableSpinner = (DrawableSpinner)parent; + } + + protected override double MaximumJudgementOffset => drawableSpinner.HitObject.Duration; + /// /// Apply a judgement result. /// diff --git a/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs b/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs index 8ff752952c..243092d067 100644 --- a/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs +++ b/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs @@ -176,6 +176,8 @@ namespace osu.Game.Rulesets.Osu.UI public OsuHitObjectLifetimeEntry(HitObject hitObject) : base(hitObject) { + // Arbitrary lifetime end to prevent past objects in idle states remaining alive in non-frame-stable contexts. + LifetimeEnd = HitObject.GetEndTime() + HitObject.HitWindows.WindowFor(HitResult.Miss) + 1000; } protected override double InitialLifetimeOffset => ((OsuHitObject)HitObject).TimePreempt; diff --git a/osu.Game.Rulesets.Taiko/Objects/Drawables/DrawableDrumRollTick.cs b/osu.Game.Rulesets.Taiko/Objects/Drawables/DrawableDrumRollTick.cs index bf44a80037..be659f6ca5 100644 --- a/osu.Game.Rulesets.Taiko/Objects/Drawables/DrawableDrumRollTick.cs +++ b/osu.Game.Rulesets.Taiko/Objects/Drawables/DrawableDrumRollTick.cs @@ -28,6 +28,8 @@ namespace osu.Game.Rulesets.Taiko.Objects.Drawables Filled = HitObject.FirstTick }); + protected override double MaximumJudgementOffset => HitObject.HitWindow; + protected override void CheckForResult(bool userTriggered, double timeOffset) { if (!userTriggered) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 537da24e01..eeaac0d77b 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -699,6 +699,18 @@ namespace osu.Game.Rulesets.Objects.Drawables UpdateResult(false); } + /// + /// The maximum offset from the end time of at which this can be judged. + /// The time offset of will be clamped to this value during . + /// + /// Defaults to the miss window of . + /// + /// + /// + /// This does not affect the time offset provided to invocations of . + /// + protected virtual double MaximumJudgementOffset => HitObject.HitWindows?.WindowFor(HitResult.Miss) ?? 0; + /// /// Applies the of this , notifying responders such as /// the of the . @@ -738,14 +750,7 @@ namespace osu.Game.Rulesets.Objects.Drawables $"{GetType().ReadableName()} applied an invalid hit result (was: {Result.Type}, expected: [{Result.Judgement.MinResult} ... {Result.Judgement.MaxResult}])."); } - // Ensure that the judgement is given a valid time offset, because this may not get set by the caller - var endTime = HitObject.GetEndTime(); - - Result.TimeOffset = Time.Current - endTime; - - double missWindow = HitObject.HitWindows.WindowFor(HitResult.Miss); - if (missWindow > 0) - Result.TimeOffset = Math.Min(Result.TimeOffset, missWindow); + Result.TimeOffset = Math.Min(MaximumJudgementOffset, Time.Current - HitObject.GetEndTime()); if (Result.HasResult) updateState(Result.IsHit ? ArmedState.Hit : ArmedState.Miss); @@ -767,8 +772,7 @@ namespace osu.Game.Rulesets.Objects.Drawables if (Judged) return false; - var endTime = HitObject.GetEndTime(); - CheckForResult(userTriggered, Time.Current - endTime); + CheckForResult(userTriggered, Time.Current - HitObject.GetEndTime()); return Judged; } From 0817dae86c14d5b7d158cb9920278e46407118ac Mon Sep 17 00:00:00 2001 From: ekrctb Date: Thu, 26 Nov 2020 12:35:49 +0900 Subject: [PATCH 12/27] Add failing test to check non-pooled lifetime --- .../Gameplay/TestSceneDrawableScrollingRuleset.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index cebe0394c7..7425c2b7c4 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -46,10 +46,11 @@ namespace osu.Game.Tests.Visual.Gameplay [SetUp] public void Setup() => Schedule(() => testClock.CurrentTime = 0); - [Test] - public void TestHitObjectPooling() + [TestCase("pooled")] + [TestCase("non-pooled")] + public void TestHitObjectLifetime(string pooled) { - var beatmap = createBeatmap(_ => new TestPooledHitObject()); + var beatmap = createBeatmap(_ => pooled == "pooled" ? new TestPooledHitObject() : new TestHitObject()); beatmap.ControlPointInfo.Add(0, new TimingControlPoint { BeatLength = time_range }); createTest(beatmap); @@ -208,13 +209,13 @@ namespace osu.Game.Tests.Visual.Gameplay /// /// Get a corresponding to the 'th . - /// When a pooling is used and the hit object is not alive, `null` is returned. + /// When the hit object is not alive, `null` is returned. /// [CanBeNull] private DrawableTestHitObject getDrawableHitObject(int index) { var hitObject = drawableRuleset.Beatmap.HitObjects.ElementAt(index); - return (DrawableTestHitObject)drawableRuleset.Playfield.HitObjectContainer.Objects.FirstOrDefault(obj => obj.HitObject == hitObject); + return (DrawableTestHitObject)drawableRuleset.Playfield.HitObjectContainer.AliveObjects.FirstOrDefault(obj => obj.HitObject == hitObject); } private float yScale => drawableRuleset.Playfield.HitObjectContainer.DrawHeight; @@ -426,6 +427,7 @@ namespace osu.Game.Tests.Visual.Gameplay } }); } + protected override void Update() => LifetimeEnd = HitObject.EndTime; } private class DrawableTestPooledHitObject : DrawableTestHitObject @@ -436,8 +438,6 @@ namespace osu.Game.Tests.Visual.Gameplay InternalChildren[0].Colour = Color4.LightSkyBlue; InternalChildren[1].Colour = Color4.Blue; } - - protected override void Update() => LifetimeEnd = HitObject.EndTime; } private class DrawableTestParentHitObject : DrawableTestHitObject From f6faf95e339960d83aa227d36b37e3b8fee1170e Mon Sep 17 00:00:00 2001 From: ekrctb Date: Thu, 26 Nov 2020 14:01:46 +0900 Subject: [PATCH 13/27] Revert changes to HitObjectUsageBegan, not use it. --- osu.Game/Rulesets/UI/HitObjectContainer.cs | 10 ++++---- osu.Game/Rulesets/UI/Playfield.cs | 4 ++-- .../Scrolling/ScrollingHitObjectContainer.cs | 24 +++++-------------- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/osu.Game/Rulesets/UI/HitObjectContainer.cs b/osu.Game/Rulesets/UI/HitObjectContainer.cs index fb91108605..5fbda305c8 100644 --- a/osu.Game/Rulesets/UI/HitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/HitObjectContainer.cs @@ -48,7 +48,7 @@ namespace osu.Game.Rulesets.UI /// /// If this uses pooled objects, this represents the time when the s become alive. /// - internal event Action HitObjectUsageBegan; + internal event Action HitObjectUsageBegan; /// /// Invoked when a becomes unused by a . @@ -56,7 +56,7 @@ namespace osu.Game.Rulesets.UI /// /// If this uses pooled objects, this represents the time when the s become dead. /// - internal event Action HitObjectUsageFinished; + internal event Action HitObjectUsageFinished; /// /// The amount of time prior to the current time within which s should be considered alive. @@ -115,7 +115,7 @@ namespace osu.Game.Rulesets.UI bindStartTime(drawable); AddInternal(drawableMap[entry] = drawable, false); - HitObjectUsageBegan?.Invoke(drawable); + HitObjectUsageBegan?.Invoke(entry.HitObject); } private void removeDrawable(HitObjectLifetimeEntry entry) @@ -129,10 +129,10 @@ namespace osu.Game.Rulesets.UI drawableMap.Remove(entry); - HitObjectUsageFinished?.Invoke(drawable); - unbindStartTime(drawable); RemoveInternal(drawable); + + HitObjectUsageFinished?.Invoke(entry.HitObject); } #endregion diff --git a/osu.Game/Rulesets/UI/Playfield.cs b/osu.Game/Rulesets/UI/Playfield.cs index 411bda77b8..2f589f4ce9 100644 --- a/osu.Game/Rulesets/UI/Playfield.cs +++ b/osu.Game/Rulesets/UI/Playfield.cs @@ -92,8 +92,8 @@ namespace osu.Game.Rulesets.UI { h.NewResult += (d, r) => NewResult?.Invoke(d, r); h.RevertResult += (d, r) => RevertResult?.Invoke(d, r); - h.HitObjectUsageBegan += o => HitObjectUsageBegan?.Invoke(o.HitObject); - h.HitObjectUsageFinished += o => HitObjectUsageFinished?.Invoke(o.HitObject); + h.HitObjectUsageBegan += o => HitObjectUsageBegan?.Invoke(o); + h.HitObjectUsageFinished += o => HitObjectUsageFinished?.Invoke(o); })); } diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index c8afe76f19..44732f490b 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -17,10 +17,6 @@ namespace osu.Game.Rulesets.UI.Scrolling private readonly IBindable timeRange = new BindableDouble(); private readonly IBindable direction = new Bindable(); - // Tracks all `DrawableHitObject` (nested or not) applied a `HitObject`. - // It dynamically changes based on approximate lifetime when a pooling is used. - private readonly HashSet hitObjectApplied = new HashSet(); - // The lifetime of a hit object in this will be computed in next update. private readonly HashSet toComputeLifetime = new HashSet(); @@ -39,9 +35,6 @@ namespace osu.Game.Rulesets.UI.Scrolling RelativeSizeAxes = Axes.Both; AddLayout(layoutCache); - - HitObjectUsageBegan += onHitObjectUsageBegin; - HitObjectUsageFinished += onHitObjectUsageFinished; } [BackgroundDependencyLoader] @@ -58,7 +51,6 @@ namespace osu.Game.Rulesets.UI.Scrolling { base.Clear(disposeChildren); - hitObjectApplied.Clear(); toComputeLifetime.Clear(); layoutComputed.Clear(); } @@ -159,18 +151,10 @@ namespace osu.Game.Rulesets.UI.Scrolling { // Lifetime computation is delayed until next update because // when the hit object is not pooled this container is not loaded here and `scrollLength` cannot be computed. - hitObjectApplied.Add(hitObject); toComputeLifetime.Add(hitObject); layoutComputed.Remove(hitObject); } - private void onHitObjectUsageFinished(DrawableHitObject hitObject) - { - hitObjectApplied.Remove(hitObject); - toComputeLifetime.Remove(hitObject); - layoutComputed.Remove(hitObject); - } - private float scrollLength; protected override void Update() @@ -179,8 +163,12 @@ namespace osu.Game.Rulesets.UI.Scrolling if (!layoutCache.IsValid) { - foreach (var hitObject in hitObjectApplied) - toComputeLifetime.Add(hitObject); + toComputeLifetime.Clear(); + foreach (var hitObject in Objects) + { + if (hitObject.HitObject != null) + toComputeLifetime.Add(hitObject); + } layoutComputed.Clear(); From e43f9285889ab3f649418f7fe74fb40979c33772 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Thu, 26 Nov 2020 14:07:40 +0900 Subject: [PATCH 14/27] Use DHO.HitObjectApplied to invalidate computation --- .../UI/Scrolling/ScrollingHitObjectContainer.cs | 5 ++++- .../Rulesets/UI/Scrolling/ScrollingPlayfield.cs | 13 +++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 44732f490b..6740fcf03d 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -147,7 +147,10 @@ namespace osu.Game.Rulesets.UI.Scrolling } } - private void onHitObjectUsageBegin(DrawableHitObject hitObject) + /// + /// Make this lifetime and layout computed in next update. + /// + internal void InvalidateHitObject(DrawableHitObject hitObject) { // Lifetime computation is delayed until next update because // when the hit object is not pooled this container is not loaded here and `scrollLength` cannot be computed. diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs index 9dac3f4de1..8aba896b34 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs @@ -15,6 +15,8 @@ namespace osu.Game.Rulesets.UI.Scrolling { protected readonly IBindable Direction = new Bindable(); + public new ScrollingHitObjectContainer HitObjectContainer => (ScrollingHitObjectContainer)base.HitObjectContainer; + [Resolved] protected IScrollingInfo ScrollingInfo { get; private set; } @@ -24,17 +26,20 @@ namespace osu.Game.Rulesets.UI.Scrolling Direction.BindTo(ScrollingInfo.Direction); } + protected override void OnNewDrawableHitObject(DrawableHitObject drawableHitObject) + { + drawableHitObject.HitObjectApplied += d => HitObjectContainer.InvalidateHitObject(d); + } + /// /// Given a position in screen space, return the time within this column. /// - public virtual double TimeAtScreenSpacePosition(Vector2 screenSpacePosition) => - ((ScrollingHitObjectContainer)HitObjectContainer).TimeAtScreenSpacePosition(screenSpacePosition); + public virtual double TimeAtScreenSpacePosition(Vector2 screenSpacePosition) => HitObjectContainer.TimeAtScreenSpacePosition(screenSpacePosition); /// /// Given a time, return the screen space position within this column. /// - public virtual Vector2 ScreenSpacePositionAtTime(double time) - => ((ScrollingHitObjectContainer)HitObjectContainer).ScreenSpacePositionAtTime(time); + public virtual Vector2 ScreenSpacePositionAtTime(double time) => HitObjectContainer.ScreenSpacePositionAtTime(time); protected sealed override HitObjectContainer CreateHitObjectContainer() => new ScrollingHitObjectContainer(); } From eae33fe74a56e7d8d8b8f7dce7b29172e57de8ce Mon Sep 17 00:00:00 2001 From: ekrctb Date: Thu, 26 Nov 2020 14:16:33 +0900 Subject: [PATCH 15/27] Fix format --- .../Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs | 1 + osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs | 1 + 2 files changed, 2 insertions(+) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index 7425c2b7c4..257ae10d82 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -427,6 +427,7 @@ namespace osu.Game.Tests.Visual.Gameplay } }); } + protected override void Update() => LifetimeEnd = HitObject.EndTime; } diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 6740fcf03d..02ee39e1b8 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -167,6 +167,7 @@ namespace osu.Game.Rulesets.UI.Scrolling if (!layoutCache.IsValid) { toComputeLifetime.Clear(); + foreach (var hitObject in Objects) { if (hitObject.HitObject != null) From f3f5ec766535b8ba201b34ed343bda7e5b8012b3 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Thu, 26 Nov 2020 18:08:40 +0900 Subject: [PATCH 16/27] Fix `Column` not calling `base.Add` --- osu.Game.Rulesets.Mania/UI/Column.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Mania/UI/Column.cs b/osu.Game.Rulesets.Mania/UI/Column.cs index 9aabcc6699..d2a9b69b60 100644 --- a/osu.Game.Rulesets.Mania/UI/Column.cs +++ b/osu.Game.Rulesets.Mania/UI/Column.cs @@ -97,7 +97,7 @@ namespace osu.Game.Rulesets.Mania.UI DrawableManiaHitObject maniaObject = (DrawableManiaHitObject)hitObject; maniaObject.CheckHittable = hitPolicy.IsHittable; - HitObjectContainer.Add(hitObject); + base.Add(hitObject); } public override bool Remove(DrawableHitObject h) From 05e245d4454987b27fdca0200b798eb131c73f26 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Thu, 26 Nov 2020 19:07:09 +0900 Subject: [PATCH 17/27] Allow non-pooled DHO to be reused --- osu.Game/Rulesets/UI/Playfield.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/osu.Game/Rulesets/UI/Playfield.cs b/osu.Game/Rulesets/UI/Playfield.cs index 2f589f4ce9..a2ac234471 100644 --- a/osu.Game/Rulesets/UI/Playfield.cs +++ b/osu.Game/Rulesets/UI/Playfield.cs @@ -135,10 +135,8 @@ namespace osu.Game.Rulesets.UI /// The DrawableHitObject to add. public virtual void Add(DrawableHitObject h) { - if (h.IsInitialized) - throw new InvalidOperationException($"{nameof(Add)} doesn't support {nameof(DrawableHitObject)} reuse. Use pooling instead."); - - onNewDrawableHitObject(h); + if (!h.IsInitialized) + onNewDrawableHitObject(h); HitObjectContainer.Add(h); OnHitObjectAdded(h.HitObject); From 6e40de58e9a17aa3374ff3722e43543c975f4a9f Mon Sep 17 00:00:00 2001 From: ekrctb Date: Fri, 27 Nov 2020 13:36:40 +0900 Subject: [PATCH 18/27] Use new OnAdd and OnRemove to invalidate DHO --- osu.Game/Rulesets/UI/HitObjectContainer.cs | 24 +++++++++++++++++ .../Scrolling/ScrollingHitObjectContainer.cs | 27 ++++++++++++++++++- .../UI/Scrolling/ScrollingPlayfield.cs | 5 ---- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/osu.Game/Rulesets/UI/HitObjectContainer.cs b/osu.Game/Rulesets/UI/HitObjectContainer.cs index 5fbda305c8..ac5d281ddc 100644 --- a/osu.Game/Rulesets/UI/HitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/HitObjectContainer.cs @@ -114,6 +114,7 @@ namespace osu.Game.Rulesets.UI bindStartTime(drawable); AddInternal(drawableMap[entry] = drawable, false); + OnAdd(drawable); HitObjectUsageBegan?.Invoke(entry.HitObject); } @@ -129,6 +130,7 @@ namespace osu.Game.Rulesets.UI drawableMap.Remove(entry); + OnRemove(drawable); unbindStartTime(drawable); RemoveInternal(drawable); @@ -147,10 +149,12 @@ namespace osu.Game.Rulesets.UI hitObject.OnRevertResult += onRevertResult; AddInternal(hitObject); + OnAdd(hitObject); } public virtual bool Remove(DrawableHitObject hitObject) { + OnRemove(hitObject); if (!RemoveInternal(hitObject)) return false; @@ -178,6 +182,26 @@ namespace osu.Game.Rulesets.UI #endregion + /// + /// Invoked when a is added to this container. + /// + /// + /// This method is not invoked for nested s. + /// + protected virtual void OnAdd(DrawableHitObject drawableHitObject) + { + } + + /// + /// Invoked when a is removed from this container. + /// + /// + /// This method is not invoked for nested s. + /// + protected virtual void OnRemove(DrawableHitObject drawableHitObject) + { + } + public virtual void Clear(bool disposeChildren = true) { lifetimeManager.ClearEntries(); diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 02ee39e1b8..71f8f95300 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -150,7 +150,7 @@ namespace osu.Game.Rulesets.UI.Scrolling /// /// Make this lifetime and layout computed in next update. /// - internal void InvalidateHitObject(DrawableHitObject hitObject) + private void invalidateHitObject(DrawableHitObject hitObject) { // Lifetime computation is delayed until next update because // when the hit object is not pooled this container is not loaded here and `scrollLength` cannot be computed. @@ -158,6 +158,31 @@ namespace osu.Game.Rulesets.UI.Scrolling layoutComputed.Remove(hitObject); } + private void onAddRecursive(DrawableHitObject hitObject) + { + invalidateHitObject(hitObject); + + hitObject.DefaultsApplied += invalidateHitObject; + + foreach (var nested in hitObject.NestedHitObjects) + onAddRecursive(nested); + } + + protected override void OnAdd(DrawableHitObject drawableHitObject) => onAddRecursive(drawableHitObject); + + private void onRemoveRecursive(DrawableHitObject hitObject) + { + toComputeLifetime.Remove(hitObject); + layoutComputed.Remove(hitObject); + + hitObject.DefaultsApplied -= invalidateHitObject; + + foreach (var nested in hitObject.NestedHitObjects) + onRemoveRecursive(nested); + } + + protected override void OnRemove(DrawableHitObject drawableHitObject) => onRemoveRecursive(drawableHitObject); + private float scrollLength; protected override void Update() diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs index 8aba896b34..2b75f93f9e 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingPlayfield.cs @@ -26,11 +26,6 @@ namespace osu.Game.Rulesets.UI.Scrolling Direction.BindTo(ScrollingInfo.Direction); } - protected override void OnNewDrawableHitObject(DrawableHitObject drawableHitObject) - { - drawableHitObject.HitObjectApplied += d => HitObjectContainer.InvalidateHitObject(d); - } - /// /// Given a position in screen space, return the time within this column. /// From 5bc76cac58a1e0690e9df0d6cd165841f9a3e736 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 27 Nov 2020 17:01:07 +0900 Subject: [PATCH 19/27] Remove unused using statement --- osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs index cf9908decd..628d95dff4 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableOsuHitObject.cs @@ -9,7 +9,6 @@ using osu.Framework.Graphics; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Osu.Judgements; using osu.Game.Graphics.Containers; -using osu.Game.Rulesets.Objects; using osu.Game.Rulesets.Osu.UI; using osuTK; From 809caaa44c72f72f5b64bf67a537ff6303d0a16a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 Nov 2020 15:39:08 +0900 Subject: [PATCH 20/27] Use standard switch syntax (preferred for now) --- .../TestSceneDrawableScrollingRuleset.cs | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index 257ae10d82..8da2b58c1e 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -302,14 +302,21 @@ namespace osu.Game.Tests.Visual.Gameplay TimeRange.Value = time_range; } - public override DrawableHitObject CreateDrawableRepresentation(TestHitObject h) => - h switch + public override DrawableHitObject CreateDrawableRepresentation(TestHitObject h) + { + switch (h) { - TestPooledHitObject _ => null, - TestPooledParentHitObject _ => null, - TestParentHitObject p => new DrawableTestParentHitObject(p), - _ => new DrawableTestHitObject(h), - }; + case TestPooledHitObject _: + case TestPooledParentHitObject _: + return null; + + case TestParentHitObject p: + return new DrawableTestParentHitObject(p); + + default: + return new DrawableTestHitObject(h); + } + } protected override PassThroughInputManager CreateInputManager() => new PassThroughInputManager(); From 31cfaefdeb57bb967555c225804db5309e979592 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 Nov 2020 15:39:43 +0900 Subject: [PATCH 21/27] Move private functions in line with others --- .../TestSceneDrawableScrollingRuleset.cs | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs index 8da2b58c1e..9931ee4a45 100644 --- a/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs +++ b/osu.Game.Tests/Visual/Gameplay/TestSceneDrawableScrollingRuleset.cs @@ -90,22 +90,6 @@ namespace osu.Game.Tests.Visual.Gameplay assertChildPosition(5); } - private void assertDead(int index) => AddAssert($"hitobject {index} is dead", () => getDrawableHitObject(index) == null); - - private void assertHeight(int index) => AddAssert($"hitobject {index} height", () => - { - var d = getDrawableHitObject(index); - return d != null && Precision.AlmostEquals(d.DrawHeight, yScale * (float)(d.HitObject.Duration / time_range), 0.1f); - }); - - private void assertChildPosition(int index) => AddAssert($"hitobject {index} child position", () => - { - var d = getDrawableHitObject(index); - return d is DrawableTestParentHitObject && Precision.AlmostEquals( - d.NestedHitObjects.First().DrawPosition.Y, - yScale * (float)((TestParentHitObject)d.HitObject).ChildTimeOffset / time_range, 0.1f); - }); - [Test] public void TestRelativeBeatLengthScaleSingleTimingPoint() { @@ -220,6 +204,22 @@ namespace osu.Game.Tests.Visual.Gameplay private float yScale => drawableRuleset.Playfield.HitObjectContainer.DrawHeight; + private void assertDead(int index) => AddAssert($"hitobject {index} is dead", () => getDrawableHitObject(index) == null); + + private void assertHeight(int index) => AddAssert($"hitobject {index} height", () => + { + var d = getDrawableHitObject(index); + return d != null && Precision.AlmostEquals(d.DrawHeight, yScale * (float)(d.HitObject.Duration / time_range), 0.1f); + }); + + private void assertChildPosition(int index) => AddAssert($"hitobject {index} child position", () => + { + var d = getDrawableHitObject(index); + return d is DrawableTestParentHitObject && Precision.AlmostEquals( + d.NestedHitObjects.First().DrawPosition.Y, + yScale * (float)((TestParentHitObject)d.HitObject).ChildTimeOffset / time_range, 0.1f); + }); + private void assertPosition(int index, float relativeY) => AddAssert($"hitobject {index} at {relativeY}", () => Precision.AlmostEquals(getDrawableHitObject(index)?.DrawPosition.Y ?? -1, yScale * relativeY)); From 274565998653284abcdef19fc4be19f8e9dc937b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 Nov 2020 15:54:20 +0900 Subject: [PATCH 22/27] Reword and xmldoc some comments --- .../UI/Scrolling/ScrollingHitObjectContainer.cs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 71f8f95300..6a77597916 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -17,11 +17,14 @@ namespace osu.Game.Rulesets.UI.Scrolling private readonly IBindable timeRange = new BindableDouble(); private readonly IBindable direction = new Bindable(); - // The lifetime of a hit object in this will be computed in next update. + /// + /// Hit objects which require lifetime computation in the next update call. + /// private readonly HashSet toComputeLifetime = new HashSet(); - // The layout (length if IHasDuration, and nested object positions) of a hit object *not* in this set will be computed in next updated. - // Only objects in `AliveObjects` are considered, to prevent a massive recomputation when scrolling speed or something changes. + /// + /// A set containing all which have an up-to-date layout. + /// private readonly HashSet layoutComputed = new HashSet(); [Resolved] @@ -223,8 +226,7 @@ namespace osu.Game.Rulesets.UI.Scrolling toComputeLifetime.Clear(); - // An assumption is that this update won't affect lifetime, - // but this is satisfied in practice because otherwise the hit object won't be aligned to its `StartTime`. + // only AliveObjects need to be considered for layout (reduces overhead in the case of scroll speed changes). foreach (var obj in AliveObjects) { if (layoutComputed.Contains(obj)) From a4e061cb11eeaca6644abca2e3bfee55eacd2c96 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 Nov 2020 17:18:29 +0900 Subject: [PATCH 23/27] Remove semi-transparent backgrounds from settings and notifications overlays I tried also updating the colours to the "new" versions from designs but they don't match due to colour profile differences (so I'm not yet sure if they are correct or not) and also don't look great without all the UI elements also being updated. --- osu.Game/Overlays/NotificationOverlay.cs | 4 ++-- osu.Game/Overlays/Settings/Sidebar.cs | 3 ++- osu.Game/Overlays/SettingsOverlay.cs | 2 -- osu.Game/Overlays/SettingsPanel.cs | 7 ++++--- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/osu.Game/Overlays/NotificationOverlay.cs b/osu.Game/Overlays/NotificationOverlay.cs index b5714fbcae..774b001224 100644 --- a/osu.Game/Overlays/NotificationOverlay.cs +++ b/osu.Game/Overlays/NotificationOverlay.cs @@ -13,6 +13,7 @@ using System; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Threading; +using osu.Game.Graphics; namespace osu.Game.Overlays { @@ -44,8 +45,7 @@ namespace osu.Game.Overlays new Box { RelativeSizeAxes = Axes.Both, - Colour = Color4.Black, - Alpha = 0.6f + Colour = OsuColour.Gray(0.05f), }, new OsuScrollContainer { diff --git a/osu.Game/Overlays/Settings/Sidebar.cs b/osu.Game/Overlays/Settings/Sidebar.cs index 031ecaae46..f548f933e2 100644 --- a/osu.Game/Overlays/Settings/Sidebar.cs +++ b/osu.Game/Overlays/Settings/Sidebar.cs @@ -9,6 +9,7 @@ using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Shapes; using osu.Framework.Input.Events; using osu.Framework.Threading; +using osu.Game.Graphics; using osu.Game.Graphics.Containers; using osuTK; using osuTK.Graphics; @@ -32,7 +33,7 @@ namespace osu.Game.Overlays.Settings { new Box { - Colour = Color4.Black, + Colour = OsuColour.Gray(0.02f), RelativeSizeAxes = Axes.Both, }, new SidebarScrollContainer diff --git a/osu.Game/Overlays/SettingsOverlay.cs b/osu.Game/Overlays/SettingsOverlay.cs index e1bcdbbaf0..e49885cdf8 100644 --- a/osu.Game/Overlays/SettingsOverlay.cs +++ b/osu.Game/Overlays/SettingsOverlay.cs @@ -61,7 +61,6 @@ namespace osu.Game.Overlays switch (state.NewValue) { case Visibility.Visible: - Background.FadeTo(0.9f, 300, Easing.OutQuint); Sidebar?.FadeColour(Color4.DarkGray, 300, Easing.OutQuint); SectionsContainer.FadeOut(300, Easing.OutQuint); @@ -69,7 +68,6 @@ namespace osu.Game.Overlays break; case Visibility.Hidden: - Background.FadeTo(0.6f, 500, Easing.OutQuint); Sidebar?.FadeColour(Color4.White, 300, Easing.OutQuint); SectionsContainer.FadeIn(500, Easing.OutQuint); diff --git a/osu.Game/Overlays/SettingsPanel.cs b/osu.Game/Overlays/SettingsPanel.cs index 2948231c4b..fea4b0738d 100644 --- a/osu.Game/Overlays/SettingsPanel.cs +++ b/osu.Game/Overlays/SettingsPanel.cs @@ -12,6 +12,7 @@ using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Shapes; using osu.Framework.Input.Events; +using osu.Game.Graphics; using osu.Game.Graphics.Containers; using osu.Game.Graphics.UserInterface; using osu.Game.Overlays.Settings; @@ -72,8 +73,8 @@ namespace osu.Game.Overlays Origin = Anchor.TopRight, Scale = new Vector2(2, 1), // over-extend to the left for transitions RelativeSizeAxes = Axes.Both, - Colour = Color4.Black, - Alpha = 0.6f, + Colour = OsuColour.Gray(0.05f), + Alpha = 1, }, SectionsContainer = new SettingsSectionsContainer { @@ -214,7 +215,7 @@ namespace osu.Game.Overlays base.UpdateAfterChildren(); // no null check because the usage of this class is strict - HeaderBackground.Alpha = -ExpandableHeader.Y / ExpandableHeader.LayoutSize.Y * 0.5f; + HeaderBackground.Alpha = -ExpandableHeader.Y / ExpandableHeader.LayoutSize.Y * 1; } } } From 7ac2fba1273e8d6d495e76ba9f6e4c3332724c68 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 30 Nov 2020 17:44:58 +0900 Subject: [PATCH 24/27] More reordering of public vs private methods --- .../Scrolling/ScrollingHitObjectContainer.cs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs index 6a77597916..3a5e3c098f 100644 --- a/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs +++ b/osu.Game/Rulesets/UI/Scrolling/ScrollingHitObjectContainer.cs @@ -150,16 +150,9 @@ namespace osu.Game.Rulesets.UI.Scrolling } } - /// - /// Make this lifetime and layout computed in next update. - /// - private void invalidateHitObject(DrawableHitObject hitObject) - { - // Lifetime computation is delayed until next update because - // when the hit object is not pooled this container is not loaded here and `scrollLength` cannot be computed. - toComputeLifetime.Add(hitObject); - layoutComputed.Remove(hitObject); - } + protected override void OnAdd(DrawableHitObject drawableHitObject) => onAddRecursive(drawableHitObject); + + protected override void OnRemove(DrawableHitObject drawableHitObject) => onRemoveRecursive(drawableHitObject); private void onAddRecursive(DrawableHitObject hitObject) { @@ -171,8 +164,6 @@ namespace osu.Game.Rulesets.UI.Scrolling onAddRecursive(nested); } - protected override void OnAdd(DrawableHitObject drawableHitObject) => onAddRecursive(drawableHitObject); - private void onRemoveRecursive(DrawableHitObject hitObject) { toComputeLifetime.Remove(hitObject); @@ -184,7 +175,16 @@ namespace osu.Game.Rulesets.UI.Scrolling onRemoveRecursive(nested); } - protected override void OnRemove(DrawableHitObject drawableHitObject) => onRemoveRecursive(drawableHitObject); + /// + /// Make this lifetime and layout computed in next update. + /// + private void invalidateHitObject(DrawableHitObject hitObject) + { + // Lifetime computation is delayed until next update because + // when the hit object is not pooled this container is not loaded here and `scrollLength` cannot be computed. + toComputeLifetime.Add(hitObject); + layoutComputed.Remove(hitObject); + } private float scrollLength; From bf2c6dc241d561c545dae3dbff0aa35cd8e306ba Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 30 Nov 2020 17:49:55 +0900 Subject: [PATCH 25/27] Remove unused usings rider couldn't see --- osu.Game/Overlays/NotificationOverlay.cs | 1 - osu.Game/Overlays/Settings/Sidebar.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/osu.Game/Overlays/NotificationOverlay.cs b/osu.Game/Overlays/NotificationOverlay.cs index 774b001224..d51d964fc4 100644 --- a/osu.Game/Overlays/NotificationOverlay.cs +++ b/osu.Game/Overlays/NotificationOverlay.cs @@ -6,7 +6,6 @@ using osu.Framework.Extensions.IEnumerableExtensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Game.Overlays.Notifications; -using osuTK.Graphics; using osu.Framework.Graphics.Shapes; using osu.Game.Graphics.Containers; using System; diff --git a/osu.Game/Overlays/Settings/Sidebar.cs b/osu.Game/Overlays/Settings/Sidebar.cs index f548f933e2..4ca6e2ec42 100644 --- a/osu.Game/Overlays/Settings/Sidebar.cs +++ b/osu.Game/Overlays/Settings/Sidebar.cs @@ -12,7 +12,6 @@ using osu.Framework.Threading; using osu.Game.Graphics; using osu.Game.Graphics.Containers; using osuTK; -using osuTK.Graphics; namespace osu.Game.Overlays.Settings { From a3ef858f3a03f7c694348d7674a72d963f3dd162 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 30 Nov 2020 17:56:04 +0900 Subject: [PATCH 26/27] Remove unnecessary multiplication --- osu.Game/Overlays/SettingsPanel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Overlays/SettingsPanel.cs b/osu.Game/Overlays/SettingsPanel.cs index fea4b0738d..7a5a586f67 100644 --- a/osu.Game/Overlays/SettingsPanel.cs +++ b/osu.Game/Overlays/SettingsPanel.cs @@ -215,7 +215,7 @@ namespace osu.Game.Overlays base.UpdateAfterChildren(); // no null check because the usage of this class is strict - HeaderBackground.Alpha = -ExpandableHeader.Y / ExpandableHeader.LayoutSize.Y * 1; + HeaderBackground.Alpha = -ExpandableHeader.Y / ExpandableHeader.LayoutSize.Y; } } } From fdef6e479c750b253c619e8c8e202bc7ba392a0a Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 30 Nov 2020 18:28:04 +0900 Subject: [PATCH 27/27] Remove 1000ms offset and adjust comment --- osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs b/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs index 243092d067..975b444699 100644 --- a/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs +++ b/osu.Game.Rulesets.Osu/UI/OsuPlayfield.cs @@ -176,8 +176,8 @@ namespace osu.Game.Rulesets.Osu.UI public OsuHitObjectLifetimeEntry(HitObject hitObject) : base(hitObject) { - // Arbitrary lifetime end to prevent past objects in idle states remaining alive in non-frame-stable contexts. - LifetimeEnd = HitObject.GetEndTime() + HitObject.HitWindows.WindowFor(HitResult.Miss) + 1000; + // Prevent past objects in idles states from remaining alive as their end times are skipped in non-frame-stable contexts. + LifetimeEnd = HitObject.GetEndTime() + HitObject.HitWindows.WindowFor(HitResult.Miss); } protected override double InitialLifetimeOffset => ((OsuHitObject)HitObject).TimePreempt;