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 ruleset icon creation failures to avoid whole game death #18935

Merged
merged 4 commits into from
Jun 29, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented Jun 29, 2022

Needs to be tested with a failing ruleset.

@ppy-sentryintegration
Copy link

Sentry issue: OSU-351

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 29, 2022
@ppy-sentryintegration
Copy link

Sentry issue: OSU-35P

@frenzibyte
Copy link
Member

frenzibyte commented Jun 29, 2022

The try-catch will not help because the exception is occurring from the icon's BDL, not CreateIcon():

Unhandled exception. System.ArgumentException: Destination is too short. (Parameter 'destination')
   at System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormattedSlow(String value)
   at System.Runtime.CompilerServices.DefaultInterpolatedStringHandler.AppendFormatted(String value)
   at osu.Framework.Graphics.Textures.TextureStore.get(String name, WrapMode wrapModeS, WrapMode wrapModeT)
   at osu.Framework.Graphics.Textures.TextureStore.Get(String name, WrapMode wrapModeS, WrapMode wrapModeT)
   at osu.Game.Rulesets.Solosu.SolosuRuleset.SolosuIcon.load(TextureStore textures, GameHost host)
--- End of stack trace from previous location ---

And that's going to be quite an interesting one in the general ruleset API and for #9348 (handling errors caused by ruleset component's BDLs or draw/update passes).

@peppy
Copy link
Member Author

peppy commented Jun 29, 2022

Have you tested? The call stacks on sentry show it running inline (as it should) since the parent component is run inline.

Note that there may be other cases that also need to be addressed, you may have run into one.

@frenzibyte
Copy link
Member

Hmm, I have tested and confirmed it's failing, but you're right, that shouldn't matter.

@frenzibyte
Copy link
Member

After going through the failures bit by bit, the following looks to be enough for handling such failure:

diff --git a/osu.Game/Overlays/Settings/SettingsFooter.cs b/osu.Game/Overlays/Settings/SettingsFooter.cs
index b9ac1aa53b..db0dc8fd5e 100644
--- a/osu.Game/Overlays/Settings/SettingsFooter.cs
+++ b/osu.Game/Overlays/Settings/SettingsFooter.cs
@@ -3,7 +3,6 @@
 
 #nullable disable
 
-using System.Collections.Generic;
 using osu.Framework.Allocation;
 using osu.Framework.Development;
 using osu.Framework.Graphics;
@@ -29,39 +28,17 @@ private void load(OsuGameBase game, RulesetStore rulesets)
             Direction = FillDirection.Vertical;
             Padding = new MarginPadding { Top = 20, Bottom = 30, Horizontal = SettingsPanel.CONTENT_MARGINS };
 
-            var modes = new List<Drawable>();
-
-            foreach (var ruleset in rulesets.AvailableRulesets)
-            {
-                try
-                {
-                    var icon = new ConstrainedIconContainer
-                    {
-                        Anchor = Anchor.TopCentre,
-                        Origin = Anchor.TopCentre,
-                        Icon = ruleset.CreateInstance().CreateIcon(),
-                        Colour = Color4.Gray,
-                        Size = new Vector2(20),
-                    };
-
-                    modes.Add(icon);
-                }
-                catch
-                {
-                    Logger.Log($"Could not create ruleset icon for {ruleset.Name}. Please check for an update from the developer.", level: LogLevel.Error);
-                }
-            }
+            FillFlowContainer modes;
 
             Children = new Drawable[]
             {
-                new FillFlowContainer
+                modes = new FillFlowContainer
                 {
                     Anchor = Anchor.TopCentre,
                     Origin = Anchor.TopCentre,
                     Direction = FillDirection.Full,
                     RelativeSizeAxes = Axes.X,
                     AutoSizeAxes = Axes.Y,
-                    Children = modes,
                     Spacing = new Vector2(5),
                     Padding = new MarginPadding { Bottom = 10 },
                 },
@@ -78,6 +55,27 @@ private void load(OsuGameBase game, RulesetStore rulesets)
                     Origin = Anchor.TopCentre,
                 }
             };
+
+            foreach (var ruleset in rulesets.AvailableRulesets)
+            {
+                try
+                {
+                    var icon = new ConstrainedIconContainer
+                    {
+                        Anchor = Anchor.TopCentre,
+                        Origin = Anchor.TopCentre,
+                        Icon = ruleset.CreateInstance().CreateIcon(),
+                        Colour = Color4.Gray,
+                        Size = new Vector2(20),
+                    };
+
+                    modes.Add(icon);
+                }
+                catch
+                {
+                    Logger.Log($"Could not create ruleset icon for {ruleset.Name}. Please check for an update from the developer.", level: LogLevel.Error);
+                }
+            }
         }
 
         private class BuildDisplay : OsuAnimatedButton
diff --git a/osu.Game/Screens/Menu/IntroTriangles.cs b/osu.Game/Screens/Menu/IntroTriangles.cs
index d59a07a350..dd45d66d9f 100644
--- a/osu.Game/Screens/Menu/IntroTriangles.cs
+++ b/osu.Game/Screens/Menu/IntroTriangles.cs
@@ -14,6 +14,7 @@
 using osu.Framework.Graphics.Containers;
 using osu.Framework.Graphics.Shapes;
 using osu.Framework.Graphics.Textures;
+using osu.Framework.Logging;
 using osu.Framework.Utils;
 using osu.Framework.Timing;
 using osu.Game.Graphics;
@@ -340,24 +341,28 @@ private class RulesetFlow : FillFlowContainer
                 [BackgroundDependencyLoader]
                 private void load(RulesetStore rulesets)
                 {
-                    var modes = new List<Drawable>();
+                    AutoSizeAxes = Axes.Both;
+
+                    Anchor = Anchor.Centre;
+                    Origin = Anchor.Centre;
 
                     foreach (var ruleset in rulesets.AvailableRulesets)
                     {
-                        var icon = new ConstrainedIconContainer
+                        try
                         {
-                            Icon = ruleset.CreateInstance().CreateIcon(),
-                            Size = new Vector2(30),
-                        };
+                            var icon = new ConstrainedIconContainer
+                            {
+                                Icon = ruleset.CreateInstance().CreateIcon(),
+                                Size = new Vector2(30),
+                            };
 
-                        modes.Add(icon);
+                            Add(icon);
+                        }
+                        catch
+                        {
+                            Logger.Log($"Could not create ruleset icon for {ruleset.Name}. Please check for an update from the developer.", level: LogLevel.Error);
+                        }
                     }
-
-                    AutoSizeAxes = Axes.Both;
-                    Children = modes;
-
-                    Anchor = Anchor.Centre;
-                    Origin = Anchor.Centre;
                 }
             }

This includes a reordering in SettingsFooter to ensure the icon is loaded inside of the try-catch, and another try-catch surrounding the ruleset icon display in "triangles" intro.

Here's the ruleset I checked this with, if you wish to test.

@peppy
Copy link
Member Author

peppy commented Jun 29, 2022

Looks good, feel free to commit.

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