Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard against potential usage of DrawablePool without being added to hierarchy #6136

Merged
merged 3 commits into from Jan 15, 2024

Conversation

peppy
Copy link
Sponsor Member

@peppy peppy commented Jan 15, 2024

This has caught me twice to date. It still works roughly correctly, but won't initialise the pool to the correct default size, taking away the startup performance benefits of pooling.

Breaking changes

DrawablePool.Get() wil now throw if the DrawablePool did not begin load

Previously the pool would return drawables correctly but it would not initialise the pool to correct default size, causing overheads on drawable retrieval (usually on update thread).

@bdach
Copy link
Collaborator

bdach commented Jan 15, 2024

Changes to apply game-side after this (aside from the 2 pulls linked above):

diff --git a/osu.Game.Rulesets.Mania.Tests/Skinning/TestSceneBarLine.cs b/osu.Game.Rulesets.Mania.Tests/Skinning/TestSceneBarLine.cs
index ab9f57ecc3..8fd0327065 100644
--- a/osu.Game.Rulesets.Mania.Tests/Skinning/TestSceneBarLine.cs
+++ b/osu.Game.Rulesets.Mania.Tests/Skinning/TestSceneBarLine.cs
@@ -26,27 +26,32 @@ private void recreate(Func<IEnumerable<BarLine>>? createBarLines = null)
                 new StageDefinition(4),
             };
 
-            SetContents(_ => new ManiaPlayfield(stageDefinitions).With(s =>
+            SetContents(_ =>
             {
-                if (createBarLines != null)
+                var playfield = new ManiaPlayfield(stageDefinitions);
+                LoadComponent(playfield);
+                return playfield.With(s =>
                 {
-                    var barLines = createBarLines();
+                    if (createBarLines != null)
+                    {
+                        var barLines = createBarLines();
 
-                    foreach (var b in barLines)
-                        s.Add(b);
+                        foreach (var b in barLines)
+                            s.Add(b);
 
-                    return;
-                }
+                        return;
+                    }
 
-                for (int i = 0; i < 64; i++)
-                {
-                    s.Add(new BarLine
+                    for (int i = 0; i < 64; i++)
                     {
-                        StartTime = Time.Current + i * 500,
-                        Major = i % 4 == 0,
-                    });
-                }
-            }));
+                        s.Add(new BarLine
+                        {
+                            StartTime = Time.Current + i * 500,
+                            Major = i % 4 == 0,
+                        });
+                    }
+                });
+            });
         }
     }
 }
diff --git a/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs b/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs
index 874130233a..122d23c77f 100644
--- a/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs
+++ b/osu.Game.Rulesets.Osu.Tests/TestSceneDrawableJudgement.cs
@@ -69,7 +69,10 @@ private void showResult(HitResult result)
                     DrawablePool<TestDrawableOsuJudgement> pool;
 
                     if (poolIndex >= pools.Count)
+                    {
                         pools.Add(pool = new DrawablePool<TestDrawableOsuJudgement>(1));
+                        LoadComponent(pool);
+                    }
                     else
                     {
                         pool = pools[poolIndex];

@bdach bdach merged commit 734252f into ppy:master Jan 15, 2024
19 of 21 checks passed
@peppy peppy deleted the dpguard branch January 17, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants