Merge pull request #26770 from peppy/fucking-hitobject-references

Fix `OsuPlayfield` being retained indefinitely after gameplay

osu.Game.Rulesets.Osu/OsuInputManager.cs

@@ -98,7 +98,7 @@ namespace osu.Game.Rulesets.Osu
                 base.ReloadMappings(realmKeyBindings);
 
                 if (!AllowGameplayInputs)
-                    KeyBindings = KeyBindings.Where(b => b.GetAction<OsuAction>() == OsuAction.Smoke).ToList();
+                    KeyBindings = KeyBindings.Where(static b => b.GetAction<OsuAction>() == OsuAction.Smoke).ToList();
             }
         }
     }

osu.Game.Tests/Visual/Gameplay/TestSceneAutoplay.cs

@@ -47,8 +47,8 @@ namespace osu.Game.Tests.Visual.Gameplay
             seekTo(referenceBeatmap.HitObjects[^1].GetEndTime());
             AddUntilStep("results displayed", () => getResultsScreen()?.IsLoaded == true);
 
-            AddAssert("score has combo", () => getResultsScreen().Score.Combo > 100);
+            AddAssert("score has combo", () => getResultsScreen().Score!.Combo > 100);
-            AddAssert("score has no misses", () => getResultsScreen().Score.Statistics[HitResult.Miss] == 0);
+            AddAssert("score has no misses", () => getResultsScreen().Score!.Statistics[HitResult.Miss] == 0);
 
             AddUntilStep("avatar displayed", () => getAvatar() != null);
             AddAssert("avatar not clickable", () => getAvatar().ChildrenOfType<OsuClickableContainer>().First().Action == null);

osu.Game.Tests/Visual/Gameplay/TestScenePlayerLocalScoreImport.cs

@@ -138,8 +138,8 @@ namespace osu.Game.Tests.Visual.Gameplay
             AddUntilStep("results displayed", () => Player.GetChildScreen() is ResultsScreen);
 
             // Player creates new instance of mods after gameplay to ensure any runtime references to drawables etc. are not retained.
-            AddAssert("results screen score has copied mods", () => (Player.GetChildScreen() as ResultsScreen)?.Score.Mods.First(), () => Is.Not.SameAs(playerMods.First()));
+            AddAssert("results screen score has copied mods", () => (Player.GetChildScreen() as ResultsScreen)?.Score?.Mods.First(), () => Is.Not.SameAs(playerMods.First()));
-            AddAssert("results screen score has matching", () => (Player.GetChildScreen() as ResultsScreen)?.Score.Mods.First(), () => Is.EqualTo(playerMods.First()));
+            AddAssert("results screen score has matching", () => (Player.GetChildScreen() as ResultsScreen)?.Score?.Mods.First(), () => Is.EqualTo(playerMods.First()));
 
             AddUntilStep("score in database", () => Realm.Run(r => r.Find<ScoreInfo>(Player.Score.ScoreInfo.ID) != null));
             AddUntilStep("databased score has correct mods", () => Realm.Run(r => r.Find<ScoreInfo>(Player.Score.ScoreInfo.ID))!.Mods.First(), () => Is.EqualTo(playerMods.First()));

osu.Game.Tests/Visual/Multiplayer/TestSceneMultiplayer.cs

@@ -698,7 +698,7 @@ namespace osu.Game.Tests.Visual.Multiplayer
             {
                 var scoreInfo = ((ResultsScreen)multiplayerComponents.CurrentScreen).Score;
 
-                return !scoreInfo.Passed && scoreInfo.Rank == ScoreRank.F;
+                return scoreInfo?.Passed == false && scoreInfo.Rank == ScoreRank.F;
             });
         }
 

osu.Game.Tests/Visual/Navigation/TestScenePresentScore.cs

@@ -193,7 +193,7 @@ namespace osu.Game.Tests.Visual.Navigation
                 case ScorePresentType.Results:
                     AddUntilStep("wait for results", () => lastWaitedScreen != Game.ScreenStack.CurrentScreen && Game.ScreenStack.CurrentScreen is ResultsScreen);
                     AddStep("store last waited screen", () => lastWaitedScreen = Game.ScreenStack.CurrentScreen);
