Merge pull request #20303 from peppy/hitobject-entry-nesting-stoled

Fix editor performance drop over time due to lingering nested object references

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

@@ -103,7 +103,7 @@ namespace osu.Game.Tests.Visual.Gameplay
             AddUntilStep("hit first hitobject", () =>
             {
                 InputManager.Click(MouseButton.Left);
-                return nextObjectEntry.Result.HasResult;
+                return nextObjectEntry.Result?.HasResult == true;
             });
 
             AddAssert("check correct object after hit", () => sampleTriggerSource.GetMostValidObject() == beatmap.HitObjects[1]);

osu.Game/Rulesets/Objects/HitObjectLifetimeEntry.cs

@@ -1,8 +1,6 @@
 // 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.
 
-#nullable disable
-
 using osu.Framework.Bindables;
 using osu.Framework.Graphics.Performance;
 using osu.Game.Rulesets.Judgements;
@@ -24,7 +22,7 @@ namespace osu.Game.Rulesets.Objects
         /// The result that <see cref="HitObject"/> was judged with.
         /// This is set by the accompanying <see cref="DrawableHitObject"/>, and reused when required for rewinding.
         /// </summary>
-        internal JudgementResult Result;
+        internal JudgementResult? Result;
 
         private readonly IBindable<double> startTimeBindable = new BindableDouble();
 

osu.Game/Rulesets/Objects/Pooling/HitObjectEntryManager.cs

@@ -0,0 +1,120 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+
+namespace osu.Game.Rulesets.Objects.Pooling
+{
+    /// <summary>
+    /// Manages a mapping between <see cref="HitObject"/> and <see cref="HitObjectLifetimeEntry"/>
+    /// </summary>
+    internal class HitObjectEntryManager
+    {
+        /// <summary>
+        /// All entries, including entries of the nested hit objects.
+        /// </summary>
+        public IEnumerable<HitObjectLifetimeEntry> AllEntries => entryMap.Values;
+
+        /// <summary>
+        /// Invoked when a new <see cref="HitObjectLifetimeEntry"/> is added to this <see cref="HitObjectEntryManager"/>..
+        /// The second parameter of the event is the parent hit object.
+        /// </summary>
+        public event Action<HitObjectLifetimeEntry, HitObject?>? OnEntryAdded;
+
+        /// <summary>
+        /// Invoked when a <see cref="HitObjectLifetimeEntry"/> is removed from this <see cref="HitObjectEntryManager"/>.
+        /// The second parameter of the event is the parent hit object.
+        /// </summary>
+        public event Action<HitObjectLifetimeEntry, HitObject?>? OnEntryRemoved;
+
+        /// <summary>
+        /// Provides the reverse mapping of <see cref="HitObjectLifetimeEntry.HitObject"/> for each entry.
+        /// </summary>
+        private readonly Dictionary<HitObject, HitObjectLifetimeEntry> entryMap = new Dictionary<HitObject, HitObjectLifetimeEntry>();
+
+        /// <summary>
+        /// Stores the parent hit object for entries of the nested hit objects.
+        /// </summary>
+        /// <remarks>
+        /// The parent hit object of a pooled hit object may be non-pooled.
+        /// In that case, no corresponding <see cref="HitObjectLifetimeEntry"/> is stored in this <see cref="HitObjectEntryManager"/>.
+        /// </remarks>
+        private readonly Dictionary<HitObjectLifetimeEntry, HitObject> parentMap = new Dictionary<HitObjectLifetimeEntry, HitObject>();
+
+        /// <summary>
+        /// Stores the list of child entries for each hit object managed by this <see cref="HitObjectEntryManager"/>.
+        /// </summary>
+        private readonly Dictionary<HitObject, List<HitObjectLifetimeEntry>> childrenMap = new Dictionary<HitObject, List<HitObjectLifetimeEntry>>();
+
+        public void Add(HitObjectLifetimeEntry entry, HitObject? parent)
+        {
+            HitObject hitObject = entry.HitObject;
+
+            if (entryMap.ContainsKey(hitObject))
+                throw new InvalidOperationException($@"The {nameof(HitObjectLifetimeEntry)} is already added to this {nameof(HitObjectEntryManager)}.");
+
+            // Add the entry.
+            entryMap[hitObject] = entry;
+            childrenMap[hitObject] = new List<HitObjectLifetimeEntry>();
+
+            // If the entry has a parent, set it and add the entry to the parent's children.
+            if (parent != null)
+            {
+                parentMap[entry] = parent;
+                if (childrenMap.TryGetValue(parent, out var parentChildEntries))
+                    parentChildEntries.Add(entry);
+            }
+
+            hitObject.DefaultsApplied += onDefaultsApplied;
+            OnEntryAdded?.Invoke(entry, parent);
+        }
+
+        public void Remove(HitObjectLifetimeEntry entry)
+        {
+            HitObject hitObject = entry.HitObject;
+
+            if (!entryMap.ContainsKey(hitObject))
+                throw new InvalidOperationException($@"The {nameof(HitObjectLifetimeEntry)} is not contained in this {nameof(HitObjectEntryManager)}.");
+
+            entryMap.Remove(hitObject);
+
+            // If the entry has a parent, unset it and remove the entry from the parents' children.
+            if (parentMap.Remove(entry, out var parent) && childrenMap.TryGetValue(parent, out var parentChildEntries))
+                parentChildEntries.Remove(entry);
+
+            // Remove all the entries' children.
+            if (childrenMap.Remove(hitObject, out var childEntries))
+            {
+                foreach (var childEntry in childEntries)
+                    Remove(childEntry);
+            }
+
+            hitObject.DefaultsApplied -= onDefaultsApplied;
+            OnEntryRemoved?.Invoke(entry, parent);
+        }
+
+        public bool TryGet(HitObject hitObject, [MaybeNullWhen(false)] out HitObjectLifetimeEntry entry)
+        {
+            return entryMap.TryGetValue(hitObject, out entry);
+        }
+
+        /// <summary>
+        /// As nested hit objects are recreated, remove entries of the old nested hit objects.
+        /// </summary>
+        private void onDefaultsApplied(HitObject hitObject)
+        {
+            if (!childrenMap.Remove(hitObject, out var childEntries))
+                return;
+
+            // Remove all the entries' children. At this point the parents' (this entries') children list has been removed from the map, so this does not cause upwards traversal.
+            foreach (var entry in childEntries)
+                Remove(entry);
+
+            // The removed children list needs to be added back to the map for the entry to potentially receive children.
+            childEntries.Clear();
+            childrenMap[hitObject] = childEntries;
+        }
+    }
+}

osu.Game/Rulesets/UI/Playfield.cs

@@ -21,6 +21,7 @@ using osu.Game.Rulesets.Objects;
 using osu.Game.Rulesets.Objects.Drawables;
 using osu.Game.Skinning;
 using osuTK;
+using osu.Game.Rulesets.Objects.Pooling;
 
 namespace osu.Game.Rulesets.UI
 {
@@ -94,6 +95,8 @@ namespace osu.Game.Rulesets.UI
         [Resolved(CanBeNull = true)]
         private IReadOnlyList<Mod> mods { get; set; }
 
+        private readonly HitObjectEntryManager entryManager = new HitObjectEntryManager();
+
         /// <summary>
         /// Creates a new <see cref="Playfield"/>.
         /// </summary>
@@ -108,6 +111,9 @@ namespace osu.Game.Rulesets.UI
                 h.HitObjectUsageBegan += o => HitObjectUsageBegan?.Invoke(o);
                 h.HitObjectUsageFinished += o => HitObjectUsageFinished?.Invoke(o);
             }));
+
+            entryManager.OnEntryAdded += onEntryAdded;
+            entryManager.OnEntryRemoved += onEntryRemoved;
         }
 
         [BackgroundDependencyLoader]
