From 7980d16b4c202bcb063c0569112ad76463e8a9c6 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 27 Apr 2021 14:26:12 +0900 Subject: [PATCH 1/7] Add failing test showing the issue of DHO lifetime --- .../Gameplay/TestSceneDrawableHitObject.cs | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs diff --git a/osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs b/osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs new file mode 100644 index 0000000000..d42802db91 --- /dev/null +++ b/osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs @@ -0,0 +1,62 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Framework.Testing; +using osu.Game.Rulesets.Objects; +using osu.Game.Rulesets.Objects.Drawables; +using osu.Game.Tests.Visual; + +namespace osu.Game.Tests.Gameplay +{ + [HeadlessTest] + public class TestSceneDrawableHitObject : OsuTestScene + { + [Test] + public void TestEntryLifetime() + { + TestDrawableHitObject dho = null; + var initialHitObject = new HitObject + { + StartTime = 1000 + }; + var entry = new TestLifetimeEntry(new HitObject + { + StartTime = 2000 + }); + + AddStep("Create DHO", () => Child = dho = new TestDrawableHitObject(initialHitObject)); + + AddAssert("Correct initial lifetime", () => dho.LifetimeStart == initialHitObject.StartTime - TestDrawableHitObject.INITIAL_LIFETIME_OFFSET); + + AddStep("Apply entry", () => dho.Apply(entry)); + + AddAssert("Correct initial lifetime", () => dho.LifetimeStart == entry.HitObject.StartTime - TestLifetimeEntry.INITIAL_LIFETIME_OFFSET); + + AddStep("Set lifetime", () => dho.LifetimeEnd = 3000); + AddAssert("Entry lifetime is updated", () => entry.LifetimeEnd == 3000); + } + + private class TestDrawableHitObject : DrawableHitObject + { + public const double INITIAL_LIFETIME_OFFSET = 100; + protected override double InitialLifetimeOffset => INITIAL_LIFETIME_OFFSET; + + public TestDrawableHitObject(HitObject hitObject) + : base(hitObject) + { + } + } + + private class TestLifetimeEntry : HitObjectLifetimeEntry + { + public const double INITIAL_LIFETIME_OFFSET = 200; + protected override double InitialLifetimeOffset => INITIAL_LIFETIME_OFFSET; + + public TestLifetimeEntry(HitObject hitObject) + : base(hitObject) + { + } + } + } +} From a2c0951d94102cee2660e65412ffb4c4850114f1 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 27 Apr 2021 15:23:33 +0900 Subject: [PATCH 2/7] Use overriding instead of hiding in HitObjectLifetimeEntry Hidden properties are used when the type is the base class. It caused issues when `DrawableHitObject` logic is factored out to `PoolableDrawableWithLifetime` because it is using the base `LifetimeEntry`, not `HitObjectLifetimeEntry`. --- .../Objects/Drawables/DrawableHitObject.cs | 5 ++- .../Objects/HitObjectLifetimeEntry.cs | 41 +++++-------------- 2 files changed, 13 insertions(+), 33 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs index 7739994527..6431ec8980 100644 --- a/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs +++ b/osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs @@ -11,6 +11,7 @@ using osu.Framework.Bindables; using osu.Framework.Extensions.TypeExtensions; using osu.Framework.Graphics; +using osu.Framework.Graphics.Performance; using osu.Framework.Graphics.Primitives; using osu.Framework.Threading; using osu.Game.Audio; @@ -431,7 +432,7 @@ private void clearExistingStateTransforms() /// /// Apply (generally fade-in) transforms leading into the start time. - /// The local drawable hierarchy is recursively delayed to for convenience. + /// The local drawable hierarchy is recursively delayed to for convenience. /// /// By default this will fade in the object from zero with no duration. /// @@ -613,7 +614,7 @@ protected override void UpdateAfterChildren() /// /// This is only used as an optimisation to delay the initial update of this and may be tuned more aggressively if required. /// It is indirectly used to decide the automatic transform offset provided to . - /// A more accurate should be set for further optimisation (in , for example). + /// A more accurate should be set for further optimisation (in , for example). /// /// Only has an effect if this is not being pooled. /// For pooled s, use instead. diff --git a/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs b/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs index 1954d7e6d2..23e991d00e 100644 --- a/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs +++ b/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs @@ -38,40 +38,19 @@ public HitObjectLifetimeEntry(HitObject hitObject) startTimeBindable.BindValueChanged(onStartTimeChanged, true); } - // The lifetime start, as set by the hitobject. + // The lifetime, as set by the hitobject. private double realLifetimeStart = double.MinValue; - - /// - /// The time at which the should become alive. - /// - public new double LifetimeStart - { - get => realLifetimeStart; - set => setLifetime(realLifetimeStart = value, LifetimeEnd); - } - - // The lifetime end, as set by the hitobject. private double realLifetimeEnd = double.MaxValue; - /// - /// The time at which the should become dead. - /// - public new double LifetimeEnd + public override void SetLifetime(double start, double end) { - get => realLifetimeEnd; - set => setLifetime(LifetimeStart, realLifetimeEnd = value); - } + realLifetimeStart = start; + realLifetimeEnd = end; - private void setLifetime(double start, double end) - { if (keepAlive) - { - start = double.MinValue; - end = double.MaxValue; - } - - base.LifetimeStart = start; - base.LifetimeEnd = end; + base.SetLifetime(double.MinValue, double.MaxValue); + else + base.SetLifetime(start, end); } private bool keepAlive; @@ -87,7 +66,7 @@ internal bool KeepAlive return; keepAlive = value; - setLifetime(realLifetimeStart, realLifetimeEnd); + SetLifetime(realLifetimeStart, realLifetimeEnd); } } @@ -98,12 +77,12 @@ internal bool KeepAlive /// /// This is only used as an optimisation to delay the initial update of the and may be tuned more aggressively if required. /// It is indirectly used to decide the automatic transform offset provided to . - /// A more accurate should be set for further optimisation (in , for example). + /// A more accurate should be set for further optimisation (in , for example). /// protected virtual double InitialLifetimeOffset => 10000; /// - /// Resets according to the change in start time of the . + /// Resets according to the change in start time of the . /// private void onStartTimeChanged(ValueChangedEvent startTime) => LifetimeStart = HitObject.StartTime - InitialLifetimeOffset; } From c9e6ca537896fd7a099d2b4a4338b1ecfcda9001 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 27 Apr 2021 15:24:52 +0900 Subject: [PATCH 3/7] Use now-public Entry.SetLifetime method --- .../Objects/Pooling/PoolableDrawableWithLifetime.cs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs b/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs index 93e476be76..049ff9d446 100644 --- a/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs +++ b/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs @@ -100,11 +100,7 @@ private void setLifetime(double start, double end) base.LifetimeStart = start; base.LifetimeEnd = end; - if (Entry != null) - { - Entry.LifetimeStart = start; - Entry.LifetimeEnd = end; - } + Entry?.SetLifetime(start, end); } private void free() From 3899e500d3edf2b15622ed4e5ace339ab68fbdc0 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 27 Apr 2021 17:54:18 +0900 Subject: [PATCH 4/7] Adopt framework change of LifetimeEntry Override SetLifetimeStart/SetLifetimeEnd separately to track individual assignment. It is necessary to ensure real lifetime is not lost when lifetime is partially updated. --- .../Rulesets/Objects/HitObjectLifetimeEntry.cs | 15 +++++++++++++-- .../Pooling/PoolableDrawableWithLifetime.cs | 6 +++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs b/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs index 23e991d00e..0181d6241b 100644 --- a/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs +++ b/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs @@ -42,11 +42,22 @@ public HitObjectLifetimeEntry(HitObject hitObject) private double realLifetimeStart = double.MinValue; private double realLifetimeEnd = double.MaxValue; - public override void SetLifetime(double start, double end) + // This method is called even if `start == LifetimeStart` when `KeepAlive` is true (necessary to update `realLifetimeStart`). + protected override void SetLifetimeStart(double start) { + // This assignment cannot be done in `SetLifetime` because otherwise setting only `LifetimeStart` will make `realLifetimeEnd` to be lost. realLifetimeStart = start; - realLifetimeEnd = end; + base.SetLifetimeStart(start); + } + protected override void SetLifetimeEnd(double end) + { + realLifetimeEnd = end; + base.SetLifetimeEnd(end); + } + + protected override void SetLifetime(double start, double end) + { if (keepAlive) base.SetLifetime(double.MinValue, double.MaxValue); else diff --git a/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs b/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs index 049ff9d446..93e476be76 100644 --- a/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs +++ b/osu.Game/Rulesets/Objects/Pooling/PoolableDrawableWithLifetime.cs @@ -100,7 +100,11 @@ private void setLifetime(double start, double end) base.LifetimeStart = start; base.LifetimeEnd = end; - Entry?.SetLifetime(start, end); + if (Entry != null) + { + Entry.LifetimeStart = start; + Entry.LifetimeEnd = end; + } } private void free() From 003553aba32d5af6a9decf2a7f2b8c307857ac9b Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 27 Apr 2021 18:10:44 +0900 Subject: [PATCH 5/7] Add test of HitObjectLifetimeEntry.KeepAlive behavior --- .../Gameplay/TestSceneDrawableHitObject.cs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs b/osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs index d42802db91..2e3f192f1b 100644 --- a/osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs +++ b/osu.Game.Tests/Gameplay/TestSceneDrawableHitObject.cs @@ -37,6 +37,38 @@ public void TestEntryLifetime() AddAssert("Entry lifetime is updated", () => entry.LifetimeEnd == 3000); } + [Test] + public void TestKeepAlive() + { + TestDrawableHitObject dho = null; + TestLifetimeEntry entry = null; + AddStep("Create DHO", () => + { + dho = new TestDrawableHitObject(null); + dho.Apply(entry = new TestLifetimeEntry(new HitObject()) + { + LifetimeStart = 0, + LifetimeEnd = 1000, + }); + Child = dho; + }); + + AddStep("KeepAlive = true", () => entry.KeepAlive = true); + AddAssert("Lifetime is overriden", () => entry.LifetimeStart == double.MinValue && entry.LifetimeEnd == double.MaxValue); + + AddStep("Set LifetimeStart", () => dho.LifetimeStart = 500); + AddStep("KeepAlive = false", () => entry.KeepAlive = false); + AddAssert("Lifetime is correct", () => entry.LifetimeStart == 500 && entry.LifetimeEnd == 1000); + + AddStep("Set LifetimeStart while KeepAlive", () => + { + entry.KeepAlive = true; + dho.LifetimeStart = double.MinValue; + entry.KeepAlive = false; + }); + AddAssert("Lifetime is changed", () => entry.LifetimeStart == double.MinValue && entry.LifetimeEnd == 1000); + } + private class TestDrawableHitObject : DrawableHitObject { public const double INITIAL_LIFETIME_OFFSET = 100; From b87446a5778ee76c96f02cd163beebdc44476e24 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 27 Apr 2021 19:37:01 +0900 Subject: [PATCH 6/7] Simplify HitObjectLifetimeEntry logic a bit --- .../Objects/HitObjectLifetimeEntry.cs | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs b/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs index 0181d6241b..0d1eb68f07 100644 --- a/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs +++ b/osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs @@ -45,23 +45,16 @@ public HitObjectLifetimeEntry(HitObject hitObject) // This method is called even if `start == LifetimeStart` when `KeepAlive` is true (necessary to update `realLifetimeStart`). protected override void SetLifetimeStart(double start) { - // This assignment cannot be done in `SetLifetime` because otherwise setting only `LifetimeStart` will make `realLifetimeEnd` to be lost. realLifetimeStart = start; - base.SetLifetimeStart(start); + if (!keepAlive) + base.SetLifetimeStart(start); } protected override void SetLifetimeEnd(double end) { realLifetimeEnd = end; - base.SetLifetimeEnd(end); - } - - protected override void SetLifetime(double start, double end) - { - if (keepAlive) - base.SetLifetime(double.MinValue, double.MaxValue); - else - base.SetLifetime(start, end); + if (!keepAlive) + base.SetLifetimeEnd(end); } private bool keepAlive; @@ -77,7 +70,10 @@ internal bool KeepAlive return; keepAlive = value; - SetLifetime(realLifetimeStart, realLifetimeEnd); + if (keepAlive) + SetLifetime(double.MinValue, double.MaxValue); + else + SetLifetime(realLifetimeStart, realLifetimeEnd); } } From 5dadfd04e7c11f0e69982401fe5647ba67621067 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 27 Apr 2021 22:36:25 +0900 Subject: [PATCH 7/7] Update framework --- osu.Android.props | 2 +- osu.Game/osu.Game.csproj | 2 +- osu.iOS.props | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Android.props b/osu.Android.props index e0b07549f4..5aee9e15cc 100644 --- a/osu.Android.props +++ b/osu.Android.props @@ -52,6 +52,6 @@ - + diff --git a/osu.Game/osu.Game.csproj b/osu.Game/osu.Game.csproj index 9a43d5f031..986bd8e7ba 100644 --- a/osu.Game/osu.Game.csproj +++ b/osu.Game/osu.Game.csproj @@ -29,7 +29,7 @@ - + diff --git a/osu.iOS.props b/osu.iOS.props index 2c41b3ef26..c32109d6db 100644 --- a/osu.iOS.props +++ b/osu.iOS.props @@ -70,7 +70,7 @@ - + @@ -93,7 +93,7 @@ - +