-                    AddUntilStep("correct score displayed", () => ((ResultsScreen)Game.ScreenStack.CurrentScreen).Score.Equals(getImport()));
+                    AddUntilStep("correct score displayed", () => ((ResultsScreen)Game.ScreenStack.CurrentScreen).Score!.Equals(getImport()));
                     AddAssert("correct ruleset selected", () => Game.Ruleset.Value.Equals(getImport().Ruleset));
                     break;
 

osu.Game.Tests/Visual/Playlists/TestScenePlaylistsResultsScreen.cs

@@ -420,7 +420,7 @@ namespace osu.Game.Tests.Visual.Playlists
             public new LoadingSpinner RightSpinner => base.RightSpinner;
             public new ScorePanelList ScorePanelList => base.ScorePanelList;
 
-            public TestResultsScreen(ScoreInfo score, int roomId, PlaylistItem playlistItem, bool allowRetry = true)
+            public TestResultsScreen([CanBeNull] ScoreInfo score, int roomId, PlaylistItem playlistItem, bool allowRetry = true)
                 : base(score, roomId, playlistItem, allowRetry)
             {
             }

osu.Game.Tests/Visual/Ranking/TestSceneResultsScreen.cs

@@ -418,7 +418,7 @@ namespace osu.Game.Tests.Visual.Ranking
             public UnrankedSoloResultsScreen(ScoreInfo score)
                 : base(score, true)
             {
-                Score.BeatmapInfo!.OnlineID = 0;
+                Score!.BeatmapInfo!.OnlineID = 0;
                 Score.BeatmapInfo.Status = BeatmapOnlineStatus.Pending;
             }
 

osu.Game/OsuGameBase.cs

@@ -340,10 +340,6 @@ namespace osu.Game
             dependencies.Cache(beatmapCache = new BeatmapLookupCache());
             base.Content.Add(beatmapCache);
 
-            var scorePerformanceManager = new ScorePerformanceCache();
-            dependencies.Cache(scorePerformanceManager);
-            base.Content.Add(scorePerformanceManager);
-
             dependencies.CacheAs<IRulesetConfigCache>(rulesetConfigCache = new RulesetConfigCache(realm, RulesetStore));
 
             var powerStatus = CreateBatteryInfo();

osu.Game/Rulesets/Difficulty/PerformanceBreakdownCalculator.cs

@@ -21,21 +21,29 @@ namespace osu.Game.Rulesets.Difficulty
     {
         private readonly IBeatmap playableBeatmap;
         private readonly BeatmapDifficultyCache difficultyCache;
-        private readonly ScorePerformanceCache performanceCache;
 
-        public PerformanceBreakdownCalculator(IBeatmap playableBeatmap, BeatmapDifficultyCache difficultyCache, ScorePerformanceCache performanceCache)
+        public PerformanceBreakdownCalculator(IBeatmap playableBeatmap, BeatmapDifficultyCache difficultyCache)
         {
             this.playableBeatmap = playableBeatmap;
             this.difficultyCache = difficultyCache;
-            this.performanceCache = performanceCache;
         }
 
         [ItemCanBeNull]
         public async Task<PerformanceBreakdown> CalculateAsync(ScoreInfo score, CancellationToken cancellationToken = default)
         {
+            var attributes = await difficultyCache.GetDifficultyAsync(score.BeatmapInfo!, score.Ruleset, score.Mods, cancellationToken).ConfigureAwait(false);
+
+            var performanceCalculator = score.Ruleset.CreateInstance().CreatePerformanceCalculator();
+
+            // Performance calculation requires the beatmap and ruleset to be locally available. If not, return a default value.
+            if (attributes?.Attributes == null || performanceCalculator == null)
+                return null;
+
+            cancellationToken.ThrowIfCancellationRequested();
+
             PerformanceAttributes[] performanceArray = await Task.WhenAll(
                 // compute actual performance
-                performanceCache.CalculatePerformanceAsync(score, cancellationToken),
+                performanceCalculator.CalculateAsync(score, attributes.Value.Attributes, cancellationToken),
                 // compute performance for perfect play
                 getPerfectPerformance(score, cancellationToken)
             ).ConfigureAwait(false);
@@ -88,8 +96,12 @@ namespace osu.Game.Rulesets.Difficulty
                     cancellationToken
                 ).ConfigureAwait(false);
 
-                // ScorePerformanceCache is not used to avoid caching multiple copies of essentially identical perfect performance attributes
+                var performanceCalculator = ruleset.CreatePerformanceCalculator();
-                return difficulty == null ? null : ruleset.CreatePerformanceCalculator()?.Calculate(perfectPlay, difficulty.Value.Attributes.AsNonNull());
+
+                if (performanceCalculator == null || difficulty == null)
+                    return null;
+
+                return await performanceCalculator.CalculateAsync(perfectPlay, difficulty.Value.Attributes.AsNonNull(), cancellationToken).ConfigureAwait(false);
             }, cancellationToken);
         }
 