@@ -171,6 +177,7 @@ namespace osu.Game.Rulesets.UI
         /// <param name="hitObject">The added <see cref="HitObject"/>.</param>
         protected virtual void OnHitObjectAdded(HitObject hitObject)
         {
+            preloadSamples(hitObject);
         }
 
         /// <summary>
@@ -264,12 +271,7 @@ namespace osu.Game.Rulesets.UI
         public virtual void Add(HitObject hitObject)
         {
             var entry = CreateLifetimeEntry(hitObject);
-            lifetimeEntryMap[entry.HitObject] = entry;
+            entryManager.Add(entry, null);
-
-            preloadSamples(hitObject);
-
-            HitObjectContainer.Add(entry);
-            OnHitObjectAdded(entry.HitObject);
         }
 
         private void preloadSamples(HitObject hitObject)
@@ -292,16 +294,31 @@ namespace osu.Game.Rulesets.UI
         /// <returns>Whether the <see cref="HitObject"/> was successfully removed.</returns>
         public virtual bool Remove(HitObject hitObject)
         {
-            if (lifetimeEntryMap.Remove(hitObject, out var entry))
+            if (entryManager.TryGet(hitObject, out var entry))
             {
-                HitObjectContainer.Remove(entry);
+                entryManager.Remove(entry);
-                OnHitObjectRemoved(hitObject);
                 return true;
             }
 
             return nestedPlayfields.Any(p => p.Remove(hitObject));
         }
 
+        private void onEntryAdded(HitObjectLifetimeEntry entry, [CanBeNull] HitObject parentHitObject)
+        {
+            if (parentHitObject != null) return;
+
+            HitObjectContainer.Add(entry);
+            OnHitObjectAdded(entry.HitObject);
+        }
+
+        private void onEntryRemoved(HitObjectLifetimeEntry entry, [CanBeNull] HitObject parentHitObject)
+        {
+            if (parentHitObject != null) return;
+
+            HitObjectContainer.Remove(entry);
+            OnHitObjectRemoved(entry.HitObject);
+        }
+
         /// <summary>
         /// Creates the <see cref="HitObjectLifetimeEntry"/> for a given <see cref="HitObject"/>.
         /// </summary>
@@ -366,8 +383,11 @@ namespace osu.Game.Rulesets.UI
                     }
                 }
 
-                if (!lifetimeEntryMap.TryGetValue(hitObject, out var entry))
+                if (!entryManager.TryGet(hitObject, out var entry))
-                    lifetimeEntryMap[hitObject] = entry = CreateLifetimeEntry(hitObject);
+                {
+                    entry = CreateLifetimeEntry(hitObject);
+                    entryManager.Add(entry, parent?.HitObject);
+                }
 
                 dho.ParentHitObject = parent;
                 dho.Apply(entry);
@@ -442,8 +462,6 @@ namespace osu.Game.Rulesets.UI
         /// </remarks>
         internal event Action<HitObject> HitObjectUsageFinished;
 
-        private readonly Dictionary<HitObject, HitObjectLifetimeEntry> lifetimeEntryMap = new Dictionary<HitObject, HitObjectLifetimeEntry>();
-
         /// <summary>
         /// Sets whether to keep a given <see cref="HitObject"/> always alive within this or any nested <see cref="Playfield"/>.
         /// </summary>
@@ -451,7 +469,7 @@ namespace osu.Game.Rulesets.UI
         /// <param name="keepAlive">Whether to keep <paramref name="hitObject"/> always alive.</param>
         internal void SetKeepAlive(HitObject hitObject, bool keepAlive)
         {
-            if (lifetimeEntryMap.TryGetValue(hitObject, out var entry))
+            if (entryManager.TryGet(hitObject, out var entry))
             {
                 entry.KeepAlive = keepAlive;
                 return;
@@ -466,7 +484,7 @@ namespace osu.Game.Rulesets.UI
         /// </summary>
         internal void KeepAllAlive()
         {
-            foreach (var (_, entry) in lifetimeEntryMap)
+            foreach (var entry in entryManager.AllEntries)
                 entry.KeepAlive = true;
 
             foreach (var p in nestedPlayfields)