osu.Game/Rulesets/Difficulty/PerformanceCalculator.cs

@@ -1,6 +1,8 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System.Threading;
+using System.Threading.Tasks;
 using osu.Game.Beatmaps;
 using osu.Game.Scoring;
 
@@ -15,6 +17,9 @@ namespace osu.Game.Rulesets.Difficulty
             Ruleset = ruleset;
         }
 
+        public Task<PerformanceAttributes> CalculateAsync(ScoreInfo score, DifficultyAttributes attributes, CancellationToken cancellationToken)
+            => Task.Run(() => CreatePerformanceAttributes(score, attributes), cancellationToken);
+
         public PerformanceAttributes Calculate(ScoreInfo score, DifficultyAttributes attributes)
             => CreatePerformanceAttributes(score, attributes);
 

osu.Game/Rulesets/Objects/Drawables/DrawableHitObject.cs

@@ -768,6 +768,13 @@ namespace osu.Game.Rulesets.Objects.Drawables
 
             if (CurrentSkin != null)
                 CurrentSkin.SourceChanged -= skinSourceChanged;
+
+            // Safeties against shooting in foot in cases where these are bound by external entities (like playfield) that don't clean up.
+            OnNestedDrawableCreated = null;
+            OnNewResult = null;
+            OnRevertResult = null;
+            DefaultsApplied = null;
+            HitObjectApplied = null;
         }
 
         public Bindable<double> AnimationStartTime { get; } = new BindableDouble();

osu.Game/Rulesets/Objects/HitObject.cs

@@ -38,6 +38,8 @@ namespace osu.Game.Rulesets.Objects
         /// <summary>
         /// Invoked after <see cref="ApplyDefaults"/> has completed on this <see cref="HitObject"/>.
         /// </summary>
+        // TODO: This has no implicit unbind flow. Currently, if a Playfield manages HitObjects it will leave a bound event on this and cause the
+        // playfield to remain in memory.
         public event Action<HitObject> DefaultsApplied;
 
         public readonly Bindable<double> StartTimeBindable = new BindableDouble();

osu.Game/Rulesets/UI/RulesetInputManager.cs

@@ -218,7 +218,7 @@ namespace osu.Game.Rulesets.UI
             {
                 base.ReloadMappings(realmKeyBindings);
 
-                KeyBindings = KeyBindings.Where(b => RealmKeyBindingStore.CheckValidForGameplay(b.KeyCombination)).ToList();
+                KeyBindings = KeyBindings.Where(static b => RealmKeyBindingStore.CheckValidForGameplay(b.KeyCombination)).ToList();
                 RealmKeyBindingStore.ClearDuplicateBindings(KeyBindings);
             }
         }

osu.Game/Scoring/ScorePerformanceCache.cs

@@ -1,68 +0,0 @@
-// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
-// See the LICENCE file in the repository root for full licence text.
-
-using System;
-using System.Threading;
-using System.Threading.Tasks;
-using osu.Framework.Allocation;
-using osu.Game.Beatmaps;
-using osu.Game.Database;
-using osu.Game.Rulesets.Difficulty;
-
-namespace osu.Game.Scoring
-{
-    /// <summary>
-    /// A component which performs and acts as a central cache for performance calculations of locally databased scores.
-    /// Currently not persisted between game sessions.
-    /// </summary>
-    public partial class ScorePerformanceCache : MemoryCachingComponent<ScorePerformanceCache.PerformanceCacheLookup, PerformanceAttributes>
-    {
-        [Resolved]
-        private BeatmapDifficultyCache difficultyCache { get; set; } = null!;
-
-        protected override bool CacheNullValues => false;
-
-        /// <summary>
-        /// Calculates performance for the given <see cref="ScoreInfo"/>.
-        /// </summary>
-        /// <param name="score">The score to do the calculation on. </param>
-        /// <param name="token">An optional <see cref="CancellationToken"/> to cancel the operation.</param>
-        public Task<PerformanceAttributes?> CalculatePerformanceAsync(ScoreInfo score, CancellationToken token = default) =>
-            GetAsync(new PerformanceCacheLookup(score), token);
-
-        protected override async Task<PerformanceAttributes?> ComputeValueAsync(PerformanceCacheLookup lookup, CancellationToken token = default)
-        {
-            var score = lookup.ScoreInfo;
-
-            var attributes = await difficultyCache.GetDifficultyAsync(score.BeatmapInfo!, score.Ruleset, score.Mods, token).ConfigureAwait(false);
-
-            // Performance calculation requires the beatmap and ruleset to be locally available. If not, return a default value.
-            if (attributes?.Attributes == null)
-                return null;
-
-            token.ThrowIfCancellationRequested();
-
-            return score.Ruleset.CreateInstance().CreatePerformanceCalculator()?.Calculate(score, attributes.Value.Attributes);
-        }
-
-        public readonly struct PerformanceCacheLookup
-        {
-            public readonly ScoreInfo ScoreInfo;
-
-            public PerformanceCacheLookup(ScoreInfo info)
-            {
-                ScoreInfo = info;
-            }
-
-            public override int GetHashCode()
-            {
-                var hash = new HashCode();
-
-                hash.Add(ScoreInfo.Hash);
-                hash.Add(ScoreInfo.ID);
-
-                return hash.ToHashCode();
-            }
-        }
-    }
-}

osu.Game/Screens/OnlinePlay/Playlists/PlaylistsResultsScreen.cs

@@ -41,7 +41,7 @@ namespace osu.Game.Screens.OnlinePlay.Playlists
         [Resolved]
         private RulesetStore rulesets { get; set; }
 
-        public PlaylistsResultsScreen(ScoreInfo score, long roomId, PlaylistItem playlistItem, bool allowRetry, bool allowWatchingReplay = true)
+        public PlaylistsResultsScreen([CanBeNull] ScoreInfo score, long roomId, PlaylistItem playlistItem, bool allowRetry, bool allowWatchingReplay = true)
             : base(score, allowRetry, allowWatchingReplay)
         {
             this.roomId = roomId;

osu.Game/Screens/Play/SpectatorResultsScreen.cs

@@ -28,7 +28,7 @@ namespace osu.Game.Screens.Play
 
         private void userBeganPlaying(int userId, SpectatorState state)
         {
-            if (userId == Score.UserID)
+            if (userId == Score?.UserID)
             {
                 Schedule(() =>
                 {

osu.Game/Screens/Ranking/Expanded/Statistics/PerformanceStatistic.cs

@@ -5,10 +5,11 @@
 
 using System;
 using System.Threading;
+using System.Threading.Tasks;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
-using osu.Framework.Extensions;
 using osu.Framework.Graphics;
+using osu.Game.Beatmaps;
 using osu.Game.Graphics.UserInterface;
 using osu.Game.Resources.Localisation.Web;
 using osu.Game.Scoring;
@@ -32,7 +33,7 @@ namespace osu.Game.Screens.Ranking.Expanded.Statistics
         }
 
         [BackgroundDependencyLoader]
-        private void load(ScorePerformanceCache performanceCache)
+        private void load(BeatmapDifficultyCache difficultyCache, CancellationToken? cancellationToken)
         {
             if (score.PP.HasValue)
             {
@@ -40,8 +41,19 @@ namespace osu.Game.Screens.Ranking.Expanded.Statistics
             }
             else
             {
-                performanceCache.CalculatePerformanceAsync(score, cancellationTokenSource.Token)
+                Task.Run(async () =>
-                                .ContinueWith(t => Schedule(() => setPerformanceValue(t.GetResultSafely()?.Total)), cancellationTokenSource.Token);
+                {
+                    var attributes = await difficultyCache.GetDifficultyAsync(score.BeatmapInfo!, score.Ruleset, score.Mods, cancellationToken ?? default).ConfigureAwait(false);
+                    var performanceCalculator = score.Ruleset.CreateInstance().CreatePerformanceCalculator();
+
+                    // Performance calculation requires the beatmap and ruleset to be locally available. If not, return a default value.
+                    if (attributes?.Attributes == null || performanceCalculator == null)
+                        return;
+
+                    var result = await performanceCalculator.CalculateAsync(score, attributes.Value.Attributes, cancellationToken ?? default).ConfigureAwait(false);
+
+                    Schedule(() => setPerformanceValue(result.Total));
+                }, cancellationToken ?? default);
             }
         }
 

osu.Game/Screens/Ranking/ResultsScreen.cs

@@ -6,6 +6,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using JetBrains.Annotations;
 using osu.Framework.Allocation;
 using osu.Framework.Audio;
 using osu.Framework.Audio.Sample;
@@ -45,6 +46,7 @@ namespace osu.Game.Screens.Ranking
 
         public readonly Bindable<ScoreInfo> SelectedScore = new Bindable<ScoreInfo>();
 
+        [CanBeNull]
         public readonly ScoreInfo Score;
 
         protected ScorePanelList ScorePanelList { get; private set; }
@@ -69,7 +71,7 @@ namespace osu.Game.Screens.Ranking
 
         private Sample popInSample;
 
-        protected ResultsScreen(ScoreInfo score, bool allowRetry, bool allowWatchingReplay = true)
+        protected ResultsScreen([CanBeNull] ScoreInfo score, bool allowRetry, bool allowWatchingReplay = true)
         {
             Score = score;
             this.allowRetry = allowRetry;
@@ -275,6 +277,11 @@ namespace osu.Game.Screens.Ranking
             if (base.OnExiting(e))
                 return true;
 
+            // This is a stop-gap safety against components holding references to gameplay after exiting the gameplay flow.
+            // Right now, HitEvents are only used up to the results screen. If this changes in the future we need to remove
+            // HitObject references from HitEvent.
+            Score?.HitEvents.Clear();
+
             this.FadeOut(100);
             return false;
         }

osu.Game/Screens/Ranking/SoloResultsScreen.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
@@ -45,12 +46,16 @@ namespace osu.Game.Screens.Ranking
         {
             base.LoadComplete();
 
+            Debug.Assert(Score != null);
+
             if (ShowUserStatistics)
                 statisticsSubscription = soloStatisticsWatcher.RegisterForStatisticsUpdateAfter(Score, update => statisticsUpdate.Value = update);
         }
 
         protected override StatisticsPanel CreateStatisticsPanel()
         {
+            Debug.Assert(Score != null);
+
             if (ShowUserStatistics)
             {
                 return new SoloStatisticsPanel(Score)
@@ -64,6 +69,8 @@ namespace osu.Game.Screens.Ranking
 
         protected override APIRequest? FetchScores(Action<IEnumerable<ScoreInfo>>? scoresCallback)
         {
+            Debug.Assert(Score != null);
+
             if (Score.BeatmapInfo!.OnlineID <= 0 || Score.BeatmapInfo.Status <= BeatmapOnlineStatus.Pending)
                 return null;
 

osu.Game/Screens/Ranking/Statistics/PerformanceBreakdownChart.cs

@@ -39,9 +39,6 @@ namespace osu.Game.Screens.Ranking.Statistics
 
         private readonly CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();
 
-        [Resolved]
-        private ScorePerformanceCache performanceCache { get; set; }
-
         [Resolved]
         private BeatmapDifficultyCache difficultyCache { get; set; }
 
@@ -148,7 +145,7 @@ namespace osu.Game.Screens.Ranking.Statistics
 
             spinner.Show();
 
-            new PerformanceBreakdownCalculator(playableBeatmap, difficultyCache, performanceCache)
+            new PerformanceBreakdownCalculator(playableBeatmap, difficultyCache)
                 .CalculateAsync(score, cancellationTokenSource.Token)
                 .ContinueWith(t => Schedule(() => setPerformanceValue(t.GetResultSafely())));
         }